lkml.org 
[lkml]   [2005]   [Sep]   [28]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH 3/3] Add disk hotswap support to libata RESEND #5
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 <lkosewsk@nit.ca>
>
> * 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
> (jim.ramsay@gmail.com) 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


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

\
 
 \ /
  Last update: 2005-09-28 21:15    [W:0.050 / U:0.184 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site