netdev - Re: [PATCH 1/1] net: stmmac: add support for platform specific reset

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <ZBst1SzcIS4j+t46@corigine.com>
Date:   Wed, 22 Mar 2023 17:33:25 +0100
From:   Simon Horman <simon.horman@...igine.com>
To:     Shenwei Wang <shenwei.wang@....com>
Cc:     "David S. Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>,
        Maxime Coquelin <mcoquelin.stm32@...il.com>,
        Shawn Guo <shawnguo@...nel.org>,
        NXP Linux Team <linux-imx@....com>,
        Giuseppe Cavallaro <peppe.cavallaro@...com>,
        Alexandre Torgue <alexandre.torgue@...s.st.com>,
        Jose Abreu <joabreu@...opsys.com>,
        Sascha Hauer <s.hauer@...gutronix.de>,
        Pengutronix Kernel Team <kernel@...gutronix.de>,
        Fabio Estevam <festevam@...il.com>,
        Wong Vee Khee <veekhee@...le.com>,
        Jon Hunter <jonathanh@...dia.com>,
        Mohammad Athari Bin Ismail <mohammad.athari.ismail@...el.com>,
        Andrey Konovalov <andrey.konovalov@...aro.org>,
        Revanth Kumar Uppala <ruppala@...dia.com>,
        Tan Tee Min <tee.min.tan@...ux.intel.com>,
        netdev@...r.kernel.org, linux-stm32@...md-mailman.stormreply.com,
        linux-arm-kernel@...ts.infradead.org, imx@...ts.linux.dev
Subject: Re: [PATCH 1/1] net: stmmac: add support for platform specific reset

On Tue, Mar 21, 2023 at 02:09:20PM -0500, Shenwei Wang wrote:
> This patch adds support for platform-specific reset logic in the
> stmmac driver. Some SoCs require a different reset mechanism than
> the standard dwmac IP reset. To support these platforms, a new function
> pointer 'fix_soc_reset' is added to the plat_stmmacenet_data structure.
> The stmmac_reset macro in hwif.h is modified to call the 'fix_soc_reset'
> function if it exists. This enables the driver to use the platform-specific
> reset logic when necessary.
> 
> Signed-off-by: Shenwei Wang <shenwei.wang@....com>
> ---
>  drivers/net/ethernet/stmicro/stmmac/hwif.h | 10 +++++++++-
>  include/linux/stmmac.h                     |  1 +
>  2 files changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/stmicro/stmmac/hwif.h b/drivers/net/ethernet/stmicro/stmmac/hwif.h
> index 16a7421715cb..e24ce870690e 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/hwif.h
> +++ b/drivers/net/ethernet/stmicro/stmmac/hwif.h
> @@ -215,7 +215,15 @@ struct stmmac_dma_ops {
>  };
>  
>  #define stmmac_reset(__priv, __args...) \
> -	stmmac_do_callback(__priv, dma, reset, __args)
> +({ \
> +	int __result = -EINVAL; \
> +	if ((__priv) && (__priv)->plat && (__priv)->plat->fix_soc_reset) { \
> +		__result = (__priv)->plat->fix_soc_reset((__priv)->plat, ##__args); \
> +	} else { \
> +		__result = stmmac_do_callback(__priv, dma, reset, __args); \
> +	} \
> +	__result; \
> +})

Hi Shenwei Wang,

I am wondering if any consideration was given to an approach
that has a bit better type safety.

Something like this (*compile tested only!*):

static inline int stmmac_reset(struct stmmac_priv *priv, void __iomem *ioaddr)
{
       struct plat_stmmacenet_data *plat = priv ? priv->plat : NULL;

       if (plat && plat->fix_soc_reset)
	       return plat->fix_soc_reset(plat, ioaddr);

       return stmmac_do_callback(priv, dma, reset, ioaddr);
}

In which case the first parameter of the fix_soc_reset field
of struct plat_stmmacenet_data can become struct plat_stmmacenet_data *.

diff --git a/include/linux/stmmac.h b/include/linux/stmmac.h
index 9044477fad61..b26ade7e4be8 100644
--- a/include/linux/stmmac.h
+++ b/include/linux/stmmac.h
@@ -223,6 +223,8 @@ struct plat_stmmacenet_data {
 	struct stmmac_rxq_cfg rx_queues_cfg[MTL_MAX_RX_QUEUES];
 	struct stmmac_txq_cfg tx_queues_cfg[MTL_MAX_TX_QUEUES];
 	void (*fix_mac_speed)(void *priv, unsigned int speed);
+	int (*fix_soc_reset)(struct plat_stmmacenet_data *,
+			     void __iomem *ioaddr);
 	int (*serdes_powerup)(struct net_device *ndev, void *priv);
 	void (*serdes_powerdown)(struct net_device *ndev, void *priv);
 	void (*speed_mode_2500)(struct net_device *ndev, void *priv);

I do see that the approach you have is
in keeping with the existing structure of stmmac_do_callback().
But I guess my question there is: why is that model used?
And could there be a plan to move away from that model?