perf/smmuv3: Add platform id table for module auto loading

  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.

Signed-off-by: Yicong Yang <yangyicong@hisilicon.com>
---
 drivers/perf/arm_smmuv3_pmu.c | 7 +++++++
 1 file changed, 7 insertions(+)
  

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.

Regards.
  

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？

>
> Regards.
>

Thanks
Barry
  

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 ?

> [1] https://documentation.suse.com/sles/15-SP1/html/SLES-all/cha-udev.html#sec-udev-drivers
> 
> Thanks.

Regards.
Liang Li
  

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 ?

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.

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

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 ?

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