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

[Krzysztof Kozlowski]

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...

(...)

>>> +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.

> 
>>
>>> +	}
>>> +
>>> +	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?

> 
> 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