[PATCH 3/8] aacraid: handle AIF hotplug events

[PATCH 3/8] aacraid: handle AIF hotplug events

[Mark Haverkamp]

Received from Mark Salyzyn from Adaptec.

Hotplug sniffs the AIFs (events) from the adapter and if a container
change resulting in the device going offline (container zero), online
(container zero completed) or changing capacity (morph) it will take
actions by calling the appropriate API.

Applies to the scsi-misc-2.6 git tree.

Signed-off-by: Mark Haverkamp <markh@osdl.org>


Index: scsi-misc-aac-2/drivers/scsi/aacraid/aachba.c
===================================================================
--- scsi-misc-aac-2.orig/drivers/scsi/aacraid/aachba.c	2005-09-08 11:32:04.000000000 -0700
+++ scsi-misc-aac-2/drivers/scsi/aacraid/aachba.c	2005-09-08 11:32:23.000000000 -0700
@@ -479,7 +479,7 @@
  *	is updated in the struct fsa_dev_info structure rather than returned.
  */
  
-static int probe_container(struct aac_dev *dev, int cid)
+int probe_container(struct aac_dev *dev, int cid)
 {
 	struct fsa_dev_info *fsa_dev_ptr;
 	int status;
@@ -1455,6 +1455,8 @@
 				case TEST_UNIT_READY:
 					spin_unlock_irq(host->host_lock);
 					probe_container(dev, cid);
+					if ((fsa_dev_ptr[cid].valid & 1) == 0)
+						fsa_dev_ptr[cid].valid = 0;
 					spin_lock_irq(host->host_lock);
 					if (fsa_dev_ptr[cid].valid == 0) {
 						scsicmd->result = DID_NO_CONNECT << 16;
Index: scsi-misc-aac-2/drivers/scsi/aacraid/aacraid.h
===================================================================
--- scsi-misc-aac-2.orig/drivers/scsi/aacraid/aacraid.h	2005-09-08 11:32:04.000000000 -0700
+++ scsi-misc-aac-2/drivers/scsi/aacraid/aacraid.h	2005-09-08 11:32:23.000000000 -0700
@@ -780,7 +780,9 @@
 	u64		last;
 	u64		size;
 	u32		type;
+	u32		config_waiting_on;
 	u16		queue_depth;
+	u8		config_needed;
 	u8		valid;
 	u8		ro;
 	u8		locked;
@@ -1715,6 +1717,7 @@
 #define		AifCmdJobProgress	2	/* Progress report */
 #define			AifJobCtrZero	101	/* Array Zero progress */
 #define			AifJobStsSuccess 1	/* Job completes */
+#define			AifJobStsRunning 102	/* Job running */
 #define		AifCmdAPIReport		3	/* Report from other user of API */
 #define		AifCmdDriverNotify	4	/* Notify host driver of event */
 #define			AifDenMorphComplete 200	/* A morph operation completed */
@@ -1785,6 +1788,7 @@
 struct aac_driver_ident* aac_get_driver_ident(int devtype);
 int aac_get_adapter_info(struct aac_dev* dev);
 int aac_send_shutdown(struct aac_dev *dev);
+int probe_container(struct aac_dev *dev, int cid);
 extern int numacb;
 extern int acbsize;
 extern char aac_driver_version[];
Index: scsi-misc-aac-2/drivers/scsi/aacraid/commsup.c
===================================================================
--- scsi-misc-aac-2.orig/drivers/scsi/aacraid/commsup.c	2005-09-08 11:32:17.000000000 -0700
+++ scsi-misc-aac-2/drivers/scsi/aacraid/commsup.c	2005-09-08 11:32:23.000000000 -0700
@@ -39,6 +39,7 @@
 #include <linux/completion.h>
 #include <linux/blkdev.h>
 #include <scsi/scsi_host.h>
+#include <scsi/scsi_device.h>
 #include <asm/semaphore.h>
 
 #include "aacraid.h"
@@ -791,6 +792,291 @@
 	memset(cp, 0,  256);
 }
 
+
+/**
+ *	aac_handle_aif		-	Handle a message from the firmware
+ *	@dev: Which adapter this fib is from
+ *	@fibptr: Pointer to fibptr from adapter
+ *
+ *	This routine handles a driver notify fib from the adapter and
+ *	dispatches it to the appropriate routine for handling.
+ */
+
+static void aac_handle_aif(struct aac_dev * dev, struct fib * fibptr)
+{
+	struct hw_fib * hw_fib = fibptr->hw_fib;
+	struct aac_aifcmd * aifcmd = (struct aac_aifcmd *)hw_fib->data;
+	int busy;
+	u32 container;
+	struct scsi_device *device;
+	enum {
+		NOTHING,
+		DELETE,
+		ADD,
+		CHANGE
+	} device_config_needed;
+
+	/* Sniff for container changes */
+
+	if (!dev)
+		return;
+	container = (u32)-1;
+
+	/*
+	 *	We have set this up to try and minimize the number of
+	 * re-configures that take place. As a result of this when
+	 * certain AIF's come in we will set a flag waiting for another
+	 * type of AIF before setting the re-config flag.
+	 */
+	switch (le32_to_cpu(aifcmd->command)) {
+	case AifCmdDriverNotify:
+		switch (le32_to_cpu(((u32 *)aifcmd->data)[0])) {
+		/*
+		 *	Morph or Expand complete
+		 */
+		case AifDenMorphComplete:
+		case AifDenVolumeExtendComplete:
+			container = le32_to_cpu(((u32 *)aifcmd->data)[1]);
+			if (container >= dev->maximum_num_containers)
+				break;
+
+			/*
+			 *	Find the Scsi_Device associated with the SCSI
+			 * address. Make sure we have the right array, and if
+			 * so set the flag to initiate a new re-config once we
+			 * see an AifEnConfigChange AIF come through.
+			 */
+
+			if ((dev != (struct aac_dev *)NULL)
+			 && (dev->scsi_host_ptr != (struct Scsi_Host *)NULL)) {
+				shost_for_each_device(device,
+					dev->scsi_host_ptr)
+				{
+					if ((device->channel ==
+						CONTAINER_TO_CHANNEL(container))
+					 && (device->id ==
+						CONTAINER_TO_ID(container))
+					 && (device->lun ==
+						CONTAINER_TO_LUN(container))) {
+
+						dev->fsa_dev[container].config_needed = CHANGE;
+						dev->fsa_dev[container].config_waiting_on = AifEnConfigChange;
+						break;
+					}
+				}
+			}
+		}
+
+		/*
+		 *	If we are waiting on something and this happens to be
+		 * that thing then set the re-configure flag.
+		 */
+		if (container != (u32)-1) {
+			if (container >= dev->maximum_num_containers)
+				break;
+			if (dev->fsa_dev[container].config_waiting_on ==
+			    le32_to_cpu(*(u32 *)aifcmd->data))
+				dev->fsa_dev[container].config_waiting_on = 0;
+		} else for (container = 0;
+		    container < dev->maximum_num_containers; ++container) {
+			if (dev->fsa_dev[container].config_waiting_on ==
+			    le32_to_cpu(*(u32 *)aifcmd->data))
+				dev->fsa_dev[container].config_waiting_on = 0;
+		}
+		break;
+
+	case AifCmdEventNotify:
+		switch (le32_to_cpu(((u32 *)aifcmd->data)[0])) {
+		/*
+		 *	Add an Array.
+		 */
+		case AifEnAddContainer:
+			container = le32_to_cpu(((u32 *)aifcmd->data)[1]);
+			if (container >= dev->maximum_num_containers)
+				break;
+			dev->fsa_dev[container].config_needed = ADD;
+			dev->fsa_dev[container].config_waiting_on =
+				AifEnConfigChange;
+			break;
+
+		/*
+		 *	Delete an Array.
+		 */
+		case AifEnDeleteContainer:
+			container = le32_to_cpu(((u32 *)aifcmd->data)[1]);
+			if (container >= dev->maximum_num_containers)
+				break;
+			dev->fsa_dev[container].config_needed = DELETE;
+			dev->fsa_dev[container].config_waiting_on =
+				AifEnConfigChange;
+			break;
+
+		/*
+		 *	Container change detected. If we currently are not
+		 * waiting on something else, setup to wait on a Config Change.
+		 */
+		case AifEnContainerChange:
+			container = le32_to_cpu(((u32 *)aifcmd->data)[1]);
+			if (container >= dev->maximum_num_containers)
+				break;
+			if (dev->fsa_dev[container].config_waiting_on)
+				break;
+			dev->fsa_dev[container].config_needed = CHANGE;
+			dev->fsa_dev[container].config_waiting_on =
+				AifEnConfigChange;
+			break;
+
+		case AifEnConfigChange:
+			break;
+
+		}
+
+		/*
+		 *	If we are waiting on something and this happens to be
+		 * that thing then set the re-configure flag.
+		 */
+		if (container != (u32)-1) {
+			if (container >= dev->maximum_num_containers)
+				break;
+			if (dev->fsa_dev[container].config_waiting_on ==
+			    le32_to_cpu(*(u32 *)aifcmd->data))
+				dev->fsa_dev[container].config_waiting_on = 0;
+		} else for (container = 0;
+		    container < dev->maximum_num_containers; ++container) {
+			if (dev->fsa_dev[container].config_waiting_on ==
+			    le32_to_cpu(*(u32 *)aifcmd->data))
+				dev->fsa_dev[container].config_waiting_on = 0;
+		}
+		break;
+
+	case AifCmdJobProgress:
+		/*
+		 *	These are job progress AIF's. When a Clear is being
+		 * done on a container it is initially created then hidden from
+		 * the OS. When the clear completes we don't get a config
+		 * change so we monitor the job status complete on a clear then
+		 * wait for a container change.
+		 */
+
+		if ((((u32 *)aifcmd->data)[1] == cpu_to_le32(AifJobCtrZero))
+		 && ((((u32 *)aifcmd->data)[6] == ((u32 *)aifcmd->data)[5])
+		  || (((u32 *)aifcmd->data)[4] == cpu_to_le32(AifJobStsSuccess)))) {
+			for (container = 0;
+			    container < dev->maximum_num_containers;
+			    ++container) {
+				/*
+				 * Stomp on all config sequencing for all
+				 * containers?
+				 */
+				dev->fsa_dev[container].config_waiting_on =
+					AifEnContainerChange;
+				dev->fsa_dev[container].config_needed = ADD;
+			}
+		}
+		if ((((u32 *)aifcmd->data)[1] == cpu_to_le32(AifJobCtrZero))
+		 && (((u32 *)aifcmd->data)[6] == 0)
+		 && (((u32 *)aifcmd->data)[4] == cpu_to_le32(AifJobStsRunning))) {
+			for (container = 0;
+			    container < dev->maximum_num_containers;
+			    ++container) {
+				/*
+				 * Stomp on all config sequencing for all
+				 * containers?
+				 */
+				dev->fsa_dev[container].config_waiting_on =
+					AifEnContainerChange;
+				dev->fsa_dev[container].config_needed = DELETE;
+			}
+		}
+		break;
+	}
+
+	device_config_needed = NOTHING;
+	for (container = 0; container < dev->maximum_num_containers;
+	    ++container) {
+		if ((dev->fsa_dev[container].config_waiting_on == 0)
+		 && (dev->fsa_dev[container].config_needed != NOTHING)) {
+			device_config_needed =
+				dev->fsa_dev[container].config_needed;
+			dev->fsa_dev[container].config_needed = NOTHING;
+			break;
+		}
+	}
+	if (device_config_needed == NOTHING)
+		return;
+
+	/*
+	 *	If we decided that a re-configuration needs to be done,
+	 * schedule it here on the way out the door, please close the door
+	 * behind you.
+	 */
+
+	busy = 0;
+
+
+	/*
+	 *	Find the Scsi_Device associated with the SCSI address,
+	 * and mark it as changed, invalidating the cache. This deals
+	 * with changes to existing device IDs.
+	 */
+
+	if (!dev || !dev->scsi_host_ptr)
+		return;
+	/*
+	 * force reload of disk info via probe_container
+	 */
+	if ((device_config_needed == CHANGE)
+	 && (dev->fsa_dev[container].valid == 1))
+		dev->fsa_dev[container].valid = 2;
+	if ((device_config_needed == CHANGE) ||
+			(device_config_needed == ADD))
+		probe_container(dev, container);
+	shost_for_each_device(device, dev->scsi_host_ptr)
+	{
+		if ((device->channel == CONTAINER_TO_CHANNEL(container))
+		 && (device->id == CONTAINER_TO_ID(container))
+		 && (device->lun == CONTAINER_TO_LUN(container))) {
+			busy |= device->device_busy ||
+				test_bit(SHOST_RECOVERY,
+				(const unsigned long*)&dev->scsi_host_ptr->shost_state);
+			if (busy == 0) {
+				device->removable = 1;
+				switch (device_config_needed) {
+				case ADD:
+					/*
+					 *	No need to call
+					 * scsi_scan_single_target
+					 */
+					device_config_needed = CHANGE;
+					scsi_add_device(dev->scsi_host_ptr,
+					  device->channel, device->id,
+					  device->lun);
+					break;
+				case DELETE:
+					scsi_remove_device(device);
+					break;
+				case CHANGE:
+	 				if (!dev->fsa_dev[container].valid) {
+						scsi_remove_device(device);
+						break;
+					}
+					scsi_rescan_device(&device->sdev_gendev);
+
+				default:
+					break;
+				}
+			}
+		}
+	}
+	if (device_config_needed == ADD) {
+		scsi_add_device(dev->scsi_host_ptr,
+		  CONTAINER_TO_CHANNEL(container),
+		  CONTAINER_TO_ID(container),
+		  CONTAINER_TO_LUN(container));
+	}
+
+}
+
 /**
  *	aac_command_thread	-	command processing thread
  *	@dev: Adapter to monitor
@@ -860,6 +1146,7 @@
 			aifcmd = (struct aac_aifcmd *) hw_fib->data;
 			if (aifcmd->command == cpu_to_le32(AifCmdDriverNotify)) {
 				/* Handle Driver Notify Events */
+				aac_handle_aif(dev, fib);
 				*(__le32 *)hw_fib->data = cpu_to_le32(ST_OK);
 				fib_adapter_complete(fib, (u16)sizeof(u32));
 			} else {
@@ -872,7 +1159,15 @@
 				unsigned num;
 				struct hw_fib ** hw_fib_pool, ** hw_fib_p;
 				struct fib ** fib_pool, ** fib_p;
-				
+			
+				/* Sniff events */
+				if ((aifcmd->command == 
+				     cpu_to_le32(AifCmdEventNotify)) ||
+				    (aifcmd->command == 
+				     cpu_to_le32(AifCmdJobProgress))) {
+					aac_handle_aif(dev, fib);
+				}
+ 				
 				time_now = jiffies/HZ;
 
 				/*

-- 
Mark Haverkamp <markh@osdl.org>

Re: [PATCH 3/8] aacraid: handle AIF hotplug events

[James Bottomley]

On Thu, 2005-09-08 at 13:50 -0700, Mark Haverkamp wrote:
> 					device_config_needed = CHANGE;
> +					scsi_add_device(dev->scsi_host_ptr,
> +					  device->channel, device->id,
> +					  device->lun);

Well ... this is actually wrong: scsi_add_device() returns an sdev on
success with the refcount bumped up for you.  If you don't do a put,
you'll be stuck with a device you can't get rid of.

However, all other users of scsi_add_device() make the same mistake ...
I'll apply this patch and fix the API to return zero or error and not
bump the sdev refcount.

James

Re: [PATCH 3/8] aacraid: handle AIF hotplug events

[Stefan Richter]

James Bottomley wrote:
> scsi_add_device() returns an sdev on
> success with the refcount bumped up for you.  If you don't do a put,
> you'll be stuck with a device you can't get rid of.
> 
> However, all other users of scsi_add_device() make the same mistake ...

Sbp2 was fixed recently, thanks to your advice. (Although the fix did 
not make it into Linus' tree yet.)

> I'll apply this patch and fix the API to return zero or error and not
> bump the sdev refcount.

If you will do this, the now correct scsi_device_put() will become not 
only incorrect but even dangerous, won't it?
-- 
Stefan Richter
-=====-=-=-= =--= -=-=-
http://arcgraph.de/sr/

Re: [PATCH 3/8] aacraid: handle AIF hotplug events

[James Bottomley]

On Sat, 2005-09-10 at 19:35 +0200, Stefan Richter wrote:
> James Bottomley wrote:
> > scsi_add_device() returns an sdev on
> > success with the refcount bumped up for you.  If you don't do a put,
> > you'll be stuck with a device you can't get rid of.
> > 
> > However, all other users of scsi_add_device() make the same mistake ...
> 
> Sbp2 was fixed recently, thanks to your advice. (Although the fix did 
> not make it into Linus' tree yet.)

Yes, I forgot about that, we will eventually have one correct user ;-)

> > I'll apply this patch and fix the API to return zero or error and not
> > bump the sdev refcount.
> 
> If you will do this, the now correct scsi_device_put() will become not 
> only incorrect but even dangerous, won't it?

I've cc'd you on the patch (and it fixes sbp2 as it currently stands in
the Linus tree, but it would conflict with the fix you have pending).
It basically makes scsi_add_device return an int rather than a device
pointer, so the compiler will now flag if something actually tries to
use the return as a pointer.

James

scsi device refcounting in sbp2 (was Re: [PATCH 3/8] aacraid: handle AIF hotplug events)

[Stefan Richter]

James Bottomley wrote:
>> > scsi_add_device() returns an sdev on
>> > success with the refcount bumped up for you.  If you don't do a put,
>> > you'll be stuck with a device you can't get rid of.
>> > 
>> > However, all other users of scsi_add_device() make the same mistake ...
[...]
>> > I'll apply this patch and fix the API to return zero or error and not
>> > bump the sdev refcount.
[...]
> I've cc'd you on the patch (and it fixes sbp2 as it currently stands in
> the Linus tree, but it would conflict with the fix you have pending).

I was just trying to adapt that fix and stumbled upon potential problems
of my code. Two questions:

1. Is the following sequence safe?
[connect to device]
	sdev = __scsi_add_device();
	scsi_device_put(sdev);
	store sdev in sbp2's private data for later use
[sbp2 .remove hook]
	scsi_remove_device(sdev);

2. Is this safe?
[connect to device]
	sdev = __scsi_add_device();
	scsi_device_put(sdev);
[sbp2 .remove hook]
	scsi_remove_host(shost);
I ask because scsi_remove_host will implicitly remove the device too.

BTW, the following won't work:
[connect to device]
	sdev = __scsi_add_device();
	store sdev for later use
[sbp2 .remove hook]
	scsi_remove_device(sdev);
	scsi_device_put(sdev);
That way, "modprobe -r sbp2" would fail because sbp2 is "in use" until
scsi_device_put.

Thanks for advice,
-- 
Stefan Richter
-=====-=-=-= =--= -=-=-
http://arcgraph.de/sr/

Re: scsi device refcounting in sbp2 (was Re: [PATCH 3/8] aacraid: handle AIF hotplug events)

[James Bottomley]

On Sat, 2005-09-10 at 23:16 +0200, Stefan Richter wrote:
> I was just trying to adapt that fix and stumbled upon potential problems
> of my code. Two questions:
> 
> 1. Is the following sequence safe?
> [connect to device]
> 	sdev = __scsi_add_device();
> 	scsi_device_put(sdev);
> 	store sdev in sbp2's private data for later use
> [sbp2 .remove hook]
> 	scsi_remove_device(sdev);

not really ... you have an undeclared reference to sdev.  If someone
removed it outside of the driver (using one of the remove APIs) then
you'd be left with a stale pointer.


> 2. Is this safe?
> [connect to device]
> 	sdev = __scsi_add_device();
> 	scsi_device_put(sdev);
> [sbp2 .remove hook]
> 	scsi_remove_host(shost);
> I ask because scsi_remove_host will implicitly remove the device too.
> 
> BTW, the following won't work:
> [connect to device]
> 	sdev = __scsi_add_device();
> 	store sdev for later use
> [sbp2 .remove hook]
> 	scsi_remove_device(sdev);
> 	scsi_device_put(sdev);
> That way, "modprobe -r sbp2" would fail because sbp2 is "in use" until
> scsi_device_put.

well, you could do this

[connect to device]
	sdev = __scsi_add_device();
	get_device(&sdev->sdev_gendev);
	scsi_device_put(sdev);
	store sdev for later use
[sbp2 .remove hook]
	scsi_remove_device(sdev);
	put_device(&sdev->sdev_gendev);

But that would hold the device for the entire lifetime of your module,
which I think, isn't really what you want.  Any user requested removal
would remove the visibility of the device but wouldn't actually destroy
it (so the next add would get into difficulty).  I suspect what you want
to do is something like this:

[connect to device]
	scsi_add_device();
	store sdev parameters (id and lun) for later use
[sbp2 .remove hook]
	spin_lock_irqsave(host_lock);
	sdev = __scsi_lookup_device(shost, c, id, lun);
	spin_unlock_irqsave(host_lock);
	if (sdev)
		scsi_remove_device(sdev);

which will behave correctly if the user removes the device;  It's a bit
inelegant, but it should be the minimum code change.

A better method would be to use the slave_alloc/slave_destroy hooks to
attach your data to that of the device.  You know that if slave_destroy
hasn't been called on the sdev, then it must be valid ... and you also
know that the user has requested an ejection if you get a slave_destroy
() call on it.

James









-------------------------------------------------------
SF.Net email is Sponsored by the Better Software Conference & EXPO
September 19-22, 2005 * San Francisco, CA * Development Lifecycle Practices
Agile & Plan-Driven Development * Managing Projects & Teams * Testing & QA
Security * Process Improvement & Measurement * http://www.sqe.com/bsce5sf

Re: scsi device refcounting in sbp2

[Stefan Richter]

James Bottomley wrote:
> ... and you also know that the user has requested an ejection if
> you get a slave_destroy() call on it.

Thanks for the clarification and suggestions. I'll try and see what 
works best.
-- 
Stefan Richter
-=====-=-=-= =--= -=-==
http://arcgraph.de/sr/

Re: [PATCH 3/8] aacraid: handle AIF hotplug events

[Christoph Hellwig]

> +			if ((dev != (struct aac_dev *)NULL)
> +			 && (dev->scsi_host_ptr != (struct Scsi_Host *)NULL)) {

no need for the casts.

> +	shost_for_each_device(device, dev->scsi_host_ptr)
> +	{
> +		if ((device->channel == CONTAINER_TO_CHANNEL(container))
> +		 && (device->id == CONTAINER_TO_ID(container))
> +		 && (device->lun == CONTAINER_TO_LUN(container))) {
> +			busy |= device->device_busy ||
> +				test_bit(SHOST_RECOVERY,
> +				(const unsigned long*)&dev->scsi_host_ptr->shost_state);

this is broken.  You must not look at the device_busy field from a driver,
and it means something different than what seems intended here.  Also
the direct messing with the host state is wrong.

> +			if (busy == 0) {
> +				device->removable = 1;

devce->removeable means the device has a removable medium, not that
it can go away.

---end quoted text---

Re: [PATCH 3/8] aacraid: handle AIF hotplug events (Updated)

[Mark Haverkamp]

On Sat, 2005-09-10 at 17:50 +0100, Christoph Hellwig wrote:
> > +			if ((dev != (struct aac_dev *)NULL)
> > +			 && (dev->scsi_host_ptr != (struct Scsi_Host *)NULL)) {
> 
> no need for the casts.
> 
> > +	shost_for_each_device(device, dev->scsi_host_ptr)
> > +	{
> > +		if ((device->channel == CONTAINER_TO_CHANNEL(container))
> > +		 && (device->id == CONTAINER_TO_ID(container))
> > +		 && (device->lun == CONTAINER_TO_LUN(container))) {
> > +			busy |= device->device_busy ||
> > +				test_bit(SHOST_RECOVERY,
> > +				(const unsigned long*)&dev->scsi_host_ptr->shost_state);
> 
> this is broken.  You must not look at the device_busy field from a driver,
> and it means something different than what seems intended here.  Also
> the direct messing with the host state is wrong.
> 
> > +			if (busy == 0) {
> > +				device->removable = 1;
> 
> devce->removeable means the device has a removable medium, not that
> it can go away.

Here is a new patch with the suggested changes.

- - - - 

Received from Mark Salyzyn from Adaptec.

Hotplug sniffs the AIFs (events) from the adapter and if a container
change resulting in the device going offline (container zero), online
(container zero completed) or changing capacity (morph) it will take
actions by calling the appropriate API.

Applies to the scsi-misc-2.6 git tree.

Signed-off-by: Mark Haverkamp <markh@osdl.org>


Index: scsi-misc-aac-2/drivers/scsi/aacraid/aachba.c
===================================================================
--- scsi-misc-aac-2.orig/drivers/scsi/aacraid/aachba.c	2005-09-09 07:47:45.000000000 -0700
+++ scsi-misc-aac-2/drivers/scsi/aacraid/aachba.c	2005-09-12 10:08:49.000000000 -0700
@@ -479,7 +479,7 @@
  *	is updated in the struct fsa_dev_info structure rather than returned.
  */
  
-static int probe_container(struct aac_dev *dev, int cid)
+int probe_container(struct aac_dev *dev, int cid)
 {
 	struct fsa_dev_info *fsa_dev_ptr;
 	int status;
@@ -1455,6 +1455,8 @@
 				case TEST_UNIT_READY:
 					spin_unlock_irq(host->host_lock);
 					probe_container(dev, cid);
+					if ((fsa_dev_ptr[cid].valid & 1) == 0)
+						fsa_dev_ptr[cid].valid = 0;
 					spin_lock_irq(host->host_lock);
 					if (fsa_dev_ptr[cid].valid == 0) {
 						scsicmd->result = DID_NO_CONNECT << 16;
Index: scsi-misc-aac-2/drivers/scsi/aacraid/aacraid.h
===================================================================
--- scsi-misc-aac-2.orig/drivers/scsi/aacraid/aacraid.h	2005-09-09 07:47:45.000000000 -0700
+++ scsi-misc-aac-2/drivers/scsi/aacraid/aacraid.h	2005-09-12 10:08:46.000000000 -0700
@@ -780,7 +780,9 @@
 	u64		last;
 	u64		size;
 	u32		type;
+	u32		config_waiting_on;
 	u16		queue_depth;
+	u8		config_needed;
 	u8		valid;
 	u8		ro;
 	u8		locked;
@@ -1715,6 +1717,7 @@
 #define		AifCmdJobProgress	2	/* Progress report */
 #define			AifJobCtrZero	101	/* Array Zero progress */
 #define			AifJobStsSuccess 1	/* Job completes */
+#define			AifJobStsRunning 102	/* Job running */
 #define		AifCmdAPIReport		3	/* Report from other user of API */
 #define		AifCmdDriverNotify	4	/* Notify host driver of event */
 #define			AifDenMorphComplete 200	/* A morph operation completed */
@@ -1785,6 +1788,7 @@
 struct aac_driver_ident* aac_get_driver_ident(int devtype);
 int aac_get_adapter_info(struct aac_dev* dev);
 int aac_send_shutdown(struct aac_dev *dev);
+int probe_container(struct aac_dev *dev, int cid);
 extern int numacb;
 extern int acbsize;
 extern char aac_driver_version[];
Index: scsi-misc-aac-2/drivers/scsi/aacraid/commsup.c
===================================================================
--- scsi-misc-aac-2.orig/drivers/scsi/aacraid/commsup.c	2005-09-09 07:58:15.000000000 -0700
+++ scsi-misc-aac-2/drivers/scsi/aacraid/commsup.c	2005-09-12 10:10:01.000000000 -0700
@@ -39,6 +39,7 @@
 #include <linux/completion.h>
 #include <linux/blkdev.h>
 #include <scsi/scsi_host.h>
+#include <scsi/scsi_device.h>
 #include <asm/semaphore.h>
 
 #include "aacraid.h"
@@ -791,6 +792,275 @@
 	memset(cp, 0,  256);
 }
 
+
+/**
+ *	aac_handle_aif		-	Handle a message from the firmware
+ *	@dev: Which adapter this fib is from
+ *	@fibptr: Pointer to fibptr from adapter
+ *
+ *	This routine handles a driver notify fib from the adapter and
+ *	dispatches it to the appropriate routine for handling.
+ */
+
+static void aac_handle_aif(struct aac_dev * dev, struct fib * fibptr)
+{
+	struct hw_fib * hw_fib = fibptr->hw_fib;
+	struct aac_aifcmd * aifcmd = (struct aac_aifcmd *)hw_fib->data;
+	int busy;
+	u32 container;
+	struct scsi_device *device;
+	enum {
+		NOTHING,
+		DELETE,
+		ADD,
+		CHANGE
+	} device_config_needed;
+
+	/* Sniff for container changes */
+
+	if (!dev)
+		return;
+	container = (u32)-1;
+
+	/*
+	 *	We have set this up to try and minimize the number of
+	 * re-configures that take place. As a result of this when
+	 * certain AIF's come in we will set a flag waiting for another
+	 * type of AIF before setting the re-config flag.
+	 */
+	switch (le32_to_cpu(aifcmd->command)) {
+	case AifCmdDriverNotify:
+		switch (le32_to_cpu(((u32 *)aifcmd->data)[0])) {
+		/*
+		 *	Morph or Expand complete
+		 */
+		case AifDenMorphComplete:
+		case AifDenVolumeExtendComplete:
+			container = le32_to_cpu(((u32 *)aifcmd->data)[1]);
+			if (container >= dev->maximum_num_containers)
+				break;
+
+			/*
+			 *	Find the Scsi_Device associated with the SCSI
+			 * address. Make sure we have the right array, and if
+			 * so set the flag to initiate a new re-config once we
+			 * see an AifEnConfigChange AIF come through.
+			 */
+
+			if ((dev != NULL)
+			 && (dev->scsi_host_ptr != NULL)) {
+				shost_for_each_device(device,
+					dev->scsi_host_ptr)
+				{
+					if ((device->channel ==
+						CONTAINER_TO_CHANNEL(container))
+					 && (device->id ==
+						CONTAINER_TO_ID(container))
+					 && (device->lun ==
+						CONTAINER_TO_LUN(container))) {
+
+						dev->fsa_dev[container].config_needed = CHANGE;
+						dev->fsa_dev[container].config_waiting_on = AifEnConfigChange;
+						break;
+					}
+				}
+			}
+		}
+
+		/*
+		 *	If we are waiting on something and this happens to be
+		 * that thing then set the re-configure flag.
+		 */
+		if (container != (u32)-1) {
+			if (container >= dev->maximum_num_containers)
+				break;
+			if (dev->fsa_dev[container].config_waiting_on ==
+			    le32_to_cpu(*(u32 *)aifcmd->data))
+				dev->fsa_dev[container].config_waiting_on = 0;
+		} else for (container = 0;
+		    container < dev->maximum_num_containers; ++container) {
+			if (dev->fsa_dev[container].config_waiting_on ==
+			    le32_to_cpu(*(u32 *)aifcmd->data))
+				dev->fsa_dev[container].config_waiting_on = 0;
+		}
+		break;
+
+	case AifCmdEventNotify:
+		switch (le32_to_cpu(((u32 *)aifcmd->data)[0])) {
+		/*
+		 *	Add an Array.
+		 */
+		case AifEnAddContainer:
+			container = le32_to_cpu(((u32 *)aifcmd->data)[1]);
+			if (container >= dev->maximum_num_containers)
+				break;
+			dev->fsa_dev[container].config_needed = ADD;
+			dev->fsa_dev[container].config_waiting_on =
+				AifEnConfigChange;
+			break;
+
+		/*
+		 *	Delete an Array.
+		 */
+		case AifEnDeleteContainer:
+			container = le32_to_cpu(((u32 *)aifcmd->data)[1]);
+			if (container >= dev->maximum_num_containers)
+				break;
+			dev->fsa_dev[container].config_needed = DELETE;
+			dev->fsa_dev[container].config_waiting_on =
+				AifEnConfigChange;
+			break;
+
+		/*
+		 *	Container change detected. If we currently are not
+		 * waiting on something else, setup to wait on a Config Change.
+		 */
+		case AifEnContainerChange:
+			container = le32_to_cpu(((u32 *)aifcmd->data)[1]);
+			if (container >= dev->maximum_num_containers)
+				break;
+			if (dev->fsa_dev[container].config_waiting_on)
+				break;
+			dev->fsa_dev[container].config_needed = CHANGE;
+			dev->fsa_dev[container].config_waiting_on =
+				AifEnConfigChange;
+			break;
+
+		case AifEnConfigChange:
+			break;
+
+		}
+
+		/*
+		 *	If we are waiting on something and this happens to be
+		 * that thing then set the re-configure flag.
+		 */
+		if (container != (u32)-1) {
+			if (container >= dev->maximum_num_containers)
+				break;
+			if (dev->fsa_dev[container].config_waiting_on ==
+			    le32_to_cpu(*(u32 *)aifcmd->data))
+				dev->fsa_dev[container].config_waiting_on = 0;
+		} else for (container = 0;
+		    container < dev->maximum_num_containers; ++container) {
+			if (dev->fsa_dev[container].config_waiting_on ==
+			    le32_to_cpu(*(u32 *)aifcmd->data))
+				dev->fsa_dev[container].config_waiting_on = 0;
+		}
+		break;
+
+	case AifCmdJobProgress:
+		/*
+		 *	These are job progress AIF's. When a Clear is being
+		 * done on a container it is initially created then hidden from
+		 * the OS. When the clear completes we don't get a config
+		 * change so we monitor the job status complete on a clear then
+		 * wait for a container change.
+		 */
+
+		if ((((u32 *)aifcmd->data)[1] == cpu_to_le32(AifJobCtrZero))
+		 && ((((u32 *)aifcmd->data)[6] == ((u32 *)aifcmd->data)[5])
+		  || (((u32 *)aifcmd->data)[4] == cpu_to_le32(AifJobStsSuccess)))) {
+			for (container = 0;
+			    container < dev->maximum_num_containers;
+			    ++container) {
+				/*
+				 * Stomp on all config sequencing for all
+				 * containers?
+				 */
+				dev->fsa_dev[container].config_waiting_on =
+					AifEnContainerChange;
+				dev->fsa_dev[container].config_needed = ADD;
+			}
+		}
+		if ((((u32 *)aifcmd->data)[1] == cpu_to_le32(AifJobCtrZero))
+		 && (((u32 *)aifcmd->data)[6] == 0)
+		 && (((u32 *)aifcmd->data)[4] == cpu_to_le32(AifJobStsRunning))) {
+			for (container = 0;
+			    container < dev->maximum_num_containers;
+			    ++container) {
+				/*
+				 * Stomp on all config sequencing for all
+				 * containers?
+				 */
+				dev->fsa_dev[container].config_waiting_on =
+					AifEnContainerChange;
+				dev->fsa_dev[container].config_needed = DELETE;
+			}
+		}
+		break;
+	}
+
+	device_config_needed = NOTHING;
+	for (container = 0; container < dev->maximum_num_containers;
+	    ++container) {
+		if ((dev->fsa_dev[container].config_waiting_on == 0)
+		 && (dev->fsa_dev[container].config_needed != NOTHING)) {
+			device_config_needed =
+				dev->fsa_dev[container].config_needed;
+			dev->fsa_dev[container].config_needed = NOTHING;
+			break;
+		}
+	}
+	if (device_config_needed == NOTHING)
+		return;
+
+	/*
+	 *	If we decided that a re-configuration needs to be done,
+	 * schedule it here on the way out the door, please close the door
+	 * behind you.
+	 */
+
+	busy = 0;
+
+
+	/*
+	 *	Find the Scsi_Device associated with the SCSI address,
+	 * and mark it as changed, invalidating the cache. This deals
+	 * with changes to existing device IDs.
+	 */
+
+	if (!dev || !dev->scsi_host_ptr)
+		return;
+	/*
+	 * force reload of disk info via probe_container
+	 */
+	if ((device_config_needed == CHANGE)
+	 && (dev->fsa_dev[container].valid == 1))
+		dev->fsa_dev[container].valid = 2;
+	if ((device_config_needed == CHANGE) ||
+			(device_config_needed == ADD))
+		probe_container(dev, container);
+	shost_for_each_device(device, dev->scsi_host_ptr)
+	{
+		if ((device->channel == CONTAINER_TO_CHANNEL(container))
+		 && (device->id == CONTAINER_TO_ID(container))
+		 && (device->lun == CONTAINER_TO_LUN(container))) {
+			switch (device_config_needed) {
+			case DELETE:
+				scsi_remove_device(device);
+				break;
+			case CHANGE:
+				if (!dev->fsa_dev[container].valid) {
+					scsi_remove_device(device);
+					break;
+				}
+				scsi_rescan_device(&device->sdev_gendev);
+
+			default:
+				break;
+			}
+		}
+	}
+	if (device_config_needed == ADD) {
+		scsi_add_device(dev->scsi_host_ptr,
+		  CONTAINER_TO_CHANNEL(container),
+		  CONTAINER_TO_ID(container),
+		  CONTAINER_TO_LUN(container));
+	}
+
+}
+
 /**
  *	aac_command_thread	-	command processing thread
  *	@dev: Adapter to monitor
@@ -860,6 +1130,7 @@
 			aifcmd = (struct aac_aifcmd *) hw_fib->data;
 			if (aifcmd->command == cpu_to_le32(AifCmdDriverNotify)) {
 				/* Handle Driver Notify Events */
+				aac_handle_aif(dev, fib);
 				*(__le32 *)hw_fib->data = cpu_to_le32(ST_OK);
 				fib_adapter_complete(fib, (u16)sizeof(u32));
 			} else {
@@ -872,7 +1143,15 @@
 				unsigned num;
 				struct hw_fib ** hw_fib_pool, ** hw_fib_p;
 				struct fib ** fib_pool, ** fib_p;
-				
+			
+				/* Sniff events */
+				if ((aifcmd->command == 
+				     cpu_to_le32(AifCmdEventNotify)) ||
+				    (aifcmd->command == 
+				     cpu_to_le32(AifCmdJobProgress))) {
+					aac_handle_aif(dev, fib);
+				}
+ 				
 				time_now = jiffies/HZ;
 
 				/*


-- 
Mark Haverkamp <markh@osdl.org>

RE: [PATCH 3/8] aacraid: handle AIF hotplug events

[Salyzyn, Mark]

Christoph Hellwig [mailto:hch@infradead.org] writes:
 
>> +			if (busy == 0) {
>> +				device->removable = 1;
> devce->removeable means the device has a removable medium, not that
> it can go away.

It also means that the capacity can change. The SCSI subsystem will
cache the partition table and capacity for the device if this is not
set.

-- Mark Salyzyn

Re: [PATCH 3/8] aacraid: handle AIF hotplug events

[Christoph Hellwig]

On Mon, Sep 12, 2005 at 07:10:05AM -0400, Salyzyn, Mark wrote:
> Christoph Hellwig [mailto:hch@infradead.org] writes:
>  
> >> +			if (busy == 0) {
> >> +				device->removable = 1;
> > devce->removeable means the device has a removable medium, not that
> > it can go away.
> 
> It also means that the capacity can change. The SCSI subsystem will
> cache the partition table and capacity for the device if this is not

please submit a patch to split ->removable into one flag for removal dervices
and one for those that allow underlying volume changes.

Usage of ->removable as-is is defintelitly wrong.

RE: [PATCH 3/8] aacraid: handle AIF hotplug events

[Salyzyn, Mark]

Christoph Hellwig sez:
>On Mon, Sep 12, 2005 at 07:10:05AM -0400, Salyzyn, Mark wrote:
>> Christoph Hellwig [mailto:hch@infradead.org] writes:
>> >> +				device->removable = 1;
>> > devce->removeable means the device has a removable medium, not that
>> > it can go away.
>> It also means that the capacity can change. The SCSI subsystem will 
>> cache the partition table and capacity for the device if this is not
>please submit a patch to split ->removable into one flag for removal
dervices and one for those that allow underlying volume changes.
>Usage of ->removable as-is is defintelitly wrong.

As this is too close to rc, and all these patches have had more than a
year of stability testing, we will remove this concern from this patch
(as already done in the Updated patch yesterday) in order to propagate
the AIF hot plug events now. Splitting and/or removable flag is perhaps
for another day as I expect such flag setting belongs in
aac_slave_configure instead. I would hope that you would approve of the
current updated patch.

And now on to the discussion.

The removable flag is used for RAID to also track prevent/allow media
removal (one recommended means to track busy status to block array
changes while array is in use) and to manage the write protect status
(transition to array failure during second chance recovery phase). What
is left as unnecessary is perhaps the check condition handling, but even
that has its desirable side-effects with regards to media change
notification after a Morph has completed (although the architectural
choice was to issue a scsi_rescan_device instead as part of this patch).
I have almost convinced myself to place if (sdp->removable ||
sdp->morphable_raid) in all the code locations in scsi_ioctl.c,
scsi_lib.c, scsi_scan.c and sd.c!

Allowing on-line array changes, failures & recovery is looking
remarkably similar to a removable device except for the pesky 'Device
not ready. Make sure there is a disc in the drive' message ;-> And now I
may be slitting my own throat. Until recently the aacraid driver set the
removable flag via the spoofed inquiry, this moved the flag to be
explicitly set so that the array could be declared clearly as an sd but
still behave like an sr. I did not expect a debate over the removable
flag as it has been done this way, in not just this array driver, for
some time.

-- Mark

RE: [PATCH 3/8] aacraid: handle AIF hotplug events

[Salyzyn, Mark]

The construct:

	shost_for_each_device(device, dev->scsi_host_ptr) {
		if (!(the_device_that_is_to_be_changed))
			continue;
		. . .
		switch (device_config_needed) {
		. . .
		case DELETE:
			scsi_remove_device(device);
			break;
		}
	}

Under intensive testing by an OEM client is proving to be 'unstable'.
Apparently the expectation of locking/usage count that is part of the
shost_for_each_device() call is not immune to the call to
scsi_remove_device() and can cause a panic in __scsi_iterate_devices ()
in 1 out of between 100 to 1000 tries.

Any suggestions? Pulling scsi_remove_device call(s) out of the loop only
serves to open up a race condition. This patch has yet to migrate into
scsi-misc-2.6, probably wise to hold off until the root cause can be
dealt with.

Sincerely -- Mark Salyzyn

RE: [PATCH 3/8] aacraid: handle AIF hotplug events

[Mark Haverkamp]

On Mon, 2005-09-19 at 13:28 -0400, Salyzyn, Mark wrote:
> The construct:
> 
> 	shost_for_each_device(device, dev->scsi_host_ptr) {
> 		if (!(the_device_that_is_to_be_changed))
> 			continue;
> 		. . .
> 		switch (device_config_needed) {
> 		. . .
> 		case DELETE:
> 			scsi_remove_device(device);
> 			break;
> 		}
> 	}
> 
> Under intensive testing by an OEM client is proving to be 'unstable'.
> Apparently the expectation of locking/usage count that is part of the
> shost_for_each_device() call is not immune to the call to
> scsi_remove_device() and can cause a panic in __scsi_iterate_devices ()
> in 1 out of between 100 to 1000 tries.
> 
> Any suggestions? Pulling scsi_remove_device call(s) out of the loop only
> serves to open up a race condition. This patch has yet to migrate into
> scsi-misc-2.6, probably wise to hold off until the root cause can be
> dealt with.

It looks like you only want to process one device.  Wouldn't you want to
break out of the shost_for_each_device after processing the
scsi_remove_device?  That way __scsi_iterate_devices() wouldn't get
called again after the device removal.

shost_for_each_device() {
        if ()
               continue;
        switch () {
        case DELETE:
              scsi_remove_device();
              break;
       }
       break;
}



-- 
Mark Haverkamp <markh@osdl.org>

RE: [PATCH 3/8] aacraid: handle AIF hotplug events

[Salyzyn, Mark]

The problem is, breaking out of the shost_for_each_device requires a put
on the device and we can no longer use it as an argument to
scsi_remove_device.

And if we decided to do so, what other race condition are we going to
trigger?

Sincerely -- Mark Salyzyn


-----Original Message-----
From: Mark Haverkamp [mailto:markh@osdl.org] 
Sent: Monday, September 19, 2005 1:57 PM
To: Salyzyn, Mark
Cc: James Bottomley; Christoph Hellwig; linux-scsi
Subject: RE: [PATCH 3/8] aacraid: handle AIF hotplug events


On Mon, 2005-09-19 at 13:28 -0400, Salyzyn, Mark wrote:
> The construct:
> 
> 	shost_for_each_device(device, dev->scsi_host_ptr) {
> 		if (!(the_device_that_is_to_be_changed))
> 			continue;
> 		. . .
> 		switch (device_config_needed) {
> 		. . .
> 		case DELETE:
> 			scsi_remove_device(device);
> 			break;
> 		}
> 	}
> 
> Under intensive testing by an OEM client is proving to be 'unstable'. 
> Apparently the expectation of locking/usage count that is part of the
> shost_for_each_device() call is not immune to the call to
> scsi_remove_device() and can cause a panic in __scsi_iterate_devices 
> () in 1 out of between 100 to 1000 tries.
> 
> Any suggestions? Pulling scsi_remove_device call(s) out of the loop 
> only serves to open up a race condition. This patch has yet to migrate

> into scsi-misc-2.6, probably wise to hold off until the root cause can

> be dealt with.

It looks like you only want to process one device.  Wouldn't you want to
break out of the shost_for_each_device after processing the
scsi_remove_device?  That way __scsi_iterate_devices() wouldn't get
called again after the device removal.

shost_for_each_device() {
        if ()
               continue;
        switch () {
        case DELETE:
              scsi_remove_device();
              break;
       }
       break;
}



-- 
Mark Haverkamp <markh@osdl.org>

RE: [PATCH 3/8] aacraid: handle AIF hotplug events

[Salyzyn, Mark]

I retract my statement.

	scsi_remove_device(device);
	scsi_host_put(dev->scsi_host_ptr);
	goto break_out_of_loop;

Should work fine. Using 'break' in a switch statement is not going to
break out of the loop.

Sincerely -- Mark Salyzyn


-----Original Message-----
From: linux-scsi-owner@vger.kernel.org
[mailto:linux-scsi-owner@vger.kernel.org] On Behalf Of Salyzyn, Mark
Sent: Monday, September 19, 2005 2:28 PM
To: Mark Haverkamp
Cc: James Bottomley; Christoph Hellwig; linux-scsi
Subject: RE: [PATCH 3/8] aacraid: handle AIF hotplug events


The problem is, breaking out of the shost_for_each_device requires a put
on the device and we can no longer use it as an argument to
scsi_remove_device.

And if we decided to do so, what other race condition are we going to
trigger?

Sincerely -- Mark Salyzyn


-----Original Message-----
From: Mark Haverkamp [mailto:markh@osdl.org] 
Sent: Monday, September 19, 2005 1:57 PM
To: Salyzyn, Mark
Cc: James Bottomley; Christoph Hellwig; linux-scsi
Subject: RE: [PATCH 3/8] aacraid: handle AIF hotplug events


On Mon, 2005-09-19 at 13:28 -0400, Salyzyn, Mark wrote:
> The construct:
> 
> 	shost_for_each_device(device, dev->scsi_host_ptr) {
> 		if (!(the_device_that_is_to_be_changed))
> 			continue;
> 		. . .
> 		switch (device_config_needed) {
> 		. . .
> 		case DELETE:
> 			scsi_remove_device(device);
> 			break;
> 		}
> 	}
> 
> Under intensive testing by an OEM client is proving to be 'unstable'.
> Apparently the expectation of locking/usage count that is part of the
> shost_for_each_device() call is not immune to the call to
> scsi_remove_device() and can cause a panic in __scsi_iterate_devices 
> () in 1 out of between 100 to 1000 tries.
> 
> Any suggestions? Pulling scsi_remove_device call(s) out of the loop
> only serves to open up a race condition. This patch has yet to migrate

> into scsi-misc-2.6, probably wise to hold off until the root cause can

> be dealt with.

It looks like you only want to process one device.  Wouldn't you want to
break out of the shost_for_each_device after processing the
scsi_remove_device?  That way __scsi_iterate_devices() wouldn't get
called again after the device removal.

shost_for_each_device() {
        if ()
               continue;
        switch () {
        case DELETE:
              scsi_remove_device();
              break;
       }
       break;
}



-- 
Mark Haverkamp <markh@osdl.org>

-
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org More majordomo info
at  http://vger.kernel.org/majordomo-info.html