[PATCH][next] mtd: rawnand: Replace one-element array with flexible-array member

[PATCH][next] mtd: rawnand: Replace one-element array with flexible-array member

[Gustavo A. R. Silva]

There is a regular need in the kernel to provide a way to declare having
a dynamically sized set of trailing elements in a structure. Kernel code
should always use “flexible array members”[1] for these cases. The older
style of one-element or zero-length arrays should no longer be used[2].

Refactor the code according to the use of a flexible-array member
instead of a one-element array. Also, make use of the struct_size()
helper to calculate the size of the allocation for _nand_. In order
to keep the code as maintainable as possible and to keep _cs_ as an
array, add a new macro CS_N to aid in the allocation size calculation,
in case there is a need for more Chip Select IDs in the future. In the
meantime, the macro is set to 1. This also avoids having to use a magic
number '1' as the last argument for struct_size().

[1] https://en.wikipedia.org/wiki/Flexible_array_member
[2] https://www.kernel.org/doc/html/v5.9-rc1/process/deprecated.html#zero-length-and-one-element-arrays

Build-tested-by: kernel test robot <lkp@intel.com>
Link: https://lore.kernel.org/lkml/5f7473c0.Vv4h6yzXSga90P04%25lkp@intel.com/
Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>
---
 drivers/mtd/nand/raw/tegra_nand.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/mtd/nand/raw/tegra_nand.c b/drivers/mtd/nand/raw/tegra_nand.c
index fbf67722a049..43b8359dcd85 100644
--- a/drivers/mtd/nand/raw/tegra_nand.c
+++ b/drivers/mtd/nand/raw/tegra_nand.c
@@ -163,6 +163,9 @@
 				HWSTATUS_RBSY_MASK(NAND_STATUS_READY) | \
 				HWSTATUS_RBSY_VALUE(NAND_STATUS_READY))
 
+/* Number of Chip Selects. Currently, only one. */
+#define CS_N			1
+
 struct tegra_nand_controller {
 	struct nand_controller controller;
 	struct device *dev;
@@ -183,7 +186,7 @@ struct tegra_nand_chip {
 	u32 config;
 	u32 config_ecc;
 	u32 bch_config;
-	int cs[1];
+	int cs[];
 };
 
 static inline struct tegra_nand_controller *
@@ -1086,14 +1089,14 @@ static int tegra_nand_chips_init(struct device *dev,
 		return -EINVAL;
 	}
 
-	/* Retrieve CS id, currently only single die NAND supported */
+	/* Retrieve CS id, currently only single-die NAND supported */
 	ret = of_property_read_u32(np_nand, "reg", &cs);
 	if (ret) {
 		dev_err(dev, "could not retrieve reg property: %d\n", ret);
 		return ret;
 	}
 
-	nand = devm_kzalloc(dev, sizeof(*nand), GFP_KERNEL);
+	nand = devm_kzalloc(dev, struct_size(nand, cs, CS_N), GFP_KERNEL);
 	if (!nand)
 		return -ENOMEM;
 
-- 
2.27.0

Re: [PATCH][next] mtd: rawnand: Replace one-element array with flexible-array member

[Jann Horn]

On Wed, Sep 30, 2020 at 11:02 PM Gustavo A. R. Silva
<gustavoars@kernel.org> wrote:
> There is a regular need in the kernel to provide a way to declare having
> a dynamically sized set of trailing elements in a structure. Kernel code
> should always use “flexible array members”[1] for these cases. The older
> style of one-element or zero-length arrays should no longer be used[2].

But this is not such a case, right? Isn't this a true fixed-size
array? It sounds like you're just changing it because it
pattern-matched on "array of length 1 at the end of a struct".

> Refactor the code according to the use of a flexible-array member
> instead of a one-element array. Also, make use of the struct_size()
> helper to calculate the size of the allocation for _nand_. In order
> to keep the code as maintainable as possible and to keep _cs_ as an
> array, add a new macro CS_N to aid in the allocation size calculation,
> in case there is a need for more Chip Select IDs in the future. In the
> meantime, the macro is set to 1. This also avoids having to use a magic
> number '1' as the last argument for struct_size().
[...]
> diff --git a/drivers/mtd/nand/raw/tegra_nand.c b/drivers/mtd/nand/raw/tegra_nand.c
[...]
> +/* Number of Chip Selects. Currently, only one. */
> +#define CS_N                   1
> +
>  struct tegra_nand_controller {
>         struct nand_controller controller;
>         struct device *dev;
> @@ -183,7 +186,7 @@ struct tegra_nand_chip {
>         u32 config;
>         u32 config_ecc;
>         u32 bch_config;
> -       int cs[1];
> +       int cs[];
>  };
[...]

Re: [PATCH][next] mtd: rawnand: Replace one-element array with flexible-array member

[Gustavo A. R. Silva]

On Wed, Sep 30, 2020 at 11:10:43PM +0200, Jann Horn wrote:
> On Wed, Sep 30, 2020 at 11:02 PM Gustavo A. R. Silva
> <gustavoars@kernel.org> wrote:
> > There is a regular need in the kernel to provide a way to declare having
> > a dynamically sized set of trailing elements in a structure. Kernel code
> > should always use “flexible array members”[1] for these cases. The older
> > style of one-element or zero-length arrays should no longer be used[2].
> 
> But this is not such a case, right? Isn't this a true fixed-size
> array? It sounds like you're just changing it because it
> pattern-matched on "array of length 1 at the end of a struct".

Yeah; I should have changed that 'dynamically' part of the text above
a bit. However, as I commented in the text below, in the case that more
CS IDs are needed (let's wait for the maintainers to comment on this...)
in the future, this change makes the code more maintainable, as for
the allocation part, the developer would only have to update the CS_N
macro to the number of CS IDs that are needed.

Thanks
--
Gustavo

> 
> > Refactor the code according to the use of a flexible-array member
> > instead of a one-element array. Also, make use of the struct_size()
> > helper to calculate the size of the allocation for _nand_. In order
> > to keep the code as maintainable as possible and to keep _cs_ as an
> > array, add a new macro CS_N to aid in the allocation size calculation,
> > in case there is a need for more Chip Select IDs in the future. In the
> > meantime, the macro is set to 1. This also avoids having to use a magic
> > number '1' as the last argument for struct_size().
> [...]
> > diff --git a/drivers/mtd/nand/raw/tegra_nand.c b/drivers/mtd/nand/raw/tegra_nand.c
> [...]
> > +/* Number of Chip Selects. Currently, only one. */
> > +#define CS_N                   1
> > +
> >  struct tegra_nand_controller {
> >         struct nand_controller controller;
> >         struct device *dev;
> > @@ -183,7 +186,7 @@ struct tegra_nand_chip {
> >         u32 config;
> >         u32 config_ecc;
> >         u32 bch_config;
> > -       int cs[1];
> > +       int cs[];
> >  };
> [...]

Re: [PATCH][next] mtd: rawnand: Replace one-element array with flexible-array member

[Jann Horn]

On Wed, Sep 30, 2020 at 11:30 PM Gustavo A. R. Silva
<gustavoars@kernel.org> wrote:
> On Wed, Sep 30, 2020 at 11:10:43PM +0200, Jann Horn wrote:
> > On Wed, Sep 30, 2020 at 11:02 PM Gustavo A. R. Silva
> > <gustavoars@kernel.org> wrote:
> > > There is a regular need in the kernel to provide a way to declare having
> > > a dynamically sized set of trailing elements in a structure. Kernel code
> > > should always use “flexible array members”[1] for these cases. The older
> > > style of one-element or zero-length arrays should no longer be used[2].
> >
> > But this is not such a case, right? Isn't this a true fixed-size
> > array? It sounds like you're just changing it because it
> > pattern-matched on "array of length 1 at the end of a struct".
>
> Yeah; I should have changed that 'dynamically' part of the text above
> a bit. However, as I commented in the text below, in the case that more
> CS IDs are needed (let's wait for the maintainers to comment on this...)
> in the future, this change makes the code more maintainable, as for
> the allocation part, the developer would only have to update the CS_N
> macro to the number of CS IDs that are needed.

But in that case, shouldn't you change it to "int cs[CS_N]" and get
rid of the struct_size() stuff?

Re: [PATCH][next] mtd: rawnand: Replace one-element array with flexible-array member

[Miquel Raynal]

Hi Jann,

Jann Horn <jannh@google.com> wrote on Thu, 1 Oct 2020 00:32:24 +0200:

> On Wed, Sep 30, 2020 at 11:30 PM Gustavo A. R. Silva
> <gustavoars@kernel.org> wrote:
> > On Wed, Sep 30, 2020 at 11:10:43PM +0200, Jann Horn wrote:  
> > > On Wed, Sep 30, 2020 at 11:02 PM Gustavo A. R. Silva
> > > <gustavoars@kernel.org> wrote:  
> > > > There is a regular need in the kernel to provide a way to declare having
> > > > a dynamically sized set of trailing elements in a structure. Kernel code
> > > > should always use “flexible array members”[1] for these cases. The older
> > > > style of one-element or zero-length arrays should no longer be used[2].  
> > >
> > > But this is not such a case, right? Isn't this a true fixed-size
> > > array? It sounds like you're just changing it because it
> > > pattern-matched on "array of length 1 at the end of a struct".  
> >
> > Yeah; I should have changed that 'dynamically' part of the text above
> > a bit. However, as I commented in the text below, in the case that more
> > CS IDs are needed (let's wait for the maintainers to comment on this...)
> > in the future, this change makes the code more maintainable, as for
> > the allocation part, the developer would only have to update the CS_N
> > macro to the number of CS IDs that are needed.  
> 
> But in that case, shouldn't you change it to "int cs[CS_N]" and get
> rid of the struct_size() stuff?

I do agree with Jann, I think it's best to consider this a fixed-size
array for now. If we ever want to extend the number of supported CS,
there is much more rework involved anyway.

Thanks,
Miquèl

Re: [PATCH][next] mtd: rawnand: Replace one-element array with flexible-array member

[Stefan Agner]

On 2020-10-01 10:12, Miquel Raynal wrote:
> Hi Jann,
> 
> Jann Horn <jannh@google.com> wrote on Thu, 1 Oct 2020 00:32:24 +0200:
> 
>> On Wed, Sep 30, 2020 at 11:30 PM Gustavo A. R. Silva
>> <gustavoars@kernel.org> wrote:
>> > On Wed, Sep 30, 2020 at 11:10:43PM +0200, Jann Horn wrote:
>> > > On Wed, Sep 30, 2020 at 11:02 PM Gustavo A. R. Silva
>> > > <gustavoars@kernel.org> wrote:
>> > > > There is a regular need in the kernel to provide a way to declare having
>> > > > a dynamically sized set of trailing elements in a structure. Kernel code
>> > > > should always use “flexible array members”[1] for these cases. The older
>> > > > style of one-element or zero-length arrays should no longer be used[2].
>> > >
>> > > But this is not such a case, right? Isn't this a true fixed-size
>> > > array? It sounds like you're just changing it because it
>> > > pattern-matched on "array of length 1 at the end of a struct".
>> >
>> > Yeah; I should have changed that 'dynamically' part of the text above
>> > a bit. However, as I commented in the text below, in the case that more
>> > CS IDs are needed (let's wait for the maintainers to comment on this...)
>> > in the future, this change makes the code more maintainable, as for
>> > the allocation part, the developer would only have to update the CS_N
>> > macro to the number of CS IDs that are needed.
>>
>> But in that case, shouldn't you change it to "int cs[CS_N]" and get
>> rid of the struct_size() stuff?
> 
> I do agree with Jann, I think it's best to consider this a fixed-size
> array for now. If we ever want to extend the number of supported CS,
> there is much more rework involved anyway.

I agree, too, just assume this is a fixed-size array of 1 element.

In fact, I am not aware of a design which needs multiple chip selects.

--
Stefan