Re: [PATCH net-next v3 4/8] net: qualcomm: ipqess: Add Ethtool ops to IPQESS port netdevices

Simon Horman
2023-11-15T18:19:08Z

On Tue, Nov 14, 2023 at 11:55:54AM +0100, Romain Gantois wrote:
> The IPQESS driver registers one netdevice for each front-facing switch
> port. Add support for several ethtool operations to these netdevices.
>
> Signed-off-by: Romain Gantois <romain.gantois@bootlin.com>

Hi Romain,

some more minor feedback from my side.

> ---
> drivers/net/ethernet/qualcomm/ipqess/Makefile | 2 +-
> .../ethernet/qualcomm/ipqess/ipqess_ethtool.c | 245 ++++++++++++++++++
> .../ethernet/qualcomm/ipqess/ipqess_port.c | 1 +
> .../ethernet/qualcomm/ipqess/ipqess_port.h | 3 +
> 4 files changed, 250 insertions(+), 1 deletion(-)
> create mode 100644 drivers/net/ethernet/qualcomm/ipqess/ipqess_ethtool.c

...

> diff --git a/drivers/net/ethernet/qualcomm/ipqess/ipqess_ethtool.c b/drivers/net/ethernet/qualcomm/ipqess/ipqess_ethtool.c

...

> +static int ipqess_port_set_eee(struct net_device *dev, struct ethtool_eee *eee)
> +{
> + struct ipqess_port *port = netdev_priv(dev);
> + int ret;
> + u32 lpi_en = QCA8K_REG_EEE_CTRL_LPI_EN(port->index);
> + struct qca8k_priv *priv = port->sw->priv;
> + u32 lpi_ctl1;

nit: Please consider using reverse xmas tree - longest line to shortest -
for local variable declarations in networking code.

> +
> + /* Port's PHY and MAC both need to be EEE capable */
> + if (!dev->phydev || !port->pl)
> + return -ENODEV;
> +
> + mutex_lock(&priv->reg_mutex);
> + lpi_ctl1 = qca8k_read(priv, QCA8K_REG_EEE_CTRL, &lpi_ctl1);
> + if (lpi_ctl1 < 0) {

lpi_ctl1 is unsigned, it can never be less than zero.

As flagged by Smatch and Coccinelle.

I think this should probably be (completely untested!):

ret = qca8k_read(priv, QCA8K_REG_EEE_CTRL, &lpi_ctl1);
if (ret < 0) {

Which would also resolve the issue immediately below too.

> + mutex_unlock(&priv->reg_mutex);
> + return ret;

It seems that ret is used uninitialised here.

Flagged by clang-16 W=1 builds.

> + }

...