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

[Julia Lawall]

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.

julia