Re: [RFC net-next 4/6] nfp: devlink: report fixed versions

[Jiri Pirko <jiri@resnulli.us>]

Tue, Jan 15, 2019 at 01:50:06AM CET, jakub.kicinski@netronome.com wrote:
>For now we only use HWinfo for fixed versions.
>
>Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
>---
> .../net/ethernet/netronome/nfp/nfp_devlink.c  | 72 +++++++++++++++++++
> 1 file changed, 72 insertions(+)
>
>diff --git a/drivers/net/ethernet/netronome/nfp/nfp_devlink.c b/drivers/net/ethernet/netronome/nfp/nfp_devlink.c
>index 4422da939937..c7759564316d 100644
>--- a/drivers/net/ethernet/netronome/nfp/nfp_devlink.c
>+++ b/drivers/net/ethernet/netronome/nfp/nfp_devlink.c
>@@ -192,6 +192,77 @@ nfp_devlink_serial_get(struct devlink *devlink, u8 *buf, size_t buf_len,
> 	return 0;
> }
> 
>+static const struct nfp_devlink_versions_simple {
>+	const char *key;
>+	const char *hwinfo;
>+} nfp_devlink_versions_hwinfo[] = {
>+	{ "board.model",	"assembly.model", },
>+	{ "board.partno",	"assembly.partno", },
>+	{ "board.revision",	"assembly.revision", },
>+	{ "board.vendor",	"assembly.vendor", },

That means that every driver would re-define these strings. Would it
make sense to have them (at least some of them) defined in a generic
way? Something in a sense of devlink params, where we have generic and
driver-specific ones.


>+};
>+
>+static int
>+nfp_devlink_versions_get_hwinfo(struct sk_buff *skb, struct nfp_nsp *nsp,
>+				struct netlink_ext_ack *extack)
>+{
>+	unsigned int i;
>+	int attr;
>+	int err;
>+
>+	attr = DEVLINK_ATTR_INFO_VERSIONS_FIXED;
>+
>+	for (i = 0; i < ARRAY_SIZE(nfp_devlink_versions_hwinfo); i++) {
>+		const struct nfp_devlink_versions_simple *info;
>+		char buf[256] = {};
>+
>+		info = &nfp_devlink_versions_hwinfo[i];
>+		strcpy(buf, info->hwinfo);
>+
>+		err = nfp_nsp_hwinfo_lookup(nsp, buf, sizeof(buf));
>+		if (err == -ENOENT)
>+			continue;
>+		if (err) {
>+			NL_SET_ERR_MSG_MOD(extack,
>+					   "error reading versions string from FW");
>+			return err;
>+		}
>+
>+		err = devlink_versions_report(skb, attr, info->key, buf);

So this is always "DEVLINK_ATTR_INFO_VERSIONS_FIXED". I don't
understand.


>+		if (err)
>+			return err;
>+	}
>+
>+	return 0;
>+}
>+
>+static int
>+nfp_devlink_versions_get(struct devlink *devlink, struct sk_buff *skb,
>+			 struct netlink_ext_ack *extack)
>+{
>+	struct nfp_pf *pf = devlink_priv(devlink);
>+	struct nfp_nsp *nsp;
>+	int err;
>+
>+	nsp = nfp_nsp_open(pf->cpp);
>+	if (IS_ERR(nsp)) {
>+		NL_SET_ERR_MSG_MOD(extack, "can't access NSP");
>+		return PTR_ERR(nsp);
>+	}
>+
>+	err = nfp_devlink_versions_get_hwinfo(skb, nsp, extack);
>+	if (err)
>+		goto err_close_nsp;
>+
>+	nfp_nsp_close(nsp);
>+
>+	return 0;
>+
>+err_close_nsp:
>+	nfp_nsp_close(nsp);
>+	return err;
>+}
>+
> const struct devlink_ops nfp_devlink_ops = {
> 	.port_split		= nfp_devlink_port_split,
> 	.port_unsplit		= nfp_devlink_port_unsplit,
>@@ -200,6 +271,7 @@ const struct devlink_ops nfp_devlink_ops = {
> 	.eswitch_mode_get	= nfp_devlink_eswitch_mode_get,
> 	.eswitch_mode_set	= nfp_devlink_eswitch_mode_set,
> 	.serial_get		= nfp_devlink_serial_get,
>+	.versions_get		= nfp_devlink_versions_get,
> };
> 
> int nfp_devlink_port_register(struct nfp_app *app, struct nfp_port *port)
>-- 
>2.19.2
>