netdev - Re: [PATCH v2 net-next 5/5] net: ethernet: mtk_wed: add reset/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: <Y7mEItAb41ANfDS+@lore-desk>
Date:   Sat, 7 Jan 2023 15:39:30 +0100
From:   Lorenzo Bianconi <lorenzo@...nel.org>
To:     Leon Romanovsky <leon@...nel.org>
Cc:     netdev@...r.kernel.org, davem@...emloft.net, edumazet@...gle.com,
        kuba@...nel.org, pabeni@...hat.com, lorenzo.bianconi@...hat.com,
        nbd@....name, john@...ozen.org, sean.wang@...iatek.com,
        Mark-MC.Lee@...iatek.com, sujuan.chen@...iatek.com,
        daniel@...rotopia.org
Subject: Re: [PATCH v2 net-next 5/5] net: ethernet: mtk_wed: add
 reset/reset_complete callbacks

> 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@...rotopia.org>
> > > > Co-developed-by: Sujuan Chen <sujuan.chen@...iatek.com>
> > > > Signed-off-by: Sujuan Chen <sujuan.chen@...iatek.com>
> > > > Signed-off-by: Lorenzo Bianconi <lorenzo@...nel.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
> > > > 
> 
> 

Download attachment "signature.asc" of type "application/pgp-signature" (229 bytes)