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
