linux-kernel - Re: [PATCH v1 4/6] ACPI: acpi_video: Replace acpi_driver with platform

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: <b9319272-5e29-4de6-8921-fdac474e421f@redhat.com>
Date:   Sat, 2 Dec 2023 14:20:40 +0100
From:   Hans de Goede <hdegoede@...hat.com>
To:     "Rafael J. Wysocki" <rafael@...nel.org>,
        Michal Wilczynski <michal.wilczynski@...el.com>
Cc:     linux-acpi@...r.kernel.org, linux-kernel@...r.kernel.org,
        rafael.j.wysocki@...el.com, andriy.shevchenko@...ux.intel.com,
        lenb@...nel.org
Subject: Re: [PATCH v1 4/6] ACPI: acpi_video: Replace acpi_driver with
 platform_driver

Hi,

On 11/29/23 15:19, Rafael J. Wysocki wrote:
> Hi Hans,
> 
> On Wed, Oct 25, 2023 at 2:35 PM Michal Wilczynski
> <michal.wilczynski@...el.com> wrote:
>>
>> The acpi_video driver uses struct acpi_driver to register itself while it
>> would be more logically consistent to use struct platform_driver for this
>> purpose, because the corresponding platform device is present and the
>> role of struct acpi_device is to amend the other bus types. ACPI devices
>> are not meant to be used as proper representation of hardware entities,
>> but to collect information on those hardware entities provided by the
>> platform firmware.
>>
>> Use struct platform_driver for registering the acpi_video driver.
>>
>> Suggested-by: Rafael J. Wysocki <rafael.j.wysocki@...el.com>
>> Signed-off-by: Michal Wilczynski <michal.wilczynski@...el.com>
> 
> Do you have any particular concerns regarding this change?  For
> example, are there any setups that can break because of it?

I have just given this a quick test spin and on most hw
it actually causes the apci_video driver to not bind
anymore *at all* which will cause a bunch of brokenness
all over the place.

The problem is that the physical-node for which the 
/sys/bus/acpi/devices/LNXVIDEO:00 fwnode / acpi-companion node 
is the companion normally is the GPU, which is a PCI
device so no /sys/bus/platform/devices/LNXVIDEO:00
device is instantiated for the new "video" platform driver
to bind to.

While I appreciate the efforts being done to clean up
the ACPI subsystem I must also say that this makes me
question how well all these convert to platform driver
patches are tested ?

Almost all modern x86 hw has a /sys/bus/acpi/devices/LNXVIDEO:00
device, so this can be tested almost everywhere and this should
have been caught during testing by a test as simple as:

1. "ls /sys/bus/platform/devices/LNXVIDEO:00" and notice this
does not exist and/or:
2. "ls /sys/bus/platform/drivers/video/" and notice it has not
bound to anything where before this change the acpi_video
module would have bound to /sys/bus/acpi/devices/LNXVIDEO:00

Also the "Video Bus" input/evdev device is now gone
from "sudo evtest" which is a third quick way to see this
now all no longer works.

One possible way to solve this is to treat LNXVIDEO devices
specially and always create a platform_device for them.

This will also require some changes to the modalias
and driver-matching code because normally acpi:xxxx
modaliases are only used / matched when the platform_device
is the first physical node, where as I think
the platform_device will end up being the second physical
node now.

One last remark, assuming we find a way to solve this,
then IMHO the .name field in the driver:

>> +static struct platform_driver acpi_video_bus = {
>> +       .probe = acpi_video_bus_probe,
>> +       .remove_new = acpi_video_bus_remove,
>> +       .driver = {
>> +               .name = "video",
>> +               .acpi_match_table = video_device_ids,
>> +       },
>>  };

MUST not be just "video" platform devices <-> drivers  also get
matched by dev_name() so if anyone now creates a platform_device
named "video" then the platform_bus will now bind this driver
to it. "acpi_video", matching the .c filename (but not the module
name for historical reasons) would be better IMHO.

Regards,

Hans