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

[PATCH][next] hpilo: 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].

For this particular case, it is important to notice that the cachelines
change from 7 to 6 after the flexible-array conversion:

$ pahole -C 'fifo' drivers/misc/hpilo.o
struct fifo {
	u64                        nrents;               /*     0     8 */
	u64                        imask;                /*     8     8 */
	u64                        merge;                /*    16     8 */
	u64                        reset;                /*    24     8 */
	u8                         pad_0[96];            /*    32    96 */
	/* --- cacheline 2 boundary (128 bytes) --- */
	u64                        head;                 /*   128     8 */
	u8                         pad_1[120];           /*   136   120 */
	/* --- cacheline 4 boundary (256 bytes) --- */
	u64                        tail;                 /*   256     8 */
	u8                         pad_2[120];           /*   264   120 */
	/* --- cacheline 6 boundary (384 bytes) --- */
	u64                        fifobar[1];           /*   384     8 */

	/* size: 392, cachelines: 7, members: 10 */
	/* last cacheline: 8 bytes */
};

$ pahole -C 'fifo' drivers/misc/hpilo.o
struct fifo {
	u64                        nrents;               /*     0     8 */
	u64                        imask;                /*     8     8 */
	u64                        merge;                /*    16     8 */
	u64                        reset;                /*    24     8 */
	u8                         pad_0[96];            /*    32    96 */
	/* --- cacheline 2 boundary (128 bytes) --- */
	u64                        head;                 /*   128     8 */
	u8                         pad_1[120];           /*   136   120 */
	/* --- cacheline 4 boundary (256 bytes) --- */
	u64                        tail;                 /*   256     8 */
	u8                         pad_2[120];           /*   264   120 */
	/* --- cacheline 6 boundary (384 bytes) --- */
	u64                        fifobar[];            /*   384     0 */

	/* size: 384, cachelines: 6, members: 10 */
};

Lastly, remove unnecessary parentheses in fifo_sz() and fix the following
checkpatch.pl warning for the whole fifo structure:

WARNING: please, no spaces at the start of a line

[1] https://en.wikipedia.org/wiki/Flexible_array_member
[2] https://github.com/KSPP/linux/issues/79

Tested-by: kernel test robot <lkp@intel.com>
Link: https://github.com/GustavoARSilva/linux-hardening/blob/master/cii/kernel-ci/hpilo-20200714.md
Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>
---
 drivers/misc/hpilo.c |  2 +-
 drivers/misc/hpilo.h | 22 +++++++++++-----------
 2 files changed, 12 insertions(+), 12 deletions(-)

diff --git a/drivers/misc/hpilo.c b/drivers/misc/hpilo.c
index 927309b86bab..10c975662f8b 100644
--- a/drivers/misc/hpilo.c
+++ b/drivers/misc/hpilo.c
@@ -207,7 +207,7 @@ static void ctrl_setup(struct ccb *ccb, int nr_desc, int l2desc_sz)
 static inline int fifo_sz(int nr_entry)
 {
 	/* size of a fifo is determined by the number of entries it contains */
-	return (nr_entry * sizeof(u64)) + FIFOHANDLESIZE;
+	return nr_entry * sizeof(u64) + FIFOHANDLESIZE;
 }
 
 static void fifo_setup(void *base_addr, int nr_entry)
diff --git a/drivers/misc/hpilo.h b/drivers/misc/hpilo.h
index 1aa433a7f66c..f69ff645cac9 100644
--- a/drivers/misc/hpilo.h
+++ b/drivers/misc/hpilo.h
@@ -160,23 +160,23 @@ struct ccb_data {
 #define ILO_START_ALIGN	4096
 #define ILO_CACHE_SZ 	 128
 struct fifo {
-    u64 nrents;	/* user requested number of fifo entries */
-    u64 imask;  /* mask to extract valid fifo index */
-    u64 merge;	/*  O/C bits to merge in during enqueue operation */
-    u64 reset;	/* set to non-zero when the target device resets */
-    u8  pad_0[ILO_CACHE_SZ - (sizeof(u64) * 4)];
+	u64 nrents;	/* user requested number of fifo entries */
+	u64 imask;  /* mask to extract valid fifo index */
+	u64 merge;	/*  O/C bits to merge in during enqueue operation */
+	u64 reset;	/* set to non-zero when the target device resets */
+	u8  pad_0[ILO_CACHE_SZ - (sizeof(u64) * 4)];
 
-    u64 head;
-    u8  pad_1[ILO_CACHE_SZ - (sizeof(u64))];
+	u64 head;
+	u8  pad_1[ILO_CACHE_SZ - (sizeof(u64))];
 
-    u64 tail;
-    u8  pad_2[ILO_CACHE_SZ - (sizeof(u64))];
+	u64 tail;
+	u8  pad_2[ILO_CACHE_SZ - (sizeof(u64))];
 
-    u64 fifobar[1];
+	u64 fifobar[];
 };
 
 /* convert between struct fifo, and the fifobar, which is saved in the ccb */
-#define FIFOHANDLESIZE (sizeof(struct fifo) - sizeof(u64))
+#define FIFOHANDLESIZE (sizeof(struct fifo))
 #define FIFOBARTOHANDLE(_fifo) \
 	((struct fifo *)(((char *)(_fifo)) - FIFOHANDLESIZE))
 
-- 
2.27.0

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

[Greg Kroah-Hartman]

On Tue, Jul 14, 2020 at 10:44:49AM -0500, 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].
> 
> For this particular case, it is important to notice that the cachelines
> change from 7 to 6 after the flexible-array conversion:

That's really funny to see.  Nice work, I'll go queue this up.  I doubt
anyone will notice as this is a very old driver :)

thanks,

greg k-h

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

[Gustavo A. R. Silva]

On Tue, Jul 14, 2020 at 06:19:48PM +0200, Greg Kroah-Hartman wrote:
> On Tue, Jul 14, 2020 at 10:44:49AM -0500, 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].
> > 
> > For this particular case, it is important to notice that the cachelines
> > change from 7 to 6 after the flexible-array conversion:
> 
> That's really funny to see.  Nice work, I'll go queue this up.  I doubt
> anyone will notice as this is a very old driver :)
> 

Yep; by the way, notice the link to the Kernel CI test results:

https://github.com/GustavoARSilva/linux-hardening/blob/master/cii/kernel-ci/hpilo-20200714.md

I plan to add the generation --and inclusion in the changelog text-- of
such links to my workflow, whenever possible.

When Kernel CI reports 'SUCCESS', they don't send the report to any mailing
list; they only send those reports to the maintainers/developers. So. I'm
going to store the ones I get in this github repository:

https://github.com/GustavoARSilva/linux-hardening/tree/master/cii/kernel-ci/

Thanks
--
Gustavo