scsi_report_lun_scan bug?

scsi_report_lun_scan bug?

[Amit Patel]

Hi,

I am using 2.6-test9-mm3. I noticed while doing
scsi_report_lun_scan(scsi_scan.c:891) the data
returned is assigned(scsi_scan.c:993) to signed char
array which causes the reported number of luns to be
huge while calculating num_luns to scan. Is there any
particular reason to be data is signed or just a bug?

I changed it to unsigned char and it seems to work
fine. I have attached a diff of scsi_scan.c. Let me
know if I am missing something.

Thanks
Amit

[root@Host200-w2k root]# diff
/cdrive/mm1/linux-2.6.0-test9/drivers/scsi/scsi_scan.c
/cdrive/mm3/linux-2.6.0-test9/drivers/scsi/scsi_scan.c
902c902
<       char *data;
---
>       unsigned char *data;
993c993
<       data = (char *) lun_data->scsi_lun;
---
>       data = (unsigned char *) lun_data->scsi_lun;
[root@Host200-w2k root]# 
 

__________________________________
Do you Yahoo!?
Protect your identity with Yahoo! Mail AddressGuard
http://antispam.yahoo.com/whatsnewfree

Re: scsi_report_lun_scan bug?

[Matthew Wilcox]

On Mon, Nov 17, 2003 at 06:48:33PM -0800, Amit Patel wrote:
> [root@Host200-w2k root]# diff
> /cdrive/mm1/linux-2.6.0-test9/drivers/scsi/scsi_scan.c
> /cdrive/mm3/linux-2.6.0-test9/drivers/scsi/scsi_scan.c
> 902c902
> <       char *data;
> ---
> >       unsigned char *data;

Hi Amit.  Can you send diffs in unified format in the future, ie diff -u
Thanks.

-- 
"It's not Hollywood.  War is real, war is primarily not about defeat or
victory, it is about death.  I've seen thousands and thousands of dead bodies.
Do you think I want to have an academic debate on this subject?" -- Robert Fisk

Re: scsi_report_lun_scan bug?

[Amit Patel]

Oops sorry did not know about that...

Here it is.

[root@Host200-w2k root]# diff -u
/cdrive/mm1/linux-2.6.0-test9/drivers/scsi/scsi_scan.c
/cdrive/mm3/linux-2.6.0-test9/drivers/scsi/scsi_scan.c
---
/cdrive/mm1/linux-2.6.0-test9/drivers/scsi/scsi_scan.c
     2003-11-04 11:52:30.000000000 -0800
+++
/cdrive/mm3/linux-2.6.0-test9/drivers/scsi/scsi_scan.c
     2003-11-17 18:25:30.534512992 -0800
@@ -899,7 +899,7 @@
        unsigned int retries;
        struct scsi_lun *lunp, *lun_data;
        struct scsi_request *sreq;
-       char *data;
+       unsigned char *data;
 
        /*
         * Only support SCSI-3 and up devices.
@@ -990,7 +990,7 @@
        /*
         * Get the length from the first four bytes of
lun_data.
         */
-       data = (char *) lun_data->scsi_lun;
+       data = (unsigned char *) lun_data->scsi_lun;
        length = ((data[0] << 24) | (data[1] << 16) |
                  (data[2] << 8) | (data[3] << 0));
 
[root@Host200-w2k root]# 
[root@Host200-w2k root]# 

--- Matthew Wilcox <willy@debian.org> wrote:
> On Mon, Nov 17, 2003 at 06:48:33PM -0800, Amit Patel
> wrote:
> > [root@Host200-w2k root]# diff
> >
>
/cdrive/mm1/linux-2.6.0-test9/drivers/scsi/scsi_scan.c
> >
>
/cdrive/mm3/linux-2.6.0-test9/drivers/scsi/scsi_scan.c
> > 902c902
> > <       char *data;
> > ---
> > >       unsigned char *data;
> 
> Hi Amit.  Can you send diffs in unified format in
> the future, ie diff -u
> Thanks.
> 
> -- 
> "It's not Hollywood.  War is real, war is primarily
> not about defeat or
> victory, it is about death.  I've seen thousands and
> thousands of dead bodies.
> Do you think I want to have an academic debate on
> this subject?" -- Robert Fisk


__________________________________
Do you Yahoo!?
Protect your identity with Yahoo! Mail AddressGuard
http://antispam.yahoo.com/whatsnewfree

Re: scsi_report_lun_scan bug?

[Patrick Mansfield]

On Mon, Nov 17, 2003 at 06:48:33PM -0800, Amit Patel wrote:
> Hi,
> 
> I am using 2.6-test9-mm3. I noticed while doing
> scsi_report_lun_scan(scsi_scan.c:891) the data
> returned is assigned(scsi_scan.c:993) to signed char
> array which causes the reported number of luns to be
> huge while calculating num_luns to scan. Is there any
> particular reason to be data is signed or just a bug?
> 
> I changed it to unsigned char and it seems to work
> fine. I have attached a diff of scsi_scan.c. Let me
> know if I am missing something.

I don't see why making it signed or unsigned would make any difference.

What values did you see before and after your patch?

It should really be a u8, since it is a pointer to an array of bytes.

(And all the scsi_cmd[]'s should be u8.)

-- Patrick Mansfield

Re: scsi_report_lun_scan bug?

[Andrew Morton]

Patrick Mansfield <patmans@us.ibm.com> wrote:
>
> On Mon, Nov 17, 2003 at 06:48:33PM -0800, Amit Patel wrote:
> > Hi,
> > 
> > I am using 2.6-test9-mm3. I noticed while doing
> > scsi_report_lun_scan(scsi_scan.c:891) the data
> > returned is assigned(scsi_scan.c:993) to signed char
> > array which causes the reported number of luns to be
> > huge while calculating num_luns to scan. Is there any
> > particular reason to be data is signed or just a bug?
> > 
> > I changed it to unsigned char and it seems to work
> > fine. I have attached a diff of scsi_scan.c. Let me
> > know if I am missing something.
> 
> I don't see why making it signed or unsigned would make any difference.


 	length = ((data[0] << 24) | (data[1] << 16) |
 		  (data[2] << 8) | (data[3] << 0));

If data[3] is 0xff, this expression will always evaluate to
0xffffffff.  etcetera.

> It should really be a u8, since it is a pointer to an array of bytes.
> 
> (And all the scsi_cmd[]'s should be u8.)

Yup.

diff -puN drivers/scsi/scsi_scan.c~scsi_report_lun-fix drivers/scsi/scsi_scan.c
--- 25/drivers/scsi/scsi_scan.c~scsi_report_lun-fix	2003-11-17 20:22:49.000000000 -0800
+++ 25-akpm/drivers/scsi/scsi_scan.c	2003-11-17 20:22:49.000000000 -0800
@@ -899,7 +899,7 @@ static int scsi_report_lun_scan(struct s
 	unsigned int retries;
 	struct scsi_lun *lunp, *lun_data;
 	struct scsi_request *sreq;
-	char *data;
+	u8 *data;
 
 	/*
 	 * Only support SCSI-3 and up devices.
@@ -990,7 +990,7 @@ static int scsi_report_lun_scan(struct s
 	/*
 	 * Get the length from the first four bytes of lun_data.
 	 */
-	data = (char *) lun_data->scsi_lun;
+	data = (u8 *) lun_data->scsi_lun;
 	length = ((data[0] << 24) | (data[1] << 16) |
 		  (data[2] << 8) | (data[3] << 0));
 

_

Re: scsi_report_lun_scan bug?

[Amit Patel]

Patrick,

In my case, I have 16 luns for which report lun
command byte 3 has value 0x80. While doing the length
calculation 
length = ((data[0] << 24) | (data[1] << 16) |
          (data[2] << 8) | (data[3] << 0));

It calucates length as 0xffffff80 and actual number of
luns is calculated as 536870896. I just write small
program with signed and unsigned char array and it
does evaluate as 0xffffff80 as Andrew mentioned.

Thanks
Amit


#include<stdio.h>
main()
{
        char x[10]={0x00,0x00,0x00,0x80};
        char *p;
        unsigned char *q;

        int len;
        int ulen;

        p=(char *)x;
        q=(unsigned char *) x;
        len=((p[0] << 24) | (p[1] << 16) | (p[2] << 8)
| (p[3] <<0));
        ulen=((q[0] << 24) |(q[1] << 16) | (q[2] << 8)
| (q[3] <<0));
        printf("len= %d len=%x\n",len,len);
        printf("ulen= %d ulen=%x\n",ulen,ulen);
}




 
--- Andrew Morton <akpm@osdl.org> wrote:
> Patrick Mansfield <patmans@us.ibm.com> wrote:
> >
> > On Mon, Nov 17, 2003 at 06:48:33PM -0800, Amit
> Patel wrote:
> > > Hi,
> > > 
> > > I am using 2.6-test9-mm3. I noticed while doing
> > > scsi_report_lun_scan(scsi_scan.c:891) the data
> > > returned is assigned(scsi_scan.c:993) to signed
> char
> > > array which causes the reported number of luns
> to be
> > > huge while calculating num_luns to scan. Is
> there any
> > > particular reason to be data is signed or just a
> bug?
> > > 
> > > I changed it to unsigned char and it seems to
> work
> > > fine. I have attached a diff of scsi_scan.c. Let
> me
> > > know if I am missing something.
> > 
> > I don't see why making it signed or unsigned would
> make any difference.
> 
> 
>  	length = ((data[0] << 24) | (data[1] << 16) |
>  		  (data[2] << 8) | (data[3] << 0));
> 
> If data[3] is 0xff, this expression will always
> evaluate to
> 0xffffffff.  etcetera.
> 
> > It should really be a u8, since it is a pointer to
> an array of bytes.
> > 
> > (And all the scsi_cmd[]'s should be u8.)
> 
> Yup.
> 
> diff -puN
> drivers/scsi/scsi_scan.c~scsi_report_lun-fix
> drivers/scsi/scsi_scan.c
> --- 25/drivers/scsi/scsi_scan.c~scsi_report_lun-fix
> 2003-11-17 20:22:49.000000000 -0800
> +++ 25-akpm/drivers/scsi/scsi_scan.c	2003-11-17
> 20:22:49.000000000 -0800
> @@ -899,7 +899,7 @@ static int
> scsi_report_lun_scan(struct s
>  	unsigned int retries;
>  	struct scsi_lun *lunp, *lun_data;
>  	struct scsi_request *sreq;
> -	char *data;
> +	u8 *data;
>  
>  	/*
>  	 * Only support SCSI-3 and up devices.
> @@ -990,7 +990,7 @@ static int
> scsi_report_lun_scan(struct s
>  	/*
>  	 * Get the length from the first four bytes of
> lun_data.
>  	 */
> -	data = (char *) lun_data->scsi_lun;
> +	data = (u8 *) lun_data->scsi_lun;
>  	length = ((data[0] << 24) | (data[1] << 16) |
>  		  (data[2] << 8) | (data[3] << 0));
>  
> 
> _
> 


__________________________________
Do you Yahoo!?
Protect your identity with Yahoo! Mail AddressGuard
http://antispam.yahoo.com/whatsnewfree

State Model for scsi device badness

[Amit Patel]

Hi James,

I was trying updated state model for scsi device
patch. I have 2.6.0-test9-mm3 and QLogic 2300 adapter.
While loading the qlogic driver I see lot of Kobject
badness messages. 

I am not really familiar with kobject/scsi code but is
it because while doing the scan of the scsi devices we
have really not setup kobject for the device but we
are sending INQUIRY command to see if there are any
devices attached to the target. This code path will go
through scsi_request_fn and as part of the patch first
thing in scsi_request_fn is get_device. This is where
the badness message is coming from. 

I don't see this messages once initial scan is done. I
am not sure what other information will be helpful but
if you need any more information please let me know.

Thanks
Amit

Badness in kobject_get at lib/kobject.c:440
Call Trace:
 [<c023cd8c>] kobject_get+0x4c/0x50
 [<c0299818>] get_device+0x18/0x30
 [<f88a5b36>] scsi_request_fn+0x26/0x980 [scsi_mod]
 [<f889d64e>] scsi_put_command+0x1ee/0x350 [scsi_mod]
 [<c029fb72>] blk_run_queue+0xa2/0x1a0
 [<f88a41ca>] scsi_wait_req+0xea/0x100 [scsi_mod]
 [<f88a3ef0>] scsi_wait_done+0x0/0x1f0 [scsi_mod]
 [<c015faeb>] check_poison_obj+0x2b/0x1a0
 [<f88a7089>] scsi_probe_lun+0x99/0x2e0 [scsi_mod]
 [<f889d028>] scsi_allocate_request+0x28/0x70
[scsi_mod]
 [<f88a7a22>] scsi_probe_and_add_lun+0x112/0x200
[scsi_mod]
 [<f88a7a3b>] scsi_probe_and_add_lun+0x12b/0x200
[scsi_mod]
 [<f88a8274>] scsi_scan_target+0x94/0x110 [scsi_mod]
 [<f88a834b>] scsi_scan_channel+0x5b/0xa0 [scsi_mod]
 [<f88a8482>] scsi_scan_host_selected+0xf2/0x140
[scsi_mod]
 [<f88a8501>] scsi_scan_host+0x31/0x40 [scsi_mod]
 [<f890a3ee>] qla2x00_probe_device+0xb0e/0xe50
[qla2300]
 [<c01ad923>] d_instantiate+0x123/0x1c0
 [<c01aa9e0>] dput+0x30/0x780
 [<c01e4400>] sysfs_new_inode+0x30/0xb0
 [<c02495cd>] pci_device_probe_static+0x4d/0x60
 [<c024961c>] __pci_device_probe+0x3c/0x50
 [<c024965c>] pci_device_probe+0x2c/0x50
 [<c029a7dd>] bus_match+0x3d/0x70
 [<c029a91c>] driver_attach+0x5c/0x90
 [<c029ac14>] bus_add_driver+0xa4/0xc0
 [<c029b0d6>] driver_register+0x96/0xa0
 [<c0249879>] pci_register_driver+0x99/0xc0
 [<f887305e>] qla2x00_module_init+0x5e/0x8a [qla2300]
 [<f8904000>] qla2x00_set_info+0x0/0x10 [qla2300]
 [<c0150306>] sys_init_module+0x1f6/0x3d0
 [<c039d942>] sysenter_past_esp+0x43/0x65


__________________________________
Do you Yahoo!?
Free Pop-Up Blocker - Get it now
http://companion.yahoo.com/

Re: State Model for scsi device badness PATCH

[Amit Patel]

James,

I just walked through some of the code and I could
work around the badness problem by not doing
get_device if the scsi device is not in running state
yet. I have attached the patch for scsi_lib.c where I
do not call get_device if the state is not
SDEV_RUNNING and similarly do not do put_device if you
have not done get_device because of the device state.

I am not sure whether this will introduce some other
problem but something which I tried and worked. Let me
know if I am missing something.

Thanks
Amit
 diff -u 
/cdrive/mm1/linux-2.6.0-test9/drivers/scsi/scsi_lib.c
scsi_lib.c
---
/cdrive/mm1/linux-2.6.0-test9/drivers/scsi/scsi_lib.c 
     2003-10-25 11:43:38.000000000 -0700
+++ scsi_lib.c  2003-11-20 09:20:42.507430872 -0800
@@ -917,6 +917,22 @@
 {
        struct scsi_device *sdev = q->queuedata;
        struct scsi_cmnd *cmd;
+       int specials_only = 0;
+
+       if(unlikely(sdev->sdev_state != SDEV_RUNNING))
{
+               /* OK, we're not in a running state
don't prep
+                * user commands */
+               if(sdev->sdev_state == SDEV_DEL) {
+                       /* Device is fully deleted, no
commands
+                        * at all allowed down */
+                       printk(KERN_ERR "scsi%d
(%d:%d): rejecting I/O to dead device\n",
+                              sdev->host->host_no,
sdev->id, sdev->lun);
+                       return BLKPREP_KILL;
+               }
+               /* OK, we only allow special commands
(i.e. not
+                * user initiated ones */
+               specials_only = 1;
+       }

        /*
         * Find the actual device driver associated
with this command.
@@ -939,6 +955,14 @@
                } else
                        cmd = req->special;
        } else if (req->flags & (REQ_CMD |
REQ_BLOCK_PC)) {
+
+               if(unlikely(specials_only)) {
+                       printk(KERN_ERR "scsi%d
(%d:%d): rejecting I/O to device being removed\n",
+                              sdev->host->host_no,
sdev->id, sdev->lun);
+                       return BLKPREP_KILL;
+               }
+
+
                /*
                 * Just check to see if the device is
online.  If
                 * it isn't, we refuse to process
ordinary commands
@@ -1120,6 +1144,11 @@
        struct Scsi_Host *shost = sdev->host;
        struct scsi_cmnd *cmd;
        struct request *req;
+
+       if(sdev->sdev_state == SDEV_RUNNING)
+               if(!get_device(&sdev->sdev_gendev))
+               /* We must be tearing the block queue
down already */
+                       return;

        /*
         * To start with, we keep looping until the
queue is empty, or until
@@ -1192,9 +1221,11 @@
                        break;
                }
        }
-
+       if(sdev->sdev_state == SDEV_RUNNING)
+               goto out;
        return;

+
  not_ready:
        spin_unlock_irq(shost->host_lock);

@@ -1211,6 +1242,12 @@
        sdev->device_busy--;
        if(sdev->device_busy == 0)
                blk_plug_device(q);
+ out:
+       /* must be careful here...if we trigger the
->remove() function
+        * we cannot be holding the q lock */
+       spin_unlock_irq(q->queue_lock);
+       put_device(&sdev->sdev_gendev);
+       spin_lock_irq(q->queue_lock);
 }

 u64 scsi_calculate_bounce_limit(struct Scsi_Host
*shost)

--- Amit Patel <patelamitv@yahoo.com> wrote:
> Hi James,
> 
> I was trying updated state model for scsi device
> patch. I have 2.6.0-test9-mm3 and QLogic 2300
> adapter.
> While loading the qlogic driver I see lot of Kobject
> badness messages. 
> 
> I am not really familiar with kobject/scsi code but
> is
> it because while doing the scan of the scsi devices
> we
> have really not setup kobject for the device but we
> are sending INQUIRY command to see if there are any
> devices attached to the target. This code path will
> go
> through scsi_request_fn and as part of the patch
> first
> thing in scsi_request_fn is get_device. This is
> where
> the badness message is coming from. 
> 
> I don't see this messages once initial scan is done.
> I
> am not sure what other information will be helpful
> but
> if you need any more information please let me know.
> 
> Thanks
> Amit
> 
> Badness in kobject_get at lib/kobject.c:440
> Call Trace:
>  [<c023cd8c>] kobject_get+0x4c/0x50
>  [<c0299818>] get_device+0x18/0x30
>  [<f88a5b36>] scsi_request_fn+0x26/0x980 [scsi_mod]
>  [<f889d64e>] scsi_put_command+0x1ee/0x350
> [scsi_mod]
>  [<c029fb72>] blk_run_queue+0xa2/0x1a0
>  [<f88a41ca>] scsi_wait_req+0xea/0x100 [scsi_mod]
>  [<f88a3ef0>] scsi_wait_done+0x0/0x1f0 [scsi_mod]
>  [<c015faeb>] check_poison_obj+0x2b/0x1a0
>  [<f88a7089>] scsi_probe_lun+0x99/0x2e0 [scsi_mod]
>  [<f889d028>] scsi_allocate_request+0x28/0x70
> [scsi_mod]
>  [<f88a7a22>] scsi_probe_and_add_lun+0x112/0x200
> [scsi_mod]
>  [<f88a7a3b>] scsi_probe_and_add_lun+0x12b/0x200
> [scsi_mod]
>  [<f88a8274>] scsi_scan_target+0x94/0x110 [scsi_mod]
>  [<f88a834b>] scsi_scan_channel+0x5b/0xa0 [scsi_mod]
>  [<f88a8482>] scsi_scan_host_selected+0xf2/0x140
> [scsi_mod]
>  [<f88a8501>] scsi_scan_host+0x31/0x40 [scsi_mod]
>  [<f890a3ee>] qla2x00_probe_device+0xb0e/0xe50
> [qla2300]
>  [<c01ad923>] d_instantiate+0x123/0x1c0
>  [<c01aa9e0>] dput+0x30/0x780
>  [<c01e4400>] sysfs_new_inode+0x30/0xb0
>  [<c02495cd>] pci_device_probe_static+0x4d/0x60
>  [<c024961c>] __pci_device_probe+0x3c/0x50
>  [<c024965c>] pci_device_probe+0x2c/0x50
>  [<c029a7dd>] bus_match+0x3d/0x70
>  [<c029a91c>] driver_attach+0x5c/0x90
>  [<c029ac14>] bus_add_driver+0xa4/0xc0
>  [<c029b0d6>] driver_register+0x96/0xa0
>  [<c0249879>] pci_register_driver+0x99/0xc0
>  [<f887305e>] qla2x00_module_init+0x5e/0x8a
> [qla2300]
>  [<f8904000>] qla2x00_set_info+0x0/0x10 [qla2300]
>  [<c0150306>] sys_init_module+0x1f6/0x3d0
>  [<c039d942>] sysenter_past_esp+0x43/0x65
> 
> 
> __________________________________
> Do you Yahoo!?
> Free Pop-Up Blocker - Get it now
> http://companion.yahoo.com/


__________________________________
Do you Yahoo!?
Free Pop-Up Blocker - Get it now
http://companion.yahoo.com/

Re: State Model for scsi device badness PATCH

[James Bottomley]

On Thu, 2003-11-20 at 13:01, Amit Patel wrote:
> I just walked through some of the code and I could
> work around the badness problem by not doing
> get_device if the scsi device is not in running state
> yet. I have attached the patch for scsi_lib.c where I
> do not call get_device if the state is not
> SDEV_RUNNING and similarly do not do put_device if you
> have not done get_device because of the device state.

Ah, I'm sorry, warning light goes off.

The patch is designed to operate on top of Mike Anderson's rework of our
device initialisation code.

http://marc.theaimsgroup.com/?l=linux-scsi&m=106810957502435&w=2

The get_device in the request function causes the badness without this
other patch.

Mike hasn't yet posted an update (hey, this is a hint, Mike) otherwise I
would already have folded it into the bugfixes tree.

James

Re: State Model for scsi device badness PATCH

[Mike Anderson]

James Bottomley [James.Bottomley@steeleye.com] wrote:
> On Thu, 2003-11-20 at 13:01, Amit Patel wrote:
> > I just walked through some of the code and I could
> > work around the badness problem by not doing
> > get_device if the scsi device is not in running state
> > yet. I have attached the patch for scsi_lib.c where I
> > do not call get_device if the state is not
> > SDEV_RUNNING and similarly do not do put_device if you
> > have not done get_device because of the device state.
> 
> Ah, I'm sorry, warning light goes off.
> 
> The patch is designed to operate on top of Mike Anderson's rework of our
> device initialisation code.
> 
> http://marc.theaimsgroup.com/?l=linux-scsi&m=106810957502435&w=2
> 
> The get_device in the request function causes the badness without this
> other patch.
> 
> Mike hasn't yet posted an update (hey, this is a hint, Mike) otherwise I
> would already have folded it into the bugfixes tree.
> 

Got the hint :-).

I will re-roll tonight.

-andmike
--
Michael Anderson
andmike@us.ibm.com