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 _______________________________________________ ibm-acpi-devel mailing list ibm-acpi-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.sourceforge.net/lists/listinfo/ibm-acpi-devel
