diff mbox series

[v2,net-next,5/5] net: ethernet: mtk_wed: add reset/reset_complete callbacks

Message ID 3145529a2588bba0ded16fc3c1c93ae799024442.1672840859.git.lorenzo@kernel.org (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series net: ethernet: mtk_wed: introduce reset support | expand

Checks

Context Check Description
netdev/tree_selection success Clearly marked for net-next
netdev/fixes_present success Fixes tag not required for -next series
netdev/subject_prefix success Link
netdev/cover_letter success Series has a cover letter
netdev/patch_count success Link
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 20 this patch: 20
netdev/cc_maintainers warning 4 maintainers not CCed: matthias.bgg@gmail.com linux-arm-kernel@lists.infradead.org bo.jiao@mediatek.com linux-mediatek@lists.infradead.org
netdev/build_clang success Errors and warnings before: 7 this patch: 7
netdev/module_param success Was 0 now: 0
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 20 this patch: 20
netdev/checkpatch warning CHECK: Please use a blank line after function/struct/union/enum declarations
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Lorenzo Bianconi Jan. 4, 2023, 2:03 p.m. UTC
Introduce reset and reset_complete wlan callback to schedule WLAN driver
reset when ethernet/wed driver is resetting.

Tested-by: Daniel Golle <daniel@makrotopia.org>
Co-developed-by: Sujuan Chen <sujuan.chen@mediatek.com>
Signed-off-by: Sujuan Chen <sujuan.chen@mediatek.com>
Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
---
 drivers/net/ethernet/mediatek/mtk_eth_soc.c |  6 ++++
 drivers/net/ethernet/mediatek/mtk_wed.c     | 40 +++++++++++++++++++++
 drivers/net/ethernet/mediatek/mtk_wed.h     |  8 +++++
 include/linux/soc/mediatek/mtk_wed.h        |  2 ++
 4 files changed, 56 insertions(+)

Comments

Leon Romanovsky Jan. 4, 2023, 2:17 p.m. UTC | #1
On Wed, Jan 04, 2023 at 03:03:14PM +0100, Lorenzo Bianconi wrote:
> Introduce reset and reset_complete wlan callback to schedule WLAN driver
> reset when ethernet/wed driver is resetting.
> 
> Tested-by: Daniel Golle <daniel@makrotopia.org>
> Co-developed-by: Sujuan Chen <sujuan.chen@mediatek.com>
> Signed-off-by: Sujuan Chen <sujuan.chen@mediatek.com>
> Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
> ---
>  drivers/net/ethernet/mediatek/mtk_eth_soc.c |  6 ++++
>  drivers/net/ethernet/mediatek/mtk_wed.c     | 40 +++++++++++++++++++++
>  drivers/net/ethernet/mediatek/mtk_wed.h     |  8 +++++
>  include/linux/soc/mediatek/mtk_wed.h        |  2 ++
>  4 files changed, 56 insertions(+)
> 
> diff --git a/drivers/net/ethernet/mediatek/mtk_eth_soc.c b/drivers/net/ethernet/mediatek/mtk_eth_soc.c
> index bafae4f0312e..2d74e26f45c9 100644
> --- a/drivers/net/ethernet/mediatek/mtk_eth_soc.c
> +++ b/drivers/net/ethernet/mediatek/mtk_eth_soc.c
> @@ -3913,6 +3913,10 @@ static void mtk_pending_work(struct work_struct *work)
>  		mtk_w32(eth, val, MTK_MAC_MCR(i));
>  	}
>  
> +	rtnl_unlock();
> +	mtk_wed_fe_reset();
> +	rtnl_lock();

Is it safe to call rtnl_unlock(), perform some work and lock again?

> +
>  	/* stop all devices to make sure that dma is properly shut down */
>  	for (i = 0; i < MTK_MAC_COUNT; i++) {
>  		if (!eth->netdev[i] || !netif_running(eth->netdev[i]))
> @@ -3949,6 +3953,8 @@ static void mtk_pending_work(struct work_struct *work)
>  
>  	clear_bit(MTK_RESETTING, &eth->state);
>  
> +	mtk_wed_fe_reset_complete();
> +
>  	rtnl_unlock();
>  }
>  
> diff --git a/drivers/net/ethernet/mediatek/mtk_wed.c b/drivers/net/ethernet/mediatek/mtk_wed.c
> index a6271449617f..4854993f2941 100644
> --- a/drivers/net/ethernet/mediatek/mtk_wed.c
> +++ b/drivers/net/ethernet/mediatek/mtk_wed.c
> @@ -206,6 +206,46 @@ mtk_wed_wo_reset(struct mtk_wed_device *dev)
>  	iounmap(reg);
>  }
>  
> +void mtk_wed_fe_reset(void)
> +{
> +	int i;
> +
> +	mutex_lock(&hw_lock);
> +
> +	for (i = 0; i < ARRAY_SIZE(hw_list); i++) {
> +		struct mtk_wed_hw *hw = hw_list[i];
> +		struct mtk_wed_device *dev = hw->wed_dev;
> +
> +		if (!dev || !dev->wlan.reset)
> +			continue;
> +
> +		/* reset callback blocks until WLAN reset is completed */
> +		if (dev->wlan.reset(dev))
> +			dev_err(dev->dev, "wlan reset failed\n");
> +	}
> +
> +	mutex_unlock(&hw_lock);
> +}
> +
> +void mtk_wed_fe_reset_complete(void)
> +{
> +	int i;
> +
> +	mutex_lock(&hw_lock);
> +
> +	for (i = 0; i < ARRAY_SIZE(hw_list); i++) {
> +		struct mtk_wed_hw *hw = hw_list[i];
> +		struct mtk_wed_device *dev = hw->wed_dev;
> +
> +		if (!dev || !dev->wlan.reset_complete)
> +			continue;
> +
> +		dev->wlan.reset_complete(dev);
> +	}
> +
> +	mutex_unlock(&hw_lock);
> +}
> +
>  static struct mtk_wed_hw *
>  mtk_wed_assign(struct mtk_wed_device *dev)
>  {
> diff --git a/drivers/net/ethernet/mediatek/mtk_wed.h b/drivers/net/ethernet/mediatek/mtk_wed.h
> index e012b8a82133..6108a7e69a80 100644
> --- a/drivers/net/ethernet/mediatek/mtk_wed.h
> +++ b/drivers/net/ethernet/mediatek/mtk_wed.h
> @@ -128,6 +128,8 @@ void mtk_wed_add_hw(struct device_node *np, struct mtk_eth *eth,
>  void mtk_wed_exit(void);
>  int mtk_wed_flow_add(int index);
>  void mtk_wed_flow_remove(int index);
> +void mtk_wed_fe_reset(void);
> +void mtk_wed_fe_reset_complete(void);
>  #else
>  static inline void
>  mtk_wed_add_hw(struct device_node *np, struct mtk_eth *eth,
> @@ -146,6 +148,12 @@ static inline int mtk_wed_flow_add(int index)
>  static inline void mtk_wed_flow_remove(int index)
>  {
>  }
> +static inline void mtk_wed_fe_reset(void)
> +{
> +}
> +static inline void mtk_wed_fe_reset_complete(void)
> +{
> +}
>  
>  #endif
>  
> diff --git a/include/linux/soc/mediatek/mtk_wed.h b/include/linux/soc/mediatek/mtk_wed.h
> index db637a13888d..ddff54fc9717 100644
> --- a/include/linux/soc/mediatek/mtk_wed.h
> +++ b/include/linux/soc/mediatek/mtk_wed.h
> @@ -150,6 +150,8 @@ struct mtk_wed_device {
>  		void (*release_rx_buf)(struct mtk_wed_device *wed);
>  		void (*update_wo_rx_stats)(struct mtk_wed_device *wed,
>  					   struct mtk_wed_wo_rx_stats *stats);
> +		int (*reset)(struct mtk_wed_device *wed);
> +		int (*reset_complete)(struct mtk_wed_device *wed);

I don't see any driver implementation of these callbacks in this series.
Did I miss it?

Thanks

>  	} wlan;
>  #endif
>  };
> -- 
> 2.39.0
>
Lorenzo Bianconi Jan. 4, 2023, 2:59 p.m. UTC | #2
> On Wed, Jan 04, 2023 at 03:03:14PM +0100, Lorenzo Bianconi wrote:
> > Introduce reset and reset_complete wlan callback to schedule WLAN driver
> > reset when ethernet/wed driver is resetting.
> > 
> > Tested-by: Daniel Golle <daniel@makrotopia.org>
> > Co-developed-by: Sujuan Chen <sujuan.chen@mediatek.com>
> > Signed-off-by: Sujuan Chen <sujuan.chen@mediatek.com>
> > Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
> > ---
> >  drivers/net/ethernet/mediatek/mtk_eth_soc.c |  6 ++++
> >  drivers/net/ethernet/mediatek/mtk_wed.c     | 40 +++++++++++++++++++++
> >  drivers/net/ethernet/mediatek/mtk_wed.h     |  8 +++++
> >  include/linux/soc/mediatek/mtk_wed.h        |  2 ++
> >  4 files changed, 56 insertions(+)
> > 
> > diff --git a/drivers/net/ethernet/mediatek/mtk_eth_soc.c b/drivers/net/ethernet/mediatek/mtk_eth_soc.c
> > index bafae4f0312e..2d74e26f45c9 100644
> > --- a/drivers/net/ethernet/mediatek/mtk_eth_soc.c
> > +++ b/drivers/net/ethernet/mediatek/mtk_eth_soc.c
> > @@ -3913,6 +3913,10 @@ static void mtk_pending_work(struct work_struct *work)
> >  		mtk_w32(eth, val, MTK_MAC_MCR(i));
> >  	}
> >  
> > +	rtnl_unlock();
> > +	mtk_wed_fe_reset();
> > +	rtnl_lock();
> 
> Is it safe to call rtnl_unlock(), perform some work and lock again?

Yes, mtk_pending_work sets MTK_RESETTING bit and a new reset work is not
scheduled until this bit is cleared

> 
> > +
> >  	/* stop all devices to make sure that dma is properly shut down */
> >  	for (i = 0; i < MTK_MAC_COUNT; i++) {
> >  		if (!eth->netdev[i] || !netif_running(eth->netdev[i]))
> > @@ -3949,6 +3953,8 @@ static void mtk_pending_work(struct work_struct *work)
> >  
> >  	clear_bit(MTK_RESETTING, &eth->state);
> >  
> > +	mtk_wed_fe_reset_complete();
> > +
> >  	rtnl_unlock();
> >  }
> >  
> > diff --git a/drivers/net/ethernet/mediatek/mtk_wed.c b/drivers/net/ethernet/mediatek/mtk_wed.c
> > index a6271449617f..4854993f2941 100644
> > --- a/drivers/net/ethernet/mediatek/mtk_wed.c
> > +++ b/drivers/net/ethernet/mediatek/mtk_wed.c
> > @@ -206,6 +206,46 @@ mtk_wed_wo_reset(struct mtk_wed_device *dev)
> >  	iounmap(reg);
> >  }
> >  
> > +void mtk_wed_fe_reset(void)
> > +{
> > +	int i;
> > +
> > +	mutex_lock(&hw_lock);
> > +
> > +	for (i = 0; i < ARRAY_SIZE(hw_list); i++) {
> > +		struct mtk_wed_hw *hw = hw_list[i];
> > +		struct mtk_wed_device *dev = hw->wed_dev;
> > +
> > +		if (!dev || !dev->wlan.reset)
> > +			continue;
> > +
> > +		/* reset callback blocks until WLAN reset is completed */
> > +		if (dev->wlan.reset(dev))
> > +			dev_err(dev->dev, "wlan reset failed\n");
> > +	}
> > +
> > +	mutex_unlock(&hw_lock);
> > +}
> > +
> > +void mtk_wed_fe_reset_complete(void)
> > +{
> > +	int i;
> > +
> > +	mutex_lock(&hw_lock);
> > +
> > +	for (i = 0; i < ARRAY_SIZE(hw_list); i++) {
> > +		struct mtk_wed_hw *hw = hw_list[i];
> > +		struct mtk_wed_device *dev = hw->wed_dev;
> > +
> > +		if (!dev || !dev->wlan.reset_complete)
> > +			continue;
> > +
> > +		dev->wlan.reset_complete(dev);
> > +	}
> > +
> > +	mutex_unlock(&hw_lock);
> > +}
> > +
> >  static struct mtk_wed_hw *
> >  mtk_wed_assign(struct mtk_wed_device *dev)
> >  {
> > diff --git a/drivers/net/ethernet/mediatek/mtk_wed.h b/drivers/net/ethernet/mediatek/mtk_wed.h
> > index e012b8a82133..6108a7e69a80 100644
> > --- a/drivers/net/ethernet/mediatek/mtk_wed.h
> > +++ b/drivers/net/ethernet/mediatek/mtk_wed.h
> > @@ -128,6 +128,8 @@ void mtk_wed_add_hw(struct device_node *np, struct mtk_eth *eth,
> >  void mtk_wed_exit(void);
> >  int mtk_wed_flow_add(int index);
> >  void mtk_wed_flow_remove(int index);
> > +void mtk_wed_fe_reset(void);
> > +void mtk_wed_fe_reset_complete(void);
> >  #else
> >  static inline void
> >  mtk_wed_add_hw(struct device_node *np, struct mtk_eth *eth,
> > @@ -146,6 +148,12 @@ static inline int mtk_wed_flow_add(int index)
> >  static inline void mtk_wed_flow_remove(int index)
> >  {
> >  }
> > +static inline void mtk_wed_fe_reset(void)
> > +{
> > +}
> > +static inline void mtk_wed_fe_reset_complete(void)
> > +{
> > +}
> >  
> >  #endif
> >  
> > diff --git a/include/linux/soc/mediatek/mtk_wed.h b/include/linux/soc/mediatek/mtk_wed.h
> > index db637a13888d..ddff54fc9717 100644
> > --- a/include/linux/soc/mediatek/mtk_wed.h
> > +++ b/include/linux/soc/mediatek/mtk_wed.h
> > @@ -150,6 +150,8 @@ struct mtk_wed_device {
> >  		void (*release_rx_buf)(struct mtk_wed_device *wed);
> >  		void (*update_wo_rx_stats)(struct mtk_wed_device *wed,
> >  					   struct mtk_wed_wo_rx_stats *stats);
> > +		int (*reset)(struct mtk_wed_device *wed);
> > +		int (*reset_complete)(struct mtk_wed_device *wed);
> 
> I don't see any driver implementation of these callbacks in this series.
> Did I miss it?

These callbacks are implemented in the mt76 driver. I have not added these
patches to the series since mt76 patches usually go through Felix/Kalle's
trees (anyway I am fine to add them to the series if they can go into net-next
directly).

Regards,
Lorenzo

> 
> Thanks
> 
> >  	} wlan;
> >  #endif
> >  };
> > -- 
> > 2.39.0
> >
Leon Romanovsky Jan. 5, 2023, 9:22 a.m. UTC | #3
On Wed, Jan 04, 2023 at 03:59:17PM +0100, Lorenzo Bianconi wrote:
> > On Wed, Jan 04, 2023 at 03:03:14PM +0100, Lorenzo Bianconi wrote:
> > > Introduce reset and reset_complete wlan callback to schedule WLAN driver
> > > reset when ethernet/wed driver is resetting.
> > > 
> > > Tested-by: Daniel Golle <daniel@makrotopia.org>
> > > Co-developed-by: Sujuan Chen <sujuan.chen@mediatek.com>
> > > Signed-off-by: Sujuan Chen <sujuan.chen@mediatek.com>
> > > Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
> > > ---
> > >  drivers/net/ethernet/mediatek/mtk_eth_soc.c |  6 ++++
> > >  drivers/net/ethernet/mediatek/mtk_wed.c     | 40 +++++++++++++++++++++
> > >  drivers/net/ethernet/mediatek/mtk_wed.h     |  8 +++++
> > >  include/linux/soc/mediatek/mtk_wed.h        |  2 ++
> > >  4 files changed, 56 insertions(+)
> > > 
> > > diff --git a/drivers/net/ethernet/mediatek/mtk_eth_soc.c b/drivers/net/ethernet/mediatek/mtk_eth_soc.c
> > > index bafae4f0312e..2d74e26f45c9 100644
> > > --- a/drivers/net/ethernet/mediatek/mtk_eth_soc.c
> > > +++ b/drivers/net/ethernet/mediatek/mtk_eth_soc.c
> > > @@ -3913,6 +3913,10 @@ static void mtk_pending_work(struct work_struct *work)
> > >  		mtk_w32(eth, val, MTK_MAC_MCR(i));
> > >  	}
> > >  
> > > +	rtnl_unlock();
> > > +	mtk_wed_fe_reset();
> > > +	rtnl_lock();
> > 
> > Is it safe to call rtnl_unlock(), perform some work and lock again?
> 
> Yes, mtk_pending_work sets MTK_RESETTING bit and a new reset work is not
> scheduled until this bit is cleared

I'm more worried about opening a window for user-space access while you
are performing FW reset.

<...>

> > > diff --git a/include/linux/soc/mediatek/mtk_wed.h b/include/linux/soc/mediatek/mtk_wed.h
> > > index db637a13888d..ddff54fc9717 100644
> > > --- a/include/linux/soc/mediatek/mtk_wed.h
> > > +++ b/include/linux/soc/mediatek/mtk_wed.h
> > > @@ -150,6 +150,8 @@ struct mtk_wed_device {
> > >  		void (*release_rx_buf)(struct mtk_wed_device *wed);
> > >  		void (*update_wo_rx_stats)(struct mtk_wed_device *wed,
> > >  					   struct mtk_wed_wo_rx_stats *stats);
> > > +		int (*reset)(struct mtk_wed_device *wed);
> > > +		int (*reset_complete)(struct mtk_wed_device *wed);
> > 
> > I don't see any driver implementation of these callbacks in this series.
> > Did I miss it?
> 
> These callbacks are implemented in the mt76 driver. I have not added these
> patches to the series since mt76 patches usually go through Felix/Kalle's
> trees (anyway I am fine to add them to the series if they can go into net-next
> directly).

Usually patches that use specific functionality are submitted together
with API changes.

> 
> Regards,
> Lorenzo
> 
> > 
> > Thanks
> > 
> > >  	} wlan;
> > >  #endif
> > >  };
> > > -- 
> > > 2.39.0
> > >
Lorenzo Bianconi Jan. 5, 2023, 11:49 a.m. UTC | #4
> On Wed, Jan 04, 2023 at 03:59:17PM +0100, Lorenzo Bianconi wrote:
> > > On Wed, Jan 04, 2023 at 03:03:14PM +0100, Lorenzo Bianconi wrote:
> > > > Introduce reset and reset_complete wlan callback to schedule WLAN driver
> > > > reset when ethernet/wed driver is resetting.
> > > > 
> > > > Tested-by: Daniel Golle <daniel@makrotopia.org>
> > > > Co-developed-by: Sujuan Chen <sujuan.chen@mediatek.com>
> > > > Signed-off-by: Sujuan Chen <sujuan.chen@mediatek.com>
> > > > Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
> > > > ---
> > > >  drivers/net/ethernet/mediatek/mtk_eth_soc.c |  6 ++++
> > > >  drivers/net/ethernet/mediatek/mtk_wed.c     | 40 +++++++++++++++++++++
> > > >  drivers/net/ethernet/mediatek/mtk_wed.h     |  8 +++++
> > > >  include/linux/soc/mediatek/mtk_wed.h        |  2 ++
> > > >  4 files changed, 56 insertions(+)
> > > > 
> > > > diff --git a/drivers/net/ethernet/mediatek/mtk_eth_soc.c b/drivers/net/ethernet/mediatek/mtk_eth_soc.c
> > > > index bafae4f0312e..2d74e26f45c9 100644
> > > > --- a/drivers/net/ethernet/mediatek/mtk_eth_soc.c
> > > > +++ b/drivers/net/ethernet/mediatek/mtk_eth_soc.c
> > > > @@ -3913,6 +3913,10 @@ static void mtk_pending_work(struct work_struct *work)
> > > >  		mtk_w32(eth, val, MTK_MAC_MCR(i));
> > > >  	}
> > > >  
> > > > +	rtnl_unlock();
> > > > +	mtk_wed_fe_reset();
> > > > +	rtnl_lock();
> > > 
> > > Is it safe to call rtnl_unlock(), perform some work and lock again?
> > 
> > Yes, mtk_pending_work sets MTK_RESETTING bit and a new reset work is not
> > scheduled until this bit is cleared
> 
> I'm more worried about opening a window for user-space access while you
> are performing FW reset.

ack, right. I will work on it.

> 
> <...>
> 
> > > > diff --git a/include/linux/soc/mediatek/mtk_wed.h b/include/linux/soc/mediatek/mtk_wed.h
> > > > index db637a13888d..ddff54fc9717 100644
> > > > --- a/include/linux/soc/mediatek/mtk_wed.h
> > > > +++ b/include/linux/soc/mediatek/mtk_wed.h
> > > > @@ -150,6 +150,8 @@ struct mtk_wed_device {
> > > >  		void (*release_rx_buf)(struct mtk_wed_device *wed);
> > > >  		void (*update_wo_rx_stats)(struct mtk_wed_device *wed,
> > > >  					   struct mtk_wed_wo_rx_stats *stats);
> > > > +		int (*reset)(struct mtk_wed_device *wed);
> > > > +		int (*reset_complete)(struct mtk_wed_device *wed);
> > > 
> > > I don't see any driver implementation of these callbacks in this series.
> > > Did I miss it?
> > 
> > These callbacks are implemented in the mt76 driver. I have not added these
> > patches to the series since mt76 patches usually go through Felix/Kalle's
> > trees (anyway I am fine to add them to the series if they can go into net-next
> > directly).
> 
> Usually patches that use specific functionality are submitted together
> with API changes.

I would say it is better mt76 patches go through Felix/Kalle's tree in order to avoid
conflicts.

@Felix, Kalle: any opinions?

Regards,
Lorenzo

> 
> > 
> > Regards,
> > Lorenzo
> > 
> > > 
> > > Thanks
> > > 
> > > >  	} wlan;
> > > >  #endif
> > > >  };
> > > > -- 
> > > > 2.39.0
> > > > 
> 
>
Jakub Kicinski Jan. 6, 2023, 5:48 a.m. UTC | #5
On Thu, 5 Jan 2023 12:49:49 +0100 Lorenzo Bianconi wrote:
> > > These callbacks are implemented in the mt76 driver. I have not added these
> > > patches to the series since mt76 patches usually go through Felix/Kalle's
> > > trees (anyway I am fine to add them to the series if they can go into net-next
> > > directly).  
> > 
> > Usually patches that use specific functionality are submitted together
> > with API changes.  
> 
> I would say it is better mt76 patches go through Felix/Kalle's tree in order to avoid
> conflicts.
> 
> @Felix, Kalle: any opinions?

FWIW as long as the implementation is in net-next before the merge
window I'm fine either way. But it would be good to see the
implementation, a co-posted RFC maybe?
Lorenzo Bianconi Jan. 6, 2023, 9:11 a.m. UTC | #6
On Jan 05, Jakub Kicinski wrote:
> On Thu, 5 Jan 2023 12:49:49 +0100 Lorenzo Bianconi wrote:
> > > > These callbacks are implemented in the mt76 driver. I have not added these
> > > > patches to the series since mt76 patches usually go through Felix/Kalle's
> > > > trees (anyway I am fine to add them to the series if they can go into net-next
> > > > directly).  
> > > 
> > > Usually patches that use specific functionality are submitted together
> > > with API changes.  
> > 
> > I would say it is better mt76 patches go through Felix/Kalle's tree in order to avoid
> > conflicts.
> > 
> > @Felix, Kalle: any opinions?
> 
> FWIW as long as the implementation is in net-next before the merge
> window I'm fine either way. But it would be good to see the
> implementation, a co-posted RFC maybe?

ack, I will post mt76 series to wireless mailing list just after the new version
of this one.

Regards,
Lorenzo
Lorenzo Bianconi Jan. 7, 2023, 2:39 p.m. UTC | #7
> On Wed, Jan 04, 2023 at 03:59:17PM +0100, Lorenzo Bianconi wrote:
> > > On Wed, Jan 04, 2023 at 03:03:14PM +0100, Lorenzo Bianconi wrote:
> > > > Introduce reset and reset_complete wlan callback to schedule WLAN driver
> > > > reset when ethernet/wed driver is resetting.
> > > > 
> > > > Tested-by: Daniel Golle <daniel@makrotopia.org>
> > > > Co-developed-by: Sujuan Chen <sujuan.chen@mediatek.com>
> > > > Signed-off-by: Sujuan Chen <sujuan.chen@mediatek.com>
> > > > Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
> > > > ---
> > > >  drivers/net/ethernet/mediatek/mtk_eth_soc.c |  6 ++++
> > > >  drivers/net/ethernet/mediatek/mtk_wed.c     | 40 +++++++++++++++++++++
> > > >  drivers/net/ethernet/mediatek/mtk_wed.h     |  8 +++++
> > > >  include/linux/soc/mediatek/mtk_wed.h        |  2 ++
> > > >  4 files changed, 56 insertions(+)
> > > > 
> > > > diff --git a/drivers/net/ethernet/mediatek/mtk_eth_soc.c b/drivers/net/ethernet/mediatek/mtk_eth_soc.c
> > > > index bafae4f0312e..2d74e26f45c9 100644
> > > > --- a/drivers/net/ethernet/mediatek/mtk_eth_soc.c
> > > > +++ b/drivers/net/ethernet/mediatek/mtk_eth_soc.c
> > > > @@ -3913,6 +3913,10 @@ static void mtk_pending_work(struct work_struct *work)
> > > >  		mtk_w32(eth, val, MTK_MAC_MCR(i));
> > > >  	}
> > > >  
> > > > +	rtnl_unlock();
> > > > +	mtk_wed_fe_reset();
> > > > +	rtnl_lock();
> > > 
> > > Is it safe to call rtnl_unlock(), perform some work and lock again?
> > 
> > Yes, mtk_pending_work sets MTK_RESETTING bit and a new reset work is not
> > scheduled until this bit is cleared
> 
> I'm more worried about opening a window for user-space access while you
> are performing FW reset.

looking at mtk_pending_work() I guess running mtk_wed_fe_reset() releasing RTNL
lock is not harmful since we just perform few actions (ppe reset, ...)  before
running mtk_wed_fe_reset(). Moreover, the core reset part in mtk_pending_work()
(mtk_stop(), mtk_open(), ..) is done after mtk_wed_fe_reset() where we reacquired
RTNL lock.
In order to avoid any possible race, I guess we can just re-do the preliminary
reset configuration done before mtk_wed_fe_reset() just after re-acquiring RTNL
lock.

Regards,
Lorenzo

> 
> <...>
> 
> > > > diff --git a/include/linux/soc/mediatek/mtk_wed.h b/include/linux/soc/mediatek/mtk_wed.h
> > > > index db637a13888d..ddff54fc9717 100644
> > > > --- a/include/linux/soc/mediatek/mtk_wed.h
> > > > +++ b/include/linux/soc/mediatek/mtk_wed.h
> > > > @@ -150,6 +150,8 @@ struct mtk_wed_device {
> > > >  		void (*release_rx_buf)(struct mtk_wed_device *wed);
> > > >  		void (*update_wo_rx_stats)(struct mtk_wed_device *wed,
> > > >  					   struct mtk_wed_wo_rx_stats *stats);
> > > > +		int (*reset)(struct mtk_wed_device *wed);
> > > > +		int (*reset_complete)(struct mtk_wed_device *wed);
> > > 
> > > I don't see any driver implementation of these callbacks in this series.
> > > Did I miss it?
> > 
> > These callbacks are implemented in the mt76 driver. I have not added these
> > patches to the series since mt76 patches usually go through Felix/Kalle's
> > trees (anyway I am fine to add them to the series if they can go into net-next
> > directly).
> 
> Usually patches that use specific functionality are submitted together
> with API changes.
> 
> > 
> > Regards,
> > Lorenzo
> > 
> > > 
> > > Thanks
> > > 
> > > >  	} wlan;
> > > >  #endif
> > > >  };
> > > > -- 
> > > > 2.39.0
> > > > 
> 
>
Kalle Valo Jan. 9, 2023, 2:46 p.m. UTC | #8
(now back from vacation)

Jakub Kicinski <kuba@kernel.org> writes:

> On Thu, 5 Jan 2023 12:49:49 +0100 Lorenzo Bianconi wrote:
>> > > These callbacks are implemented in the mt76 driver. I have not added these
>> > > patches to the series since mt76 patches usually go through Felix/Kalle's
>> > > trees (anyway I am fine to add them to the series if they can go into net-next
>> > > directly).  
>> > 
>> > Usually patches that use specific functionality are submitted together
>> > with API changes.  
>> 
>> I would say it is better mt76 patches go through Felix/Kalle's tree in order to avoid
>> conflicts.
>> 
>> @Felix, Kalle: any opinions?
>
> FWIW as long as the implementation is in net-next before the merge
> window I'm fine either way. But it would be good to see the
> implementation, a co-posted RFC maybe?

FWIW I agree with Lorenzo, it would be good to get mt76 patches via
Felix's tree. Otherwise the conflicts might be annoying to fix.
diff mbox series

Patch

diff --git a/drivers/net/ethernet/mediatek/mtk_eth_soc.c b/drivers/net/ethernet/mediatek/mtk_eth_soc.c
index bafae4f0312e..2d74e26f45c9 100644
--- a/drivers/net/ethernet/mediatek/mtk_eth_soc.c
+++ b/drivers/net/ethernet/mediatek/mtk_eth_soc.c
@@ -3913,6 +3913,10 @@  static void mtk_pending_work(struct work_struct *work)
 		mtk_w32(eth, val, MTK_MAC_MCR(i));
 	}
 
+	rtnl_unlock();
+	mtk_wed_fe_reset();
+	rtnl_lock();
+
 	/* stop all devices to make sure that dma is properly shut down */
 	for (i = 0; i < MTK_MAC_COUNT; i++) {
 		if (!eth->netdev[i] || !netif_running(eth->netdev[i]))
@@ -3949,6 +3953,8 @@  static void mtk_pending_work(struct work_struct *work)
 
 	clear_bit(MTK_RESETTING, &eth->state);
 
+	mtk_wed_fe_reset_complete();
+
 	rtnl_unlock();
 }
 
diff --git a/drivers/net/ethernet/mediatek/mtk_wed.c b/drivers/net/ethernet/mediatek/mtk_wed.c
index a6271449617f..4854993f2941 100644
--- a/drivers/net/ethernet/mediatek/mtk_wed.c
+++ b/drivers/net/ethernet/mediatek/mtk_wed.c
@@ -206,6 +206,46 @@  mtk_wed_wo_reset(struct mtk_wed_device *dev)
 	iounmap(reg);
 }
 
+void mtk_wed_fe_reset(void)
+{
+	int i;
+
+	mutex_lock(&hw_lock);
+
+	for (i = 0; i < ARRAY_SIZE(hw_list); i++) {
+		struct mtk_wed_hw *hw = hw_list[i];
+		struct mtk_wed_device *dev = hw->wed_dev;
+
+		if (!dev || !dev->wlan.reset)
+			continue;
+
+		/* reset callback blocks until WLAN reset is completed */
+		if (dev->wlan.reset(dev))
+			dev_err(dev->dev, "wlan reset failed\n");
+	}
+
+	mutex_unlock(&hw_lock);
+}
+
+void mtk_wed_fe_reset_complete(void)
+{
+	int i;
+
+	mutex_lock(&hw_lock);
+
+	for (i = 0; i < ARRAY_SIZE(hw_list); i++) {
+		struct mtk_wed_hw *hw = hw_list[i];
+		struct mtk_wed_device *dev = hw->wed_dev;
+
+		if (!dev || !dev->wlan.reset_complete)
+			continue;
+
+		dev->wlan.reset_complete(dev);
+	}
+
+	mutex_unlock(&hw_lock);
+}
+
 static struct mtk_wed_hw *
 mtk_wed_assign(struct mtk_wed_device *dev)
 {
diff --git a/drivers/net/ethernet/mediatek/mtk_wed.h b/drivers/net/ethernet/mediatek/mtk_wed.h
index e012b8a82133..6108a7e69a80 100644
--- a/drivers/net/ethernet/mediatek/mtk_wed.h
+++ b/drivers/net/ethernet/mediatek/mtk_wed.h
@@ -128,6 +128,8 @@  void mtk_wed_add_hw(struct device_node *np, struct mtk_eth *eth,
 void mtk_wed_exit(void);
 int mtk_wed_flow_add(int index);
 void mtk_wed_flow_remove(int index);
+void mtk_wed_fe_reset(void);
+void mtk_wed_fe_reset_complete(void);
 #else
 static inline void
 mtk_wed_add_hw(struct device_node *np, struct mtk_eth *eth,
@@ -146,6 +148,12 @@  static inline int mtk_wed_flow_add(int index)
 static inline void mtk_wed_flow_remove(int index)
 {
 }
+static inline void mtk_wed_fe_reset(void)
+{
+}
+static inline void mtk_wed_fe_reset_complete(void)
+{
+}
 
 #endif
 
diff --git a/include/linux/soc/mediatek/mtk_wed.h b/include/linux/soc/mediatek/mtk_wed.h
index db637a13888d..ddff54fc9717 100644
--- a/include/linux/soc/mediatek/mtk_wed.h
+++ b/include/linux/soc/mediatek/mtk_wed.h
@@ -150,6 +150,8 @@  struct mtk_wed_device {
 		void (*release_rx_buf)(struct mtk_wed_device *wed);
 		void (*update_wo_rx_stats)(struct mtk_wed_device *wed,
 					   struct mtk_wed_wo_rx_stats *stats);
+		int (*reset)(struct mtk_wed_device *wed);
+		int (*reset_complete)(struct mtk_wed_device *wed);
 	} wlan;
 #endif
 };