[PATCH V2 1/1] scsi/ses: Saw "Failed to get diagnostic page 0x1"

[PATCH V2 1/1] scsi/ses: Saw "Failed to get diagnostic page 0x1"

[wenxiong]

From: Wen Xiong <root@ltczz405-lp2.aus.stglabs.ibm.com>

We saw two errors with Slider drawer:
- Failed to get diagnostic page 0x1 during booting up
- /sys/class/enclosure is empty with ipr adapter + Slider drawer

From scsi logging level with error=3, looks ses_recv_diag not try on a UA.
The patch addes retrying on a UA in ses_recv_diag();

Signed-Off-by: Wen Xiong<wenxiong@linux.vnet.ibm.com>
Reviewed-by: Brian King <brking@linux.vnet.ibm.com>
---
 drivers/scsi/ses.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/ses.c b/drivers/scsi/ses.c
index c2afba2a5414..93f6a8ce1bea 100644
--- a/drivers/scsi/ses.c
+++ b/drivers/scsi/ses.c
@@ -87,9 +87,16 @@ static int ses_recv_diag(struct scsi_device *sdev, int page_code,
 		0
 	};
 	unsigned char recv_page_code;
+	struct scsi_sense_hdr sshdr;
+	int retries = SES_RETRIES;
+
+	do {
+		ret =  scsi_execute_req(sdev, cmd, DMA_FROM_DEVICE, buf,
+			bufflen, &sshdr, NULL, SES_TIMEOUT, SES_RETRIES, NULL);
+
+	} while (scsi_sense_valid(&sshdr) &&
+                 sshdr.sense_key == UNIT_ATTENTION && --retries);
 
-	ret =  scsi_execute_req(sdev, cmd, DMA_FROM_DEVICE, buf, bufflen,
-				NULL, SES_TIMEOUT, SES_RETRIES, NULL);
 	if (unlikely(ret))
 		return ret;
 
-- 
2.27.0

Re: [PATCH V2 1/1] scsi/ses: Saw "Failed to get diagnostic page 0x1"

[kernel test robot]

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

Hi,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on scsi/for-next]
[also build test ERROR on mkp-scsi/for-next v5.14 next-20210910]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/wenxiong-linux-vnet-ibm-com/scsi-ses-Saw-Failed-to-get-diagnostic-page-0x1/20210911-043434
base:   https://git.kernel.org/pub/scm/linux/kernel/git/jejb/scsi.git for-next
config: microblaze-buildonly-randconfig-r001-20210910 (attached as .config)
compiler: microblaze-linux-gcc (GCC) 11.2.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/a117eeeef2a13989a97ac0e10d86ffa6314f481e
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review wenxiong-linux-vnet-ibm-com/scsi-ses-Saw-Failed-to-get-diagnostic-page-0x1/20210911-043434
        git checkout a117eeeef2a13989a97ac0e10d86ffa6314f481e
        # save the attached .config to linux build tree
        mkdir build_dir
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross O=build_dir ARCH=microblaze SHELL=/bin/bash drivers/scsi/

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

All error/warnings (new ones prefixed by >>):

   In file included from include/uapi/linux/posix_types.h:5,
                    from include/uapi/linux/types.h:14,
                    from include/linux/types.h:6,
                    from include/linux/kasan-checks.h:5,
                    from include/asm-generic/rwonce.h:26,
                    from ./arch/microblaze/include/generated/asm/rwonce.h:1,
                    from include/linux/compiler.h:264,
                    from include/asm-generic/bug.h:5,
                    from ./arch/microblaze/include/generated/asm/bug.h:1,
                    from include/linux/bug.h:5,
                    from include/linux/mmdebug.h:5,
                    from include/linux/gfp.h:5,
                    from include/linux/slab.h:15,
                    from drivers/scsi/ses.c:8:
   drivers/scsi/ses.c: In function 'ses_recv_diag':
>> include/linux/stddef.h:8:14: warning: passing argument 7 of 'scsi_execute_req' makes integer from pointer without a cast [-Wint-conversion]
       8 | #define NULL ((void *)0)
         |              ^~~~~~~~~~~
         |              |
         |              void *
   drivers/scsi/ses.c:95:42: note: in expansion of macro 'NULL'
      95 |                         bufflen, &sshdr, NULL, SES_TIMEOUT, SES_RETRIES, NULL);
         |                                          ^~~~
   In file included from include/scsi/scsi_cmnd.h:12,
                    from drivers/scsi/ses.c:15:
   include/scsi/scsi_device.h:467:61: note: expected 'int' but argument is of type 'void *'
     467 |         unsigned bufflen, struct scsi_sense_hdr *sshdr, int timeout,
         |                                                         ~~~~^~~~~~~
>> drivers/scsi/ses.c:61:21: warning: passing argument 9 of 'scsi_execute_req' makes pointer from integer without a cast [-Wint-conversion]
      61 | #define SES_RETRIES 3
         |                     ^
         |                     |
         |                     int
   drivers/scsi/ses.c:95:61: note: in expansion of macro 'SES_RETRIES'
      95 |                         bufflen, &sshdr, NULL, SES_TIMEOUT, SES_RETRIES, NULL);
         |                                                             ^~~~~~~~~~~
   In file included from include/scsi/scsi_cmnd.h:12,
                    from drivers/scsi/ses.c:15:
   include/scsi/scsi_device.h:468:27: note: expected 'int *' but argument is of type 'int'
     468 |         int retries, int *resid)
         |                      ~~~~~^~~~~
>> drivers/scsi/ses.c:94:24: error: too many arguments to function 'scsi_execute_req'
      94 |                 ret =  scsi_execute_req(sdev, cmd, DMA_FROM_DEVICE, buf,
         |                        ^~~~~~~~~~~~~~~~
   In file included from include/scsi/scsi_cmnd.h:12,
                    from drivers/scsi/ses.c:15:
   include/scsi/scsi_device.h:465:19: note: declared here
     465 | static inline int scsi_execute_req(struct scsi_device *sdev,
         |                   ^~~~~~~~~~~~~~~~


vim +/scsi_execute_req +94 drivers/scsi/ses.c

    59	
    60	#define SES_TIMEOUT (30 * HZ)
  > 61	#define SES_RETRIES 3
    62	
    63	static void init_device_slot_control(unsigned char *dest_desc,
    64					     struct enclosure_component *ecomp,
    65					     unsigned char *status)
    66	{
    67		memcpy(dest_desc, status, 4);
    68		dest_desc[0] = 0;
    69		/* only clear byte 1 for ENCLOSURE_COMPONENT_DEVICE */
    70		if (ecomp->type == ENCLOSURE_COMPONENT_DEVICE)
    71			dest_desc[1] = 0;
    72		dest_desc[2] &= 0xde;
    73		dest_desc[3] &= 0x3c;
    74	}
    75	
    76	
    77	static int ses_recv_diag(struct scsi_device *sdev, int page_code,
    78				 void *buf, int bufflen)
    79	{
    80		int ret;
    81		unsigned char cmd[] = {
    82			RECEIVE_DIAGNOSTIC,
    83			1,		/* Set PCV bit */
    84			page_code,
    85			bufflen >> 8,
    86			bufflen & 0xff,
    87			0
    88		};
    89		unsigned char recv_page_code;
    90		struct scsi_sense_hdr sshdr;
    91		int retries = SES_RETRIES;
    92	
    93		do {
  > 94			ret =  scsi_execute_req(sdev, cmd, DMA_FROM_DEVICE, buf,
    95				bufflen, &sshdr, NULL, SES_TIMEOUT, SES_RETRIES, NULL);
    96	
    97		} while (scsi_sense_valid(&sshdr) &&
    98	                 sshdr.sense_key == UNIT_ATTENTION && --retries);
    99	
   100		if (unlikely(ret))
   101			return ret;
   102	
   103		recv_page_code = ((unsigned char *)buf)[0];
   104	
   105		if (likely(recv_page_code == page_code))
   106			return ret;
   107	
   108		/* successful diagnostic but wrong page code.  This happens to some
   109		 * USB devices, just print a message and pretend there was an error */
   110	
   111		sdev_printk(KERN_ERR, sdev,
   112			    "Wrong diagnostic page; asked for %d got %u\n",
   113			    page_code, recv_page_code);
   114	
   115		return -EINVAL;
   116	}
   117	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

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

Re: [PATCH V2 1/1] scsi/ses: Saw "Failed to get diagnostic page 0x1"

[kernel test robot]

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

Hi,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on scsi/for-next]
[also build test ERROR on mkp-scsi/for-next v5.14 next-20210910]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/wenxiong-linux-vnet-ibm-com/scsi-ses-Saw-Failed-to-get-diagnostic-page-0x1/20210911-043434
base:   https://git.kernel.org/pub/scm/linux/kernel/git/jejb/scsi.git for-next
config: microblaze-buildonly-randconfig-r001-20210910 (attached as .config)
compiler: microblaze-linux-gcc (GCC) 11.2.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/a117eeeef2a13989a97ac0e10d86ffa6314f481e
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review wenxiong-linux-vnet-ibm-com/scsi-ses-Saw-Failed-to-get-diagnostic-page-0x1/20210911-043434
        git checkout a117eeeef2a13989a97ac0e10d86ffa6314f481e
        # save the attached .config to linux build tree
        mkdir build_dir
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross O=build_dir ARCH=microblaze SHELL=/bin/bash drivers/scsi/

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

All error/warnings (new ones prefixed by >>):

   In file included from include/uapi/linux/posix_types.h:5,
                    from include/uapi/linux/types.h:14,
                    from include/linux/types.h:6,
                    from include/linux/kasan-checks.h:5,
                    from include/asm-generic/rwonce.h:26,
                    from ./arch/microblaze/include/generated/asm/rwonce.h:1,
                    from include/linux/compiler.h:264,
                    from include/asm-generic/bug.h:5,
                    from ./arch/microblaze/include/generated/asm/bug.h:1,
                    from include/linux/bug.h:5,
                    from include/linux/mmdebug.h:5,
                    from include/linux/gfp.h:5,
                    from include/linux/slab.h:15,
                    from drivers/scsi/ses.c:8:
   drivers/scsi/ses.c: In function 'ses_recv_diag':
>> include/linux/stddef.h:8:14: warning: passing argument 7 of 'scsi_execute_req' makes integer from pointer without a cast [-Wint-conversion]
       8 | #define NULL ((void *)0)
         |              ^~~~~~~~~~~
         |              |
         |              void *
   drivers/scsi/ses.c:95:42: note: in expansion of macro 'NULL'
      95 |                         bufflen, &sshdr, NULL, SES_TIMEOUT, SES_RETRIES, NULL);
         |                                          ^~~~
   In file included from include/scsi/scsi_cmnd.h:12,
                    from drivers/scsi/ses.c:15:
   include/scsi/scsi_device.h:467:61: note: expected 'int' but argument is of type 'void *'
     467 |         unsigned bufflen, struct scsi_sense_hdr *sshdr, int timeout,
         |                                                         ~~~~^~~~~~~
>> drivers/scsi/ses.c:61:21: warning: passing argument 9 of 'scsi_execute_req' makes pointer from integer without a cast [-Wint-conversion]
      61 | #define SES_RETRIES 3
         |                     ^
         |                     |
         |                     int
   drivers/scsi/ses.c:95:61: note: in expansion of macro 'SES_RETRIES'
      95 |                         bufflen, &sshdr, NULL, SES_TIMEOUT, SES_RETRIES, NULL);
         |                                                             ^~~~~~~~~~~
   In file included from include/scsi/scsi_cmnd.h:12,
                    from drivers/scsi/ses.c:15:
   include/scsi/scsi_device.h:468:27: note: expected 'int *' but argument is of type 'int'
     468 |         int retries, int *resid)
         |                      ~~~~~^~~~~
>> drivers/scsi/ses.c:94:24: error: too many arguments to function 'scsi_execute_req'
      94 |                 ret =  scsi_execute_req(sdev, cmd, DMA_FROM_DEVICE, buf,
         |                        ^~~~~~~~~~~~~~~~
   In file included from include/scsi/scsi_cmnd.h:12,
                    from drivers/scsi/ses.c:15:
   include/scsi/scsi_device.h:465:19: note: declared here
     465 | static inline int scsi_execute_req(struct scsi_device *sdev,
         |                   ^~~~~~~~~~~~~~~~


vim +/scsi_execute_req +94 drivers/scsi/ses.c

    59	
    60	#define SES_TIMEOUT (30 * HZ)
  > 61	#define SES_RETRIES 3
    62	
    63	static void init_device_slot_control(unsigned char *dest_desc,
    64					     struct enclosure_component *ecomp,
    65					     unsigned char *status)
    66	{
    67		memcpy(dest_desc, status, 4);
    68		dest_desc[0] = 0;
    69		/* only clear byte 1 for ENCLOSURE_COMPONENT_DEVICE */
    70		if (ecomp->type == ENCLOSURE_COMPONENT_DEVICE)
    71			dest_desc[1] = 0;
    72		dest_desc[2] &= 0xde;
    73		dest_desc[3] &= 0x3c;
    74	}
    75	
    76	
    77	static int ses_recv_diag(struct scsi_device *sdev, int page_code,
    78				 void *buf, int bufflen)
    79	{
    80		int ret;
    81		unsigned char cmd[] = {
    82			RECEIVE_DIAGNOSTIC,
    83			1,		/* Set PCV bit */
    84			page_code,
    85			bufflen >> 8,
    86			bufflen & 0xff,
    87			0
    88		};
    89		unsigned char recv_page_code;
    90		struct scsi_sense_hdr sshdr;
    91		int retries = SES_RETRIES;
    92	
    93		do {
  > 94			ret =  scsi_execute_req(sdev, cmd, DMA_FROM_DEVICE, buf,
    95				bufflen, &sshdr, NULL, SES_TIMEOUT, SES_RETRIES, NULL);
    96	
    97		} while (scsi_sense_valid(&sshdr) &&
    98	                 sshdr.sense_key == UNIT_ATTENTION && --retries);
    99	
   100		if (unlikely(ret))
   101			return ret;
   102	
   103		recv_page_code = ((unsigned char *)buf)[0];
   104	
   105		if (likely(recv_page_code == page_code))
   106			return ret;
   107	
   108		/* successful diagnostic but wrong page code.  This happens to some
   109		 * USB devices, just print a message and pretend there was an error */
   110	
   111		sdev_printk(KERN_ERR, sdev,
   112			    "Wrong diagnostic page; asked for %d got %u\n",
   113			    page_code, recv_page_code);
   114	
   115		return -EINVAL;
   116	}
   117	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

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

Re: [PATCH V2 1/1] scsi/ses: Saw "Failed to get diagnostic page 0x1"

[James Bottomley]

On Fri, 2021-09-10 at 14:04 -0500, wenxiong@linux.vnet.ibm.com wrote:
> From: Wen Xiong <root@ltczz405-lp2.aus.stglabs.ibm.com>
> 
> We saw two errors with Slider drawer:
> - Failed to get diagnostic page 0x1 during booting up
> - /sys/class/enclosure is empty with ipr adapter + Slider drawer
> 
> From scsi logging level with error=3, looks ses_recv_diag not try on
> a UA. The patch addes retrying on a UA in ses_recv_diag();

Do we know why the device is returning a UA?  Presumably it's a check
condition UA meaning the device is trying to tell us something and
wants us to request sense?

> Signed-Off-by: Wen Xiong<wenxiong@linux.vnet.ibm.com>
> Reviewed-by: Brian King <brking@linux.vnet.ibm.com>
> ---
>  drivers/scsi/ses.c | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/scsi/ses.c b/drivers/scsi/ses.c
> index c2afba2a5414..93f6a8ce1bea 100644
> --- a/drivers/scsi/ses.c
> +++ b/drivers/scsi/ses.c
> @@ -87,9 +87,16 @@ static int ses_recv_diag(struct scsi_device *sdev,
> int page_code,
>  		0
>  	};
>  	unsigned char recv_page_code;
> +	struct scsi_sense_hdr sshdr;
> +	int retries = SES_RETRIES;
> +
> +	do {
> +		ret =  scsi_execute_req(sdev, cmd, DMA_FROM_DEVICE,
> buf,
> +			bufflen, &sshdr, NULL, SES_TIMEOUT, 

This grew an additional argument: you want to replace the NULL with the
sshdr, I think ... and compile test it next time.

> SES_RETRIES, NULL);

I think you want a 1 here instead of SES_RETRIES because you're
retrying for SES_RETRIES in an outer loop now, so if you maxed out both
sets of retries, you'd retry for SES_RETRIES^2.

If this is a CC/UA, you can simplify all this by setting

cmd->expecting_cc_ua = 1

and avoiding the loop.

James

Re: [PATCH V2 1/1] scsi/ses: Saw "Failed to get diagnostic page 0x1"

[Martin K. Petersen]

Hi Wendy!

> I am going to re-do V2 patch with retries. Is it ok?

We probably do not want to blindly retry on all errors. We should only
retry in cases where the command has the potential to subsequently
succeed. Hence the request to retry "transient errors" in my previous
mail.

Classifying which errors should result in a retry is the important
piece. And for that we need to know exactly what error your tray is
returning.

I also suggest you have a look at scsi_check_sense().

Thanks!

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH V2 1/1] scsi/ses: Saw "Failed to get diagnostic page 0x1"

[Martin K. Petersen]

Wendy,

> Below is error with enabling scsi_log_level;
>  
> [108017.427791] ses 0:0:9:0: tag#641 Sense Key : Unit Attention [current]
> [108017.427793] ses 0:0:9:0: tag#641 Add. Sense: Bus device reset function occurred

OK. I propose you refine your check to retry on NOT_READY as well as
UNIT_ATTENTION with ASC 0x29. I think that would be a good start. Do the
same for ses_send_diag().

Thanks!

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH V2 1/1] scsi/ses: Saw "Failed to get diagnostic page 0x1"

[James Bottomley]

On Tue, 2021-09-14 at 11:27 -0400, Martin K. Petersen wrote:
> Wendy,
> 
> > Below is error with enabling scsi_log_level;
> >  
> > [108017.427791] ses 0:0:9:0: tag#641 Sense Key : Unit Attention
> > [current]
> > [108017.427793] ses 0:0:9:0: tag#641 Add. Sense: Bus device reset
> > function occurred
> 
> OK. I propose you refine your check to retry on NOT_READY as well as
> UNIT_ATTENTION with ASC 0x29. I think that would be a good start. Do
> the same for ses_send_diag().

Something is wrong with the outbound email: the list isn't getting
copies of this:

https://lore.kernel.org/all/yq1v934yysg.fsf@ca-mkp.ca.oracle.com/

because you're sending HTML email ... please don't.

I still think setting expecting_cc_ua is the best thing to do because
it's designed for exactly the problem you're seeing, but open coding it
in the loop would be fine as well.

James

Re: [PATCH V2 1/1] scsi/ses: Saw "Failed to get diagnostic page 0x1"

[Martin K. Petersen]

Wendy,

A few small additional nits...

> From: Wen Xiong <root@ltczz405-lp2.aus.stglabs.ibm.com>

Please make sure your email address is correct.

> Signed-Off-by: Wen Xiong<wenxiong@linux.vnet.ibm.com>

This should be Signed-off-by: and you need a space before your email
address.

> +		ret =  scsi_execute_req(sdev, cmd, DMA_FROM_DEVICE, buf,
> +			bufflen, &sshdr, NULL, SES_TIMEOUT, SES_RETRIES, NULL);

The sense header goes in the field before SES_TIMEOUT:

int __scsi_execute(struct scsi_device *sdev, const unsigned char *cmd,
                 int data_direction, void *buffer, unsigned bufflen,
                 unsigned char *sense, struct scsi_sense_hdr *sshdr,
                                       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
                 int timeout, int retries, u64 flags, req_flags_t rq_flags,
                 ^^^^^^^^^^^
                 int *resid)

Thanks!

PS. Bonus points for whoever fixes up the scsi_execute calls to have
less than 10,000 arguments. One would be good. And some sensible input
validation/type checking.

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH V2 1/1] scsi/ses: Saw "Failed to get diagnostic page 0x1"

[Martin K. Petersen]

James,

> I still think setting expecting_cc_ua is the best thing to do because
> it's designed for exactly the problem you're seeing, but open coding
> it in the loop would be fine as well.

The problem with that approach is that (as far as I can tell) we didn't
issue the reset in question.

We could explicitly set the flag in ses_recv_diag(). However, even
though scsi_check_sense() returns RETRY, scsi_decide_disposition()
bypasses it because a check condition is considered a "no retry" command
by scsi_noretry_cmd(). So despite expecting_cc_ua being set, the retry
is not performed.

Fun can of worms...

-- 
Martin K. Petersen	Oracle Linux Engineering