Re: [PATCH net-next v3 01/11] net/smc: adapt SMC-D device dump for Emulated-ISM

[Jan Karcher]

On 12/03/2024 15:27, Wen Gu wrote:
> The introduction of Emulated-ISM requires adaptation of SMC-D device
> dump. Software implemented non-PCI device (loopback-ism) should be
> handled correctly and the CHID reserved for Emulated-ISM should be got
> from smcd_ops interface instead of PCI information.
>
> Signed-off-by: Wen Gu <guwen@xxxxxxxxxxxxxxxxx>
> ---
>   net/smc/smc_ism.c | 13 ++++++++++---
>   1 file changed, 10 insertions(+), 3 deletions(-)
>
> diff --git a/net/smc/smc_ism.c b/net/smc/smc_ism.c
> index ac88de2a06a0..b6eca4231913 100644
> --- a/net/smc/smc_ism.c
> +++ b/net/smc/smc_ism.c
> @@ -252,12 +252,11 @@ static int smc_nl_handle_smcd_dev(struct smcd_dev *smcd,
>   	char smc_pnet[SMC_MAX_PNETID_LEN + 1];
>   	struct smc_pci_dev smc_pci_dev;
>   	struct nlattr *port_attrs;
> +	struct device *device;
>   	struct nlattr *attrs;
> -	struct ism_dev *ism;
>   	int use_cnt = 0;
>   	void *nlh;
>   
> -	ism = smcd->priv;
>   	nlh = genlmsg_put(skb, NETLINK_CB(cb->skb).portid, cb->nlh->nlmsg_seq,
>   			  &smc_gen_nl_family, NLM_F_MULTI,
>   			  SMC_NETLINK_GET_DEV_SMCD);
> @@ -272,7 +271,15 @@ static int smc_nl_handle_smcd_dev(struct smcd_dev *smcd,
>   	if (nla_put_u8(skb, SMC_NLA_DEV_IS_CRIT, use_cnt > 0))
>   		goto errattr;
>   	memset(&smc_pci_dev, 0, sizeof(smc_pci_dev));
> -	smc_set_pci_values(to_pci_dev(ism->dev.parent), &smc_pci_dev);
> +	device = smcd->ops->get_dev(smcd);
> +	if (device->parent)
> +		smc_set_pci_values(to_pci_dev(device->parent), &smc_pci_dev);
> +	if (smc_ism_is_emulated(smcd)) {
> +		smc_pci_dev.pci_pchid = smc_ism_get_chid(smcd);
> +		if (!device->parent)
> +			snprintf(smc_pci_dev.pci_id, sizeof(smc_pci_dev.pci_id),
> +				 "%s", dev_name(device));
> +	}

Hi Wen Gu,

playing around with the loopback-ism device and testing it, i developed 
some concerns regarding this exposure. Basically this enables us to see 
the loopback device in the `smcd device` tool without any changes.
E.g.:
```
# smcd device
FID  Type  PCI-ID        PCHID  InUse  #LGs  PNET-ID
0000 0     loopback-ism  ffff   No        0
102a ISM   0000:00:00.0  07c2   No        0  NET1
```

Now the problem with this is that "loopback-ism" is no valid PCI-ID and 
should not be there. My first thought was to put an "n/a" instead, but 
that opens up another problem. Currently you could set - even if it does 
not make sense - a PNET_ID for the loopback device:
```
# smc_pnet -a -D loopback-ism NET1
# smcd device
FID  Type  PCI-ID        PCHID  InUse  #LGs  PNET-ID
0000 0     loopback-ism  ffff   No        0  *NET1
102a ISM   0000:00:00.0  07c2   No        0  NET1
```
If we would change the PCI-ID to "n/a" it would be a valid input 
parameter for the tooling which is... to put it nice... not so beautiful.
I brainstormed this with them team and the problem is more complex.
In theory there shouldn't be PCI information set for the loopback 
device. There should be a better abstraction in the netlink interface 
that creates this output and the tooling should be made aware of it.

Do you rely on the output currently? What are your thoughts about it?
If not I'd ask you to not fill the netlink interface for the loopback 
device and refactor it with the next stage when we create a right 
interface for it.

Since the Merge-Window is closed feel free to send new versions as RFC.
Thank you
- Jan

>   	if (nla_put_u32(skb, SMC_NLA_DEV_PCI_FID, smc_pci_dev.pci_fid))
>   		goto errattr;
>   	if (nla_put_u16(skb, SMC_NLA_DEV_PCI_CHID, smc_pci_dev.pci_pchid))