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;
Message-ID: <12cf34d6-8267-ac81-02c4-190bb9afc50b@oracle.com>
Date:   Tue, 20 Dec 2022 08:43:31 +0000
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Thunderbird/102.6.0
Subject: Re: [PATCH V2] scsi: libsas: Directly kick-off EH when ATA device
 fell off
To:     Damien Le Moal <damien.lemoal@opensource.wdc.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>
 <4489f80f-9f39-8f3f-5d10-6b113131e65c@opensource.wdc.com>
Content-Language: en-US
From:   John Garry <john.g.garry@oracle.com>
Organization: Oracle Corporation
In-Reply-To: <4489f80f-9f39-8f3f-5d10-6b113131e65c@opensource.wdc.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?aG1lSjcxRDNRcUZXazlFMGJSZW1uOTBNTWVkUTRpbmROSDRuSWc4Q1hieG1V?=
 =?utf-8?B?UkZrcHhpZi9iNVA0aGs0L0Y2RE5LKzFKaVo1dzZ4dmFsc2VrczRlL2dqanRj?=
 =?utf-8?B?SjA2TFpDa3J4THNva29MNnk3Y3NkdFBBT3A4RGZ1cDd6QXlHT1c2TmNPVElH?=
 =?utf-8?B?TnNsUXJoaVhSdFkvcWtSalZOWW5kSFdqODJoaFBCYkxta0dybUlPYjlXMDBv?=
 =?utf-8?B?MXhJR3NoUHFVcW5PaFMxWE5xOE5sdFZoZS9WTHNDaXZPVFpLQU51clZMTG1i?=
 =?utf-8?B?UTJIL29XTk4wQngrLyttQmVLbkxmd3lFMVBIQTl1NVUxRHV2dDdtdzJKVUkv?=
 =?utf-8?B?cDhFcjVTYmovQmgxNW1neUtFdGZqQzkwWUptL3gwcVA3Uzl2Ti9Tc3VzL2or?=
 =?utf-8?B?UkJnSytCNVVZWUsybEVGWnFiTm9OUU4zcW4wbTN4MElFMzNCbFBOdUQ4cnRy?=
 =?utf-8?B?b1B2bDNQL3prN3NwR2FXajRTNlQ4NFlzUkNRb0dBU21xR1BmMEFhWS9qQ015?=
 =?utf-8?B?Y1ZoNy9Oekx2WEFCKzYzUjhyUWlBd2g0REMwNTFlZUlRVGNONDRUTjhROFVP?=
 =?utf-8?B?NVM0WWt4TzBSc1BrenRNeXRuNnVMUEZlSE9kREJBandsd015YUd6bEJmdGl4?=
 =?utf-8?B?ak9kclc4T1dkdnAvWVFGYUpBMHFKUC9RcFlNdlVib29oMytHSUpNaFBVSFUz?=
 =?utf-8?B?YjU0NjJRY1RheTRRS3VtZ3BETU5JRGxybnFHdkduZVdHNlBXQlcybDZzT1g2?=
 =?utf-8?B?VjU2MTdzWEJ4VGd1d255N1d2NWJmMWFTV21ibzQzWGxONUhNRTN2dTg5Wm5V?=
 =?utf-8?B?VDFmc083VVNjRFVGc2t0cktBTnR2ZGVmeXduQVJ2Ui9uaDFmc3BORjlzTHQ5?=
 =?utf-8?B?WDY1dU1TU3J0bWZuSHlBNTRLOFVzK0xKcllpc0RwNGh1TU1xUzR2ZHZ6dmNi?=
 =?utf-8?B?a2wra0kvMTBka3FpdktzUHh0Nlg3VHB6dzdzbDVLc3JVblh2UTRUVnE3cW9h?=
 =?utf-8?B?ZkYxM3paM1IwM0xVYTZzUjBRM1F4NjVKSDVlV3pTbE9EWTBqZGdPOWRJWStN?=
 =?utf-8?B?QmN1S2F4RnloOVlHRkJtalUvVE9aTC9DN3dYSW9nMGc4SWYzRHY1Z0xnMWhK?=
 =?utf-8?B?MXNHempQMFZpL3JkSkVjN0FBVWZKSTZwYjMrZ2tpNWF0UDVwMVRhTG9keUsx?=
 =?utf-8?B?alpTWUovNnBXekwyUmVwblhjaUpkMVJpWE11bUVZRXZ1WjVENG80ZUp6WUFz?=
 =?utf-8?B?K0h4TnU4Q3NWaDRlbDVSQkVlalhIaVVlQ0RLMmNSZEZmdmVMMUFxNXlaVW9q?=
 =?utf-8?B?TEhNaVBjNHFyNmE1MUdGUTRZMWhWQjRwWE85MkJMRHRta1BwUmVYTGRHU2dC?=
 =?utf-8?B?c3pmQU1sK2xoYzA4aExTOVlqVVpqMHJaSm1pUFZRUVhRQnIwY3dub0dhcmJQ?=
 =?utf-8?B?c3EwZWNqQnBjRWdwV0JzUDJKV2c0T0dIdzlCU0I2MHp4SEFiQ0xPMHFoVmJa?=
 =?utf-8?B?OGlzQ3Bic3BGYm5DV1lhbWhIRDRjRFM2VHhOMXVPTEFIT2QyWjNwb3E4L0x5?=
 =?utf-8?B?TGZ3WjhnZFN4RUFXQXFNZWpHL1p6aFBvM3pHWWpyb2hrbVZDdkVURzN0Unp1?=
 =?utf-8?B?Nk9RL3VBN3JZSGE4dStQUTJJZHBoMG81cXhBYVFCdGZxN1NlL0psNUdZUFNj?=
 =?utf-8?B?NWVnUjFkRW1qMFhQVGJHOUVKSGZQWXNFYXY4Z3NXZDJwMHBjb095Qk1jNzdP?=
 =?utf-8?B?aGZaYW42ajhxRldKNmpoN3UraXBiM3dqVEtma1Era2NzR0hhSEd0KzdtNG1D?=
 =?utf-8?B?cURBNTlJSUZDTENiYmliMThLSi9KYnlONENwYnhtaVhRd2JmUUVlTmZvWVJF?=
 =?utf-8?B?Sndncmg3QklZU0I5UlJ5bDYrNWVhaHRxUnQ2STZNZUJtb2hmTmVlaFNZeURt?=
 =?utf-8?B?dTlDaWxLNGk2SUZWQUp0U0JNN2UrREFGeWg0MWdLblRwNzl6VmoxWlQ5d3gy?=
 =?utf-8?B?dzlGVVdNMDFnYVllSEpROWVhdWdqYzZaUDkwYXhFUDNiOWdGUHVScGZOUzFz?=
 =?utf-8?B?NGJIRTVkRU9NbG1qQkk5dTk5UGZTakhITVY2c3Nkc2FzcWs1ekt6TE5wUzNv?=
 =?utf-8?B?L2tZQkd6UGhFS0pncXFaNVZxYjFqaXBrSGI5WHFTcmtjOWs1WE1Fam9yRnJy?=
 =?utf-8?B?OGc9PQ==?=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 52baf503-e775-47fc-e82d-08dae26647ef
X-MS-Exchange-CrossTenant-AuthSource: DM6PR10MB4313.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 20 Dec 2022 08:43:34.9823
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 4e2c6054-71cb-48f1-bd6c-3a9705aca71b
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: O0BZFHqTSlVyxpBLPkWSEwXkkOftJo30Vit0Psi20du3mn+Rginw4Om8NllVn/qr606GS+MNJVlq6mMLI303Qw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MN6PR10MB7542
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.205,Aquarius:18.0.923,Hydra:6.0.545,FMLib:17.11.122.1
 definitions=2022-12-20_02,2022-12-15_02,2022-06-22_01
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 spamscore=0 adultscore=0 phishscore=0
 mlxlogscore=999 bulkscore=0 malwarescore=0 suspectscore=0 mlxscore=0
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2212070000
 definitions=main-2212200071
X-Proofpoint-ORIG-GUID: aCAz3QWqSyXZqGoc_zZCJ4pCdMAfiHrQ
X-Proofpoint-GUID: aCAz3QWqSyXZqGoc_zZCJ4pCdMAfiHrQ
X-Spam-Status: No, score=-3.9 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,NICE_REPLY_A,RCVD_IN_DNSWL_LOW,
        RCVD_IN_MSPIKE_H2,SPF_HELO_NONE,SPF_NONE autolearn=ham
        autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On 19/12/2022 23:00, Damien Le Moal wrote:
>> But it is expected that ata_qc_issue() should be called with that the
>> host lock grabbed (and keep it).
>>
>> I think that the reason libsas drops the lock is because some LLDD
>> queuecommand CBs calls task_done() in some error paths. If we kept the
>> lock held, then we could have a deadlock, for example:
>>
>> sas_ata_qc_issue (has lock) -> lldd_execute_task() =
>> pm8001_queue_command() -> task_done() = sas_ata_task_done() -> grab host
>> lock => deadlock.
> That should be easily solvable using a workqueue for doing task_done(), no ?
> 

I don't see why we cannot just return an error code directly from the 
lldd_execute_task CB always - we end up calling scsi_done() directly 
then. But I am suspicious why it is not already done this way.

Looking at the code history, this fiddling with the ap->lock actually 
looks related to commit 312d3e56119a4bc5c36a96818f87f650c069ddc2 
("[SCSI] libsas: remove ata_port.lock management duties from lldds"). I 
will check that further.

Thanks,
John