Re: [PATCH net-next] net: mctp: Expose transport binding identifier via IFLA attribute

[Matt Johnston <matt@codeconstruct.com.au>]

Thanks Khang, this looks good.

On Tue, 2024-11-05 at 14:19 +0700, Khang Nguyen wrote:
> MCTP control protocol implementations are transport binding dependent.
> Endpoint discovery is mandatory based on transport binding.
> Message timing requirements are specified in each respective transport
> binding specification.
> 
> However, we currently have no means to get this information from MCTP
> links.
> 
> Add a IFLA_MCTP_PHYS_BINDING netlink link attribute, which represents
> the transport type using the DMTF DSP0239-defined type numbers, returned
> as part of RTM_GETLINK data.
> 
> We get an IFLA_MCTP_PHYS_BINDING attribute for each MCTP link, for
> example:
> 
> - 0x00 (unspec) for loopback interface;
> - 0x01 (SMBus/I2C) for mctpi2c%d interfaces; and
> - 0x05 (serial) for mctpserial%d interfaces.
> 
> Signed-off-by: Khang Nguyen <khangng@os.amperecomputing.com>
> ---
>  drivers/net/mctp/mctp-i2c.c    |  3 ++-
>  drivers/net/mctp/mctp-i3c.c    |  2 +-
>  drivers/net/mctp/mctp-serial.c |  5 +++--
>  include/net/mctp.h             | 18 ++++++++++++++++++
>  include/net/mctpdevice.h       |  4 +++-
>  include/uapi/linux/if_link.h   |  1 +
>  net/mctp/device.c              | 12 +++++++++---
>  7 files changed, 37 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/net/mctp/mctp-i2c.c b/drivers/net/mctp/mctp-i2c.c
> index 4dc057c121f5..86151a03570e 100644
> --- a/drivers/net/mctp/mctp-i2c.c
> +++ b/drivers/net/mctp/mctp-i2c.c
> @@ -877,7 +877,8 @@ static int mctp_i2c_add_netdev(struct mctp_i2c_client *mcli,
>  		goto err;
>  	}
>  
> -	rc = mctp_register_netdev(ndev, &mctp_i2c_mctp_ops);
> +	rc = mctp_register_netdev(ndev, &mctp_i2c_mctp_ops,
> +				  MCTP_PHYS_BINDING_SMBUS);
>  	if (rc < 0) {
>  		dev_err(&mcli->client->dev,
>  			"register netdev \"%s\" failed %d\n",
> diff --git a/drivers/net/mctp/mctp-i3c.c b/drivers/net/mctp/mctp-i3c.c
> index 1bc87a062686..9adad59b8676 100644
> --- a/drivers/net/mctp/mctp-i3c.c
> +++ b/drivers/net/mctp/mctp-i3c.c
> @@ -607,7 +607,7 @@ __must_hold(&busdevs_lock)
>  		goto err_free_uninit;
>  	}
>  
> -	rc = mctp_register_netdev(ndev, NULL);
> +	rc = mctp_register_netdev(ndev, NULL, MCTP_PHYS_BINDING_I3C);
>  	if (rc < 0) {
>  		dev_warn(&ndev->dev, "netdev register failed: %d\n", rc);
>  		goto err_free_netdev;
> diff --git a/drivers/net/mctp/mctp-serial.c b/drivers/net/mctp/mctp-serial.c
> index e63720ec3238..26c9a33fd636 100644
> --- a/drivers/net/mctp/mctp-serial.c
> +++ b/drivers/net/mctp/mctp-serial.c
> @@ -23,6 +23,7 @@
>  
>  #include <linux/mctp.h>
>  #include <net/mctp.h>
> +#include <net/mctpdevice.h>
>  #include <net/pkt_sched.h>
>  
>  #define MCTP_SERIAL_MTU		68 /* base mtu (64) + mctp header */
> @@ -470,7 +471,7 @@ static int mctp_serial_open(struct tty_struct *tty)
>  	spin_lock_init(&dev->lock);
>  	INIT_WORK(&dev->tx_work, mctp_serial_tx_work);
>  
> -	rc = register_netdev(ndev);
> +	rc = mctp_register_netdev(ndev, NULL, MCTP_PHYS_BINDING_SERIAL);
>  	if (rc)
>  		goto free_netdev;
>  
> @@ -492,7 +493,7 @@ static void mctp_serial_close(struct tty_struct *tty)
>  	struct mctp_serial *dev = tty->disc_data;
>  	int idx = dev->idx;
>  
> -	unregister_netdev(dev->netdev);
> +	mctp_unregister_netdev(dev->netdev);
>  	ida_free(&mctp_serial_ida, idx);
>  }
>  
> diff --git a/include/net/mctp.h b/include/net/mctp.h
> index 28d59ae94ca3..1ecbff7116f6 100644
> --- a/include/net/mctp.h
> +++ b/include/net/mctp.h
> @@ -298,4 +298,22 @@ void mctp_routes_exit(void);
>  int mctp_device_init(void);
>  void mctp_device_exit(void);
>  
> +/* MCTP IDs and Codes from DMTF specification
> + * "DSP0239 Management Component Transport Protocol (MCTP) IDs and Codes"
> + * https://www.dmtf.org/sites/default/files/standards/documents/DSP0239_1.11.1.pdf
> + */
> +enum mctp_phys_binding {
> +	MCTP_PHYS_BINDING_UNSPEC	= 0x00,
> +	MCTP_PHYS_BINDING_SMBUS		= 0x01,
> +	MCTP_PHYS_BINDING_PCIE_VDM	= 0x02,
> +	MCTP_PHYS_BINDING_USB		= 0x03,
> +	MCTP_PHYS_BINDING_KCS		= 0x04,
> +	MCTP_PHYS_BINDING_SERIAL	= 0x05,
> +	MCTP_PHYS_BINDING_I3C		= 0x06,
> +	MCTP_PHYS_BINDING_MMBI		= 0x07,
> +	MCTP_PHYS_BINDING_PCC		= 0x08,
> +	MCTP_PHYS_BINDING_UCIE		= 0x09,
> +	MCTP_PHYS_BINDING_VENDOR	= 0xFF,
> +};
> +
>  #endif /* __NET_MCTP_H */
> diff --git a/include/net/mctpdevice.h b/include/net/mctpdevice.h
> index 5c0d04b5c12c..957d9ef924c5 100644
> --- a/include/net/mctpdevice.h
> +++ b/include/net/mctpdevice.h
> @@ -22,6 +22,7 @@ struct mctp_dev {
>  	refcount_t		refs;
>  
>  	unsigned int		net;
> +	enum mctp_phys_binding	binding;
>  
>  	const struct mctp_netdev_ops *ops;
>  
> @@ -44,7 +45,8 @@ struct mctp_dev *mctp_dev_get_rtnl(const struct net_device *dev);
>  struct mctp_dev *__mctp_dev_get(const struct net_device *dev);
>  
>  int mctp_register_netdev(struct net_device *dev,
> -			 const struct mctp_netdev_ops *ops);
> +			 const struct mctp_netdev_ops *ops,
> +			 enum mctp_phys_binding binding);
>  void mctp_unregister_netdev(struct net_device *dev);
>  
>  void mctp_dev_hold(struct mctp_dev *mdev);
> diff --git a/include/uapi/linux/if_link.h b/include/uapi/linux/if_link.h
> index 8516c1ccd57a..2575e0cd9b48 100644
> --- a/include/uapi/linux/if_link.h
> +++ b/include/uapi/linux/if_link.h
> @@ -1958,6 +1958,7 @@ struct ifla_rmnet_flags {
>  enum {
>  	IFLA_MCTP_UNSPEC,
>  	IFLA_MCTP_NET,
> +	IFLA_MCTP_PHYS_BINDING,
>  	__IFLA_MCTP_MAX,
>  };
>  
> diff --git a/net/mctp/device.c b/net/mctp/device.c
> index 3d75b919995d..26ce34b7e88e 100644
> --- a/net/mctp/device.c
> +++ b/net/mctp/device.c
> @@ -371,6 +371,8 @@ static int mctp_fill_link_af(struct sk_buff *skb,
>  		return -ENODATA;
>  	if (nla_put_u32(skb, IFLA_MCTP_NET, mdev->net))
>  		return -EMSGSIZE;
> +	if (nla_put_u8(skb, IFLA_MCTP_PHYS_BINDING, mdev->binding))
> +		return -EMSGSIZE;
>  	return 0;
>  }
>  
> @@ -385,6 +387,7 @@ static size_t mctp_get_link_af_size(const struct net_device *dev,
>  	if (!mdev)
>  		return 0;
>  	ret = nla_total_size(4); /* IFLA_MCTP_NET */
> +	ret += nla_total_size(1); /* IFLA_MCTP_PHYS_BINDING */
>  	mctp_dev_put(mdev);
>  	return ret;
>  }
> @@ -480,7 +483,8 @@ static int mctp_dev_notify(struct notifier_block *this, unsigned long event,
>  }
>  
>  static int mctp_register_netdevice(struct net_device *dev,
> -				   const struct mctp_netdev_ops *ops)
> +				   const struct mctp_netdev_ops *ops,
> +				   enum mctp_phys_binding binding)
>  {
>  	struct mctp_dev *mdev;
>  
> @@ -489,17 +493,19 @@ static int mctp_register_netdevice(struct net_device *dev,
>  		return PTR_ERR(mdev);
>  
>  	mdev->ops = ops;
> +	mdev->binding = binding;
>  
>  	return register_netdevice(dev);
>  }
>  
>  int mctp_register_netdev(struct net_device *dev,
> -			 const struct mctp_netdev_ops *ops)
> +			 const struct mctp_netdev_ops *ops,
> +			 enum mctp_phys_binding binding)
>  {
>  	int rc;
>  
>  	rtnl_lock();
> -	rc = mctp_register_netdevice(dev, ops);
> +	rc = mctp_register_netdevice(dev, ops, binding);
>  	rtnl_unlock();
>  
>  	return rc;

Reviewed-by: Matt Johnston <matt@codeconstruct.com.au>