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.
Please send a fix to the mailing lists. See
https://docs.kernel.org/process/handling-regressions.html
for some tips.
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.
Thanks again for the quick response.
Sandy
