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
[...]
> --- 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.
[...]
> --- 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?
[...]
> 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);
