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
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
