Message ID | 20240221073159.29408-1-yangxingui@huawei.com |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel+bounces-74238-ouuuleilei=gmail.com@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a05:693c:2685:b0:108:e6aa:91d0 with SMTP id mn5csp887083dyc; Tue, 20 Feb 2024 23:40:07 -0800 (PST) X-Forwarded-Encrypted: i=3; AJvYcCVn+rdV027nhdFN6fEfocfpY59yOfpzvIw0+VcHaUpV4BTGL0PHS9+dhfIIVggg2kaefjPqIg6PdnzoiwUtzh9ETiWfVw== X-Google-Smtp-Source: AGHT+IEMHnv6i4UTISlOBkmxXvm1wWiPKgWz7pgPi3Hgf7qoBlEnemxznfw9N5N/huQfJbQtOzRj X-Received: by 2002:a05:6a20:6f07:b0:19e:5fe5:ff98 with SMTP id gt7-20020a056a206f0700b0019e5fe5ff98mr18478899pzb.52.1708501207082; Tue, 20 Feb 2024 23:40:07 -0800 (PST) ARC-Seal: i=2; a=rsa-sha256; t=1708501207; cv=pass; d=google.com; s=arc-20160816; b=TzXOSnURjM8cpm+AeTJPGB+gfqXy4XPE8+POQXuBpd4991FDFOV4hGENR/zPhx1xPn tPPI6lkgnmKOuM2wfINYBHb9NdD536dCuyVGb57DmuUc1ev6eeIXwAiZV3uA7ipBJLRv vPhRfU6FtRTUpxgsbtsSfkKbwv0ivcoiQuaZlIPPXkxKnDoSfYPzY9C6HEoZhCotL6/F oSqXjTsIsShGvxXO4J90PJ1P90SE3dUpn17BkZdyJQaonJR5gU7cTSASlIqwxAH7u7l1 KZKLbt3IIWlr27g3ZYRsgnWmKZSAqAicmhHlXvlFTnfxomiHwZu2X/y1AtxD6r5YuhX0 lpfQ== ARC-Message-Signature: i=2; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=mime-version:list-unsubscribe:list-subscribe:list-id:precedence :message-id:date:subject:cc:to:from; bh=7GgEXXWCQZf22Lksd8+ncIxFu8TgO0CpQj3ssJUtF2o=; fh=PKov2v7mtZqWrG2ToL14QCqscLF6oofVifdomTnOWI8=; b=jMY6DAfjWi02hobZ2HWUumtpaZ7xgaEAWLfH66c1tVwgz+QoXR0L11EA+J5KXGEGM4 mBNCU5xGsA2TIMtyjJ1dPrrM6lXdutKjW8xkN+hD1Tu4ekKXTN2y7OC0BrRL2dMz96pC sfqmORdj/GMKk2ldjslS9XhasiUhOILSy7v9ZG0AyLO953G6RTVauPfF6XxABrXo3dTd ikYleHzPQAsKYGNL/GRR74wu0O8JZfuqEURhNi5af/UhHUjOj1xmZ3FeNW5hit2fFFAX 3z3Q048A4Hvv7twvTtYaUTsdBRLx9My5kPhPQTrv/HR6jbCyo1Zza+L29rI1kbu6O3oB DbHA==; dara=google.com ARC-Authentication-Results: i=2; mx.google.com; arc=pass (i=1 spf=pass spfdomain=huawei.com dmarc=pass fromdomain=huawei.com); spf=pass (google.com: domain of linux-kernel+bounces-74238-ouuuleilei=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) smtp.mailfrom="linux-kernel+bounces-74238-ouuuleilei=gmail.com@vger.kernel.org"; dmarc=fail (p=QUARANTINE sp=QUARANTINE dis=NONE) header.from=huawei.com Received: from sv.mirrors.kernel.org (sv.mirrors.kernel.org. [2604:1380:45e3:2400::1]) by mx.google.com with ESMTPS id ne9-20020a17090b374900b00298ee54d6cdsi974779pjb.153.2024.02.20.23.40.06 for <ouuuleilei@gmail.com> (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 20 Feb 2024 23:40:07 -0800 (PST) Received-SPF: pass (google.com: domain of linux-kernel+bounces-74238-ouuuleilei=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1; Authentication-Results: mx.google.com; arc=pass (i=1 spf=pass spfdomain=huawei.com dmarc=pass fromdomain=huawei.com); spf=pass (google.com: domain of linux-kernel+bounces-74238-ouuuleilei=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) smtp.mailfrom="linux-kernel+bounces-74238-ouuuleilei=gmail.com@vger.kernel.org"; dmarc=fail (p=QUARANTINE sp=QUARANTINE dis=NONE) header.from=huawei.com Received: from smtp.subspace.kernel.org (wormhole.subspace.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by sv.mirrors.kernel.org (Postfix) with ESMTPS id 7ECE728A826 for <ouuuleilei@gmail.com>; Wed, 21 Feb 2024 07:33:46 +0000 (UTC) Received: from localhost.localdomain (localhost.localdomain [127.0.0.1]) by smtp.subspace.kernel.org (Postfix) with ESMTP id 5D1F43B1B7; Wed, 21 Feb 2024 07:33:35 +0000 (UTC) Received: from szxga05-in.huawei.com (szxga05-in.huawei.com [45.249.212.191]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 45AF823C9; Wed, 21 Feb 2024 07:33:30 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=45.249.212.191 ARC-Seal: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1708500813; cv=none; b=JRlUK4Rn5wc4+obzesvExZyT9Hi4G23hMtfUFzkLu8SzicMYSR2WAuC2HX9qIeVVY6HrSmZqql/HI+4W2eiPO3xv43OX4hmZZxLbOK0X+UDMg7o2RJ71nkEh/Ctu1DrW3UJ/Vz69wF/0LPSUD1Atu/kHrYvjFBWMfRteBXkOMAQ= ARC-Message-Signature: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1708500813; c=relaxed/simple; bh=PlY8APXUkmE6j7kWNFypEEvOafZlgJQqQ7toSvbcGM0=; h=From:To:CC:Subject:Date:Message-ID:MIME-Version:Content-Type; b=T9a0tI6PxL4FpueF23hQqgF0A1g89QKrzT+p7qG1BscqhcmNdEsPrUOg0Z7DNfHTMux9dX6hW/NERlpWvq0I+g3+Jfq4nUM5eXZvlE2PR1SrFaEVWxUM0695kc0KYyDVtkfyFJN+b092pLGHhlUP24h7ppMUGfFm3VsOFSILbEs= ARC-Authentication-Results: i=1; smtp.subspace.kernel.org; dmarc=pass (p=quarantine dis=none) header.from=huawei.com; spf=pass smtp.mailfrom=huawei.com; arc=none smtp.client-ip=45.249.212.191 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=quarantine dis=none) header.from=huawei.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=huawei.com Received: from mail.maildlp.com (unknown [172.19.163.17]) by szxga05-in.huawei.com (SkyGuard) with ESMTP id 4Tfnv16861z1FLF3; Wed, 21 Feb 2024 15:28:29 +0800 (CST) Received: from dggpemd100001.china.huawei.com (unknown [7.185.36.94]) by mail.maildlp.com (Postfix) with ESMTPS id 6FCD51A0172; Wed, 21 Feb 2024 15:33:21 +0800 (CST) Received: from localhost.localdomain (10.50.165.33) by dggpemd100001.china.huawei.com (7.185.36.94) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.2.1258.28; Wed, 21 Feb 2024 15:33:21 +0800 From: Xingui Yang <yangxingui@huawei.com> To: <john.g.garry@oracle.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> Subject: [PATCH] scsi: libsas: Fix disk not being scanned in after being removed Date: Wed, 21 Feb 2024 07:31:59 +0000 Message-ID: <20240221073159.29408-1-yangxingui@huawei.com> X-Mailer: git-send-email 2.17.1 Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: <linux-kernel.vger.kernel.org> List-Subscribe: <mailto:linux-kernel+subscribe@vger.kernel.org> List-Unsubscribe: <mailto:linux-kernel+unsubscribe@vger.kernel.org> MIME-Version: 1.0 Content-Type: text/plain X-ClientProxiedBy: dggems704-chm.china.huawei.com (10.3.19.181) To dggpemd100001.china.huawei.com (7.185.36.94) X-getmail-retrieved-from-mailbox: INBOX X-GMAIL-THRID: 1791493361680338233 X-GMAIL-MSGID: 1791493361680338233 |
Series |
scsi: libsas: Fix disk not being scanned in after being removed
|
|
Commit Message
yangxingui
Feb. 21, 2024, 7:31 a.m. UTC
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(-)
Comments
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)) > + sas_set_ex_phy(dev, phy_id, disc_resp); > + > 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); It would be nice to be able to call sas_set_ex_phy() here (instead of sas_get_phy_attached_dev()), but I assume that you can't do that as the disc_resp memory is not available. If we were to manually set the PHY info here instead, how would that look? Thanks, John > return res; > } else if (SAS_ADDR(sas_addr) == SAS_ADDR(phy->attached_sas_addr) && > dev_type_flutter(type, phy->attached_dev_type)) {
Hi, John On 2024/2/22 20:41, John Garry wrote: > 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)) >> + sas_set_ex_phy(dev, phy_id, disc_resp); >> + >> 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); > > It would be nice to be able to call sas_set_ex_phy() here (instead of > sas_get_phy_attached_dev()), but I assume that you can't do that as the > disc_resp memory is not available. > > If we were to manually set the PHY info here instead, how would that look? Yes, I think it is indeed better to use sas_set_ex_phy, as you said, disc_resp memory is not available. Maybe we can use sas_get_phy_discover instead of sas_get_phy_attached_dev so we can use disc_resp? as follow: +++ 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,41 @@ 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); .. - /* - * 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; + if (res == 0) + sas_set_ex_phy(dev, phy_id, disc_resp); + goto out; .. +out: + kfree(disc_resp); + return res; Thanks. Xingui
On 2024/2/23 12:04, yangxingui wrote: > Hi, John > > On 2024/2/22 20:41, John Garry wrote: >> 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)) >>> + sas_set_ex_phy(dev, phy_id, disc_resp); >>> + >>> 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); >> >> It would be nice to be able to call sas_set_ex_phy() here (instead of >> sas_get_phy_attached_dev()), but I assume that you can't do that as >> the disc_resp memory is not available. >> >> If we were to manually set the PHY info here instead, how would that >> look? > Yes, I think it is indeed better to use sas_set_ex_phy, as you said, > disc_resp memory is not available. Maybe we can use sas_get_phy_discover > instead of sas_get_phy_attached_dev so we can use disc_resp? Can we directly set phy->negotiated_linkrate = SAS_PHY_DISABLED here? For an empty PHY the other variables means nothing, so why bother get and update them? Thanks, Jason
Hi Jason, On 2024/2/23 15:04, Jason Yan wrote: > On 2024/2/23 12:04, yangxingui wrote: >> Hi, John >> >> On 2024/2/22 20:41, John Garry wrote: >>> 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)) >>>> + sas_set_ex_phy(dev, phy_id, disc_resp); >>>> + >>>> 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); >>> >>> It would be nice to be able to call sas_set_ex_phy() here (instead of >>> sas_get_phy_attached_dev()), but I assume that you can't do that as >>> the disc_resp memory is not available. >>> >>> If we were to manually set the PHY info here instead, how would that >>> look? >> Yes, I think it is indeed better to use sas_set_ex_phy, as you said, >> disc_resp memory is not available. Maybe we can use >> sas_get_phy_discover instead of sas_get_phy_attached_dev so we can use >> disc_resp? > > Can we directly set phy->negotiated_linkrate = SAS_PHY_DISABLED here? > For an empty PHY the other variables means nothing, so why bother get > and update them? The value of the negotiated link rate has two possible values in the current processing branch: SAS_LINK_RATE_UNKNOWN and SAS_PHY_DISABLED, and both come from disc_resp. If we do not use disc_resp, but set a fixed value SAS_PHY_DISABLED for it, it may not be appropriate. Thanks, Xingui
On 2024/2/27 11:06, yangxingui wrote: > Hi Jason, > > On 2024/2/23 15:04, Jason Yan wrote: >> On 2024/2/23 12:04, yangxingui wrote: >>> Hi, John >>> >>> On 2024/2/22 20:41, John Garry wrote: >>>> 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)) >>>>> + sas_set_ex_phy(dev, phy_id, disc_resp); >>>>> + >>>>> 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); >>>> >>>> It would be nice to be able to call sas_set_ex_phy() here (instead >>>> of sas_get_phy_attached_dev()), but I assume that you can't do that >>>> as the disc_resp memory is not available. >>>> >>>> If we were to manually set the PHY info here instead, how would that >>>> look? >>> Yes, I think it is indeed better to use sas_set_ex_phy, as you said, >>> disc_resp memory is not available. Maybe we can use >>> sas_get_phy_discover instead of sas_get_phy_attached_dev so we can >>> use disc_resp? >> >> Can we directly set phy->negotiated_linkrate = SAS_PHY_DISABLED here? >> For an empty PHY the other variables means nothing, so why bother get >> and update them? > The value of the negotiated link rate has two possible values in the > current processing branch: SAS_LINK_RATE_UNKNOWN and SAS_PHY_DISABLED, > and both come from disc_resp. If we do not use disc_resp, but set a > fixed value SAS_PHY_DISABLED for it, it may not be appropriate. OK, makes sense. > > Thanks, > Xingui > > .
On 27/02/2024 07:16, Jason Yan wrote: >>> >>> Can we directly set phy->negotiated_linkrate = SAS_PHY_DISABLED here? >>> For an empty PHY the other variables means nothing, so why bother get >>> and update them? >> The value of the negotiated link rate has two possible values in the >> current processing branch: SAS_LINK_RATE_UNKNOWN and SAS_PHY_DISABLED, >> and both come from disc_resp. If we do not use disc_resp, but set a >> fixed value SAS_PHY_DISABLED for it, it may not be appropriate. But we know that the phy is disabled, right? It's our phy, isn't it? Thanks, John
Hi John, On 2024/2/27 17:06, John Garry wrote: > On 27/02/2024 07:16, Jason Yan wrote: >>>> >>>> Can we directly set phy->negotiated_linkrate = SAS_PHY_DISABLED >>>> here? For an empty PHY the other variables means nothing, so why >>>> bother get and update them? >>> The value of the negotiated link rate has two possible values in >>> the current processing branch: SAS_LINK_RATE_UNKNOWN and >>> SAS_PHY_DISABLED, and both come from disc_resp. If we do not use >>> disc_resp, but set a fixed value SAS_PHY_DISABLED for it, it may not >>> be appropriate. > > But we know that the phy is disabled, right? It's our phy, isn't it? Yes, just like the previous submission, if we disable phy ourselves through the sysfs node, we can configure the negotiation rate to SAS_PHY_DISABLED by setting phy->phy->enable to 0. It might be better to use sas_set_ex_phy() as you described before, it will refresh other phy information synchronously, such as sas_address, device_type, target_protocols, etc. If we only update the negotiation rate and maintain the old information, is it because it is special? Is it better to update phy information uniformly? Thanks, Xingui
Hi John, On 2024/2/22 20:41, John Garry wrote: > 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)) >> + sas_set_ex_phy(dev, phy_id, disc_resp); >> + >> 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); > > It would be nice to be able to call sas_set_ex_phy() here (instead of > sas_get_phy_attached_dev()), but I assume that you can't do that as the > disc_resp memory is not available. > By the way, I have updated a version and call sas_set_ex_phy() here, please check it again. Thanks, Xingui
On 28/02/2024 13:13, yangxingui wrote: > Hi John, > > On 2024/2/22 20:41, John Garry wrote: >> 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)) >>> + sas_set_ex_phy(dev, phy_id, disc_resp); >>> + >>> 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); >> >> It would be nice to be able to call sas_set_ex_phy() here (instead of >> sas_get_phy_attached_dev()), but I assume that you can't do that as >> the disc_resp memory is not available. >> > By the way, I have updated a version and call sas_set_ex_phy() here, > please check it again. > Then maybe the first patch is a better approach. Let me check it again. Sorry for the delay. John
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)) {
On 2024/2/29 2:13, John Garry wrote: > 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. The current code actually re-send the SMP command and update the PHY status only when the the SMP command is responded correctly. Xinggui, can you please fix this and send v3? Thanks, Jason
Hi Jason, On 2024/3/1 9:55, Jason Yan wrote: > On 2024/2/29 2:13, John Garry wrote: >> 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. > > The current code actually re-send the SMP command and update the PHY > status only when the the SMP command is responded correctly. > > Xinggui, can you please fix this and send v3? The current location cannot directly update the phy information. The previous phy information will be used later, and the previous sas address will be compared with the currently queried sas address. At present, v2 is more suitable after many days of testing. Thanks, Xingui
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)) + sas_set_ex_phy(dev, phy_id, disc_resp); + 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)) {