From: Dan Carpenter <dan.carpenter@linaro.org>
To: Julia Lawall <julia.lawall@inria.fr>
Cc: Ayush Tiwari <ayushtiw0110@gmail.com>,
Larry.Finger@lwfinger.net, florian.c.schilhabel@googlemail.com,
gregkh@linuxfoundation.org, linux-kernel@vger.kernel.org,
linux-staging@lists.linux.dev, outreachy@lists.linux.dev
Subject: Re: [PATCH v5] staging: greybus: Constify gb_audio_module_type
Date: Mon, 18 Mar 2024 14:41:48 +0300 [thread overview]
Message-ID: <499006ee-f436-476f-a669-d1abfdb082db@moroto.mountain> (raw)
In-Reply-To: <95d1caf9-aac4-9b10-a933-63874274464@inria.fr>
On Mon, Mar 18, 2024 at 12:25:55PM +0100, Julia Lawall wrote:
>
>
> On Mon, 18 Mar 2024, Dan Carpenter wrote:
>
> > On Sat, Mar 16, 2024 at 11:54:21PM +0530, Ayush Tiwari wrote:
> > > Constify static struct kobj_type gb_audio_module_type to prevent
> > > modification of data shared across many instances(instances here
> > > refer to multiple kobject instances being created, since this same
> > > gb_audio_module_type structure is used as a template for all audio
> > > manager module kobjs, it is effectively 'shared' across all these
> > > instances), ensuring that the structure's usage is consistent and
> > > predictable throughout the driver and allowing the compiler to place
> > > it in read-only memory. The gb_audio_module_type structure is used
> > > when initializing and adding kobj instances to the kernel's object
> > > hierarchy. After adding const, any attempt to alter
> > > gb_audio_module_type in the code would raise a compile-time error.
> > > This enforcement ensures that the sysfs interface and operations for
> > > audio modules remain stable.
> > >
> >
> > Basically the patch is fine. The only comments have been around the
> > commit message. And all the reviewers have said correct things... But
> > I'm still going to chime in as well.
> >
> > The commit message is too long for something very simple.
> >
> > Basically all kernel maintainers understand about constness. There is
> > sometimes trickiness around constness but in this specific case there
> > isn't anything subtle or interesting. You don't need to explain about
> > constness. Maybe you can say the word "hardenning" as an explanation.
> >
> > Julia asked you to write what steps you had done to ensure that the
> > patch doesn't break anything. And I was curious what she meant by that
> > because I had forgotten that it would be bad if there were a cast that
> > removed the const. So the bit about "any attempt to alter
> > gb_audio_module_type in the code would raise a compile-time error." is
> > not true.
> >
> > Also we assume that you have compile tested everything so you never need
> > to write that.
> >
> > The information which is missing from this commit message is the
> > checkpatch warning. I'm more familiar with checkpatch than a lot of
> > kernel maintainers and I had forgotten that this was a checkpatch
> > warning. Please copy and paste the warning.
> >
> > Basically what I want in a commit message is this:
> >
> > "Checkpatch complains that "gb_audio_module_type" should be const as
> > part of kernel hardenning. <Copy and paste relevant bits from
> > checkpatch>. I have reviewed how this struct is used and it's never
> > modified anywhere so checkpatch is correct that it can safely be
> > declared as const. Constify it."
>
> I would still prefer to see that the structure is only passed to a certain
> function, and that function only uses the structure in a certain way (fill
> in the exact details). In this case, one may just rely on the fact that
> the parameter that receives the value is also declared as const, or one
> could check that that const declaration is actually correct. I think that
> is what Ayush was trying to do, although in a somewhat verbose way. This
> case is a bit more complex than many others, because the structure isn't
> used locally, but is passed off to some other function.
Huh. Yeah. That's almost certainly how it was intended to be read.
I apologize. Maybe something like the following:
The "gb_audio_module_type" struct is only used in one place:
err = kobject_init_and_add(&m->kobj, &gb_audio_module_type, NULL, ...
so checkpatch is correct that it can be made const.
That kind of commit message wouldn't work if the struct was used a lot
but it works here.
regards,
dan carpenter
prev parent reply other threads:[~2024-03-18 11:41 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-03-16 18:24 [PATCH v5] staging: greybus: Constify gb_audio_module_type Ayush Tiwari
2024-03-16 18:45 ` Fabio M. De Francesco
2024-03-18 11:16 ` Dan Carpenter
2024-03-18 11:25 ` Julia Lawall
2024-03-18 11:41 ` Dan Carpenter [this message]
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=499006ee-f436-476f-a669-d1abfdb082db@moroto.mountain \
--to=dan.carpenter@linaro.org \
--cc=Larry.Finger@lwfinger.net \
--cc=ayushtiw0110@gmail.com \
--cc=florian.c.schilhabel@googlemail.com \
--cc=gregkh@linuxfoundation.org \
--cc=julia.lawall@inria.fr \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-staging@lists.linux.dev \
--cc=outreachy@lists.linux.dev \
/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 read-only IMAP folder(s) and NNTP newsgroup(s).