[PATCH] mwifiex: set needed_headroom, not hard_header_len

[PATCH] mwifiex: set needed_headroom, not hard_header_len

[Brian Norris]

hard_header_len provides limitations for things like AF_PACKET, such
that we don't allow transmitting packets smaller than this.

needed_headroom provides a suggested minimum headroom for SKBs, so that
we can trivally add our headers to the front.

The latter is the correct field to use in this case, while the former
mostly just prevents sending small AF_PACKET frames.

In any case, mwifiex already does its own bounce buffering [1] if we
don't have enough headroom, so hints (not hard limits) are all that are
needed.

This is the essentially the same bug (and fix) that brcmfmac had, fixed
in commit cb39288fd6bb ("brcmfmac: use ndev->needed_headroom to reserve
additional header space").

[1] mwifiex_hard_start_xmit():
	if (skb_headroom(skb) < MWIFIEX_MIN_DATA_HEADER_LEN) {
	[...]
		/* Insufficient skb headroom - allocate a new skb */

Fixes: 5e6e3a92b9a4 ("wireless: mwifiex: initial commit for Marvell mwifiex driver")
Signed-off-by: Brian Norris <briannorris@chromium.org>
---
 drivers/net/wireless/marvell/mwifiex/cfg80211.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/wireless/marvell/mwifiex/cfg80211.c b/drivers/net/wireless/marvell/mwifiex/cfg80211.c
index 0a6da6fe2f89..1566d2197906 100644
--- a/drivers/net/wireless/marvell/mwifiex/cfg80211.c
+++ b/drivers/net/wireless/marvell/mwifiex/cfg80211.c
@@ -3052,7 +3052,7 @@ struct wireless_dev *mwifiex_add_virtual_intf(struct wiphy *wiphy,
 
 	dev->flags |= IFF_BROADCAST | IFF_MULTICAST;
 	dev->watchdog_timeo = MWIFIEX_DEFAULT_WATCHDOG_TIMEOUT;
-	dev->hard_header_len += MWIFIEX_MIN_DATA_HEADER_LEN;
+	dev->needed_headroom = MWIFIEX_MIN_DATA_HEADER_LEN;
 	dev->ethtool_ops = &mwifiex_ethtool_ops;
 
 	mdev_priv = netdev_priv(dev);
-- 
2.25.0.265.gbab2e86ba0-goog

RE: [EXT] [PATCH] mwifiex: set needed_headroom, not hard_header_len

[Ganapathi Bhat]

Hi Brian,
 
> hard_header_len provides limitations for things like AF_PACKET, such that
> we don't allow transmitting packets smaller than this.


OK; However, are we not supposed to mention hard_header_len also?

> 
> needed_headroom provides a suggested minimum headroom for SKBs, so
> that we can trivally add our headers to the front.
> 
> The latter is the correct field to use in this case, while the former mostly just
> prevents sending small AF_PACKET frames.
> 
> In any case, mwifiex already does its own bounce buffering [1] if we don't
> have enough headroom, so hints (not hard limits) are all that are needed.
> 
> This is the essentially the same bug (and fix) that brcmfmac had, fixed in
> commit cb39288fd6bb ("brcmfmac: use ndev->needed_headroom to reserve
> additional header space").

OK; I read this commit:

"... According to definition of LL_RESERVED_SPACE() and hard_header_len, we should use hard_header_len to reserve for L2 header, like ethernet header(ETH_HLEN) in our case and use needed_headroom for the additional headroom needed by hardware..."

So, does it mean, hard_header_len is already considered by upper layer?


Regards,
Ganapathi

Re: [EXT] [PATCH] mwifiex: set needed_headroom, not hard_header_len

[Brian Norris]

Hi Ganapathi,

On Wed, Mar 4, 2020 at 8:00 PM Ganapathi Bhat <ganapathi.bhat@nxp.com> wrote:
> > hard_header_len provides limitations for things like AF_PACKET, such that
> > we don't allow transmitting packets smaller than this.
>
> OK; However, are we not supposed to mention hard_header_len also?

No, my understanding is that we do not need to bother with
hard_header_len ourselves -- ether_setup() establishes the appropriate
L2 header parameters. I think that's covered a little better below.

> > This is the essentially the same bug (and fix) that brcmfmac had, fixed in
> > commit cb39288fd6bb ("brcmfmac: use ndev->needed_headroom to reserve
> > additional header space").
>
> OK; I read this commit:
>
> "... According to definition of LL_RESERVED_SPACE() and hard_header_len, we should use hard_header_len to reserve for L2 header, like ethernet header(ETH_HLEN) in our case and use needed_headroom for the additional headroom needed by hardware..."

Yeah, that's probably a little more verbose and accurate description,
which is partly why I referred to that commit :)

> So, does it mean, hard_header_len is already considered by upper layer?

Right, it's set by ether_setup().

Brian

RE: [EXT] [PATCH] mwifiex: set needed_headroom, not hard_header_len

[Ganapathi Bhat]

Hi Brian,

> > > hard_header_len provides limitations for things like AF_PACKET, such
> > > that we don't allow transmitting packets smaller than this.
> >
> > OK; However, are we not supposed to mention hard_header_len also?
> 
> No, my understanding is that we do not need to bother with
> hard_header_len ourselves -- ether_setup() establishes the appropriate
> L2 header parameters. I think that's covered a little better below.

OK. I got you.
> 
> > > This is the essentially the same bug (and fix) that brcmfmac had,
> > > fixed in commit cb39288fd6bb ("brcmfmac: use ndev-
> >needed_headroom
> > > to reserve additional header space").
> >
> > OK; I read this commit:
> >
> > "... According to definition of LL_RESERVED_SPACE() and hard_header_len,
> we should use hard_header_len to reserve for L2 header, like ethernet
> header(ETH_HLEN) in our case and use needed_headroom for the additional
> headroom needed by hardware..."
> 
> Yeah, that's probably a little more verbose and accurate description, which is
> partly why I referred to that commit :)
> 
> > So, does it mean, hard_header_len is already considered by upper layer?
> 
> Right, it's set by ether_setup().

Yes, Thanks.

Acked-by: Ganapathi Bhat <ganapathi.gbhat@nxp.com>

Regards,
Ganapathi

Re: [PATCH] mwifiex: set needed_headroom, not hard_header_len

[Kalle Valo]

Brian Norris <briannorris@chromium.org> wrote:

> hard_header_len provides limitations for things like AF_PACKET, such
> that we don't allow transmitting packets smaller than this.
> 
> needed_headroom provides a suggested minimum headroom for SKBs, so that
> we can trivally add our headers to the front.
> 
> The latter is the correct field to use in this case, while the former
> mostly just prevents sending small AF_PACKET frames.
> 
> In any case, mwifiex already does its own bounce buffering [1] if we
> don't have enough headroom, so hints (not hard limits) are all that are
> needed.
> 
> This is the essentially the same bug (and fix) that brcmfmac had, fixed
> in commit cb39288fd6bb ("brcmfmac: use ndev->needed_headroom to reserve
> additional header space").
> 
> [1] mwifiex_hard_start_xmit():
> 	if (skb_headroom(skb) < MWIFIEX_MIN_DATA_HEADER_LEN) {
> 	[...]
> 		/* Insufficient skb headroom - allocate a new skb */
> 
> Fixes: 5e6e3a92b9a4 ("wireless: mwifiex: initial commit for Marvell mwifiex driver")
> Signed-off-by: Brian Norris <briannorris@chromium.org>
> Acked-by: Ganapathi Bhat <ganapathi.gbhat@nxp.com>

Patch applied to wireless-drivers-next.git, thanks.

9454f7a895b8 mwifiex: set needed_headroom, not hard_header_len

-- 
https://patchwork.kernel.org/patch/11407493/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches