Messages in this thread | | | Date | Sat, 25 Sep 2021 13:07:20 +0200 | From | Greg KH <> | Subject | Re: [PATCH] platform/x86: thinkpad_acpi: Prefer struct_size over open coded arithmetic |
| |
On Sat, Sep 25, 2021 at 12:40:44PM +0200, Len Baker wrote: > Hi, > > On Tue, Sep 21, 2021 at 05:15:35PM +0200, Greg KH wrote: > > > > First off, why is a single driver doing so many odd things with > > attribute groups? Why not just use them the way that the rest of the > > kernel does? Why does this driver need this special handling and no one > > else does? > > Is [1] the correct way to deal with devices attributes? I think so. > > [1] https://www.kernel.org/doc/html/latest/driver-api/driver-model/driver.html#attributes
No, do not use driver_create_file(), see: http://kroah.com/log/blog/2013/06/26/how-to-create-a-sysfs-file-correctly/ as a more up to date thing.
Someone should fix that in-kernel documentation one day :)
> > I think the default way of handling if an attribute is enabled or not, > > should suffice here, and make things much simpler overall as all of this > > crazy attribute handling can just be removed. > > Sorry but what is the default way? Would it be correct to check if the > file exists?
Use the is_visable() callback for the attribute group to enable/disable the creation of the sysfs file.
> > Bonus would also be that I think it would fix the race conditions that > > happen when trying to create attributes after the device is bound to the > > driver that I think the existing driver has today. > > > > > > (I see the caller uses +2? Why? It seems to be using each of hotkey_attributes, > > > > plus 1 more attr, plus a final NULL?) > > > > > > The +2 is actually for 2 extra attributes (making the total number > > > of extra attributes +3 because the sizeof(struct attribute_set_obj) > > > already includes 1 extra). > > > > > > FWIW these 2 extra attributes are for devices with a > > > a physical rfkill on/off switch and for the device being > > > a convertible capable of reporting laptop- vs tablet-mode. > > > > Again, using the default way to show (or not show) attributes should > > solve this issue. Why not just use that instead? > > What is the default way? Would it be correct to use device_create_file() > and device_remove_file()?
Put all the attributes into an attribute group and attach it to the driver. The driver core will create/remove the files when needed. The link above should help explain that a bit better, and I can point you at examples if needed.
Does that help?
thanks,
greg k-h
| |