Received-SPF: pass (google.com: domain of linux-kernel+bounces-94441-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Message-ID: <f8de67cb-8d0c-475d-b3f3-bc38ff097823@oracle.com>
Date: Wed, 6 Mar 2024 18:43:13 +0000
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v2] scsi: libsas: Fix disk not being scanned in after
 being removed
Content-Language: en-US
To: Xingui Yang <yangxingui@huawei.com>, yanaijie@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,
        chenxiang66@hisilicon.com, kangfenglong@huawei.com
References: <20240227090149.29039-1-yangxingui@huawei.com>
From: John Garry <john.g.garry@oracle.com>
Organization: Oracle Corporation
In-Reply-To: <20240227090149.29039-1-yangxingui@huawei.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: 
	=?utf-8?B?VzltWGxPVXNKWmNBVnZVZ2txOVJCSVhScGVudG1PbDJ5REY5WnJJbGVMazdC?=
 =?utf-8?B?WDIxejlIOVkwNVhVa1V3dTdjNlVKNHZjT21sRzhyU1R4TUF0MjZLVzNOY0hP?=
 =?utf-8?B?UEl0YTZ6aGdDMStJZjM4NmlZRmJDY0RldEdLTmh0YTBBNUg3YzF6K3czcTIv?=
 =?utf-8?B?SGJhL1prL1EyKzlTZXpJb3M3QTZtdTlaWWlGcXFEd0gvb0hxcTl3UEcrN0E0?=
 =?utf-8?B?OTNMQUVRWnRaRUUvNXFDN09MemRscldMZXpaWDY5T2Fmck45UUlwRXZTTkN4?=
 =?utf-8?B?dlhrWmx5bVRPdWJHN0VON3BvYU5lbzdDMzY5b2g0Yis0YjY1L0VFQjVzS3l4?=
 =?utf-8?B?aWR2c1FqdjVyMVV6cTRBbTRCMmQweDcxdERHSTkwWUtJSHdaNng4S0lLb08r?=
 =?utf-8?B?SkFHcGgxMmxicTRFcE1ac2pVWmF5YUswK2x5TGUvMUhUaGcwQTg5alprVFRJ?=
 =?utf-8?B?dExSTE1KRUN4bzJPeEk4VDIraXpvMlR5TGg3ZnBvYit4elFtbjEzeldFRm01?=
 =?utf-8?B?czNuRGpldXQwN1ZTdUxsZGZUTG1sOVZyRm11Sm5GN1lkS0ZPUGNhYWk3eFQv?=
 =?utf-8?B?L2VGTFZnak9DaEQ4RGhQaXJYRHB3RnFzMDR5eEZuOWl2SjkyV1NpSnJ0aUVh?=
 =?utf-8?B?WDg3aHZnT3VyWWc5bkhlaXRVaFJ4dFNpdGw5dFlLcnFmSDJTa1JqcEluRW12?=
 =?utf-8?B?RTZ6UjZtMW5qWkdRZktlbDA4TlliZ0UyZ0VReGlBU3pqS09Zb2RRRjhSVlpS?=
 =?utf-8?B?ZTE1QUxBcmIzNG9lN0MyVUNlRzlNS2tidnlwdUFlWTRyVWF1dnhtVjRLL1lO?=
 =?utf-8?B?S1piS0JFaVVldDFRcUR5N01LeWpreTRXdEROQzJ6MENML1M1MHhoV0NqZWQ4?=
 =?utf-8?B?c2VOSmNtcWRGWHVyTkYxSzBEaWxhdjY2QW9keUR5MVlSR0VyMFcxWlBaMFIv?=
 =?utf-8?B?VkIrenRjTUZMU29VWndjWVc0cEVXMlVTVnlrdXNubnYxcG5xOVprS29PUThx?=
 =?utf-8?B?SUVYZUd4YkhBY1FEdmxEZzJ6WnBFeU1GUkZTaXVmU2hDREJERzZ1OXlKZUJi?=
 =?utf-8?B?S0pLYTRvM1dUeVJSYkp5TEZWODZjUCtGOFo4Ujh3czFxcUhzZ0hrdmpNcWVI?=
 =?utf-8?B?U1VqTjU3azI1WGxFUEhBNmxxck5GQnV6K1VGRWU2UzdmNllMRUk2dmJrSzBm?=
 =?utf-8?B?OS9Gd1pLcWhBVXBWYmdIVmZ0akg0NEJDVUJVM2xrVnBESnBTUWJWcUR4NWlY?=
 =?utf-8?B?ak12Nm1tZncvYXhzL3FVWDdJWTJmTEdGazFSYXR1Qy9QUzFPRy9iMGpvTDNR?=
 =?utf-8?B?WGFQVG1wTG1LWE9RbU1Oc0gyZllrYmVCS1RwOXZ6dDNJVngrb29RcCs1eVQv?=
 =?utf-8?B?VWgwVTNwYTUzcG5BSWwveGdpcFMrVCtkWmxLcVBIQ0xjTnFMOHJoeDVJTHpl?=
 =?utf-8?B?aGFqWk9oUEJQSlBlRndraDBFTUt5MGZoSDJMdkEzcm96M2JZQzZka21FTW5T?=
 =?utf-8?B?ajNrVlcrcXQwamttWnVDa2g2QTBHSEQwY3V3OVRwcENNb3JBWlhWQ1RLWVkw?=
 =?utf-8?B?U0M2VFpyMTZNeEtsVlR4MVV4STlMMW00eVBDN0dBS2x6UTlDOHZ1a2laTEQx?=
 =?utf-8?B?UUVXQmZMWEtpdHVsSDM1RmV0ZTg1bmVIWVZwc2I5dHNNdTNRK2wwQzhMQWRP?=
 =?utf-8?B?WWtEZloyb29Ed3c1cEU0S1laYXhhcHpIeTJWQVVhSjdjR1ZqMUFHSXBwdXYr?=
 =?utf-8?B?UDV3WDVUNzJ1cGlPWHpJWXA2djFpU0kweWNMeUM1MHZyVFg0MHVPeHNHeXFR?=
 =?utf-8?B?aEpOSjNJOXY0N1FPNEhTS0l4Y2dnSTNZL05jWnJIUlJqYmVOaU5BbVFJaHk4?=
 =?utf-8?B?bXQ5N2QzYnJzTmJBSmZlRGdtYU8vdDVUUzNkTkZMbzQwbStEVWlTZG1tTnQ2?=
 =?utf-8?B?dlBTSG9BbVk3UWI3NmRuY2dkeDN1cWpVc25DdDNvbnBkQkg0bXJ1ZjVkVUNx?=
 =?utf-8?B?ZjNzNmFIb2FHM3NnQ1hTL2Yyc3FDY0xQM25NTzA0Yk5vejlaMjFhRUFvM0Jj?=
 =?utf-8?B?ZFZERmJEb0czQWltMElpNW44OEsxekc3WUduSWdqUmJnVkFUR1NpajVrczRZ?=
 =?utf-8?B?UXNuUlJxTjVONFlkVUMxYlpKOHoxRnY4SFFKMnIyRTYzK3FWRUNwZVFFVmlN?=
 =?utf-8?B?eGc9PQ==?=
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-0: 
	/BXNeVNbWDgEXisgABIAqcB4gT6BAbBJqht4sTQszqAHd8xn4+w0rpsdZcP5MiN+1zLEzqqI0wfUmXv298We5rHldTzqj1GQHXDqn37Smco0DEfw5O8+uAAmBaOabXHTkXuOESNBBNtOGjKJT1n+39Vvwu35IOszSuYND9ZdKeIODCtLPbVa10+avGh9ABcKGQxTWN5tl6hetIqQMrbAzda+vlqsjm9YrrdhaJVVIOcfJ91IW9+NNztQx56u+YpKxgOq3DpkSJP5NVHnz7/dwe08GiZ0TbnpYmHSGuMM5fGRCJjO+P/TasIaX18FilM8se+arHRpKvTD5PAYCQ/32Tvy0eGDZ9DycXMawCGUw7i3UxC3qnJpD20pEiwxpHBnqV1rudDdZ3WLR+KX4m2u46hXiuB9+ZsCyKEa+JQEoEwIUdOXNt9vvvcj6eZCkm5RjiPqbTrI2rfLzqzPj1XwpaaBvtMJzWRHAUUbGEwjFDGyXnDE7rUIwpZkTbg62C/WoD83QvKs8Iiz/X+/yz4sWqQ6s+74QiO89iDlYJSX+o5pt+bHB9HR8XfZHV8oLjMHFUAJwb0JoqRujnPXqKjGKRQwTIDNdB36yL3W9pqcZYo=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 9fa561a9-945c-4b52-bb2d-08dc3e0d49f5
X-MS-Exchange-CrossTenant-AuthSource: DM6PR10MB4313.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 06 Mar 2024 18:43:17.8055
 (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: lqghIt/fzK4x5Yx9oVI12pUJPchbqGe2SKlwWHRD2MS1VgIJAb/JQaZqKJPwYeqApApS/cH3e8kxylS7qvtPIw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CY5PR10MB6009
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.272,Aquarius:18.0.1011,Hydra:6.0.619,FMLib:17.11.176.26
 definitions=2024-03-06_12,2024-03-05_01,2023-05-22_02
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 adultscore=0 mlxlogscore=999
 spamscore=0 phishscore=0 mlxscore=0 malwarescore=0 bulkscore=0
 suspectscore=0 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2311290000 definitions=main-2403060152
X-Proofpoint-GUID: 5V1t9EcWxA0vVRUdsfqgy1zk1KZHEMnw
X-Proofpoint-ORIG-GUID: 5V1t9EcWxA0vVRUdsfqgy1zk1KZHEMnw

On 27/02/2024 09:01, Xingui Yang wrote:
> As of commit d8649fc1c5e4 ("scsi: libsas: Do discovery on empty PHY to
> update PHY info"), do discovery will send a new SMP_DISCOVER and update
> phy->phy_change_count. We found that if the disk is reconnected and phy
> change_count changes at this time, the disk scanning process will not be
> triggered.
> 
> So update the PHY info with the last query results.

Please mention that sas_unregister_devs_sas_addr() should be called 
before sas_set_ex_phy() for this case.

> 
> Fixes: d8649fc1c5e4 ("scsi: libsas: Do discovery on empty PHY to update PHY info")
> Signed-off-by: Xingui Yang <yangxingui@huawei.com>
> ---
> v1 -> v2:
> Use sas_get_phy_discover() instead of sas_get_phy_attached_dev() in
> sas_rediscover_dev() and use disc_resp to update phy info.
> ---
>   drivers/scsi/libsas/sas_expander.c | 37 ++++++++++++++++++++++--------
>   1 file changed, 28 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/scsi/libsas/sas_expander.c b/drivers/scsi/libsas/sas_expander.c
> index a2204674b680..a75dcce7a9ba 100644
> --- a/drivers/scsi/libsas/sas_expander.c
> +++ b/drivers/scsi/libsas/sas_expander.c
> @@ -1940,6 +1940,7 @@ static int sas_rediscover_dev(struct domain_device *dev, int phy_id,
>   	struct expander_device *ex = &dev->ex_dev;
>   	struct ex_phy *phy = &ex->ex_phy[phy_id];
>   	enum sas_device_type type = SAS_PHY_UNUSED;
> +	struct smp_disc_resp *disc_resp;
>   	u8 sas_addr[SAS_ADDR_SIZE];
>   	char msg[80] = "";
>   	int res;
> @@ -1951,33 +1952,47 @@ static int sas_rediscover_dev(struct domain_device *dev, int phy_id,
>   		 SAS_ADDR(dev->sas_addr), phy_id, msg);
>   
>   	memset(sas_addr, 0, SAS_ADDR_SIZE);
> -	res = sas_get_phy_attached_dev(dev, phy_id, sas_addr, &type);
> +	disc_resp = alloc_smp_resp(DISCOVER_RESP_SIZE);
> +	if (!disc_resp)
> +		return -ENOMEM;
> +
> +	res = sas_get_phy_discover(dev, phy_id, disc_resp);
>   	switch (res) {
>   	case SMP_RESP_NO_PHY:
>   		phy->phy_state = PHY_NOT_PRESENT;
>   		sas_unregister_devs_sas_addr(dev, phy_id, last);
> -		return res;
> +		goto out;
>   	case SMP_RESP_PHY_VACANT:
>   		phy->phy_state = PHY_VACANT;
>   		sas_unregister_devs_sas_addr(dev, phy_id, last);
> -		return res;
> +		goto out;
>   	case SMP_RESP_FUNC_ACC:
>   		break;
>   	case -ECOMM:
>   		break;
>   	default:
> -		return res;
> +		goto out;
> +	}
> +
> +	if (res == 0) {
> +		struct discover_resp *dr = &disc_resp->disc;
> +
> +		memcpy(sas_addr, dr->attached_sas_addr, SAS_ADDR_SIZE);
> +		type = to_dev_type(dr);
> +		if (type == 0)

Please use SAS_PHY_UNUSED


> +			memset(sas_addr, 0, SAS_ADDR_SIZE);
>   	}

Any chance we can factor out some of this code with 
sas_get_phy_attached_dev() (where it has been copied from)?

>   
>   	if ((SAS_ADDR(sas_addr) == 0) || (res == -ECOMM)) {
>   		phy->phy_state = PHY_EMPTY;
>   		sas_unregister_devs_sas_addr(dev, phy_id, last);
>   		/*
> -		 * Even though the PHY is empty, for convenience we discover
> -		 * the PHY to update the PHY info, like negotiated linkrate.
> +		 * Even though the PHY is empty, for convenience we update
> +		 * the PHY info, like negotiated linkrate.
>   		 */
> -		sas_ex_phy_discover(dev, phy_id);
> -		return res;
> +		if (res == 0)
> +			sas_set_ex_phy(dev, phy_id, disc_resp);
> +		goto out;
>   	} else if (SAS_ADDR(sas_addr) == SAS_ADDR(phy->attached_sas_addr) &&
>   		   dev_type_flutter(type, phy->attached_dev_type)) {
>   		struct domain_device *ata_dev = sas_ex_to_ata(dev, phy_id);
> @@ -1989,7 +2004,7 @@ static int sas_rediscover_dev(struct domain_device *dev, int phy_id,
>   			action = ", needs recovery";
>   		pr_debug("ex %016llx phy%02d broadcast flutter%s\n",
>   			 SAS_ADDR(dev->sas_addr), phy_id, action);
> -		return res;
> +		goto out;

is there any way to get rid of all the goto's? Ideally we would have 
just one location to call kfree(disc_resp)

>   	}
>   
>   	/* we always have to delete the old device when we went here */
> @@ -1998,7 +2013,11 @@ static int sas_rediscover_dev(struct domain_device *dev, int phy_id,
>   		SAS_ADDR(phy->attached_sas_addr));
>   	sas_unregister_devs_sas_addr(dev, phy_id, last);
>   
> +	kfree(disc_resp);
>   	return sas_discover_new(dev, phy_id);
> +out:

Calling this simply "out" is odd, as we exit in many places

> +	kfree(disc_resp);
> +	return res;
>   }
>   
>   /**


As an aside, could libsas - and your changes here - be simpler if we 
changed smp_execute_task() like this:

static int smp_execute_task(struct domain_device *dev, void *req /* can 
be on the stack */, int req_size,
			    void *resp /* can be on the stack */, int resp_size)
{
	struct scatterlist req_sg;
	struct scatterlist resp_sg;
	int ret;
	void *_req = kmemdup(req, req_size, GFP_KERNEL);
	void *_resp = alloc_smp_resp(resp_size);
	if (!_req || !resp)
		return -ENOMEM;

	sg_init_one(&req_sg, _req, req_size);
	sg_init_one(&resp_sg, _resp, resp_size);
	ret = smp_execute_task_sg(dev, &req_sg, &resp_sg);
	memcpy(resp, _resp, resp_size);
	kfree(_req);
	kfree(_resp);
	return ret;
}

We need to use alloc_smp_resp() and alloc_smp_req() as we can't allocate 
these memories on the stack for calling sg_init_one(). But if we changed 
smp_execute_task() to memcpy from/to data on the stack, it might make 
callers simpler. I'm not sure.

Thanks,
John