| 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: <20231030205025.b4dryzqzuunrjils@skbuf>
Date: Mon, 30 Oct 2023 22:50:25 +0200
From: Vladimir Oltean <olteanv@...il.com>
To: Luiz Angelo Daros de Luca <luizluca@...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 v2 3/3] net: dsa: realtek: support reset
controller
On Fri, Oct 27, 2023 at 04:00:57PM -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.
>
> 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 | 51 ++++++++++++++++++++++----
> drivers/net/dsa/realtek/realtek-smi.c | 49 ++++++++++++++++++++++---
> drivers/net/dsa/realtek/realtek.h | 2 +
> 3 files changed, 89 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/net/dsa/realtek/realtek-mdio.c b/drivers/net/dsa/realtek/realtek-mdio.c
> index 292e6d087e8b..aad94e49d4c9 100644
> --- a/drivers/net/dsa/realtek/realtek-mdio.c
> +++ b/drivers/net/dsa/realtek/realtek-mdio.c
> @@ -140,6 +140,40 @@ static const struct regmap_config realtek_mdio_nolock_regmap_config = {
> .disable_locking = true,
> };
>
> +static void realtek_mdio_reset_assert(struct realtek_priv *priv)
> +{
> + int ret;
> +
> + if (priv->reset_ctl) {
> + ret = reset_control_assert(priv->reset_ctl);
> + if (ret)
> + dev_warn(priv->dev, "Failed to assert the switch reset control. Error: %i",
> + ret);
Instead of "Error: %i" you can say ".. reset control: %pe\n", ERR_PTR(ret)
which will print the error as a symbolic error name (if CONFIG_SYMBOLIC_ERRNAME=y)
rather than just a numeric value.
Also, I don't know if this is explicit in the coding style, but I
believe it is more consistent if single function calls are enveloped in
curly braces if they span multiple lines, like so:
if (ret) {
dev_warn(priv->dev,
"Failed to assert the switch reset control: %pe",
ERR_PTR(ret));
}
Also, please note that netdev still prefers the 80 character line limit.
> +
> + return;
> + }
> +
> + if (priv->reset)
> + gpiod_set_value(priv->reset, true);
> +}
> +
> +static void realtek_mdio_reset_deassert(struct realtek_priv *priv)
> +{
> + int ret;
> +
> + if (priv->reset_ctl) {
> + ret = reset_control_deassert(priv->reset_ctl);
> + if (ret)
> + dev_warn(priv->dev, "Failed to deassert the switch reset control. Error: %i",
> + ret);
> +
> + return;
Is there a particular reason why this has to ignore a reset GPIO if
present, rather than fall through, checking for the latter as well?
> + }
> +
> + if (priv->reset)
> + gpiod_set_value(priv->reset, false);
> +}
> +
> static int realtek_mdio_probe(struct mdio_device *mdiodev)
> {
> struct realtek_priv *priv;
> @@ -194,20 +228,24 @@ 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 */
As Andrew mentions, this commit does not make power software-controlled,
so don't remove this.
> priv->leds_disabled = of_property_read_bool(np, "realtek,disable-leds");
>
> + priv->reset_ctl = devm_reset_control_get_optional(dev, NULL);
> + if (IS_ERR(priv->reset_ctl)) {
> + ret = PTR_ERR(priv->reset_ctl);
> + return dev_err_probe(dev, ret, "failed to get reset control\n");
> + }
> +
> 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_reset_assert(priv);
> dev_dbg(dev, "asserted RESET\n");
> msleep(REALTEK_HW_STOP_DELAY);
> - gpiod_set_value(priv->reset, 0);
> + realtek_mdio_reset_deassert(priv);
> msleep(REALTEK_HW_START_DELAY);
> dev_dbg(dev, "deasserted RESET\n");
> }
> @@ -246,8 +284,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_reset_assert(priv);
> }
>
> 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..a99e53b5b662 100644
> --- a/drivers/net/dsa/realtek/realtek-smi.c
> +++ b/drivers/net/dsa/realtek/realtek-smi.c
> @@ -408,6 +408,40 @@ static int realtek_smi_setup_mdio(struct dsa_switch *ds)
> return ret;
> }
>
> +static void realtek_smi_reset_assert(struct realtek_priv *priv)
> +{
> + int ret;
> +
> + if (priv->reset_ctl) {
> + ret = reset_control_assert(priv->reset_ctl);
> + if (ret)
> + dev_warn(priv->dev, "Failed to assert the switch reset control. Error: %i",
> + ret);
> +
> + return;
> + }
> +
> + if (priv->reset)
> + gpiod_set_value(priv->reset, true);
> +}
> +
> +static void realtek_smi_reset_deassert(struct realtek_priv *priv)
> +{
> + int ret;
> +
> + if (priv->reset_ctl) {
> + ret = reset_control_deassert(priv->reset_ctl);
> + if (ret)
> + dev_warn(priv->dev, "Failed to deassert the switch reset control. Error: %i",
> + ret);
> +
> + return;
> + }
> +
> + if (priv->reset)
> + gpiod_set_value(priv->reset, false);
> +}
> +
To respond here, in a single email, to your earlier question (sorry):
https://lore.kernel.org/netdev/CAJq09z7miTe7HUzsL4GBSwkrzyy4mVi6z40+ETgvmY=iWGRN-g@mail.gmail.com/
| 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?
The answer is: I ran "meld" between realtek-mdio.c and realtek-smi.c,
and the probe, remove and shutdown functions are surprisingly similar
already, and perhaps might become even more similar in the future.
I think it is worth introducing a common kernel module for both
interface drivers as a preliminary patch, rather than keeping duplicated
probe/remove/shutdown code.
> static int realtek_smi_probe(struct platform_device *pdev)
> {
> const struct realtek_variant *var;
> @@ -457,18 +491,22 @@ static int realtek_smi_probe(struct platform_device *pdev)
> dev_set_drvdata(dev, priv);
> spin_lock_init(&priv->lock);
>
> - /* TODO: if power is software controlled, set up any regulators here */
> + priv->reset_ctl = devm_reset_control_get_optional(dev, NULL);
> + if (IS_ERR(priv->reset_ctl)) {
> + ret = PTR_ERR(priv->reset_ctl);
> + return dev_err_probe(dev, ret, "failed to get reset control\n");
> + }
>
> 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_smi_reset_assert(priv);
> dev_dbg(dev, "asserted RESET\n");
> msleep(REALTEK_HW_STOP_DELAY);
> - gpiod_set_value(priv->reset, 0);
> + realtek_smi_reset_deassert(priv);
> msleep(REALTEK_HW_START_DELAY);
> dev_dbg(dev, "deasserted RESET\n");
> }
> @@ -518,8 +556,7 @@ static void realtek_smi_remove(struct platform_device *pdev)
> of_node_put(priv->slave_mii_bus->dev.of_node);
slave_mii_bus was renamed to user_mii_bus, and this prevents the
application of the patch currently, so you will need to respin. But I
think net-next is going to close soon for 2 weeks, so either you respin
as RFC or you wait until it reopens.
>
> /* leave the device reset asserted */
> - if (priv->reset)
> - gpiod_set_value(priv->reset, 1);
> + realtek_smi_reset_assert(priv);
> }
>
Powered by blists - more mailing lists