Re: [PATCH] libsas: fix/amend device gone notification in sas_deform_port()

[Dan Williams <dan.j.williams@intel.com>]

On Fri, 2011-03-04 at 14:44 -0800, David Milburn wrote:
> Dan Williams wrote:
> >> Dan,
> >>
> >> So, you are seeing sas_ata_task_done() trying to get the ata_port lock,
> >> but it has already been taken earlier in the code path?
> >>
> >> ata_scsi_queuecmd (spin_lock_irqsave(ap->lock, irq_flags)
> >>     __ata_scsi_queuecmd
> >>       ata_scsi_translate
> >>         ata_qc_issue
> >>           sas_ata_qc_issue
> >>             isci_task_execute_task
> >>               isci_task_complete_for_upper_layer
> >>                 sas_ata_task_done
> >> (spin_lock_irqsave(dev->sata_dev.ap->lock, flags)
> > 
> > Exactly.
> > 
> > sas_device_gone() should prevent an lldd from ever attempting an i/o to 
> > a missing sata device (one that has been notified via lldd_dev_gone). 
> > Although, I have only had time to verify the simple unplug case and 
> > sync-cache commands at driver unload.  We'll still need to handle 
> > missing ssp devices, but there are no lldd-external locking concerns in 
> > that path.
> > 
> 
> Dan,
> 
> One other question, should sas_device_gone() be sync'ing with sata io
> thru ata port lock instead of the Scsi_Host lock?
> 
> Looking at sas_queuecommand, the host_lock is released and the ata
> port lock is taken before calling ata_sas_queuecmd, and ata port lock is 
> held
> down the __ata_scsi_queuecmd path.
> 
> Should sas_device_gone get the ap->lock from the domain_device->
> sata_dev to better sync against sata io?

Unless I am mistaken they are one in the same, see ata_sas_port_alloc:

struct ata_port *ata_sas_port_alloc(struct ata_host *host,
                                    struct ata_port_info *port_info,
                                    struct Scsi_Host *shost)
{
        struct ata_port *ap;

        ap = ata_port_alloc(host);
        if (!ap)
                return NULL;

        ap->port_no = 0;
        ap->lock = shost->host_lock;
        ap->pio_mask = port_info->pio_mask;
        ap->mwdma_mask = port_info->mwdma_mask;
        ap->udma_mask = port_info->udma_mask;
        ap->flags |= port_info->flags;
        ap->ops = port_info->port_ops;
        ap->cbl = ATA_CBL_SATA;

        return ap;
}

--
Dan