Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
IronPort-SDR: nd4YVgZoFTdhUHMqpYZYWMmYZbv8TngCTTCd82N/krPNOw7LyG8062fsjfApNsg1FCf5/LOYJ1
 Pj5S0N2HKSBSKIHEd7ru6zCp7kcz8w1/ggiOAbveYpmyv1xijY/WBEGeSib7HhYn/jtYpipkvt
 jOGhTQeGxVFKiutcwRcrO+4nP9+RgYPfi2oml6XHxvemvE894Lg36SpSGNqE0C8xUrtjH4UUov
 Kae1U2j8Y9Ky0Bw8tUsq67sdEs4gP4teEfIY3bUzSOGtoE6w48YAloPkK2GQGch6TcMMfkZTAS
 DP4=
IronPort-SDR: DpAxVR7YgOd0a+g4nskdLq6HhySRFrt956Uwr67Oy0bzbsJSyeNeOK/WMht2tiD+MZXUbj7c52
 9M5vV1GzRq68r+Q8axCEhYel170thatCkGpEgjBLMKeSX9weV+5aN9o4azUWgIGNl3TT/yrBuJ
 JaTFxpDMVFwUyc9gZ1PayLrPivWWNNvVXqSiVr5gfStCWrs3s6QBpUf7UsYrfYeAWUU0NUL9Yo
 TJ9qhocNeKD7vxoUhZMv9I804qdCSJM0E9CFT7ebULdbDZiMuz2usBvciU/TlBj4U6/hT/eaUS
 9mY=
WDCIronportException: Internal
Message-ID: <4ff0ca00-31f5-2867-ff59-cecb5d6d1048@opensource.wdc.com>
Date:   Wed, 21 Dec 2022 12:59:54 +0900
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:102.0)
 Gecko/20100101 Thunderbird/102.6.0
Subject: Re: [PATCH] scsi: libsas: Grab the host lock in
 sas_ata_device_link_abort()
Content-Language: en-US
To:     Jason Yan <yanaijie@huawei.com>,
        John Garry <john.g.garry@oracle.com>,
        Xingui Yang <yangxingui@huawei.com>, jejb@linux.ibm.com,
        martin.petersen@oracle.com, niklas.cassel@wdc.com
Cc:     linux-scsi@vger.kernel.org, linux-kernel@vger.kernel.org,
        linuxarm@huawei.com, prime.zeng@hisilicon.com,
        kangfenglong@huawei.com
References: <20221220125349.45091-1-yangxingui@huawei.com>
 <4ec9dbed-1758-d6b4-dc1d-ac42e8c22731@oracle.com>
 <c8387766-2ca0-51f3-e332-71492b13e5c1@opensource.wdc.com>
 <7347d117-6e0b-dd18-90a8-25685f757689@huawei.com>
From:   Damien Le Moal <damien.lemoal@opensource.wdc.com>
Organization: Western Digital Research
In-Reply-To: <7347d117-6e0b-dd18-90a8-25685f757689@huawei.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 2022/12/21 11:42, Jason Yan wrote:
> On 2022/12/21 8:36, Damien Le Moal wrote:
>> On 2022/12/20 23:59, John Garry wrote:
>>> On 20/12/2022 12:53, Xingui Yang wrote:
>>>> Grab the host lock in sas_ata_device_link_abort() before calling
>>>
>>> This is should be the ata port lock, right? I know that the ata comments
>>> say differently.
>>>
>>>> ata_link_abort(), as the comment in ata_link_abort() mentions.
>>>>
>>>
>>> Can you please add a fixes tag?
>>>
>>>> Signed-off-by: Xingui Yang <yangxingui@huawei.com>
>>>
>>> Reviewed-by: John Garry <john.g.garry@oracle.com>
>>>
>>>> ---
>>>>    drivers/scsi/libsas/sas_ata.c | 3 +++
>>>>    1 file changed, 3 insertions(+)
>>>>
>>>> diff --git a/drivers/scsi/libsas/sas_ata.c b/drivers/scsi/libsas/sas_ata.c
>>>> index f7439bf9cdc6..4f2017b21e6d 100644
>>>> --- a/drivers/scsi/libsas/sas_ata.c
>>>> +++ b/drivers/scsi/libsas/sas_ata.c
>>>> @@ -889,7 +889,9 @@ void sas_ata_device_link_abort(struct domain_device *device, bool force_reset)
>>>>    {
>>>>    	struct ata_port *ap = device->sata_dev.ap;
>>>>    	struct ata_link *link = &ap->link;
>>>> +	unsigned long flags;
>>>>    
>>>> +	spin_lock_irqsave(ap->lock, flags);
>>>>    	device->sata_dev.fis[2] = ATA_ERR | ATA_DRDY; /* tf status */
>>>>    	device->sata_dev.fis[3] = ATA_ABORTED; /* tf error */
>>>>    
>>>> @@ -897,6 +899,7 @@ void sas_ata_device_link_abort(struct domain_device *device, bool force_reset)
>>>>    	if (force_reset)
>>>>    		link->eh_info.action |= ATA_EH_RESET;
>>>>    	ata_link_abort(link);
>>
>> Really need to add lockdep annotations in libata to avoid/catch such bugs...
>> Will work on that.
> 
> Actually in libata there are many places calling ata_link_abort() not 
> holding port lock. And some places are holding the real host 
> lock(ata_host->lock) while calling ata_link_abort(). So if you add the 
> lockdep annotations, there may be too many warnings. If these are real 
> issues, we should fix them first.

libata-EH does most of its work without the port lock held because by the time
we get EH started, we are guaranteed to be idle with no commands in flight. That
is why the calls you mention look like "bugs" but are not.

lockdep annotation will be a little tricky, but not too hard to do either.

> 
> Thanks,
> Jason

-- 
Damien Le Moal
Western Digital Research