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: <43d5ba4a-efc7-09ae-74dc-81b19f635a19@oracle.com>
Date:   Mon, 22 May 2023 12:28:09 +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
To:     Jason Yan <yanaijie@huawei.com>,
        Damien Le Moal <dlemoal@kernel.org>,
        yangxingui <yangxingui@huawei.com>, jejb@linux.ibm.com,
        martin.petersen@oracle.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>
 <02d36ee9-cdad-454d-d822-95442d7bd67b@huawei.com>
 <f4ba7a92-1f00-c254-d196-7d21fe14dee2@kernel.org>
 <938d6b5b-0271-977d-f046-5fd70d29b3ca@huawei.com>
 <a5c2e157-aaf7-1300-3fbb-1300ac216cee@kernel.org>
 <68953040-1622-254b-f6f8-b279eccacfb1@kernel.org>
 <c88dcbc3-d530-3e9e-f674-a2fe64ad5fdc@huawei.com>
Content-Language: en-US
From:   John Garry <john.g.garry@oracle.com>
Organization: Oracle Corporation
In-Reply-To: <c88dcbc3-d530-3e9e-f674-a2fe64ad5fdc@huawei.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?QjhoVHFIQ2ltakV6a1Q4VkJURkJSTS9LRTBFcEdOampOWGlVR3lWazRBc3g2?=
 =?utf-8?B?c1JRdlRFOEpUNGR4aHV2cjkvSXhKemh6MDd4ZjF5VVIyZElqSDExVHNOZzhF?=
 =?utf-8?B?enJkaUNGbTFKbG51Vk1COUZ2RmJybGg0ZmhVeGtic2tlWFhETjJhaHJ6emY0?=
 =?utf-8?B?eERmazhHandEV0huNjZIZEgyT3dGWVJNeWV5VThObi9SVXE4WEc1VyswRExo?=
 =?utf-8?B?NFR6ZGlncUphdVp1L3ZLSVlXU1ZuZHAveVFrL21VRGYvb1RFZ1Y3aEk5UjJH?=
 =?utf-8?B?SnRhVDVwYkJyaU5aNzcvK3VUb25rVFI3TU5Ma0NmY050dFM2NnMvQjd6a3FI?=
 =?utf-8?B?ZXRsamo3U3Rqd1BBNGJmQW9hWVlJMTFlamJ0R3lOWjJaL0RydjZXcXo4ZlJ5?=
 =?utf-8?B?UDFqZERORVJ1RE1YcG5nZ3hEY3czRFFOUU9KbHZITlA0UWtjQ1d6TmRXdmFm?=
 =?utf-8?B?dm85c1lZQVQ3NE1tLzB5Z0V1UmNNKzU2L05YWHJoQnFIRFhTQUUrQlk0Rm5r?=
 =?utf-8?B?K3RxSkZIZnFlRDVRZkY2V1FROXRISnRWVnpUMVNYRE02eTRHc0JHNEc2aUVL?=
 =?utf-8?B?cUw3NHp6VnUxdE54L0tuMWJIbCtidVBka3dsN0JUclRGNzdBdTF3RDB4a1ZE?=
 =?utf-8?B?Q0RSV0YvaHRkd1ZNb1RmWkxQbHkxYTVGZW1NVUpkaE1WOThLbGlVSEQrVHFl?=
 =?utf-8?B?MEJJSnhQVU1FTmR3OW45WmVlTUZxcjdNcTgvWjhQeWoxZDJWZWwzd3k1WUZv?=
 =?utf-8?B?RVJKdjduWEFQQTlSWWFmSExacm1vWnlQNmg1SVlGMUlaeHFsWnJTWkh4VUR5?=
 =?utf-8?B?ZFRZTzlUbGdDd2dXbmdaV2RCOHozL1hKbjFkYnZrU1dxeWo3SFhVSHJ3b1BO?=
 =?utf-8?B?aGtqTzdXVUVHc2JEdWQ5eGV5RnBnSWlvOVhwTDVhQkx6V21Na08xRmUxY05J?=
 =?utf-8?B?Vkdrd3RxaDhQK2RHN0FQcWI0ZzlGWnhrLzZ4OVd4R0RaZzNNZFRPd0k5aU10?=
 =?utf-8?B?bCs2QlBybXkyR0dObGxBcVBLTnd2bk1iRHQrNHFzNUFNVFdwQXVheU1tZlU4?=
 =?utf-8?B?UXFtbGovMzdWWWJVcThuaGZwUzd4VzZ5QWNGZkdIcUo1eTZ1MnVoUjNEd2M4?=
 =?utf-8?B?S1AzdjlyUXhudm1YYmVBUS8rMHlTUXJBNzNDd2s4dGUyK3lhVEQwcGFJZHRm?=
 =?utf-8?B?M0ZFVEF2dU53SW9hMmtxYTB0am83WUVvYnl4VktTcGJxYWpaNzQ1WkJaQ01U?=
 =?utf-8?B?VWNDQ1dPcjNOZ2lVL3NSU1I3ZzJUN0RrNmlGMkRFMHZEaGdMMGVLR0Y3Y3Jm?=
 =?utf-8?B?NlFzWFFxU3c0dE9qdDI5TXRRUkRhcXkvRERWM0FCVElUTExleHZnSk90RHc3?=
 =?utf-8?B?SHErYzVMeFA1K05yWVJyUXJ3YVFjdWJ6Qm5pTVpJKzV1OCtmekRDL0R5UGky?=
 =?utf-8?B?ZlNIYXVFdllaWkN0dWptM1dyblR2R2NjaFZhVTlobWV3QnBMb3VpTzlWdXZK?=
 =?utf-8?B?VzdHNGg0QjhIVGxnOUdLbXp5MGxFYlNPZVdXbTZHWnd4WHpWRXZCTkc4UUI5?=
 =?utf-8?B?dWpTNnkvRExhREYwVTdHRU5HUnpvZ3IvZ01TRW40M1p3Qi9qOEZjTVQ1VUQz?=
 =?utf-8?B?ckdGcnhEKzhLRVlocFA0eXBjREdYQmQ5UnA2U1RQSlQwcjdGT2RHdkU4MUFC?=
 =?utf-8?B?ZmdzSEQ4YndLSzY3MFJ3N2VQT0VsbDFyNEdHaTVyTDByNGpYd294NTI0OXo5?=
 =?utf-8?B?VXhxMG5jOUZLaHZMVlQ5bURPRjFTWlAwZ3VXcnp5dnZSR29SeHY3eGRyMmx1?=
 =?utf-8?B?N3RxdVluYk5ienRzQk9XNy9zOHM1MkJxTzdpNVJYeWFoL1ltaC9YMXEyN2hJ?=
 =?utf-8?B?ekp3UmtPT3pveUlqTGRXd3dTblpLaUlZcTRzSE5xT0ZsQlZneWFKbjVaSEYv?=
 =?utf-8?B?bnJLemZ1NU5mcGhSYUNtMFV0QnlBVEZPdHdIZm9CS2Z1NlduTHMzTVV5dnR6?=
 =?utf-8?B?YWZOR3JjbmgyWFN2YUkrWGI2anhDQVBML2xaRUdGSXZTUG9raFNrTmtiUHRL?=
 =?utf-8?B?Nk5Ud04vQmNZUkI2NElrbzlHa2VTYm4rMzlOL01nQlgvaEhONDJ4Vm5BSGZL?=
 =?utf-8?Q?h1vm1WjJF3scOyAqrBSVf4j2z?=
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-0: 2GNq86UVEcWunqLJKiV7zS9nvAsqCsZnXx+9hCVP4brNfo/aapYNWLtVjraTK0yHGb722NDvxB4MLQuBWvN4Z1kRTjzA/3Ck01oFqgnZ2yCyl6Gabq7dfWtkNp/MavhAYOAx9FyHkX5k0kTmzCc7a2tyr3lqZ53S9M4JMcg5+GH+F8KirU/iqeZ1F4BUUhqQT6d9gDYUXrGmGGKqQCdaCarFZEjbeMez8PckIGVLQttNcNqB31C4NuFOrpPIG4XTkCjaoI39EwhfpsUJNZgI2/uaSSrDffsDYBcZZQqb/ISM3h/PyXgtgtb++9zXWiRxvRg2eejzn2F05CvWJtAttR7AWeRrBNU0MmbXO7+Mo8OAg6WU0UrPvjbJngk5VgMtOGJg+8l8zPhTDndZO++8veKtTBfwrrWKsL/yz+h65GPTpunY+piooL8Ta9zuitFm6s57VJy1y2vJOW4535NaBHsEIvFHXj/uMgVOt4W0KXXT7nrmUxrMAecFf8V20PQLxRqnQXpK0I+5mKFcnc67s60aVz9Ff5AiNnG7t062HV20ToCAN55lGI6b5RycYteUVOAq3uSC/9UT3sOxQFjz89atq27yJPpZ5e/pPIt/hVwPiQ5+vEQ4ke1h3F4IgNpMI5fJkm2v8NW4+RNoJNdGBS+ykG1jSL0MnalEu5l55tZetLtYIKdPPx07H2tqXJI/4J90NGVh7ZI9Z7YrPJG5cbJw/MOYnqVzqUY+NL4/7gS99MecFVZD/NpSapKM1ktCzdhIaZQdoFBf4gKHn7sBn/cEFfe4WtwyjGRlFEgcqwbbA89i41msK8WIxWX2UBhpIAog/If8PsO5LuUAdENgFikA4Y50sq94kSPqxO4163o6DYHCNIbWE224ML2Khfgpo5LrAO5g8V7sviPGyt/vnw==
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: ba8520a3-aaef-42a0-3b91-08db5ab7a200
X-MS-Exchange-CrossTenant-AuthSource: DM6PR10MB4313.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 22 May 2023 11:28:14.8974
 (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: cauGcG1Zv7P6uOtKRgwRH/72L1dWhmozybF3TAquCaWw4qooZL3zSiJgeULly3vqdmVtXnocPO76sh5w7WXl1g==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BN0PR10MB5013
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.254,Aquarius:18.0.957,Hydra:6.0.573,FMLib:17.11.170.22
 definitions=2023-05-22_07,2023-05-22_03,2023-02-09_01
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 adultscore=0 phishscore=0 mlxscore=0
 mlxlogscore=999 bulkscore=0 malwarescore=0 suspectscore=0 spamscore=0
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2304280000
 definitions=main-2305220097
X-Proofpoint-GUID: vjyzvGlB4Lj3o86hWu3l0keafvGM9dx0
X-Proofpoint-ORIG-GUID: vjyzvGlB4Lj3o86hWu3l0keafvGM9dx0
X-Spam-Status: No, score=-2.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,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 22/05/2023 09:00, Jason Yan wrote:
> 
> OK, so the issue is that __ata_scsi_find_dev() calls ata_find_dev() with 
> devno
> == scsidev->id. This leads to devno being 0, 1, 2 and 3 for connected 
> drives

This numbering comes from sas_rphy_add():
...
if (identify->device_type == SAS_END_DEVICE &&
     (identify->target_port_protocols &
      (SAS_PROTOCOL_SSP | SAS_PROTOCOL_STP | SAS_PROTOCOL_SATA)))
	rphy->scsi_target_id = sas_host->next_target_id++;

..

	scsi_scan_target(&rphy->dev, 0, rphy->scsi_target_id, lun,
SCSI_SCAN_INITIAL);
}

So libata and scsi_transport_sas just use different sdev id numbering 
schemes for host scan.

> sdd, sd1, sdf and sdg, as shown by lsscsi. However, each drive has its own
> port+link, with the link for each one having  ata_link_max_devices() == 
> 1, so
> ata_find_dev() works only for the first drive with scsidev->id == 0 and 
> fails
> for the others. A naive fix would be this:
> 
> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
> index 7bb12deab70c..e4d6f17d7ccc 100644
> --- a/drivers/ata/libata-scsi.c
> +++ b/drivers/ata/libata-scsi.c
> @@ -2718,7 +2718,7 @@ static struct ata_device *__ata_scsi_find_dev(struct
> ata_port *ap,
>           if (!sata_pmp_attached(ap)) {
>                   if (unlikely(scsidev->channel || scsidev->lun))
>                           return NULL;
> -               devno = scsidev->id;
> +               devno = 0;
Would this pattern work:

ata_for_each_dev(ata_dev, link, ALL) {
	if (ata_dev->sdev == sdev)
		return ata_dev;
}

If not, I think it's ok to have devno = 0 assignment under SAS_HOST 
flag, even though it's far from ideal. Not both of these are not 
preferred, then, as I mentioned before, some per-port callback to do the 
conversion.

Thanks,
John