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@xxxxxxxxxxx>
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.
> + }
...
