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@xxxxxxxxxxxxx>
Signed-off-by: Wen Gu <guwen@xxxxxxxxxxxxxxxxx>
---
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);
