Re: [PATCH v5] staging: greybus: Constify gb_audio_module_type

[Dan Carpenter]

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