Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:1 as permitted sender) client-ip=2620:137:e000::3:1;
Message-ID: <117f0100-a3c2-49c2-88a9-aa0835942773@linux.ibm.com>
Date:   Mon, 4 Dec 2023 10:15:04 +0100
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH net-next v3 5/7] net/smc: compatible with 128-bits extend
 GID of virtual ISM device
To:     Alexandra Winter <wintera@linux.ibm.com>,
        Wen Gu <guwen@linux.alibaba.com>, hca@linux.ibm.com,
        gor@linux.ibm.com, agordeev@linux.ibm.com, davem@davemloft.net,
        edumazet@google.com, kuba@kernel.org, pabeni@redhat.com,
        kgraul@linux.ibm.com, jaka@linux.ibm.com
Cc:     borntraeger@linux.ibm.com, svens@linux.ibm.com,
        alibuda@linux.alibaba.com, tonylu@linux.alibaba.com,
        raspl@linux.ibm.com, schnelle@linux.ibm.com,
        linux-s390@vger.kernel.org, netdev@vger.kernel.org,
        linux-kernel@vger.kernel.org
References: <1701343695-122657-1-git-send-email-guwen@linux.alibaba.com>
 <1701343695-122657-6-git-send-email-guwen@linux.alibaba.com>
 <19b288d3-5434-40b1-93fa-7db47e417f60@linux.ibm.com>
Content-Language: en-GB
From:   Wenjia Zhang <wenjia@linux.ibm.com>
In-Reply-To: <19b288d3-5434-40b1-93fa-7db47e417f60@linux.ibm.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
Precedence: bulk


On 01.12.23 17:30, Alexandra Winter wrote:
> 
> 
> On 30.11.23 12:28, Wen Gu wrote:
> [...]
>[...]
> 
>>   	if (nla_put_u8(skb, SMC_NLA_LGR_D_VLAN_ID, lgr->vlan_id))
>> @@ -876,7 +877,10 @@ static int smc_lgr_create(struct smc_sock *smc, struct smc_init_info *ini)
>>   		/* SMC-D specific settings */
> 
> 
> I guess I never really understood, why we define a single linkgroup for SMC-D.
> Probably because SMC-R linkgroups were implemented before SMC-D support was added.
As I'm aware, this is indeed the reason.

> To all: Do we want to keep that concept?
> 
Since SMC-D and SMC-R still share many common code, this concept should 
be kept, at least for now and now for this patch.

> 
>>   		smcd = ini->ism_dev[ini->ism_selected];
>>   		get_device(smcd->ops->get_dev(smcd));
>> -		lgr->peer_gid = ini->ism_peer_gid[ini->ism_selected];
>> +		lgr->peer_gid.gid =
>> +			ini->ism_peer_gid[ini->ism_selected].gid;
>> +		lgr->peer_gid.gid_ext =
>> +			ini->ism_peer_gid[ini->ism_selected].gid_ext;
>>   		lgr->smcd = ini->ism_dev[ini->ism_selected];
>>   		lgr_list = &ini->ism_dev[ini->ism_selected]->lgr_list;
>>   		lgr_lock = &lgr->smcd->lgr_lock;
>> @@ -1514,7 +1518,8 @@ void smc_lgr_terminate_sched(struct smc_link_group *lgr)
>>   }
>>   
>>   /* Called when peer lgr shutdown (regularly or abnormally) is received */
>> -void smc_smcd_terminate(struct smcd_dev *dev, u64 peer_gid, unsigned short vlan)
>> +void smc_smcd_terminate(struct smcd_dev *dev, struct smcd_gid *peer_gid,
>> +			unsigned short vlan)
>>   {
>>   	struct smc_link_group *lgr, *l;
>>   	LIST_HEAD(lgr_free_list);
>> @@ -1522,9 +1527,12 @@ void smc_smcd_terminate(struct smcd_dev *dev, u64 peer_gid, unsigned short vlan)
>>   	/* run common cleanup function and build free list */
>>   	spin_lock_bh(&dev->lgr_lock);
>>   	list_for_each_entry_safe(lgr, l, &dev->lgr_list, list) {
>> -		if ((!peer_gid || lgr->peer_gid == peer_gid) &&
>> +		if ((!peer_gid->gid ||
>> +		     (lgr->peer_gid.gid == peer_gid->gid &&
>> +		      !smc_ism_is_virtual(dev) ? 1 :
>> +		      lgr->peer_gid.gid_ext == peer_gid->gid_ext)) &&
>>   		    (vlan == VLAN_VID_MASK || lgr->vlan_id == vlan)) {
>> -			if (peer_gid) /* peer triggered termination */
>> +			if (peer_gid->gid) /* peer triggered termination */
>>   				lgr->peer_shutdown = 1;
>>   			list_move(&lgr->list, &lgr_free_list);
>>   			lgr->freeing = 1;
>> @@ -1859,10 +1867,12 @@ static bool smcr_lgr_match(struct smc_link_group *lgr, u8 smcr_version,
>>   	return false;
>>   }
>>   
>> -static bool smcd_lgr_match(struct smc_link_group *lgr,
>> -			   struct smcd_dev *smcismdev, u64 peer_gid)
>> +static bool smcd_lgr_match(struct smc_link_group *lgr, struct smcd_dev *smcismdev,
>> +			   struct smcd_gid *peer_gid)
>>   {
>> -	return lgr->peer_gid == peer_gid && lgr->smcd == smcismdev;
>> +	return lgr->peer_gid.gid == peer_gid->gid && lgr->smcd == smcismdev &&
>> +		smc_ism_is_virtual(smcismdev) ?
>> +		(lgr->peer_gid.gid_ext == peer_gid->gid_ext) : 1;
>>   }
>>   
>>   /* create a new SMC connection (and a new link group if necessary) */
>> @@ -1892,7 +1902,7 @@ int smc_conn_create(struct smc_sock *smc, struct smc_init_info *ini)
>>   		write_lock_bh(&lgr->conns_lock);
>>   		if ((ini->is_smcd ?
>>   		     smcd_lgr_match(lgr, ini->ism_dev[ini->ism_selected],
>> -				    ini->ism_peer_gid[ini->ism_selected]) :
>> +				    &ini->ism_peer_gid[ini->ism_selected]) :
>>   		     smcr_lgr_match(lgr, ini->smcr_version,
>>   				    ini->peer_systemid,
>>   				    ini->peer_gid, ini->peer_mac, role,
> [...]
>> diff --git a/net/smc/smc_diag.c b/net/smc/smc_diag.c
>> index a584613..c180c18 100644
>> --- a/net/smc/smc_diag.c
>> +++ b/net/smc/smc_diag.c
>> @@ -21,6 +21,7 @@
>>   
>>   #include "smc.h"
>>   #include "smc_core.h"
>> +#include "smc_ism.h"
>>   
>>   struct smc_diag_dump_ctx {
>>   	int pos[2];
>> @@ -168,12 +169,14 @@ static int __smc_diag_dump(struct sock *sk, struct sk_buff *skb,
>>   		struct smc_connection *conn = &smc->conn;
>>   		struct smcd_diag_dmbinfo dinfo;
>>   		struct smcd_dev *smcd = conn->lgr->smcd;
>> +		struct smcd_gid smcd_gid;
>>   
>>   		memset(&dinfo, 0, sizeof(dinfo));
>>   
>>   		dinfo.linkid = *((u32 *)conn->lgr->id);
>> -		dinfo.peer_gid = conn->lgr->peer_gid;
>> -		dinfo.my_gid = smcd->ops->get_local_gid(smcd);
>> +		dinfo.peer_gid = conn->lgr->peer_gid.gid;
>> +		smcd->ops->get_local_gid(smcd, &smcd_gid);
>> +		dinfo.my_gid = smcd_gid.gid;
> 
> For virtual ism, you will only see the first half of the GID.
> Is that acceptable?
> 
>>   		dinfo.token = conn->rmb_desc->token;
>>   		dinfo.peer_token = conn->peer_token;
>>   
>> diff --git a/net/smc/smc_ism.c b/net/smc/smc_ism.c
>> index fbee249..a33f861 100644
>> --- a/net/smc/smc_ism.c
>> +++ b/net/smc/smc_ism.c
> 
> Some of the content of this file is specific to s390 firmware ISMs and some is
> relevant to all future ism devices.
> IMO there is some more work to do to create a clean "smcd-protocol to scmd-device" interface.
> Maybe also some moving between this file and drivers/s390/net/ism_drv.c
> 
> Maybe this would be a good next patchset?
> 

I see the need, too. However, there are bunch of things to do in order 
to improve the SMC code. We need to get our priorities straight. What 
clear is that this is not in the scope of this patchset ;-)

> Whoever takes this work, remember:
> https://lore.kernel.org/netdev/1c6bdfbf-54c1-4251-916e-9a703a9f644c@infradead.org/T/
> We want to be able to combine SMC, ISM and future kernel modules in any combination.
> Gerd's patch above was meant to solve the current problem. For additional ism devices,
> we need some more improvements, I think.
> 
> 
Thank you, Alexandra, for the remember! That we should keep in mind!
> 
> 
> 
>> @@ -44,7 +44,8 @@ static void smcd_handle_irq(struct ism_dev *ism, unsigned int dmbno,
>>   #endif
>>   
>>   /* Test if an ISM communication is possible - same CPC */
>> -int smc_ism_cantalk(u64 peer_gid, unsigned short vlan_id, struct smcd_dev *smcd)
>> +int smc_ism_cantalk(struct smcd_gid *peer_gid, unsigned short vlan_id,
>> +		    struct smcd_dev *smcd)
>>   {
>>   	return smcd->ops->query_remote_gid(smcd, peer_gid, vlan_id ? 1 : 0,
>>   					   vlan_id);
>> @@ -208,7 +209,7 @@ int smc_ism_register_dmb(struct smc_link_group *lgr, int dmb_len,
>>   	dmb.dmb_len = dmb_len;
>>   	dmb.sba_idx = dmb_desc->sba_idx;
>>   	dmb.vlan_id = lgr->vlan_id;
>> -	dmb.rgid = lgr->peer_gid;
>> +	dmb.rgid = lgr->peer_gid.gid;
>>   	rc = lgr->smcd->ops->register_dmb(lgr->smcd, &dmb, &smc_ism_client);
>>   	if (!rc) {
>>   		dmb_desc->sba_idx = dmb.sba_idx;
>> @@ -340,18 +341,20 @@ struct smc_ism_event_work {
>>   
>>   static void smcd_handle_sw_event(struct smc_ism_event_work *wrk)
>>   {
>> +	struct smcd_gid peer_gid = { .gid = wrk->event.tok,
>> +				     .gid_ext = 0 };
>>   	union smcd_sw_event_info ev_info;
>>   
>>   	ev_info.info = wrk->event.info;
>>   	switch (wrk->event.code) {
>>   	case ISM_EVENT_CODE_SHUTDOWN:	/* Peer shut down DMBs */
>> -		smc_smcd_terminate(wrk->smcd, wrk->event.tok, ev_info.vlan_id);
>> +		smc_smcd_terminate(wrk->smcd, &peer_gid, ev_info.vlan_id);
>>   		break;
>>   	case ISM_EVENT_CODE_TESTLINK:	/* Activity timer */
>>   		if (ev_info.code == ISM_EVENT_REQUEST) {
>>   			ev_info.code = ISM_EVENT_RESPONSE;
>>   			wrk->smcd->ops->signal_event(wrk->smcd,
>> -						     wrk->event.tok,
>> +						     &peer_gid,
>>   						     ISM_EVENT_REQUEST_IR,
>>   						     ISM_EVENT_CODE_TESTLINK,
>>   						     ev_info.info);
>> @@ -365,10 +368,12 @@ static void smc_ism_event_work(struct work_struct *work)
>>   {
>>   	struct smc_ism_event_work *wrk =
>>   		container_of(work, struct smc_ism_event_work, work);
>> +	struct smcd_gid smcd_gid = { .gid = wrk->event.tok,
>> +				     .gid_ext = 0 };
>>   
>>   	switch (wrk->event.type) {
>>   	case ISM_EVENT_GID:	/* GID event, token is peer GID */
>> -		smc_smcd_terminate(wrk->smcd, wrk->event.tok, VLAN_VID_MASK);
>> +		smc_smcd_terminate(wrk->smcd, &smcd_gid, VLAN_VID_MASK);
>>   		break;
>>   	case ISM_EVENT_DMB:
>>   		break;
>> @@ -525,7 +530,7 @@ int smc_ism_signal_shutdown(struct smc_link_group *lgr)
>>   	memcpy(ev_info.uid, lgr->id, SMC_LGR_ID_SIZE);
>>   	ev_info.vlan_id = lgr->vlan_id;
>>   	ev_info.code = ISM_EVENT_REQUEST;
>> -	rc = lgr->smcd->ops->signal_event(lgr->smcd, lgr->peer_gid,
>> +	rc = lgr->smcd->ops->signal_event(lgr->smcd, &lgr->peer_gid,
>>   					  ISM_EVENT_REQUEST_IR,
>>   					  ISM_EVENT_CODE_SHUTDOWN,
>>   					  ev_info.info);
> [...]