diff mbox series

[v5,1/2] net: stmmac: add support for platform specific reset

Message ID 20230403152408.238530-1-shenwei.wang@nxp.com (mailing list archive)
State New, archived
Headers show
Series [v5,1/2] net: stmmac: add support for platform specific reset | expand

Commit Message

Shenwei Wang April 3, 2023, 3:24 p.m. UTC 
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 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@nxp.com>
---
 v5:
  - add the missing __iomem tag in the stmmac_reset definition.

 drivers/net/ethernet/stmicro/stmmac/hwif.c | 10 ++++++++++
 drivers/net/ethernet/stmicro/stmmac/hwif.h |  3 +--
 include/linux/stmmac.h                     |  1 +
 3 files changed, 12 insertions(+), 2 deletions(-)

--
2.34.1

Comments

Simon Horman April 3, 2023, 7:49 p.m. UTC | #1 
On Mon, Apr 03, 2023 at 10:24:07AM -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 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@nxp.com>
> ---
>  v5:
>   - add the missing __iomem tag in the stmmac_reset definition.
> 
>  drivers/net/ethernet/stmicro/stmmac/hwif.c | 10 ++++++++++
>  drivers/net/ethernet/stmicro/stmmac/hwif.h |  3 +--
>  include/linux/stmmac.h                     |  1 +
>  3 files changed, 12 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/stmicro/stmmac/hwif.c b/drivers/net/ethernet/stmicro/stmmac/hwif.c
> index bb7114f970f8..0eefa697ffe8 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/hwif.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/hwif.c
> @@ -87,6 +87,16 @@ static int stmmac_dwxlgmac_quirks(struct stmmac_priv *priv)
>  	return 0;
>  }
> 
> +int stmmac_reset(struct stmmac_priv *priv, void __iomem *ioaddr)
> +{
> +	struct plat_stmmacenet_data *plat = priv ? priv->plat : NULL;

Here the case where priv is NULL is handled.

> +
> +	if (plat && plat->fix_soc_reset)
> +		return plat->fix_soc_reset(plat, ioaddr);
> +
> +	return stmmac_do_callback(priv, dma, reset, ioaddr);

But this will dereference priv unconditionally.

I think perhaps this is code that I suggested.
If so, sorry about not noticing this then.

> +}
> +
>  static const struct stmmac_hwif_entry {
>  	bool gmac;
>  	bool gmac4;

...
Shenwei Wang April 3, 2023, 10:16 p.m. UTC | #2 
> -----Original Message-----
> From: Simon Horman <simon.horman@corigine.com>
> Sent: Monday, April 3, 2023 2:50 PM
> To: Shenwei Wang <shenwei.wang@nxp.com>
> Cc: David S. Miller <davem@davemloft.net>; Eric Dumazet
> <edumazet@google.com>; Jakub Kicinski <kuba@kernel.org>; Paolo Abeni
> <pabeni@redhat.com>; Shawn Guo <shawnguo@kernel.org>; Sascha Hauer
> <s.hauer@pengutronix.de>; Pengutronix Kernel Team <kernel@pengutronix.de>;
> Giuseppe Cavallaro <peppe.cavallaro@st.com>; Alexandre Torgue
> <alexandre.torgue@foss.st.com>; Jose Abreu <joabreu@synopsys.com>; Fabio
> Estevam <festevam@gmail.com>; dl-linux-imx <linux-imx@nxp.com>; Maxime
> Coquelin <mcoquelin.stm32@gmail.com>; Wong Vee Khee
> <veekhee@apple.com>; Kurt Kanzenbach <kurt@linutronix.de>; Mohammad
> Athari Bin Ismail <mohammad.athari.ismail@intel.com>; Andrey Konovalov
> <andrey.konovalov@linaro.org>; Jochen Henneberg <jh@henneberg-
> systemdesign.com>; Tan Tee Min <tee.min.tan@linux.intel.com>;
> netdev@vger.kernel.org; linux-arm-kernel@lists.infradead.org; linux-stm32@st-
> md-mailman.stormreply.com; imx@lists.linux.dev
> Subject: [EXT] Re: [PATCH v5 1/2] net: stmmac: add support for platform specific
> reset
> 
> Caution: EXT Email
> 
> On Mon, Apr 03, 2023 at 10:24:07AM -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 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@nxp.com>
> > ---
> >  v5:
> >   - add the missing __iomem tag in the stmmac_reset definition.
> >
> >  drivers/net/ethernet/stmicro/stmmac/hwif.c | 10 ++++++++++
> > drivers/net/ethernet/stmicro/stmmac/hwif.h |  3 +--
> >  include/linux/stmmac.h                     |  1 +
> >  3 files changed, 12 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/stmicro/stmmac/hwif.c
> > b/drivers/net/ethernet/stmicro/stmmac/hwif.c
> > index bb7114f970f8..0eefa697ffe8 100644
> > --- a/drivers/net/ethernet/stmicro/stmmac/hwif.c
> > +++ b/drivers/net/ethernet/stmicro/stmmac/hwif.c
> > @@ -87,6 +87,16 @@ static int stmmac_dwxlgmac_quirks(struct stmmac_priv
> *priv)
> >       return 0;
> >  }
> >
> > +int stmmac_reset(struct stmmac_priv *priv, void __iomem *ioaddr) {
> > +     struct plat_stmmacenet_data *plat = priv ? priv->plat : NULL;
> 
> Here the case where priv is NULL is handled.
> 
> > +
> > +     if (plat && plat->fix_soc_reset)
> > +             return plat->fix_soc_reset(plat, ioaddr);
> > +
> > +     return stmmac_do_callback(priv, dma, reset, ioaddr);
> 
> But this will dereference priv unconditionally.
> 

The original macro implementation assumes that the priv pointer will not be NULL. However, adding 
an extra condition check for priv in the stmmac_reset() function can ensure that the code is more 
robust and secure.

Thanks,
Shenwei

> I think perhaps this is code that I suggested.
> If so, sorry about not noticing this then.
> 
> > +}
> > +
> >  static const struct stmmac_hwif_entry {
> >       bool gmac;
> >       bool gmac4;
> 
> ...
Simon Horman April 4, 2023, 9:34 a.m. UTC | #3 
On Mon, Apr 03, 2023 at 10:16:46PM +0000, Shenwei Wang wrote:
> 
> 
> > -----Original Message-----
> > From: Simon Horman <simon.horman@corigine.com>
> > Sent: Monday, April 3, 2023 2:50 PM
> > To: Shenwei Wang <shenwei.wang@nxp.com>
> > Cc: David S. Miller <davem@davemloft.net>; Eric Dumazet
> > <edumazet@google.com>; Jakub Kicinski <kuba@kernel.org>; Paolo Abeni
> > <pabeni@redhat.com>; Shawn Guo <shawnguo@kernel.org>; Sascha Hauer
> > <s.hauer@pengutronix.de>; Pengutronix Kernel Team <kernel@pengutronix.de>;
> > Giuseppe Cavallaro <peppe.cavallaro@st.com>; Alexandre Torgue
> > <alexandre.torgue@foss.st.com>; Jose Abreu <joabreu@synopsys.com>; Fabio
> > Estevam <festevam@gmail.com>; dl-linux-imx <linux-imx@nxp.com>; Maxime
> > Coquelin <mcoquelin.stm32@gmail.com>; Wong Vee Khee
> > <veekhee@apple.com>; Kurt Kanzenbach <kurt@linutronix.de>; Mohammad
> > Athari Bin Ismail <mohammad.athari.ismail@intel.com>; Andrey Konovalov
> > <andrey.konovalov@linaro.org>; Jochen Henneberg <jh@henneberg-
> > systemdesign.com>; Tan Tee Min <tee.min.tan@linux.intel.com>;
> > netdev@vger.kernel.org; linux-arm-kernel@lists.infradead.org; linux-stm32@st-
> > md-mailman.stormreply.com; imx@lists.linux.dev
> > Subject: [EXT] Re: [PATCH v5 1/2] net: stmmac: add support for platform specific
> > reset
> > 
> > Caution: EXT Email
> > 
> > On Mon, Apr 03, 2023 at 10:24:07AM -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 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@nxp.com>
> > > ---
> > >  v5:
> > >   - add the missing __iomem tag in the stmmac_reset definition.
> > >
> > >  drivers/net/ethernet/stmicro/stmmac/hwif.c | 10 ++++++++++
> > > drivers/net/ethernet/stmicro/stmmac/hwif.h |  3 +--
> > >  include/linux/stmmac.h                     |  1 +
> > >  3 files changed, 12 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/net/ethernet/stmicro/stmmac/hwif.c
> > > b/drivers/net/ethernet/stmicro/stmmac/hwif.c
> > > index bb7114f970f8..0eefa697ffe8 100644
> > > --- a/drivers/net/ethernet/stmicro/stmmac/hwif.c
> > > +++ b/drivers/net/ethernet/stmicro/stmmac/hwif.c
> > > @@ -87,6 +87,16 @@ static int stmmac_dwxlgmac_quirks(struct stmmac_priv
> > *priv)
> > >       return 0;
> > >  }
> > >
> > > +int stmmac_reset(struct stmmac_priv *priv, void __iomem *ioaddr) {
> > > +     struct plat_stmmacenet_data *plat = priv ? priv->plat : NULL;
> > 
> > Here the case where priv is NULL is handled.
> > 
> > > +
> > > +     if (plat && plat->fix_soc_reset)
> > > +             return plat->fix_soc_reset(plat, ioaddr);
> > > +
> > > +     return stmmac_do_callback(priv, dma, reset, ioaddr);
> > 
> > But this will dereference priv unconditionally.
> > 
> 
> The original macro implementation assumes that the priv pointer will not be NULL. However, adding 
> an extra condition check for priv in the stmmac_reset() function can ensure that the code is more 
> robust and secure.

But it seems to me that it is not safe because stmmac_do_callback
will dereference priv even if it is NULL.

So I think either the NULL case should be handled in a safe way.
Or there is no point in checking for it at all.
diff mbox series

Patch

diff --git a/drivers/net/ethernet/stmicro/stmmac/hwif.c b/drivers/net/ethernet/stmicro/stmmac/hwif.c
index bb7114f970f8..0eefa697ffe8 100644
--- a/drivers/net/ethernet/stmicro/stmmac/hwif.c
+++ b/drivers/net/ethernet/stmicro/stmmac/hwif.c
@@ -87,6 +87,16 @@  static int stmmac_dwxlgmac_quirks(struct stmmac_priv *priv)
 	return 0;
 }

+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);
+}
+
 static const struct stmmac_hwif_entry {
 	bool gmac;
 	bool gmac4;
diff --git a/drivers/net/ethernet/stmicro/stmmac/hwif.h b/drivers/net/ethernet/stmicro/stmmac/hwif.h
index 16a7421715cb..1cc286b000b6 100644
--- a/drivers/net/ethernet/stmicro/stmmac/hwif.h
+++ b/drivers/net/ethernet/stmicro/stmmac/hwif.h
@@ -214,8 +214,6 @@  struct stmmac_dma_ops {
 	int (*enable_tbs)(void __iomem *ioaddr, bool en, u32 chan);
 };

-#define stmmac_reset(__priv, __args...) \
-	stmmac_do_callback(__priv, dma, reset, __args)
 #define stmmac_dma_init(__priv, __args...) \
 	stmmac_do_void_callback(__priv, dma, init, __args)
 #define stmmac_init_chan(__priv, __args...) \
@@ -640,6 +638,7 @@  extern const struct stmmac_mmc_ops dwxgmac_mmc_ops;
 #define GMAC_VERSION		0x00000020	/* GMAC CORE Version */
 #define GMAC4_VERSION		0x00000110	/* GMAC4+ CORE Version */

+int stmmac_reset(struct stmmac_priv *priv, void __iomem *ioaddr);
 int stmmac_hwif_init(struct stmmac_priv *priv);

 #endif /* __STMMAC_HWIF_H__ */
diff --git a/include/linux/stmmac.h b/include/linux/stmmac.h
index a2414c187483..dafa001e9e7a 100644
--- a/include/linux/stmmac.h
+++ b/include/linux/stmmac.h
@@ -223,6 +223,7 @@  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)(void *priv, 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);