[PATCH 3/3] Add disk hotswap support to libata RESEND #2

[PATCH 3/3] Add disk hotswap support to libata RESEND #2

[Lukasz Kosewski]

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

Patch 03:  Have sata_promise use the perfect, flawless API from the
previous patch

As described in the archives, this patch build on patch 02 in the
series to actually allow the sata_promise controller to hotswap.  Be
careful, untested!

This version comes with all of Jeff's suggestions (documented in the
patch itself).  It depends on patch 01 to apply, and both patch 01 and
02 to compile and run.

Luke Kosewski

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 03-promise_hotswap_support-2.6.13-rc3-mm1-2.diff --]
[-- Type: text/x-patch; name="03-promise_hotswap_support-2.6.13-rc3-mm1-2.diff", Size: 6400 bytes --]

01.08.05  Luke Kosewski  <lkosewsk@nit.ca>

	* A full implementation of hotplug on a libata controller, this being
	  the Promise Tx4/Tx2 Plus controller line (both SATA150 and SATAII150).
	  Almost all of the code pertaining to how to talk to the hotplug
	  registers has been stolen from the pdc-ulsata2 and ultra-1.0.8 Promise
	  drivers.  This involves detecting when we have an interrupt pending
	  and on what device, as well as the bit where a hard SATA reset gets
	  a controller to re-spew a plug interrupt.
	* Note that the hotplug handling code comes AFTER the normal interrupt
	  handling code in pdc_interrupt_common; this is because we're much
	  more likely to receive normal interrupts, so this drops the AVERAGE
	  interrupt handling time down a lot.
	* This is a resend of the original patch which in particular:
	  - Doesn't special-case the turning off of channel interrupts during a
	    hard reset to the SATAII150 controllers; all controllers now do
	    this.
	  - Actually aborts the interrupt handler if, after reading mmio_base +
	    PDC_INT_SEQMASK, we don't get ANYTHING (aka no device).
	  - On Jeff's suggestion, the whole hotswap system was changed over to
	    use a debounce timer; see patch 02 for that goodie.

Signed-off-by:  Luke Kosewski <lkosewsk@nit.ca>

--- linux/drivers/scsi/sata_promise.c.old	2005-07-28 20:43:52.000000000 -0700
+++ linux/drivers/scsi/sata_promise.c	2005-08-01 02:23:27.000000000 -0700
@@ -67,12 +67,16 @@ enum {
 	PDC_HAS_PATA		= (1 << 1), /* PDC20375/20575 has PATA */
 
 	PDC_RESET		= (1 << 11), /* HDMA reset */
+
+	HOTPLUG_PLUG		= 1,
+	HOTPLUG_UNPLUG		= 0,
 };
 
 
 struct pdc_port_priv {
 	u8			*pkt;
 	dma_addr_t		pkt_dma;
+	int			hotplug_status;
 };
 
 struct pdc_host_priv {
@@ -87,6 +91,7 @@ static void pdc_eng_timeout(struct ata_p
 static int pdc_port_start(struct ata_port *ap);
 static void pdc_port_stop(struct ata_port *ap);
 static void pdc_phy_reset(struct ata_port *ap);
+static int pdc_hotplug_action(struct ata_port *ap);
 static void pdc_pata_phy_reset(struct ata_port *ap);
 static void pdc_pata_cbl_detect(struct ata_port *ap);
 static void pdc_qc_prep(struct ata_queued_cmd *qc);
@@ -133,6 +138,7 @@ static struct ata_port_operations pdc_at
 	.port_start		= pdc_port_start,
 	.port_stop		= pdc_port_stop,
 	.host_stop		= ata_host_stop,
+	.hotplug_action		= pdc_hotplug_action,
 };
 
 static struct ata_port_info pdc_port_info[] = {
@@ -299,15 +305,61 @@ static void pdc_reset_port(struct ata_po
 	readl(mmio);	/* flush */
 }
 
+/* Mask hotplug interrupts for one channel (ap) */
+static inline void pdc_disable_channel_hotplug_interrupts(struct ata_port *ap)
+{
+	struct pdc_host_priv *hp = ap->host_set->private_data;
+	void *mmio = ap->host_set->mmio_base + hp->hotplug_offset + 2;
+
+	u8 maskflags = readb(mmio);
+	maskflags |= (0x11 << (u8)ap->hard_port_no);
+	writeb(maskflags, mmio);
+}
+
+/* Clear and unmask hotplug interrupts for one channel (ap) */
+static inline void pdc_enable_channel_hotplug_interrupts(struct ata_port *ap)
+{
+	struct pdc_host_priv *hp = ap->host_set->private_data;
+	void *mmio = ap->host_set->mmio_base + hp->hotplug_offset;
+
+	//Clear channel hotplug interrupts
+	u8 maskflags = readb(mmio);
+	maskflags |= (0x11 << (u8)ap->hard_port_no);
+	writeb(maskflags, mmio);
+
+	//Unmask channel hotplug interrupts
+	maskflags = readb(mmio + 2);
+	maskflags &= ~(0x11 << (u8)ap->hard_port_no);
+	writeb(maskflags, mmio + 2);
+}
+
 static void pdc_phy_reset(struct ata_port *ap)
 {
 	pdc_reset_port(ap);
-	if (ap->flags & ATA_FLAG_SATA)
-		sata_phy_reset(ap);
-	else
+	if (ap->flags & ATA_FLAG_SATA) {
+		/* A hard reset triggers a hotplug interrupt to be played out
+		 * on some controllers.  Hence, disable hotplug interrupts for
+		 * the reset, re-enable them when it's done.
+		 *
+		 * No PATA hotswap support yet
+		 */
+		if (ap->flags & ATA_FLAG_SATA_RESET) {
+			pdc_disable_channel_hotplug_interrupts(ap);
+			sata_phy_reset(ap);
+			pdc_enable_channel_hotplug_interrupts(ap);
+		} else
+			sata_phy_reset(ap);
+	} else
 		pdc_pata_phy_reset(ap);
 }
 
+/* Should be called with host_set->lock held */
+static int pdc_hotplug_action(struct ata_port *ap)
+{
+	struct pdc_port_priv *pp = ap->private_data;
+	return pp->hotplug_status;
+}
+
 static void pdc_pata_cbl_detect(struct ata_port *ap)
 {
 	u8 tmp;
@@ -467,7 +519,9 @@ static irqreturn_t pdc_interrupt (int ir
 	struct ata_port *ap;
 	void *mmio_base;
 	u32 mask = 0;
-	unsigned int i, tmp, handled = 0;
+	u8 plugdata, maskflags;
+	struct pdc_host_priv *hp = host_set->private_data;
+	unsigned int i, tmp, handled = 0, hotplug_offset = hp->hotplug_offset;
 
 	VPRINTK("ENTER\n");
 
@@ -491,7 +545,7 @@ static irqreturn_t pdc_interrupt (int ir
 	mask &= 0xffff;		/* only 16 tags possible */
 	if (!mask) {
 		VPRINTK("QUICK EXIT 3\n");
-		goto done_irq;
+		goto try_hotplug;
 	}
 
 	writel(mask, mmio_base + PDC_INT_SEQMASK);
@@ -509,7 +563,40 @@ static irqreturn_t pdc_interrupt (int ir
 		}
 	}
 
-	VPRINTK("EXIT\n");
+	if (handled) {
+		VPRINTK("EXIT 4\n");
+		goto done_irq;
+	}
+
+try_hotplug:
+	plugdata = readb(mmio_base + hotplug_offset);
+	maskflags = readb(mmio_base + hotplug_offset + 2);
+	plugdata &= ~maskflags;
+	if (plugdata) {
+		struct pdc_port_priv *pp;
+		writeb(plugdata, mmio_base + hotplug_offset);
+		for (i = 0; i < host_set->n_ports; ++i) {
+			ap = host_set->ports[i];
+			pp = ap->private_data;
+			if (!(ap->flags & ATA_FLAG_SATA))
+				continue;  //No PATA support here... yet
+			// Check unplug flag
+			if (plugdata & 0x1) {
+				/* Do stuff related to unplugging a device */
+				pp->hotplug_status = HOTPLUG_UNPLUG;
+				ata_hotplug_op(ap);
+				handled = 1;
+			} else if ((plugdata >> 4) & 0x1) {  //Check plug flag
+				/* Do stuff related to plugging in a device */
+				pp->hotplug_status = HOTPLUG_PLUG;
+				ata_hotplug_op(ap);
+				handled = 1;
+			}
+			plugdata >>= 1;
+		}
+	}
+
+	VPRINTK("EXIT 5\n");
        
 done_irq:
 	spin_unlock(&host_set->lock);
@@ -609,9 +696,9 @@ static void pdc_host_init(unsigned int c
 	tmp = readl(mmio + hotplug_offset);
 	writel(tmp | 0xff, mmio + hotplug_offset);
 
-	/* mask plug/unplug ints */
+	/* unmask plug/unplug ints */
 	tmp = readl(mmio + hotplug_offset);
-	writel(tmp | 0xff0000, mmio + hotplug_offset);
+	writel(tmp & ~0xff0000, mmio + hotplug_offset);
 
 	/* reduce TBG clock to 133 Mhz. */
 	tmp = readl(mmio + PDC_TBG_MODE);

Re: [PATCH 3/3] Add disk hotswap support to libata RESEND #2

[Jim Ramsay]

On 8/1/05, Lukasz Kosewski <lkosewsk@gmail.com> wrote:
> Patch 03:  Have sata_promise use the perfect, flawless API from the
> previous patch

Hmmm... Flawless :)

Then I must have found an undocumented feature!  I've applied this set
of patches to a 2.6.11 kernel (with few problems) and ran into a bunch
of "scheduling while atomic" errors when hotplugging a drive, culprit
being probably scsi_sysfs.c where scsi_remove_device locks a mutex, or
perhaps when it then calls class_device_unregister, which does a
'down_write'.

Perhaps we need some sort of workqueue for hotplug requests to get
them out of the atomic interrupt handler context where they originate?

-- 
Jim Ramsay
"Me fail English?  That's unpossible!"

Re: [PATCH 3/3] Add disk hotswap support to libata RESEND #2

[Jim Ramsay]

On 8/23/05, Jim Ramsay <jim.ramsay@gmail.com> wrote:
> Then I must have found an undocumented feature!  I've applied this set
> of patches to a 2.6.11 kernel (with few problems) and ran into a bunch
> of "scheduling while atomic" errors when hotplugging a drive, culprit
> being probably scsi_sysfs.c where scsi_remove_device locks a mutex, or
> perhaps when it then calls class_device_unregister, which does a
> 'down_write'.

After further debugging, it appears that the problem is the debounce
timer in libata-core.c.

Timers appear to operate in an atomic context, so timers should not be
allowed to call scsi_remove_device, which eventually schedules.

Any suggestions on the best way to fix this?

-- 
Jim Ramsay
"Me fail English?  That's unpossible!"

Re: [PATCH 3/3] Add disk hotswap support to libata RESEND #2

[George Anzinger]

Jim Ramsay wrote:
> On 8/23/05, Jim Ramsay <jim.ramsay@gmail.com> wrote:
> 
>>Then I must have found an undocumented feature!  I've applied this set
>>of patches to a 2.6.11 kernel (with few problems) and ran into a bunch
>>of "scheduling while atomic" errors when hotplugging a drive, culprit
>>being probably scsi_sysfs.c where scsi_remove_device locks a mutex, or
>>perhaps when it then calls class_device_unregister, which does a
>>'down_write'.
> 
> 
> After further debugging, it appears that the problem is the debounce
> timer in libata-core.c.
> 
> Timers appear to operate in an atomic context, so timers should not be
> allowed to call scsi_remove_device, which eventually schedules.
> 
> Any suggestions on the best way to fix this?

Workqueue, perhaps.
> 

-- 
George Anzinger   george@mvista.com
HRT (High-res-timers):  http://sourceforge.net/projects/high-res-timers/

Re: [PATCH 3/3] Add disk hotswap support to libata RESEND #2

[Stefan Richter]

George Anzinger wrote:
> Jim Ramsay wrote:
>> On 8/23/05, Jim Ramsay <jim.ramsay@gmail.com> wrote:
>>> I've applied this set
>>> of patches to a 2.6.11 kernel (with few problems) and ran into a bunch
>>> of "scheduling while atomic" errors when hotplugging a drive, culprit
>>> being probably scsi_sysfs.c
...
>> After further debugging, it appears that the problem is the debounce
>> timer in libata-core.c.
>>
>> Timers appear to operate in an atomic context, so timers should not be
>> allowed to call scsi_remove_device, which eventually schedules.
>>
>> Any suggestions on the best way to fix this?
> 
> Workqueue, perhaps.

The USB and IEEE 1394 subsystems have kernel threads which manage node 
additions and removals (usb/storage/usb.c, ieee1394/nodemgr.c). The add 
and remove functions of their storage drivers are called from these 
threads' non-atomic context. However if you don't need an own thread for 
any further bus management purposes, a workqueue looks suitable for hot 
plugging and unplugging.
-- 
Stefan Richter
-=====-=-=-= =--- ==---
http://arcgraph.de/sr/

Re: [PATCH 3/3] Add disk hotswap support to libata RESEND #2

[Lukasz Kosewski]

On 8/24/05, Stefan Richter <stefanr@s5r6.in-berlin.de> wrote:
> >> Timers appear to operate in an atomic context, so timers should not be
> >> allowed to call scsi_remove_device, which eventually schedules.
> >>
> >> Any suggestions on the best way to fix this?
> >
> > Workqueue, perhaps.

Perhaps.  Actually, of course :)

The reason these aren't working is because they have never been
tested.  I sent in my not-entirely-finished patches the night before I
left for China for one month.

When I get back to Waterloo (Ontario) in September, I should send in
revised versions of these patches with the following fixes:

- mod_timer instead of delete_timer/change timeout/add_timer
- bunch of code cleanups
- proper error handling
- actually making the patches work.

For now, check the list archives for the very first send of the
patches (with a resend of patch #3), and use those.  Those patches do
not use a debounce timer, so can be prone to some failures.  However,
I wasn't able to get those failures while testing, so at least you can
help me test out the overall logic and robustness of my work.

Luke Kosewski

Re: [PATCH 3/3] Add disk hotswap support to libata RESEND #2

[Jim Ramsay]

On 8/24/05, Lukasz Kosewski <lkosewsk@gmail.com> wrote:
> On 8/24/05, Stefan Richter <stefanr@s5r6.in-berlin.de> wrote:
> > >> Timers appear to operate in an atomic context, so timers should not be
> > >> allowed to call scsi_remove_device, which eventually schedules.
> > >>
> > >> Any suggestions on the best way to fix this?
> > >
> > > Workqueue, perhaps.
> 
> Perhaps.  Actually, of course :)

How about the existing ata_wq workqueue?  This makes sense.  When the
timer expires, it adds a task to this queue.

> The reason these aren't working is because they have never been
> tested.  I sent in my not-entirely-finished patches the night before I
> left for China for one month.

Well, I'm on a time-sensitive project right now, and they "need"
hotplug support, so maybe I'll patch your patches and do what testing
I can.  I'll post a fourth patch in a few days.

> When I get back to Waterloo (Ontario) in September, I should send in
> revised versions of these patches with the following fixes:
> 
> - mod_timer instead of delete_timer/change timeout/add_timer
That's easy.  I'll add it in my 'patch 4/3'

> - bunch of code cleanups
Haven't touched this, looks pretty clean to me

> - proper error handling
This may be something I'll have to stick my fingers in as I do more testing

> - actually making the patches work.
Hopefully I'll get this going.

[Update, 10 mins later] Hey, I've got unplugging working already!

-- 
Jim Ramsay
"Me fail English?  That's unpossible!"

Re: [PATCH 3/3] Add disk hotswap support to libata RESEND #2

[Jim Ramsay]

On 8/24/05, Jim Ramsay <jim.ramsay@gmail.com> wrote:
> On 8/24/05, Lukasz Kosewski <lkosewsk@gmail.com> wrote:
> > On 8/24/05, Stefan Richter <stefanr@s5r6.in-berlin.de> wrote:
> > > >> Timers appear to operate in an atomic context, so timers should not be
> > > >> allowed to call scsi_remove_device, which eventually schedules.
> > > >>
> > > >> Any suggestions on the best way to fix this?
> > > >
> > > > Workqueue, perhaps.
> >
> > Perhaps.  Actually, of course :)
> 
> How about the existing ata_wq workqueue?  This makes sense.  When the
> timer expires, it adds a task to this queue.

Note to self - No, you cannot use the exsting 'ata_wq' workqueue - The
plug-in events need to put other work on the queue during the hotplug
event... and of course this deadlocks since you're in the queuethread
already.

-- 
Jim Ramsay
"Me fail English?  That's unpossible!"

Re: [PATCH 3/3] Add disk hotswap support to libata RESEND #2

[Jim Ramsay]

Another interesting issue with hotplug which may need SCSI-layer changes:
 
If you unplug a device that some process already has open (for
example, md), the device is removed, but the internal scsi_device
struct is not actually freed until ALL references to this device
(including that still held by md) release it.
 
When you re-plugin the device at this time (scsi_device has been
marked as 'deleted' but not actually freed), the code properly detects
that while there is still a scsi_device for this host/channel/id/lun
combination, it is marked as "SDEV_DEL" and so should not be used. 
Fine - a new scsi_device struct is allocated and added to  the back of
the Scsi_Host->__devices list.
 
The problem: From now on, whenever someone wants to use this scsi
device, they will end up calling scsi_device_lookup, which calls
__scsi_device_lookup, which will always return the first scsi_device
struct, the one which is now SDEV_DEL, so scsi_device_lookup will
return NULL.  This means that subsequent hot-unplugs will not remove
the most-recently-allocated scsi_device struct, so no hotplug events
are sent and no udev devices are removed.  This is bad -> a memory
leak.
 
The workaround:  This probably has ramifications I'm not considering,
but the following replacement for __scsi_device_lookup in scsi.c seems
to work for me.  It returns the LAST matching scsi_device (and
therefore most-recently allocated), as opposed to the old code which
returned the FIRST matching scsi_device:
 
struct scsi_device *__scsi_device_lookup(struct Scsi_Host *shost,
                 uint channel, uint id, uint lun)
{
         struct scsi_device *found_sdev = NULL;
         struct scsi_device *sdev;
 
         list_for_each_entry(sdev, &shost->__devices, siblings) {
                 printk( "device lookup checking %d:%d:%d:%d against
%d:%d:%d:%d\n",
                                 shost->host_no, channel, id, lun,
                                 shost->host_no, sdev->channel,
sdev->id, sdev->lun );
                 if (sdev->channel == channel && sdev->id == id &&
                                 sdev->lun ==lun)
                 {
                         found_sdev = sdev;
                 }
         }
 
         return found_sdev;
}
 
How else could this be solved?  I suppose adding newly allocated
scsi_device structs to the FRONT of the Scsi_Host->__devices list
would have the same effect as my alternate search routine, but I don't
know which would be better.  I can't think of any solution that
wouldn't touch the existing SCSI layer, though.

-- 
Jim Ramsay
"Me fail English?  That's unpossible!"

Re: [PATCH 3/3] Add disk hotswap support to libata RESEND #2

[Jim Ramsay]

Regarding the sata_promise.c patch, I think I have found a bug in the
interrupt handler:

Just before the 'try_hotplug' label, you provide a check that will
kick us out of the interrupt handler if the interrupt was just handled
by a DMA command completing successfully.

However, I have seen the occasion where a single IRQ is used to signal
both a DMA completion AND a hotplug event.  Of course in this case the
hotplug event itself would be ignored completely.

So I would recommend getting rid of that check entirely.

-- 
Jim Ramsay
"Me fail English?  That's unpossible!"

Re: [PATCH 3/3] Add disk hotswap support to libata RESEND #2

[Lukasz Kosewski]

On 9/6/05, Jim Ramsay <jim.ramsay@gmail.com> wrote:
> However, I have seen the occasion where a single IRQ is used to signal
> both a DMA completion AND a hotplug event.  Of course in this case the
> hotplug event itself would be ignored completely.
> 
> So I would recommend getting rid of that check entirely.

Hey Jim,

Not that I disbelieve you, but do you have an example of a controller
where this happens?  I've done a lot of testing and never seen this...

Luke Kosewski

Re: [PATCH 3/3] Add disk hotswap support to libata RESEND #2

[Mark Lord]

Lukasz Kosewski wrote:
> On 9/6/05, Jim Ramsay <jim.ramsay@gmail.com> wrote:
> 
>>However, I have seen the occasion where a single IRQ is used to signal
>>both a DMA completion AND a hotplug event.  Of course in this case the
>>hotplug event itself would be ignored completely.
>>
>>So I would recommend getting rid of that check entirely.
> 
> 
> Hey Jim,
> 
> Not that I disbelieve you, but do you have an example of a controller
> where this happens?  I've done a lot of testing and never seen this...

I missed the beginning of this discussion,
but here's a data point:

The QStor SATA/RAID controller hardware fully supports hotplug
(and NCQ, TCQ, Host-Queuing, RAID 0/1/10, PM, etc..).

It uses a single interrupt for all onboard events from the four channels.
An internal "status FIFO" provides a readout for the interrupt handler
of recent happenings, in sequence, mixing together DMA-completions
with hotplug-events (insert, removal) and various fault-conditions.

All of this is supported in the out-of-tree qstor driver,
but only simple single-IO is supported by sata_qstor at present.

Dunno if that info is of any use to you in hotplug considerations.
Once the libata infrastructure for hotplug is in place,
I *may* experiment with adding that functionality to sata_qstor.

Cheers