| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAJq09z7miTe7HUzsL4GBSwkrzyy4mVi6z40+ETgvmY=iWGRN-g@mail.gmail.com>
Date: Wed, 25 Oct 2023 15:30:27 -0300
From: Luiz Angelo Daros de Luca <luizluca@...il.com>
To: Vladimir Oltean <olteanv@...il.com>
Cc: netdev@...r.kernel.org, linus.walleij@...aro.org, alsi@...g-olufsen.dk,
andrew@...n.ch, vivien.didelot@...il.com, f.fainelli@...il.com,
davem@...emloft.net, kuba@...nel.org, pabeni@...hat.com, robh+dt@...nel.org,
krzk+dt@...nel.org, arinc.unal@...nc9.com
Subject: Re: [PATCH net-next 1/2] net: dsa: realtek: support reset controller
> On Tue, Oct 24, 2023 at 05:58:04PM -0300, Luiz Angelo Daros de Luca wrote:
> > The 'reset-gpios' will not work when the switch reset is controlled by a
> > reset controller.
> >
> > Although the reset is optional and the driver performs a soft reset
> > during setup, if the initial reset state was asserted, the driver will
> > not detect it.
> >
> > This is an example of how to use the reset controller:
> >
> > switch {
> > compatible = "realtek,rtl8366rb";
> >
> > resets = <&rst 8>;
> > reset-names = "switch";
> >
> > ...
> > }
>
> Mix of tabs and spaces here.
> Also, examples belong to the dt-schema.
OK
>
> >
> > The reset controller will take precedence over the reset GPIO.
> >
> > Signed-off-by: Luiz Angelo Daros de Luca <luizluca@...il.com>
> > ---
> > drivers/net/dsa/realtek/realtek-mdio.c | 36 +++++++++++++++++++++-----
> > drivers/net/dsa/realtek/realtek-smi.c | 34 +++++++++++++++++++-----
> > drivers/net/dsa/realtek/realtek.h | 6 +++++
> > 3 files changed, 63 insertions(+), 13 deletions(-)
> >
> > diff --git a/drivers/net/dsa/realtek/realtek-mdio.c b/drivers/net/dsa/realtek/realtek-mdio.c
> > index 292e6d087e8b..600124c58c00 100644
> > --- a/drivers/net/dsa/realtek/realtek-mdio.c
> > +++ b/drivers/net/dsa/realtek/realtek-mdio.c
> > @@ -140,6 +140,23 @@ static const struct regmap_config realtek_mdio_nolock_regmap_config = {
> > .disable_locking = true,
> > };
> >
> > +static int realtek_mdio_hwreset(struct realtek_priv *priv, bool active)
> > +{
> > +#ifdef CONFIG_RESET_CONTROLLER
> > + if (priv->reset_ctl) {
> > + if (active)
> > + return reset_control_assert(priv->reset_ctl);
> > + else
> > + return reset_control_deassert(priv->reset_ctl);
> > + }
> > +#endif
> > +
> > + if (priv->reset)
> > + gpiod_set_value(priv->reset, active);
> > +
> > + return 0;
> > +}
> > +
>
> This "bool active" artificially unifies two discrete code paths in the
> same function, where the callers are not the same and the implementation
> is not the same (given a priv->reset_ctl presence), separated by an "if".
>
> Would it make more sense to have discrete functions, each with its
> unique caller, like this?
>
> static int realtek_reset_assert(struct realtek_priv *priv)
> {
> if (priv->reset_ctl)
> return reset_control_assert(priv->reset_ctl);
>
> if (priv->reset)
> gpiod_set_value(priv->reset, 1);
>
> return 0;
> }
>
> static int realtek_reset_deassert(struct realtek_priv *priv)
> {
> if (priv->reset_ctl)
> return reset_control_deassert(priv->reset_ctl);
>
> if (priv->reset)
> gpiod_set_value(priv->reset, 0);
>
> return 0;
> }
Sure. It is better.
> Also, you return int but ignore error values everywhere. I guess it
> would make more sense to return void, but print warnings within the
> reset functions if the calls to the reset control fail.
>
> > static int realtek_mdio_probe(struct mdio_device *mdiodev)
> > {
> > struct realtek_priv *priv;
> > @@ -194,20 +211,26 @@ static int realtek_mdio_probe(struct mdio_device *mdiodev)
> >
> > dev_set_drvdata(dev, priv);
> >
> > - /* TODO: if power is software controlled, set up any regulators here */
>
> I'm not sure if "power" and "reset" are the same thing...
>
> > priv->leds_disabled = of_property_read_bool(np, "realtek,disable-leds");
> >
> > +#ifdef CONFIG_RESET_CONTROLLER
> > + priv->reset_ctl = devm_reset_control_get(dev, "switch");
> > + if (IS_ERR(priv->reset_ctl)) {
> > + dev_err(dev, "failed to get switch reset control\n");
> > + return PTR_ERR(priv->reset_ctl);
>
> ret = PTR_ERR(priv->reset_ctl);
> return dev_err_probe(dev, err, "failed to get reset control\n");
>
> This suppresses -EPROBE_DEFER prints.
OK
>
> > + }
> > +#endif
> > +
> > priv->reset = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_LOW);
> > if (IS_ERR(priv->reset)) {
> > dev_err(dev, "failed to get RESET GPIO\n");
> > return PTR_ERR(priv->reset);
> > }
> > -
> > - if (priv->reset) {
> > - gpiod_set_value(priv->reset, 1);
> > + if (priv->reset_ctl || priv->reset) {
> > + realtek_mdio_hwreset(priv, 1);
> > dev_dbg(dev, "asserted RESET\n");
> > msleep(REALTEK_HW_STOP_DELAY);
> > - gpiod_set_value(priv->reset, 0);
> > + realtek_mdio_hwreset(priv, 0);
> > msleep(REALTEK_HW_START_DELAY);
> > dev_dbg(dev, "deasserted RESET\n");
> > }
> > @@ -246,8 +269,7 @@ static void realtek_mdio_remove(struct mdio_device *mdiodev)
> > dsa_unregister_switch(priv->ds);
> >
> > /* leave the device reset asserted */
> > - if (priv->reset)
> > - gpiod_set_value(priv->reset, 1);
> > + realtek_mdio_hwreset(priv, 1);
>
> nitpick: "bool" arguments should take "true" or "false".
OK
>
> > }
> >
> > static void realtek_mdio_shutdown(struct mdio_device *mdiodev)
> > diff --git a/drivers/net/dsa/realtek/realtek-smi.c b/drivers/net/dsa/realtek/realtek-smi.c
> > index bfd11591faf4..751159d71223 100644
> > --- a/drivers/net/dsa/realtek/realtek-smi.c
> > +++ b/drivers/net/dsa/realtek/realtek-smi.c
> > @@ -408,6 +408,23 @@ static int realtek_smi_setup_mdio(struct dsa_switch *ds)
> > return ret;
> > }
> >
> > +static int realtek_smi_hwreset(struct realtek_priv *priv, bool active)
> > +{
> > +#ifdef CONFIG_RESET_CONTROLLER
> > + if (priv->reset_ctl) {
> > + if (active)
> > + return reset_control_assert(priv->reset_ctl);
> > + else
> > + return reset_control_deassert(priv->reset_ctl);
> > + }
> > +#endif
> > +
> > + if (priv->reset)
> > + gpiod_set_value(priv->reset, active);
> > +
> > + return 0;
> > +}
>
> What is the reason for duplicating realtek_mdio_hwreset()?
Both interface modules, realtek-smi and realtek-mdio, do not share
code, except for the realtek.h header file. I don't know if it is
worth it to put the code in a new shared module. What is the best
practice here? Create a realtek_common.c linked to both modules?
Regards,
Luiz
Powered by blists - more mailing lists