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

[Jakub Kicinski <jakub.kicinski@netronome.com>]

On Tue, 15 Jan 2019 11:18:18 +0100, Jiri Pirko wrote:
> 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.

Ack, I will add those as defines in the devlink header plus some docs.
Let's see how it goes..

> >+};
> >+
> >+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.

Tmp var makes it look more like the other function (from next patch)...
and it makes the line fit in 80 char ;)