[PATCH] scsi: retry MODE SENSE on unit attention

[PATCH] scsi: retry MODE SENSE on unit attention

[Hannes Reinecke]

The 'sd' driver is calling scsi_mode_sense() to figure out
internal details. But scsi_mode_sense() never checks for
any pending unit attentions, so we're getting annoying error
messages like:

MODE SENSE: unimplemented page/subpage: 0x00/0x00

and a possible wrong decision for device cache handling.

Signed-off-by: Hannes Reinecke <hare@suse.de>
---
 drivers/scsi/scsi_lib.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 2428d96..d7915c8 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -2423,7 +2423,7 @@ scsi_mode_sense(struct scsi_device *sdev, int dbd, int modepage,
 	unsigned char cmd[12];
 	int use_10_for_ms;
 	int header_length;
-	int result;
+	int result, retry_count = retries;
 	struct scsi_sense_hdr my_sshdr;
 
 	memset(data, 0, sizeof(*data));
@@ -2502,6 +2502,11 @@ scsi_mode_sense(struct scsi_device *sdev, int dbd, int modepage,
 			data->block_descriptor_length = buffer[3];
 		}
 		data->header_length = header_length;
+	} else if ((status_byte(result) == CHECK_CONDITION) &&
+		   scsi_sense_valid(sshdr) &&
+		   sshdr->sense_key == UNIT_ATTENTION && retry_count) {
+		retry_count--;
+		goto retry;
 	}
 
 	return result;
-- 
1.8.5.2

Re: [PATCH] scsi: retry MODE SENSE on unit attention

[Ewan Milne]

On Thu, 2015-06-11 at 13:01 +0200, Hannes Reinecke wrote:
> The 'sd' driver is calling scsi_mode_sense() to figure out
> internal details. But scsi_mode_sense() never checks for
> any pending unit attentions, so we're getting annoying error
> messages like:
> 
> MODE SENSE: unimplemented page/subpage: 0x00/0x00
> 
> and a possible wrong decision for device cache handling.
> 
> Signed-off-by: Hannes Reinecke <hare@suse.de>
> ---
>  drivers/scsi/scsi_lib.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> index 2428d96..d7915c8 100644
> --- a/drivers/scsi/scsi_lib.c
> +++ b/drivers/scsi/scsi_lib.c
> @@ -2423,7 +2423,7 @@ scsi_mode_sense(struct scsi_device *sdev, int dbd, int modepage,
>  	unsigned char cmd[12];
>  	int use_10_for_ms;
>  	int header_length;
> -	int result;
> +	int result, retry_count = retries;
>  	struct scsi_sense_hdr my_sshdr;
>  
>  	memset(data, 0, sizeof(*data));
> @@ -2502,6 +2502,11 @@ scsi_mode_sense(struct scsi_device *sdev, int dbd, int modepage,
>  			data->block_descriptor_length = buffer[3];
>  		}
>  		data->header_length = header_length;
> +	} else if ((status_byte(result) == CHECK_CONDITION) &&
> +		   scsi_sense_valid(sshdr) &&
> +		   sshdr->sense_key == UNIT_ATTENTION && retry_count) {
> +		retry_count--;
> +		goto retry;
>  	}
>  
>  	return result;

Great, but shouldn't we be doing this more generally?  What about
scsi_mode_select()?

(And, with the number of status changes that can get reported by
 UAs, we might want to think about increasing the retry count on
 these commands up from 3 at some point.)

Reviewed-by: Ewan D. Milne <emilne@redhat.com>

Re: [PATCH] scsi: retry MODE SENSE on unit attention

[Hannes Reinecke]

On 06/11/2015 05:07 PM, Ewan Milne wrote:
> On Thu, 2015-06-11 at 13:01 +0200, Hannes Reinecke wrote:
>> The 'sd' driver is calling scsi_mode_sense() to figure out
>> internal details. But scsi_mode_sense() never checks for
>> any pending unit attentions, so we're getting annoying error
>> messages like:
>>
>> MODE SENSE: unimplemented page/subpage: 0x00/0x00
>>
>> and a possible wrong decision for device cache handling.
>>
>> Signed-off-by: Hannes Reinecke <hare@suse.de>
>> ---
>>  drivers/scsi/scsi_lib.c | 7 ++++++-
>>  1 file changed, 6 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
>> index 2428d96..d7915c8 100644
>> --- a/drivers/scsi/scsi_lib.c
>> +++ b/drivers/scsi/scsi_lib.c
>> @@ -2423,7 +2423,7 @@ scsi_mode_sense(struct scsi_device *sdev, int dbd, int modepage,
>>  	unsigned char cmd[12];
>>  	int use_10_for_ms;
>>  	int header_length;
>> -	int result;
>> +	int result, retry_count = retries;
>>  	struct scsi_sense_hdr my_sshdr;
>>  
>>  	memset(data, 0, sizeof(*data));
>> @@ -2502,6 +2502,11 @@ scsi_mode_sense(struct scsi_device *sdev, int dbd, int modepage,
>>  			data->block_descriptor_length = buffer[3];
>>  		}
>>  		data->header_length = header_length;
>> +	} else if ((status_byte(result) == CHECK_CONDITION) &&
>> +		   scsi_sense_valid(sshdr) &&
>> +		   sshdr->sense_key == UNIT_ATTENTION && retry_count) {
>> +		retry_count--;
>> +		goto retry;
>>  	}
>>  
>>  	return result;
> 
> Great, but shouldn't we be doing this more generally?  What about
> scsi_mode_select()?
> 
I haven't seen any issues with scsi_mode_select() as of now, so I
didn't do anything about this :-)

> (And, with the number of status changes that can get reported by
>  UAs, we might want to think about increasing the retry count on
>  these commands up from 3 at some point.)
> 
Hmm. _Actually_, we're not getting _more_ UAs (neither the number
nor the situation at which UAs are being send has changed).
It's just that we're trying to _use_ UAs so these things pop up.
But yeah, raising the number or retries to eg 5 is probably a good idea.

> Reviewed-by: Ewan D. Milne <emilne@redhat.com>
> 
Cheers,

Hannes
-- 
Dr. Hannes Reinecke		               zSeries & Storage
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)
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] scsi: retry MODE SENSE on unit attention

[Ewan Milne]

On Fri, 2015-06-12 at 08:27 +0200, Hannes Reinecke wrote:
> On 06/11/2015 05:07 PM, Ewan Milne wrote:
> > On Thu, 2015-06-11 at 13:01 +0200, Hannes Reinecke wrote:
> >> The 'sd' driver is calling scsi_mode_sense() to figure out
> >> internal details. But scsi_mode_sense() never checks for
> >> any pending unit attentions, so we're getting annoying error
> >> messages like:
> >>
> >> MODE SENSE: unimplemented page/subpage: 0x00/0x00
> >>
> >> and a possible wrong decision for device cache handling.
> >>
> >> Signed-off-by: Hannes Reinecke <hare@suse.de>
> >> ---
> >>  drivers/scsi/scsi_lib.c | 7 ++++++-
> >>  1 file changed, 6 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> >> index 2428d96..d7915c8 100644
> >> --- a/drivers/scsi/scsi_lib.c
> >> +++ b/drivers/scsi/scsi_lib.c
> >> @@ -2423,7 +2423,7 @@ scsi_mode_sense(struct scsi_device *sdev, int dbd, int modepage,
> >>  	unsigned char cmd[12];
> >>  	int use_10_for_ms;
> >>  	int header_length;
> >> -	int result;
> >> +	int result, retry_count = retries;
> >>  	struct scsi_sense_hdr my_sshdr;
> >>  
> >>  	memset(data, 0, sizeof(*data));
> >> @@ -2502,6 +2502,11 @@ scsi_mode_sense(struct scsi_device *sdev, int dbd, int modepage,
> >>  			data->block_descriptor_length = buffer[3];
> >>  		}
> >>  		data->header_length = header_length;
> >> +	} else if ((status_byte(result) == CHECK_CONDITION) &&
> >> +		   scsi_sense_valid(sshdr) &&
> >> +		   sshdr->sense_key == UNIT_ATTENTION && retry_count) {
> >> +		retry_count--;
> >> +		goto retry;
> >>  	}
> >>  
> >>  	return result;
> > 
> > Great, but shouldn't we be doing this more generally?  What about
> > scsi_mode_select()?
> > 
> I haven't seen any issues with scsi_mode_select() as of now, so I
> didn't do anything about this :-)
> 
> > (And, with the number of status changes that can get reported by
> >  UAs, we might want to think about increasing the retry count on
> >  these commands up from 3 at some point.)
> > 
> Hmm. _Actually_, we're not getting _more_ UAs (neither the number
> nor the situation at which UAs are being send has changed).
> It's just that we're trying to _use_ UAs so these things pop up.
> But yeah, raising the number or retries to eg 5 is probably a good idea.

Yeah, or maybe add a time limit to we don't drag out the boot time.
We can do this later, though, I'm fine with your patch as it is.

> 
> > Reviewed-by: Ewan D. Milne <emilne@redhat.com>
> > 
> Cheers,
> 
> Hannes

[PATCH] scsi: retry MODE SENSE on unit attention

[Hannes Reinecke]

The 'sd' driver is calling scsi_mode_sense() to figure out
internal details. But scsi_mode_sense() never checks for
any pending unit attentions, so we're getting annoying error
messages like:

MODE SENSE: unimplemented page/subpage: 0x00/0x00

and a possible wrong decision for device cache handling.

Reviewed-by: Ewan Milne <emilne@redhat.com>
Signed-off-by: Hannes Reinecke <hare@suse.de>
---
 drivers/scsi/scsi_lib.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index f04be65..b66652d 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -2423,7 +2423,7 @@ scsi_mode_sense(struct scsi_device *sdev, int dbd, int modepage,
 	unsigned char cmd[12];
 	int use_10_for_ms;
 	int header_length;
-	int result;
+	int result, retry_count = retries;
 	struct scsi_sense_hdr my_sshdr;
 
 	memset(data, 0, sizeof(*data));
@@ -2502,6 +2502,11 @@ scsi_mode_sense(struct scsi_device *sdev, int dbd, int modepage,
 			data->block_descriptor_length = buffer[3];
 		}
 		data->header_length = header_length;
+	} else if ((status_byte(result) == CHECK_CONDITION) &&
+		   scsi_sense_valid(sshdr) &&
+		   sshdr->sense_key == UNIT_ATTENTION && retry_count) {
+		retry_count--;
+		goto retry;
 	}
 
 	return result;
-- 
1.8.5.2

[PATCH] scsi: retry MODE SENSE on unit attention

[Hannes Reinecke]

The 'sd' driver is calling scsi_mode_sense() to figure out
internal details. But scsi_mode_sense() never checks for
any pending unit attentions, leading to incorrect decisions
during probing.

Signed-off-by: Hannes Reinecke <hare@suse.de>
Reviewed-by: Ewan Milne <emilne@redhat.com>
---
 drivers/scsi/scsi_lib.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index b1a2631..4f3c120 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -2423,7 +2423,7 @@ scsi_mode_sense(struct scsi_device *sdev, int dbd, int modepage,
 	unsigned char cmd[12];
 	int use_10_for_ms;
 	int header_length;
-	int result;
+	int result, retry_count = retries;
 	struct scsi_sense_hdr my_sshdr;
 
 	memset(data, 0, sizeof(*data));
@@ -2502,6 +2502,11 @@ scsi_mode_sense(struct scsi_device *sdev, int dbd, int modepage,
 			data->block_descriptor_length = buffer[3];
 		}
 		data->header_length = header_length;
+	} else if ((status_byte(result) == CHECK_CONDITION) &&
+		   scsi_sense_valid(sshdr) &&
+		   sshdr->sense_key == UNIT_ATTENTION && retry_count) {
+		retry_count--;
+		goto retry;
 	}
 
 	return result;
-- 
1.8.5.2

Re: [PATCH] scsi: retry MODE SENSE on unit attention

[Bart Van Assche]

On 07/06/2015 04:16 AM, Hannes Reinecke wrote:
> The 'sd' driver is calling scsi_mode_sense() to figure out
> internal details. But scsi_mode_sense() never checks for
> any pending unit attentions, leading to incorrect decisions
> during probing.
>
> Signed-off-by: Hannes Reinecke <hare@suse.de>
> Reviewed-by: Ewan Milne <emilne@redhat.com>
> ---
>   drivers/scsi/scsi_lib.c | 7 ++++++-
>   1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> index b1a2631..4f3c120 100644
> --- a/drivers/scsi/scsi_lib.c
> +++ b/drivers/scsi/scsi_lib.c
> @@ -2423,7 +2423,7 @@ scsi_mode_sense(struct scsi_device *sdev, int dbd, int modepage,
>   	unsigned char cmd[12];
>   	int use_10_for_ms;
>   	int header_length;
> -	int result;
> +	int result, retry_count = retries;
>   	struct scsi_sense_hdr my_sshdr;
>
>   	memset(data, 0, sizeof(*data));
> @@ -2502,6 +2502,11 @@ scsi_mode_sense(struct scsi_device *sdev, int dbd, int modepage,
>   			data->block_descriptor_length = buffer[3];
>   		}
>   		data->header_length = header_length;
> +	} else if ((status_byte(result) == CHECK_CONDITION) &&
> +		   scsi_sense_valid(sshdr) &&
> +		   sshdr->sense_key == UNIT_ATTENTION && retry_count) {
> +		retry_count--;
> +		goto retry;
>   	}
>
>   	return result;

Reviewed-by: Bart Van Assche <bart.vanassche@sandisk.com>