| Message ID | 0a22f0c81e87fde34e3444e1bc83012a17498e8e.1673457624.git.lorenzo@kernel.org (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: ethernet: mtk_wed: introduce reset support | expand |
On Wed, 2023-01-11 at 18:22 +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 | 7 ++++ > 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, 57 insertions(+) > Do we have any updates on the implementation that would be making use of this? It looks like there was a discussion for the v2 of this set to include a link to an RFC posting that would make use of this set. > diff --git a/drivers/net/ethernet/mediatek/mtk_eth_soc.c b/drivers/net/ethernet/mediatek/mtk_eth_soc.c > index 1af74e9a6cd3..0147e98009c2 100644 > --- a/drivers/net/ethernet/mediatek/mtk_eth_soc.c > +++ b/drivers/net/ethernet/mediatek/mtk_eth_soc.c > @@ -3924,6 +3924,11 @@ static void mtk_pending_work(struct work_struct *work) > set_bit(MTK_RESETTING, ð->state); > > mtk_prepare_for_reset(eth); > + mtk_wed_fe_reset(); > + /* Run again reset preliminary configuration in order to avoid any > + * possible race during FE reset since it can run releasing RTNL lock. > + */ > + mtk_prepare_for_reset(eth); > > /* stop all devices to make sure that dma is properly shut down */ > for (i = 0; i < MTK_MAC_COUNT; i++) { > @@ -3961,6 +3966,8 @@ static void mtk_pending_work(struct work_struct *work) > > clear_bit(MTK_RESETTING, ð->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"); The reason why having the consumer would be useful are cases like this. My main concern is if the error value might be useful to actually expose rather than just treating it as a boolean. Usually for things like this I prefer to see the result captured and if it indicates error we return the error value since this could be one of several possible causes for the error assuming this returns an int and not a bool. > + } > + > + 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) > {
> On Wed, 2023-01-11 at 18:22 +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 | 7 ++++ > > 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, 57 insertions(+) > > > > Do we have any updates on the implementation that would be making use > of this? It looks like there was a discussion for the v2 of this set to > include a link to an RFC posting that would make use of this set. I posted the series to linux-wireless mailing list adding netdev one in cc: https://lore.kernel.org/linux-wireless/cover.1673103214.git.lorenzo@kernel.org/T/#md34b4ffcb07056794378fa4e8079458ecca69109 > > > diff --git a/drivers/net/ethernet/mediatek/mtk_eth_soc.c b/drivers/net/ethernet/mediatek/mtk_eth_soc.c > > index 1af74e9a6cd3..0147e98009c2 100644 > > --- a/drivers/net/ethernet/mediatek/mtk_eth_soc.c > > +++ b/drivers/net/ethernet/mediatek/mtk_eth_soc.c > > @@ -3924,6 +3924,11 @@ static void mtk_pending_work(struct work_struct *work) > > set_bit(MTK_RESETTING, ð->state); > > > > mtk_prepare_for_reset(eth); > > + mtk_wed_fe_reset(); > > + /* Run again reset preliminary configuration in order to avoid any > > + * possible race during FE reset since it can run releasing RTNL lock. > > + */ > > + mtk_prepare_for_reset(eth); > > > > /* stop all devices to make sure that dma is properly shut down */ > > for (i = 0; i < MTK_MAC_COUNT; i++) { > > @@ -3961,6 +3966,8 @@ static void mtk_pending_work(struct work_struct *work) > > > > clear_bit(MTK_RESETTING, ð->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"); > > The reason why having the consumer would be useful are cases like this. > My main concern is if the error value might be useful to actually > expose rather than just treating it as a boolean. Usually for things > like this I prefer to see the result captured and if it indicates error > we return the error value since this could be one of several possible > causes for the error assuming this returns an int and not a bool. we can have 2 independent wireless chips connected here so, if the first one fails, should we exit or just log the error? Regards, Lorenzo > > > + } > > + > > + 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) > > { >
On Thu, Jan 12, 2023 at 8:26 AM Lorenzo Bianconi <lorenzo@kernel.org> wrote: > > > On Wed, 2023-01-11 at 18:22 +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 | 7 ++++ > > > 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, 57 insertions(+) > > > > > > > Do we have any updates on the implementation that would be making use > > of this? It looks like there was a discussion for the v2 of this set to > > include a link to an RFC posting that would make use of this set. > > I posted the series to linux-wireless mailing list adding netdev one in cc: > https://lore.kernel.org/linux-wireless/cover.1673103214.git.lorenzo@kernel.org/T/#md34b4ffcb07056794378fa4e8079458ecca69109 Thanks. It would be useful to include this link in the next revision to make it easier to review. > > > > > diff --git a/drivers/net/ethernet/mediatek/mtk_eth_soc.c b/drivers/net/ethernet/mediatek/mtk_eth_soc.c > > > index 1af74e9a6cd3..0147e98009c2 100644 > > > --- a/drivers/net/ethernet/mediatek/mtk_eth_soc.c > > > +++ b/drivers/net/ethernet/mediatek/mtk_eth_soc.c > > > @@ -3924,6 +3924,11 @@ static void mtk_pending_work(struct work_struct *work) > > > set_bit(MTK_RESETTING, ð->state); > > > > > > mtk_prepare_for_reset(eth); > > > + mtk_wed_fe_reset(); > > > + /* Run again reset preliminary configuration in order to avoid any > > > + * possible race during FE reset since it can run releasing RTNL lock. > > > + */ > > > + mtk_prepare_for_reset(eth); > > > > > > /* stop all devices to make sure that dma is properly shut down */ > > > for (i = 0; i < MTK_MAC_COUNT; i++) { > > > @@ -3961,6 +3966,8 @@ static void mtk_pending_work(struct work_struct *work) > > > > > > clear_bit(MTK_RESETTING, ð->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"); > > > > The reason why having the consumer would be useful are cases like this. > > My main concern is if the error value might be useful to actually > > expose rather than just treating it as a boolean. Usually for things > > like this I prefer to see the result captured and if it indicates error > > we return the error value since this could be one of several possible > > causes for the error assuming this returns an int and not a bool. > > we can have 2 independent wireless chips connected here so, if the first one > fails, should we exit or just log the error? I would think you should log the error. I notice in your wireless implementation you can return BUSY or TIMEOUT. Rather than doing the dev_err in your reset function to distinguish between the two you could just return the error and leave the printing of the error to this dev_err message. Also a follow-on question I had. It looks like reset_complete returns an int but it is being ignored and in your implementation it is just returning 0. Should that be a void instead of an int?
> On Thu, Jan 12, 2023 at 8:26 AM Lorenzo Bianconi <lorenzo@kernel.org> wrote: > > > > > On Wed, 2023-01-11 at 18:22 +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 | 7 ++++ > > > > 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, 57 insertions(+) > > > > > > > > > > Do we have any updates on the implementation that would be making use > > > of this? It looks like there was a discussion for the v2 of this set to > > > include a link to an RFC posting that would make use of this set. > > > > I posted the series to linux-wireless mailing list adding netdev one in cc: > > https://lore.kernel.org/linux-wireless/cover.1673103214.git.lorenzo@kernel.org/T/#md34b4ffcb07056794378fa4e8079458ecca69109 > > Thanks. It would be useful to include this link in the next revision > to make it easier to review. ack, will do. > > > > > > > > diff --git a/drivers/net/ethernet/mediatek/mtk_eth_soc.c b/drivers/net/ethernet/mediatek/mtk_eth_soc.c > > > > index 1af74e9a6cd3..0147e98009c2 100644 > > > > --- a/drivers/net/ethernet/mediatek/mtk_eth_soc.c > > > > +++ b/drivers/net/ethernet/mediatek/mtk_eth_soc.c > > > > @@ -3924,6 +3924,11 @@ static void mtk_pending_work(struct work_struct *work) > > > > set_bit(MTK_RESETTING, ð->state); > > > > > > > > mtk_prepare_for_reset(eth); > > > > + mtk_wed_fe_reset(); > > > > + /* Run again reset preliminary configuration in order to avoid any > > > > + * possible race during FE reset since it can run releasing RTNL lock. > > > > + */ > > > > + mtk_prepare_for_reset(eth); > > > > > > > > /* stop all devices to make sure that dma is properly shut down */ > > > > for (i = 0; i < MTK_MAC_COUNT; i++) { > > > > @@ -3961,6 +3966,8 @@ static void mtk_pending_work(struct work_struct *work) > > > > > > > > clear_bit(MTK_RESETTING, ð->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"); > > > > > > The reason why having the consumer would be useful are cases like this. > > > My main concern is if the error value might be useful to actually > > > expose rather than just treating it as a boolean. Usually for things > > > like this I prefer to see the result captured and if it indicates error > > > we return the error value since this could be one of several possible > > > causes for the error assuming this returns an int and not a bool. > > > > we can have 2 independent wireless chips connected here so, if the first one > > fails, should we exit or just log the error? > > I would think you should log the error. I notice in your wireless > implementation you can return BUSY or TIMEOUT. Rather than doing the > dev_err in your reset function to distinguish between the two you > could just return the error and leave the printing of the error to > this dev_err message. ack, will do. > > Also a follow-on question I had. It looks like reset_complete returns > an int but it is being ignored and in your implementation it is just > returning 0. Should that be a void instead of an int? > ack, will do. Regards, Lorenzo
diff --git a/drivers/net/ethernet/mediatek/mtk_eth_soc.c b/drivers/net/ethernet/mediatek/mtk_eth_soc.c index 1af74e9a6cd3..0147e98009c2 100644 --- a/drivers/net/ethernet/mediatek/mtk_eth_soc.c +++ b/drivers/net/ethernet/mediatek/mtk_eth_soc.c @@ -3924,6 +3924,11 @@ static void mtk_pending_work(struct work_struct *work) set_bit(MTK_RESETTING, ð->state); mtk_prepare_for_reset(eth); + mtk_wed_fe_reset(); + /* Run again reset preliminary configuration in order to avoid any + * possible race during FE reset since it can run releasing RTNL lock. + */ + mtk_prepare_for_reset(eth); /* stop all devices to make sure that dma is properly shut down */ for (i = 0; i < MTK_MAC_COUNT; i++) { @@ -3961,6 +3966,8 @@ static void mtk_pending_work(struct work_struct *work) clear_bit(MTK_RESETTING, ð->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 };