> >> +static int
> >> +ax88796c_set_tunable(struct net_device *ndev, const struct ethtool_tunable *tuna,
> >> + const void *data)
> >> +{
> >> + struct ax88796c_device *ax_local = to_ax88796c_device(ndev);
> >> +
> >> + switch (tuna->id) {
> >> + case ETHTOOL_SPI_COMPRESSION:
> >> + if (netif_running(ndev))
> >> + return -EBUSY;
> >> + ax_local->capabilities &= ~AX_CAP_COMP;
> >> + ax_local->capabilities |= *(u32 *)data ? AX_CAP_COMP : 0;
> >
> > You should probably validate here that data is 0 or 1. That is what
> > ax88796c_get_tunable() will return.
> >
> > It seems like this controls two hardware bits:
> >
> > SPICR_RCEN | SPICR_QCEN
> >
> > Maybe at some point it would make sense to allow these bits to be set
> > individually? If you never validate the tunable, you cannot make use
> > of other values to control the bits individually.
>
> Good point. What is your recommendation for the userland facing
> interface, so that future changes will be least disruptive?
I would KISS and just validate data is 0 or 1, and leave it at that.
That then later gives you the option to have other values, if that is
ever interesting.
Andrew
