linux-kernel - Re: [PATCH] PCI: hotplug: Add extension driver for Ampere Altra hotplug LED control

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: <86a5t4r03g.fsf@scott-ph-mail.amperecomputing.com>
Date:   Fri, 29 Sep 2023 17:11:47 -0700
From:   D Scott Phillips <scott@...amperecomputing.com>
To:     Bjorn Helgaas <helgaas@...nel.org>,
        "Rafael J. Wysocki" <rafael@...nel.org>
Cc:     linux-acpi@...r.kernel.org, linux-pci@...r.kernel.org,
        Bjorn Helgaas <bhelgaas@...gle.com>,
        Len Brown <lenb@...nel.org>, linux-kernel@...r.kernel.org,
        Darren Hart <darren@...amperecomputing.com>,
        patches@...erecomputing.com
Subject: Re: [PATCH] PCI: hotplug: Add extension driver for Ampere Altra
 hotplug LED control

Bjorn Helgaas <helgaas@...nel.org> writes:

> On Fri, Sep 29, 2023 at 09:06:02PM +0200, Rafael J. Wysocki wrote:
>> On Thu, Sep 28, 2023 at 5:47 PM Bjorn Helgaas <helgaas@...nel.org> wrote:
>> > On Wed, Sep 27, 2023 at 01:23:47PM -0700, D Scott Phillips wrote:
>> > > On Ampere Altra, PCIe hotplug is handled through ACPI. A side interface is
>> > > also present to request system firmware control of attention LEDs. Add an
>> > > ACPI PCI Hotplug companion driver to support attention LED control.
>
>> > > +static int __init acpiphp_ampere_altra_init(void)
>> > > +{
>> > > +     struct fwnode_handle *fwnode;
>> > > +     acpi_handle leds_handle = NULL;
>> > > +     struct acpi_device *leds;
>> > > +     acpi_status status;
>> > > +     int ret;
>> > > +
>> > > +     status = acpi_get_devices("AMPC0008", get_acpi_handle, NULL,
>> > > +                               &leds_handle);
>> >
>> > Rafael, can you comment on whether we should use acpi_get_devices(),
>> > acpi_bus_register_driver(), acpi_acpi_scan_add_handler(), or something
>> > else here?
>> 
>> Personally, I would go for a platform driver, because the ACPI core
>> should create a platform device for this object.
>> 
>> acpi_get_devices() carries out a namespace walk that is costly and
>> entirely avoidable.
>> 
>> >  I try to avoid pci_get_device() because it subverts the
>> > driver model (no hotplug support, no driver/device binding).
>> >
>> > I see Documentation/driver-api/acpi/scan_handlers.rst, but I'm not
>> > clear on when to use acpi_bus_register_driver() vs
>> 
>> Never.
>> 
>> > acpi_acpi_scan_add_handler().
>> 
>> When you don't want the ACPI core to create a platform device for your
>> ACPI device object.  There are cases like that, but they are rare.
>
> Ah, so none of the above (not acpi_get_devices(),
> acpi_bus_register_driver(), OR acpi_acpi_scan_add_handler()).
>
> IIUC, what you propose would look something like this:
>
>   static u32 led_service_id[4];
>
>   static int altra_led_probe(struct platform_device *pdev)
>   {
>     struct fwnode_handle *fwnode = dev_fwnode(&pdev->dev);
>
>     fwnode_property_read_u32_array(fwnode, "uuid", led_service_id, 4);
>   }
>
>   static const struct acpi_device_id altra_led_ids[] = {
>     {"AMPC0008", 0}, {}
>   };
>   MODULE_DEVICE_TABLE(acpi, altra_led_ids);
>
>   static struct platform_driver altra_led_driver = {
>     .driver = {
>       .acpi_match_table = altra_led_ids,
>     },
>     .probe = altra_led_probe,
>   };
>   module_platform_driver(altra_led_driver);

Ok, thanks Bjorn & Rafael, will do.