Re: [PATCH v1 4/6] ACPI: acpi_video: Replace acpi_driver with platform_driver

[Hans de Goede <hdegoede@redhat.com>]

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@intel.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@intel.com>
>> Signed-off-by: Michal Wilczynski <michal.wilczynski@intel.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