| Message ID | 20250107010220.7215-1-michael.christie@oracle.com |
|---|---|
| State | New |
| Headers | show |
| Series | [1/1] scsi: Fix command pass through retry regression | expand |
On 1/6/25 5:02 PM, Mike Christie wrote: > scsi_check_passthrough is always called, but it doesn't check for if a > command completed successfully. As a result, if a command was successful > and the caller used SCMD_FAILURE_RESULT_ANY to indicate what failures it > wanted to retry, we will end up retrying the command. This will cause > delays during device discovery because of the command being sent multiple > times. For some USB devices it can also cause the wrong device size to be > used. > > This patch adds a check for if the command was successful. If it is we > return immediately instead of trying to match a failure. Reviewed-by: Bart Van Assche <bvanassche@acm.org>
On 07/01/2025 01:02, Mike Christie wrote: > scsi_check_passthrough is always called ...upon completion? >, but it doesn't check for if a doesn't properly check > command completed successfully. As a result, if a command was successful > and the caller used SCMD_FAILURE_RESULT_ANY to indicate what failures it > wanted to retry, we will end up retrying the command. This will cause > delays during device discovery because of the command being sent multiple > times. For some USB devices it can also cause the wrong device size to be > used. > > This patch adds a check for if the command was successful. If it is we > return immediately instead of trying to match a failure. > > Fixes: 994724e6b3f0 ("scsi: core: Allow passthrough to request midlayer retries") > Reported-by: Kris Karas <bugs-a21@moonlit-rail.com> > Closes: https://urldefense.com/v3/__https://bugzilla.kernel.org/show_bug.cgi?id=219652__;!!ACWV5N9M2RV99hQ!Lvd5QQ8J-3Z1AIBugQywYdRmGcEJdIffF6RBkszOroX8uqGBwmdS0ggKuA7mclKJvk3Cq38QEoOMDRqX3BZ4IOqpAb9Frw$ > Signed-off-by: Mike Christie <michael.christie@oracle.com> > > --- > drivers/scsi/scsi_lib.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c > index adee6f60c966..0cc6a0f77b09 100644 > --- a/drivers/scsi/scsi_lib.c > +++ b/drivers/scsi/scsi_lib.c > @@ -210,6 +210,9 @@ static int scsi_check_passthrough(struct scsi_cmnd *scmd, > struct scsi_sense_hdr sshdr; > enum sam_status status; > > + if (!scmd->result) this could be combined into the !failures check to have a single return, but that's personal coding preference. > + return 0; Would it be messy to drop the separate SCMD_FAILURE_RESULT_ANY check at the start of the for loop and include that check in the individual result status and host byte checks? In that way we would not have the separate !scmd->result check, introduced here. I guess that it would (be messy). Regardless of comments: Reviewed-by: John Garry <john.g.garry@oracle.com>
Mike, > scsi_check_passthrough is always called, but it doesn't check for if a > command completed successfully. As a result, if a command was > successful and the caller used SCMD_FAILURE_RESULT_ANY to indicate > what failures it wanted to retry, we will end up retrying the command. Please add a suitable kunit test for this case. Thanks!
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c index adee6f60c966..0cc6a0f77b09 100644 --- a/drivers/scsi/scsi_lib.c +++ b/drivers/scsi/scsi_lib.c @@ -210,6 +210,9 @@ static int scsi_check_passthrough(struct scsi_cmnd *scmd, struct scsi_sense_hdr sshdr; enum sam_status status; + if (!scmd->result) + return 0; + if (!failures) return 0;
scsi_check_passthrough is always called, but it doesn't check for if a command completed successfully. As a result, if a command was successful and the caller used SCMD_FAILURE_RESULT_ANY to indicate what failures it wanted to retry, we will end up retrying the command. This will cause delays during device discovery because of the command being sent multiple times. For some USB devices it can also cause the wrong device size to be used. This patch adds a check for if the command was successful. If it is we return immediately instead of trying to match a failure. Fixes: 994724e6b3f0 ("scsi: core: Allow passthrough to request midlayer retries") Reported-by: Kris Karas <bugs-a21@moonlit-rail.com> Closes: https://bugzilla.kernel.org/show_bug.cgi?id=219652 Signed-off-by: Mike Christie <michael.christie@oracle.com> --- drivers/scsi/scsi_lib.c | 3 +++ 1 file changed, 3 insertions(+)