Received-SPF: pass (google.com: domain of linux-kernel+bounces-38263-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Message-ID: <d93bdf89-d724-4f2a-a3fc-f3a46e54202c@linux.alibaba.com>
Date: Thu, 25 Jan 2024 17:28:43 +0800
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [REGRESSION] v6.8 SMC-D issues
To: Alexandra Winter <wintera@linux.ibm.com>, wenjia@linux.ibm.com,
 hca@linux.ibm.com, gor@linux.ibm.com, agordeev@linux.ibm.com,
 davem@davemloft.net, edumazet@google.com, kuba@kernel.org,
 pabeni@redhat.com, jaka@linux.ibm.com,
 Matthew Rosato <mjrosato@linux.ibm.com>
Cc: Linux regressions mailing list <regressions@lists.linux.dev>,
 borntraeger@linux.ibm.com, svens@linux.ibm.com, alibuda@linux.alibaba.com,
 tonylu@linux.alibaba.com, raspl@linux.ibm.com, schnelle@linux.ibm.com,
 guangguan.wang@linux.alibaba.com, linux-s390@vger.kernel.org,
 netdev@vger.kernel.org, linux-kernel@vger.kernel.org,
 Halil Pasic <pasic@linux.ibm.com>
References: <20231219142616.80697-1-guwen@linux.alibaba.com>
 <20231219142616.80697-8-guwen@linux.alibaba.com>
 <13579588-eb9d-4626-a063-c0b77ed80f11@linux.ibm.com>
 <530afe45-ba6b-4970-a71c-1f1255f5fca9@linux.alibaba.com>
 <8090bb34-1b70-43ea-ae13-df5d9a5eb761@linux.ibm.com>
From: Wen Gu <guwen@linux.alibaba.com>
In-Reply-To: <8090bb34-1b70-43ea-ae13-df5d9a5eb761@linux.ibm.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit


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