Hey Jeff, all,
Patch 3/3 for libata hotswapping, the reference implementation for the
API in Patch #2 for Promise SATA150 and SATAII150 Tx4 and Tx2 Plus
controllers. Seems to be working on UP machines (heavy, heavy
testing) and SMP (2 processor Athlon machine, fairly heavy testing).
Lots of comments available in patch and header, please review and
apply if you like it!
This patch depends on patch 1 to apply, and patch 1 and 2 to compile and work.
Luke Kosewski
Lukasz Kosewski wrote:
> Hey Jeff, all,
>
> Patch 3/3 for libata hotswapping, the reference implementation for the
> API in Patch #2 for Promise SATA150 and SATAII150 Tx4 and Tx2 Plus
> controllers. Seems to be working on UP machines (heavy, heavy
> testing) and SMP (2 processor Athlon machine, fairly heavy testing).
>
> Lots of comments available in patch and header, please review and
> apply if you like it!
>
> This patch depends on patch 1 to apply, and patch 1 and 2 to compile and work.
>
> Luke Kosewski
>
>
> ------------------------------------------------------------------------
>
> 26.09.05 Luke Kosewski <[email protected]>
>
> * A patch to sata_promise.c (dependent on patches 1 and 2 in this
> series) which makes it use the hotswap API in patch 2. The Promise
> controllers are fairly simple in terms of their hotplug mechanism,
> so none of the funky 'janitor' functions are used here.
> ata_hotplug_plug is called on a plug event, and ata_hotplug_unplug on
> an unplug. Simple, simple.
> * Pending some confirmation and suggestions from Jim Ramsay
> ([email protected]) the interrupt handler might change to check for
> DMA commands completing as WELL as hotplug events in the same pass.
>
> diff -rpuN linux-2.6.14-rc1/drivers/scsi/sata_promise.c linux-2.6.14-rc1-new/drivers/scsi/sata_promise.c
> --- linux-2.6.14-rc1/drivers/scsi/sata_promise.c 2005-09-14 19:57:54.000000000 -0400
> +++ linux-2.6.14-rc1-new/drivers/scsi/sata_promise.c 2005-09-14 20:16:09.000000000 -0400
> @@ -332,10 +332,43 @@ 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);
> +}
Rather than two functions, I prefer one function that takes an 'enable'
boolean argument, just like pci_intx()
Also, the function names are way too long.
> static void pdc_sata_phy_reset(struct ata_port *ap)
> {
> pdc_reset_port(ap);
> - sata_phy_reset(ap);
> + 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);
I don't see the point of this. Might as well unconditionally disable
hotplug interrupts.
> }
>
> static void pdc_pata_phy_reset(struct ata_port *ap)
> @@ -485,11 +518,13 @@ static void pdc_irq_clear(struct ata_por
> static irqreturn_t pdc_interrupt (int irq, void *dev_instance, struct pt_regs *regs)
> {
> struct ata_host_set *host_set = dev_instance;
> + struct pdc_host_priv *hp = host_set->private_data;
> struct ata_port *ap;
> u32 mask = 0;
> unsigned int i, tmp;
> - unsigned int handled = 0;
> + unsigned int handled = 0, hotplug_offset = hp->hotplug_offset;
> void __iomem *mmio_base;
> + u8 plugdata, maskflags;
>
> VPRINTK("ENTER\n");
>
> @@ -513,7 +548,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);
> @@ -532,7 +567,36 @@ 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) {
> + writeb(plugdata, mmio_base + hotplug_offset);
> + for (i = 0; i < host_set->n_ports; ++i) {
> + ap = host_set->ports[i];
> + 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 */
> + ata_hotplug_unplug(ap);
> + handled = 1;
> + } else if ((plugdata >> 4) & 0x1) { //Check plug flag
> + /* Do stuff related to plugging in a device */
> + ata_hotplug_plug(ap);
> + handled = 1;
What happens if both bits are set? Seems like that could happen, if a
plug+unplug (cable blip?) occurs in rapid succession.
The rest seems OK to me.
Jeff
On 9/28/05, Jeff Garzik <[email protected]> wrote:
> > +/* Mask hotplug interrupts for one channel (ap) */
> > +static inline void pdc_disable_channel_hotplug_interrupts(struct ata_port *ap)
<snip>
> > +/* Clear and unmask hotplug interrupts for one channel (ap) */
> > +static inline void pdc_enable_channel_hotplug_interrupts(struct ata_port *ap)
> Rather than two functions, I prefer one function that takes an 'enable'
> boolean argument, just like pci_intx()
>
> Also, the function names are way too long.
OK, I'll change that.
> > - sata_phy_reset(ap);
> > + 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);
>
> I don't see the point of this. Might as well unconditionally disable
> hotplug interrupts.
Sure.
> > + if (plugdata) {
> > + writeb(plugdata, mmio_base + hotplug_offset);
> > + for (i = 0; i < host_set->n_ports; ++i) {
> > + ap = host_set->ports[i];
> > + 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 */
> > + ata_hotplug_unplug(ap);
> > + handled = 1;
> > + } else if ((plugdata >> 4) & 0x1) { //Check plug flag
> > + /* Do stuff related to plugging in a device */
> > + ata_hotplug_plug(ap);
> > + handled = 1;
>
> What happens if both bits are set? Seems like that could happen, if a
> plug+unplug (cable blip?) occurs in rapid succession.
What IF both bits are set? This is why we have a debounce timer to
take care of the problem :P
The way this is set up, unplugging will win out (plugging will come
first, unplugging will come and destroy 'plug's timer, and then the
unplug action will be performed on timer expiry). Personally, I like
it this way, but I can reverse the order of these two to make plugging
the default action. Which do you prefer?
> The rest seems OK to me.
Excellent. I'll repost when the issues you point out have been addressed.
Luke
Lukasz Kosewski wrote:
>>>+ if (plugdata) {
>>>+ writeb(plugdata, mmio_base + hotplug_offset);
>>>+ for (i = 0; i < host_set->n_ports; ++i) {
>>>+ ap = host_set->ports[i];
>>>+ 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 */
>>>+ ata_hotplug_unplug(ap);
>>>+ handled = 1;
>>>+ } else if ((plugdata >> 4) & 0x1) { //Check plug flag
>>>+ /* Do stuff related to plugging in a device */
>>>+ ata_hotplug_plug(ap);
>>>+ handled = 1;
>>
>>What happens if both bits are set? Seems like that could happen, if a
>>plug+unplug (cable blip?) occurs in rapid succession.
>
>
> What IF both bits are set? This is why we have a debounce timer to
> take care of the problem :P
>
> The way this is set up, unplugging will win out (plugging will come
> first, unplugging will come and destroy 'plug's timer, and then the
> unplug action will be performed on timer expiry). Personally, I like
> it this way, but I can reverse the order of these two to make plugging
> the default action. Which do you prefer?
The above logic
* acks multiple events
* handles only a single event
so either way you lose an event. In the code as it is written above, if
both 'plug' and 'unplug' events are noted, then only the unplug get
handled, and the newly-plugged device is never noticed.
Jeff
On 9/28/05, Jeff Garzik <[email protected]> wrote:
> Lukasz Kosewski wrote:
> >>>+ if (plugdata) {
> >>>+ writeb(plugdata, mmio_base + hotplug_offset);
> >>>+ for (i = 0; i < host_set->n_ports; ++i) {
> >>>+ ap = host_set->ports[i];
> >>>+ 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 */
> >>>+ ata_hotplug_unplug(ap);
> >>>+ handled = 1;
> >>>+ } else if ((plugdata >> 4) & 0x1) { //Check plug flag
> >>>+ /* Do stuff related to plugging in a device */
> >>>+ ata_hotplug_plug(ap);
> >>>+ handled = 1;
> >>
> >>What happens if both bits are set? Seems like that could happen, if a
> >>plug+unplug (cable blip?) occurs in rapid succession.
> >
> >
> > What IF both bits are set? This is why we have a debounce timer to
> > take care of the problem :P
> >
> > The way this is set up, unplugging will win out (plugging will come
> > first, unplugging will come and destroy 'plug's timer, and then the
> > unplug action will be performed on timer expiry). Personally, I like
> > it this way, but I can reverse the order of these two to make plugging
> > the default action. Which do you prefer?
>
> The above logic
> * acks multiple events
> * handles only a single event
>
> so either way you lose an event. In the code as it is written above, if
> both 'plug' and 'unplug' events are noted, then only the unplug get
> handled, and the newly-plugged device is never noticed.
Yeah, that's exactly what it does. I see your point, but no matter
how I design this (debounce timer, no debounce timer, whatever), in
such a situation we always have either:
- plug/unplug
or
- unplug/plug
One of them will always win.
I will change this so that 'plug' wins (as in, the unplug call will be
made first, the plug second). This way if we have both bits set, but
what we ACTUALLY wanted was an unplug:
- the unplug API interface ata_port_disables the ap in question. It
queues up and unplug event.
- the plug API interface deletes the queued plug event, and stores up
a plug on the timer.
- the plug timer event triggers, the first thing it does is execute
ata_scsi_handle_unplug, which the unplug handler would have done
anyways. It removes the associated struct scsi_device from existence.
Following this, it does a reset and attempts to detect a new device;
there is none (we actually unplugged it), so it just fails, no device
detected. So this case works.
If we have both bits set but wanted a a plug:
- the unplug API interface ata_port_disables the ap in question. It
queues up and unplug event.
- the plug API interface deletes the queued plug event, and stores up
a plug on the timer.
- the plug timer fires off and does what we want.
Is this acceptable? I'd love to make the hardware acknowledge only
one request and interrupt me again with another, but there is no way
to get it to do that; when you acknowledge a hotplug interrupt it
clears the register of all hotplug events whether you handled them
specifically or not.
Luke