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

[Lorenzo Bianconi <lorenzo@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 3503 bytes --]

> 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
> > > > 
> 
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]