On 2019/10/30 16:15, Bart Van Assche wrote: > On 10/30/19 2:08 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. >> >> Also fix compilation errors due to min() and max() type mismatch >> introduced by this change in scsi debug code, usb transport code and in >> the USB ENE card reader driver. > Please answer my question about how a SCSI LLD should report residual > overflows. I think this patch is incompatible with the approach used by > the SRP initiator driver: > > if (unlikely(rsp->flags & SRP_RSP_FLAG_DIUNDER)) > scsi_set_resid(scmnd, be32_to_cpu(rsp->data_in_res_cnt)); be32_to_cpu(rsp->data_in_res_cnt) is always >= 0 so no problems. > else if (unlikely(rsp->flags & SRP_RSP_FLAG_DIOVER)) > scsi_set_resid(scmnd, -be32_to_cpu(rsp->data_in_res_cnt)); -be32_to_cpu(rsp->data_in_res_cnt) is always <= 0. But it does *not* matter if my patch is applied or not, this negative value gets stored into scmd->req.resid_len which is an *unsigned int*. How does that work ? My patch changes the function resid argument type and that function is inline, so in practice, there are 0 changes, none whatsoever, isn't it ? The problem you are raising here may exist, and how the scsi core will handle a negative value cast to an unsigned int, likely resulting in a value far larger than the command buffer size, is certainly a serious concern. But it is unrelated to what my patch does. If you feel strongly about it, I have absolutely no problem with dropping the patch. I just would like that it be dropped for the right reasons... > else if (unlikely(rsp->flags & SRP_RSP_FLAG_DOUNDER)) > scsi_set_resid(scmnd, be32_to_cpu(rsp->data_out_res_cnt)); > else if (unlikely(rsp->flags & SRP_RSP_FLAG_DOOVER)) > scsi_set_resid(scmnd, -be32_to_cpu(rsp->data_out_res_cnt)); > > Thanks, > > Bart. > -- Damien Le Moal Western Digital Research
