Hi Greg, On Sat, Sep 25, 2021 at 01:07:20PM +0200, Greg KH wrote: > 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. Ok, understood. Thanks. > > 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. Ok, I will take a look at it. > > > > 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? Yes, things are clearer to me now. Also, since the only way to learn is to do so, I will take the task to switch this driver to the common use of attributes. Thank you very much for your time and guidance. Regards, Len
