| Message ID | aBO_32OsMj3hsJsv@kspp |
|---|---|
| State | New |
| Headers | show |
| Series | [v2,next] scsi: sd: Avoid -Wflex-array-member-not-at-end warning | expand |
I actually send a patch for this yesterday, also keeping the proto test that actually still works despite my initial overreaching removal. (I still wish we'd kill the stupid struct and this test and just used the simpler and cleaner bitshifting and masking)
On 5/1/25 11:50 PM, Christoph Hellwig wrote: > (I still wish we'd kill the stupid struct and this test and just used > the simpler and cleaner bitshifting and masking) Bit-shifting and masking results in faster code. For slow path code I prefer bitfields because this results in much more compact source code. Bart.
On Sat, May 03, 2025 at 12:08:23PM -0700, Bart Van Assche wrote: > On 5/1/25 11:50 PM, Christoph Hellwig wrote: > > (I still wish we'd kill the stupid struct and this test and just used > > the simpler and cleaner bitshifting and masking) > > Bit-shifting and masking results in faster code. For slow path code I > prefer bitfields because this results in much more compact source code. Despite claims to the contrary bit tests and masking are often a lot simpler, and you don't need the duplicate struct layouts and tests verifying that you get them right as it is almost impossible to write code that only works on LE or BE. E.g. the patch below would simplify the stream status quite a bit. It also fixes the assumption that the bitfield endianess is the same as the byte endianess, which is not true in general (although I think it is for all Linux supported architectures). diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c index f0eec4708ddd..41ed22f1ad0b 100644 --- a/drivers/scsi/scsi_debug.c +++ b/drivers/scsi/scsi_debug.c @@ -5528,7 +5528,8 @@ static int resp_get_stream_status(struct scsi_cmnd *scp, offset += 8, stream_id++) { struct scsi_stream_status *stream_status = (void *)arr + offset; - stream_status->perm = stream_id < PERMANENT_STREAM_COUNT; + if (stream_id < PERMANENT_STREAM_COUNT) + stream_status->flags |= SCSI_STREAM_STATUS_PERM; put_unaligned_be16(stream_id, &stream_status->stream_identifier); stream_status->rel_lifetime = stream_id + 1; diff --git a/drivers/scsi/scsi_proto_test.c b/drivers/scsi/scsi_proto_test.c index c093389edabb..d61302e7715a 100644 --- a/drivers/scsi/scsi_proto_test.c +++ b/drivers/scsi/scsi_proto_test.c @@ -22,15 +22,6 @@ static void test_scsi_proto(struct kunit *test) KUNIT_EXPECT_EQ(test, d.desc.params[0] + 0, 0xe4); KUNIT_EXPECT_EQ(test, d.desc.params[1] + 0, 0xe3); - static const union { - struct scsi_stream_status s; - u8 arr[sizeof(struct scsi_stream_status)]; - } ss = { .arr = { 0x80, 0, 0x12, 0x34, 0x3f } }; - KUNIT_EXPECT_EQ(test, ss.s.perm + 0, 1); - KUNIT_EXPECT_EQ(test, get_unaligned_be16(&ss.s.stream_identifier), - 0x1234); - KUNIT_EXPECT_EQ(test, ss.s.rel_lifetime + 0, 0x3f); - static const union { struct scsi_stream_status_header h; u8 arr[sizeof(struct scsi_stream_status_header)]; diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c index 3f6e87705b62..deb8af152f13 100644 --- a/drivers/scsi/sd.c +++ b/drivers/scsi/sd.c @@ -3215,7 +3215,7 @@ static bool sd_is_perm_stream(struct scsi_disk *sdkp, unsigned int stream_id) return false; if (get_unaligned_be32(&buf.h.len) < sizeof(struct scsi_stream_status)) return false; - return buf.s.perm; + return buf.s.flags & SCSI_STREAM_STATUS_PERM; } static void sd_read_io_hints(struct scsi_disk *sdkp, unsigned char *buffer) diff --git a/include/scsi/scsi_proto.h b/include/scsi/scsi_proto.h index f64385cde5b9..88c1d950be54 100644 --- a/include/scsi/scsi_proto.h +++ b/include/scsi/scsi_proto.h @@ -319,26 +319,10 @@ static_assert(sizeof(struct scsi_io_group_descriptor) == 16); /* SCSI stream status descriptor */ struct scsi_stream_status { -#if defined(__BIG_ENDIAN) - u8 perm: 1; - u8 reserved1: 7; -#elif defined(__LITTLE_ENDIAN) - u8 reserved1: 7; - u8 perm: 1; -#else -#error -#endif - u8 reserved2; + u8 flags; +#define SCSI_STREAM_STATUS_PERM (1u << 7) __be16 stream_identifier; -#if defined(__BIG_ENDIAN) - u8 reserved3: 2; - u8 rel_lifetime: 6; -#elif defined(__LITTLE_ENDIAN) - u8 rel_lifetime: 6; - u8 reserved3: 2; -#else -#error -#endif + u8 rel_lifetime; u8 reserved4[3]; };
diff --git a/drivers/scsi/scsi_proto_test.c b/drivers/scsi/scsi_proto_test.c index c093389edabb..2fc5c6523184 100644 --- a/drivers/scsi/scsi_proto_test.c +++ b/drivers/scsi/scsi_proto_test.c @@ -30,14 +30,6 @@ static void test_scsi_proto(struct kunit *test) KUNIT_EXPECT_EQ(test, get_unaligned_be16(&ss.s.stream_identifier), 0x1234); KUNIT_EXPECT_EQ(test, ss.s.rel_lifetime + 0, 0x3f); - - static const union { - struct scsi_stream_status_header h; - u8 arr[sizeof(struct scsi_stream_status_header)]; - } sh = { .arr = { 1, 2, 3, 4, 0, 0, 5, 6 } }; - KUNIT_EXPECT_EQ(test, get_unaligned_be32(&sh.h.len), 0x1020304); - KUNIT_EXPECT_EQ(test, get_unaligned_be16(&sh.h.number_of_open_streams), - 0x506); } static struct kunit_case scsi_proto_test_cases[] = { diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c index 950d8c9fb884..3f6e87705b62 100644 --- a/drivers/scsi/sd.c +++ b/drivers/scsi/sd.c @@ -3215,7 +3215,7 @@ static bool sd_is_perm_stream(struct scsi_disk *sdkp, unsigned int stream_id) return false; if (get_unaligned_be32(&buf.h.len) < sizeof(struct scsi_stream_status)) return false; - return buf.h.stream_status[0].perm; + return buf.s.perm; } static void sd_read_io_hints(struct scsi_disk *sdkp, unsigned char *buffer) diff --git a/include/scsi/scsi_proto.h b/include/scsi/scsi_proto.h index aeca37816506..bc8f2b2226be 100644 --- a/include/scsi/scsi_proto.h +++ b/include/scsi/scsi_proto.h @@ -349,7 +349,6 @@ struct scsi_stream_status_header { __be32 len; /* length in bytes of stream_status[] array. */ u16 reserved; __be16 number_of_open_streams; - DECLARE_FLEX_ARRAY(struct scsi_stream_status, stream_status); }; static_assert(sizeof(struct scsi_stream_status_header) == 8);