[PATCH v5] staging: greybus: Constify gb_audio_module_type

[PATCH v5] staging: greybus: Constify gb_audio_module_type

[Ayush Tiwari]

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.

Signed-off-by: Ayush Tiwari <ayushtiw0110@gmail.com>

---
Changes in v5: added more details as per feedback.

Changes in v4: added more details verifying the change.

Changes in v3: added the message that verifies the change,
as suggested by Julia

Changes in v2: incorporated changes in commit message
as suggested by Alison
---
 drivers/staging/greybus/audio_manager_module.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/greybus/audio_manager_module.c b/drivers/staging/greybus/audio_manager_module.c
index 5f9dcbdbc191..4a4dfb42f50f 100644
--- a/drivers/staging/greybus/audio_manager_module.c
+++ b/drivers/staging/greybus/audio_manager_module.c
@@ -144,7 +144,7 @@ static struct attribute *gb_audio_module_default_attrs[] = {
 };
 ATTRIBUTE_GROUPS(gb_audio_module_default);
 
-static struct kobj_type gb_audio_module_type = {
+static const struct kobj_type gb_audio_module_type = {
 	.sysfs_ops = &gb_audio_module_sysfs_ops,
 	.release = gb_audio_module_release,
 	.default_groups = gb_audio_module_default_groups,
-- 
2.40.1

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

[Fabio M. De Francesco]

On Saturday, 16 March 2024 19:24:21 CET Ayush Tiwari wrote:
> Constify static struct kobj_type gb_audio_module_type to prevent
> modification 

I'm not yet convinced that being or not being const has much to do with 
sharing data. 

And next time please wait for other people to comment your versions. Wait a 
day or two and then please delete everything from here...

> 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), 

to here.

Thanks,

Fabio
 
> 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.
> 
> Signed-off-by: Ayush Tiwari <ayushtiw0110@gmail.com>
> 
> ---
> Changes in v5: added more details as per feedback.
> 
> Changes in v4: added more details verifying the change.
> 
> Changes in v3: added the message that verifies the change,
> as suggested by Julia
> 
> Changes in v2: incorporated changes in commit message
> as suggested by Alison
> ---
>  drivers/staging/greybus/audio_manager_module.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/greybus/audio_manager_module.c
> b/drivers/staging/greybus/audio_manager_module.c index
> 5f9dcbdbc191..4a4dfb42f50f 100644
> --- a/drivers/staging/greybus/audio_manager_module.c
> +++ b/drivers/staging/greybus/audio_manager_module.c
> @@ -144,7 +144,7 @@ static struct attribute
> *gb_audio_module_default_attrs[] = { };
>  ATTRIBUTE_GROUPS(gb_audio_module_default);
> 
> -static struct kobj_type gb_audio_module_type = {
> +static const struct kobj_type gb_audio_module_type = {
>  	.sysfs_ops = &gb_audio_module_sysfs_ops,
>  	.release = gb_audio_module_release,
>  	.default_groups = gb_audio_module_default_groups,

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

[Dan Carpenter]

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."

regards,
dan carpenter

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

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