Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <2ff23fc7-c393-46b8-e358-31a39ed2c56b@linux.ibm.com>
Date:   Thu, 19 Jan 2023 17:25:01 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:102.0)
 Gecko/20100101 Thunderbird/102.6.1
From:   Alexandra Winter <wintera@linux.ibm.com>
Subject: Re: [RFC PATCH net-next v2 1/5] net/smc: introduce SMC-D loopback
 device
To:     Wen Gu <guwen@linux.alibaba.com>, kgraul@linux.ibm.com,
        wenjia@linux.ibm.com, jaka@linux.ibm.com, davem@davemloft.net,
        edumazet@google.com, kuba@kernel.org, pabeni@redhat.com
Cc:     linux-s390@vger.kernel.org, netdev@vger.kernel.org,
        linux-kernel@vger.kernel.org
References: <1671506505-104676-1-git-send-email-guwen@linux.alibaba.com>
 <1671506505-104676-2-git-send-email-guwen@linux.alibaba.com>
Content-Language: en-US
In-Reply-To: <1671506505-104676-2-git-send-email-guwen@linux.alibaba.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk


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@linux.alibaba.com>
> ---

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?