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@xxxxxx> 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
