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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAGsJ_4wivZ186D9JgyL-46zRuBDboST+c-ODx0FEbXdy5Tt3NA@mail.gmail.com>
Date:   Sat, 12 Aug 2023 13:24:45 +0800
From:   Barry Song <21cnbao@...il.com>
To:     Yicong Yang <yangyicong@...wei.com>
Cc:     Liang Li <liliang6@...il.cn>, yangyicong@...ilicon.com,
        will@...nel.org, mark.rutland@....com,
        linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
        jonathan.cameron@...wei.com, linuxarm@...wei.com
Subject: Re: [PATCH] perf/smmuv3: Add platform id table for module auto loading

On Fri, Aug 11, 2023 at 6:14 PM Yicong Yang <yangyicong@...wei.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@...wei.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@...il.cn> wrote:
> >>>>
> >>>> On 2023-08-07 20:22, Yicong Yang <yangyicong@...wei.com> wrote:
> >>>>> From: Yicong Yang <yangyicong@...ilicon.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@...eaurora.org>");
>  MODULE_AUTHOR("Shameer Kolothum <shameerali.kolothum.thodi@...wei.com>");
>

Thanks
Barry