diff mbox series

[1/6] usb:gadget:mass-storage: Improve the signature of SCSI handler function

Message ID 20210626211820.107310-2-i.kononenko@yadro.com
State New
Headers show
Series [1/6] usb:gadget:mass-storage: Improve the signature of SCSI handler function | expand

Commit Message

Igor Kononenko June 26, 2021, 9:18 p.m. UTC 
SCSI command handlers currently have an ambiguous return value. This
return value may indicate the length of the data written to the response
buffer and the command's processing status. Thus, the understanding of
command handling may be implicit.

After this patch, the output buffer's size will be set in the
'data_size_to_handle' field of 'struct fsg_common', and the command
handler's return value indicates only the processing status.

Tested: By probing the USBGadget Mass-Storage on the YADRO VEGMAN
BMC(AST2500) sample, each SCSI command was sent through HOST->BMC; the
USBGadget MassStorage debug print showed all sent commands works
properly.

Signed-off-by: Igor Kononenko <i.kononenko@yadro.com>
---
 drivers/usb/gadget/function/f_mass_storage.c | 176 ++++++++++---------
 1 file changed, 95 insertions(+), 81 deletions(-)

Comments

Alan Stern June 27, 2021, 2:18 p.m. UTC | #1 
On Sun, Jun 27, 2021 at 12:18:14AM +0300, Igor Kononenko wrote:
> SCSI command handlers currently have an ambiguous return value. This


(I dislike very much this way of writing patch descriptions.  Unless
the reader has already looked at the email subject line and remembers
that this patch affects the mass-storage gadget, he will think the
sentence above is talking about command handlers in the SCSI core -- a
completely different part of the kernel.  When writing patch
descriptions, please do not assume that the reader already knows what
the patch is about.)

> return value may indicate the length of the data written to the response

> buffer and the command's processing status. Thus, the understanding of

> command handling may be implicit.


The return value is _not_ ambiguous.  If the value is >= 0 then it is
a data length, otherwise it is a status.  Yes, this is implicit, but it
is a very common pattern used throughout the kernel and everyone
understands it.

> After this patch, the output buffer's size will be set in the

> 'data_size_to_handle' field of 'struct fsg_common', and the command

> handler's return value indicates only the processing status.


What is the reason for making this change?  Does it fix any problems
or prepare the way for any future patches?  It seems like this is
completely unnecessary.

Alan Stern

> Tested: By probing the USBGadget Mass-Storage on the YADRO VEGMAN

> BMC(AST2500) sample, each SCSI command was sent through HOST->BMC; the

> USBGadget MassStorage debug print showed all sent commands works

> properly.

> 

> Signed-off-by: Igor Kononenko <i.kononenko@yadro.com>
Alan Stern June 27, 2021, 4:39 p.m. UTC | #2 
On Sun, Jun 27, 2021 at 06:32:03PM +0300, i.kononenko wrote:
> Good morning, Alan!

> 

> First of all, thank you for your time to review my first patchset for 

> the Linux Kernel and valuable advice on the right way of patchwriting!

> 

> On 27.06.2021 17:18, Alan Stern wrote:

> > On Sun, Jun 27, 2021 at 12:18:14AM +0300, Igor Kononenko wrote:

> >> SCSI command handlers currently have an ambiguous return value. This

> > 

> > (I dislike very much this way of writing patch descriptions.  Unless

> > the reader has already looked at the email subject line and remembers

> > that this patch affects the mass-storage gadget, he will think the

> > sentence above is talking about command handlers in the SCSI core -- a

> > completely different part of the kernel.  When writing patch

> > descriptions, please do not assume that the reader already knows what

> > the patch is about.)

> > 

> >> return value may indicate the length of the data written to the response

> >> buffer and the command's processing status. Thus, the understanding of

> >> command handling may be implicit.

> 

> First of all, thank you for your time to review my first patchset for the

> Linux Kernel and valuable advice on the right way of patchwriting!

> 

> I noticed that the status/datasize return value pattern is pervasive for 

> Linux and used through many subsystems. But for the f_mass_storage.c,

> such approach use case is not documented anywhere, and implementation has 

> too many magic-constant, e.g.

> ```

> static int do_inquiry(struct fsg_common *common, struct fsg_buffhd *bh)

> {

>    ....

>    return 36;

> }

> ```

> IMHO, this way is not giving the developer an explicit understanding of 

> 'what is the 36' and its origin.

> If moving to the suggested way is unwanted, I'd keep the implementation 

> as is with additional documentation for each function where uses this 

> approach.


Since every one of the command handler functions uses this convention, 
it would be wasteful to have separate documentation of the return value 
for each function.  A single documentation comment that covers all the 
command handlers would be acceptable.

> Additionally, I guess, define clarify macros of return value instead of 

> magic numbers is required.


If you want, okay.  That should go in a separate patch from the 
documentation patch.

Also, since the return values are different for each command handler, I 
suggest that the macro definitions be placed along with the handler 
functions and not in a separate header file.  Having a separate file for 
these macros would not make any sense, because the values do not need to 
be shared across multiple functions or source files.

> > The return value is _not_ ambiguous.  If the value is >= 0 then it is

> > a data length, otherwise it is a status.  Yes, this is implicit, but it

> > is a very common pattern used throughout the kernel and everyone

> > understands it.

> > 

> >> After this patch, the output buffer's size will be set in the

> >> 'data_size_to_handle' field of 'struct fsg_common', and the command

> >> handler's return value indicates only the processing status.

> > 

> > What is the reason for making this change?  Does it fix any problems

> > or prepare the way for any future patches?  It seems like this is

> > completely unnecessary.

> 

> Yes, the patch uses as part of the incoming implementation of refactoring

> 'usb:gadget:mass-storage:scsi' command handling.


That incoming implementation uses the refactored command handling but 
doesn't depend on the refactoring.  It could just as easily use the 
existing command handling.

> I believed the suggested improvement would be useful for the community as 

> an improvement of code.


Unless you can provide a convincing reason for this change, it doesn't 
seem like an improvement to me.  It's no easier to read or understand, 
and it doesn't improve execution speed on a critical pathway.  It just 
seems like pointless code churn.

Alan Stern
diff mbox series

Patch

diff --git a/drivers/usb/gadget/function/f_mass_storage.c b/drivers/usb/gadget/function/f_mass_storage.c
index 4a4703634a2a..e9a7f87b4df3 100644
--- a/drivers/usb/gadget/function/f_mass_storage.c
+++ b/drivers/usb/gadget/function/f_mass_storage.c
@@ -296,6 +296,7 @@  struct fsg_common {
 	enum data_direction	data_dir;
 	u32			data_size;
 	u32			data_size_from_cmnd;
+	u32			data_size_to_handle;
 	u32			tag;
 	u32			residue;
 	u32			usb_amount_left;
@@ -1066,7 +1067,8 @@  static int do_inquiry(struct fsg_common *common, struct fsg_buffhd *bh)
 		memset(buf, 0, 36);
 		buf[0] = TYPE_NO_LUN;	/* Unsupported, no device-type */
 		buf[4] = 31;		/* Additional length */
-		return 36;
+		common->data_size_to_handle = 36;
+		return 0;
 	}
 
 	buf[0] = curlun->cdrom ? TYPE_ROM : TYPE_DISK;
@@ -1083,7 +1085,8 @@  static int do_inquiry(struct fsg_common *common, struct fsg_buffhd *bh)
 	else
 		memcpy(buf + 8, common->inquiry_string,
 		       sizeof(common->inquiry_string));
-	return 36;
+	common->data_size_to_handle = 36;
+	return 0;
 }
 
 static int do_request_sense(struct fsg_common *common, struct fsg_buffhd *bh)
@@ -1136,7 +1139,8 @@  static int do_request_sense(struct fsg_common *common, struct fsg_buffhd *bh)
 	buf[7] = 18 - 8;			/* Additional sense length */
 	buf[12] = ASC(sd);
 	buf[13] = ASCQ(sd);
-	return 18;
+	common->data_size_to_handle = 18;
+	return 0;
 }
 
 static int do_read_capacity(struct fsg_common *common, struct fsg_buffhd *bh)
@@ -1155,7 +1159,8 @@  static int do_read_capacity(struct fsg_common *common, struct fsg_buffhd *bh)
 	put_unaligned_be32(curlun->num_sectors - 1, &buf[0]);
 						/* Max logical block */
 	put_unaligned_be32(curlun->blksize, &buf[4]);/* Block length */
-	return 8;
+	common->data_size_to_handle = 8;
+	return 0;
 }
 
 static int do_read_header(struct fsg_common *common, struct fsg_buffhd *bh)
@@ -1177,7 +1182,8 @@  static int do_read_header(struct fsg_common *common, struct fsg_buffhd *bh)
 	memset(buf, 0, 8);
 	buf[0] = 0x01;		/* 2048 bytes of user data, rest is EC */
 	store_cdrom_address(&buf[4], msf, lba);
-	return 8;
+	common->data_size_to_handle = 8;
+	return 0;
 }
 
 static int do_read_toc(struct fsg_common *common, struct fsg_buffhd *bh)
@@ -1204,7 +1210,8 @@  static int do_read_toc(struct fsg_common *common, struct fsg_buffhd *bh)
 	buf[13] = 0x16;			/* Lead-out track is data */
 	buf[14] = 0xAA;			/* Lead-out track number */
 	store_cdrom_address(&buf[16], msf, curlun->num_sectors);
-	return 20;
+	common->data_size_to_handle = 20;
+	return 0;
 }
 
 static int do_mode_sense(struct fsg_common *common, struct fsg_buffhd *bh)
@@ -1290,7 +1297,8 @@  static int do_mode_sense(struct fsg_common *common, struct fsg_buffhd *bh)
 		buf0[0] = len - 1;
 	else
 		put_unaligned_be16(len - 2, buf0);
-	return len;
+	common->data_size_to_handle = len;
+	return 0;
 }
 
 static int do_start_stop(struct fsg_common *common)
@@ -1381,7 +1389,8 @@  static int do_read_format_capacities(struct fsg_common *common,
 						/* Number of blocks */
 	put_unaligned_be32(curlun->blksize, &buf[4]);/* Block length */
 	buf[4] = 0x02;				/* Current capacity */
-	return 12;
+	common->data_size_to_handle = 12;
+	return 0;
 }
 
 static int do_mode_select(struct fsg_common *common, struct fsg_buffhd *bh)
@@ -1796,7 +1805,7 @@  static int do_scsi_command(struct fsg_common *common)
 {
 	struct fsg_buffhd	*bh;
 	int			rc;
-	int			reply = -EINVAL;
+	int			status = -EINVAL;
 	int			i;
 	static char		unknown[16];
 
@@ -1813,104 +1822,107 @@  static int do_scsi_command(struct fsg_common *common)
 	common->short_packet_received = 0;
 
 	down_read(&common->filesem);	/* We're using the backing file */
+	/* flash all unhandled data */
+	common->data_size_to_handle = 0;
+
 	switch (common->cmnd[0]) {
 
 	case INQUIRY:
 		common->data_size_from_cmnd = common->cmnd[4];
-		reply = check_command(common, 6, DATA_DIR_TO_HOST,
+		status = check_command(common, 6, DATA_DIR_TO_HOST,
 				      (1<<4), 0,
 				      "INQUIRY");
-		if (reply == 0)
-			reply = do_inquiry(common, bh);
+		if (status == 0)
+			status = do_inquiry(common, bh);
 		break;
 
 	case MODE_SELECT:
 		common->data_size_from_cmnd = common->cmnd[4];
-		reply = check_command(common, 6, DATA_DIR_FROM_HOST,
+		status = check_command(common, 6, DATA_DIR_FROM_HOST,
 				      (1<<1) | (1<<4), 0,
 				      "MODE SELECT(6)");
-		if (reply == 0)
-			reply = do_mode_select(common, bh);
+		if (status == 0)
+			status = do_mode_select(common, bh);
 		break;
 
 	case MODE_SELECT_10:
 		common->data_size_from_cmnd =
 			get_unaligned_be16(&common->cmnd[7]);
-		reply = check_command(common, 10, DATA_DIR_FROM_HOST,
+		status = check_command(common, 10, DATA_DIR_FROM_HOST,
 				      (1<<1) | (3<<7), 0,
 				      "MODE SELECT(10)");
-		if (reply == 0)
-			reply = do_mode_select(common, bh);
+		if (status == 0)
+			status = do_mode_select(common, bh);
 		break;
 
 	case MODE_SENSE:
 		common->data_size_from_cmnd = common->cmnd[4];
-		reply = check_command(common, 6, DATA_DIR_TO_HOST,
+		status = check_command(common, 6, DATA_DIR_TO_HOST,
 				      (1<<1) | (1<<2) | (1<<4), 0,
 				      "MODE SENSE(6)");
-		if (reply == 0)
-			reply = do_mode_sense(common, bh);
+		if (status == 0)
+			status = do_mode_sense(common, bh);
 		break;
 
 	case MODE_SENSE_10:
 		common->data_size_from_cmnd =
 			get_unaligned_be16(&common->cmnd[7]);
-		reply = check_command(common, 10, DATA_DIR_TO_HOST,
+		status = check_command(common, 10, DATA_DIR_TO_HOST,
 				      (1<<1) | (1<<2) | (3<<7), 0,
 				      "MODE SENSE(10)");
-		if (reply == 0)
-			reply = do_mode_sense(common, bh);
+		if (status == 0)
+			status = do_mode_sense(common, bh);
 		break;
 
 	case ALLOW_MEDIUM_REMOVAL:
 		common->data_size_from_cmnd = 0;
-		reply = check_command(common, 6, DATA_DIR_NONE,
+		status = check_command(common, 6, DATA_DIR_NONE,
 				      (1<<4), 0,
 				      "PREVENT-ALLOW MEDIUM REMOVAL");
-		if (reply == 0)
-			reply = do_prevent_allow(common);
+		if (status == 0)
+			status = do_prevent_allow(common);
 		break;
 
 	case READ_6:
 		i = common->cmnd[4];
 		common->data_size_from_cmnd = (i == 0) ? 256 : i;
-		reply = check_command_size_in_blocks(common, 6,
+		status = check_command_size_in_blocks(common, 6,
 				      DATA_DIR_TO_HOST,
 				      (7<<1) | (1<<4), 1,
 				      "READ(6)");
-		if (reply == 0)
-			reply = do_read(common);
+		if (status == 0)
+			status = do_read(common);
 		break;
 
 	case READ_10:
 		common->data_size_from_cmnd =
 				get_unaligned_be16(&common->cmnd[7]);
-		reply = check_command_size_in_blocks(common, 10,
+		status = check_command_size_in_blocks(common, 10,
 				      DATA_DIR_TO_HOST,
 				      (1<<1) | (0xf<<2) | (3<<7), 1,
 				      "READ(10)");
-		if (reply == 0)
-			reply = do_read(common);
+		if (status == 0)
+			status = do_read(common);
 		break;
 
 	case READ_12:
 		common->data_size_from_cmnd =
 				get_unaligned_be32(&common->cmnd[6]);
-		reply = check_command_size_in_blocks(common, 12,
+		status = check_command_size_in_blocks(common, 12,
 				      DATA_DIR_TO_HOST,
 				      (1<<1) | (0xf<<2) | (0xf<<6), 1,
 				      "READ(12)");
-		if (reply == 0)
-			reply = do_read(common);
+		if (status == 0)
+			status = do_read(common);
 		break;
 
 	case READ_CAPACITY:
 		common->data_size_from_cmnd = 8;
-		reply = check_command(common, 10, DATA_DIR_TO_HOST,
+		status = check_command(common, 10, DATA_DIR_TO_HOST,
 				      (0xf<<2) | (1<<8), 1,
 				      "READ CAPACITY");
-		if (reply == 0)
-			reply = do_read_capacity(common, bh);
+		if (status == 0)
+			status = do_read_capacity(common, bh);
 		break;
 
 	case READ_HEADER:
@@ -1918,11 +1930,11 @@  static int do_scsi_command(struct fsg_common *common)
 			goto unknown_cmnd;
 		common->data_size_from_cmnd =
 			get_unaligned_be16(&common->cmnd[7]);
-		reply = check_command(common, 10, DATA_DIR_TO_HOST,
+		status = check_command(common, 10, DATA_DIR_TO_HOST,
 				      (3<<7) | (0x1f<<1), 1,
 				      "READ HEADER");
-		if (reply == 0)
-			reply = do_read_header(common, bh);
+		if (status == 0)
+			status = do_read_header(common, bh);
 		break;
 
 	case READ_TOC:
@@ -1930,53 +1942,53 @@  static int do_scsi_command(struct fsg_common *common)
 			goto unknown_cmnd;
 		common->data_size_from_cmnd =
 			get_unaligned_be16(&common->cmnd[7]);
-		reply = check_command(common, 10, DATA_DIR_TO_HOST,
+		status = check_command(common, 10, DATA_DIR_TO_HOST,
 				      (7<<6) | (1<<1), 1,
 				      "READ TOC");
-		if (reply == 0)
-			reply = do_read_toc(common, bh);
+		if (status == 0)
+			status = do_read_toc(common, bh);
 		break;
 
 	case READ_FORMAT_CAPACITIES:
 		common->data_size_from_cmnd =
 			get_unaligned_be16(&common->cmnd[7]);
-		reply = check_command(common, 10, DATA_DIR_TO_HOST,
+		status = check_command(common, 10, DATA_DIR_TO_HOST,
 				      (3<<7), 1,
 				      "READ FORMAT CAPACITIES");
-		if (reply == 0)
-			reply = do_read_format_capacities(common, bh);
+		if (status == 0)
+			status = do_read_format_capacities(common, bh);
 		break;
 
 	case REQUEST_SENSE:
 		common->data_size_from_cmnd = common->cmnd[4];
-		reply = check_command(common, 6, DATA_DIR_TO_HOST,
+		status = check_command(common, 6, DATA_DIR_TO_HOST,
 				      (1<<4), 0,
 				      "REQUEST SENSE");
-		if (reply == 0)
-			reply = do_request_sense(common, bh);
+		if (status == 0)
+			status = do_request_sense(common, bh);
 		break;
 
 	case START_STOP:
 		common->data_size_from_cmnd = 0;
-		reply = check_command(common, 6, DATA_DIR_NONE,
+		status = check_command(common, 6, DATA_DIR_NONE,
 				      (1<<1) | (1<<4), 0,
 				      "START-STOP UNIT");
-		if (reply == 0)
-			reply = do_start_stop(common);
+		if (status == 0)
+			status = do_start_stop(common);
 		break;
 
 	case SYNCHRONIZE_CACHE:
 		common->data_size_from_cmnd = 0;
-		reply = check_command(common, 10, DATA_DIR_NONE,
+		status = check_command(common, 10, DATA_DIR_NONE,
 				      (0xf<<2) | (3<<7), 1,
 				      "SYNCHRONIZE CACHE");
-		if (reply == 0)
-			reply = do_synchronize_cache(common);
+		if (status == 0)
+			status = do_synchronize_cache(common);
 		break;
 
 	case TEST_UNIT_READY:
 		common->data_size_from_cmnd = 0;
-		reply = check_command(common, 6, DATA_DIR_NONE,
+		status = check_command(common, 6, DATA_DIR_NONE,
 				0, 1,
 				"TEST UNIT READY");
 		break;
@@ -1987,44 +1999,44 @@  static int do_scsi_command(struct fsg_common *common)
 	 */
 	case VERIFY:
 		common->data_size_from_cmnd = 0;
-		reply = check_command(common, 10, DATA_DIR_NONE,
+		status = check_command(common, 10, DATA_DIR_NONE,
 				      (1<<1) | (0xf<<2) | (3<<7), 1,
 				      "VERIFY");
-		if (reply == 0)
-			reply = do_verify(common);
+		if (status == 0)
+			status = do_verify(common);
 		break;
 
 	case WRITE_6:
 		i = common->cmnd[4];
 		common->data_size_from_cmnd = (i == 0) ? 256 : i;
-		reply = check_command_size_in_blocks(common, 6,
+		status = check_command_size_in_blocks(common, 6,
 				      DATA_DIR_FROM_HOST,
 				      (7<<1) | (1<<4), 1,
 				      "WRITE(6)");
-		if (reply == 0)
-			reply = do_write(common);
+		if (status == 0)
+			status = do_write(common);
 		break;
 
 	case WRITE_10:
 		common->data_size_from_cmnd =
 				get_unaligned_be16(&common->cmnd[7]);
-		reply = check_command_size_in_blocks(common, 10,
+		status = check_command_size_in_blocks(common, 10,
 				      DATA_DIR_FROM_HOST,
 				      (1<<1) | (0xf<<2) | (3<<7), 1,
 				      "WRITE(10)");
-		if (reply == 0)
-			reply = do_write(common);
+		if (status == 0)
+			status = do_write(common);
 		break;
 
 	case WRITE_12:
 		common->data_size_from_cmnd =
 				get_unaligned_be32(&common->cmnd[6]);
-		reply = check_command_size_in_blocks(common, 12,
+		status = check_command_size_in_blocks(common, 12,
 				      DATA_DIR_FROM_HOST,
 				      (1<<1) | (0xf<<2) | (0xf<<6), 1,
 				      "WRITE(12)");
-		if (reply == 0)
-			reply = do_write(common);
+		if (status == 0)
+			status = do_write(common);
 		break;
 
 	/*
@@ -2042,27 +2054,29 @@  static int do_scsi_command(struct fsg_common *common)
 unknown_cmnd:
 		common->data_size_from_cmnd = 0;
 		sprintf(unknown, "Unknown x%02x", common->cmnd[0]);
-		reply = check_command(common, common->cmnd_size,
+		status = check_command(common, common->cmnd_size,
 				      DATA_DIR_UNKNOWN, ~0, 0, unknown);
-		if (reply == 0) {
+		if (status == 0) {
 			common->curlun->sense_data = SS_INVALID_COMMAND;
-			reply = -EINVAL;
+			status = -EINVAL;
 		}
 		break;
 	}
 	up_read(&common->filesem);
 
-	if (reply == -EINTR || signal_pending(current))
+	if (status == -EINTR || signal_pending(current))
 		return -EINTR;
 
-	/* Set up the single reply buffer for finish_reply() */
-	if (reply == -EINVAL)
-		reply = 0;		/* Error reply length */
-	if (reply >= 0 && common->data_dir == DATA_DIR_TO_HOST) {
-		reply = min((u32)reply, common->data_size_from_cmnd);
-		bh->inreq->length = reply;
+	/* Set up the single status buffer for finish_reply() */
+	if (status == -EINVAL)
+		status = 0;		/* Error reply length */
+	if (status == 0 && common->data_dir == DATA_DIR_TO_HOST) {
+		common->data_size_to_handle =
+			min_t(u32, common->data_size_to_handle,
+			      common->data_size_from_cmnd);
+		bh->inreq->length = common->data_size_to_handle;
 		bh->state = BUF_STATE_FULL;
-		common->residue -= reply;
+		common->residue -= common->data_size_to_handle;
 	}				/* Otherwise it's already set */
 
 	return 0;