Re: [PATCH v2 2/2] platform/x86: Add driver for Yoga Tablet Mode switch

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

Hi,

On 3/25/23 22:49, Armin Wolf wrote:
> Am 25.03.23 um 16:16 schrieb Andrew Kallmeyer:
> 
>> On Sat, Mar 25, 2023 at 12:10 AM Armin Wolf <W_Armin@gmx.de> wrote:
>>> Hi,
>>>
>>> is it really necessary to allow userspace to read/write ec_trigger? The ACPI device
>>> used for triggering the EC is only initialized during probing, so changing the value
>>> of ec_trigger will make no difference in such cases.
>>>
>>> Maybe you could change module_param(ec_trigger, bool, 0644) to module_param(ec_trigger, bool, 0)?
>>>
>>> Armin Wolf
>> Great point, this is actually a regression from Gergo's original patch
>> that I didn't realize I caused. I believe the intention was that if
>> the quirk detection code doesn't have full coverage users can set the
>> parameter themselves. In Gergo's code it used the acpi_device from
>> ideapad-laptop.c which is always loaded if it exists. Now I only load
>> it if ec_trigger is true at probe time, I think I should update it to
>> load the acpi device always if it exists so that the user can set this
>> parameter at any time. I suppose I would just remove the if
>> (ec_trigger) (and the debug print) in the probe code when I load it.
>>
>> That is, unless you think it is best to just patch in more models to
>> the quirk detection later and not provide a parameter. Barnabás
>> actually suggested removing the ec_trigger flag completely because
>> right now the code isn't relying on it, but I think that is a bug.
> 
> I think it is best to still provide this module param for people who need
> it, but only allow enabling it when loading the module. This way userspace
> should not be able to read/write ec_trigger after the module has been loaded.
> 
> Because of this, i believe the ACPI device should only be initialized when
> the DMI table says its necessary or ec_trigger was set. So the current solution
> is fine for me, except for ec_trigger being accessible to userspace after
> the module was loaded.

Right, ack.

For the next version please use 0444 and not 0 for the sysfs file rights on the file, this way users can still read the parameters under /sys/modules/.../parameters/ec_trigger to e.g. check if the parameter was set, which is useful to e.g. see if an /etc/modprobe.d/foo.conf dropin file is working as expected.

Also please address Barnabás' remarks. I have not taken a really close look myself yet, but given the close look others have already given this drivers I don't expect to find any issues.

So please prepare a v3 (when you have time) and then I'll try to merge that right away.

Regards,

Hans