| Message ID | 20240221073159.29408-1-yangxingui@huawei.com |
|---|---|
| State | New |
| Headers | show |
| Series | scsi: libsas: Fix disk not being scanned in after being removed | expand |
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
On 2024/3/4 20:50, yangxingui wrote: > 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. OK, so let me have a closer look at v2. Thanks, Jason > > Thanks, > Xingui > .
Hi John, On 2024/3/5 18:15, John Garry wrote: > On 05/03/2024 02:56, Jason Yan wrote: >> On 2024/3/4 20:50, yangxingui wrote: >>> 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>kkkkk >>>>>> --- >>>>>> 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. > > I don't understand this. Where is the previous SAS address compared to > the current SAS address? > > Could this work: > > diff --git a/drivers/scsi/libsas/sas_expander.c > b/drivers/scsi/libsas/sas_expander.c > index a2204674b680..e190038ba7bd 100644 > --- a/drivers/scsi/libsas/sas_expander.c > +++ b/drivers/scsi/libsas/sas_expander.c > @@ -1675,11 +1675,13 @@ int sas_get_phy_attached_dev(struct > domain_device *dev, int phy_id, > > res = sas_get_phy_discover(dev, phy_id, disc_resp); > if (res == 0) { > - memcpy(sas_addr, disc_resp->disc.attached_sas_addr, > - SAS_ADDR_SIZE); > *type = to_dev_type(&disc_resp->disc); > - if (*type == 0) > + if (*type == SAS_PHY_UNUSED) > memset(sas_addr, 0, SAS_ADDR_SIZE); > + else > + memcpy(sas_addr, disc_resp->disc.attached_sas_addr, > + SAS_ADDR_SIZE); > + sas_set_ex_phy(dev, phy_id, disc_resp); > } > kfree(disc_resp); > return res; > lines 1-21/21 (END) > > It's like the change in this patch. This doesn't work properly. the previous sas address will be compared with the currently queried sas address and the previous phy information will also be used when calling sas_unregister_devs_sas_addr() after the sas_rediscover_dev() function calls sas_get_phy_attached_dev(). Therefore, it is more appropriate to update the phy information after the device is unregistered. as follows: static int sas_rediscover_dev(struct domain_device *dev, int phy_id, bool last, int sibling) { ... res = sas_get_phy_attached_dev(dev, phy_id, sas_addr, &type); switch (res) { case SMP_RESP_NO_PHY: phy->phy_state = PHY_NOT_PRESENT; sas_unregister_devs_sas_addr(dev, phy_id, last); return res; case SMP_RESP_PHY_VACANT: phy->phy_state = PHY_VACANT; sas_unregister_devs_sas_addr(dev, phy_id, last); return res; case SMP_RESP_FUNC_ACC: break; case -ECOMM: break; default: return res; } 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) && // <=== Compare the previous sas address with the current sas address dev_type_flutter(type, phy->attached_dev_type)) { struct domain_device *ata_dev = sas_ex_to_ata(dev, phy_id); char *action = ""; sas_ex_phy_discover(dev, phy_id); if (ata_dev && phy->attached_dev_type == SAS_SATA_PENDING) action = ", needs recovery"; pr_debug("ex %016llx phy%02d broadcast flutter%s\n", SAS_ADDR(dev->sas_addr), phy_id, action); return res; } > > >> >> OK, so let me have a closer look at v2. > > I have to say that v2 is quite complicated... Yes, but it works. Thanks, Xingui
On 05/03/2024 11:25, yangxingui wrote: >> >> It's like the change in this patch. > This doesn't work properly. the previous sas address will be compared > with the currently queried sas address and the previous phy information > will also be used when calling sas_unregister_devs_sas_addr() after the > sas_rediscover_dev() function calls sas_get_phy_attached_dev(). > Therefore, it is more appropriate to update the phy information after > the device is unregistered. ok, fine > as follows: > static int sas_rediscover_dev(struct domain_device *dev, int phy_id, > bool last, int sibling) > { > ... > res = sas_get_phy_attached_dev(dev, phy_id, sas_addr, &type); > switch (res) { > case SMP_RESP_NO_PHY: > phy->phy_state = PHY_NOT_PRESENT; > sas_unregister_devs_sas_addr(dev, phy_id, last); > return res; > case SMP_RESP_PHY_VACANT: > phy->phy_state = PHY_VACANT; > sas_unregister_devs_sas_addr(dev, phy_id, last); > return res; > case SMP_RESP_FUNC_ACC: > break; > case -ECOMM: > break; > default: > return res; > } > > 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) && // <=== Compare the previous sas > address with the current sas address > dev_type_flutter(type, phy->attached_dev_type)) { > struct domain_device *ata_dev = sas_ex_to_ata(dev, > phy_id); > char *action = ""; > > sas_ex_phy_discover(dev, phy_id); > > if (ata_dev && phy->attached_dev_type == SAS_SATA_PENDING) > action = ", needs recovery"; > pr_debug("ex %016llx phy%02d broadcast flutter%s\n", > SAS_ADDR(dev->sas_addr), phy_id, action); > return res; > } > >> >> >>> >>> OK, so let me have a closer look at v2. >> >> I have to say that v2 is quite complicated... > Yes, but it works. I'll check it again. Thanks, John
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)) {
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(-)