Re: [PATCH v5 3/5] net: ax88796c: ASIX AX88796C SPI Ethernet Adapter Driver

quoted

quoted

+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