lkml.org 
[lkml]   [2022]   [Aug]   [16]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [RFC PATCH 02/14] drm/qaic: Add uapi and core driver file
From
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

\
 
 \ /
  Last update: 2022-08-16 20:23    [W:0.055 / U:0.708 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site