Hi Martin,
On 3/24/21 20:18, Martin K. Petersen wrote:
>
> Hi Gustavo!
>
> Your changes and the original code do not appear to be functionally
> equivalent.
>
>> @@ -1235,8 +1235,8 @@ static int aac_read_raw_io(struct fib * fib, struct scsi_cmnd * cmd, u64 lba, u3
>> if (ret < 0)
>> return ret;
>> command = ContainerRawIo2;
>> - fibsize = sizeof(struct aac_raw_io2) +
>> - ((le32_to_cpu(readcmd2->sgeCnt)-1) * sizeof(struct sge_ieee1212));
>> + fibsize = struct_size(readcmd2, sge,
>> + le32_to_cpu(readcmd2->sgeCnt));
>
> The old code allocated sgeCnt-1 elements (whether that was a mistake or
> not I do not know) whereas the new code would send a larger fib to the
> ASIC. I don't have any aacraid adapters and I am hesitant to merging
> changes that have not been validated on real hardware.
Precisely this sort of confusion is one of the things we want to avoid
by using flexible-array members instead of one-element arrays.
fibsize is actually the same for both the old and the new code. The
difference is that in the original code, the one-element array _sge_
at the bottom of struct aac_raw_io2, contributes to the size of the
structure, as it occupies at least as much space as a single object
of its type. On the other hand, flexible-array members don't contribute
to the size of the enclosing structure. See below...
Old code:
$ pahole -C aac_raw_io2 drivers/scsi/aacraid/aachba.o
struct aac_raw_io2 {
__le32 blockLow; /* 0 4 */
__le32 blockHigh; /* 4 4 */
__le32 byteCount; /* 8 4 */
__le16 cid; /* 12 2 */
__le16 flags; /* 14 2 */
__le32 sgeFirstSize; /* 16 4 */
__le32 sgeNominalSize; /* 20 4 */
u8 sgeCnt; /* 24 1 */
u8 bpTotal; /* 25 1 */
u8 bpComplete; /* 26 1 */
u8 sgeFirstIndex; /* 27 1 */
u8 unused[4]; /* 28 4 */
struct sge_ieee1212 sge[1]; /* 32 16 */
/* size: 48, cachelines: 1, members: 13 */
/* last cacheline: 48 bytes */
};
New code:
$ pahole -C aac_raw_io2 drivers/scsi/aacraid/aachba.o
struct aac_raw_io2 {
__le32 blockLow; /* 0 4 */
__le32 blockHigh; /* 4 4 */
__le32 byteCount; /* 8 4 */
__le16 cid; /* 12 2 */
__le16 flags; /* 14 2 */
__le32 sgeFirstSize; /* 16 4 */
__le32 sgeNominalSize; /* 20 4 */
u8 sgeCnt; /* 24 1 */
u8 bpTotal; /* 25 1 */
u8 bpComplete; /* 26 1 */
u8 sgeFirstIndex; /* 27 1 */
u8 unused[4]; /* 28 4 */
struct sge_ieee1212 sge[]; /* 32 0 */
/* size: 32, cachelines: 1, members: 13 */
/* last cacheline: 32 bytes */
};
So, the old code allocates sgeCnt-1 elements because sizeof(struct aac_raw_io2) is
already counting one element of the _sge_ array.
Please, let me know if this is clear now.
Thanks!
--
Gustavo
