Re: [REGRESSION] v6.8 SMC-D issues

[Wen Gu]

On 2024/1/25 16:26, Alexandra Winter wrote:
> On 25.01.24 05:59, Wen Gu wrote:
> > After a while debug I found an elementary mistake of mine in
> > b40584d ("net/smc: compatible with 128-bits extended GID of virtual ISM device")..
> >
> > The operator order in smcd_lgr_match() is not as expected. It will always return
> > 'true' in remote-system case.
> >
> >   static bool smcd_lgr_match(struct smc_link_group *lgr,
> > -                          struct smcd_dev *smcismdev, u64 peer_gid)
> > +                          struct smcd_dev *smcismdev,
> > +                          struct smcd_gid *peer_gid)
> >   {
> > -       return lgr->peer_gid == peer_gid && lgr->smcd == smcismdev;
> > +       return lgr->peer_gid.gid == peer_gid->gid && lgr->smcd == smcismdev &&
> > +               smc_ism_is_virtual(smcismdev) ?
> > +               (lgr->peer_gid.gid_ext == peer_gid->gid_ext) : 1;
> >   }
> >
> > Could you please try again with this patch? to see if this is the root cause.
> > Really sorry for the inconvenience.
> >
> > diff --git a/net/smc/smc_core.c b/net/smc/smc_core.c
> > index da6a8d9c81ea..c6a6ba56c9e3 100644
> > --- a/net/smc/smc_core.c
> > +++ b/net/smc/smc_core.c
> > @@ -1896,8 +1896,8 @@ static bool smcd_lgr_match(struct smc_link_group *lgr,
> >                             struct smcd_gid *peer_gid)
> >   {
> >          return lgr->peer_gid.gid == peer_gid->gid && lgr->smcd == smcismdev &&
> > -               smc_ism_is_virtual(smcismdev) ?
> > -               (lgr->peer_gid.gid_ext == peer_gid->gid_ext) : 1;
> > +               (smc_ism_is_virtual(smcismdev) ?
> > +                (lgr->peer_gid.gid_ext == peer_gid->gid_ext) : 1);
> >   }
> >
> >
> > Thanks,
> > Wen Gu
>
> Hello Wen Gu,
>
> thank you for the quick resposne and for finding this nasty bug.
> I can confirm that with your patch I do not see the issue anymore.

Thank you very much for your confirmation, Alexandra.

> Please send a fix to the mailing lists. See
> https://docs.kernel.org/process/handling-regressions.html
> for some tips.

Thank you. Will do.

> May I propose that instead of adding the brackets, you change this function
> to an if-then-else sequence for readability and maintainability?
> I would still mention the missing brackets in the commit message, so
> readers can quickly understand the issue.

I agree. if-then-else will make it clearer. I will fix it like this:

diff --git a/net/smc/smc_core.c b/net/smc/smc_core.c
index da6a8d9c81ea..1d5bce82d4d8 100644
--- a/net/smc/smc_core.c
+++ b/net/smc/smc_core.c
@@ -1895,9 +1895,15 @@ static bool smcd_lgr_match(struct smc_link_group *lgr,
                           struct smcd_dev *smcismdev,
                           struct smcd_gid *peer_gid)
 {
-       return lgr->peer_gid.gid == peer_gid->gid && lgr->smcd == smcismdev &&
-               smc_ism_is_virtual(smcismdev) ?
-               (lgr->peer_gid.gid_ext == peer_gid->gid_ext) : 1;
+       if (lgr->peer_gid.gid != peer_gid->gid ||
+           lgr->smcd != smcismdev)
+               return false;
+
+       if (smc_ism_is_virtual(smcismdev) &&
+           lgr->peer_gid.gid_ext != peer_gid->gid_ext)
+               return false;
+
+       return true;
 }

Thanks again,
Wen Gu

> Thanks again for the quick response.
> Sandy