| Message ID | cedc0a98fb419f3d520a38271628e5d35a01be97.1694507095.git.lorenzo@kernel.org (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next] net: ethernet: mtk_wed: do not assume offload callbacks are always set | expand |
On Tue, Sep 12, 2023 at 10:26:07AM +0200, Lorenzo Bianconi wrote: > Check if wlan.offload_enable and wlan.offload_disable callbacks are set > in mtk_wed_flow_add/mtk_wed_flow_remove since mt7996 will not rely > on them. > > Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org> Hi Lorenzo, It's not not a big deal from my perspective, but I do wonder if these mediatek patches could have been a series. > --- > drivers/net/ethernet/mediatek/mtk_wed.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/drivers/net/ethernet/mediatek/mtk_wed.c b/drivers/net/ethernet/mediatek/mtk_wed.c > index 94376aa2b34c..d8cd59f44401 100644 > --- a/drivers/net/ethernet/mediatek/mtk_wed.c > +++ b/drivers/net/ethernet/mediatek/mtk_wed.c > @@ -1718,6 +1718,9 @@ int mtk_wed_flow_add(int index) > if (!hw || !hw->wed_dev) > return -ENODEV; > > + if (!hw->wed_dev->wlan.offload_enable) > + return 0; A little further down in this function it is assumed that hw->wed_dev may be NULL, a check made under a lock no less. But it is dereferenced unconditionally here without a lock. This doesn't seem right one way or another. As flagged by Smatch. > + > if (hw->num_flows) { > hw->num_flows++; > return 0; > @@ -1747,6 +1750,9 @@ void mtk_wed_flow_remove(int index) > if (!hw) > return; > > + if (!hw->wed_dev->wlan.offload_disable) > + return; > + > if (--hw->num_flows) > return; > > -- > 2.41.0 > >
> On Tue, Sep 12, 2023 at 10:26:07AM +0200, Lorenzo Bianconi wrote: > > Check if wlan.offload_enable and wlan.offload_disable callbacks are set > > in mtk_wed_flow_add/mtk_wed_flow_remove since mt7996 will not rely > > on them. > > > > Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org> > > Hi Lorenzo, > > It's not not a big deal from my perspective, but > I do wonder if these mediatek patches could have been a series. > > > --- > > drivers/net/ethernet/mediatek/mtk_wed.c | 6 ++++++ > > 1 file changed, 6 insertions(+) > > > > diff --git a/drivers/net/ethernet/mediatek/mtk_wed.c b/drivers/net/ethernet/mediatek/mtk_wed.c > > index 94376aa2b34c..d8cd59f44401 100644 > > --- a/drivers/net/ethernet/mediatek/mtk_wed.c > > +++ b/drivers/net/ethernet/mediatek/mtk_wed.c > > @@ -1718,6 +1718,9 @@ int mtk_wed_flow_add(int index) > > if (!hw || !hw->wed_dev) > > return -ENODEV; > > > > + if (!hw->wed_dev->wlan.offload_enable) > > + return 0; > > A little further down in this function it is assumed that hw->wed_dev may > be NULL, a check made under a lock no less. But it is dereferenced > unconditionally here without a lock. This doesn't seem right one way or > another. > > As flagged by Smatch. Hi Simon, you are right. I will fix it in v2. Regards, Lorenzo > > > + > > if (hw->num_flows) { > > hw->num_flows++; > > return 0; > > @@ -1747,6 +1750,9 @@ void mtk_wed_flow_remove(int index) > > if (!hw) > > return; > > > > + if (!hw->wed_dev->wlan.offload_disable) > > + return; > > + > > if (--hw->num_flows) > > return; > > > > -- > > 2.41.0 > > > >
diff --git a/drivers/net/ethernet/mediatek/mtk_wed.c b/drivers/net/ethernet/mediatek/mtk_wed.c index 94376aa2b34c..d8cd59f44401 100644 --- a/drivers/net/ethernet/mediatek/mtk_wed.c +++ b/drivers/net/ethernet/mediatek/mtk_wed.c @@ -1718,6 +1718,9 @@ int mtk_wed_flow_add(int index) if (!hw || !hw->wed_dev) return -ENODEV; + if (!hw->wed_dev->wlan.offload_enable) + return 0; + if (hw->num_flows) { hw->num_flows++; return 0; @@ -1747,6 +1750,9 @@ void mtk_wed_flow_remove(int index) if (!hw) return; + if (!hw->wed_dev->wlan.offload_disable) + return; + if (--hw->num_flows) return;
Check if wlan.offload_enable and wlan.offload_disable callbacks are set in mtk_wed_flow_add/mtk_wed_flow_remove since mt7996 will not rely on them. Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org> --- drivers/net/ethernet/mediatek/mtk_wed.c | 6 ++++++ 1 file changed, 6 insertions(+)