[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <b7caa2cc-4615-6f0e-c296-6142b3724e01@linux.alibaba.com>
Date: Tue, 28 Nov 2023 11:13:39 +0800
From: Wen Gu <guwen@...ux.alibaba.com>
To: Alexandra Winter <wintera@...ux.ibm.com>, wenjia@...ux.ibm.com,
hca@...ux.ibm.com, gor@...ux.ibm.com, agordeev@...ux.ibm.com,
davem@...emloft.net, edumazet@...gle.com, kuba@...nel.org,
pabeni@...hat.com, kgraul@...ux.ibm.com, jaka@...ux.ibm.com
Cc: borntraeger@...ux.ibm.com, svens@...ux.ibm.com,
alibuda@...ux.alibaba.com, tonylu@...ux.alibaba.com,
raspl@...ux.ibm.com, schnelle@...ux.ibm.com,
linux-s390@...r.kernel.org, netdev@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH net-next v2 7/7] net/smc: manage system EID in SMC stack
instead of ISM driver
On 2023/11/27 22:04, Alexandra Winter wrote:
>
>
> On 24.11.23 15:42, Wen Gu wrote:
>> The System EID (SEID) is an internal EID that is used by the SMCv2
>> software stack that has a predefined and constant value representing
>> the s390 physical machine that the OS is executing on. So it should
>> be managed by SMC stack instead of ISM driver and be consistent for
>> all ISMv2 device (including virtual ISM devices) on s390 architecture.
>>
>> Suggested-by: Alexandra Winter <wintera@...ux.ibm.com>
>> Signed-off-by: Wen Gu <guwen@...ux.alibaba.com>
>> ---
>
> Yes, this is what I had in mind. Thank you Wen Gu.
:)
> [...]
>
>>
>> diff --git a/drivers/s390/net/ism.h b/drivers/s390/net/ism.h
>> index 70c5bbd..49ccbd68 100644
>> --- a/drivers/s390/net/ism.h
>> +++ b/drivers/s390/net/ism.h
>
> Please remove ISM_IDENT_MASK from drivers/s390/net/ism.h
> [...]
>
Thanks for reminding. I will remove it.
>> --- a/drivers/s390/net/ism_drv.c
>> +++ b/drivers/s390/net/ism_drv.c
>> @@ -36,6 +36,7 @@
> [...]
>> -static void ism_create_system_eid(void)
>> -{
>> - struct cpuid id;
>> - u16 ident_tail;
>> - char tmp[5];
>> -
>> - get_cpu_id(&id);
>> - ident_tail = (u16)(id.ident & ISM_IDENT_MASK);
>> - snprintf(tmp, 5, "%04X", ident_tail);
>> - memcpy(&SYSTEM_EID.serial_number, tmp, 4);
>> - snprintf(tmp, 5, "%04X", id.machine);
>> - memcpy(&SYSTEM_EID.type, tmp, 4);
>> -}
>> -
> [...]
>> @@ -560,7 +535,7 @@ static int ism_dev_init(struct ism_dev *ism)
>>
>> if (!ism_add_vlan_id(ism, ISM_RESERVED_VLANID))
>> /* hardware is V2 capable */
>> - ism_create_system_eid();
>> + ism_v2_capable = true;
>>
>
> Please assign 'false' in the else path.
> This is required here for backwards compatibility. Hardware that only supports v1,
> will reject ISM_RESERVED_VLANID.
>
OK. I will assign false in the else path to explicitly express the meaning. Thank you.
> [...]
>
>
>> --- a/net/smc/smc_ism.c
>> +++ b/net/smc/smc_ism.c
> [...]
>> @@ -70,6 +91,11 @@ bool smc_ism_is_v2_capable(void)
>> return smc_ism_v2_capable;
>> }
>>
>> +void smc_ism_set_v2_capable(void)
>> +{
>> + smc_ism_v2_capable = true;
>> +}
>> +
>> /* Set a connection using this DMBE. */
>> void smc_ism_set_conn(struct smc_connection *conn)
>> {
>> @@ -431,14 +457,8 @@ static void smcd_register_dev(struct ism_dev *ism)
>>
>> mutex_lock(&smcd_dev_list.mutex);
>> if (list_empty(&smcd_dev_list.list)) {
>> - u8 *system_eid = NULL;
>> -
>> - system_eid = smcd->ops->get_system_eid();
>> - if (smcd->ops->supports_v2()) {
>> - smc_ism_v2_capable = true;
>> - memcpy(smc_ism_v2_system_eid, system_eid,
>> - SMC_MAX_EID_LEN);
>> - }
>> + if (smcd->ops->supports_v2())
>> + smc_ism_set_v2_capable();
>
> I don't see the benefit in declaring smc_ism_set_v2_capable() and exporting it in smc_ism.h,
> when it is used only once and only here.
> Why don't you just set
> smc_ism_v2_capable = true;
> here?
>
Yes.. it may be confusing if readers only look at this patch set.
It is because loopback-ism (or other kinds of ISMv2.1) will also use this helper in such
as smc_loopback.c to set smc_ism_v2_capable (defined in smc_ism.c) as true. So I think it
may be better to introduce a helper here instead of exposing smc_ism_v2_capable variable.
Thanks.
> [...]
>> diff --git a/net/smc/smc_ism.h b/net/smc/smc_ism.h
>> index 0e5e563..6903cd5 100644
>> --- a/net/smc/smc_ism.h
>> +++ b/net/smc/smc_ism.h
>> @@ -16,6 +16,7 @@
>> #include "smc.h"
>>
>> #define SMC_VIRTUAL_ISM_CHID_MASK 0xFF00
>> +#define SMC_ISM_IDENT_MASK 0x00FFFF
>>
> [...]
>> @@ -45,6 +52,7 @@ int smc_ism_register_dmb(struct smc_link_group *lgr, int buf_size,
>> void smc_ism_get_system_eid(u8 **eid);
>> u16 smc_ism_get_chid(struct smcd_dev *dev);
>> bool smc_ism_is_v2_capable(void);
>> +void smc_ism_set_v2_capable(void);
>> int smc_ism_init(void);
>> void smc_ism_exit(void);
>> int smcd_nl_get_device(struct sk_buff *skb, struct netlink_callback *cb);
Powered by blists - more mailing lists