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: <b13c9445-39c5-f207-d5d0-d6c86eee54ae@oracle.com>
Date:   Fri, 5 May 2023 09:25:48 +0100
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Thunderbird/102.10.0
Subject: Re: [PATCH v2] ata: libata-scsi: Fix get identity data failed
Content-Language: en-US
To:     Damien Le Moal <dlemoal@kernel.org>,
        Xingui Yang <yangxingui@huawei.com>, jejb@linux.ibm.com,
        martin.petersen@oracle.com, damien.lemoal@opensource.wdc.com
Cc:     linux-scsi@vger.kernel.org, linux-kernel@vger.kernel.org,
        linuxarm@huawei.com, prime.zeng@hisilicon.com,
        kangfenglong@huawei.com
References: <20230505025712.19438-1-yangxingui@huawei.com>
 <291f1d97-9195-45ac-8e12-058f5c797277@kernel.org>
From:   John Garry <john.g.garry@oracle.com>
Organization: Oracle Corporation
In-Reply-To: <291f1d97-9195-45ac-8e12-058f5c797277@kernel.org>
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?c2gxUjJMZlF4ZjZnU2E2Y01jb2pta2J0ZmphWkJNSTZVSEFZOUhWdExNV0ZT?=
 =?utf-8?B?d1RxOHJheVQwRGdUVy9pZWtLR0RkMDdObGpaOTNuc29RMUFwa2V5Z2xZK1p5?=
 =?utf-8?B?MVhTQ3JqenVLekcrYVlCVE1wZkNGTlpVbnZ1Q25ya2lFK2ZCT1dwSjFwYkVZ?=
 =?utf-8?B?T3pINDVXdWcrc3hwT1I4VXBJSnpVUS9mcHNWWnFRNTU4QWlKb2xhRGF4TW9R?=
 =?utf-8?B?czgya1R2WWdVOXZLd3lsTlRvVGt4TmU0aWJQMWMwN1lJaWFoa3ZUMFlXQnUy?=
 =?utf-8?B?Z1FQY3Z2TWJIakdhcWgza2RSQjRBL0trSUFISENGVlFXcTYrbDVPV21SY08v?=
 =?utf-8?B?Q1FKN3JsVll1MXZmNjJEcm5oTTlySjBRRDZFU3dVbUVUa0N6cnJwbmJzWCtl?=
 =?utf-8?B?NUMyUkxXcmUvdzVQOFdJUWR0djByMUhIcWkwYVdtVUFXT3FmMjRPWDhDTHJs?=
 =?utf-8?B?RE14YlgwWjFuTUtjSkQrZ3BCMHdUa040RjlpUmtlVk45K1BwZlBJb29LYWpD?=
 =?utf-8?B?aDJDSTlJL2kyVWFLNGNZcUtsbmdCb3pZanB3cjNwRUFpVlV1MUpKTUtLVWlB?=
 =?utf-8?B?Qk0ya3dNNHFTSno5ZFNHTW5nS2Q2eS8zUkVpbXBpVlhmZWFpRXZ3WHAyK1A5?=
 =?utf-8?B?RXFnQVJvR3ZVMnVwcXNmcjl6Vms0VnZQc2xGQXNhaUZ0eW9IeVpjWmxDVGZz?=
 =?utf-8?B?YTBYNTRJbC9JOHpuTDVEQWgzOHdDVnV4bm13ejROMllOSXlYVDlRTGpQWnp4?=
 =?utf-8?B?NTJJaDZiZm9LS3BRKzdlNzlnbzdMNU1ndUdUR0ZjZTJNekNOUTNzUjEzWjZY?=
 =?utf-8?B?dGZlUkVmRWN0bGJQV2tGTDIrS2kvVjRhMHVxVzdJNTg1czlHRjFUYml3a3Qr?=
 =?utf-8?B?aGU5YkVQblI5RFVxbldWVjhjandFeDkzLzNLZGNnV2tPeE5UaGZPQTRxYVdZ?=
 =?utf-8?B?T2ZZeXZXdUh0bEJFZ3RlMHhKNkxnUlpPS21BNWp2RmdDMTBGRmVUcHNmTysy?=
 =?utf-8?B?aTcwVW9LNkV4bWJwNUh5bWRMS28yTFMzdDZIZzVCVi80Z0MvSktEVkNMWmg4?=
 =?utf-8?B?a2Rwd0pacHI4M0VPNm1hV3ZNSnFXUVFoUGN6M1FpMWtWQWY4eDlqUmpTQjVv?=
 =?utf-8?B?UFFLdndWK0FvUGlRWHJsZkFJVTN2U0JMcENFT3E1dDQ3SDNUeUNNQWRYUGhK?=
 =?utf-8?B?RjRHaUFpc2lEMFdTMFErekR0ektxUzBrcUQ3Q01CeDFhRHhPMEZEZFp5T0Nm?=
 =?utf-8?B?a3o3cjRuM0tPbVJMTUpHaGpUZWo2MVYxRHYybEcyRmczRG1ING9OVk9mWlhZ?=
 =?utf-8?B?NjFHN2w5OFp6Z2xWL0s4aHRKTGFPaklYWWE3U3FabGhzMUI2ckRNWjh3N0hy?=
 =?utf-8?B?YUwzZklkb3VjNVczc0NNU21JUTh3Y0MxTnpyckR1dkVuWHNUL2JSbXNQeEpX?=
 =?utf-8?B?eE9jRm9iRnQxWElzaTlnUWV1RXVEc0NnK2RiUkoxRDY5ZFRnb2UyN05GMXc4?=
 =?utf-8?B?UkJ0QjIrWmYwcEpKNzdQemp5dVI2SVlNSFJVdElOeTVoOHlDN1pIb2N2bEUz?=
 =?utf-8?B?K3cwWWdQUStvK0E1YWRaSlBBSEhsTU9RaW9oU0s3cUU4R3RBaFl0N1h5SHFF?=
 =?utf-8?B?dVpRZi93ckN2aysySnBRbmlpNklRejYvUkNoUkltcFBQS3o4RGJFUVd0bTJD?=
 =?utf-8?B?bVBwaG9JL0w1dVhGaStTZUVQZnl6RlN5TjFlR09QNCtmNTRYbk5rL2NjcW9V?=
 =?utf-8?B?djF2ZWhtZEVuKzZwbTdGVlpvSVFCQnB5NnBJRzBkWWFQSEdneU43WWZvRlYx?=
 =?utf-8?B?YU4zTmZFaUlTNU1BZjI0OEhidDhCb1VGNkN4MjcvWDNsWVJNQ3d2WDM1a2Jw?=
 =?utf-8?B?c1RaUGdHTERYVHdyWTJSSDlYcExzUHVsU0xhM0JaZXJabVVuWEI3Q2tmME9w?=
 =?utf-8?B?YjRLMlVLbXBadEVxSlhIWTgzVFlKQmZ4LzR4MlhWRzFVdTNrMVRLeUlTY2kx?=
 =?utf-8?B?cjk5Q0F6bmFFNk1rcmlyZjdXSmg0VXhIZWdtUndBeC8ydmNySWY5V3VUSFJC?=
 =?utf-8?B?cThNMTYyd0IzQ3cvQi92U2N2NmxEVW94dFBFdzQ0WEk4TTM1Y3FBNTQ2TUhC?=
 =?utf-8?Q?eOfxtAndFNZTvdBQ8pLk8tdjS?=
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-0: =?utf-8?B?bnZDWHBkOWlBdEk4ZnBSb2k5OUl2T0l0UnNYeVNYS1N0TW55ZVJnSURwb2JH?=
 =?utf-8?B?S0ozRWNvWmdFMXFzVnd1dHc5YWZlMDRua1ZreXB1WDZHYk1KWjd2WWZleUw3?=
 =?utf-8?B?a25hRW16TVJ4N28vaTBzNFpJUnpqZzZDaDlCMkpZSXlUOGxycElEVUUvbUZC?=
 =?utf-8?B?eURYQURjWWtMMVkzUGxSYU13cjlUNVdzNzBBTTdSYTlKY0NkQnZnQ1lRc2Ji?=
 =?utf-8?B?Vlc3NDl1dEtkL1VjSkFLaVlNVkZkOFNFWDY5V0Z5dFhxbjY5QVRkSklna01l?=
 =?utf-8?B?Ky82V1FyVkVJcVkzZ2c2SE8vVUFDMmttU2Rwa3lCWWJ5cTJQcmFKTEIydkVS?=
 =?utf-8?B?eU5ZUWhCZGlkK2k5MkIzSjN3blhXSFhVKzVYYWtsMDQ4SDBxa1NuVHZqRXls?=
 =?utf-8?B?VlI4R084Q2dBYmhlQUlsZXpZdllJZ0FFa3FST1AzK1dCUjZRSk9YTVZWbnA3?=
 =?utf-8?B?WGRWSkdrdE8zemp3TXNLY2szZStLcXlMQWhybTJOeXVQWHZHeVE3TDJ2emxW?=
 =?utf-8?B?M1IyWXh1b0dyUm9tZ1RjWU8wVS9na21qUFNjUHRibTVRMjFudVVBU0RnZE5L?=
 =?utf-8?B?QVdYRG11MGJpYjBoSkdsYndycEtsTTd3Qmg5MzgzS05uMnhZbVhTVUNERXVC?=
 =?utf-8?B?MWQzYkJ4T0VDQzgwY3FuWlJaWE43TW9FRVFTRGtuQ3p2NUtib1V0aTVkSkFT?=
 =?utf-8?B?dDA3RVppWHJMRlVGTXd4N1pQZEJRUUVnNDAvaG5Vd1g3Ty96MUxrNlQvK2pa?=
 =?utf-8?B?VmE0ZWJKRVZ3ampsbGFIOUF5cVZDV0Nvem93RmlwMVNaS0JVbFhNRzVWc3Rl?=
 =?utf-8?B?MmFabm4ycmJOckkzMVlOeVp1MERwbHRuSXF5TnZORUdWUkdmRDJ2KzV0KzIz?=
 =?utf-8?B?TldIa0JJWkdiMm9JdCtmcWtHRzYyNFFhajAwdjZNM0FpMTNDTUFDNVlRTlp5?=
 =?utf-8?B?L2oyUnVaS2NHcXlVWjliMVdLZjh4L1pZNEYyMGRESXp3TmJNY09ESGRkSllh?=
 =?utf-8?B?Z25kZXZ2QWhTSituMFJoYTFxNERxeTQwYzJhR05FRzZZcWxDR25FY2JJY2o2?=
 =?utf-8?B?VDRLOEhHZDNlemlRS1pOejluUVByKzNKc00xTGpPTWtrYW5jb2Y4cllsV0FK?=
 =?utf-8?B?WEpCT1JvdS9yem5JMDhlMlIvUFJhTUdJQS9Ga0cxWXRLOU94aHpsbklIMUdK?=
 =?utf-8?B?RW43SHQ0K2tMUFVRdVBLbmhDditua3hZM0FFMWY4S0NUS0l2MFZXYXNiaDQr?=
 =?utf-8?Q?dOXdbuxszrX8TgF?=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: f0e88806-fb63-4015-53a2-08db4d4256ba
X-MS-Exchange-CrossTenant-AuthSource: DM6PR10MB4313.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 05 May 2023 08:25:52.3199
 (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: LSzFb4LUAFS1dTWd3QEfOF0PiHfDE7oskLlGFeGMgfRJONZ7n0G1MDmua70NPKfADivnDjMqI21s33WGOd2MHA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BLAPR10MB5188
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.254,Aquarius:18.0.942,Hydra:6.0.573,FMLib:17.11.170.22
 definitions=2023-05-04_15,2023-05-04_01,2023-02-09_01
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 adultscore=0 phishscore=0 spamscore=0
 mlxlogscore=999 bulkscore=0 suspectscore=0 mlxscore=0 malwarescore=0
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2303200000
 definitions=main-2305050070
X-Proofpoint-GUID: Of7D4JrgPKLtwFV-OXhJGyStE7NLKqRJ
X-Proofpoint-ORIG-GUID: Of7D4JrgPKLtwFV-OXhJGyStE7NLKqRJ
X-Spam-Status: No, score=-7.1 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,T_SCC_BODY_TEXT_LINE
        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 05/05/2023 09:17, Damien Le Moal wrote:
>> --- a/drivers/ata/libata-scsi.c
>> +++ b/drivers/ata/libata-scsi.c
>> @@ -26,6 +26,7 @@
>>   #include <scsi/scsi_device.h>
>>   #include <scsi/scsi_tcq.h>
>>   #include <scsi/scsi_transport.h>
>> +#include <scsi/libsas.h>

hmmm... is it really acceptable that libata is referencing libsas? I 
didn't think that it would be. libsas uses libata, not the other way around.

>>   #include <linux/libata.h>
>>   #include <linux/hdreg.h>
>>   #include <linux/uaccess.h>
>> @@ -2745,10 +2746,17 @@ static struct ata_device *__ata_scsi_find_dev(struct ata_port *ap,
>>    *	Associated ATA device, or %NULL if not found.
>>    */
>>   struct ata_device *
>> -ata_scsi_find_dev(struct ata_port *ap, const struct scsi_device *scsidev)
> Why drop the const ?
> 
>> +ata_scsi_find_dev(struct ata_port *ap, struct scsi_device *scsidev)
>>   {
>> -	struct ata_device *dev = __ata_scsi_find_dev(ap, scsidev);
>> +	struct ata_device *dev;
>> +
>> +	if (ap->flags & ATA_FLAG_SAS_HOST) {

And this is SAS host. Not necessarily libsas (even though with ipr 
libata usage gone, it would be the only user).

>> +		struct domain_device *ddev = sdev_to_domain_dev(scsidev);
>> +
>> +		return sas_to_ata_dev(ddev);
> Do you really need the ddev variable ? Also, this really should be a libsas
> helper. I beleive this pattern is repeated in several places in libsas, so that
> would nicely clean things up.
>
Thanks,
John