[PATCH] sym53c8xx PPR negotiation fix

[PATCH] sym53c8xx PPR negotiation fix

[Doug Ledford]

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

Marcelo,

I've verified that this problem still exists as of 2.4.23-pre8.  The
sym53c8xx driver doesn't bother to check that a device should support
PPR negotiation messages before attempting to use them on Ultra160
controllers.  PPR negotiations were added to the SCSI-3 standard (they
are required to negotiate Ultra160 or higher speeds) and should be
limited to SCSI-3 devices.  For back compatibility some modern devices
that support Ultra160 or above speeds still need to report themselves as
SCSI-2 devices.  Those devices are required to signal that they know
about PPR messages by setting the DT bit in the device's INQUIRY data. 
This patch adds that check.

Why add this check?  Two reasons.  One, correctness.  It's the right
thing to do.  Two, it fixes a bug with some DVD drives (the drives don't
report that they support PPR, but if the driver tries to use PPR then
the drive will accept it and respond in kind, but the drive fails to
actually properly set the sync speed after the PPR negotiation is
complete and the bus locks up as a result).

I haven't checked if this is applicable to 2.6 as well.  James?

-- 
  Doug Ledford <dledford@redhat.com>     919-754-3700 x44233
         Red Hat, Inc.
         1801 Varsity Dr.
         Raleigh, NC 27606


[-- Attachment #2: sym.patch --]
[-- Type: text/x-patch, Size: 682 bytes --]

diff -urNp linux-5420/drivers/scsi/sym53c8xx.c linux-5430/drivers/scsi/sym53c8xx.c
--- linux-5420/drivers/scsi/sym53c8xx.c
+++ linux-5430/drivers/scsi/sym53c8xx.c
@@ -11981,6 +11981,19 @@ static lcb_p ncr_setup_lcb (ncb_p np, u_
 		inq_byte7 |= INQ7_SYNC;
 
 	/*
+	**	Don't do PPR negotiations on SCSI-2 devices unless
+	**	they set the DT bit (0x04) in byte 57 of the INQUIRY
+	**	return data.
+	*/
+	if (((inq_data[2] & 0x07) < 3) && (inq_data[4] < 53 ||
+					   !(inq_data[56] & 0x04))) {
+		if (tp->minsync < 10)
+			tp->minsync = 10;
+		if (tp->usrsync < 10)
+			tp->usrsync = 10;
+	}
+
+	/*
 	**	Prepare negotiation if SIP capabilities have changed.
 	*/
 	tp->inq_done = 1;

Re: [PATCH] sym53c8xx PPR negotiation fix

[James Bottomley]

On Wed, 2003-10-29 at 11:07, Doug Ledford wrote:
> I haven't checked if this is applicable to 2.6 as well.  James?

Well, the sym driver doesn't exist anymore in 2.6...however sym_2 had a
similar issue.  Matthew Wilcox has already fixed it though, I think.

James

Re: [PATCH] sym53c8xx PPR negotiation fix

[Matthew Wilcox]

On Wed, Oct 29, 2003 at 11:11:28AM -0600, James Bottomley wrote:
> On Wed, 2003-10-29 at 11:07, Doug Ledford wrote:
> > I haven't checked if this is applicable to 2.6 as well.  James?
> 
> Well, the sym driver doesn't exist anymore in 2.6...however sym_2 had a
> similar issue.  Matthew Wilcox has already fixed it though, I think.

Oh, wishful thinking.  I'm trying to fix it and haven't succeeded yet.
These days, there's a PPR bit in the scsi_device, but it never seems to
be set, so we don't even try negotiating it.  Maybe the drives I have
to play with aren't really U160.

my current patch looks something like this (terribly mangled):

@@ -518,16 +518,21 @@ static int sym_queue_command(struct sym_
        order = (lp && lp->s.reqtags) ? M_SIMPLE_TAG : 0;
 
+       /* Ensure we don't issue unwanted negotiations */
+       if (!ccb->device->ppr) {
+               tp->tinfo.goal.options &= ~PPR_OPT_MASK;
+       }
+
        /*
         *  Queue the SCSI IO.

but I have it mixed in with some other changes that're supposed to
help negotiate U160, so I can't say there might be some reason to not
do this.

-- 
"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: [PATCH] sym53c8xx PPR negotiation fix

[Doug Ledford]

On Wed, 2003-10-29 at 12:50, Matthew Wilcox wrote:
> On Wed, Oct 29, 2003 at 11:11:28AM -0600, James Bottomley wrote:
> > On Wed, 2003-10-29 at 11:07, Doug Ledford wrote:
> > > I haven't checked if this is applicable to 2.6 as well.  James?
> > 
> > Well, the sym driver doesn't exist anymore in 2.6...however sym_2 had a
> > similar issue.  Matthew Wilcox has already fixed it though, I think.
> 
> Oh, wishful thinking.  I'm trying to fix it and haven't succeeded yet.
> These days, there's a PPR bit in the scsi_device, but it never seems to
> be set, so we don't even try negotiating it.  Maybe the drives I have
> to play with aren't really U160.
> 
> my current patch looks something like this (terribly mangled):
> 
> @@ -518,16 +518,21 @@ static int sym_queue_command(struct sym_
>         order = (lp && lp->s.reqtags) ? M_SIMPLE_TAG : 0;
>  
> +       /* Ensure we don't issue unwanted negotiations */
> +       if (!ccb->device->ppr) {
> +               tp->tinfo.goal.options &= ~PPR_OPT_MASK;
> +       }
> +

Well, I do believe I recall adding the ppr bit to the scsi_device struct
long ago, and I thought I added the test to set it.  If not, then on
device scan once we have the inquiry return data and have determined
scsi level of the device this would be sufficient to properly set it:

if (sdev->scsi_level >= 3 || (inquiry_len >= 53 && inquiry[56] & 0x04))
	sdev->ppr = 1;

(I mainly added this so that drivers could rip out their INQUIRY
snooping code and rely upon the mid layer's snooping of INQUIRY to set
the relevant options, but I'm not sure that it's been completed well
enough that the drivers can do that yet)


-- 
  Doug Ledford <dledford@redhat.com>     919-754-3700 x44233
         Red Hat, Inc.
         1801 Varsity Dr.
         Raleigh, NC 27606

Re: [PATCH] sym53c8xx PPR negotiation fix

[Doug Ledford]

On Wed, 2003-10-29 at 13:31, Matthew Wilcox wrote:

> Ahhh.  Then I'm misusing it.  We haven't sent an INQUIRY to the drive
> at this point, so of course it will be zero.  Some more pondering required.

Yeah, the appropriate time to check that bit would be at
slave_configure() time.  (Hmmm...I should have documented that...)

-- 
  Doug Ledford <dledford@redhat.com>     919-754-3700 x44233
         Red Hat, Inc.
         1801 Varsity Dr.
         Raleigh, NC 27606

Re: [PATCH] sym53c8xx PPR negotiation fix

[Justin T. Gibbs]

> Yeah, the appropriate time to check that bit would be at
> slave_configure() time.  (Hmmm...I should have documented that...)

BTW, slave_destroy() doesn't seem to be called after a probe
for a target fails due to a selection timeout.  Is this the
expected behavior?  I only keep persistent allocations after
slave_configure() is called, so this doesn't affect my drivers,
but the behavior isn't what I expected.  This is 2.6.0-test9.

--
Justin

Re: [PATCH] sym53c8xx PPR negotiation fix

[James Bottomley]

On Fri, 2003-10-31 at 17:55, Justin T. Gibbs wrote:
> BTW, slave_destroy() doesn't seem to be called after a probe
> for a target fails due to a selection timeout.  Is this the
> expected behavior?  I only keep persistent allocations after
> slave_configure() is called, so this doesn't affect my drivers,
> but the behavior isn't what I expected.  This is 2.6.0-test9.

Yes.

slave_configure is only called if the mid-layer decides there's
something worth attaching to there (i.e. if the initial inquiry
succeeds); otherwise it just calls slave_destroy to signal loss of
interest in the target.

The idea is that slave_alloc informs the driver that a probe will be
tried;  slave_configure tells the driver we like what we found, we've
initialised the inquiry variables in the target, now negotiate with it.

James

Re: [PATCH] sym53c8xx PPR negotiation fix

[Doug Ledford]

On Fri, 2003-10-31 at 18:55, James Bottomley wrote:
> On Fri, 2003-10-31 at 17:55, Justin T. Gibbs wrote:
> > BTW, slave_destroy() doesn't seem to be called after a probe
> > for a target fails due to a selection timeout.  Is this the
> > expected behavior?  I only keep persistent allocations after
> > slave_configure() is called, so this doesn't affect my drivers,
> > but the behavior isn't what I expected.  This is 2.6.0-test9.
> 
> Yes.
> 
> slave_configure is only called if the mid-layer decides there's
> something worth attaching to there (i.e. if the initial inquiry
> succeeds); otherwise it just calls slave_destroy to signal loss of
> interest in the target.

That's exactly what Justin was saying *isn't* happening (aka, no target
is there, we *don't* call slave_destroy is what Justin said).


-- 
  Doug Ledford <dledford@redhat.com>     919-754-3700 x44233
         Red Hat, Inc.
         1801 Varsity Dr.
         Raleigh, NC 27606

Re: [PATCH] sym53c8xx PPR negotiation fix

[Justin T. Gibbs]

>> slave_configure is only called if the mid-layer decides there's
>> something worth attaching to there (i.e. if the initial inquiry
>> succeeds); otherwise it just calls slave_destroy to signal loss of
>> interest in the target.
> 
> That's exactly what Justin was saying *isn't* happening (aka, no target
> is there, we *don't* call slave_destroy is what Justin said).

It seems the slave_destroy is missing from scsi_free_sdev().

--
Justin

Re: [PATCH] sym53c8xx PPR negotiation fix

[Mike Anderson]

Justin T. Gibbs [gibbs@scsiguy.com] wrote:
> >> slave_configure is only called if the mid-layer decides there's
> >> something worth attaching to there (i.e. if the initial inquiry
> >> succeeds); otherwise it just calls slave_destroy to signal loss of
> >> interest in the target.
> > 
> > That's exactly what Justin was saying *isn't* happening (aka, no target
> > is there, we *don't* call slave_destroy is what Justin said).
> 
> It seems the slave_destroy is missing from scsi_free_sdev().

It is called from scsi_remove_device.

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

Re: [PATCH] sym53c8xx PPR negotiation fix

[James Bottomley]

On Fri, 2003-10-31 at 19:22, Mike Anderson wrote:
> It is called from scsi_remove_device.

But that's only called for configured devices.  The original intent was
to call slave_destroy for all slave_alloc'd devices (whether configured
or not).

It originally was in scsi_free_sdev, but was moved with

ChangeSet 1.1046.597.3 2003/08/02 12:17:19 andmike@us.ibm.com
  [PATCH] scsi host / scsi device ref counting take 2 [3/3]

The changelog isn't very explicit about why this was done, what was the
particular reason?

James

Re: [PATCH] sym53c8xx PPR negotiation fix

[Doug Ledford]

On Fri, 2003-10-31 at 21:34, James Bottomley wrote:
> On Fri, 2003-10-31 at 19:22, Mike Anderson wrote:
> > It is called from scsi_remove_device.
> 
> But that's only called for configured devices.  The original intent was
> to call slave_destroy for all slave_alloc'd devices (whether configured
> or not).
> 
> It originally was in scsi_free_sdev, but was moved with
> 
> ChangeSet 1.1046.597.3 2003/08/02 12:17:19 andmike@us.ibm.com
>   [PATCH] scsi host / scsi device ref counting take 2 [3/3]
> 
> The changelog isn't very explicit about why this was done, what was the
> particular reason?

It really should be called on all devices, not just configured devices. 
The assumption that a device driver doesn't need to allocate local
storage to even do something as simple as INQUIRY and can wait until
slave_configure() to allocate anything is an invalid assumption.  In
fact, when I originally was working on this I think I modified the
aic7xxx_old driver to get rid of as much static array type data as
possible and move it to a struct allocated in slave_alloc.  Whether the
device is kept or not, it still has to alloc to work.  At one point
there was also another optimization in there so that if a driver didn't
do anything besides simply kfree() the memory pointed to by
sdptr->hostdata then you could skip defining a slave_destroy() function
and instead just let scsi_free_sdev do a simple kmalloc for you.  I
think that was argued against as a special case just confuses people,
can't remember.

-- 
  Doug Ledford <dledford@redhat.com>     919-754-3700 x44233
         Red Hat, Inc.
         1801 Varsity Dr.
         Raleigh, NC 27606

Re: [PATCH] sym53c8xx PPR negotiation fix

[Mike Anderson]

James Bottomley [James.Bottomley@steeleye.com] wrote:
> On Fri, 2003-10-31 at 19:22, Mike Anderson wrote:
> > It is called from scsi_remove_device.
> 
> But that's only called for configured devices.  The original intent was
> to call slave_destroy for all slave_alloc'd devices (whether configured
> or not).
> 
> It originally was in scsi_free_sdev, but was moved with
> 
> ChangeSet 1.1046.597.3 2003/08/02 12:17:19 andmike@us.ibm.com
>   [PATCH] scsi host / scsi device ref counting take 2 [3/3]
> 
> The changelog isn't very explicit about why this was done, what was the
> particular reason?
> 

While it was changed to call slave_destroy prior to returning from
scsi_remove_host.

In the thread below there is a little more info, but the slides from OLS
are outdated.

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

But..

As this thread has indicated this leaks memory for devices not
configured. Since the slave_destroy function is only releasing resources
and we have a reference to the host structure calling this from the scsi
device release function should be ok in unexpected disconnect cases.

I have attached a patch which calls slave_destroy from scsi device
release which now covers the case for all previous callers of
slave_alloc (configured or not). The patch also aligns scsi device
struct device init with how scsi host structures are done. This was
previously discussed in the thread pointed to below.

http://marc.theaimsgroup.com/?t=106737767300003&r=1&w=2 

I have ran the patch with scsi_debug and aic7xxx. I am still doing more
testing once a get a few more devices on the test system.

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


DESC
This patch splits the scsi device struct device register into init and
add. It also addresses memory leak issues of not calling slave_destroy on
scsi_devices that are not configured in.

Details:
- Make scsi_device_dev_release extern for scsi_scan to use in
alloc_sdev.
- Move scsi_free_sdev code to scsi_device_dev_release.
- Changed name of scsi_device_register to scsi_sysfs_add_sdev to
match host call and align with split struct device init.
- Move sdev_gendev device and class init to scsi_alloc_sdev.
- Moved call to slave_destroy to scsi_device_dev_release.
- Removed scsi_free_sdev. Two callers now do put_device.
EDESC


 drivers/scsi/scsi_priv.h  |    4 +--
 drivers/scsi/scsi_scan.c  |   55 ++++++++++++++++++----------------------------
 drivers/scsi/scsi_sysfs.c |   44 +++++++++++++++++++-----------------
 3 files changed, 47 insertions(+), 56 deletions(-)

diff -puN drivers/scsi/scsi_sysfs.c~sdev_ldm_reg drivers/scsi/scsi_sysfs.c
--- patched-scsi-bugfixes-2.6/drivers/scsi/scsi_sysfs.c~sdev_ldm_reg	Fri Oct 31 09:07:07 2003
+++ patched-scsi-bugfixes-2.6-andmike/drivers/scsi/scsi_sysfs.c	Sun Nov  2 22:20:15 2003
@@ -115,14 +115,32 @@ static void scsi_device_cls_release(stru
 	put_device(&sdev->sdev_gendev);
 }
 
-static void scsi_device_dev_release(struct device *dev)
+void scsi_device_dev_release(struct device *dev)
 {
 	struct scsi_device *sdev;
 	struct device *parent;
+	unsigned long flags;
 
 	parent = dev->parent;
 	sdev = to_scsi_device(dev);
-	scsi_free_sdev(sdev);
+
+	spin_lock_irqsave(sdev->host->host_lock, flags);
+	list_del(&sdev->siblings);
+	list_del(&sdev->same_target_siblings);
+	list_del(&sdev->starved_entry);
+	if (sdev->single_lun && --sdev->sdev_target->starget_refcnt == 0)
+		kfree(sdev->sdev_target);
+	spin_unlock_irqrestore(sdev->host->host_lock, flags);
+
+	if (sdev->request_queue)
+		scsi_free_queue(sdev->request_queue);
+
+	if (sdev->host->hostt->slave_destroy)
+		sdev->host->hostt->slave_destroy(sdev);
+
+	kfree(sdev->inquiry);
+	kfree(sdev);
+
 	put_device(parent);
 }
 
@@ -321,29 +339,17 @@ static int attr_add(struct device *dev, 
 }
 
 /**
- * scsi_device_register - register a scsi device with the scsi bus
- * @sdev:	scsi_device to register
+ * scsi_sysfs_add_sdev - add scsi device to sysfs
+ * @sdev:	scsi_device to add
  *
  * Return value:
  * 	0 on Success / non-zero on Failure
  **/
-int scsi_device_register(struct scsi_device *sdev)
+int scsi_sysfs_add_sdev(struct scsi_device *sdev)
 {
 	int error = 0, i;
 
 	set_bit(SDEV_ADD, &sdev->sdev_state);
-	device_initialize(&sdev->sdev_gendev);
-	sprintf(sdev->sdev_gendev.bus_id,"%d:%d:%d:%d",
-		sdev->host->host_no, sdev->channel, sdev->id, sdev->lun);
-	sdev->sdev_gendev.parent = &sdev->host->shost_gendev;
-	sdev->sdev_gendev.bus = &scsi_bus_type;
-	sdev->sdev_gendev.release = scsi_device_dev_release;
-
-	class_device_initialize(&sdev->sdev_classdev);
-	sdev->sdev_classdev.dev = &sdev->sdev_gendev;
-	sdev->sdev_classdev.class = &sdev_class;
-	snprintf(sdev->sdev_classdev.class_id, BUS_ID_SIZE, "%d:%d:%d:%d",
-		sdev->host->host_no, sdev->channel, sdev->id, sdev->lun);
 
 	error = device_add(&sdev->sdev_gendev);
 	if (error) {
@@ -351,8 +357,6 @@ int scsi_device_register(struct scsi_dev
 		return error;
 	}
 
-	get_device(sdev->sdev_gendev.parent);
-
 	error = class_device_add(&sdev->sdev_classdev);
 	if (error) {
 		printk(KERN_INFO "error 2\n");
@@ -398,8 +402,6 @@ void scsi_remove_device(struct scsi_devi
 {
 	set_bit(SDEV_DEL, &sdev->sdev_state);
 	class_device_unregister(&sdev->sdev_classdev);
-	if (sdev->host->hostt->slave_destroy)
-		sdev->host->hostt->slave_destroy(sdev);
 	device_del(&sdev->sdev_gendev);
 	put_device(&sdev->sdev_gendev);
 }
diff -puN drivers/scsi/scsi_scan.c~sdev_ldm_reg drivers/scsi/scsi_scan.c
--- patched-scsi-bugfixes-2.6/drivers/scsi/scsi_scan.c~sdev_ldm_reg	Sat Nov  1 12:25:12 2003
+++ patched-scsi-bugfixes-2.6-andmike/drivers/scsi/scsi_scan.c	Sun Nov  2 12:50:52 2003
@@ -236,6 +236,25 @@ static struct scsi_device *scsi_alloc_sd
 			goto out_free_queue;
 	}
 
+	if (get_device(&sdev->host->shost_gendev)) {
+
+		device_initialize(&sdev->sdev_gendev);
+		sdev->sdev_gendev.parent = &sdev->host->shost_gendev;
+		sdev->sdev_gendev.bus = &scsi_bus_type;
+		sdev->sdev_gendev.release = scsi_device_dev_release;
+		sprintf(sdev->sdev_gendev.bus_id,"%d:%d:%d:%d",
+			sdev->host->host_no, sdev->channel, sdev->id,
+			sdev->lun);
+
+		class_device_initialize(&sdev->sdev_classdev);
+		sdev->sdev_classdev.dev = &sdev->sdev_gendev;
+		sdev->sdev_classdev.class = &sdev_class;
+		snprintf(sdev->sdev_classdev.class_id, BUS_ID_SIZE,
+			 "%d:%d:%d:%d", sdev->host->host_no,
+			 sdev->channel, sdev->id, sdev->lun);
+	} else
+		goto out_free_queue;
+
 	/*
 	 * If there are any same target siblings, add this to the
 	 * sibling list
@@ -273,36 +292,6 @@ out:
 }
 
 /**
- * scsi_free_sdev - cleanup and free a scsi_device
- * @sdev:	cleanup and free this scsi_device
- *
- * Description:
- *     Undo the actions in scsi_alloc_sdev, including removing @sdev from
- *     the list, and freeing @sdev.
- **/
-void scsi_free_sdev(struct scsi_device *sdev)
-{
-	unsigned long flags;
-
-	spin_lock_irqsave(sdev->host->host_lock, flags);
-	list_del(&sdev->siblings);
-	list_del(&sdev->same_target_siblings);
-	spin_unlock_irqrestore(sdev->host->host_lock, flags);
-
-	if (sdev->request_queue)
-		scsi_free_queue(sdev->request_queue);
-
-	spin_lock_irqsave(sdev->host->host_lock, flags);
-	list_del(&sdev->starved_entry);
-	if (sdev->single_lun && --sdev->sdev_target->starget_refcnt == 0)
-		kfree(sdev->sdev_target);
-	spin_unlock_irqrestore(sdev->host->host_lock, flags);
-
-	kfree(sdev->inquiry);
-	kfree(sdev);
-}
-
-/**
  * scsi_probe_lun - probe a single LUN using a SCSI INQUIRY
  * @sreq:	used to send the INQUIRY
  * @inq_result:	area to store the INQUIRY result
@@ -642,7 +631,7 @@ static int scsi_add_lun(struct scsi_devi
 	 * register it and tell the rest of the kernel
 	 * about it.
 	 */
-	scsi_device_register(sdev);
+	scsi_sysfs_add_sdev(sdev);
 
 	return SCSI_SCAN_LUN_PRESENT;
 }
@@ -749,7 +738,7 @@ static int scsi_probe_and_add_lun(struct
 		if (sdevp)
 			*sdevp = sdev;
 	} else
-		scsi_free_sdev(sdev);
+		put_device(&sdev->sdev_gendev);
  out:
 	return res;
 }
@@ -1301,5 +1290,5 @@ struct scsi_device *scsi_get_host_dev(st
 void scsi_free_host_dev(struct scsi_device *sdev)
 {
 	BUG_ON(sdev->id != sdev->host->this_id);
-	scsi_free_sdev(sdev);
+	put_device(&sdev->sdev_gendev);
 }
diff -puN drivers/scsi/scsi_priv.h~sdev_ldm_reg drivers/scsi/scsi_priv.h
--- patched-scsi-bugfixes-2.6/drivers/scsi/scsi_priv.h~sdev_ldm_reg	Sat Nov  1 12:42:59 2003
+++ patched-scsi-bugfixes-2.6-andmike/drivers/scsi/scsi_priv.h	Sun Nov  2 18:26:09 2003
@@ -130,7 +130,6 @@ extern void scsi_exit_procfs(void);
 extern int scsi_scan_host_selected(struct Scsi_Host *, unsigned int,
 				   unsigned int, unsigned int, int);
 extern void scsi_forget_host(struct Scsi_Host *);
-extern void scsi_free_sdev(struct scsi_device *);
 extern void scsi_rescan_device(struct device *);
 
 /* scsi_sysctl.c */
@@ -143,7 +142,8 @@ extern void scsi_exit_sysctl(void);
 #endif /* CONFIG_SYSCTL */
 
 /* scsi_sysfs.c */
-extern int scsi_device_register(struct scsi_device *);
+extern void scsi_device_dev_release(struct device *);
+extern int scsi_sysfs_add_sdev(struct scsi_device *);
 extern int scsi_sysfs_add_host(struct Scsi_Host *);
 extern int scsi_sysfs_register(void);
 extern void scsi_sysfs_unregister(void);

_

Re: [PATCH] sym53c8xx PPR negotiation fix

[Christoph Hellwig]

Umm, the code implementing ->slave_destroy may no more exist when the
scsi_device reference count hits zero..

-- 
Christoph Hellwig <hch@lst.de>		-	Freelance Hacker
Contact me for driver hacking and kernel development consulting

Re: [PATCH] sym53c8xx PPR negotiation fix

[Mike Anderson]

Christoph Hellwig [hch@infradead.org] wrote:
> Umm, the code implementing ->slave_destroy may no more exist when the
> scsi_device reference count hits zero..
> 

Yes that is my bad. I re-rolled the patch and will run a couple of
tests. I will repost today. 

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

Re: [PATCH] sym53c8xx PPR negotiation fix

[Mike Anderson]

Christoph Hellwig [hch@infradead.org] wrote:
> Umm, the code implementing ->slave_destroy may no more exist when the
> scsi_device reference count hits zero..

Ok here is an update. This should address both issues of not calling
slave_destroy on scan'd but not configured devices and also calling
slave_destroy on host removal prior to the return of scsi_remove_host.

I added a piece of two test runs. A more complete log is available.

Ex 1. Configured device mounted with unexpected disconnect followed by
unmount.

chk_scsi_ref: Remove adapter

kobject ./<NULL>: cleaning up
kobject scsi_device/2:0:0:0: cleaning up
kobject ./<NULL>: cleaning up
kobject queue/iosched: cleaning up
kobject sdd/queue: cleaning up
scsi_debug: slave_destroy <2 0 0 0>
kobject scsi_host/host2: cleaning up

chk_scsi_ref: umount sdd

Buffer I/O error on device sdd, logical block 1
lost page write due to I/O error on sdd
kobject host2/2:0:0:0: cleaning up
kobject adapter1/host2: cleaning up
kobject pseudo_0/adapter1: cleaning up
kobject ./<NULL>: cleaning up
kobject block/sdd: cleaning up

Ex 2. Scanning a device that was not configured.

scsi_debug: slave_alloc <9 0 1 0>
scsi_debug: cmd 12 00 00 00 24 00
scsi_debug: ... <9 0 1 0> non-zero result=0x10000
scsi_debug: slave_destroy <9 0 1 0>
kobject ./<NULL>: cleaning up

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


DESC
This patch splits the scsi device struct device register into init and
add. It also addresses memory leak issues of not calling slave_destroy on
scsi_devices that are not configured in.

Details:
* Make scsi_device_dev_release extern for scsi_scan to use in
alloc_sdev.
* Move scsi_free_sdev code to scsi_device_dev_release. Have
scsi_free_sdev call slave_destroy plus put_device.
* Changed name of scsi_device_register to scsi_sysfs_add_sdev to
match host call and align with split struct device init.
* Move sdev_gendev device and class init to scsi_alloc_sdev.

Thu Nov  6 07:43:56 UTC 2003
EDESC


 drivers/scsi/scsi_priv.h  |    3 +-
 drivers/scsi/scsi_scan.c  |   42 ++++++++++++++++++----------------
 drivers/scsi/scsi_sysfs.c |   56 +++++++++++++++++++++++-----------------------
 3 files changed, 54 insertions(+), 47 deletions(-)

diff -puN drivers/scsi/scsi_priv.h~sdev_ldm_reg drivers/scsi/scsi_priv.h
--- scsi-bugfixes-2.6/drivers/scsi/scsi_priv.h~sdev_ldm_reg	Tue Nov  4 23:17:24 2003
+++ scsi-bugfixes-2.6-andmike/drivers/scsi/scsi_priv.h	Tue Nov  4 23:17:24 2003
@@ -143,7 +143,8 @@ extern void scsi_exit_sysctl(void);
 #endif /* CONFIG_SYSCTL */
 
 /* scsi_sysfs.c */
-extern int scsi_device_register(struct scsi_device *);
+extern void scsi_device_dev_release(struct device *);
+extern int scsi_sysfs_add_sdev(struct scsi_device *);
 extern int scsi_sysfs_add_host(struct Scsi_Host *);
 extern int scsi_sysfs_register(void);
 extern void scsi_sysfs_unregister(void);
diff -puN drivers/scsi/scsi_scan.c~sdev_ldm_reg drivers/scsi/scsi_scan.c
--- scsi-bugfixes-2.6/drivers/scsi/scsi_scan.c~sdev_ldm_reg	Tue Nov  4 23:17:24 2003
+++ scsi-bugfixes-2.6-andmike/drivers/scsi/scsi_scan.c	Tue Nov  4 23:17:24 2003
@@ -236,6 +236,25 @@ static struct scsi_device *scsi_alloc_sd
 			goto out_free_queue;
 	}
 
+	if (get_device(&sdev->host->shost_gendev)) {
+
+		device_initialize(&sdev->sdev_gendev);
+		sdev->sdev_gendev.parent = &sdev->host->shost_gendev;
+		sdev->sdev_gendev.bus = &scsi_bus_type;
+		sdev->sdev_gendev.release = scsi_device_dev_release;
+		sprintf(sdev->sdev_gendev.bus_id,"%d:%d:%d:%d",
+			sdev->host->host_no, sdev->channel, sdev->id,
+			sdev->lun);
+
+		class_device_initialize(&sdev->sdev_classdev);
+		sdev->sdev_classdev.dev = &sdev->sdev_gendev;
+		sdev->sdev_classdev.class = &sdev_class;
+		snprintf(sdev->sdev_classdev.class_id, BUS_ID_SIZE,
+			 "%d:%d:%d:%d", sdev->host->host_no,
+			 sdev->channel, sdev->id, sdev->lun);
+	} else
+		goto out_free_queue;
+
 	/*
 	 * If there are any same target siblings, add this to the
 	 * sibling list
@@ -282,24 +301,9 @@ out:
  **/
 void scsi_free_sdev(struct scsi_device *sdev)
 {
-	unsigned long flags;
-
-	spin_lock_irqsave(sdev->host->host_lock, flags);
-	list_del(&sdev->siblings);
-	list_del(&sdev->same_target_siblings);
-	spin_unlock_irqrestore(sdev->host->host_lock, flags);
-
-	if (sdev->request_queue)
-		scsi_free_queue(sdev->request_queue);
-
-	spin_lock_irqsave(sdev->host->host_lock, flags);
-	list_del(&sdev->starved_entry);
-	if (sdev->single_lun && --sdev->sdev_target->starget_refcnt == 0)
-		kfree(sdev->sdev_target);
-	spin_unlock_irqrestore(sdev->host->host_lock, flags);
-
-	kfree(sdev->inquiry);
-	kfree(sdev);
+	if (sdev->host->hostt->slave_destroy)
+		sdev->host->hostt->slave_destroy(sdev);
+	put_device(&sdev->sdev_gendev);
 }
 
 /**
@@ -642,7 +646,7 @@ static int scsi_add_lun(struct scsi_devi
 	 * register it and tell the rest of the kernel
 	 * about it.
 	 */
-	scsi_device_register(sdev);
+	scsi_sysfs_add_sdev(sdev);
 
 	return SCSI_SCAN_LUN_PRESENT;
 }
diff -puN drivers/scsi/scsi_sysfs.c~sdev_ldm_reg drivers/scsi/scsi_sysfs.c
--- scsi-bugfixes-2.6/drivers/scsi/scsi_sysfs.c~sdev_ldm_reg	Tue Nov  4 23:17:24 2003
+++ scsi-bugfixes-2.6-andmike/drivers/scsi/scsi_sysfs.c	Tue Nov  4 23:17:24 2003
@@ -115,14 +115,29 @@ static void scsi_device_cls_release(stru
 	put_device(&sdev->sdev_gendev);
 }
 
-static void scsi_device_dev_release(struct device *dev)
+void scsi_device_dev_release(struct device *dev)
 {
 	struct scsi_device *sdev;
 	struct device *parent;
+	unsigned long flags;
 
 	parent = dev->parent;
 	sdev = to_scsi_device(dev);
-	scsi_free_sdev(sdev);
+
+	spin_lock_irqsave(sdev->host->host_lock, flags);
+	list_del(&sdev->siblings);
+	list_del(&sdev->same_target_siblings);
+	list_del(&sdev->starved_entry);
+	if (sdev->single_lun && --sdev->sdev_target->starget_refcnt == 0)
+		kfree(sdev->sdev_target);
+	spin_unlock_irqrestore(sdev->host->host_lock, flags);
+
+	if (sdev->request_queue)
+		scsi_free_queue(sdev->request_queue);
+
+	kfree(sdev->inquiry);
+	kfree(sdev);
+
 	put_device(parent);
 }
 
@@ -321,29 +336,18 @@ static int attr_add(struct device *dev, 
 }
 
 /**
- * scsi_device_register - register a scsi device with the scsi bus
- * @sdev:	scsi_device to register
+ * scsi_sysfs_add_sdev - add scsi device to sysfs
+ * @sdev:	scsi_device to add
  *
  * Return value:
  * 	0 on Success / non-zero on Failure
  **/
-int scsi_device_register(struct scsi_device *sdev)
+int scsi_sysfs_add_sdev(struct scsi_device *sdev)
 {
-	int error = 0, i;
+	int error = -EINVAL, i;
 
-	set_bit(SDEV_ADD, &sdev->sdev_state);
-	device_initialize(&sdev->sdev_gendev);
-	sprintf(sdev->sdev_gendev.bus_id,"%d:%d:%d:%d",
-		sdev->host->host_no, sdev->channel, sdev->id, sdev->lun);
-	sdev->sdev_gendev.parent = &sdev->host->shost_gendev;
-	sdev->sdev_gendev.bus = &scsi_bus_type;
-	sdev->sdev_gendev.release = scsi_device_dev_release;
-
-	class_device_initialize(&sdev->sdev_classdev);
-	sdev->sdev_classdev.dev = &sdev->sdev_gendev;
-	sdev->sdev_classdev.class = &sdev_class;
-	snprintf(sdev->sdev_classdev.class_id, BUS_ID_SIZE, "%d:%d:%d:%d",
-		sdev->host->host_no, sdev->channel, sdev->id, sdev->lun);
+	if (test_and_set_bit(SDEV_ADD, &sdev->sdev_state))
+		return error;
 
 	error = device_add(&sdev->sdev_gendev);
 	if (error) {
@@ -351,8 +355,6 @@ int scsi_device_register(struct scsi_dev
 		return error;
 	}
 
-	get_device(sdev->sdev_gendev.parent);
-
 	error = class_device_add(&sdev->sdev_classdev);
 	if (error) {
 		printk(KERN_INFO "error 2\n");
@@ -396,12 +398,12 @@ clean_device:
  **/
 void scsi_remove_device(struct scsi_device *sdev)
 {
-	set_bit(SDEV_DEL, &sdev->sdev_state);
-	class_device_unregister(&sdev->sdev_classdev);
-	if (sdev->host->hostt->slave_destroy)
-		sdev->host->hostt->slave_destroy(sdev);
-	device_del(&sdev->sdev_gendev);
-	put_device(&sdev->sdev_gendev);
+	if (test_and_clear_bit(SDEV_ADD, &sdev->sdev_state)) {
+		set_bit(SDEV_DEL, &sdev->sdev_state);
+		class_device_unregister(&sdev->sdev_classdev);
+		device_del(&sdev->sdev_gendev);
+		scsi_free_sdev(sdev);
+	}
 }
 
 int scsi_register_driver(struct device_driver *drv)

_

Re: [PATCH] sym53c8xx PPR negotiation fix

[Christoph Hellwig]

> @@ -282,24 +301,9 @@ out:
>   **/
>  void scsi_free_sdev(struct scsi_device *sdev)
>  {
> -	unsigned long flags;
> -
> -	spin_lock_irqsave(sdev->host->host_lock, flags);
> -	list_del(&sdev->siblings);
> -	list_del(&sdev->same_target_siblings);
> -	spin_unlock_irqrestore(sdev->host->host_lock, flags);
> -
> -	if (sdev->request_queue)
> -		scsi_free_queue(sdev->request_queue);
> -
> -	spin_lock_irqsave(sdev->host->host_lock, flags);
> -	list_del(&sdev->starved_entry);
> -	if (sdev->single_lun && --sdev->sdev_target->starget_refcnt == 0)
> -		kfree(sdev->sdev_target);
> -	spin_unlock_irqrestore(sdev->host->host_lock, flags);
> -
> -	kfree(sdev->inquiry);
> -	kfree(sdev);
> +	if (sdev->host->hostt->slave_destroy)
> +		sdev->host->hostt->slave_destroy(sdev);
> +	put_device(&sdev->sdev_gendev);

The name of this function is rather misleading now, I'd rather kill
it and inline it into the caller - it's just three lines anyway.

Otherwise the patch looks ok to me, but I need to look at a tree
with it applied before coming to final conclusions..


-- 
Christoph Hellwig <hch@lst.de>		-	Freelance Hacker
Contact me for driver hacking and kernel development consulting

Re: [PATCH] sym53c8xx PPR negotiation fix

[Mike Anderson]

Christoph Hellwig [hch@infradead.org] wrote:
> > @@ -282,24 +301,9 @@ out:
> >   **/
> >  void scsi_free_sdev(struct scsi_device *sdev)
> >  {
> > -	unsigned long flags;
> > -
> > -	spin_lock_irqsave(sdev->host->host_lock, flags);
> > -	list_del(&sdev->siblings);
> > -	list_del(&sdev->same_target_siblings);
> > -	spin_unlock_irqrestore(sdev->host->host_lock, flags);
> > -
> > -	if (sdev->request_queue)
> > -		scsi_free_queue(sdev->request_queue);
> > -
> > -	spin_lock_irqsave(sdev->host->host_lock, flags);
> > -	list_del(&sdev->starved_entry);
> > -	if (sdev->single_lun && --sdev->sdev_target->starget_refcnt == 0)
> > -		kfree(sdev->sdev_target);
> > -	spin_unlock_irqrestore(sdev->host->host_lock, flags);
> > -
> > -	kfree(sdev->inquiry);
> > -	kfree(sdev);
> > +	if (sdev->host->hostt->slave_destroy)
> > +		sdev->host->hostt->slave_destroy(sdev);
> > +	put_device(&sdev->sdev_gendev);
> 
> The name of this function is rather misleading now, I'd rather kill
> it and inline it into the caller - it's just three lines anyway.
> 
> Otherwise the patch looks ok to me, but I need to look at a tree
> with it applied before coming to final conclusions..

I agree on the misleading name. I debated about inlining it when I was
doing the last updates. I will update my patch and wait for anymore
feedback.

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

Re: [PATCH] sym53c8xx PPR negotiation fix

[Doug Ledford]

On Fri, 2003-10-31 at 18:55, Justin T. Gibbs wrote:
> > Yeah, the appropriate time to check that bit would be at
> > slave_configure() time.  (Hmmm...I should have documented that...)
> 
> BTW, slave_destroy() doesn't seem to be called after a probe
> for a target fails due to a selection timeout.  Is this the
> expected behavior?  I only keep persistent allocations after
> slave_configure() is called, so this doesn't affect my drivers,
> but the behavior isn't what I expected.  This is 2.6.0-test9.

I haven't been able to follow 2.5/2.6 SCSI stuff as close as I wanted,
but originally I thought it did call slave_destroy() even if it never
called slave_configure()....hmmm...maybe I should go check my stuff back
then...I could be wrong on that.

-- 
  Doug Ledford <dledford@redhat.com>     919-754-3700 x44233
         Red Hat, Inc.
         1801 Varsity Dr.
         Raleigh, NC 27606