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

[David Milburn <dmilburn@redhat.com>]

Dan Williams wrote:
> 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;
> }

Your right, thanks.

David

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