On 7/26/05, Jeff Garzik <jgarzik@pobox.com> wrote:
> > +static void pdc2_phy_reset(struct ata_port *ap)
> > +{
> > +     /* As observed on the Promise SATAII150 Tx2 Plus/Tx4, giving the
> > +      * controller a hard reset triggers another hotplug interrupt.  So
> > +      * disable them for the hard reset, and re-enable afterwards.
> > +      *
> > +      * No PATA support here yet
> > +      */
> > +     if (ap->flags & ATA_FLAG_SATA_RESET && ap->flags & ATA_FLAG_SATA) {
> > +             pdc2_disable_channel_hotplug_interrupts(ap);
> > +             pdc_phy_reset(ap);
> > +             pdc2_enable_channel_hotplug_interrupts(ap);
> > +     } else
> > +             pdc_phy_reset(ap);
> > +}
> 
> Don't special case this.  disable/enable hotplug interrupts for all
> controllers.
> 

OK, I've done this in my new patches.

<snip>

> > +     if (mask == 0xffffffff)
> > +             goto try_hotplug;
> 
> you are confusing controller hotplug (mask == 0xffffffff) and device
> hotplug.
> 
> controller hotplug should already be handled correct, by testing
> 0xffffffff and supporting the PCI ->remove hook.
> 

Yes.  Done this as well.

<snip>

> > +     if (plugdata) {
> > +             writeb(plugdata, mmio_base + hotplug_regs);
> > +             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 & 0x01) {
> > +                             ata_hotplug_unplug(ap);
> > +                             handled = 1;
> > +                     } else if ((plugdata >> 4) & 0x01) {  //Check plug flag
> > +                             ata_hotplug_plug(ap);
> > +                             handled = 1;
> > +                     }
> > +                     plugdata >>= 1;
> 
> you probably need a debounce timer; see the SiI email I am about to send.

Keep in mind that I sent three patches:
- 01 - sata_promise properly supporting SATAII150 controllers
- 02 - libata hotswap infrastructure
- 03 - sata_promise supporting the infrastructure

I believe the debounce timer should really go into patch 02.  This is
actually something that's probably needed by every controller... if
you're just plugging drives in/out of a controller, what you're doing
is connecting metal pins, they can be imprecise and trigger a whole
host of interrupts.  Hence, I propose changing patch 02 (attached for
all of your convenience) to be as follows:
- on calling ata_hotplug_unplug:  if a debounce timer is running, stop
it.  Conditionally call ap->ops->unplug_janitor or something like
that, for cases (like the Sil driver, it would seem) that need to do
something special like hard-reset the channel.  Start a new
debounce_timer  (say, with an interval of 500ms, as suggested by the
Sil guys?
- on calling ata_hotplug_plug:  if a debounce_timer is running, stop
it.  Conditionally call ap->ops->plug_janitor, if any driver needs
anything special done on a plug action.  Start a new one.
- when the debounce_timer goes off, the first thing it does is call
ap->ops->hotplug_action, which returns whether it should be plugging
or unplugging.  On a plug, do an ata_bus_probe, an an ata_scsi_plug on
success, ata_scsi_unplug on failure.  On an unplug event,
ata_port_disable and then call ata_scsi_unplug (kill any outstanding
taskfiles).

This means that patch 02 will no longer have a workqueue, but a timer.

Any suggestions/comments on this approach?

Luke Kosewski