Re: [RFC PATCH 02/14] drm/qaic: Add uapi and core driver file

[Jeffrey Hugo]

On 8/16/2022 12:00 PM, Krzysztof Kozlowski wrote:
> On 16/08/2022 20:47, Jeffrey Hugo wrote:
> > > > +static int qaic_pci_probe(struct pci_dev *pdev,
> > > > +			  const struct pci_device_id *id)
> > > > +{
> > > > +	int ret;
> > > > +	int i;
> > > > +	int mhi_irq;
> > > > +	struct qaic_device *qdev;
> > > > +
> > > > +	qdev = kzalloc(sizeof(*qdev), GFP_KERNEL);
> > > > +	if (!qdev) {
> > >
> > > return -ENOMEM;
> >
> > So, no centralized exit per the coding style?  Ok.
>
> Centralized exit except for cases where it is simply return. >
> > > > +		ret = -ENOMEM;
> > > > +		goto qdev_fail;
> > > > +	}
> > > > +
> > > > +	if (id->device == PCI_DEV_AIC100) {
> > > > +		qdev->num_dbc = 16;
> > > > +		qdev->dbc = kcalloc(qdev->num_dbc, sizeof(*qdev->dbc),
> > > > +				    GFP_KERNEL);
> > >
> > > Why not devm?
> >
> > We were having issues with devm and the PCI stuff.  Looking at this
> > again, I think we can apply that here.
> >
> > > > +		if (!qdev->dbc) {
> > > > +			ret = -ENOMEM;
> > > > +			goto device_id_fail;
> > > > +		}
> > > > +	} else {
> > > > +		pci_dbg(pdev, "%s: No matching device found for device id %d\n",
> > > > +			__func__, id->device);
> > >
> > > How this can happen?
> >
> > Badly functioning hardware.  That hardware has been removed from
> > circulation.  Given that this is an init path and not performance
> > critical, having this handle the scenario in a sane way instead of
> > having the driver blow up in a weird way much later on makes me feel better.
>
> How badly functioning hardware can bind and then report some different
> ID? If it reports different ID, it cannot bind...

It was one of those issues that was painful enough that I still remember 
it occurring, but long ago enough that I don't remember the specifics.

I don't think I'll be able to recreate the issue to re-debug it, so I'll 
just remove this.

> > > > +static int __init qaic_init(void)
> > > > +{
> > > > +	int ret;
> > > > +
> > > > +	if (datapath_polling) {
> > > > +		poll_datapath = true;
> > > > +		pr_info("qaic: driver initializing in datapath polling mode\n");
> > >
> > > No pr() in normal path of init/exit.
> >
> > This is not the normal path.  datapath_polling is a module parameter.
> > This is something the user can enable, and so it would be good to have
> > the user informed that the option took effect.
>
> No, not really. User always can check via sysfs and there is no point in
> polluting dmesg on module load. It might be printed (if someone has such
> modprobe setting) on every system, even without the actual device.

So, I guess this is limited to platform devices?
I see GIC, IOMMU, and PCI doing the same
I guess, will remove.

> > > > +	}
> > > > +
> > > > +	qaic_logging_register();
> > > > +
> > > > +	ret = mhi_driver_register(&qaic_mhi_driver);
> > > > +	if (ret) {
> > > > +		pr_debug("qaic: mhi_driver_register failed %d\n", ret);
> > > > +		goto free_class;
> > > > +	}
> > > > +
> > > > +	ret = pci_register_driver(&qaic_pci_driver);
> > > > +
> > >
> > > No need for such blank lines.
> >
> > Agreed.
> >
> > > > +	if (ret) {
> > > > +		pr_debug("qaic: pci_register_driver failed %d\n", ret);
> > > > +		goto free_mhi;
> > > > +	}
> > > > +
> > > > +	qaic_telemetry_register();
> > > > +	qaic_ras_register();
> > > > +	qaic_ssr_register();
> > > > +	goto out;
> > >
> > > return 0.
> >
> > Ok.
> >
> > > > +
> > > > +free_mhi:
> > > > +	mhi_driver_unregister(&qaic_mhi_driver);
> > > > +free_class:
> > > > +out:
> > > > +	if (ret)
> > > > +		qaic_logging_unregister();
> > > > +
> > > > +	return ret;
> > > > +}
> > > > +
> > > > +static void __exit qaic_exit(void)
> > > > +{
> > > > +	pr_debug("qaic: exit\n");
> > >
> > > No pr() in normal path of init/exit.
> >
> > Sure.
> >
> > > > +	link_up = true;
> > >
> > > This is confusing...
> >
> > Will add a comment.  This ties into MHI, and how it can tear down in two
> > different ways, usually based on the link state.
>
> Shouldn't this be link_up=false?

No.  module_exit() will be triggered on rmmod.  exit() will unregister 
the driver, which will cause qaic_pci_remove() to be called.  remove() 
calls qaic_mhi_free_controller() which uses the link state.

However, a hotplug event will also trigger qaic_pci_remove().

rmmod - link is up
hotplug - link is down

> > In this case, we are doing a clean tear down where the link is still up,
> > and so we should have MHI do the extra tear down that leaves the device
> > in a good state, in the event the driver gets added again.
> >
> > >
>
>
>
> Best regards,
> Krzysztof