From: Yicong Yang <[email protected]>
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 <[email protected]>
---
drivers/perf/arm_smmuv3_pmu.c | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/drivers/perf/arm_smmuv3_pmu.c b/drivers/perf/arm_smmuv3_pmu.c
index 25a269d431e4..f27c5f585524 100644
--- a/drivers/perf/arm_smmuv3_pmu.c
+++ b/drivers/perf/arm_smmuv3_pmu.c
@@ -946,7 +946,14 @@ static const struct of_device_id smmu_pmu_of_match[] = {
MODULE_DEVICE_TABLE(of, smmu_pmu_of_match);
#endif
+static const struct platform_device_id smmu_pmu_platform_match[] = {
+ { "arm-smmu-v3-pmcg", 0 },
+ {}
+};
+MODULE_DEVICE_TABLE(platform, smmu_pmu_platform_match);
+
static struct platform_driver smmu_pmu_driver = {
+ .id_table = smmu_pmu_platform_match,
.driver = {
.name = "arm-smmu-v3-pmcg",
.of_match_table = of_match_ptr(smmu_pmu_of_match),
--
2.24.0
On 2023-08-07 20:22, Yicong Yang <[email protected]> wrote:
> From: Yicong Yang <[email protected]>
>
> 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 <[email protected]> wrote:
>
> On 2023-08-07 20:22, Yicong Yang <[email protected]> wrote:
> > From: Yicong Yang <[email protected]>
> >
> > 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 <[email protected]> wrote:
> Hi Barry, Liang,
>
> On 2023/8/9 13:47, Barry Song wrote:
> > On Wed, Aug 9, 2023 at 1:01 PM Liang Li <[email protected]> wrote:
> >>
> >> On 2023-08-07 20:22, Yicong Yang <[email protected]> wrote:
> >>> From: Yicong Yang <[email protected]>
> >>>
> >>> 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 Sat, Aug 12, 2023 at 1:24 PM Barry Song <[email protected]> wrote:
>
> On Fri, Aug 11, 2023 at 6:14 PM Yicong Yang <[email protected]> wrote:
> >
> > On 2023/8/9 20:11, Liang Li wrote:
> > > Hi Yicong,
> > >
> > > Thanks for your reply,
> > >
> > > On 2023-08-09 14:31, Yicong Yang <[email protected]> wrote:
> > >> Hi Barry, Liang,
> > >>
> > >> On 2023/8/9 13:47, Barry Song wrote:
> > >>> On Wed, Aug 9, 2023 at 1:01 PM Liang Li <[email protected]> wrote:
> > >>>>
> > >>>> On 2023-08-07 20:22, Yicong Yang <[email protected]> wrote:
> > >>>>> From: Yicong Yang <[email protected]>
> > >>>>>
> > >>>>> 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 <[email protected]>");
> > MODULE_AUTHOR("Shameer Kolothum <[email protected]>");
> >
>
> Thanks
> Barry
On Fri, Aug 11, 2023 at 6:14 PM Yicong Yang <[email protected]> wrote:
>
> On 2023/8/9 20:11, Liang Li wrote:
> > Hi Yicong,
> >
> > Thanks for your reply,
> >
> > On 2023-08-09 14:31, Yicong Yang <[email protected]> wrote:
> >> Hi Barry, Liang,
> >>
> >> On 2023/8/9 13:47, Barry Song wrote:
> >>> On Wed, Aug 9, 2023 at 1:01 PM Liang Li <[email protected]> wrote:
> >>>>
> >>>> On 2023-08-07 20:22, Yicong Yang <[email protected]> wrote:
> >>>>> From: Yicong Yang <[email protected]>
> >>>>>
> >>>>> 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 <[email protected]>");
> MODULE_AUTHOR("Shameer Kolothum <[email protected]>");
>
Thanks
Barry