Messages in this thread | | | Date | Tue, 16 Aug 2022 12:22:54 -0600 | Subject | Re: [RFC PATCH 02/14] drm/qaic: Add uapi and core driver file | From | 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
| |