Messages in this thread | | | Subject | Re: [PATCH] scsi: libsas: Fix disk not being scanned in after being removed | From | Jason Yan <> | Date | Tue, 27 Feb 2024 15:16:58 +0800 |
| |
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 > > .
| |