Re: [PATCH] platform/x86: thinkpad_acpi: Prefer struct_size over open coded arithmetic

[Len Baker]

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