| Message ID | 20241105071915.821871-1-khangng@os.amperecomputing.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 580db513b4a9d52f306580015a1872eea0a0894e |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next] net: mctp: Expose transport binding identifier via IFLA attribute | expand |
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>
On Tue, 5 Nov 2024 14:19:15 +0700 Khang Nguyen wrote: > However, we currently have no means to get this information from MCTP > links. I'm not opposed to the netlink attribute, but to be clear this info is indirectly available in sysfs, right? We link the netdev to the parent device so the type of /sys/class/net/$your_ifc/device should reveal what the transport is?
On 08/11/2024 11:41, Jakub Kicinski wrote: > On Tue, 5 Nov 2024 14:19:15 +0700 Khang Nguyen wrote: >> However, we currently have no means to get this information from MCTP >> links. > > I'm not opposed to the netlink attribute, but to be clear this info > is indirectly available in sysfs, right? We link the netdev to > the parent device so the type of /sys/class/net/$your_ifc/device > should reveal what the transport is? Good point, I did not think about using the parent device, that would be a good workaround for the currently supported interfaces. For the long term, we should still need the attribute. For example, vendor-defined transports need their 0xFF code, which cannot be derived anywhere. Or binding implementations that have parent SoC platform devices from the device tree, which do not always have a clear type shown in the sysfs... (The MCTP-over-serial binding also does not have a parent device currently, but I believe we can fix that if necessary)
Hi Jakub, > > However, we currently have no means to get this information from > > MCTP links. > > I'm not opposed to the netlink attribute, but to be clear this info > is indirectly available in sysfs, right? We link the netdev to > the parent device so the type of /sys/class/net/$your_ifc/device > should reveal what the transport is? It's likely derivable from the parent device, but requires some heuristics in userspace to map this to a transport type. Having a well-defined place to provide the DMTF-specified transport identifier makes this a little more straightforward to determine which spec we're dealing with, for any transport-specific behaviour. For example, some bus types require endpoints to announce their presence, others do not. Cheers, Jeremy
Hello: This patch was applied to netdev/net-next.git (main) by Jakub Kicinski <kuba@kernel.org>: On Tue, 5 Nov 2024 14:19:15 +0700 you 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. > > [...] Here is the summary with links: - [net-next] net: mctp: Expose transport binding identifier via IFLA attribute https://git.kernel.org/netdev/net-next/c/580db513b4a9 You are awesome, thank you!
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;
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(-)