[PATCH wpan-next] mac802154: Avoid superfluous endianness handling

[PATCH wpan-next] mac802154: Avoid superfluous endianness handling

[Miquel Raynal]

When compiling scan.c with C=1, Sparse complains with:

   sparse:     expected unsigned short [usertype] val
   sparse:     got restricted __le16 [usertype] pan_id
   sparse: sparse: cast from restricted __le16

   sparse:     expected unsigned long long [usertype] val
   sparse:     got restricted __le64 [usertype] extended_addr
   sparse: sparse: cast from restricted __le64

The tool is right, both pan_id and extended_addr already are rightfully
defined as being __le16 and __le64 on both sides of the operations and
do not require extra endianness handling.

Fixes: 3accf4762734 ("mac802154: Handle basic beaconing")
Reported-by: kernel test robot <lkp@intel.com>
Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 net/mac802154/scan.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/mac802154/scan.c b/net/mac802154/scan.c
index cfbe20b1ec5e..8f98efec7753 100644
--- a/net/mac802154/scan.c
+++ b/net/mac802154/scan.c
@@ -419,8 +419,8 @@ int mac802154_send_beacons_locked(struct ieee802154_sub_if_data *sdata,
 	local->beacon.mhr.fc.source_addr_mode = IEEE802154_EXTENDED_ADDRESSING;
 	atomic_set(&request->wpan_dev->bsn, -1);
 	local->beacon.mhr.source.mode = IEEE802154_ADDR_LONG;
-	local->beacon.mhr.source.pan_id = cpu_to_le16(request->wpan_dev->pan_id);
-	local->beacon.mhr.source.extended_addr = cpu_to_le64(request->wpan_dev->extended_addr);
+	local->beacon.mhr.source.pan_id = request->wpan_dev->pan_id;
+	local->beacon.mhr.source.extended_addr = request->wpan_dev->extended_addr;
 	local->beacon.mac_pl.beacon_order = request->interval;
 	local->beacon.mac_pl.superframe_order = request->interval;
 	local->beacon.mac_pl.final_cap_slot = 0xf;
-- 
2.34.1

Re: [PATCH wpan-next] mac802154: Avoid superfluous endianness handling

[Stefan Schmidt]

Hello.

On 30.01.23 16:43, Miquel Raynal wrote:
> When compiling scan.c with C=1, Sparse complains with:
> 
>     sparse:     expected unsigned short [usertype] val
>     sparse:     got restricted __le16 [usertype] pan_id
>     sparse: sparse: cast from restricted __le16
> 
>     sparse:     expected unsigned long long [usertype] val
>     sparse:     got restricted __le64 [usertype] extended_addr
>     sparse: sparse: cast from restricted __le64
> 
> The tool is right, both pan_id and extended_addr already are rightfully
> defined as being __le16 and __le64 on both sides of the operations and
> do not require extra endianness handling.
> 
> Fixes: 3accf4762734 ("mac802154: Handle basic beaconing")
> Reported-by: kernel test robot <lkp@intel.com>
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> ---
>   net/mac802154/scan.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/net/mac802154/scan.c b/net/mac802154/scan.c
> index cfbe20b1ec5e..8f98efec7753 100644
> --- a/net/mac802154/scan.c
> +++ b/net/mac802154/scan.c
> @@ -419,8 +419,8 @@ int mac802154_send_beacons_locked(struct ieee802154_sub_if_data *sdata,
>   	local->beacon.mhr.fc.source_addr_mode = IEEE802154_EXTENDED_ADDRESSING;
>   	atomic_set(&request->wpan_dev->bsn, -1);
>   	local->beacon.mhr.source.mode = IEEE802154_ADDR_LONG;
> -	local->beacon.mhr.source.pan_id = cpu_to_le16(request->wpan_dev->pan_id);
> -	local->beacon.mhr.source.extended_addr = cpu_to_le64(request->wpan_dev->extended_addr);
> +	local->beacon.mhr.source.pan_id = request->wpan_dev->pan_id;
> +	local->beacon.mhr.source.extended_addr = request->wpan_dev->extended_addr;
>   	local->beacon.mac_pl.beacon_order = request->interval;
>   	local->beacon.mac_pl.superframe_order = request->interval;
>   	local->beacon.mac_pl.final_cap_slot = 0xf;

This patch has been applied to the wpan-next tree and will be
part of the next pull request to net-next. Thanks!

regards
Stefan Schmidt

Re: [PATCH wpan-next] mac802154: Avoid superfluous endianness handling

[Alexander Aring]

Hi,

On Mon, Jan 30, 2023 at 11:34 AM Stefan Schmidt
<stefan@datenfreihafen.org> wrote:
>
> Hello.
>
> On 30.01.23 16:43, Miquel Raynal wrote:
> > When compiling scan.c with C=1, Sparse complains with:
> >
> >     sparse:     expected unsigned short [usertype] val
> >     sparse:     got restricted __le16 [usertype] pan_id
> >     sparse: sparse: cast from restricted __le16
> >
> >     sparse:     expected unsigned long long [usertype] val
> >     sparse:     got restricted __le64 [usertype] extended_addr
> >     sparse: sparse: cast from restricted __le64
> >
> > The tool is right, both pan_id and extended_addr already are rightfully
> > defined as being __le16 and __le64 on both sides of the operations and
> > do not require extra endianness handling.
> >
> > Fixes: 3accf4762734 ("mac802154: Handle basic beaconing")
> > Reported-by: kernel test robot <lkp@intel.com>
> > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> > ---
> >   net/mac802154/scan.c | 4 ++--
> >   1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/net/mac802154/scan.c b/net/mac802154/scan.c
> > index cfbe20b1ec5e..8f98efec7753 100644
> > --- a/net/mac802154/scan.c
> > +++ b/net/mac802154/scan.c
> > @@ -419,8 +419,8 @@ int mac802154_send_beacons_locked(struct ieee802154_sub_if_data *sdata,
> >       local->beacon.mhr.fc.source_addr_mode = IEEE802154_EXTENDED_ADDRESSING;
> >       atomic_set(&request->wpan_dev->bsn, -1);
> >       local->beacon.mhr.source.mode = IEEE802154_ADDR_LONG;
> > -     local->beacon.mhr.source.pan_id = cpu_to_le16(request->wpan_dev->pan_id);
> > -     local->beacon.mhr.source.extended_addr = cpu_to_le64(request->wpan_dev->extended_addr);
> > +     local->beacon.mhr.source.pan_id = request->wpan_dev->pan_id;
> > +     local->beacon.mhr.source.extended_addr = request->wpan_dev->extended_addr;
> >       local->beacon.mac_pl.beacon_order = request->interval;
> >       local->beacon.mac_pl.superframe_order = request->interval;
> >       local->beacon.mac_pl.final_cap_slot = 0xf;
>
> This patch has been applied to the wpan-next tree and will be
> part of the next pull request to net-next. Thanks!

fyi: in my opinion, depending on system endianness this is actually a bug.

- Alex

Re: [PATCH wpan-next] mac802154: Avoid superfluous endianness handling

[Miquel Raynal]

Hi Alexander,

aahringo@redhat.com wrote on Mon, 30 Jan 2023 11:41:20 -0500:

> Hi,
> 
> On Mon, Jan 30, 2023 at 11:34 AM Stefan Schmidt
> <stefan@datenfreihafen.org> wrote:
> >
> > Hello.
> >
> > On 30.01.23 16:43, Miquel Raynal wrote:  
> > > When compiling scan.c with C=1, Sparse complains with:
> > >
> > >     sparse:     expected unsigned short [usertype] val
> > >     sparse:     got restricted __le16 [usertype] pan_id
> > >     sparse: sparse: cast from restricted __le16
> > >
> > >     sparse:     expected unsigned long long [usertype] val
> > >     sparse:     got restricted __le64 [usertype] extended_addr
> > >     sparse: sparse: cast from restricted __le64
> > >
> > > The tool is right, both pan_id and extended_addr already are rightfully
> > > defined as being __le16 and __le64 on both sides of the operations and
> > > do not require extra endianness handling.
> > >
> > > Fixes: 3accf4762734 ("mac802154: Handle basic beaconing")
> > > Reported-by: kernel test robot <lkp@intel.com>
> > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> > > ---
> > >   net/mac802154/scan.c | 4 ++--
> > >   1 file changed, 2 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/net/mac802154/scan.c b/net/mac802154/scan.c
> > > index cfbe20b1ec5e..8f98efec7753 100644
> > > --- a/net/mac802154/scan.c
> > > +++ b/net/mac802154/scan.c
> > > @@ -419,8 +419,8 @@ int mac802154_send_beacons_locked(struct ieee802154_sub_if_data *sdata,
> > >       local->beacon.mhr.fc.source_addr_mode = IEEE802154_EXTENDED_ADDRESSING;
> > >       atomic_set(&request->wpan_dev->bsn, -1);
> > >       local->beacon.mhr.source.mode = IEEE802154_ADDR_LONG;
> > > -     local->beacon.mhr.source.pan_id = cpu_to_le16(request->wpan_dev->pan_id);
> > > -     local->beacon.mhr.source.extended_addr = cpu_to_le64(request->wpan_dev->extended_addr);
> > > +     local->beacon.mhr.source.pan_id = request->wpan_dev->pan_id;
> > > +     local->beacon.mhr.source.extended_addr = request->wpan_dev->extended_addr;
> > >       local->beacon.mac_pl.beacon_order = request->interval;
> > >       local->beacon.mac_pl.superframe_order = request->interval;
> > >       local->beacon.mac_pl.final_cap_slot = 0xf;  
> >
> > This patch has been applied to the wpan-next tree and will be
> > part of the next pull request to net-next. Thanks!  
> 
> fyi: in my opinion, depending on system endianness this is actually a bug.

Actually there are many uses of __le16 and __le64 for PAN IDs, short
and extended addresses. I did follow the existing patterns, I think
they are legitimate. Can you clarify what you think is a bug in the
current state? I always feel a bit flaky when it comes to properly
handling endianness, so all feedback welcome, if you have any hints
of what should be fixed after this patch, I'll do it. 

Thanks,
Miquèl

Re: [PATCH wpan-next] mac802154: Avoid superfluous endianness handling

[Alexander Aring]

Hi,

On Tue, Jan 31, 2023 at 6:03 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote:
>
> Hi Alexander,
>
> aahringo@redhat.com wrote on Mon, 30 Jan 2023 11:41:20 -0500:
>
> > Hi,
> >
> > On Mon, Jan 30, 2023 at 11:34 AM Stefan Schmidt
> > <stefan@datenfreihafen.org> wrote:
> > >
> > > Hello.
> > >
> > > On 30.01.23 16:43, Miquel Raynal wrote:
> > > > When compiling scan.c with C=1, Sparse complains with:
> > > >
> > > >     sparse:     expected unsigned short [usertype] val
> > > >     sparse:     got restricted __le16 [usertype] pan_id
> > > >     sparse: sparse: cast from restricted __le16
> > > >
> > > >     sparse:     expected unsigned long long [usertype] val
> > > >     sparse:     got restricted __le64 [usertype] extended_addr
> > > >     sparse: sparse: cast from restricted __le64
> > > >
> > > > The tool is right, both pan_id and extended_addr already are rightfully
> > > > defined as being __le16 and __le64 on both sides of the operations and
> > > > do not require extra endianness handling.
> > > >
> > > > Fixes: 3accf4762734 ("mac802154: Handle basic beaconing")
> > > > Reported-by: kernel test robot <lkp@intel.com>
> > > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> > > > ---
> > > >   net/mac802154/scan.c | 4 ++--
> > > >   1 file changed, 2 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/net/mac802154/scan.c b/net/mac802154/scan.c
> > > > index cfbe20b1ec5e..8f98efec7753 100644
> > > > --- a/net/mac802154/scan.c
> > > > +++ b/net/mac802154/scan.c
> > > > @@ -419,8 +419,8 @@ int mac802154_send_beacons_locked(struct ieee802154_sub_if_data *sdata,
> > > >       local->beacon.mhr.fc.source_addr_mode = IEEE802154_EXTENDED_ADDRESSING;
> > > >       atomic_set(&request->wpan_dev->bsn, -1);
> > > >       local->beacon.mhr.source.mode = IEEE802154_ADDR_LONG;
> > > > -     local->beacon.mhr.source.pan_id = cpu_to_le16(request->wpan_dev->pan_id);
> > > > -     local->beacon.mhr.source.extended_addr = cpu_to_le64(request->wpan_dev->extended_addr);
> > > > +     local->beacon.mhr.source.pan_id = request->wpan_dev->pan_id;
> > > > +     local->beacon.mhr.source.extended_addr = request->wpan_dev->extended_addr;
> > > >       local->beacon.mac_pl.beacon_order = request->interval;
> > > >       local->beacon.mac_pl.superframe_order = request->interval;
> > > >       local->beacon.mac_pl.final_cap_slot = 0xf;
> > >
> > > This patch has been applied to the wpan-next tree and will be
> > > part of the next pull request to net-next. Thanks!
> >
> > fyi: in my opinion, depending on system endianness this is actually a bug.
>
> Actually there are many uses of __le16 and __le64 for PAN IDs, short
> and extended addresses. I did follow the existing patterns, I think
> they are legitimate. Can you clarify what you think is a bug in the
> current state? I always feel a bit flaky when it comes to properly
> handling endianness, so all feedback welcome, if you have any hints
> of what should be fixed after this patch, I'll do it.
>

net/ policy is so far I understood to always use endianness how it's
stored on wire. There is no bug that addresses are stored as little
endian, but there is a bug doing a conversion when it's not necessary.
In this example, so far I see, big endian does a byteswap here.
And as you figured there will be less byteswaps when it's stored as
little endian. Consider this in netlink, that we store things as it is
on wire (or in our case on air/wireless).

- Alex

Re: [PATCH wpan-next] mac802154: Avoid superfluous endianness handling

[Miquel Raynal]

Hi Alexander,

aahringo@redhat.com wrote on Sun, 5 Feb 2023 20:37:45 -0500:

> Hi,
> 
> On Tue, Jan 31, 2023 at 6:03 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> >
> > Hi Alexander,
> >
> > aahringo@redhat.com wrote on Mon, 30 Jan 2023 11:41:20 -0500:
> >  
> > > Hi,
> > >
> > > On Mon, Jan 30, 2023 at 11:34 AM Stefan Schmidt
> > > <stefan@datenfreihafen.org> wrote:  
> > > >
> > > > Hello.
> > > >
> > > > On 30.01.23 16:43, Miquel Raynal wrote:  
> > > > > When compiling scan.c with C=1, Sparse complains with:
> > > > >
> > > > >     sparse:     expected unsigned short [usertype] val
> > > > >     sparse:     got restricted __le16 [usertype] pan_id
> > > > >     sparse: sparse: cast from restricted __le16
> > > > >
> > > > >     sparse:     expected unsigned long long [usertype] val
> > > > >     sparse:     got restricted __le64 [usertype] extended_addr
> > > > >     sparse: sparse: cast from restricted __le64
> > > > >
> > > > > The tool is right, both pan_id and extended_addr already are rightfully
> > > > > defined as being __le16 and __le64 on both sides of the operations and
> > > > > do not require extra endianness handling.
> > > > >
> > > > > Fixes: 3accf4762734 ("mac802154: Handle basic beaconing")
> > > > > Reported-by: kernel test robot <lkp@intel.com>
> > > > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> > > > > ---
> > > > >   net/mac802154/scan.c | 4 ++--
> > > > >   1 file changed, 2 insertions(+), 2 deletions(-)
> > > > >
> > > > > diff --git a/net/mac802154/scan.c b/net/mac802154/scan.c
> > > > > index cfbe20b1ec5e..8f98efec7753 100644
> > > > > --- a/net/mac802154/scan.c
> > > > > +++ b/net/mac802154/scan.c
> > > > > @@ -419,8 +419,8 @@ int mac802154_send_beacons_locked(struct ieee802154_sub_if_data *sdata,
> > > > >       local->beacon.mhr.fc.source_addr_mode = IEEE802154_EXTENDED_ADDRESSING;
> > > > >       atomic_set(&request->wpan_dev->bsn, -1);
> > > > >       local->beacon.mhr.source.mode = IEEE802154_ADDR_LONG;
> > > > > -     local->beacon.mhr.source.pan_id = cpu_to_le16(request->wpan_dev->pan_id);
> > > > > -     local->beacon.mhr.source.extended_addr = cpu_to_le64(request->wpan_dev->extended_addr);
> > > > > +     local->beacon.mhr.source.pan_id = request->wpan_dev->pan_id;
> > > > > +     local->beacon.mhr.source.extended_addr = request->wpan_dev->extended_addr;
> > > > >       local->beacon.mac_pl.beacon_order = request->interval;
> > > > >       local->beacon.mac_pl.superframe_order = request->interval;
> > > > >       local->beacon.mac_pl.final_cap_slot = 0xf;  
> > > >
> > > > This patch has been applied to the wpan-next tree and will be
> > > > part of the next pull request to net-next. Thanks!  
> > >
> > > fyi: in my opinion, depending on system endianness this is actually a bug.  
> >
> > Actually there are many uses of __le16 and __le64 for PAN IDs, short
> > and extended addresses. I did follow the existing patterns, I think
> > they are legitimate. Can you clarify what you think is a bug in the
> > current state? I always feel a bit flaky when it comes to properly
> > handling endianness, so all feedback welcome, if you have any hints
> > of what should be fixed after this patch, I'll do it.
> >  
> 
> net/ policy is so far I understood to always use endianness how it's
> stored on wire. There is no bug that addresses are stored as little
> endian, but there is a bug doing a conversion when it's not necessary.
> In this example, so far I see, big endian does a byteswap here.
> And as you figured there will be less byteswaps when it's stored as
> little endian. Consider this in netlink, that we store things as it is
> on wire (or in our case on air/wireless).

Ah ok I get it: "keep the endianness as on the medium". All right so
the __le16/__le64 variables are the right thing to do. Thanks for the
validation.

Thanks,
Miquèl

Re: [PATCH wpan-next] mac802154: Avoid superfluous endianness handling

[Alexander Aring]

Hi,

On Mon, Feb 6, 2023 at 3:53 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote:
>
> Hi Alexander,
>
> aahringo@redhat.com wrote on Sun, 5 Feb 2023 20:37:45 -0500:
>
> > Hi,
> >
> > On Tue, Jan 31, 2023 at 6:03 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> > >
> > > Hi Alexander,
> > >
> > > aahringo@redhat.com wrote on Mon, 30 Jan 2023 11:41:20 -0500:
> > >
> > > > Hi,
> > > >
> > > > On Mon, Jan 30, 2023 at 11:34 AM Stefan Schmidt
> > > > <stefan@datenfreihafen.org> wrote:
> > > > >
> > > > > Hello.
> > > > >
> > > > > On 30.01.23 16:43, Miquel Raynal wrote:
> > > > > > When compiling scan.c with C=1, Sparse complains with:
> > > > > >
> > > > > >     sparse:     expected unsigned short [usertype] val
> > > > > >     sparse:     got restricted __le16 [usertype] pan_id
> > > > > >     sparse: sparse: cast from restricted __le16
> > > > > >
> > > > > >     sparse:     expected unsigned long long [usertype] val
> > > > > >     sparse:     got restricted __le64 [usertype] extended_addr
> > > > > >     sparse: sparse: cast from restricted __le64
> > > > > >
> > > > > > The tool is right, both pan_id and extended_addr already are rightfully
> > > > > > defined as being __le16 and __le64 on both sides of the operations and
> > > > > > do not require extra endianness handling.
> > > > > >
> > > > > > Fixes: 3accf4762734 ("mac802154: Handle basic beaconing")
> > > > > > Reported-by: kernel test robot <lkp@intel.com>
> > > > > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> > > > > > ---
> > > > > >   net/mac802154/scan.c | 4 ++--
> > > > > >   1 file changed, 2 insertions(+), 2 deletions(-)
> > > > > >
> > > > > > diff --git a/net/mac802154/scan.c b/net/mac802154/scan.c
> > > > > > index cfbe20b1ec5e..8f98efec7753 100644
> > > > > > --- a/net/mac802154/scan.c
> > > > > > +++ b/net/mac802154/scan.c
> > > > > > @@ -419,8 +419,8 @@ int mac802154_send_beacons_locked(struct ieee802154_sub_if_data *sdata,
> > > > > >       local->beacon.mhr.fc.source_addr_mode = IEEE802154_EXTENDED_ADDRESSING;
> > > > > >       atomic_set(&request->wpan_dev->bsn, -1);
> > > > > >       local->beacon.mhr.source.mode = IEEE802154_ADDR_LONG;
> > > > > > -     local->beacon.mhr.source.pan_id = cpu_to_le16(request->wpan_dev->pan_id);
> > > > > > -     local->beacon.mhr.source.extended_addr = cpu_to_le64(request->wpan_dev->extended_addr);
> > > > > > +     local->beacon.mhr.source.pan_id = request->wpan_dev->pan_id;
> > > > > > +     local->beacon.mhr.source.extended_addr = request->wpan_dev->extended_addr;
> > > > > >       local->beacon.mac_pl.beacon_order = request->interval;
> > > > > >       local->beacon.mac_pl.superframe_order = request->interval;
> > > > > >       local->beacon.mac_pl.final_cap_slot = 0xf;
> > > > >
> > > > > This patch has been applied to the wpan-next tree and will be
> > > > > part of the next pull request to net-next. Thanks!
> > > >
> > > > fyi: in my opinion, depending on system endianness this is actually a bug.
> > >
> > > Actually there are many uses of __le16 and __le64 for PAN IDs, short
> > > and extended addresses. I did follow the existing patterns, I think
> > > they are legitimate. Can you clarify what you think is a bug in the
> > > current state? I always feel a bit flaky when it comes to properly
> > > handling endianness, so all feedback welcome, if you have any hints
> > > of what should be fixed after this patch, I'll do it.
> > >
> >
> > net/ policy is so far I understood to always use endianness how it's
> > stored on wire. There is no bug that addresses are stored as little
> > endian, but there is a bug doing a conversion when it's not necessary.
> > In this example, so far I see, big endian does a byteswap here.
> > And as you figured there will be less byteswaps when it's stored as
> > little endian. Consider this in netlink, that we store things as it is
> > on wire (or in our case on air/wireless).
>
> Ah ok I get it: "keep the endianness as on the medium". All right so

Note: this is valid even for 802.15.4 UAPI. Just want to mention it again.

- Alex