Received-SPF: pass (google.com: domain of linux-kernel+bounces-112554-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Precedence: bulk
MIME-Version: 1.0
References: <20240310-realtek-led-v1-0-4d9813ce938e@gmail.com>
 <20240310-realtek-led-v1-4-4d9813ce938e@gmail.com> <20240310114928.GB1623@kernel.org>
In-Reply-To: <20240310114928.GB1623@kernel.org>
From: Luiz Angelo Daros de Luca <luizluca@gmail.com>
Date: Sat, 23 Mar 2024 23:31:21 -0300
Message-ID: <CAJq09z5n8iRe4Qt=qMFU6064ci6WavKcx8_D6Dx=hLst_vXHrw@mail.gmail.com>
Subject: Re: [PATCH net-next 4/4] net: dsa: realtek: add LED drivers for rtl8366rb
To: Simon Horman <horms@kernel.org>
Cc: Linus Walleij <linus.walleij@linaro.org>, =?UTF-8?Q?Alvin_=C5=A0ipraga?= <alsi@bang-olufsen.dk>, 
	Andrew Lunn <andrew@lunn.ch>, Florian Fainelli <f.fainelli@gmail.com>, 
	Vladimir Oltean <olteanv@gmail.com>, "David S. Miller" <davem@davemloft.net>, 
	Eric Dumazet <edumazet@google.com>, Jakub Kicinski <kuba@kernel.org>, Paolo Abeni <pabeni@redhat.com>, 
	netdev@vger.kernel.org, linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"

> On Sun, Mar 10, 2024 at 01:52:01AM -0300, Luiz Angelo Daros de Luca wrote:
> > This commit introduces LED drivers for rtl8366rb, enabling LEDs to be
> > described in the device tree using the same format as qca8k. Each port
> > can configure up to 4 LEDs.
> >
> > If all LEDs in a group use the default state "keep", they will use the
> > default behavior after a reset. Changing the brightness of one LED,
> > either manually or by a trigger, will disable the default hardware
> > trigger and switch the entire LED group to manually controlled LEDs.
> > Once in this mode, there is no way to revert to hardware-controlled LEDs
> > (except by resetting the switch).
> >
> > Software triggers function as expected with manually controlled LEDs.
> >
> > Signed-off-by: Luiz Angelo Daros de Luca <luizluca@gmail.com>
> > Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
> > ---
> >  drivers/net/dsa/realtek/rtl8366rb.c | 270 ++++++++++++++++++++++++++++++++----
>
> ...
>
> > +static int rtl8366rb_setup_led(struct realtek_priv *priv, struct dsa_port *dp,
> > +                            struct fwnode_handle *led_fwnode)
> > +{
> > +     struct rtl8366rb *rb = priv->chip_data;
> > +     struct led_init_data init_data = { };
> > +     struct rtl8366rb_led *led;
> > +     enum led_default_state state;
> > +     u32 led_group;
> > +     int ret;
>
> nit: Please consider using reverse xmas tree - longest line to shortest -
>      for local variables in networking code.

Sorry, I might have renamed a variable to a shorter form without
rechecking the order. Thanks for the tip.

> ...
>
> > +static int rtl8366rb_setup_leds(struct realtek_priv *priv)
> > +{
> > +     struct device_node *leds_np, *led_np;
> > +     struct dsa_switch *ds = &priv->ds;
> > +     struct dsa_port *dp;
> > +     int ret;
> > +
> > +     dsa_switch_for_each_port(dp, ds) {
> > +             if (!dp->dn)
> > +                     continue;
> > +
> > +             leds_np = of_get_child_by_name(dp->dn, "leds");
> > +             if (!leds_np) {
> > +                     dev_dbg(priv->dev, "No leds defined for port %d",
> > +                             dp->index);
> > +                     continue;
> > +             }
> > +
> > +             for_each_child_of_node(leds_np, led_np) {
> > +                     ret = rtl8366rb_setup_led(priv, dp,
> > +                                               of_fwnode_handle(led_np));
> > +                     if (ret) {
> > +                             of_node_put(led_np);
>
> FWIIW, Coccinelle complans about "probable double put" here.
> But it looks correct to me as it's only called when breaking out of
> the loop, when it is required AFAIK.

I agree. I do think the put is required when you break the loop as the
put happens in the increment/decrement code, including the last one
when it naturally ends the loop with led_np defined as null.

>
> > +                             break;
> > +                     }
> > +             }
> > +
> > +             of_node_put(leds_np);
> > +             if (ret)
>
> I'm unsure if this can happen. But if for_each_child_of_node()
> iterates zero times then ret may be uninitialised here.
>
> Flagged by Smatch.

Yes, I'll initialize it as 0. It could also use if (led_np), as it
will only be defined if the loop was broken (but checking ret seems to
be simpler).

>
> > +                     return ret;
> > +     }
> > +     return 0;
> > +}
>
> ...