On Tue, Oct 24, 2017 at 5:22 PM, Yuval Mintz [off-list ref]
wrote:

quoted

quoted

Add support for permanent config parameter get/set commands. Used
for persistent device configuration parameters.

...

quoted

+     int (*perm_config_get)(struct devlink *devlink,
+                            enum devlink_perm_config_param param, u8
type,
+                            void *value);
+     int (*perm_config_set)(struct devlink *devlink,
+                            enum devlink_perm_config_param param, u8
type,
+                            void *value, u8 *restart_reqd);
 };
+static int devlink_nl_single_param_get(struct sk_buff *msg,
+                                    struct devlink *devlink,
+                                    u32 param, u8 type)
+{
+     const struct devlink_ops *ops = devlink->ops;
+     struct nlattr *param_attr;
+     void *value;
+     u32 val;
+     int err;
+
+     /* Allocate buffer for parameter value */
+     switch (type) {
+     case NLA_U8:
+             value = kmalloc(sizeof(u8), GFP_KERNEL);
+             break;
+     case NLA_U16:
+             value = kmalloc(sizeof(u16), GFP_KERNEL);
+             break;
+     case NLA_U32:
+             value = kmalloc(sizeof(u32), GFP_KERNEL);
+             break;
+     default:
+             return -EINVAL; /* Unsupported Type */
+     }
+
+     if (!value)
+             return -ENOMEM;
+
+     err = ops->perm_config_get(devlink, param, type, value);
+     if (err)
+             return err;

I suspect this logic might be risky - its dependent on the driver to cast the
'value' into the proper type or else, E.g., the following switch might break
for BE platforms.
Is there any reason to have the devlink <-> driver API be based on void*
and not on some typed data [of sufficient size]?
...

quoted

+     switch (type) {
+     case NLA_U8:
+             val = *((u8 *)value);
+             if (nla_put_u8(msg, DEVLINK_ATTR_PERM_CONFIG_VALUE,
val))
+                     goto nest_err;
+             break;
+     case NLA_U16:
+             val = *((u16 *)value);
+             if (nla_put_u16(msg,
DEVLINK_ATTR_PERM_CONFIG_VALUE, val))
+                     goto nest_err;
+             break;
+     case NLA_U32:
+             val = *((u32 *)value);
+             if (nla_put_u32(msg,
DEVLINK_ATTR_PERM_CONFIG_VALUE, val))
+                     goto nest_err;
+             break;
+     }

Why might this break on a BE system?  It's not as though driver will
be compiled LE and kernel BE or vice versa - as long as driver and
kernel are same endian-ness, I would think it should be okay?

It depends on the driver implementation to cast your pointer to the right type.
E.g., driver needs to fill in a u8 data in *value for a given parameter.
If the driver casted the pointer to (u8*) everything is fine. But if he casted
it to (u32*) [naïve implementation that doesn't care about the type]
and filled it, then on a LE machine value[0] would contain the data while on
BE value[3] would contain it.

In general, the issue is that the parameter could be any of the
netlink types (per Jiri's suggestion to the previous version of this
patch).  So, we allocate some space, tell the driver the type we're
expecting (the type argument to the perm_config_get() function), and
yes, we rely on the driver to write something of the type we request
to the pointer we provide.  Are you suggesting defining a union of
U8/U16/U32, and passing a pointer to that for the driver to fill in?

Problem is that the driver-side could always use the biggest data-type
as long as it's working on a LE machine, but that approach would break
if same driver would be tried on a BE machine.
And the developer would have no way of knowing other than via code-review.

The issue is that whatever the types we support now, we want future
parameters to be able to be of arbitrary types.  Defining the
interface to use the void pointer means that some future parameter can
be of some other type, without having to update all the drivers using
this API...

Or did I misunderstand your suggestion?