[PATCH] fusion: streamline ->slave_alloc/->slave_destroy

[PATCH] fusion: streamline ->slave_alloc/->slave_destroy

[Christoph Hellwig]

In addition the code to free target structures in mptscsih_remove is
removed as the scsi layer guarantees to call ->slave_destory on
every life scsi_device before returning from scsi_remove_host.


--- 1.50/drivers/message/fusion/mptscsih.c	2004-09-29 01:58:39 +02:00
+++ edited/drivers/message/fusion/mptscsih.c	2004-10-01 12:45:04 +02:00
@@ -1374,7 +1374,7 @@
 
 	hd = (MPT_SCSI_HOST *)host->hostdata;
 	if (hd != NULL) {
-		int sz1, sz3, sztarget=0;
+		int sz1, sz3;
 
 		mptscsih_shutdown(&pdev->dev);
 
@@ -1387,28 +1387,9 @@
 		}
 
 		if (hd->Targets != NULL) {
-			int max, ii;
-
-			/*
-			 * Free any target structures that were allocated.
-			 */
-			if (hd->is_spi) {
-				max = MPT_MAX_SCSI_DEVICES;
-			} else {
-				max = MPT_MAX_FC_DEVICES<256 ? MPT_MAX_FC_DEVICES : 255;
-			}
-			for (ii=0; ii < max; ii++) {
-				if (hd->Targets[ii]) {
-					kfree(hd->Targets[ii]);
-					hd->Targets[ii] = NULL;
-					sztarget += sizeof(VirtDevice);
-				}
-			}
-
 			/*
 			 * Free pointer array.
 			 */
-			sz3 = max * sizeof(void *);
 			kfree(hd->Targets);
 			hd->Targets = NULL;
 		}
@@ -2767,39 +2738,52 @@
 mptscsih_slave_alloc(struct scsi_device *device)
 {
 	struct Scsi_Host	*host = device->host;
-	MPT_SCSI_HOST		*hd;
-	VirtDevice		*vdev;
+	MPT_SCSI_HOST		*hd = (MPT_SCSI_HOST *)host->hostdata;
+	VirtDevice		*vdev = hd->Targets[device->id];
 
-	hd = (MPT_SCSI_HOST *)host->hostdata;
+	if (vdev)
+		goto out;
 
-	if (hd == NULL)
-		return -ENODEV;
-
-	if ((vdev = hd->Targets[device->id]) == NULL) {
-		if ((vdev = kmalloc(sizeof(VirtDevice), GFP_ATOMIC)) == NULL) {
-			printk(MYIOC_s_ERR_FMT "slave_alloc kmalloc(%d) FAILED!\n",
-			hd->ioc->name, (int)sizeof(VirtDevice));
-			return -ENOMEM;
-		} else {
-			memset(vdev, 0, sizeof(VirtDevice));
-			vdev->tflags = MPT_TARGET_FLAGS_Q_YES;
-			vdev->ioc_id = hd->ioc->id;
-			vdev->target_id = device->id;
-			vdev->bus_id = device->channel;
-			vdev->raidVolume = 0;
-			hd->Targets[device->id] = vdev;
-			if (hd->is_spi) {
-				if (hd->ioc->spi_data.isRaid & (1 << device->id)) {
-					vdev->raidVolume = 1;
-					ddvtprintk((KERN_INFO
-					    "RAID Volume @ id %d\n", device->id));
-				}
-			} else {
-				vdev->tflags |= MPT_TARGET_FLAGS_VALID_INQUIRY;
-			}
+	vdev = kmalloc(sizeof(VirtDevice), GFP_KERNEL);
+	if (!vdev) {
+		printk(MYIOC_s_ERR_FMT "slave_alloc kmalloc(%zd) FAILED!\n",
+				hd->ioc->name, sizeof(VirtDevice));
+		return -ENOMEM;
+	}
+
+	memset(vdev, 0, sizeof(VirtDevice));
+	vdev->tflags = MPT_TARGET_FLAGS_Q_YES;
+	vdev->ioc_id = hd->ioc->id;
+	vdev->target_id = device->id;
+	vdev->bus_id = device->channel;
+	vdev->raidVolume = 0;
+	hd->Targets[device->id] = vdev;
+	if (hd->is_spi) {
+		if (hd->ioc->spi_data.isRaid & (1 << device->id)) {
+			vdev->raidVolume = 1;
+			ddvtprintk((KERN_INFO
+			    "RAID Volume @ id %d\n", device->id));
 		}
+	} else {
+		vdev->tflags |= MPT_TARGET_FLAGS_VALID_INQUIRY;
 	}
+
+ out:
 	vdev->num_luns++;
+	return 0;
+}
+
+static int mptscsih_is_raid_volume(MPT_SCSI_HOST *hd, uint id)
+{
+	int i;
+
+	if (!hd->ioc->spi_data.isRaid || !hd->ioc->spi_data.pIocPg3)
+		return 0;
+
+	for (i = 0; i < hd->ioc->spi_data.pIocPg3->NumPhysDisks; i++) {
+		if (id == hd->ioc->spi_data.pIocPg3->PhysDisk[i].PhysDiskID)
+			return 1;
+	}
 
 	return 0;
 }
@@ -2812,59 +2796,33 @@
 mptscsih_slave_destroy(struct scsi_device *device)
 {
 	struct Scsi_Host	*host = device->host;
-	MPT_SCSI_HOST		*hd;
-	VirtDevice		*vdev;
-	int 			raid_volume=0;
+	MPT_SCSI_HOST		*hd = (MPT_SCSI_HOST *)host->hostdata;
+	VirtDevice		*vdev =  hd->Targets[device->id];
+	uint			target = device->id;
+	uint			lun = device->lun;
 
-	hd = (MPT_SCSI_HOST *)host->hostdata;
+	mptscsih_search_running_cmds(hd, target, lun);
 
-	if (hd == NULL)
+	vdev->luns[0] &= ~(1 << lun);
+	if (--vdev->num_luns)
 		return;
 
-	mptscsih_search_running_cmds(hd, device->id, device->lun);
-
-	/* Free memory and reset all flags for this target
-	 */
-	if ((vdev = hd->Targets[device->id]) != NULL) {
-		vdev->num_luns--;
-
-		if (vdev->luns[0] & (1 << device->lun))
-			vdev->luns[0] &= ~(1 << device->lun);
+	kfree(hd->Targets[target]);
+	hd->Targets[target] = NULL;
 
-		/* Free device structure only if number of luns is 0.
-		 */
-		if (vdev->num_luns == 0) {
-			kfree(hd->Targets[device->id]);
-			hd->Targets[device->id] = NULL;
-
-			if (!hd->is_spi)
-				return;
-
-			if((hd->ioc->spi_data.isRaid) && (hd->ioc->spi_data.pIocPg3)) {
-			int i;
-				for(i=0;i<hd->ioc->spi_data.pIocPg3->NumPhysDisks &&
-					raid_volume==0;i++)
-
-					if(device->id ==
-					  hd->ioc->spi_data.pIocPg3->PhysDisk[i].PhysDiskID) {
-						raid_volume=1;
-						hd->ioc->spi_data.forceDv |=
-						  MPT_SCSICFG_RELOAD_IOC_PG3;
-					}
-			}
-
-			if(!raid_volume){
-				hd->ioc->spi_data.dvStatus[device->id] =
+	if (hd->is_spi) {
+		if (mptscsih_is_raid_volume(hd, target)) {
+			hd->ioc->spi_data.forceDv |= MPT_SCSICFG_RELOAD_IOC_PG3;
+		} else {
+			hd->ioc->spi_data.dvStatus[target] =
 				MPT_SCSICFG_NEGOTIATE;
 
-				if (hd->negoNvram == 0)
-					hd->ioc->spi_data.dvStatus[device->id]
-					|= MPT_SCSICFG_DV_NOT_DONE;
+			if (!hd->negoNvram) {
+				hd->ioc->spi_data.dvStatus[target] |=
+					MPT_SCSICFG_DV_NOT_DONE;
 			}
 		}
 	}
-
-	return;
 }
 
 static void

RE: [PATCH] fusion: streamline ->slave_alloc/->slave_destroy

[Moore, Eric Dean]

On Saturday, October 02, 2004 2:24 AM,  Christoph Hellwig wrote:
> @@ -2812,59 +2796,33 @@
>  mptscsih_slave_destroy(struct scsi_device *device)
>  {
>  	struct Scsi_Host	*host = device->host;
> -	MPT_SCSI_HOST		*hd;
> -	VirtDevice		*vdev;
> -	int 			raid_volume=0;
> +	MPT_SCSI_HOST		*hd = (MPT_SCSI_HOST *)host->hostdata;
> +	VirtDevice		*vdev =  hd->Targets[device->id];
> +	uint			target = device->id;
> +	uint			lun = device->lun;
>  
> -	hd = (MPT_SCSI_HOST *)host->hostdata;
> +	mptscsih_search_running_cmds(hd, target, lun);
>  
> -	if (hd == NULL)
> +	vdev->luns[0] &= ~(1 << lun);

Ok - I'm finally getting around to testing this patch.

The driver will oops on this line because this routine is 
sometimes called when vdev=NULL.  Your patch had removed the
check for this NULL pointer. We probally should add it back.

Eric

Re: [PATCH] fusion: streamline ->slave_alloc/->slave_destroy

[Christoph Hellwig]

On Wed, Oct 06, 2004 at 02:25:53PM -0400, Moore, Eric Dean wrote:
> On Saturday, October 02, 2004 2:24 AM,  Christoph Hellwig wrote:
> > @@ -2812,59 +2796,33 @@
> >  mptscsih_slave_destroy(struct scsi_device *device)
> >  {
> >  	struct Scsi_Host	*host = device->host;
> > -	MPT_SCSI_HOST		*hd;
> > -	VirtDevice		*vdev;
> > -	int 			raid_volume=0;
> > +	MPT_SCSI_HOST		*hd = (MPT_SCSI_HOST *)host->hostdata;
> > +	VirtDevice		*vdev =  hd->Targets[device->id];
> > +	uint			target = device->id;
> > +	uint			lun = device->lun;
> >  
> > -	hd = (MPT_SCSI_HOST *)host->hostdata;
> > +	mptscsih_search_running_cmds(hd, target, lun);
> >  
> > -	if (hd == NULL)
> > +	vdev->luns[0] &= ~(1 << lun);
> 
> Ok - I'm finally getting around to testing this patch.
> 
> The driver will oops on this line because this routine is 
> sometimes called when vdev=NULL.  Your patch had removed the
> check for this NULL pointer. We probally should add it back.

Do you have a backtrace of such a case?  It really shouldn't happen
as the vdev is allocated in ->slave_alloc, and that one returns an
error if it couldn't allocate.

RE: [PATCH] fusion: streamline ->slave_alloc/->slave_destroy

[Moore, Eric Dean]

On Saturday, October 09, 2004 8:51 AM, Christoph Hellwig wrote:
> 
> On Wed, Oct 06, 2004 at 02:25:53PM -0400, Moore, Eric Dean wrote:
> > On Saturday, October 02, 2004 2:24 AM,  Christoph Hellwig wrote:
> > > @@ -2812,59 +2796,33 @@
> > >  mptscsih_slave_destroy(struct scsi_device *device)
> > >  {
> > >  	struct Scsi_Host	*host = device->host;
> > > -	MPT_SCSI_HOST		*hd;
> > > -	VirtDevice		*vdev;
> > > -	int 			raid_volume=0;
> > > +	MPT_SCSI_HOST		*hd = (MPT_SCSI_HOST *)host->hostdata;
> > > +	VirtDevice		*vdev =  hd->Targets[device->id];
> > > +	uint			target = device->id;
> > > +	uint			lun = device->lun;
> > >  
> > > -	hd = (MPT_SCSI_HOST *)host->hostdata;
> > > +	mptscsih_search_running_cmds(hd, target, lun);
> > >  
> > > -	if (hd == NULL)
> > > +	vdev->luns[0] &= ~(1 << lun);
> > 
> > Ok - I'm finally getting around to testing this patch.
> > 
> > The driver will oops on this line because this routine is 
> > sometimes called when vdev=NULL.  Your patch had removed the
> > check for this NULL pointer. We probally should add it back.
> 
> Do you have a backtrace of such a case?  It really shouldn't happen
> as the vdev is allocated in ->slave_alloc, and that one returns an
> error if it couldn't allocate.
>


The problem is mptscsih_slave_destroy is being called twice. The first
time we free hd->Targets, the second time we hit the oops.  I'm still
investigating. Here is back trace using source level debugger.   Also
beyond the back trace is snip of source, having the embedded printks.



Fusion MPT base driver 3.02.05
Copyright (c) 1999-2004 LSI Logic Corporation
mptbase: Initiating ioc0 bringup
ioc0: 53C1030: Capabilities={Initiator}
mptbase: Initiating ioc1 bringup
ioc1: 53C1030: Capabilities={Initiator}
Fusion MPT SCSI Host driver 3.02.05
scsi0 : ioc0: LSI53C1030, FwRev=01032700h, Ports=1, MaxQ=222, IRQ=52
allocating vdev for target=00
  Vendor: FUJITSU   Model: MAP3367NC       <7>Losing some ticks... checking
if CPU frequency changed.
  Rev: 0106
  Type:   Direct-Access                      ANSI SCSI revision: 03
SCSI device sda: 71775284 512-byte hdwr sectors (36749 MB)
SCSI device sda: drive cache: write through
 sda: sda1
Attached scsi disk sda at scsi0, channel 0, id 0, lun 0
allocating vdev for target=01
  Vendor: FUJITSU   Model: MAP3367NC       <7>Losing some ticks... checking
if CPU frequency changed.
  Rev: 0106
  Type:   Direct-Access                      ANSI SCSI revision: 03
SCSI device sdb: 71775284 512-byte hdwr sectors (36749 MB)
SCSI device sdb: drive cache: write through
 sdb: sdb1
Attached scsi disk sdb at scsi0, channel 0, id 1, lun 0
allocating vdev for target=02
  Vendor: SEAGATE   Model: ST318406LW        Rev: 0109
  Type:   Direct-Access                      ANSI SCSI revision: 03
SCSI device sdc: 35843670 512-byte hdwr sectors (18352 MB)
SCSI device sdc: drive cache: write through
 sdc: sdc1
Attached scsi disk sdc at scsi0, channel 0, id 2, lun 0
allocating vdev for target=03
setting vdev=NULL, target=03
[New Thread 1]
[Switching to Thread 1]

Breakpoint 1, mptscsih_slave_destroy (device=Variable "device" is not
available.
) at drivers/message/fusion/mptscsih.c:2863
2863                    printk("BAD vdev=NULL, target=0%x\n",target);
(gdb) bt
#0  mptscsih_slave_destroy (device=Variable "device" is not available.
) at drivers/message/fusion/mptscsih.c:2863
#1  0xc025d8ca in scsi_probe_and_add_lun (host=0xc1559800, channel=Variable
"channel" is not available.
) at drivers/scsi/scsi_scan.c:775
#2  0xc025df75 in scsi_scan_target (shost=0xc1559800, channel=0x0, id=0x3,
lun=0xc1560114, rescan=0x0)
    at drivers/scsi/scsi_scan.c:1179
#3  0xc025e030 in scsi_scan_channel (shost=0xc1559800, channel=0x0, id=0x4,
lun=0xffffffff, rescan=0x0)
    at drivers/scsi/scsi_scan.c:1224
#4  0xc025e0ca in scsi_scan_host_selected (shost=0xc1559800, channel=0x1,
id=0xffffffff, lun=0xffffffff, rescan=0x0)
    at drivers/scsi/scsi_scan.c:1244
#5  0xc025e142 in scsi_scan_host (shost=Variable "shost" is not available.
) at drivers/scsi/scsi_scan.c:1258
#6  0xc0268daa in mptscsih_probe (pdev=0xdfebb000, id=Variable "id" is not
available.
) at drivers/message/fusion/mptscsih.c:1328
#7  0xc0262214 in mpt_device_driver_register (dd_cbfunc=0xc0390800,
cb_idx=Variable "cb_idx" is not available.
) at drivers/message/fusion/mptbase.c:770
#8  0xc03f49a3 in mptscsih_init () at drivers/message/fusion/mptscsih.c:1682
#9  0xc03d8683 in do_initcalls () at init/main.c:523
#10 0xc01003ea in init (unused=Variable "unused" is not available.
) at init/main.c:629
#11 0xc0104265 in kernel_thread_helper () at arch/i386/kernel/process.c:229
(gdb)


----------------------------------------------------------------------------
--

Source Snip


static int
mptscsih_slave_alloc(struct scsi_device *device)
{
	struct Scsi_Host	*host = device->host;
	MPT_SCSI_HOST		*hd = (MPT_SCSI_HOST *)host->hostdata;
	VirtDevice		*vdev;
	uint			target = device->id;

	if (hd == NULL)
		return -ENODEV;

	if ((vdev = hd->Targets[target]) != NULL) {
		printk("returning for target=0%x\n",target);
		goto out;
	}

	vdev = kmalloc(sizeof(VirtDevice), GFP_KERNEL);
	if (!vdev) {
		printk(MYIOC_s_ERR_FMT "slave_alloc kmalloc(%zd) FAILED!\n",
				hd->ioc->name, sizeof(VirtDevice));
		return -ENOMEM;
	}

	printk("allocating vdev for target=0%x\n",target);

.
.
.
.
[snip]



/*
 *	OS entry point to allow for host driver to free allocated memory
 *	Called if no device present or device being unloaded
 */
static void
mptscsih_slave_destroy(struct scsi_device *device)
{
	struct Scsi_Host	*host = device->host;
	MPT_SCSI_HOST		*hd = (MPT_SCSI_HOST *)host->hostdata;
	VirtDevice		*vdev;
	uint			target = device->id;
	uint			lun = device->lun;

	if (hd == NULL){
		printk("hd=NULL\n");
		return;
	}

	mptscsih_search_running_cmds(hd, target, lun);

	if ((vdev = hd->Targets[target]) == NULL) {
		printk("BAD vdev=NULL, target=0%x\n",target);
		return;
	}

	vdev->luns[0] &= ~(1 << lun);
	if (--vdev->num_luns)
		return;

	kfree(hd->Targets[target]);
	hd->Targets[target] = NULL;
	printk("setting vdev=NULL, target=0%x\n",target);

.
.
.
.
[snip]



 




 

Re: [PATCH] fusion: streamline ->slave_alloc/->slave_destroy

[Christoph Hellwig]

On Mon, Oct 11, 2004 at 02:39:54PM -0400, Moore, Eric Dean wrote:
> > > > -	hd = (MPT_SCSI_HOST *)host->hostdata;
> > > > +	mptscsih_search_running_cmds(hd, target, lun);
> > > >  
> > > > -	if (hd == NULL)
> > > > +	vdev->luns[0] &= ~(1 << lun);
> > > 
> > > Ok - I'm finally getting around to testing this patch.
> > > 
> > > The driver will oops on this line because this routine is 
> > > sometimes called when vdev=NULL.  Your patch had removed the
> > > check for this NULL pointer. We probally should add it back.
> > 
> > Do you have a backtrace of such a case?  It really shouldn't happen
> > as the vdev is allocated in ->slave_alloc, and that one returns an
> > error if it couldn't allocate.
> >
> 
> 
> The problem is mptscsih_slave_destroy is being called twice. The first
> time we free hd->Targets, the second time we hit the oops.  I'm still
> investigating. Here is back trace using source level debugger.   Also
> beyond the back trace is snip of source, having the embedded printks.

the only thing I could imagine that could cause this is a race in
num_luns manipulation.  Care to check whether the oops goes away when
you turn it into an atomic_t?

Re: [PATCH] fusion: streamline ->slave_alloc/->slave_destroy

[Christoph Hellwig]

> The problem is mptscsih_slave_destroy is being called twice. The first
> time we free hd->Targets, the second time we hit the oops.  I'm still
> investigating. Here is back trace using source level debugger.   Also
> beyond the back trace is snip of source, having the embedded printks.

That was not just with my patch but with your new monster update, right?

It contains this totally bogus line:

+                       
+                       /* remove the device from our internal data structures */
+                       if(hd->Targets[pScsiReq->TargetID] != NULL) {
+                               mptscsih_slave_destroy(sc->device);
+                       }
+                   

in mptscsih_io_done

RE: [PATCH] fusion: streamline ->slave_alloc/->slave_destroy

[Moore, Eric Dean]

On Tuesday, October 26, 2004 6:55 AM, Christoph Hellwig wrote:
> That was not just with my patch but with your new monster 
> update, right?
> 
> It contains this totally bogus line:
> 
> +                       
> +                       /* remove the device from our 
> internal data structures */
> +                       if(hd->Targets[pScsiReq->TargetID] != NULL) {
> +                               mptscsih_slave_destroy(sc->device);
> +                       }
> +                   
> 
> in mptscsih_io_done
>

This code was added by request of the megalib team, and Matt Domsch.
This was added for support of hot swap in non-raid environment.  
The IOCSTATUS=MPI_IOCSTATUS_SCSI_DEVICE_NOT_THERE will be received 
when drive has been removed, and this code was added to automatically
remove the instance of the device.  Thus an application such as megalib
would
know real time that devices have been removed, instead of waiting on a scsi
bus scan.

Eric

Re: [PATCH] fusion: streamline ->slave_alloc/->slave_destroy

[Christoph Hellwig]

On Tue, Oct 26, 2004 at 12:02:13PM -0400, Moore, Eric Dean wrote:
> On Tuesday, October 26, 2004 6:55 AM, Christoph Hellwig wrote:
> > That was not just with my patch but with your new monster 
> > update, right?
> > 
> > It contains this totally bogus line:
> > 
> > +                       
> > +                       /* remove the device from our 
> > internal data structures */
> > +                       if(hd->Targets[pScsiReq->TargetID] != NULL) {
> > +                               mptscsih_slave_destroy(sc->device);
> > +                       }
> > +                   
> > 
> > in mptscsih_io_done
> >
> 
> This code was added by request of the megalib team, and Matt Domsch.
> This was added for support of hot swap in non-raid environment.  
> The IOCSTATUS=MPI_IOCSTATUS_SCSI_DEVICE_NOT_THERE will be received 
> when drive has been removed, and this code was added to automatically
> remove the instance of the device.  Thus an application such as megalib
> would
> know real time that devices have been removed, instead of waiting on a scsi
> bus scan.

but this is buggy.  You must tell the scsi midlayer to perform a removal
with scsi_remove_device, which will call back into ->slave_destroy.  If
you just call mptscsih_slave_destroy there's still a midlayer
scsi_device object, but your internal datastructures are dangling.