From: Len Baker <len.baker@gmx.com>
To: Greg KH <greg@kroah.com>
Cc: Len Baker <len.baker@gmx.com>,
Hans de Goede <hdegoede@redhat.com>,
Kees Cook <keescook@chromium.org>,
Henrique de Moraes Holschuh <hmh@hmh.eng.br>,
Mark Gross <mgross@linux.intel.com>,
"Gustavo A. R. Silva" <gustavoars@kernel.org>,
ibm-acpi-devel@lists.sourceforge.net,
platform-driver-x86@vger.kernel.org,
linux-hardening@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] platform/x86: thinkpad_acpi: Prefer struct_size over open coded arithmetic
Date: Sat, 25 Sep 2021 15:33:45 +0200 [thread overview]
Message-ID: <20210925133345.GA1661@titan> (raw)
In-Reply-To: <YU8C6B5zw5t4vsh7@kroah.com>
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
next prev parent reply other threads:[~2021-09-25 13:34 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-09-18 15:05 [PATCH] platform/x86: thinkpad_acpi: Prefer struct_size over open coded arithmetic Len Baker
2021-09-20 5:58 ` Kees Cook
2021-09-21 13:46 ` Hans de Goede
2021-09-21 15:15 ` Greg KH
2021-09-21 15:38 ` Hans de Goede
2021-09-21 15:45 ` Greg KH
2021-09-25 10:40 ` Len Baker
2021-09-25 11:07 ` Greg KH
2021-09-25 13:33 ` Len Baker [this message]
2021-09-25 10:37 ` Len Baker
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20210925133345.GA1661@titan \
--to=len.baker@gmx.com \
--cc=greg@kroah.com \
--cc=gustavoars@kernel.org \
--cc=hdegoede@redhat.com \
--cc=hmh@hmh.eng.br \
--cc=ibm-acpi-devel@lists.sourceforge.net \
--cc=keescook@chromium.org \
--cc=linux-hardening@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=mgross@linux.intel.com \
--cc=platform-driver-x86@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).