linux-kernel - Re: [PATCH v1 11/12] ACPI: platform: Add macro for acpi platform driver

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] [day] [month] [year] [list]
Message-ID: <CAF3aWvGt69Lo1xKLBb8_DVu-+RYe9Lq4BnAm4T68VZ-5GsAorw@mail.gmail.com>
Date: Mon, 15 Sep 2025 19:14:37 +0200
From: Sławomir Rosek <srosek@...gle.com>
To: Hans de Goede <hansg@...nel.org>
Cc: "Rafael J . Wysocki" <rafael@...nel.org>, Alex Hung <alexhung@...il.com>, 
	Ilpo Jarvinen <ilpo.jarvinen@...ux.intel.com>, AceLan Kao <acelan.kao@...onical.com>, 
	Daniel Lezcano <daniel.lezcano@...aro.org>, Greg Kroah-Hartman <gregkh@...uxfoundation.org>, 
	Zhang Rui <rui.zhang@...el.com>, 
	Srinivas Pandruvada <srinivas.pandruvada@...ux.intel.com>, Tomasz Nowicki <tnowicki@...gle.com>, 
	Stanislaw Kardach <skardach@...gle.com>, Michal Krawczyk <mikrawczyk@...gle.com>, 
	linux-kernel@...r.kernel.org, linux-acpi@...r.kernel.org, 
	platform-driver-x86@...r.kernel.org, linux-pm@...r.kernel.org
Subject: Re: [PATCH v1 11/12] ACPI: platform: Add macro for acpi platform driver

Hi Hans,

First of all I would like to apologize for the late reply and thank
you for your comments.

On Sat, Aug 30, 2025 at 5:13 PM Hans de Goede <hansg@...nel.org> wrote:
>
> Hi Slawomir,
>
> On 30-Aug-25 7:34 AM, Slawomir Rosek wrote:
> > Introduce module_acpi_platform_driver() macro to simplify dynamic
> > enumeration of ACPI device objects on the platform bus by loadable
> > modules. Move common code from the intel-hid and intel-vbtn drivers
> > to the ACPI platform core.
> >
> > Signed-off-by: Slawomir Rosek <srosek@...gle.com>
>
> Thank you for your interesting patch.
>
> > ---
> >  drivers/acpi/acpi_platform.c      | 27 ++++++++++++++++++++
> >  drivers/platform/x86/intel/hid.c  | 41 +------------------------------
> >  drivers/platform/x86/intel/vbtn.c | 30 +---------------------
> >  include/linux/platform_device.h   | 17 +++++++++++++
> >  4 files changed, 46 insertions(+), 69 deletions(-)
> >
> > diff --git a/drivers/acpi/acpi_platform.c b/drivers/acpi/acpi_platform.c
> > index 48d15dd785f6..adf32ffa6be6 100644
> > --- a/drivers/acpi/acpi_platform.c
> > +++ b/drivers/acpi/acpi_platform.c
> > @@ -190,6 +190,33 @@ struct platform_device *acpi_create_platform_device(struct acpi_device *adev,
> >  }
> >  EXPORT_SYMBOL_GPL(acpi_create_platform_device);
> >
> > +static acpi_status
> > +__acpi_platform_driver_register_cb(acpi_handle handle, u32 lvl,
> > +                             void *context, void **rv)
> > +{
> > +     const struct acpi_device_id *ids = context;
> > +     struct acpi_device *dev = acpi_fetch_acpi_dev(handle);
> > +
> > +     if (dev && acpi_match_device_ids(dev, ids) == 0)
> > +             if (!IS_ERR_OR_NULL(acpi_create_platform_device(dev, NULL))) {
> > +                     dev_info(&dev->dev,
> > +                              "created platform device\n");
> > +             }
> > +
> > +     return AE_OK;
> > +}
> > +
> > +int __acpi_platform_driver_register(struct platform_driver *drv,
> > +                             struct module *owner)
> > +{
> > +     acpi_walk_namespace(ACPI_TYPE_DEVICE, ACPI_ROOT_OBJECT, ACPI_UINT32_MAX,
> > +                         __acpi_platform_driver_register_cb, NULL,
> > +                         (void *)drv->driver.acpi_match_table, NULL);
> > +
> > +     return __platform_driver_register(drv, owner);
> > +}
> > +EXPORT_SYMBOL_GPL(__acpi_platform_driver_register);
> > +
> >  void __init acpi_platform_init(void)
> >  {
> >       acpi_reconfig_notifier_register(&acpi_platform_notifier);
> > diff --git a/drivers/platform/x86/intel/hid.c b/drivers/platform/x86/intel/hid.c
> > index f25a427cccda..e2e0fc95e177 100644
> > --- a/drivers/platform/x86/intel/hid.c
> > +++ b/drivers/platform/x86/intel/hid.c
> > @@ -766,43 +766,4 @@ static struct platform_driver intel_hid_pl_driver = {
> >       .remove = intel_hid_remove,
> >  };
> >
> > -/*
> > - * Unfortunately, some laptops provide a _HID="INT33D5" device with
> > - * _CID="PNP0C02".  This causes the pnpacpi scan driver to claim the
> > - * ACPI node, so no platform device will be created.  The pnpacpi
> > - * driver rejects this device in subsequent processing, so no physical
> > - * node is created at all.
> > - *
> > - * As a workaround until the ACPI core figures out how to handle
> > - * this corner case, manually ask the ACPI platform device code to
> > - * claim the ACPI node.
> > - */
>
> This comment contains useful info, please preserve the comment changing
> the last paragraph to:
>
>  * As a workaround until the ACPI core figures out how to handle
>  * this corner case, manually ask the ACPI platform device code to
>  * claim the ACPI node by using module_acpi_platform_driver()
>  * instead of the regular module_platform_driver().
>

Good point. I shouldn't have deleted the original comment. I will restore and
rephrase it according to your suggestions.

> > -static acpi_status __init
> > -check_acpi_dev(acpi_handle handle, u32 lvl, void *context, void **rv)
> > -{
> > -     const struct acpi_device_id *ids = context;
> > -     struct acpi_device *dev = acpi_fetch_acpi_dev(handle);
> > -
> > -     if (dev && acpi_match_device_ids(dev, ids) == 0)
> > -             if (!IS_ERR_OR_NULL(acpi_create_platform_device(dev, NULL)))
> > -                     dev_info(&dev->dev,
> > -                              "intel-hid: created platform device\n");
> > -
> > -     return AE_OK;
> > -}
> > -
> > -static int __init intel_hid_init(void)
> > -{
> > -     acpi_walk_namespace(ACPI_TYPE_DEVICE, ACPI_ROOT_OBJECT,
> > -                         ACPI_UINT32_MAX, check_acpi_dev, NULL,
> > -                         (void *)intel_hid_ids, NULL);
> > -
> > -     return platform_driver_register(&intel_hid_pl_driver);
> > -}
> > -module_init(intel_hid_init);
> > -
> > -static void __exit intel_hid_exit(void)
> > -{
> > -     platform_driver_unregister(&intel_hid_pl_driver);
> > -}
> > -module_exit(intel_hid_exit);
> > +module_acpi_platform_driver(intel_hid_pl_driver);
> > diff --git a/drivers/platform/x86/intel/vbtn.c b/drivers/platform/x86/intel/vbtn.c
> > index 232cd12e3c9f..42932479de35 100644
> > --- a/drivers/platform/x86/intel/vbtn.c
> > +++ b/drivers/platform/x86/intel/vbtn.c
>
> ...
>
> > -static int __init intel_vbtn_init(void)
> > -{
> > -     acpi_walk_namespace(ACPI_TYPE_DEVICE, ACPI_ROOT_OBJECT,
> > -                         ACPI_UINT32_MAX, check_acpi_dev, NULL,
> > -                         (void *)intel_vbtn_ids, NULL);
>
> Too bad there is no comment here. I wonder if this is necessary
> at all, or if this was just copy & pasted from the intel/hid.c
> driver.
>
> git blame is not really helpful here, the acpi_walk_namespace()
> was added in 332e081225fc2 ("intel-vbtn: new driver for Intel Virtual
> Button").
>
> So it looks like this is just copy paste and maybe a regular
> module_platform_driver() will be sufficient here. But changing
> behavior like that is out of scope for this patch-set, so please
> keep using module_acpi_platform_driver()
>

Changing vbtn driver behaviour is definitely out of the scope for this
patch-set but you may be right about copy-paste. Should I update
the commit message and mention about it?

> Otherwise this looks good to me.
>
> Regards,
>
> Hans
>
>

Thanks, Regards,
Slawek