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: aDgS5hJdo+GCauJrnoG/6dc5v9y9ANaRnR3FIfzFKfqZ3dUCWfiTgoBwRQ84fqc3l9Ix86Yrnj
 4q1raowBcCiPo6zf+Wi6My6HZVpCURGDBvzRUMy8Lt5PmqGj9GWqFEoVhhJ7tITomxC2SYN/m7
 x99Uv79P5z8VMRhgs/M3TWYDuP2qFTsCEZ+c0U4wwKD9QCa3lBhuQDlN1eFepFjJHfF8T9Lc02
 ECSX9SzVC0gunVOleEwy7IwUGdI3OBHqI7udhMkZrNb6X/1xciXD9JNgjDAmeRssQJo71ZA2i+
 bg4=
IronPort-SDR: XsLIFWD0cT0HQcxLMrCvLZqT0Z62G31AtSYmatd4vIEB2Dg/gmti1Oe/krQz+Y+suGgyJW5qAO
 UvksgssOcOqp1Hg76tETXyI+xFW6FkAHvJj0SdmQ1OLDxj7VuBiaT+kCw7LcLkeUMYEgEXrL51
 sS04LdXXE415yt1X8GPAqDWsa5k/8q6/zClDHNdKDrQ/Dzg9X07khelxgm5m32f3IrGVlpadO0
 WLFh5ofwXVs5ssTa+udgiYHGxcXCb5MXVGHYEG+WchzovtilCjK9zsVaqeTcOyE8PuLcnARauR
 //I=
WDCIronportException: Internal
Message-ID: <4489f80f-9f39-8f3f-5d10-6b113131e65c@opensource.wdc.com>
Date:   Tue, 20 Dec 2022 08:00:43 +0900
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.5.1
Subject: Re: [PATCH V2] scsi: libsas: Directly kick-off EH when ATA device
 fell off
Content-Language: en-US
To:     John Garry <john.g.garry@oracle.com>,
        Jason Yan <yanaijie@huawei.com>,
        Xingui Yang <yangxingui@huawei.com>, jejb@linux.ibm.com,
        martin.petersen@oracle.com, linux-ide@vger.kernel.org,
        hare@suse.com, hch@lst.de
Cc:     linux-scsi@vger.kernel.org, linux-kernel@vger.kernel.org,
        linuxarm@huawei.com, prime.zeng@hisilicon.com,
        kangfenglong@huawei.com
References: <20221216100327.7386-1-yangxingui@huawei.com>
 <565fcf28-ec53-8d74-00a3-94be8e5b60e4@oracle.com>
 <60ace19e-d029-f14d-9aac-d5cef83b5b64@huawei.com>
 <257dcd6c-2ffd-f518-9b13-c309348088d9@oracle.com>
From:   Damien Le Moal <damien.lemoal@opensource.wdc.com>
Organization: Western Digital Research
In-Reply-To: <257dcd6c-2ffd-f518-9b13-c309348088d9@oracle.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On 12/20/22 00:55, John Garry wrote:
> On 19/12/2022 15:28, Jason Yan wrote:
>>>> +=C2=A0=C2=A0=C2=A0 if (test_bit(SAS_DEV_GONE, &dev->state) && dev_i=
s_sata(dev))
>>>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 sas_ata_device_link_abor=
t(dev, false);
>>>
>>> Firstly, I think that there is a bug in sas_ata_device_link_abort() -=
>=20
>>> ata_link_abort() code in that the host lock in not grabbed, as the=20
>>> comment in ata_port_abort() mentions. Having said that, libsas had=20
>>> already some dodgy host locking usage - specifically dropping the loc=
k=20
>>> for the queuing path (that's something else to be fixed up ... I thin=
k=20
>>
>> Taking big locks in queuing path is not a good idea. This will bring=20
>> down performance.
>=20
> But it is expected that ata_qc_issue() should be called with that the=20
> host lock grabbed (and keep it).
>=20
> I think that the reason libsas drops the lock is because some LLDD=20
> queuecommand CBs calls task_done() in some error paths. If we kept the=20
> lock held, then we could have a deadlock, for example:
>=20
> sas_ata_qc_issue (has lock) -> lldd_execute_task() =3D=20
> pm8001_queue_command() -> task_done() =3D sas_ata_task_done() -> grab h=
ost=20
> lock =3D> deadlock.

That should be easily solvable using a workqueue for doing task_done(), n=
o ?

>=20
> Thanks,
> John

--=20
Damien Le Moal
Western Digital Research