Re: [PATCH][next] mtd: rawnand: Replace one-element array with flexible-array member
On 2020-10-01 10:12, Miquel Raynal wrote: > Hi Jann, > > Jann Horn wrote on Thu, 1 Oct 2020 00:32:24 +0200: > >> On Wed, Sep 30, 2020 at 11:30 PM Gustavo A. R. Silva >> 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 >> > > 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
Re: [PATCH][next] mtd: rawnand: Replace one-element array with flexible-array member
Hi Jann, Jann Horn wrote on Thu, 1 Oct 2020 00:32:24 +0200: > On Wed, Sep 30, 2020 at 11:30 PM Gustavo A. R. Silva > 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 > > > 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
On Wed, Sep 30, 2020 at 11:30 PM Gustavo A. R. Silva 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 > > 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
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 > 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
On Wed, Sep 30, 2020 at 11:02 PM Gustavo A. R. Silva 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[]; > }; [...]
[PATCH][next] mtd: rawnand: Replace one-element array with flexible-array member
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 Link: https://lore.kernel.org/lkml/5f7473c0.vv4h6yzxsga90p04%25...@intel.com/ Signed-off-by: Gustavo A. R. Silva --- 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", ); 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