Received-SPF: pass (google.com: domain of linux-kernel+bounces-85513-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Message-ID: <f095aa1c-f233-40f9-ad0f-fcd8fe69a80d@oracle.com>
Date: Wed, 28 Feb 2024 18:13:08 +0000
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH] scsi: libsas: Fix disk not being scanned in after being
 removed
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: <20240221073159.29408-1-yangxingui@huawei.com>
Content-Language: en-US
From: John Garry <john.g.garry@oracle.com>
Organization: Oracle Corporation
In-Reply-To: <20240221073159.29408-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?RHlFcWpCVGthbzJXVnl1a0VqdHZBby9UamJlT2s5SzJPQThMZ3lad0dFL1NZ?=
 =?utf-8?B?aW1SZXVGRkdNbDlVNmFqd050dThqejVqT2IrdzBDUnRKU0VEMDR5TysrelBF?=
 =?utf-8?B?cFNnK1ArMVJFOVhkNndSUDBVYTcrMlBIa250VmZ1SUMzajJJUFBpamQ5aTVD?=
 =?utf-8?B?M2ZzZk1LWHN0emIwYXQraHJqK2ZkNFZPMmoyZ210cWhtSUpEZWtmOFplNWZz?=
 =?utf-8?B?ZmtaQlFvWmJJRldrRUM4RUxpaDQ3STJQQ0N3L3F2Mlgya0dtcCs5MnZhUUlQ?=
 =?utf-8?B?bzhUU0FKdU5yK0FaZ01wMmsxQ0RycWM4aTROanJBVnZWRnc2UlhzK2IvYW5O?=
 =?utf-8?B?SzFCcy8rZDJuOUNFS0I1YVM0amdZdjU5OUhlSzFMeEhuYjM3b2ROc3BKWmVS?=
 =?utf-8?B?L0laMWk2VVVZbytBRitDMUkxekJFVVBxRVFJUDhNd2UvQWtsUmMvbVhDY0dM?=
 =?utf-8?B?TDNQUkVKQ2JHMW1TR1hBVU9rT1kzVnJROTBCdTdhS01Vd0FzeDd2ZXNQZjdV?=
 =?utf-8?B?akdHc3RZTWRIa0orM0JNOEV5YmtLWWhTVjg5cHBXRlhjczU0TWxiTURWUzRX?=
 =?utf-8?B?TlllRmtvMy9GOFNTL29RQ3ZHUnRjY1N3NEdnbjZrT09TY3lxUVZxaFloRVpH?=
 =?utf-8?B?c05mR2VsMXF0YjlHeXU0TVZvbUppZDV5aUR1d2VqRXI1eDNlZVNQZFREKzdJ?=
 =?utf-8?B?bXdLNUlHZmRUdm1URjdkSnhSMk1YQnBBUWRiR0pUV0VyT29xMW1yRnhoRnpX?=
 =?utf-8?B?d3d1dHFxYVRYM285elRSaXBYOTRUMW9VcGdlMDJ6cFloZU5yczU1SkFkdTZX?=
 =?utf-8?B?WUZ4MHg1WDY1UkRXS0oraFlnbGVhYmZjaUhuaDNqR0dRSjRtc3Y1U3VhYmJW?=
 =?utf-8?B?S3RXVlZYL3VYSUNMU2ttNThvR2ZVcTR3dVJDZWlBdzN4bUlTaTN5MXZ2UVVO?=
 =?utf-8?B?U3hNOHJ5SWdacmd4SldKQVJrUkFqYlRndjFMTjlKVnlyUE1NS1BiWERLTmdl?=
 =?utf-8?B?VGg5bVJDWFlBdzdweE5xK0YxdXJmR1ZzSzFrNkRXdzFnK0hRK2x5bzgwdnFH?=
 =?utf-8?B?SnRXY2hXY1dyTVpvK2p0SkRQdzRMRldUck5iNHJxbUpvRmVQTW5XRzU1TzNU?=
 =?utf-8?B?aGFsOHgwcUk1VlVUUnMzanZkcFMvZ0NLbjFaZ1pzbjZwekppVVZZb0Yrbksr?=
 =?utf-8?B?ZUxJeWc1ZEIxMVhjckJpZnoxVWhFT1NTSWVOTmR0dVIxaU1CZUhjQzBvT3Qz?=
 =?utf-8?B?Qms1emlrUGpqeTRXRUhzRGlxSGpzL2pncEkvUUlRdVhCUWR0bWEybjNPNVJw?=
 =?utf-8?B?aFQxZ2hTc3R1d0E1MUlJVEtKQTBQYjY0WGxDTXZmaFZVM24zZFhNR29tUHJO?=
 =?utf-8?B?eHZVSnZNTk9aTmhtYVB3WlhubndUL1dsVGI4RnF0b3dxQWVZaEprVHJNblBi?=
 =?utf-8?B?VkFuaUFOZU1UY0lBNnNRUjdIakxydUd5OU1tUGF6L1psdkt5TkxhbzBpOUpq?=
 =?utf-8?B?MlRLTEV0ZHQvSXNtQUwramV1aXp1MTlHOFV4U01jdjN1eGtwVFZrbVVvWUdZ?=
 =?utf-8?B?aEYwdzlHK1ZQUUJIWVhGVW9mQ3NXazFNM3pFWDQ3Q2tod1ZUM3hxRXI1VFdT?=
 =?utf-8?B?Sm5xeUU3b0FwZVdXWjI0cjNFcmxpL0RJSXAxS2VtdWRYRGZmZlhSTzVJRWdI?=
 =?utf-8?B?UktJa1k5ZlJSUVJlbWhRZThicFJ4bTBzRlp2ZFQ2bjBEK2wzWEZUTmVLYmU1?=
 =?utf-8?B?VjRSWXpVeHR5RDEwUUdReWZMWlRXY1dreHVXZFY4K3N3cEZxM1NmZkQzUUhq?=
 =?utf-8?B?M0JyUk1ZTnNHeXJRZStjeWlUNy9NQjVqbWZwTlFDR3FtZ0tQQWVTWENFZlgr?=
 =?utf-8?B?QWlJVFNIeEg4N2lzbUhKdGQxZmJOQUwycGp6UmlIb2RUdkR5RzVjY1ZscHV2?=
 =?utf-8?B?dTdNR1RmM1d4MC9zVEM4OWkvU2c2Q3Y2RStESnZrR0dRYUk4Ujg3Q3BsVDAw?=
 =?utf-8?B?MzU3UFV3azhmZFlOMnNJdFhWNU16VWp1VURuaEU3U3RDam5DZENDUEpXNXAv?=
 =?utf-8?B?QU5CdE5VRFhla3RjajdzYlp2elM2RkJISmF4VDZjUG1XQm1uNkdrZTg4MVNt?=
 =?utf-8?Q?NOf6+td1Y9/fAl/6QZAaFb82r?=
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-0: 
	trjVLOvsXHfOpCBw00NuMJmdiFXJcZN81jS4SxE8OYE869Ucq5lOf8AkdLVJiAp9xobIygqKvW6SwRCns3yqimDiQ0Xd81L495wW5jq+Um8LyQUDGVP+Phz5WOZkznOlR34vaq8279u8V58kjZ5p7Op6TZVhI0qnYQRONdrwJJVcaaMaNpwa706NZpcNnxjGxvkjUfLfWji/Ff2OE5iZ6Bi0oN8G41Gjp7wwTltKixi7H66onlH+Drg6s81p5Jvptdf/7eg9gzQBZqO7VL8xvliN3PIwQeHdDO+THtounpjNW0wdSVBH49COB+9+R+dgk9CTI5INZRsC3FOWxrViuMf3CtamcuNHHe3tAHu/McW3ERteb1cvnEtTEAmdtrEw1WX3zq+NypfrHqB8QpLQViniWaCxvdmGDl09Hie7QOmqa9LCqeGL3ZKGPfEJ8wPoxfx9989s9iPew15Viorff4n15HrWdp44lh8zP7gSL4IYDjJl2WGpW9K274sOGEvAzwaYYtv6CMI5rrihjCYiDDWZ7cIoeJjCHy9NX5yMepccscxmNNmUkjaLOQ/Yb3H6/DrPFBdvBwDPyEAGsNkeeD2eNn9Tr01tMUK7IEQO5L4=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: bc9f6b1e-b701-4901-ec52-08dc3888ed32
X-MS-Exchange-CrossTenant-AuthSource: DM6PR10MB4313.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 28 Feb 2024 18:13:12.8398
 (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: CNATXLe88AIVWho0QyJhUvQdxWsVp1612f3JNw9BCjMsYFGfYToWOt8qnRWcsM5Dv5s/izsj0eWw+IVdkUFpIA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SN7PR10MB7002
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-02-28_08,2024-02-27_01,2023-05-22_02
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 suspectscore=0 phishscore=0 spamscore=0
 mlxlogscore=999 adultscore=0 mlxscore=0 bulkscore=0 malwarescore=0
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2311290000
 definitions=main-2402280144
X-Proofpoint-GUID: 9PguZkiM764zsirtvijkwLdIziMH-6Wv
X-Proofpoint-ORIG-GUID: 9PguZkiM764zsirtvijkwLdIziMH-6Wv

On 21/02/2024 07:31, 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.
> 
> Fixes: d8649fc1c5e4 ("scsi: libsas: Do discovery on empty PHY to update PHY info")
> Signed-off-by: Xingui Yang <yangxingui@huawei.com>
> ---
>   drivers/scsi/libsas/sas_expander.c | 9 ++++-----
>   1 file changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/scsi/libsas/sas_expander.c b/drivers/scsi/libsas/sas_expander.c
> index a2204674b680..9563f5589948 100644
> --- a/drivers/scsi/libsas/sas_expander.c
> +++ b/drivers/scsi/libsas/sas_expander.c
> @@ -1681,6 +1681,10 @@ int sas_get_phy_attached_dev(struct domain_device *dev, int phy_id,
>   		if (*type == 0)
>   			memset(sas_addr, 0, SAS_ADDR_SIZE);
>   	}
> +
> +	if ((SAS_ADDR(sas_addr) == 0) || (res == -ECOMM))

It's odd to call sas_set_ex_phy() if we got res == -ECOMM. I mean, in 
this this case disc_resp is not filled in as the command did not 
execute, right? I know that is what the current code does, but it is 
strange.

> +		sas_set_ex_phy(dev, phy_id, disc_resp);

So can we just call this here when we know that the SMP command was 
executed properly?

Thanks,
John

> +
>   	kfree(disc_resp);
>   	return res;
>   }
> @@ -1972,11 +1976,6 @@ static int sas_rediscover_dev(struct domain_device *dev, int phy_id,
>   	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.
> -		 */
> -		sas_ex_phy_discover(dev, phy_id);
>   		return res;
>   	} else if (SAS_ADDR(sas_addr) == SAS_ADDR(phy->attached_sas_addr) &&
>   		   dev_type_flutter(type, phy->attached_dev_type)) {