Re: [PATCH] perf/smmuv3: Add platform id table for module auto loading

[Yicong Yang <yangyicong@huawei.com>]

On 2023/8/12 13:31, Barry Song wrote:
> On Sat, Aug 12, 2023 at 1:24 PM Barry Song <21cnbao@gmail.com> wrote:
>>
>> On Fri, Aug 11, 2023 at 6:14 PM Yicong Yang <yangyicong@huawei.com> wrote:
>>>
>>> On 2023/8/9 20:11, Liang Li wrote:
>>>> Hi Yicong,
>>>>
>>>> Thanks for your reply,
>>>>
>>>> On 2023-08-09 14:31, Yicong Yang <yangyicong@huawei.com> wrote:
>>>>> Hi Barry, Liang,
>>>>>
>>>>> On 2023/8/9 13:47, Barry Song wrote:
>>>>>> On Wed, Aug 9, 2023 at 1:01 PM Liang Li <liliang6@email.cn> wrote:
>>>>>>>
>>>>>>> On 2023-08-07 20:22, Yicong Yang <yangyicong@huawei.com> wrote:
>>>>>>>> From: Yicong Yang <yangyicong@hisilicon.com>
>>>>>>>>
>>>>>>>> On ACPI based system the device is probed by the name directly. If the
>>>>>>>> driver is configured as module it can only be loaded manually. Add the
>>>>>>>> platform id table as well as the module alias then the driver will be
>>>>>>>> loaded automatically by the udev or others once the device added.
>>>>>>>>
>>>>>>>
>>>>>>> Please consider revise the long log to clearly express the purpose of the
>>>>>>> changes in this patch:
>>>>>>>
>>>>>>> - What's the exact issue the patch is addressing
>>>>>>> - Why the changes in this patch can fix the issue or make something working
>>>>>>> - Consider impact of the changes introduced by this patch
>>>>>>>
>>>>>>> These info may help reviewers and maintainers .. and yourself on code merge.
>>>>>>
>>>>>> years ago, i found a good doc regarding this,
>>>>>> https://wiki.archlinux.org/title/Modalias
>>>>>>
>>>>>> guess it is because /lib/modules/$(uname -r)/modules.alias fails to contain smmu
>>>>>> driver without the MODULE_DEVICE_TABLE, isn't it, yicong？
>>>>>
>>>>> Yes I think it's the reason. I didn't find summary in kernel docs for the modalias
>>>>> as well as the uevent mechanism. Arch wiki has a well illustration for the modalias
>>>>> and suse[1] describes how this is used by the udev for module auto loading.
>>>>>
>>>>> For my case I'm using a ACPI based arm64 server and after booting the arm_smmuv3_pmu.ko
>>>>> is not auto loaded by the udevd since we aren't providing this information. In order
>>>>> to support this we need to provide this MODULE_DEVICE_TABLE() when the smmu pmu added
>>>>> as a platform device, then the userspace udev can know which module to load after the
>>>>> device is added.
>>>>>
>>>>
>>>> Then what's the purpose of the added '.id_table = ...' line in the previous
>>>> patch ?
>>>> <We lost the patch context in this thread.>
>>>>
>>>> Based on above clarification, the updated DEVICE_TABLE would update modalias
>>>> as expected, right ?
>>>
>>> ok, it's lack of illustration in the commit. If we're going to use MODULE_DEVICE_TABLE we need
>>> a platform id table. So I add it and I found it weired if we have a id table but not use it for
>>> probing, so I also initialize .id_table.
>>>
>>> I found there's also an another way to implement this by used MODULE_ALIAS(), and no need to add
>>> an id table. Maybe this way is less controversial.
>>
>> right, how is your driver matching with your device?
>>
>> acpi_driver_match_device()  or strcmp(pdev->name, drv->name) == 0 ?
> 

The latter. The smmu pmu platform device is created when parsing the IORT so probed by matching the
device and driver name directly.

> btw, those drivers supporting acpi probe usually have a acpi table
> 
> #if IS_ENABLED(CONFIG_ACPI)
> static const struct acpi_device_id hidma_acpi_ids[] = {
> {"QCOM8061"},
> {"QCOM8062", HIDMA_MSI_CAP},
> {"QCOM8063", (HIDMA_MSI_CAP | HIDMA_IDENTITY_CAP)},
> {},
> };
> MODULE_DEVICE_TABLE(acpi, hidma_acpi_ids);
> #endif
> 
>>
>> static int platform_match(struct device *dev, struct device_driver *drv)
>> {
>>         struct platform_device *pdev = to_platform_device(dev);
>>         struct platform_driver *pdrv = to_platform_driver(drv);
>>
>>         /* When driver_override is set, only bind to the matching driver */
>>         if (pdev->driver_override)
>>                 return !strcmp(pdev->driver_override, drv->name);
>>
>>         /* Attempt an OF style match first */
>>         if (of_driver_match_device(dev, drv))
>>                 return 1;
>>
>>         /* Then try ACPI style match */
>>         if (acpi_driver_match_device(dev, drv))
>>                 return 1;
>>
>>         /* Then try to match against the id table */
>>         if (pdrv->id_table)
>>                 return platform_match_id(pdrv->id_table, pdev) != NULL;
>>
>>         /* fall-back to driver name match */
>>         return (strcmp(pdev->name, drv->name) == 0);
>> }
>>
>> In both cases, it seems we don't need id_table. id_table can support
>> two or above device names, so that one driver can support
>> multiple device IDs usually with different specific device data.
>>

yes I think you're right. For my case use MODULE_ALIAS is enough. Will respin
a v2.

Thanks!

>>>
>>> diff --git a/drivers/perf/arm_smmuv3_pmu.c b/drivers/perf/arm_smmuv3_pmu.c
>>> index 25a269d431e4..4c32b6dbfe76 100644
>>> --- a/drivers/perf/arm_smmuv3_pmu.c
>>> +++ b/drivers/perf/arm_smmuv3_pmu.c
>>> @@ -984,6 +984,7 @@ static void __exit arm_smmu_pmu_exit(void)
>>>
>>>  module_exit(arm_smmu_pmu_exit);
>>>
>>> +MODULE_ALIAS("platform:arm-smmu-v3-pmcg");
>>>  MODULE_DESCRIPTION("PMU driver for ARM SMMUv3 Performance Monitors Extension");
>>>  MODULE_AUTHOR("Neil Leeder <nleeder@codeaurora.org>");
>>>  MODULE_AUTHOR("Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>");
>>>
>>
>> Thanks
>> Barry
> .
>