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