VLA removal, device_handler and COMMAND_SIZE

VLA removal, device_handler and COMMAND_SIZE

[Stephen Kitt]

[-- Attachment #1: Type: text/plain, Size: 1789 bytes --]

Hi,

I’ve been looking into removing some VLAs from device_handler drivers,
prompted by https://lkml.org/lkml/2018/3/7/621

The uses in question here are quite straightforward, e.g. in
drivers/scsi/device_handler/scsi_dh_alua.c:

	u8 cdb[COMMAND_SIZE(MAINTENANCE_IN)];

There’s no trivial way of keeping the compiler happy with -Wvla though here,
at least not while keeping the behaviour strictly identical. I’ve come up
with two approaches, and I’m curious whether they’re appropriate or if
there’s a better way...

The first approach is to use MAX_COMMAND_SIZE instead; this wastes a few
bytes on the stack here and there, and stays reasonably maintainable.

The second approach might be symptomatic of a twisted mind, and involves
replacing COMMAND_SIZE so that it can be calculated at compile time when the
opcode is known:

/*
 * SCSI command sizes are as follows, in bytes, for fixed size commands, per
 * group: 6, 10, 10, 12, 16, 12, 10, 10. The top three bits of an opcode
 * determine its group.
 * The size table is encoded into a 32-bit value by subtracting each value
 * from 16, resulting in a value of 1715488362
 * (6 << 28 + 6 << 24 + 4 << 20 + 0 << 16 + 4 << 12 + 6 << 8 + 6 << 4 + 10).
 * Command group 3 is reserved and should never be used.
 */
#define COMMAND_SIZE(opcode) \
       (16 - (15 & (1715488362 >> (4 * (((opcode) >> 5) & 7)))))


This has the side-effect of making some of the call sites more complex, and
the macro itself isn’t the most maintainer-friendly. It does mean we can drop
BLK_SCSI_REQUEST from drivers/target/Kconfig so it’s not all bad...

Both patches will follow in reply to this email, I’ll let more familiar
developers judge which is appropriate (if any).

Regards,

Stephen

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH] device_handler: remove VLAs

[Stephen Kitt]

In preparation to enabling -Wvla, remove VLAs and replace them with
fixed-length arrays instead.

scsi_dh_{alua,emc,rdac} use variable-length array declarations to
store command blocks, with the appropriate size as determined by
COMMAND_SIZE. This patch replaces these with fixed-sized arrays using
MAX_COMMAND_SIZE, so that the array size can be determined at compile
time.

This was prompted by https://lkml.org/lkml/2018/3/7/621

Signed-off-by: Stephen Kitt <steve@sk2.org>
---
 drivers/scsi/device_handler/scsi_dh_alua.c | 8 ++++----
 drivers/scsi/device_handler/scsi_dh_emc.c  | 2 +-
 drivers/scsi/device_handler/scsi_dh_rdac.c | 2 +-
 3 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/scsi/device_handler/scsi_dh_alua.c b/drivers/scsi/device_handler/scsi_dh_alua.c
index 4b44325d1a82..982a52528a1c 100644
--- a/drivers/scsi/device_handler/scsi_dh_alua.c
+++ b/drivers/scsi/device_handler/scsi_dh_alua.c
@@ -138,12 +138,12 @@ static void release_port_group(struct kref *kref)
 static int submit_rtpg(struct scsi_device *sdev, unsigned char *buff,
 		       int bufflen, struct scsi_sense_hdr *sshdr, int flags)
 {
-	u8 cdb[COMMAND_SIZE(MAINTENANCE_IN)];
+	u8 cdb[MAX_COMMAND_SIZE];
 	int req_flags = REQ_FAILFAST_DEV | REQ_FAILFAST_TRANSPORT |
 		REQ_FAILFAST_DRIVER;
 
 	/* Prepare the command. */
-	memset(cdb, 0x0, COMMAND_SIZE(MAINTENANCE_IN));
+	memset(cdb, 0x0, MAX_COMMAND_SIZE);
 	cdb[0] = MAINTENANCE_IN;
 	if (!(flags & ALUA_RTPG_EXT_HDR_UNSUPP))
 		cdb[1] = MI_REPORT_TARGET_PGS | MI_EXT_HDR_PARAM_FMT;
@@ -166,7 +166,7 @@ static int submit_rtpg(struct scsi_device *sdev, unsigned char *buff,
 static int submit_stpg(struct scsi_device *sdev, int group_id,
 		       struct scsi_sense_hdr *sshdr)
 {
-	u8 cdb[COMMAND_SIZE(MAINTENANCE_OUT)];
+	u8 cdb[MAX_COMMAND_SIZE];
 	unsigned char stpg_data[8];
 	int stpg_len = 8;
 	int req_flags = REQ_FAILFAST_DEV | REQ_FAILFAST_TRANSPORT |
@@ -178,7 +178,7 @@ static int submit_stpg(struct scsi_device *sdev, int group_id,
 	put_unaligned_be16(group_id, &stpg_data[6]);
 
 	/* Prepare the command. */
-	memset(cdb, 0x0, COMMAND_SIZE(MAINTENANCE_OUT));
+	memset(cdb, 0x0, MAX_COMMAND_SIZE);
 	cdb[0] = MAINTENANCE_OUT;
 	cdb[1] = MO_SET_TARGET_PGS;
 	put_unaligned_be32(stpg_len, &cdb[6]);
diff --git a/drivers/scsi/device_handler/scsi_dh_emc.c b/drivers/scsi/device_handler/scsi_dh_emc.c
index 6a2792f3a37e..95c47909a58f 100644
--- a/drivers/scsi/device_handler/scsi_dh_emc.c
+++ b/drivers/scsi/device_handler/scsi_dh_emc.c
@@ -249,7 +249,7 @@ static int send_trespass_cmd(struct scsi_device *sdev,
 			    struct clariion_dh_data *csdev)
 {
 	unsigned char *page22;
-	unsigned char cdb[COMMAND_SIZE(MODE_SELECT)];
+	unsigned char cdb[MAX_COMMAND_SIZE];
 	int err, res = SCSI_DH_OK, len;
 	struct scsi_sense_hdr sshdr;
 	u64 req_flags = REQ_FAILFAST_DEV | REQ_FAILFAST_TRANSPORT |
diff --git a/drivers/scsi/device_handler/scsi_dh_rdac.c b/drivers/scsi/device_handler/scsi_dh_rdac.c
index 7af31a1247ee..d27fabae8ddd 100644
--- a/drivers/scsi/device_handler/scsi_dh_rdac.c
+++ b/drivers/scsi/device_handler/scsi_dh_rdac.c
@@ -533,7 +533,7 @@ static void send_mode_select(struct work_struct *work)
 	int err = SCSI_DH_OK, retry_cnt = RDAC_RETRY_COUNT;
 	struct rdac_queue_data *tmp, *qdata;
 	LIST_HEAD(list);
-	unsigned char cdb[COMMAND_SIZE(MODE_SELECT_10)];
+	unsigned char cdb[MAX_COMMAND_SIZE];
 	struct scsi_sense_hdr sshdr;
 	unsigned int data_size;
 	u64 req_flags = REQ_FAILFAST_DEV | REQ_FAILFAST_TRANSPORT |
-- 
2.11.0

[PATCH] scsi: resolve COMMAND_SIZE at compile time

[Stephen Kitt]

COMMAND_SIZE currently uses an array of values in block/scsi_ioctl.c.
A number of device_handler functions use this to initialise arrays,
and this is flagged by -Wvla.

This patch replaces COMMAND_SIZE with a variant using a formula which
can be resolved at compile time in cases where the opcode is fixed,
resolving the array size and avoiding the VLA. The downside is that
the code is less maintainable and that some call sites end up having
more complex generated code.

Since scsi_command_size_tbl is dropped, we can remove the dependency
on BLK_SCSI_REQUEST from drivers/target/Kconfig.

This was prompted by https://lkml.org/lkml/2018/3/7/621

Signed-off-by: Stephen Kitt <steve@sk2.org>
---
 block/scsi_ioctl.c         |  8 --------
 drivers/target/Kconfig     |  1 -
 include/scsi/scsi_common.h | 13 +++++++++++--
 3 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/block/scsi_ioctl.c b/block/scsi_ioctl.c
index 60b471f8621b..b9e176e537be 100644
--- a/block/scsi_ioctl.c
+++ b/block/scsi_ioctl.c
@@ -41,14 +41,6 @@ struct blk_cmd_filter {
 
 static struct blk_cmd_filter blk_default_cmd_filter;
 
-/* Command group 3 is reserved and should never be used.  */
-const unsigned char scsi_command_size_tbl[8] =
-{
-	6, 10, 10, 12,
-	16, 12, 10, 10
-};
-EXPORT_SYMBOL(scsi_command_size_tbl);
-
 #include <scsi/sg.h>
 
 static int sg_get_version(int __user *p)
diff --git a/drivers/target/Kconfig b/drivers/target/Kconfig
index 4c44d7bed01a..b5f05b60cf06 100644
--- a/drivers/target/Kconfig
+++ b/drivers/target/Kconfig
@@ -4,7 +4,6 @@ menuconfig TARGET_CORE
 	depends on SCSI && BLOCK
 	select CONFIGFS_FS
 	select CRC_T10DIF
-	select BLK_SCSI_REQUEST # only for scsi_command_size_tbl..
 	select SGL_ALLOC
 	default n
 	help
diff --git a/include/scsi/scsi_common.h b/include/scsi/scsi_common.h
index 731ac09ed231..48d950666376 100644
--- a/include/scsi/scsi_common.h
+++ b/include/scsi/scsi_common.h
@@ -15,8 +15,17 @@ scsi_varlen_cdb_length(const void *hdr)
 	return ((struct scsi_varlen_cdb_hdr *)hdr)->additional_cdb_length + 8;
 }
 
-extern const unsigned char scsi_command_size_tbl[8];
-#define COMMAND_SIZE(opcode) scsi_command_size_tbl[((opcode) >> 5) & 7]
+/*
+ * SCSI command sizes are as follows, in bytes, for fixed size commands, per
+ * group: 6, 10, 10, 12, 16, 12, 10, 10. The top three bits of an opcode
+ * determine its group.
+ * The size table is encoded into a 32-bit value by subtracting each value
+ * from 16, resulting in a value of 1715488362
+ * (6 << 28 + 6 << 24 + 4 << 20 + 0 << 16 + 4 << 12 + 6 << 8 + 6 << 4 + 10).
+ * Command group 3 is reserved and should never be used.
+ */
+#define COMMAND_SIZE(opcode) \
+	(16 - (15 & (1715488362 >> (4 * (((opcode) >> 5) & 7)))))
 
 static inline unsigned
 scsi_command_size(const unsigned char *cmnd)
-- 
2.11.0

Re: [PATCH] scsi: resolve COMMAND_SIZE at compile time

[Bart Van Assche]

T24gRnJpLCAyMDE4LTAzLTA5IGF0IDIzOjMzICswMTAwLCBTdGVwaGVuIEtpdHQgd3JvdGU6DQo+
ICsvKg0KPiArICogU0NTSSBjb21tYW5kIHNpemVzIGFyZSBhcyBmb2xsb3dzLCBpbiBieXRlcywg
Zm9yIGZpeGVkIHNpemUgY29tbWFuZHMsIHBlcg0KPiArICogZ3JvdXA6IDYsIDEwLCAxMCwgMTIs
IDE2LCAxMiwgMTAsIDEwLiBUaGUgdG9wIHRocmVlIGJpdHMgb2YgYW4gb3Bjb2RlDQo+ICsgKiBk
ZXRlcm1pbmUgaXRzIGdyb3VwLg0KPiArICogVGhlIHNpemUgdGFibGUgaXMgZW5jb2RlZCBpbnRv
IGEgMzItYml0IHZhbHVlIGJ5IHN1YnRyYWN0aW5nIGVhY2ggdmFsdWUNCj4gKyAqIGZyb20gMTYs
IHJlc3VsdGluZyBpbiBhIHZhbHVlIG9mIDE3MTU0ODgzNjINCj4gKyAqICg2IDw8IDI4ICsgNiA8
PCAyNCArIDQgPDwgMjAgKyAwIDw8IDE2ICsgNCA8PCAxMiArIDYgPDwgOCArIDYgPDwgNCArIDEw
KS4NCj4gKyAqIENvbW1hbmQgZ3JvdXAgMyBpcyByZXNlcnZlZCBhbmQgc2hvdWxkIG5ldmVyIGJl
IHVzZWQuDQo+ICsgKi8NCj4gKyNkZWZpbmUgQ09NTUFORF9TSVpFKG9wY29kZSkgXA0KPiArCSgx
NiAtICgxNSAmICgxNzE1NDg4MzYyID4+ICg0ICogKCgob3Bjb2RlKSA+PiA1KSAmIDcpKSkpKQ0K
DQpUbyBtZSB0aGlzIHNlZW1zIGhhcmQgdG8gcmVhZCBhbmQgaGFyZCB0byB2ZXJpZnkuIENvdWxk
IHRoaXMgaGF2ZSBiZWVuIHdyaXR0ZW4NCmFzIGEgY29tYmluYXRpb24gb2YgdGVybmFyeSBleHBy
ZXNzaW9ucywgZS5nLiB1c2luZyBhIGdjYyBzdGF0ZW1lbnQgZXhwcmVzc2lvbg0KdG8gZW5zdXJl
IHRoYXQgb3Bjb2RlIGlzIGV2YWx1YXRlZCBvbmNlPw0KDQpUaGFua3MsDQoNCkJhcnQuDQoNCg0K

Re: [PATCH] scsi: resolve COMMAND_SIZE at compile time

[Bart Van Assche]

On Fri, 2018-03-09 at 23:33 +0100, Stephen Kitt wrote:
> +/*
> + * SCSI command sizes are as follows, in bytes, for fixed size commands, per
> + * group: 6, 10, 10, 12, 16, 12, 10, 10. The top three bits of an opcode
> + * determine its group.
> + * The size table is encoded into a 32-bit value by subtracting each value
> + * from 16, resulting in a value of 1715488362
> + * (6 << 28 + 6 << 24 + 4 << 20 + 0 << 16 + 4 << 12 + 6 << 8 + 6 << 4 + 10).
> + * Command group 3 is reserved and should never be used.
> + */
> +#define COMMAND_SIZE(opcode) \
> +	(16 - (15 & (1715488362 >> (4 * (((opcode) >> 5) & 7)))))

To me this seems hard to read and hard to verify. Could this have been written
as a combination of ternary expressions, e.g. using a gcc statement expression
to ensure that opcode is evaluated once?

Thanks,

Bart.

Re: [PATCH] device_handler: remove VLAs

[Bart Van Assche]

T24gRnJpLCAyMDE4LTAzLTA5IGF0IDIzOjMyICswMTAwLCBTdGVwaGVuIEtpdHQgd3JvdGU6DQo+
IEluIHByZXBhcmF0aW9uIHRvIGVuYWJsaW5nIC1XdmxhLCByZW1vdmUgVkxBcyBhbmQgcmVwbGFj
ZSB0aGVtIHdpdGgNCj4gZml4ZWQtbGVuZ3RoIGFycmF5cyBpbnN0ZWFkLg0KPiANCj4gc2NzaV9k
aF97YWx1YSxlbWMscmRhY30gdXNlIHZhcmlhYmxlLWxlbmd0aCBhcnJheSBkZWNsYXJhdGlvbnMg
dG8NCj4gc3RvcmUgY29tbWFuZCBibG9ja3MsIHdpdGggdGhlIGFwcHJvcHJpYXRlIHNpemUgYXMg
ZGV0ZXJtaW5lZCBieQ0KPiBDT01NQU5EX1NJWkUuIFRoaXMgcGF0Y2ggcmVwbGFjZXMgdGhlc2Ug
d2l0aCBmaXhlZC1zaXplZCBhcnJheXMgdXNpbmcNCj4gTUFYX0NPTU1BTkRfU0laRSwgc28gdGhh
dCB0aGUgYXJyYXkgc2l6ZSBjYW4gYmUgZGV0ZXJtaW5lZCBhdCBjb21waWxlDQo+IHRpbWUuDQoN
CklmIENPTU1BTkRfU0laRSgpIGlzIGV2YWx1YXRlZCBhdCBjb21waWxlIHRpbWUsIGRvIHdlIHN0
aWxsIG5lZWQgdGhpcyBwYXRjaD8NCg0KVGhhbmtzLA0KDQpCYXJ0Lg0KDQoNCg==

Re: [PATCH] device_handler: remove VLAs

[Bart Van Assche]

On Fri, 2018-03-09 at 23:32 +0100, Stephen Kitt wrote:
> In preparation to enabling -Wvla, remove VLAs and replace them with
> fixed-length arrays instead.
> 
> scsi_dh_{alua,emc,rdac} use variable-length array declarations to
> store command blocks, with the appropriate size as determined by
> COMMAND_SIZE. This patch replaces these with fixed-sized arrays using
> MAX_COMMAND_SIZE, so that the array size can be determined at compile
> time.

If COMMAND_SIZE() is evaluated at compile time, do we still need this patch?

Thanks,

Bart.

Re: [PATCH] device_handler: remove VLAs

[Stephen Kitt]

[-- Attachment #1: Type: text/plain, Size: 1050 bytes --]

Hi Bart,

On Fri, 9 Mar 2018 22:48:10 +0000, Bart Van Assche <Bart.VanAssche@wdc.com>
wrote:
> On Fri, 2018-03-09 at 23:32 +0100, Stephen Kitt wrote:
> > In preparation to enabling -Wvla, remove VLAs and replace them with
> > fixed-length arrays instead.
> > 
> > scsi_dh_{alua,emc,rdac} use variable-length array declarations to
> > store command blocks, with the appropriate size as determined by
> > COMMAND_SIZE. This patch replaces these with fixed-sized arrays using
> > MAX_COMMAND_SIZE, so that the array size can be determined at compile
> > time.  
> 
> If COMMAND_SIZE() is evaluated at compile time, do we still need this patch?

The two patches I sent were supposed to be alternative solutions; see
https://marc.info/?l=linux-scsi&m=152063671005295&w=2 for the introduction (I
seem to have messed up the headers, so the mails didn’t end up threaded
properly).

The MAX_COMMAND_SIZE approach is nice and simple, but I thought it worth
eliminating scsi_command_size_tbl while I was at it...

Regards,

Stephen

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] scsi: resolve COMMAND_SIZE at compile time

[Stephen Kitt]

[-- Attachment #1: Type: text/plain, Size: 1691 bytes --]

Hi Bart,

On Fri, 9 Mar 2018 22:47:12 +0000, Bart Van Assche <Bart.VanAssche@wdc.com>
wrote:
> On Fri, 2018-03-09 at 23:33 +0100, Stephen Kitt wrote:
> > +/*
> > + * SCSI command sizes are as follows, in bytes, for fixed size commands,
> > per
> > + * group: 6, 10, 10, 12, 16, 12, 10, 10. The top three bits of an opcode
> > + * determine its group.
> > + * The size table is encoded into a 32-bit value by subtracting each
> > value
> > + * from 16, resulting in a value of 1715488362
> > + * (6 << 28 + 6 << 24 + 4 << 20 + 0 << 16 + 4 << 12 + 6 << 8 + 6 << 4 +
> > 10).
> > + * Command group 3 is reserved and should never be used.
> > + */
> > +#define COMMAND_SIZE(opcode) \
> > +	(16 - (15 & (1715488362 >> (4 * (((opcode) >> 5) & 7)))))  
> 
> To me this seems hard to read and hard to verify. Could this have been
> written as a combination of ternary expressions, e.g. using a gcc statement
> expression to ensure that opcode is evaluated once?

That’s what I’d tried initially, e.g.

#define COMMAND_SIZE(opcode) ({ \
int index = ((opcode) >> 5) & 7; \
index == 0 ? 6 : (index == 4 ? 16 : index == 3 || index == 5 ? 12 : 10); \
})

But gcc still reckons that results in a VLA, defeating the initial purpose of
the exercise.

Does it help if I make the magic value construction clearer?

#define SCSI_COMMAND_SIZE_TBL (	\
	   (16 -  6)		\
	+ ((16 - 10) <<  4)	\
	+ ((16 - 10) <<  8)	\
	+ ((16 - 12) << 12)	\
	+ ((16 - 16) << 16)	\
	+ ((16 - 12) << 20)	\
	+ ((16 - 10) << 24)	\
	+ ((16 - 10) << 28))

#define COMMAND_SIZE(opcode)						\
  (16 - (15 & (SCSI_COMMAND_SIZE_TBL >> (4 * (((opcode) >> 5) & 7)))))

Regards,

Stephen

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] scsi: resolve COMMAND_SIZE at compile time

[James Bottomley]

[-- Attachment #1: Type: text/plain, Size: 2092 bytes --]

On Sat, 2018-03-10 at 14:29 +0100, Stephen Kitt wrote:
> Hi Bart,
> 
> On Fri, 9 Mar 2018 22:47:12 +0000, Bart Van Assche <Bart.VanAssche@wd
> c.com>
> wrote:
> > 
> > On Fri, 2018-03-09 at 23:33 +0100, Stephen Kitt wrote:
> > > 
> > > +/*
> > > + * SCSI command sizes are as follows, in bytes, for fixed size
> > > commands,
> > > per
> > > + * group: 6, 10, 10, 12, 16, 12, 10, 10. The top three bits of
> > > an opcode
> > > + * determine its group.
> > > + * The size table is encoded into a 32-bit value by subtracting
> > > each
> > > value
> > > + * from 16, resulting in a value of 1715488362
> > > + * (6 << 28 + 6 << 24 + 4 << 20 + 0 << 16 + 4 << 12 + 6 << 8 + 6
> > > << 4 +
> > > 10).
> > > + * Command group 3 is reserved and should never be used.
> > > + */
> > > +#define COMMAND_SIZE(opcode) \
> > > +	(16 - (15 & (1715488362 >> (4 * (((opcode) >> 5) &
> > > 7)))))  
> > 
> > To me this seems hard to read and hard to verify. Could this have
> > been
> > written as a combination of ternary expressions, e.g. using a gcc
> > statement
> > expression to ensure that opcode is evaluated once?
> 
> That’s what I’d tried initially, e.g.
> 
> #define COMMAND_SIZE(opcode) ({ \
> int index = ((opcode) >> 5) & 7; \
> index == 0 ? 6 : (index == 4 ? 16 : index == 3 || index == 5 ? 12 :
> 10); \
> })
> 
> But gcc still reckons that results in a VLA, defeating the initial
> purpose of
> the exercise.
> 
> Does it help if I make the magic value construction clearer?
> 
> #define SCSI_COMMAND_SIZE_TBL (	\
> 	   (16 -  6)		\
> 	+ ((16 - 10) <<  4)	\
> 	+ ((16 - 10) <<  8)	\
> 	+ ((16 - 12) << 12)	\
> 	+ ((16 - 16) << 16)	\
> 	+ ((16 - 12) << 20)	\
> 	+ ((16 - 10) << 24)	\
> 	+ ((16 - 10) << 28))
> 
> #define COMMAND_SIZE(opcode)						
> \
>   (16 - (15 & (SCSI_COMMAND_SIZE_TBL >> (4 * (((opcode) >> 5) &
> 7)))))

Couldn't we do the less clever thing of making the array a static const
and moving it to a header?  That way the compiler should be able to
work it out at compile time.

James

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH] scsi: resolve COMMAND_SIZE at compile time

[Douglas Gilbert]

On 2018-03-10 03:49 PM, James Bottomley wrote:
> On Sat, 2018-03-10 at 14:29 +0100, Stephen Kitt wrote:
>> Hi Bart,
>>
>> On Fri, 9 Mar 2018 22:47:12 +0000, Bart Van Assche <Bart.VanAssche@wd
>> c.com>
>> wrote:
>>>
>>> On Fri, 2018-03-09 at 23:33 +0100, Stephen Kitt wrote:
>>>>
>>>> +/*
>>>> + * SCSI command sizes are as follows, in bytes, for fixed size
>>>> commands,
>>>> per
>>>> + * group: 6, 10, 10, 12, 16, 12, 10, 10. The top three bits of
>>>> an opcode
>>>> + * determine its group.
>>>> + * The size table is encoded into a 32-bit value by subtracting
>>>> each
>>>> value
>>>> + * from 16, resulting in a value of 1715488362
>>>> + * (6 << 28 + 6 << 24 + 4 << 20 + 0 << 16 + 4 << 12 + 6 << 8 + 6
>>>> << 4 +
>>>> 10).
>>>> + * Command group 3 is reserved and should never be used.
>>>> + */
>>>> +#define COMMAND_SIZE(opcode) \
>>>> +	(16 - (15 & (1715488362 >> (4 * (((opcode) >> 5) &
>>>> 7)))))
>>>
>>> To me this seems hard to read and hard to verify. Could this have
>>> been
>>> written as a combination of ternary expressions, e.g. using a gcc
>>> statement
>>> expression to ensure that opcode is evaluated once?
>>
>> That’s what I’d tried initially, e.g.
>>
>> #define COMMAND_SIZE(opcode) ({ \
>> int index = ((opcode) >> 5) & 7; \
>> index == 0 ? 6 : (index == 4 ? 16 : index == 3 || index == 5 ? 12 :
>> 10); \
>> })
>>
>> But gcc still reckons that results in a VLA, defeating the initial
>> purpose of
>> the exercise.
>>
>> Does it help if I make the magic value construction clearer?
>>
>> #define SCSI_COMMAND_SIZE_TBL (	\
>> 	   (16 -  6)		\
>> 	+ ((16 - 10) <<  4)	\
>> 	+ ((16 - 10) <<  8)	\
>> 	+ ((16 - 12) << 12)	\
>> 	+ ((16 - 16) << 16)	\
>> 	+ ((16 - 12) << 20)	\
>> 	+ ((16 - 10) << 24)	\
>> 	+ ((16 - 10) << 28))
>>
>> #define COMMAND_SIZE(opcode)						
>> \
>>    (16 - (15 & (SCSI_COMMAND_SIZE_TBL >> (4 * (((opcode) >> 5) &
>> 7)))))
> 
> Couldn't we do the less clever thing of making the array a static const
> and moving it to a header?  That way the compiler should be able to
> work it out at compile time.

And maybe add a comment that as of now (SPC-5 rev 19), COMMAND_SIZE is not
valid for opcodes 0x7e and 0x7f plus everything above and including 0xc0.
The latter ones are vendor specific and are loosely constrained, probably
all even numbered lengths in the closed range: [6,260].


If the SCSI command sets want to keep up with NVMe, they may want to think
about how they can gainfully use cdb_s that are > 64 bytes long. WRITE
SCATTERED got into SBC-4 but READ GATHERED didn't, due to lack of interest.
The READ GATHERED proposed was a bidi command, but it could have been a
a simpler data-in command with a looong cdb (holding LBA, number_of_blocks
pairs).

Doug Gilbert

Re: [PATCH] scsi: resolve COMMAND_SIZE at compile time

[Stephen Kitt]

[-- Attachment #1: Type: text/plain, Size: 2619 bytes --]

On Sat, 10 Mar 2018 12:49:17 -0800, James Bottomley <jejb@linux.vnet.ibm.com>
wrote:
> On Sat, 2018-03-10 at 14:29 +0100, Stephen Kitt wrote:
> > On Fri, 9 Mar 2018 22:47:12 +0000, Bart Van Assche <Bart.VanAssche@wd  
> > c.com>  
> > wrote:  
> > > On Fri, 2018-03-09 at 23:33 +0100, Stephen Kitt wrote:  
> > > > +/*
> > > > + * SCSI command sizes are as follows, in bytes, for fixed size
> > > > commands,
> > > > per
> > > > + * group: 6, 10, 10, 12, 16, 12, 10, 10. The top three bits of
> > > > an opcode
> > > > + * determine its group.
> > > > + * The size table is encoded into a 32-bit value by subtracting
> > > > each
> > > > value
> > > > + * from 16, resulting in a value of 1715488362
> > > > + * (6 << 28 + 6 << 24 + 4 << 20 + 0 << 16 + 4 << 12 + 6 << 8 + 6
> > > > << 4 +
> > > > 10).
> > > > + * Command group 3 is reserved and should never be used.
> > > > + */
> > > > +#define COMMAND_SIZE(opcode) \
> > > > +	(16 - (15 & (1715488362 >> (4 * (((opcode) >> 5) &
> > > > 7)))))    
> > > 
> > > To me this seems hard to read and hard to verify. Could this have
> > > been
> > > written as a combination of ternary expressions, e.g. using a gcc
> > > statement
> > > expression to ensure that opcode is evaluated once?  
> > 
> > That’s what I’d tried initially, e.g.
> > 
> > #define COMMAND_SIZE(opcode) ({ \
> > int index = ((opcode) >> 5) & 7; \
> > index == 0 ? 6 : (index == 4 ? 16 : index == 3 || index == 5 ? 12 :
> > 10); \
> > })
> > 
> > But gcc still reckons that results in a VLA, defeating the initial
> > purpose of
> > the exercise.
> > 
> > Does it help if I make the magic value construction clearer?
> > 
> > #define SCSI_COMMAND_SIZE_TBL (	\
> > 	   (16 -  6)		\
> > 	+ ((16 - 10) <<  4)	\
> > 	+ ((16 - 10) <<  8)	\
> > 	+ ((16 - 12) << 12)	\
> > 	+ ((16 - 16) << 16)	\
> > 	+ ((16 - 12) << 20)	\
> > 	+ ((16 - 10) << 24)	\
> > 	+ ((16 - 10) << 28))
> > 
> > #define
> > COMMAND_SIZE(opcode) \
> >   (16 - (15 & (SCSI_COMMAND_SIZE_TBL >> (4 * (((opcode) >> 5) &
> > 7)))))  
> 
> Couldn't we do the less clever thing of making the array a static const
> and moving it to a header?  That way the compiler should be able to
> work it out at compile time.

I hoped so too, but const-ifying a variable doesn’t make it a compile-time
constant (even though the optimiser can resolve it at build-time), so GCC
still considers uses such as

	u8 cdb[COMMAND_SIZE(MAINTENANCE_IN)];

as variable-length arrays, which C90 forbids (and which we’re trying to
eliminate here).

Regards,

Stephen

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] scsi: resolve COMMAND_SIZE at compile time

[Stephen Kitt]

[-- Attachment #1: Type: text/plain, Size: 2619 bytes --]

On Sat, 10 Mar 2018 12:49:17 -0800, James Bottomley <jejb@linux.vnet.ibm.com>
wrote:
> On Sat, 2018-03-10 at 14:29 +0100, Stephen Kitt wrote:
> > On Fri, 9 Mar 2018 22:47:12 +0000, Bart Van Assche <Bart.VanAssche@wd  
> > c.com>  
> > wrote:  
> > > On Fri, 2018-03-09 at 23:33 +0100, Stephen Kitt wrote:  
> > > > +/*
> > > > + * SCSI command sizes are as follows, in bytes, for fixed size
> > > > commands,
> > > > per
> > > > + * group: 6, 10, 10, 12, 16, 12, 10, 10. The top three bits of
> > > > an opcode
> > > > + * determine its group.
> > > > + * The size table is encoded into a 32-bit value by subtracting
> > > > each
> > > > value
> > > > + * from 16, resulting in a value of 1715488362
> > > > + * (6 << 28 + 6 << 24 + 4 << 20 + 0 << 16 + 4 << 12 + 6 << 8 + 6
> > > > << 4 +
> > > > 10).
> > > > + * Command group 3 is reserved and should never be used.
> > > > + */
> > > > +#define COMMAND_SIZE(opcode) \
> > > > +	(16 - (15 & (1715488362 >> (4 * (((opcode) >> 5) &
> > > > 7)))))    
> > > 
> > > To me this seems hard to read and hard to verify. Could this have
> > > been
> > > written as a combination of ternary expressions, e.g. using a gcc
> > > statement
> > > expression to ensure that opcode is evaluated once?  
> > 
> > That’s what I’d tried initially, e.g.
> > 
> > #define COMMAND_SIZE(opcode) ({ \
> > int index = ((opcode) >> 5) & 7; \
> > index == 0 ? 6 : (index == 4 ? 16 : index == 3 || index == 5 ? 12 :
> > 10); \
> > })
> > 
> > But gcc still reckons that results in a VLA, defeating the initial
> > purpose of
> > the exercise.
> > 
> > Does it help if I make the magic value construction clearer?
> > 
> > #define SCSI_COMMAND_SIZE_TBL (	\
> > 	   (16 -  6)		\
> > 	+ ((16 - 10) <<  4)	\
> > 	+ ((16 - 10) <<  8)	\
> > 	+ ((16 - 12) << 12)	\
> > 	+ ((16 - 16) << 16)	\
> > 	+ ((16 - 12) << 20)	\
> > 	+ ((16 - 10) << 24)	\
> > 	+ ((16 - 10) << 28))
> > 
> > #define
> > COMMAND_SIZE(opcode) \
> >   (16 - (15 & (SCSI_COMMAND_SIZE_TBL >> (4 * (((opcode) >> 5) &
> > 7)))))  
> 
> Couldn't we do the less clever thing of making the array a static const
> and moving it to a header?  That way the compiler should be able to
> work it out at compile time.

I hoped so too, but const-ifying a variable doesn’t make it a compile-time
constant (even though the optimiser can resolve it at build-time), so GCC
still considers uses such as

	u8 cdb[COMMAND_SIZE(MAINTENANCE_IN)];

as variable-length arrays, which C90 forbids (and which we’re trying to
eliminate here).

Regards,

Stephen

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] device_handler: remove VLAs

[Hannes Reinecke]

On 03/09/2018 11:32 PM, Stephen Kitt wrote:
> In preparation to enabling -Wvla, remove VLAs and replace them with
> fixed-length arrays instead.
> 
> scsi_dh_{alua,emc,rdac} use variable-length array declarations to
> store command blocks, with the appropriate size as determined by
> COMMAND_SIZE. This patch replaces these with fixed-sized arrays using
> MAX_COMMAND_SIZE, so that the array size can be determined at compile
> time.
> 
> This was prompted by https://lkml.org/lkml/2018/3/7/621
> 
> Signed-off-by: Stephen Kitt <steve@sk2.org>
> ---
>  drivers/scsi/device_handler/scsi_dh_alua.c | 8 ++++----
>  drivers/scsi/device_handler/scsi_dh_emc.c  | 2 +-
>  drivers/scsi/device_handler/scsi_dh_rdac.c | 2 +-
>  3 files changed, 6 insertions(+), 6 deletions(-)
> 
Reviewed-by: Hannes Reinecke <hare@suse.com>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		   Teamlead Storage & Networking
hare@suse.de			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)

Re: [PATCH] device_handler: remove VLAs

[Bart Van Assche]

T24gU2F0LCAyMDE4LTAzLTEwIGF0IDE0OjE0ICswMTAwLCBTdGVwaGVuIEtpdHQgd3JvdGU6DQo+
IFRoZSB0d28gcGF0Y2hlcyBJIHNlbnQgd2VyZSBzdXBwb3NlZCB0byBiZSBhbHRlcm5hdGl2ZSBz
b2x1dGlvbnM7IHNlZQ0KPiBodHRwczovL21hcmMuaW5mby8/bD1saW51eC1zY3NpJm09MTUyMDYz
NjcxMDA1Mjk1Jnc9MiBmb3IgdGhlIGludHJvZHVjdGlvbiAoSQ0KPiBzZWVtIHRvIGhhdmUgbWVz
c2VkIHVwIHRoZSBoZWFkZXJzLCBzbyB0aGUgbWFpbHMgZGlkbuKAmXQgZW5kIHVwIHRocmVhZGVk
DQo+IHByb3Blcmx5KS4NCg0KVGhlIHR3byBwYXRjaGVzIGFycml2ZWQgaW4gbXkgaW5ib3ggc2V2
ZXJhbCBtaW51dGVzIGJlZm9yZSB0aGUgY292ZXIgbGV0dGVyLiBJbg0KdGhlIGUtbWFpbCBoZWFk
ZXIgb2YgdGhlIGNvdmVyIGxldHRlciBJIGZvdW5kIHRoZSBmb2xsb3dpbmc6DQoNClgtR3JleWxp
c3Q6IGRlbGF5ZWQgMTgxMCBzZWNvbmRzIGJ5IHBvc3RncmV5LTEuMjcgYXQgdmdlci5rZXJuZWwu
b3JnOyBGcmksIDA5IE1hciAyMDE4IDE4OjA1OjA4IEVTVA0KDQpEb2VzIHRoaXMgbWVhbiB0aGF0
IHRoZSBkZWxheSBoYXBwZW5lZCBkdWUgdG8gdmdlciBzZXJ2ZXIncyBhbnRpLXNwYW0gYWxnb3Jp
dGhtPw0KDQpCYXJ0Lg0KDQoNCg0K

Re: [PATCH] device_handler: remove VLAs

[Bart Van Assche]

On Sat, 2018-03-10 at 14:14 +0100, Stephen Kitt wrote:
> The two patches I sent were supposed to be alternative solutions; see
> https://marc.info/?l=linux-scsi&m=152063671005295&w=2 for the introduction (I
> seem to have messed up the headers, so the mails didn’t end up threaded
> properly).

The two patches arrived in my inbox several minutes before the cover letter. In
the e-mail header of the cover letter I found the following:

X-Greylist: delayed 1810 seconds by postgrey-1.27 at vger.kernel.org; Fri, 09 Mar 2018 18:05:08 EST

Does this mean that the delay happened due to vger server's anti-spam algorithm?

Bart.

Re: [PATCH] device_handler: remove VLAs

[Stephen Kitt]

[-- Attachment #1: Type: text/plain, Size: 841 bytes --]

On Mon, 12 Mar 2018 15:41:26 +0000, Bart Van Assche <Bart.VanAssche@wdc.com>
wrote:
> On Sat, 2018-03-10 at 14:14 +0100, Stephen Kitt wrote:
> > The two patches I sent were supposed to be alternative solutions; see
> > https://marc.info/?l=linux-scsi&m=152063671005295&w=2 for the
> > introduction (I seem to have messed up the headers, so the mails didn’t
> > end up threaded properly).  
> 
> The two patches arrived in my inbox several minutes before the cover
> letter. In the e-mail header of the cover letter I found the following:
> 
> X-Greylist: delayed 1810 seconds by postgrey-1.27 at vger.kernel.org; Fri,
> 09 Mar 2018 18:05:08 EST
> 
> Does this mean that the delay happened due to vger server's anti-spam
> algorithm?

That’s right, the greylisting part of its anti-spam measures.

Regards,

Stephen

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] device_handler: remove VLAs

[Martin K. Petersen]

Stephen,

> In preparation to enabling -Wvla, remove VLAs and replace them with
> fixed-length arrays instead.
>
> scsi_dh_{alua,emc,rdac} use variable-length array declarations to
> store command blocks, with the appropriate size as determined by
> COMMAND_SIZE. This patch replaces these with fixed-sized arrays using
> MAX_COMMAND_SIZE, so that the array size can be determined at compile
> time.

Applied to 4.17/scsi-queue. Thank you!

-- 
Martin K. Petersen	Oracle Linux Engineering

RE: [PATCH] scsi: resolve COMMAND_SIZE at compile time

[David Laight]

From: Stephen Kitt
> Sent: 09 March 2018 22:34
>=20
> COMMAND_SIZE currently uses an array of values in block/scsi_ioctl.c.
> A number of device_handler functions use this to initialise arrays,
> and this is flagged by -Wvla.
>=20
> This patch replaces COMMAND_SIZE with a variant using a formula which
> can be resolved at compile time in cases where the opcode is fixed,
> resolving the array size and avoiding the VLA. The downside is that
> the code is less maintainable and that some call sites end up having
> more complex generated code.
>=20
> Since scsi_command_size_tbl is dropped, we can remove the dependency
> on BLK_SCSI_REQUEST from drivers/target/Kconfig.
>=20
> This was prompted by https://lkml.org/lkml/2018/3/7/621
>=20
> Signed-off-by: Stephen Kitt <steve@sk2.org>
> ---
>  block/scsi_ioctl.c         |  8 --------
>  drivers/target/Kconfig     |  1 -
>  include/scsi/scsi_common.h | 13 +++++++++++--
>  3 files changed, 11 insertions(+), 11 deletions(-)
>=20
> diff --git a/block/scsi_ioctl.c b/block/scsi_ioctl.c
> index 60b471f8621b..b9e176e537be 100644
> --- a/block/scsi_ioctl.c
> +++ b/block/scsi_ioctl.c
> @@ -41,14 +41,6 @@ struct blk_cmd_filter {
>=20
>  static struct blk_cmd_filter blk_default_cmd_filter;
>=20
> -/* Command group 3 is reserved and should never be used.  */
> -const unsigned char scsi_command_size_tbl[8] =3D
> -{
> -	6, 10, 10, 12,
> -	16, 12, 10, 10
> -};
> -EXPORT_SYMBOL(scsi_command_size_tbl);
> -
>  #include <scsi/sg.h>
>=20
>  static int sg_get_version(int __user *p)
> diff --git a/drivers/target/Kconfig b/drivers/target/Kconfig
> index 4c44d7bed01a..b5f05b60cf06 100644
> --- a/drivers/target/Kconfig
> +++ b/drivers/target/Kconfig
> @@ -4,7 +4,6 @@ menuconfig TARGET_CORE
>  	depends on SCSI && BLOCK
>  	select CONFIGFS_FS
>  	select CRC_T10DIF
> -	select BLK_SCSI_REQUEST # only for scsi_command_size_tbl..
>  	select SGL_ALLOC
>  	default n
>  	help
> diff --git a/include/scsi/scsi_common.h b/include/scsi/scsi_common.h
> index 731ac09ed231..48d950666376 100644
> --- a/include/scsi/scsi_common.h
> +++ b/include/scsi/scsi_common.h
> @@ -15,8 +15,17 @@ scsi_varlen_cdb_length(const void *hdr)
>  	return ((struct scsi_varlen_cdb_hdr *)hdr)->additional_cdb_length + 8;
>  }
>=20
> -extern const unsigned char scsi_command_size_tbl[8];
> -#define COMMAND_SIZE(opcode) scsi_command_size_tbl[((opcode) >> 5) & 7]
> +/*
> + * SCSI command sizes are as follows, in bytes, for fixed size commands,=
 per
> + * group: 6, 10, 10, 12, 16, 12, 10, 10. The top three bits of an opcode
> + * determine its group.
> + * The size table is encoded into a 32-bit value by subtracting each val=
ue
> + * from 16, resulting in a value of 1715488362
> + * (6 << 28 + 6 << 24 + 4 << 20 + 0 << 16 + 4 << 12 + 6 << 8 + 6 << 4 + =
10).
> + * Command group 3 is reserved and should never be used.
> + */
> +#define COMMAND_SIZE(opcode) \
> +	(16 - (15 & (1715488362 >> (4 * (((opcode) >> 5) & 7)))))

Why not:
	(6 + (15 & (0x446a6440u ....

Specifying the constant in hex makes it more obvious.
It is a shame about the 16, but an offset is easier than the negate.

	David

RE: [PATCH] scsi: resolve COMMAND_SIZE at compile time

[David Laight]

From: Stephen Kitt
> Sent: 09 March 2018 22:34
> 
> COMMAND_SIZE currently uses an array of values in block/scsi_ioctl.c.
> A number of device_handler functions use this to initialise arrays,
> and this is flagged by -Wvla.
> 
> This patch replaces COMMAND_SIZE with a variant using a formula which
> can be resolved at compile time in cases where the opcode is fixed,
> resolving the array size and avoiding the VLA. The downside is that
> the code is less maintainable and that some call sites end up having
> more complex generated code.
> 
> Since scsi_command_size_tbl is dropped, we can remove the dependency
> on BLK_SCSI_REQUEST from drivers/target/Kconfig.
> 
> This was prompted by https://lkml.org/lkml/2018/3/7/621
> 
> Signed-off-by: Stephen Kitt <steve@sk2.org>
> ---
>  block/scsi_ioctl.c         |  8 --------
>  drivers/target/Kconfig     |  1 -
>  include/scsi/scsi_common.h | 13 +++++++++++--
>  3 files changed, 11 insertions(+), 11 deletions(-)
> 
> diff --git a/block/scsi_ioctl.c b/block/scsi_ioctl.c
> index 60b471f8621b..b9e176e537be 100644
> --- a/block/scsi_ioctl.c
> +++ b/block/scsi_ioctl.c
> @@ -41,14 +41,6 @@ struct blk_cmd_filter {
> 
>  static struct blk_cmd_filter blk_default_cmd_filter;
> 
> -/* Command group 3 is reserved and should never be used.  */
> -const unsigned char scsi_command_size_tbl[8] =
> -{
> -	6, 10, 10, 12,
> -	16, 12, 10, 10
> -};
> -EXPORT_SYMBOL(scsi_command_size_tbl);
> -
>  #include <scsi/sg.h>
> 
>  static int sg_get_version(int __user *p)
> diff --git a/drivers/target/Kconfig b/drivers/target/Kconfig
> index 4c44d7bed01a..b5f05b60cf06 100644
> --- a/drivers/target/Kconfig
> +++ b/drivers/target/Kconfig
> @@ -4,7 +4,6 @@ menuconfig TARGET_CORE
>  	depends on SCSI && BLOCK
>  	select CONFIGFS_FS
>  	select CRC_T10DIF
> -	select BLK_SCSI_REQUEST # only for scsi_command_size_tbl..
>  	select SGL_ALLOC
>  	default n
>  	help
> diff --git a/include/scsi/scsi_common.h b/include/scsi/scsi_common.h
> index 731ac09ed231..48d950666376 100644
> --- a/include/scsi/scsi_common.h
> +++ b/include/scsi/scsi_common.h
> @@ -15,8 +15,17 @@ scsi_varlen_cdb_length(const void *hdr)
>  	return ((struct scsi_varlen_cdb_hdr *)hdr)->additional_cdb_length + 8;
>  }
> 
> -extern const unsigned char scsi_command_size_tbl[8];
> -#define COMMAND_SIZE(opcode) scsi_command_size_tbl[((opcode) >> 5) & 7]
> +/*
> + * SCSI command sizes are as follows, in bytes, for fixed size commands, per
> + * group: 6, 10, 10, 12, 16, 12, 10, 10. The top three bits of an opcode
> + * determine its group.
> + * The size table is encoded into a 32-bit value by subtracting each value
> + * from 16, resulting in a value of 1715488362
> + * (6 << 28 + 6 << 24 + 4 << 20 + 0 << 16 + 4 << 12 + 6 << 8 + 6 << 4 + 10).
> + * Command group 3 is reserved and should never be used.
> + */
> +#define COMMAND_SIZE(opcode) \
> +	(16 - (15 & (1715488362 >> (4 * (((opcode) >> 5) & 7)))))

Why not:
	(6 + (15 & (0x446a6440u ....

Specifying the constant in hex makes it more obvious.
It is a shame about the 16, but an offset is easier than the negate.

	David