Re: [PATCH v2][next] scsi: megaraid_sas: Avoid a couple -Wflex-array-member-not-at-end warnings

[James Bottomley <James.Bottomley@HansenPartnership.com>]

On Tue, 2025-10-07 at 15:18 +0100, Gustavo A. R. Silva wrote:
> 
> 
> On 10/7/25 12:59, James Bottomley wrote:
> > On Tue, 2025-10-07 at 11:43 +0100, Gustavo A. R. Silva wrote:
> > > Hi all,
> > > 
> > > Friendly ping: who can take this, please?
> > 
> > After what happened with the qla2xxx driver, everyone is a bit wary
> > of these changes, particularly when they affect structures shared
> > with the hardware. Megaraid is a broadcom acquisition so although
> > maintained it might take them a while to check this.
> 
> I've been in constant communication with the people involved. So far,
> none of them has expressed any concerns about this to me. However, I
> appreciate your feedback.
> 
> In any case, I promptly submitted a bugfix minutes after getting the
> report.

I'm not criticizing the response, just saying that problems like this
cause me to think that someone who understands and can test the
hardware needs to look at this.  However ...

> > However, you could help us with this: as I understand it (there is
> > a bit of a no documentation problem here), the TRAILING_OVERLAP
> > formalism merely gets the compiler not to warn about the situation
> > rather than actually changing anything in the layout of the
> > structure?  In which case you should be able to demonstrate the
> > binary produced before and after this patch is the same, which
> > would very much reduce the risk of taking it.
> 
> This is quite simple. Here you go the pahole output before and after
> changes.
> 
> BEFORE CHANGES:
> 
> pahole -C MR_FW_RAID_MAP_ALL drivers/scsi/megaraid/megaraid_sas_fp.o
> struct MR_FW_RAID_MAP_ALL {
>          struct MR_FW_RAID_MAP      raidMap;              /*     0
> 10408 */
>          /* --- cacheline 162 boundary (10368 bytes) was 40 bytes ago
> --- */
>          struct MR_LD_SPAN_MAP      ldSpanMap[64];        /* 10408
> 161792 */
> 
>          /* size: 172200, cachelines: 2691, members: 2 */
>          /* last cacheline: 40 bytes */
> };
> 
> AFTER CHANGES:
> 
> pahole -C MR_FW_RAID_MAP_ALL drivers/scsi/megaraid/megaraid_sas_fp.o
> struct MR_FW_RAID_MAP_ALL {
>          union {
>                  struct MR_FW_RAID_MAP raidMap;           /*     0
> 10408 */
>                  struct {
>                          unsigned char __offset_to_FAM[10408]; /*    
> 0 10408 */
>                          /* --- cacheline 162 boundary (10368 bytes)
> was 40 bytes ago --- */
>                          struct MR_LD_SPAN_MAP ldSpanMap[64]; /*
> 10408 161792 */
>                  };                                       /*     0
> 172200 */
>          };                                               /*     0
> 172200 */
> 
>          /* size: 172200, cachelines: 2691, members: 1 */
>          /* last cacheline: 40 bytes */
> };
> 
> As you can see, the size is exactly the same, as are the offsets for
> both members raidMap and ldSpanMap.

... this is good enough to prove that the binary before and after is
identical and thus there's no change to the structures, which means the
risk of accepting the patch is significantly lower.

>  The trick is that, thanks to the union and __offset_to_FAM, the
> flexible-array member raidMap.ldSpanMap[] now appears as the last
> member instead of somewhere in the middle.
> 
> So both ldSpanMap and raidMap.ldSpanMap[] now cleanly overlap, as
> seems to have been intended.
> 
> (Exactly the same applies for struct MR_DRV_RAID_MAP_ALL)
> 
> I can include this explanation to the changelog text if you'd like.

I'll leave it up to Martin, but I think going forwards it would be
helpful if you could indicate that you've checked that the binary
layout before and after is unchanged and thus the risk of merging the
patch is low.

Regards,

James