[PATCH net-next] r8169: fix ntohs/htons sparse warnings

[PATCH net-next] r8169: fix ntohs/htons sparse warnings

[Heiner Kallweit]

Sparse complains about casting to/from restricted __be16. Fix this.

Fixes: 759d09574172 ("r8169: improve handling VLAN tag")
Reported-by: kbuild test robot <lkp@intel.com>
Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/net/ethernet/realtek/r8169_main.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/realtek/r8169_main.c b/drivers/net/ethernet/realtek/r8169_main.c
index a73f25321..450c74dc1 100644
--- a/drivers/net/ethernet/realtek/r8169_main.c
+++ b/drivers/net/ethernet/realtek/r8169_main.c
@@ -1528,7 +1528,7 @@ static int rtl8169_set_features(struct net_device *dev,
 static inline u32 rtl8169_tx_vlan_tag(struct sk_buff *skb)
 {
 	return (skb_vlan_tag_present(skb)) ?
-		TxVlanTag | htons(skb_vlan_tag_get(skb)) : 0x00;
+		TxVlanTag | (__force u16)htons(skb_vlan_tag_get(skb)) : 0x00;
 }
 
 static void rtl8169_rx_vlan_tag(struct RxDesc *desc, struct sk_buff *skb)
@@ -1537,7 +1537,7 @@ static void rtl8169_rx_vlan_tag(struct RxDesc *desc, struct sk_buff *skb)
 
 	if (opts2 & RxVlanTag)
 		__vlan_hwaccel_put_tag(skb, htons(ETH_P_8021Q),
-				       ntohs(opts2 & 0xffff));
+				       ntohs((__force __be16)(opts2 & 0xffff)));
 }
 
 static void rtl8169_get_regs(struct net_device *dev, struct ethtool_regs *regs,
-- 
2.22.0

Re: [PATCH net-next] r8169: fix ntohs/htons sparse warnings

[Al Viro]

On Mon, Jul 01, 2019 at 09:35:28PM +0200, Heiner Kallweit wrote:
> Sparse complains about casting to/from restricted __be16. Fix this.

Fix what, exactly?  Force-cast is not a fix - it's "STFU, I know
better, it's really correct" to sparse.  Which may or may not
match the reality, but it definitely requires more in way of
commit message than "sparse says it's wrong; shut it up".

>  static void rtl8169_rx_vlan_tag(struct RxDesc *desc, struct sk_buff *skb)
> @@ -1537,7 +1537,7 @@ static void rtl8169_rx_vlan_tag(struct RxDesc *desc, struct sk_buff *skb)
>  
>  	if (opts2 & RxVlanTag)
>  		__vlan_hwaccel_put_tag(skb, htons(ETH_P_8021Q),
> -				       ntohs(opts2 & 0xffff));
> +				       ntohs((__force __be16)(opts2 & 0xffff)));
>  }

Should that be ntohs at all?  What behaviour is correct on big-endian host?

AFAICS, in that code opts2 comes from little-endian 32bit.  It's converted to
host-endian, lower 16 bits (i.e. the first two octets in memory) are then
fed to ntohs.  Suppose we had in-core value stored as A0, A1, A2, A3.
On little-endian that code will yield A0 * 256 + A1, treated as host-endian.
On big-endian the same will yield A1 * 256 + A0.  Is that actually correct?

The code dealing with the value passed to __vlan_hwaccel_put_tag() as the
third argument treats it as a host-endian integer.  So... Has anyone
tested that code on b-e host?  Should that ntohs() actually be swab16(),
yielding (on any host) the same value we currently get for l-e hosts only?

Re: [PATCH net-next] r8169: fix ntohs/htons sparse warnings

[Heiner Kallweit]

On 01.07.2019 21:56, Al Viro wrote:
> On Mon, Jul 01, 2019 at 09:35:28PM +0200, Heiner Kallweit wrote:
>> Sparse complains about casting to/from restricted __be16. Fix this.
> 
> Fix what, exactly?  Force-cast is not a fix - it's "STFU, I know
> better, it's really correct" to sparse.  Which may or may not
> match the reality, but it definitely requires more in way of
> commit message than "sparse says it's wrong; shut it up".
> 
>>  static void rtl8169_rx_vlan_tag(struct RxDesc *desc, struct sk_buff *skb)
>> @@ -1537,7 +1537,7 @@ static void rtl8169_rx_vlan_tag(struct RxDesc *desc, struct sk_buff *skb)
>>  
>>  	if (opts2 & RxVlanTag)
>>  		__vlan_hwaccel_put_tag(skb, htons(ETH_P_8021Q),
>> -				       ntohs(opts2 & 0xffff));
>> +				       ntohs((__force __be16)(opts2 & 0xffff)));
>>  }
> 
> Should that be ntohs at all?  What behaviour is correct on big-endian host?
> 
> AFAICS, in that code opts2 comes from little-endian 32bit.  It's converted to
> host-endian, lower 16 bits (i.e. the first two octets in memory) are then
> fed to ntohs.  Suppose we had in-core value stored as A0, A1, A2, A3.
> On little-endian that code will yield A0 * 256 + A1, treated as host-endian.
> On big-endian the same will yield A1 * 256 + A0.  Is that actually correct?
> 
I think you're right and the original patch should be reverted.

> The code dealing with the value passed to __vlan_hwaccel_put_tag() as the
> third argument treats it as a host-endian integer.  So... Has anyone
> tested that code on b-e host?  Should that ntohs() actually be swab16(),
> yielding (on any host) the same value we currently get for l-e hosts only?
> 
I haven't seen any b-e host with a Realtek network chip yet.

Re: [PATCH net-next] r8169: fix ntohs/htons sparse warnings

[Al Viro]

On Mon, Jul 01, 2019 at 10:36:26PM +0200, Heiner Kallweit wrote:

> > The code dealing with the value passed to __vlan_hwaccel_put_tag() as the
> > third argument treats it as a host-endian integer.  So... Has anyone
> > tested that code on b-e host?  Should that ntohs() actually be swab16(),
> > yielding (on any host) the same value we currently get for l-e hosts only?
> > 
> I haven't seen any b-e host with a Realtek network chip yet.

Ever tried to google for realtek 8169 pcie card?  The first hit is this:
https://www.amazon.com/Realtek-Chipset-Ethernet-Interface-Software/dp/B007MWYCG2
and certainly does look like it should fit into at least some G5 Macs.
What's more, googling for realtek 8169 PCI card brings quite a bit (top
hit happens to be on ebay for ~$8).  That certainly shall fit into
any number of big-endian motherboards...

Sure, there's a plenty of embedded r8169 on motherboards (mostly x86 ones),
but these beasts do exist on discrete cards.  I'm fairly certain that I've
got one or two somewhere in the detritus pile and they are fairly cheap
these days.

So it wouldn't cost too much to put together a mixed network, with
r8169 both on l-e and b-e hosts and play with VLAN setups there...

Re: [PATCH net-next] r8169: fix ntohs/htons sparse warnings

[Al Viro]

On Mon, Jul 01, 2019 at 10:13:56PM +0100, Al Viro wrote:
> On Mon, Jul 01, 2019 at 10:36:26PM +0200, Heiner Kallweit wrote:
> 
> > > The code dealing with the value passed to __vlan_hwaccel_put_tag() as the
> > > third argument treats it as a host-endian integer.  So... Has anyone
> > > tested that code on b-e host?  Should that ntohs() actually be swab16(),
> > > yielding (on any host) the same value we currently get for l-e hosts only?
> > > 
> > I haven't seen any b-e host with a Realtek network chip yet.
> 
> Ever tried to google for realtek 8169 pcie card?  The first hit is this:
> https://www.amazon.com/Realtek-Chipset-Ethernet-Interface-Software/dp/B007MWYCG2
> and certainly does look like it should fit into at least some G5 Macs.
> What's more, googling for realtek 8169 PCI card brings quite a bit (top
> hit happens to be on ebay for ~$8).  That certainly shall fit into
> any number of big-endian motherboards...
> 
> Sure, there's a plenty of embedded r8169 on motherboards (mostly x86 ones),
> but these beasts do exist on discrete cards.  I'm fairly certain that I've
> got one or two somewhere in the detritus pile and they are fairly cheap
> these days.
> 
> So it wouldn't cost too much to put together a mixed network, with
> r8169 both on l-e and b-e hosts and play with VLAN setups there...

FWIW, looking at r8169 docs, they say that Rx descriptor has, at offset
4, a 32bit value (l-e) with
	bits 17..31 reserved
	bit 16: TAVA (Tag Available)
	bits 8..15: VIDL (lower 8 bits of VLAN ID)
	bits 5..7: PRIO (priority)
	bit 4: CFI (Canonical Format Indicator)
	bits 0..3: VIDH (upper 4 bits of VLAN ID)

AFAICS, in kernel-side representation we want VIDL in bits 0..7, VIDH - 8..11,
CFI - 12 and PRIO - 13..15, so that VLAN ID is simply value & 0xfff.

IOW, we do want 256 * octet4 + octet5, i.e.
	swab16(le32_to_cpu(desc->opts2) & 0xffff).
Regardless of the host endianness.  Some cards might be possible to set up to
byteswap the 32bit values on the way to/from host, but even in that case we
would have wanted
	swab16(desc->opts2 & 0xffff),
not
	be16_to_cpu(le32_to_cpu(desc->opts2 & 0xffff)
and if it *is* set that way, the current code will break anyway.

Again, all of that is from RTFS alone - I have _not_ tested that, but
it does look like your original patch has missed that while these
16 bits are stored in network order _in_ _card_ _memory_, we'd
already done cpu_to_le32 to the containing 32bit word.  So the
value is host-endian, and the lower 16 bits are consistently in
the wrong order, be it b-e or l-e host.  IOW, swab16 is the right
thing to do and sparse warning has been correct.

Re: [PATCH net-next] r8169: fix ntohs/htons sparse warnings

[Al Viro]

On Mon, Jul 01, 2019 at 10:46:49PM +0100, Al Viro wrote:
> already done cpu_to_le32 to the containing 32bit word.  So the
               le32_to_cpu, sorry