[PATCH] scsi: Fix scsi_get/set_resid() interface

[PATCH] scsi: Fix scsi_get/set_resid() interface

[Damien Le Moal]

struct scsi_cmnd cmd->req.resid_len which is returned and set
respectively by the helper functions scsi_get_resid() and
scsi_set_resid() is an unsigned int. Reflect this fact in the interface
of these helper functions.

Also fix compilation errors due to min() and max() type mismatch
introduced by this change in scsi debug code and usb transport code.

Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com>
---
 drivers/scsi/scsi_debug.c       | 4 ++--
 drivers/usb/storage/transport.c | 3 +--
 include/scsi/scsi_cmnd.h        | 4 ++--
 3 files changed, 5 insertions(+), 6 deletions(-)

diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
index d323523f5f9d..4daf2637c011 100644
--- a/drivers/scsi/scsi_debug.c
+++ b/drivers/scsi/scsi_debug.c
@@ -1025,7 +1025,7 @@ static int fill_from_dev_buffer(struct scsi_cmnd *scp, unsigned char *arr,
 static int p_fill_from_dev_buffer(struct scsi_cmnd *scp, const void *arr,
 				  int arr_len, unsigned int off_dst)
 {
-	int act_len, n;
+	unsigned int act_len, n;
 	struct scsi_data_buffer *sdb = &scp->sdb;
 	off_t skip = off_dst;
 
@@ -1039,7 +1039,7 @@ static int p_fill_from_dev_buffer(struct scsi_cmnd *scp, const void *arr,
 	pr_debug("%s: off_dst=%u, scsi_bufflen=%u, act_len=%u, resid=%d\n",
 		 __func__, off_dst, scsi_bufflen(scp), act_len,
 		 scsi_get_resid(scp));
-	n = (int)scsi_bufflen(scp) - ((int)off_dst + act_len);
+	n = scsi_bufflen(scp) - (off_dst + act_len);
 	scsi_set_resid(scp, min(scsi_get_resid(scp), n));
 	return 0;
 }
diff --git a/drivers/usb/storage/transport.c b/drivers/usb/storage/transport.c
index 96cb0409dd89..238a8088e17f 100644
--- a/drivers/usb/storage/transport.c
+++ b/drivers/usb/storage/transport.c
@@ -1284,8 +1284,7 @@ int usb_stor_Bulk_transport(struct scsi_cmnd *srb, struct us_data *us)
 
 		} else {
 			residue = min(residue, transfer_length);
-			scsi_set_resid(srb, max(scsi_get_resid(srb),
-			                                       (int) residue));
+			scsi_set_resid(srb, max(scsi_get_resid(srb), residue));
 		}
 	}
 
diff --git a/include/scsi/scsi_cmnd.h b/include/scsi/scsi_cmnd.h
index 91bd749a02f7..3772ae5d40cd 100644
--- a/include/scsi/scsi_cmnd.h
+++ b/include/scsi/scsi_cmnd.h
@@ -190,12 +190,12 @@ static inline unsigned scsi_bufflen(struct scsi_cmnd *cmd)
 	return cmd->sdb.length;
 }
 
-static inline void scsi_set_resid(struct scsi_cmnd *cmd, int resid)
+static inline void scsi_set_resid(struct scsi_cmnd *cmd, unsigned int resid)
 {
 	cmd->req.resid_len = resid;
 }
 
-static inline int scsi_get_resid(struct scsi_cmnd *cmd)
+static inline unsigned int scsi_get_resid(struct scsi_cmnd *cmd)
 {
 	return cmd->req.resid_len;
 }
-- 
2.21.0

Re: [PATCH] scsi: Fix scsi_get/set_resid() interface

[Bart Van Assche]

On 10/28/19 3:57 AM, Damien Le Moal wrote:
> struct scsi_cmnd cmd->req.resid_len which is returned and set
> respectively by the helper functions scsi_get_resid() and
> scsi_set_resid() is an unsigned int. Reflect this fact in the interface
> of these helper functions.
> [ ... ]
> -static inline void scsi_set_resid(struct scsi_cmnd *cmd, int resid)
> +static inline void scsi_set_resid(struct scsi_cmnd *cmd, unsigned int resid)
>   {
>   	cmd->req.resid_len = resid;
>   }
>   
> -static inline int scsi_get_resid(struct scsi_cmnd *cmd)
> +static inline unsigned int scsi_get_resid(struct scsi_cmnd *cmd)
>   {
>   	return cmd->req.resid_len;
>   }

 From the iSCSI RFC:

    SCSI-Presented Data Transfer Length (SPDTL) is the term this document
    uses (see Section 1.1 for definition) to represent the aggregate data
    length that the target SCSI layer attempts to transfer using the
    local iSCSI layer for a task.  Expected Data Transfer Length (EDTL)
    is the iSCSI term that represents the length of data that the iSCSI
    layer expects to transfer for a task.  EDTL is specified in the SCSI
    Command PDU.

    When SPDTL = EDTL for a task, the target iSCSI layer completes the
    task with no residuals.  Whenever SPDTL differs from EDTL for a task,
    that task is said to have a residual.

    If SPDTL > EDTL for a task, iSCSI Overflow MUST be signaled in the
    SCSI Response PDU as specified in [RFC3720].  The Residual Count MUST
    be set to the numerical value of (SPDTL - EDTL).

    If SPDTL < EDTL for a task, iSCSI Underflow MUST be signaled in the
    SCSI Response PDU as specified in [RFC3720].  The Residual Count MUST
    be set to the numerical value of (EDTL - SPDTL).

    Note that the Overflow and Underflow scenarios are independent of
    Data-In and Data-Out.  Either scenario is logically possible in
    either direction of data transfer.

If the residual is changed from signed into unsigned, how is a SCSI LLD 
expected to report the difference between residual overflow and residual 
underflow to the SCSI core?

Thanks,

Bart.

Re: [PATCH] scsi: Fix scsi_get/set_resid() interface

[Damien Le Moal]

Bart,

On 2019/10/28 21:38, Bart Van Assche wrote:
> On 10/28/19 3:57 AM, Damien Le Moal wrote:
>> struct scsi_cmnd cmd->req.resid_len which is returned and set
>> respectively by the helper functions scsi_get_resid() and
>> scsi_set_resid() is an unsigned int. Reflect this fact in the interface
>> of these helper functions.
>> [ ... ]
>> -static inline void scsi_set_resid(struct scsi_cmnd *cmd, int resid)
>> +static inline void scsi_set_resid(struct scsi_cmnd *cmd, unsigned int resid)
>>   {
>>   	cmd->req.resid_len = resid;
>>   }
>>   
>> -static inline int scsi_get_resid(struct scsi_cmnd *cmd)
>> +static inline unsigned int scsi_get_resid(struct scsi_cmnd *cmd)
>>   {
>>   	return cmd->req.resid_len;
>>   }
> 
>  From the iSCSI RFC:
> 
>     SCSI-Presented Data Transfer Length (SPDTL) is the term this document
>     uses (see Section 1.1 for definition) to represent the aggregate data
>     length that the target SCSI layer attempts to transfer using the
>     local iSCSI layer for a task.  Expected Data Transfer Length (EDTL)
>     is the iSCSI term that represents the length of data that the iSCSI
>     layer expects to transfer for a task.  EDTL is specified in the SCSI
>     Command PDU.
> 
>     When SPDTL = EDTL for a task, the target iSCSI layer completes the
>     task with no residuals.  Whenever SPDTL differs from EDTL for a task,
>     that task is said to have a residual.
> 
>     If SPDTL > EDTL for a task, iSCSI Overflow MUST be signaled in the
>     SCSI Response PDU as specified in [RFC3720].  The Residual Count MUST
>     be set to the numerical value of (SPDTL - EDTL).
> 
>     If SPDTL < EDTL for a task, iSCSI Underflow MUST be signaled in the
>     SCSI Response PDU as specified in [RFC3720].  The Residual Count MUST
>     be set to the numerical value of (EDTL - SPDTL).
> 
>     Note that the Overflow and Underflow scenarios are independent of
>     Data-In and Data-Out.  Either scenario is logically possible in
>     either direction of data transfer.
> 
> If the residual is changed from signed into unsigned, how is a SCSI LLD 
> expected to report the difference between residual overflow and residual 
> underflow to the SCSI core?

As mentioned in the commit message, cmd->req.resid_len where the resid
is stored is an unsigned int. And it has been an unsigned int forever as
far as I know.

So I think changing the interface to unsigned int makes sense in that
context. Also, unless I am reading this wrong, the above definition you
quote always lead to resid >= 0, so I do not see what problem you are
worried about. Could you elaborate your concerns please ?

> 
> Thanks,
> 
> Bart.
> 


-- 
Damien Le Moal
Western Digital Research

Re: [PATCH] scsi: Fix scsi_get/set_resid() interface

[Bart Van Assche]

On 10/29/19 1:17 AM, Damien Le Moal wrote:
> So I think changing the interface to unsigned int makes sense in that
> context. Also, unless I am reading this wrong, the above definition you
> quote always lead to resid >= 0, so I do not see what problem you are
> worried about. Could you elaborate your concerns please ?

Hi Damien,

SCSI LLDs handle residual overflow in at least two different ways. I 
think the iSCSI initiator handles this by reporting the DID_BAD_TARGET 
error code. In the tcm_loop driver I found the following, which I think 
is incorrect:

	set_host_byte(sc, DID_OK);
	if ((se_cmd->se_cmd_flags & SCF_OVERFLOW_BIT) ||
	    (se_cmd->se_cmd_flags & SCF_UNDERFLOW_BIT))
		scsi_set_resid(sc, se_cmd->residual_count);

Seeing this made me wonder what the best approach is for a SCSI LLD to 
report a residual overflow?

Thanks,

Bart.

Re: [PATCH] scsi: Fix scsi_get/set_resid() interface

[Douglas Gilbert]

On 2019-10-29 4:17 a.m., Damien Le Moal wrote:
> Bart,
> 
> On 2019/10/28 21:38, Bart Van Assche wrote:
>> On 10/28/19 3:57 AM, Damien Le Moal wrote:
>>> struct scsi_cmnd cmd->req.resid_len which is returned and set
>>> respectively by the helper functions scsi_get_resid() and
>>> scsi_set_resid() is an unsigned int. Reflect this fact in the interface
>>> of these helper functions.
>>> [ ... ]
>>> -static inline void scsi_set_resid(struct scsi_cmnd *cmd, int resid)
>>> +static inline void scsi_set_resid(struct scsi_cmnd *cmd, unsigned int resid)
>>>    {
>>>    	cmd->req.resid_len = resid;
>>>    }
>>>    
>>> -static inline int scsi_get_resid(struct scsi_cmnd *cmd)
>>> +static inline unsigned int scsi_get_resid(struct scsi_cmnd *cmd)
>>>    {
>>>    	return cmd->req.resid_len;
>>>    }
>>
>>   From the iSCSI RFC:
>>
>>      SCSI-Presented Data Transfer Length (SPDTL) is the term this document
>>      uses (see Section 1.1 for definition) to represent the aggregate data
>>      length that the target SCSI layer attempts to transfer using the
>>      local iSCSI layer for a task.  Expected Data Transfer Length (EDTL)
>>      is the iSCSI term that represents the length of data that the iSCSI
>>      layer expects to transfer for a task.  EDTL is specified in the SCSI
>>      Command PDU.
>>
>>      When SPDTL = EDTL for a task, the target iSCSI layer completes the
>>      task with no residuals.  Whenever SPDTL differs from EDTL for a task,
>>      that task is said to have a residual.
>>
>>      If SPDTL > EDTL for a task, iSCSI Overflow MUST be signaled in the
>>      SCSI Response PDU as specified in [RFC3720].  The Residual Count MUST
>>      be set to the numerical value of (SPDTL - EDTL).
>>
>>      If SPDTL < EDTL for a task, iSCSI Underflow MUST be signaled in the
>>      SCSI Response PDU as specified in [RFC3720].  The Residual Count MUST
>>      be set to the numerical value of (EDTL - SPDTL).
>>
>>      Note that the Overflow and Underflow scenarios are independent of
>>      Data-In and Data-Out.  Either scenario is logically possible in
>>      either direction of data transfer.
>>
>> If the residual is changed from signed into unsigned, how is a SCSI LLD
>> expected to report the difference between residual overflow and residual
>> underflow to the SCSI core?



> As mentioned in the commit message, cmd->req.resid_len where the resid
> is stored is an unsigned int. And it has been an unsigned int forever as
> far as I know.

If my memory serves, I put it in over 20 years ago, about the same time
as the sg v2 interface (struct sg_io_hdr) was implemented. And when I
put it in it was a (signed) int and this is why:

struct ccb_accept_targ3 {
     .....
     CAM_I32 cam_resid;    /* Transfer residual length: 2's comp */
     .....
};

[from cam3r03.pdf]

So it coped with both underflow and the less likely case of overflow.

Notice in:
    struct sg_io_hdr {
       .....
       int resid;  /* [o] dxfer_len - actual_transferred */
       ...
};

At the time the SCSI mid-level and the block layer didn't bother about
resid, only some LLDs which is where the sg driver picked it up from.

So some ignoramus changed it to unsigned since then, probably before
git strangled the kernel source tree.

> So I think changing the interface to unsigned int makes sense in that
> context. Also, unless I am reading this wrong, the above definition you
> quote always lead to resid >= 0, so I do not see what problem you are
> worried about. Could you elaborate your concerns please ?

Linux's internal representation is wrong and should be fixed to match
its exposed interface (struct sg_io_hdr) which predates the breakage.

Doug Gilbert

Re: [PATCH] scsi: Fix scsi_get/set_resid() interface

[Bart Van Assche]

On 2019-10-29 10:59, Douglas Gilbert wrote:
> Linux's internal representation is wrong and should be fixed to match
> its exposed interface (struct sg_io_hdr) which predates the breakage.

If resid_len is changed from unsigned int to int all SCSI core and SCSI
LLD code that uses resid_len will have to be reviewed to see whether it
handles residual overflow correctly.

Bart.

Re: [PATCH] scsi: Fix scsi_get/set_resid() interface

[kbuild test robot]

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

Hi Damien,

I love your patch! Perhaps something to improve:

[auto build test WARNING on scsi/for-next]
[also build test WARNING on v5.4-rc5 next-20191029]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Damien-Le-Moal/scsi-Fix-scsi_get-set_resid-interface/20191030-074824
base:   https://git.kernel.org/pub/scm/linux/kernel/git/jejb/scsi.git for-next
config: sh-allmodconfig (attached as .config)
compiler: sh4-linux-gcc (GCC) 7.4.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        GCC_VERSION=7.4.0 make.cross ARCH=sh 

If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

   In file included from include/linux/jiffies.h:7:0,
                    from drivers/usb/storage/ene_ub6250.c:2:
   drivers/usb/storage/ene_ub6250.c: In function 'ene_send_scsi_cmd':
   include/linux/kernel.h:842:29: warning: comparison of distinct pointer types lacks a cast
      (!!(sizeof((typeof(x) *)1 == (typeof(y) *)1)))
                                ^
   include/linux/kernel.h:856:4: note: in expansion of macro '__typecheck'
      (__typecheck(x, y) && __no_side_effects(x, y))
       ^~~~~~~~~~~
   include/linux/kernel.h:866:24: note: in expansion of macro '__safe_cmp'
     __builtin_choose_expr(__safe_cmp(x, y), \
                           ^~~~~~~~~~
   include/linux/kernel.h:882:19: note: in expansion of macro '__careful_cmp'
    #define max(x, y) __careful_cmp(x, y, >)
                      ^~~~~~~~~~~~~
>> drivers/usb/storage/ene_ub6250.c:563:28: note: in expansion of macro 'max'
       scsi_set_resid(us->srb, max(scsi_get_resid(us->srb),
                               ^~~

vim +/max +563 drivers/usb/storage/ene_ub6250.c

41e568d14ec0ac huajun li    2011-03-04  492  
41e568d14ec0ac huajun li    2011-03-04  493  static int ene_send_scsi_cmd(struct us_data *us, u8 fDir, void *buf, int use_sg)
41e568d14ec0ac huajun li    2011-03-04  494  {
41e568d14ec0ac huajun li    2011-03-04  495  	struct bulk_cb_wrap *bcb = (struct bulk_cb_wrap *) us->iobuf;
41e568d14ec0ac huajun li    2011-03-04  496  	struct bulk_cs_wrap *bcs = (struct bulk_cs_wrap *) us->iobuf;
41e568d14ec0ac huajun li    2011-03-04  497  
41e568d14ec0ac huajun li    2011-03-04  498  	int result;
41e568d14ec0ac huajun li    2011-03-04  499  	unsigned int residue;
41e568d14ec0ac huajun li    2011-03-04  500  	unsigned int cswlen = 0, partial = 0;
41e568d14ec0ac huajun li    2011-03-04  501  	unsigned int transfer_length = bcb->DataTransferLength;
41e568d14ec0ac huajun li    2011-03-04  502  
191648d03d2022 Joe Perches  2013-04-19  503  	/* usb_stor_dbg(us, "transport --- ene_send_scsi_cmd\n"); */
41e568d14ec0ac huajun li    2011-03-04  504  	/* send cmd to out endpoint */
41e568d14ec0ac huajun li    2011-03-04  505  	result = usb_stor_bulk_transfer_buf(us, us->send_bulk_pipe,
41e568d14ec0ac huajun li    2011-03-04  506  					    bcb, US_BULK_CB_WRAP_LEN, NULL);
41e568d14ec0ac huajun li    2011-03-04  507  	if (result != USB_STOR_XFER_GOOD) {
191648d03d2022 Joe Perches  2013-04-19  508  		usb_stor_dbg(us, "send cmd to out endpoint fail ---\n");
41e568d14ec0ac huajun li    2011-03-04  509  		return USB_STOR_TRANSPORT_ERROR;
41e568d14ec0ac huajun li    2011-03-04  510  	}
41e568d14ec0ac huajun li    2011-03-04  511  
41e568d14ec0ac huajun li    2011-03-04  512  	if (buf) {
41e568d14ec0ac huajun li    2011-03-04  513  		unsigned int pipe = fDir;
41e568d14ec0ac huajun li    2011-03-04  514  
41e568d14ec0ac huajun li    2011-03-04  515  		if (fDir  == FDIR_READ)
41e568d14ec0ac huajun li    2011-03-04  516  			pipe = us->recv_bulk_pipe;
41e568d14ec0ac huajun li    2011-03-04  517  		else
41e568d14ec0ac huajun li    2011-03-04  518  			pipe = us->send_bulk_pipe;
41e568d14ec0ac huajun li    2011-03-04  519  
41e568d14ec0ac huajun li    2011-03-04  520  		/* Bulk */
41e568d14ec0ac huajun li    2011-03-04  521  		if (use_sg) {
41e568d14ec0ac huajun li    2011-03-04  522  			result = usb_stor_bulk_srb(us, pipe, us->srb);
41e568d14ec0ac huajun li    2011-03-04  523  		} else {
41e568d14ec0ac huajun li    2011-03-04  524  			result = usb_stor_bulk_transfer_sg(us, pipe, buf,
41e568d14ec0ac huajun li    2011-03-04  525  						transfer_length, 0, &partial);
41e568d14ec0ac huajun li    2011-03-04  526  		}
41e568d14ec0ac huajun li    2011-03-04  527  		if (result != USB_STOR_XFER_GOOD) {
191648d03d2022 Joe Perches  2013-04-19  528  			usb_stor_dbg(us, "data transfer fail ---\n");
41e568d14ec0ac huajun li    2011-03-04  529  			return USB_STOR_TRANSPORT_ERROR;
41e568d14ec0ac huajun li    2011-03-04  530  		}
41e568d14ec0ac huajun li    2011-03-04  531  	}
41e568d14ec0ac huajun li    2011-03-04  532  
41e568d14ec0ac huajun li    2011-03-04  533  	/* Get CSW for device status */
41e568d14ec0ac huajun li    2011-03-04  534  	result = usb_stor_bulk_transfer_buf(us, us->recv_bulk_pipe, bcs,
41e568d14ec0ac huajun li    2011-03-04  535  					    US_BULK_CS_WRAP_LEN, &cswlen);
41e568d14ec0ac huajun li    2011-03-04  536  
41e568d14ec0ac huajun li    2011-03-04  537  	if (result == USB_STOR_XFER_SHORT && cswlen == 0) {
191648d03d2022 Joe Perches  2013-04-19  538  		usb_stor_dbg(us, "Received 0-length CSW; retrying...\n");
41e568d14ec0ac huajun li    2011-03-04  539  		result = usb_stor_bulk_transfer_buf(us, us->recv_bulk_pipe,
41e568d14ec0ac huajun li    2011-03-04  540  					    bcs, US_BULK_CS_WRAP_LEN, &cswlen);
41e568d14ec0ac huajun li    2011-03-04  541  	}
41e568d14ec0ac huajun li    2011-03-04  542  
41e568d14ec0ac huajun li    2011-03-04  543  	if (result == USB_STOR_XFER_STALLED) {
41e568d14ec0ac huajun li    2011-03-04  544  		/* get the status again */
191648d03d2022 Joe Perches  2013-04-19  545  		usb_stor_dbg(us, "Attempting to get CSW (2nd try)...\n");
41e568d14ec0ac huajun li    2011-03-04  546  		result = usb_stor_bulk_transfer_buf(us, us->recv_bulk_pipe,
41e568d14ec0ac huajun li    2011-03-04  547  						bcs, US_BULK_CS_WRAP_LEN, NULL);
41e568d14ec0ac huajun li    2011-03-04  548  	}
41e568d14ec0ac huajun li    2011-03-04  549  
41e568d14ec0ac huajun li    2011-03-04  550  	if (result != USB_STOR_XFER_GOOD)
41e568d14ec0ac huajun li    2011-03-04  551  		return USB_STOR_TRANSPORT_ERROR;
41e568d14ec0ac huajun li    2011-03-04  552  
41e568d14ec0ac huajun li    2011-03-04  553  	/* check bulk status */
41e568d14ec0ac huajun li    2011-03-04  554  	residue = le32_to_cpu(bcs->Residue);
41e568d14ec0ac huajun li    2011-03-04  555  
f0183a338e4f90 Felipe Balbi 2016-04-18  556  	/*
f0183a338e4f90 Felipe Balbi 2016-04-18  557  	 * try to compute the actual residue, based on how much data
f0183a338e4f90 Felipe Balbi 2016-04-18  558  	 * was really transferred and what the device tells us
f0183a338e4f90 Felipe Balbi 2016-04-18  559  	 */
41e568d14ec0ac huajun li    2011-03-04  560  	if (residue && !(us->fflags & US_FL_IGNORE_RESIDUE)) {
41e568d14ec0ac huajun li    2011-03-04  561  		residue = min(residue, transfer_length);
41e568d14ec0ac huajun li    2011-03-04  562  		if (us->srb != NULL)
41e568d14ec0ac huajun li    2011-03-04 @563  			scsi_set_resid(us->srb, max(scsi_get_resid(us->srb),
41e568d14ec0ac huajun li    2011-03-04  564  								(int)residue));
41e568d14ec0ac huajun li    2011-03-04  565  	}
41e568d14ec0ac huajun li    2011-03-04  566  
41e568d14ec0ac huajun li    2011-03-04  567  	if (bcs->Status != US_BULK_STAT_OK)
41e568d14ec0ac huajun li    2011-03-04  568  		return USB_STOR_TRANSPORT_ERROR;
41e568d14ec0ac huajun li    2011-03-04  569  
41e568d14ec0ac huajun li    2011-03-04  570  	return USB_STOR_TRANSPORT_GOOD;
41e568d14ec0ac huajun li    2011-03-04  571  }
41e568d14ec0ac huajun li    2011-03-04  572  

:::::: The code at line 563 was first introduced by commit
:::::: 41e568d14ec0aca1b2bb19563517aad3b06d6805 Staging: Merge ENE UB6250 SD card codes from keucr to drivers/usb/storage

:::::: TO: huajun li <huajun.li.lee@gmail.com>
:::::: CC: Greg Kroah-Hartman <gregkh@suse.de>

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 52253 bytes --]

Re: [PATCH] scsi: Fix scsi_get/set_resid() interface

[Hannes Reinecke]

On 10/28/19 9:38 PM, Bart Van Assche wrote:
> On 10/28/19 3:57 AM, Damien Le Moal wrote:
>> struct scsi_cmnd cmd->req.resid_len which is returned and set
>> respectively by the helper functions scsi_get_resid() and
>> scsi_set_resid() is an unsigned int. Reflect this fact in the interface
>> of these helper functions.
>> [ ... ]
>> -static inline void scsi_set_resid(struct scsi_cmnd *cmd, int resid)
>> +static inline void scsi_set_resid(struct scsi_cmnd *cmd, unsigned int 
>> resid)
>>   {
>>       cmd->req.resid_len = resid;
>>   }
>> -static inline int scsi_get_resid(struct scsi_cmnd *cmd)
>> +static inline unsigned int scsi_get_resid(struct scsi_cmnd *cmd)
>>   {
>>       return cmd->req.resid_len;
>>   }
> 
>  From the iSCSI RFC:
> 
>     SCSI-Presented Data Transfer Length (SPDTL) is the term this document
>     uses (see Section 1.1 for definition) to represent the aggregate data
>     length that the target SCSI layer attempts to transfer using the
>     local iSCSI layer for a task.  Expected Data Transfer Length (EDTL)
>     is the iSCSI term that represents the length of data that the iSCSI
>     layer expects to transfer for a task.  EDTL is specified in the SCSI
>     Command PDU.
> 
>     When SPDTL = EDTL for a task, the target iSCSI layer completes the
>     task with no residuals.  Whenever SPDTL differs from EDTL for a task,
>     that task is said to have a residual.
> 
>     If SPDTL > EDTL for a task, iSCSI Overflow MUST be signaled in the
>     SCSI Response PDU as specified in [RFC3720].  The Residual Count MUST
>     be set to the numerical value of (SPDTL - EDTL).
> 
>     If SPDTL < EDTL for a task, iSCSI Underflow MUST be signaled in the
>     SCSI Response PDU as specified in [RFC3720].  The Residual Count MUST
>     be set to the numerical value of (EDTL - SPDTL).
> 
>     Note that the Overflow and Underflow scenarios are independent of
>     Data-In and Data-Out.  Either scenario is logically possible in
>     either direction of data transfer.
> 
> If the residual is changed from signed into unsigned, how is a SCSI LLD 
> expected to report the difference between residual overflow and residual 
> underflow to the SCSI core?
> 
You don't have to. To quote RFC 3720 page 122:

      bit 5 - (O) set for Residual Overflow.  In this case, the Residual
        Count indicates the number of bytes that were not transferred
        because the initiator's Expected Data Transfer Length was not
        sufficient.  For a bidirectional operation, the Residual Count
        contains the residual for the write operation.

IE the 'overflow' setting in the iSCSI command response is an indicator 
that there _would_ be more data if the command request _would_ have 
specified a larger buffer.
But as it didn't, the entire buffer was filled, and the overflow counter 
is set.
Which, of course, is then ignored by the linux SCSI stack as the request 
got all data, and the residual is set to zero.
Then it's left to the caller to re-send with a larger buffer if 
required. But it's nothing the SCSI stack can nor should be attempting 
on its own.

As such I think the patch is correct.

Reviewed-by: Hannes Reinecke <hare@suse.de>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke            Teamlead Storage & Networking
hare@suse.de                              +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Mary Higgins, Sri Rasiah
HRB 21284 (AG Nürnberg)

Re: [PATCH] scsi: Fix scsi_get/set_resid() interface

[Bart Van Assche]

On 10/30/19 1:30 AM, Hannes Reinecke wrote:
> On 10/28/19 9:38 PM, Bart Van Assche wrote:
>> If the residual is changed from signed into unsigned, how is a SCSI 
>> LLD expected to report the difference between residual overflow and 
>> residual underflow to the SCSI core?
>
> You don't have to. To quote RFC 3720 page 122:
> 
>       bit 5 - (O) set for Residual Overflow.  In this case, the Residual
>         Count indicates the number of bytes that were not transferred
>         because the initiator's Expected Data Transfer Length was not
>         sufficient.  For a bidirectional operation, the Residual Count
>         contains the residual for the write operation.
> 
> IE the 'overflow' setting in the iSCSI command response is an indicator 
> that there _would_ be more data if the command request _would_ have 
> specified a larger buffer.
> But as it didn't, the entire buffer was filled, and the overflow counter 
> is set.
> Which, of course, is then ignored by the linux SCSI stack as the request 
> got all data, and the residual is set to zero.
> Then it's left to the caller to re-send with a larger buffer if 
> required. But it's nothing the SCSI stack can nor should be attempting 
> on its own.

Hi Hannes,

I do not agree that reporting a residual overflow by calling 
scsi_set_resid(..., 0) is acceptable. For reads a residual overflow 
means that the length specified in the CDB (scsi_bufflen()) exceeds the 
data buffer size (length of scsi_sglist()). I think it's dangerous to 
report to the block layer that such requests completed successfully and 
with residual zero.

Bart.

Re: [PATCH] scsi: Fix scsi_get/set_resid() interface

[Hannes Reinecke]

On 10/30/19 4:12 PM, Bart Van Assche wrote:
> On 10/30/19 1:30 AM, Hannes Reinecke wrote:
>> On 10/28/19 9:38 PM, Bart Van Assche wrote:
>>> If the residual is changed from signed into unsigned, how is a SCSI 
>>> LLD expected to report the difference between residual overflow and 
>>> residual underflow to the SCSI core?
>>
>> You don't have to. To quote RFC 3720 page 122:
>>
>>       bit 5 - (O) set for Residual Overflow.  In this case, the Residual
>>         Count indicates the number of bytes that were not transferred
>>         because the initiator's Expected Data Transfer Length was not
>>         sufficient.  For a bidirectional operation, the Residual Count
>>         contains the residual for the write operation.
>>
>> IE the 'overflow' setting in the iSCSI command response is an 
>> indicator that there _would_ be more data if the command request 
>> _would_ have specified a larger buffer.
>> But as it didn't, the entire buffer was filled, and the overflow 
>> counter is set.
>> Which, of course, is then ignored by the linux SCSI stack as the 
>> request got all data, and the residual is set to zero.
>> Then it's left to the caller to re-send with a larger buffer if 
>> required. But it's nothing the SCSI stack can nor should be attempting 
>> on its own.
> 
> Hi Hannes,
> 
> I do not agree that reporting a residual overflow by calling 
> scsi_set_resid(..., 0) is acceptable. For reads a residual overflow 
> means that the length specified in the CDB (scsi_bufflen()) exceeds the 
> data buffer size (length of scsi_sglist()). I think it's dangerous to 
> report to the block layer that such requests completed successfully and 
> with residual zero.
> 
But that is an error on submission, and should be aborted before it even 
got send to the drive.

However, this does not relate to the residual, which is handled after 
the command completes (and which sparked this entire thread ...).

Cheers,

Hannes
-- 
Dr. Hannes Reinecke            Teamlead Storage & Networking
hare@suse.de                              +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Mary Higgins, Sri Rasiah
HRB 21284 (AG Nürnberg)

Re: [PATCH] scsi: Fix scsi_get/set_resid() interface

[Bart Van Assche]

On 10/30/19 8:18 AM, Hannes Reinecke wrote:
> On 10/30/19 4:12 PM, Bart Van Assche wrote:
>> I do not agree that reporting a residual overflow by calling 
>> scsi_set_resid(..., 0) is acceptable. For reads a residual overflow 
>> means that the length specified in the CDB (scsi_bufflen()) exceeds 
>> the data buffer size (length of scsi_sglist()). I think it's dangerous 
>> to report to the block layer that such requests completed successfully 
>> and with residual zero.
>>
> But that is an error on submission, and should be aborted before it even 
> got send to the drive.

If such a bug ever gets introduced in the SCSI core, I think that SCSI 
target code should detect and report it. If the SCSI core receives a 
response with a residual overflow it can then take appropriate action, 
e.g. call WARN_ON_ONCE().

Users of sg_raw can trigger the residual overflow case easily.

> However, this does not relate to the residual, which is handled after 
> the command completes (and which sparked this entire thread ...).

I'm still waiting for an answer to my question of how SCSI LLDs are 
expected to report a residual overflow to the SCSI core.

Thanks,

Bart.

Re: [PATCH] scsi: Fix scsi_get/set_resid() interface

[Douglas Gilbert]

On 2019-10-30 11:18 a.m., Hannes Reinecke wrote:
> On 10/30/19 4:12 PM, Bart Van Assche wrote:
>> On 10/30/19 1:30 AM, Hannes Reinecke wrote:
>>> On 10/28/19 9:38 PM, Bart Van Assche wrote:
>>>> If the residual is changed from signed into unsigned, how is a SCSI LLD 
>>>> expected to report the difference between residual overflow and residual 
>>>> underflow to the SCSI core?
>>>
>>> You don't have to. To quote RFC 3720 page 122:
>>>
>>>       bit 5 - (O) set for Residual Overflow.  In this case, the Residual
>>>         Count indicates the number of bytes that were not transferred
>>>         because the initiator's Expected Data Transfer Length was not
>>>         sufficient.  For a bidirectional operation, the Residual Count
>>>         contains the residual for the write operation.
>>>
>>> IE the 'overflow' setting in the iSCSI command response is an indicator that 
>>> there _would_ be more data if the command request _would_ have specified a 
>>> larger buffer.
>>> But as it didn't, the entire buffer was filled, and the overflow counter is set.
>>> Which, of course, is then ignored by the linux SCSI stack as the request got 
>>> all data, and the residual is set to zero.
>>> Then it's left to the caller to re-send with a larger buffer if required. But 
>>> it's nothing the SCSI stack can nor should be attempting on its own.
>>
>> Hi Hannes,
>>
>> I do not agree that reporting a residual overflow by calling 
>> scsi_set_resid(..., 0) is acceptable. For reads a residual overflow means that 
>> the length specified in the CDB (scsi_bufflen()) exceeds the data buffer size 
>> (length of scsi_sglist()). I think it's dangerous to report to the block layer 
>> that such requests completed successfully and with residual zero.
>>
> But that is an error on submission, and should be aborted before it even got 
> send to the drive.
> 
> However, this does not relate to the residual, which is handled after the 
> command completes (and which sparked this entire thread ...).

Seen from a pass-through perspective, the resid is just about the near-end
data transfer between the HBA and pass-through, or as cam3r03 says:

− cam_resid;
     The data residual length member contains the difference in twos complement
     form of the number of data bytes transferred by the HA for the SCSI
     command compared with the number of bytes requested by the CCB
     cam_dxfer_len member. This is calculated by the total number of bytes
     requested to be transferred by the CCB minus the actual number of bytes
     transferred by the HA.

[where "HA" is HBA (or the initiator)]

That makes overflow (negative resid) a bit interesting as it is only
reasonable that the pass-though user allocated a buffer big enough to
receive dxfer_len bytes. One would hope that in the READ case of overflow,
the HBA would have sent the residual bytes to the bit bucket (i.e.
/dev/null) rather than overfill the data buffer provided by the pass-through.

Handling discrepancies between page length (e.g. of VPD, LOG and MODE pages)
and the ALLOCATION LENGTH field are defined in the SCSI standards.

That leaves the difficult cases: when there is a discrepancy between what the
SCSI command (and the storage device) implied as a data length and the
dxfer_len allocated at the near-end. In some, but not all, cases that is
detectable before the command is issued.

Overflow can happen, for example if the RDPROTECT field in a READ(10) is
accidentally set (e.g. because it uses bits previously used for a SPI
LUN) and the storage device has protection information. That will result
in an extra 8 bytes per logical block being returned.

Doug Gilbert

Re: [PATCH] scsi: Fix scsi_get/set_resid() interface

[kbuild test robot]

Hi Damien,

I love your patch! Perhaps something to improve:

[auto build test WARNING on scsi/for-next]
[cannot apply to v5.4-rc5 next-20191030]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Damien-Le-Moal/scsi-Fix-scsi_get-set_resid-interface/20191030-074824
base:   https://git.kernel.org/pub/scm/linux/kernel/git/jejb/scsi.git for-next
reproduce:
        # apt-get install sparse
        # sparse version: v0.6.1-dirty
        make ARCH=x86_64 allmodconfig
        make C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__'

If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>


sparse warnings: (new ones prefixed by >>)

   drivers/usb/storage/ene_ub6250.c:2161:33: sparse: sparse: incorrect type in assignment (different base types) @@    expected restricted __le32 [usertype] DataTransferLength @@    got e] DataTransferLength @@
   drivers/usb/storage/ene_ub6250.c:2161:33: sparse:    expected restricted __le32 [usertype] DataTransferLength
   drivers/usb/storage/ene_ub6250.c:2161:33: sparse:    got int
   drivers/usb/storage/ene_ub6250.c:2091:33: sparse: sparse: incorrect type in assignment (different base types) @@    expected restricted __le32 [usertype] DataTransferLength @@    got e] DataTransferLength @@
   drivers/usb/storage/ene_ub6250.c:2091:33: sparse:    expected restricted __le32 [usertype] DataTransferLength
   drivers/usb/storage/ene_ub6250.c:2091:33: sparse:    got int
   drivers/usb/storage/ene_ub6250.c:1935:33: sparse: sparse: incorrect type in assignment (different base types) @@    expected restricted __le32 [usertype] DataTransferLength @@    got unrestricted __le32 [usertype] DataTransferLength @@
   drivers/usb/storage/ene_ub6250.c:1935:33: sparse:    expected restricted __le32 [usertype] DataTransferLength
   drivers/usb/storage/ene_ub6250.c:1935:33: sparse:    got unsigned long const [usertype] size
   drivers/usb/storage/ene_ub6250.c:501:43: sparse: sparse: incorrect type in initializer (different base types) @@    expected unsigned int transfer_length @@    got restricted __le3unsigned int transfer_length @@
   drivers/usb/storage/ene_ub6250.c:501:43: sparse:    expected unsigned int transfer_length
   drivers/usb/storage/ene_ub6250.c:501:43: sparse:    got restricted __le32 [usertype] DataTransferLength
>> drivers/usb/storage/ene_ub6250.c:563:49: sparse: sparse: incompatible types in comparison expression (different signedness):
>> drivers/usb/storage/ene_ub6250.c:563:49: sparse:    unsigned int *
>> drivers/usb/storage/ene_ub6250.c:563:49: sparse:    int *
   drivers/usb/storage/ene_ub6250.c:702:33: sparse: sparse: incorrect type in assignment (different base types) @@    expected restricted __le32 [usertype] DataTransferLength @@    got icted __le32 [usertype] DataTransferLength @@
   drivers/usb/storage/ene_ub6250.c:702:33: sparse:    expected restricted __le32 [usertype] DataTransferLength
   drivers/usb/storage/ene_ub6250.c:702:33: sparse:    got unsigned int [usertype] blenByte
   drivers/usb/storage/ene_ub6250.c:742:33: sparse: sparse: incorrect type in assignment (different base types) @@    expected restricted __le32 [usertype] DataTransferLength @@    got icted __le32 [usertype] DataTransferLength @@
   drivers/usb/storage/ene_ub6250.c:742:33: sparse:    expected restricted __le32 [usertype] DataTransferLength
   drivers/usb/storage/ene_ub6250.c:742:33: sparse:    got unsigned int [usertype] blenByte
   drivers/usb/storage/ene_ub6250.c:888:33: sparse: sparse: incorrect type in assignment (different base types) @@    expected restricted __le32 [usertype] DataTransferLength @@    got e] DataTransferLength @@
   drivers/usb/storage/ene_ub6250.c:888:33: sparse:    expected restricted __le32 [usertype] DataTransferLength
   drivers/usb/storage/ene_ub6250.c:888:33: sparse:    got int
   drivers/usb/storage/ene_ub6250.c:907:33: sparse: sparse: incorrect type in assignment (different base types) @@    expected restricted __le32 [usertype] DataTransferLength @@    got e] DataTransferLength @@
   drivers/usb/storage/ene_ub6250.c:907:33: sparse:    expected restricted __le32 [usertype] DataTransferLength
   drivers/usb/storage/ene_ub6250.c:907:33: sparse:    got int
   drivers/usb/storage/ene_ub6250.c:953:18: sparse: sparse: cast to restricted __be16
   drivers/usb/storage/ene_ub6250.c:953:18: sparse: sparse: cast to restricted __be16
   drivers/usb/storage/ene_ub6250.c:953:18: sparse: sparse: cast to restricted __be16
   drivers/usb/storage/ene_ub6250.c:953:18: sparse: sparse: cast to restricted __be16
   drivers/usb/storage/ene_ub6250.c:973:34: sparse: sparse: cast to restricted __be16
   drivers/usb/storage/ene_ub6250.c:973:34: sparse: sparse: cast to restricted __be16
   drivers/usb/storage/ene_ub6250.c:973:34: sparse: sparse: cast to restricted __be16
   drivers/usb/storage/ene_ub6250.c:973:34: sparse: sparse: cast to restricted __be16
   drivers/usb/storage/ene_ub6250.c:974:41: sparse: sparse: cast to restricted __be16
   drivers/usb/storage/ene_ub6250.c:974:41: sparse: sparse: cast to restricted __be16
   drivers/usb/storage/ene_ub6250.c:974:41: sparse: sparse: cast to restricted __be16
   drivers/usb/storage/ene_ub6250.c:974:41: sparse: sparse: cast to restricted __be16
   drivers/usb/storage/ene_ub6250.c:975:41: sparse: sparse: cast to restricted __be16
   drivers/usb/storage/ene_ub6250.c:975:41: sparse: sparse: cast to restricted __be16
   drivers/usb/storage/ene_ub6250.c:975:41: sparse: sparse: cast to restricted __be16
   drivers/usb/storage/ene_ub6250.c:975:41: sparse: sparse: cast to restricted __be16
   drivers/usb/storage/ene_ub6250.c:978:26: sparse: sparse: cast to restricted __be16
   drivers/usb/storage/ene_ub6250.c:978:26: sparse: sparse: cast to restricted __be16
   drivers/usb/storage/ene_ub6250.c:978:26: sparse: sparse: cast to restricted __be16
   drivers/usb/storage/ene_ub6250.c:978:26: sparse: sparse: cast to restricted __be16
   drivers/usb/storage/ene_ub6250.c:992:31: sparse: sparse: cast to restricted __be32
   drivers/usb/storage/ene_ub6250.c:992:31: sparse: sparse: cast to restricted __be32
   drivers/usb/storage/ene_ub6250.c:992:31: sparse: sparse: cast to restricted __be32
   drivers/usb/storage/ene_ub6250.c:992:31: sparse: sparse: cast to restricted __be32
   drivers/usb/storage/ene_ub6250.c:992:31: sparse: sparse: cast to restricted __be32
   drivers/usb/storage/ene_ub6250.c:992:31: sparse: sparse: cast to restricted __be32
   drivers/usb/storage/ene_ub6250.c:996:29: sparse: sparse: cast to restricted __be32
   drivers/usb/storage/ene_ub6250.c:996:29: sparse: sparse: cast to restricted __be32
   drivers/usb/storage/ene_ub6250.c:996:29: sparse: sparse: cast to restricted __be32
   drivers/usb/storage/ene_ub6250.c:996:29: sparse: sparse: cast to restricted __be32
   drivers/usb/storage/ene_ub6250.c:996:29: sparse: sparse: cast to restricted __be32
   drivers/usb/storage/ene_ub6250.c:996:29: sparse: sparse: cast to restricted __be32
   drivers/usb/storage/ene_ub6250.c:1028:42: sparse: sparse: cast to restricted __be16
   drivers/usb/storage/ene_ub6250.c:1028:42: sparse: sparse: cast to restricted __be16
   drivers/usb/storage/ene_ub6250.c:1028:42: sparse: sparse: cast to restricted __be16
   drivers/usb/storage/ene_ub6250.c:1028:42: sparse: sparse: cast to restricted __be16
   drivers/usb/storage/ene_ub6250.c:1052:29: sparse: sparse: cast to restricted __le16
   drivers/usb/storage/ene_ub6250.c:1055:55: sparse: sparse: cast to restricted __le16
   drivers/usb/storage/ene_ub6250.c:1167:33: sparse: sparse: incorrect type in assignment (different base types) @@    expected restricted __le32 [usertype] DataTransferLength @@    got e] DataTransferLength @@
   drivers/usb/storage/ene_ub6250.c:1167:33: sparse:    expected restricted __le32 [usertype] DataTransferLength
   drivers/usb/storage/ene_ub6250.c:1167:33: sparse:    got int
   drivers/usb/storage/ene_ub6250.c:1200:33: sparse: sparse: incorrect type in assignment (different base types) @@    expected restricted __le32 [usertype] DataTransferLength @@    got e] DataTransferLength @@
   drivers/usb/storage/ene_ub6250.c:1200:33: sparse:    expected restricted __le32 [usertype] DataTransferLength
   drivers/usb/storage/ene_ub6250.c:1200:33: sparse:    got int
   drivers/usb/storage/ene_ub6250.c:1231:23: sparse: sparse: cast to restricted __be16
   drivers/usb/storage/ene_ub6250.c:1231:23: sparse: sparse: cast to restricted __be16
   drivers/usb/storage/ene_ub6250.c:1231:23: sparse: sparse: cast to restricted __be16
   drivers/usb/storage/ene_ub6250.c:1231:23: sparse: sparse: cast to restricted __be16
   drivers/usb/storage/ene_ub6250.c:1277:33: sparse: sparse: incorrect type in assignment (different base types) @@    expected restricted __le32 [usertype] DataTransferLength @@    got e] DataTransferLength @@
   drivers/usb/storage/ene_ub6250.c:1277:33: sparse:    expected restricted __le32 [usertype] DataTransferLength
   drivers/usb/storage/ene_ub6250.c:1277:33: sparse:    got int
   drivers/usb/storage/ene_ub6250.c:1359:33: sparse: sparse: incorrect type in assignment (different base types) @@    expected restricted __le32 [usertype] DataTransferLength @@    got e] DataTransferLength @@
   drivers/usb/storage/ene_ub6250.c:1359:33: sparse:    expected restricted __le32 [usertype] DataTransferLength
   drivers/usb/storage/ene_ub6250.c:1359:33: sparse:    got int
   drivers/usb/storage/ene_ub6250.c:1543:33: sparse: sparse: incorrect type in assignment (different base types) @@    expected restricted __le32 [usertype] DataTransferLength @@    got e] DataTransferLength @@
   drivers/usb/storage/ene_ub6250.c:1543:33: sparse:    expected restricted __le32 [usertype] DataTransferLength
   drivers/usb/storage/ene_ub6250.c:1543:33: sparse:    got int
   drivers/usb/storage/ene_ub6250.c:1662:41: sparse: sparse: incorrect type in assignment (different base types) @@    expected restricted __le32 [usertype] DataTransferLength @@    got icted __le32 [usertype] DataTransferLength @@
   drivers/usb/storage/ene_ub6250.c:1662:41: sparse:    expected restricted __le32 [usertype] DataTransferLength
   drivers/usb/storage/ene_ub6250.c:1662:41: sparse:    got unsigned int [usertype] blenByte
   drivers/usb/storage/ene_ub6250.c:1706:49: sparse: sparse: incorrect type in assignment (different base types) @@    expected restricted __le32 [usertype] DataTransferLength @@    got e] DataTransferLength @@
   drivers/usb/storage/ene_ub6250.c:1706:49: sparse:    expected restricted __le32 [usertype] DataTransferLength
   drivers/usb/storage/ene_ub6250.c:1706:49: sparse:    got int
   drivers/usb/storage/ene_ub6250.c:1763:41: sparse: sparse: incorrect type in assignment (different base types) @@    expected restricted __le32 [usertype] DataTransferLength @@    got icted __le32 [usertype] DataTransferLength @@
   drivers/usb/storage/ene_ub6250.c:1763:41: sparse:    expected restricted __le32 [usertype] DataTransferLength
   drivers/usb/storage/ene_ub6250.c:1763:41: sparse:    got unsigned int [usertype] blenByte
   drivers/usb/storage/ene_ub6250.c:1839:33: sparse: sparse: incorrect type in assignment (different base types) @@    expected restricted __le32 [usertype] DataTransferLength @@    got e] DataTransferLength @@
   drivers/usb/storage/ene_ub6250.c:1839:33: sparse:    expected restricted __le32 [usertype] DataTransferLength
   drivers/usb/storage/ene_ub6250.c:1839:33: sparse:    got int
   drivers/usb/storage/ene_ub6250.c:1991:26: sparse: sparse: cast to restricted __be16
   drivers/usb/storage/ene_ub6250.c:1991:26: sparse: sparse: cast to restricted __be16
   drivers/usb/storage/ene_ub6250.c:1991:26: sparse: sparse: cast to restricted __be16
   drivers/usb/storage/ene_ub6250.c:1991:26: sparse: sparse: cast to restricted __be16
   drivers/usb/storage/ene_ub6250.c:1992:26: sparse: sparse: cast to restricted __be16
   drivers/usb/storage/ene_ub6250.c:1992:26: sparse: sparse: cast to restricted __be16
   drivers/usb/storage/ene_ub6250.c:1992:26: sparse: sparse: cast to restricted __be16
   drivers/usb/storage/ene_ub6250.c:1992:26: sparse: sparse: cast to restricted __be16

vim +563 drivers/usb/storage/ene_ub6250.c

41e568d14ec0ac huajun li    2011-03-04  492  
41e568d14ec0ac huajun li    2011-03-04  493  static int ene_send_scsi_cmd(struct us_data *us, u8 fDir, void *buf, int use_sg)
41e568d14ec0ac huajun li    2011-03-04  494  {
41e568d14ec0ac huajun li    2011-03-04  495  	struct bulk_cb_wrap *bcb = (struct bulk_cb_wrap *) us->iobuf;
41e568d14ec0ac huajun li    2011-03-04  496  	struct bulk_cs_wrap *bcs = (struct bulk_cs_wrap *) us->iobuf;
41e568d14ec0ac huajun li    2011-03-04  497  
41e568d14ec0ac huajun li    2011-03-04  498  	int result;
41e568d14ec0ac huajun li    2011-03-04  499  	unsigned int residue;
41e568d14ec0ac huajun li    2011-03-04  500  	unsigned int cswlen = 0, partial = 0;
41e568d14ec0ac huajun li    2011-03-04  501  	unsigned int transfer_length = bcb->DataTransferLength;
41e568d14ec0ac huajun li    2011-03-04  502  
191648d03d2022 Joe Perches  2013-04-19  503  	/* usb_stor_dbg(us, "transport --- ene_send_scsi_cmd\n"); */
41e568d14ec0ac huajun li    2011-03-04  504  	/* send cmd to out endpoint */
41e568d14ec0ac huajun li    2011-03-04  505  	result = usb_stor_bulk_transfer_buf(us, us->send_bulk_pipe,
41e568d14ec0ac huajun li    2011-03-04  506  					    bcb, US_BULK_CB_WRAP_LEN, NULL);
41e568d14ec0ac huajun li    2011-03-04  507  	if (result != USB_STOR_XFER_GOOD) {
191648d03d2022 Joe Perches  2013-04-19  508  		usb_stor_dbg(us, "send cmd to out endpoint fail ---\n");
41e568d14ec0ac huajun li    2011-03-04  509  		return USB_STOR_TRANSPORT_ERROR;
41e568d14ec0ac huajun li    2011-03-04  510  	}
41e568d14ec0ac huajun li    2011-03-04  511  
41e568d14ec0ac huajun li    2011-03-04  512  	if (buf) {
41e568d14ec0ac huajun li    2011-03-04  513  		unsigned int pipe = fDir;
41e568d14ec0ac huajun li    2011-03-04  514  
41e568d14ec0ac huajun li    2011-03-04  515  		if (fDir  == FDIR_READ)
41e568d14ec0ac huajun li    2011-03-04  516  			pipe = us->recv_bulk_pipe;
41e568d14ec0ac huajun li    2011-03-04  517  		else
41e568d14ec0ac huajun li    2011-03-04  518  			pipe = us->send_bulk_pipe;
41e568d14ec0ac huajun li    2011-03-04  519  
41e568d14ec0ac huajun li    2011-03-04  520  		/* Bulk */
41e568d14ec0ac huajun li    2011-03-04  521  		if (use_sg) {
41e568d14ec0ac huajun li    2011-03-04  522  			result = usb_stor_bulk_srb(us, pipe, us->srb);
41e568d14ec0ac huajun li    2011-03-04  523  		} else {
41e568d14ec0ac huajun li    2011-03-04  524  			result = usb_stor_bulk_transfer_sg(us, pipe, buf,
41e568d14ec0ac huajun li    2011-03-04  525  						transfer_length, 0, &partial);
41e568d14ec0ac huajun li    2011-03-04  526  		}
41e568d14ec0ac huajun li    2011-03-04  527  		if (result != USB_STOR_XFER_GOOD) {
191648d03d2022 Joe Perches  2013-04-19  528  			usb_stor_dbg(us, "data transfer fail ---\n");
41e568d14ec0ac huajun li    2011-03-04  529  			return USB_STOR_TRANSPORT_ERROR;
41e568d14ec0ac huajun li    2011-03-04  530  		}
41e568d14ec0ac huajun li    2011-03-04  531  	}
41e568d14ec0ac huajun li    2011-03-04  532  
41e568d14ec0ac huajun li    2011-03-04  533  	/* Get CSW for device status */
41e568d14ec0ac huajun li    2011-03-04  534  	result = usb_stor_bulk_transfer_buf(us, us->recv_bulk_pipe, bcs,
41e568d14ec0ac huajun li    2011-03-04  535  					    US_BULK_CS_WRAP_LEN, &cswlen);
41e568d14ec0ac huajun li    2011-03-04  536  
41e568d14ec0ac huajun li    2011-03-04  537  	if (result == USB_STOR_XFER_SHORT && cswlen == 0) {
191648d03d2022 Joe Perches  2013-04-19  538  		usb_stor_dbg(us, "Received 0-length CSW; retrying...\n");
41e568d14ec0ac huajun li    2011-03-04  539  		result = usb_stor_bulk_transfer_buf(us, us->recv_bulk_pipe,
41e568d14ec0ac huajun li    2011-03-04  540  					    bcs, US_BULK_CS_WRAP_LEN, &cswlen);
41e568d14ec0ac huajun li    2011-03-04  541  	}
41e568d14ec0ac huajun li    2011-03-04  542  
41e568d14ec0ac huajun li    2011-03-04  543  	if (result == USB_STOR_XFER_STALLED) {
41e568d14ec0ac huajun li    2011-03-04  544  		/* get the status again */
191648d03d2022 Joe Perches  2013-04-19  545  		usb_stor_dbg(us, "Attempting to get CSW (2nd try)...\n");
41e568d14ec0ac huajun li    2011-03-04  546  		result = usb_stor_bulk_transfer_buf(us, us->recv_bulk_pipe,
41e568d14ec0ac huajun li    2011-03-04  547  						bcs, US_BULK_CS_WRAP_LEN, NULL);
41e568d14ec0ac huajun li    2011-03-04  548  	}
41e568d14ec0ac huajun li    2011-03-04  549  
41e568d14ec0ac huajun li    2011-03-04  550  	if (result != USB_STOR_XFER_GOOD)
41e568d14ec0ac huajun li    2011-03-04  551  		return USB_STOR_TRANSPORT_ERROR;
41e568d14ec0ac huajun li    2011-03-04  552  
41e568d14ec0ac huajun li    2011-03-04  553  	/* check bulk status */
41e568d14ec0ac huajun li    2011-03-04  554  	residue = le32_to_cpu(bcs->Residue);
41e568d14ec0ac huajun li    2011-03-04  555  
f0183a338e4f90 Felipe Balbi 2016-04-18  556  	/*
f0183a338e4f90 Felipe Balbi 2016-04-18  557  	 * try to compute the actual residue, based on how much data
f0183a338e4f90 Felipe Balbi 2016-04-18  558  	 * was really transferred and what the device tells us
f0183a338e4f90 Felipe Balbi 2016-04-18  559  	 */
41e568d14ec0ac huajun li    2011-03-04  560  	if (residue && !(us->fflags & US_FL_IGNORE_RESIDUE)) {
41e568d14ec0ac huajun li    2011-03-04  561  		residue = min(residue, transfer_length);
41e568d14ec0ac huajun li    2011-03-04  562  		if (us->srb != NULL)
41e568d14ec0ac huajun li    2011-03-04 @563  			scsi_set_resid(us->srb, max(scsi_get_resid(us->srb),
41e568d14ec0ac huajun li    2011-03-04  564  								(int)residue));
41e568d14ec0ac huajun li    2011-03-04  565  	}
41e568d14ec0ac huajun li    2011-03-04  566  
41e568d14ec0ac huajun li    2011-03-04  567  	if (bcs->Status != US_BULK_STAT_OK)
41e568d14ec0ac huajun li    2011-03-04  568  		return USB_STOR_TRANSPORT_ERROR;
41e568d14ec0ac huajun li    2011-03-04  569  
41e568d14ec0ac huajun li    2011-03-04  570  	return USB_STOR_TRANSPORT_GOOD;
41e568d14ec0ac huajun li    2011-03-04  571  }
41e568d14ec0ac huajun li    2011-03-04  572  

:::::: The code at line 563 was first introduced by commit
:::::: 41e568d14ec0aca1b2bb19563517aad3b06d6805 Staging: Merge ENE UB6250 SD card codes from keucr to drivers/usb/storage

:::::: TO: huajun li <huajun.li.lee@gmail.com>
:::::: CC: Greg Kroah-Hartman <gregkh@suse.de>

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

Re: [PATCH] scsi: Fix scsi_get/set_resid() interface

[Hannes Reinecke]

On 10/30/19 5:18 PM, Douglas Gilbert wrote:
> On 2019-10-30 11:18 a.m., Hannes Reinecke wrote:
>> On 10/30/19 4:12 PM, Bart Van Assche wrote:
>>> On 10/30/19 1:30 AM, Hannes Reinecke wrote:
>>>> On 10/28/19 9:38 PM, Bart Van Assche wrote:
>>>>> If the residual is changed from signed into unsigned, how is a SCSI 
>>>>> LLD expected to report the difference between residual overflow and 
>>>>> residual underflow to the SCSI core?
>>>>
>>>> You don't have to. To quote RFC 3720 page 122:
>>>>
>>>>       bit 5 - (O) set for Residual Overflow.  In this case, the 
>>>> Residual
>>>>         Count indicates the number of bytes that were not transferred
>>>>         because the initiator's Expected Data Transfer Length was not
>>>>         sufficient.  For a bidirectional operation, the Residual Count
>>>>         contains the residual for the write operation.
>>>>
>>>> IE the 'overflow' setting in the iSCSI command response is an 
>>>> indicator that there _would_ be more data if the command request 
>>>> _would_ have specified a larger buffer.
>>>> But as it didn't, the entire buffer was filled, and the overflow 
>>>> counter is set.
>>>> Which, of course, is then ignored by the linux SCSI stack as the 
>>>> request got all data, and the residual is set to zero.
>>>> Then it's left to the caller to re-send with a larger buffer if 
>>>> required. But it's nothing the SCSI stack can nor should be 
>>>> attempting on its own.
>>>
>>> Hi Hannes,
>>>
>>> I do not agree that reporting a residual overflow by calling 
>>> scsi_set_resid(..., 0) is acceptable. For reads a residual overflow 
>>> means that the length specified in the CDB (scsi_bufflen()) exceeds 
>>> the data buffer size (length of scsi_sglist()). I think it's 
>>> dangerous to report to the block layer that such requests completed 
>>> successfully and with residual zero.
>>>
>> But that is an error on submission, and should be aborted before it 
>> even got send to the drive.
>>
>> However, this does not relate to the residual, which is handled after 
>> the command completes (and which sparked this entire thread ...).
> 
> Seen from a pass-through perspective, the resid is just about the near-end
> data transfer between the HBA and pass-through, or as cam3r03 says:
> 
> − cam_resid;
>      The data residual length member contains the difference in twos 
> complement
>      form of the number of data bytes transferred by the HA for the SCSI
>      command compared with the number of bytes requested by the CCB
>      cam_dxfer_len member. This is calculated by the total number of bytes
>      requested to be transferred by the CCB minus the actual number of 
> bytes
>      transferred by the HA.
> 
> [where "HA" is HBA (or the initiator)]
> 
> That makes overflow (negative resid) a bit interesting as it is only
> reasonable that the pass-though user allocated a buffer big enough to
> receive dxfer_len bytes. One would hope that in the READ case of overflow,
> the HBA would have sent the residual bytes to the bit bucket (i.e.
> /dev/null) rather than overfill the data buffer provided by the 
> pass-through.
> 
Yes. But my point here is that any residual values are handled (and 
defined) at the protocol level. Any SCSI midlayer protocol like SPC or, 
indeed, SAM have no concept of residuals.

THe SCSI midlayer chose to add a residual value to document the number 
of bytes _not_ being handled from hardware.

> Handling discrepancies between page length (e.g. of VPD, LOG and MODE 
> pages) and the ALLOCATION LENGTH field are defined in the SCSI standards.
> 
> That leaves the difficult cases: when there is a discrepancy between 
> what the
> SCSI command (and the storage device) implied as a data length and the
> dxfer_len allocated at the near-end. In some, but not all, cases that is
> detectable before the command is issued.
> 
As mentioned, this needs to be handled in the submission path, and the 
command should be aborted before being sent to the drive.

> Overflow can happen, for example if the RDPROTECT field in a READ(10) is
> accidentally set (e.g. because it uses bits previously used for a SPI
> LUN) and the storage device has protection information. That will result
> in an extra 8 bytes per logical block being returned.
> 

See above. We should intercept such a command, and simply abort it with 
an appropriate error.
That goes for internal commands, of course; I know it doesn't work for 
things like sg_raw, but one could argue that this was precisely the 
point of that command ...

Cheers,

Hannes
-- 
Dr. Hannes Reinecke            Teamlead Storage & Networking
hare@suse.de                              +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Mary Higgins, Sri Rasiah
HRB 21284 (AG Nürnberg)

Re: [PATCH] scsi: Fix scsi_get/set_resid() interface

[Hannes Reinecke]

On 10/30/19 4:12 PM, Bart Van Assche wrote:
> On 10/30/19 1:30 AM, Hannes Reinecke wrote:
>> On 10/28/19 9:38 PM, Bart Van Assche wrote:
>>> If the residual is changed from signed into unsigned, how is a SCSI 
>>> LLD expected to report the difference between residual overflow and 
>>> residual underflow to the SCSI core?
>>
>> You don't have to. To quote RFC 3720 page 122:
>>
>>       bit 5 - (O) set for Residual Overflow.  In this case, the Residual
>>         Count indicates the number of bytes that were not transferred
>>         because the initiator's Expected Data Transfer Length was not
>>         sufficient.  For a bidirectional operation, the Residual Count
>>         contains the residual for the write operation.
>>
>> IE the 'overflow' setting in the iSCSI command response is an 
>> indicator that there _would_ be more data if the command request 
>> _would_ have specified a larger buffer.
>> But as it didn't, the entire buffer was filled, and the overflow 
>> counter is set.
>> Which, of course, is then ignored by the linux SCSI stack as the 
>> request got all data, and the residual is set to zero.
>> Then it's left to the caller to re-send with a larger buffer if 
>> required. But it's nothing the SCSI stack can nor should be attempting 
>> on its own.
> 
> Hi Hannes,
> 
> I do not agree that reporting a residual overflow by calling 
> scsi_set_resid(..., 0) is acceptable. For reads a residual overflow 
> means that the length specified in the CDB (scsi_bufflen()) exceeds the 
> data buffer size (length of scsi_sglist()). I think it's dangerous to 
> report to the block layer that such requests completed successfully and 
> with residual zero.
> 
Dangerous how?

fcp3:
For read operations and write operations, if the FCP_RESID_OVER bit is 
set to one, the FCP_RESID field contains the excess of the number of 
bytes required by the SCSI command to be transferred over the number of 
bytes specified by the FCP_DL field.

SAS doesn't even have the notion of residuals

srp04:
DOOVER , when set to one, indicates that the DATA-OUT RESIDUAL COUNT 
field is valid and contains the count of data bytes that could not be 
transferred from the data-out buffer because the length of the data-out 
buffer was not sufficient. The application client should examine the 
DATA-OUT RESIDUAL COUNT field in the context of the command to determine 
whether or not an error condition occurred.

iSCSI we've already covered.

In all cases, the overflow value is an _indicator_ that additional data 
is available, but was not transferred due to the lack of space.
So we will not have any buffer overflow as data is never transferred.

And in most cases an overflow is actually desired; it is a pretty common 
use pattern to send a SCSI command with a small enough buffer to return 
the length of available data, and then send the actual command knowing 
how large the buffer needs to be.
See for example scsi_report_lun_scan() or the VPD handling code.

So again, I don't think it's something we need to worry about.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke            Teamlead Storage & Networking
hare@suse.de                              +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Mary Higgins, Sri Rasiah
HRB 21284 (AG Nürnberg)