On 20.12.22 04:21, Wen Gu wrote:
> This patch introduces a kind of loopback device for SMC-D, thus
> enabling the SMC communication between two local sockets in one
> kernel.
>
> The loopback device supports basic capabilities defined by SMC-D,
> including registering DMB, unregistering DMB and moving data.
>
> Considering that there is no ism device on other servers expect
> IBM z13,
Please use the wording 'on other architectures except s390'.
That is how IBM Z is referred to in the Linux kernel.
> the loopback device can be used as a dummy device to
> test SMC-D logic for the broad community.
>
> Signed-off-by: Wen Gu <guwen@xxxxxxxxxxxxxxxxx>
> ---
Hello Wen Gu,
as the general design discussions are ongoing, I didn't
do a thorough review. But here are some general remarks
that you may want to consider for future versions.
I would propose to add a module parameter (default off) to enable
SMC-D loopback.
> include/net/smc.h | 1 +
> net/smc/Makefile | 2 +-
> net/smc/af_smc.c | 12 ++-
> net/smc/smc_cdc.c | 6 ++
> net/smc/smc_cdc.h | 1 +
> net/smc/smc_loopback.c | 282 +++++++++++++++++++++++++++++++++++++++++++++++++
> net/smc/smc_loopback.h | 59 +++++++++++
> 7 files changed, 361 insertions(+), 2 deletions(-)
> create mode 100644 net/smc/smc_loopback.c
> create mode 100644 net/smc/smc_loopback.h
>
I am not convinced that this warrants a separate file.
[...]
>
> +}
> +
> +static int lo_add_vlan_id(struct smcd_dev *smcd, u64 vlan_id)
> +{
> + return 0;
> +}
> +
> +static int lo_del_vlan_id(struct smcd_dev *smcd, u64 vlan_id)
> +{
> + return 0;
> +}
> +
> +static int lo_set_vlan_required(struct smcd_dev *smcd)
> +{
> + return 0;
> +}
> +
> +static int lo_reset_vlan_required(struct smcd_dev *smcd)
> +{
> + return 0;
> +}
The VLAN functions are only required for SMC-Dv1
Seems you want to provide v1 support for loopback?
May be nice for testing v1 VLAN support.
But then you need proper VLAN support.
[...]
> +
> +static u8 *lo_get_system_eid(void)
> +{
> + return &LO_SYSTEM_EID.seid_string[0];
> +}
SEID is for the whole system not per device.
We probably need to register a different function
for each architecture.
> +
> +static u16 lo_get_chid(struct smcd_dev *smcd)
> +{
> + return 0;
> +}
> +
Shouldn't this return 0xFFFF in your current concept?
