> On Tue, Oct 24, 2017 at 5:22 PM, Yuval Mintz <yuv...@mellanox.com>
> wrote:
> >> Add support for permanent config parameter get/set commands. Used
> >> for persistent device configuration parameters.
> >>
> > ...
> >> + 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]?
> > ...
> >> + 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?
