[Ksummit-discuss] [TECH TOPIC] How to encourage driver authors to annotate integer endianness properly

[Ksummit-discuss] [TECH TOPIC] How to encourage driver authors to annotate integer endianness properly

[Bart Van Assche]

Hello Ted,

As you most likely know endianness annotations like __be32 can be verified
by the static source code analyzer called sparse. These annotations are a
big help to verify whether endianness conversions in drivers are correct
(e.g. be32_to_cpu()). However, many driver authors either are not familiar
with sparse or do not use it to verify their work. I think we need a way
to encourage driver authors to pay attention to endianness annotations,
e.g. by letting the zero-day kernel test infrastructure verify endianness
annotations. Please consider to add this topic to the kernel summit agenda. 

Thanks,

Bart.

Re: [Ksummit-discuss] [TECH TOPIC] How to encourage driver authors to annotate integer endianness properly

[Greg KH]

On Wed, Oct 25, 2017 at 09:47:25AM +0000, Bart Van Assche wrote:
> Hello Ted,
> 
> As you most likely know endianness annotations like __be32 can be verified
> by the static source code analyzer called sparse. These annotations are a
> big help to verify whether endianness conversions in drivers are correct
> (e.g. be32_to_cpu()). However, many driver authors either are not familiar
> with sparse or do not use it to verify their work. I think we need a way
> to encourage driver authors to pay attention to endianness annotations,
> e.g. by letting the zero-day kernel test infrastructure verify endianness
> annotations. Please consider to add this topic to the kernel summit agenda. 

Driver subsystem maintainers should know this, and sparse reports should
be simple to run and notice these issues.  If you know of a subsystem
that is not paying attention to this, please let those maintainers know
and send patches to resolve the issues :)

thanks,

greg k-h

Re: [Ksummit-discuss] [TECH TOPIC] How to encourage driver authors to annotate integer endianness properly

[Bart Van Assche]

On Wed, 2017-10-25 at 11:54 +0200, Greg KH wrote:
> On Wed, Oct 25, 2017 at 09:47:25AM +0000, Bart Van Assche wrote:
> > Hello Ted,
> > 
> > As you most likely know endianness annotations like __be32 can be verified
> > by the static source code analyzer called sparse. These annotations are a
> > big help to verify whether endianness conversions in drivers are correct
> > (e.g. be32_to_cpu()). However, many driver authors either are not familiar
> > with sparse or do not use it to verify their work. I think we need a way
> > to encourage driver authors to pay attention to endianness annotations,
> > e.g. by letting the zero-day kernel test infrastructure verify endianness
> > annotations. Please consider to add this topic to the kernel summit agenda. 
> 
> Driver subsystem maintainers should know this, and sparse reports should
> be simple to run and notice these issues.  If you know of a subsystem
> that is not paying attention to this, please let those maintainers know
> and send patches to resolve the issues :)

Hi Greg,

A significant challenge is that for several drivers (e.g. drivers/scsi/qla2xxx
and drivers/infiniband/hw/nes) fixing the endianness annotations is so hard
that only the driver authors can make these drivers endianness clean. Three
challenges that have been encountered while trying to make drivers endianness
clean are:
- Some driver authors use a single variable to store both cpu-endian and big
  endian data.
- Endianness bugs - using a big endian integer where a cpu endian number
  should have been used or vice versa.
- The definitive resource for the endianness format is the firmware
  documentation. For many kernel drivers the firmware documentation is either
  not available or only available under NDA.

Bart.

Re: [Ksummit-discuss] [TECH TOPIC] How to encourage driver authors to annotate integer endianness properly

[greg]

On Wed, Oct 25, 2017 at 10:06:59AM +0000, Bart Van Assche wrote:
> On Wed, 2017-10-25 at 11:54 +0200, Greg KH wrote:
> > On Wed, Oct 25, 2017 at 09:47:25AM +0000, Bart Van Assche wrote:
> > > Hello Ted,
> > > 
> > > As you most likely know endianness annotations like __be32 can be verified
> > > by the static source code analyzer called sparse. These annotations are a
> > > big help to verify whether endianness conversions in drivers are correct
> > > (e.g. be32_to_cpu()). However, many driver authors either are not familiar
> > > with sparse or do not use it to verify their work. I think we need a way
> > > to encourage driver authors to pay attention to endianness annotations,
> > > e.g. by letting the zero-day kernel test infrastructure verify endianness
> > > annotations. Please consider to add this topic to the kernel summit agenda. 
> > 
> > Driver subsystem maintainers should know this, and sparse reports should
> > be simple to run and notice these issues.  If you know of a subsystem
> > that is not paying attention to this, please let those maintainers know
> > and send patches to resolve the issues :)
> 
> Hi Greg,
> 
> A significant challenge is that for several drivers (e.g. drivers/scsi/qla2xxx
> and drivers/infiniband/hw/nes) fixing the endianness annotations is so hard
> that only the driver authors can make these drivers endianness clean.

Then ask them to do so, and if they will not, then mark the drivers in
Kconfig so they do not get built for big endian platforms.

> Three challenges that have been encountered while trying to make
> drivers endianness clean are:
> - Some driver authors use a single variable to store both cpu-endian and big
>   endian data.
> - Endianness bugs - using a big endian integer where a cpu endian number
>   should have been used or vice versa.
> - The definitive resource for the endianness format is the firmware
>   documentation. For many kernel drivers the firmware documentation is either
>   not available or only available under NDA.

These problems are not "new" at all, it's been this way for 20+ years now :)

Oh, and the maintainers summit is already half over, and the schedule is
full from what I can tell...

thanks,

greg k-h

Re: [Ksummit-discuss] [TECH TOPIC] How to encourage driver authors to annotate integer endianness properly

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 237 bytes --]

On Wed, Oct 25, 2017 at 12:13:55PM +0200, greg@kroah.com wrote:

> Oh, and the maintainers summit is already half over, and the schedule is
> full from what I can tell...

There is one TBA session at 1615, just before the TAB elections.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Ksummit-discuss] [TECH TOPIC] How to encourage driver authors to annotate integer endianness properly

[Theodore Ts'o]

On Wed, Oct 25, 2017 at 12:16:30PM +0200, Mark Brown wrote:
> 
> There is one TBA session at 1615, just before the TAB elections.

If people want to talk about Device Driver Endianess Problems, the
room is yours at 16:15.  I've added it to the agenda spread sheet.

     	      	 	      - Ted

Re: [Ksummit-discuss] [TECH TOPIC] How to encourage driver authors to annotate integer endianness properly

[Leon Romanovsky]

[-- Attachment #1: Type: text/plain, Size: 2665 bytes --]

On Wed, Oct 25, 2017 at 12:13:55PM +0200, greg@kroah.com wrote:
> On Wed, Oct 25, 2017 at 10:06:59AM +0000, Bart Van Assche wrote:
> > On Wed, 2017-10-25 at 11:54 +0200, Greg KH wrote:
> > > On Wed, Oct 25, 2017 at 09:47:25AM +0000, Bart Van Assche wrote:
> > > > Hello Ted,
> > > >
> > > > As you most likely know endianness annotations like __be32 can be verified
> > > > by the static source code analyzer called sparse. These annotations are a
> > > > big help to verify whether endianness conversions in drivers are correct
> > > > (e.g. be32_to_cpu()). However, many driver authors either are not familiar
> > > > with sparse or do not use it to verify their work. I think we need a way
> > > > to encourage driver authors to pay attention to endianness annotations,
> > > > e.g. by letting the zero-day kernel test infrastructure verify endianness
> > > > annotations. Please consider to add this topic to the kernel summit agenda.
> > >
> > > Driver subsystem maintainers should know this, and sparse reports should
> > > be simple to run and notice these issues.  If you know of a subsystem
> > > that is not paying attention to this, please let those maintainers know
> > > and send patches to resolve the issues :)
> >
> > Hi Greg,
> >
> > A significant challenge is that for several drivers (e.g. drivers/scsi/qla2xxx
> > and drivers/infiniband/hw/nes) fixing the endianness annotations is so hard
> > that only the driver authors can make these drivers endianness clean.
>
> Then ask them to do so, and if they will not, then mark the drivers in
> Kconfig so they do not get built for big endian platforms.
>
> > Three challenges that have been encountered while trying to make
> > drivers endianness clean are:
> > - Some driver authors use a single variable to store both cpu-endian and big
> >   endian data.
> > - Endianness bugs - using a big endian integer where a cpu endian number
> >   should have been used or vice versa.
> > - The definitive resource for the endianness format is the firmware
> >   documentation. For many kernel drivers the firmware documentation is either
> >   not available or only available under NDA.
>
> These problems are not "new" at all, it's been this way for 20+ years now :)

The major difference between past and current is in the tools, they evolved.

>
> Oh, and the maintainers summit is already half over, and the schedule is
> full from what I can tell...

There is one open TBD slot.

>
> thanks,
>
> greg k-h
> _______________________________________________
> Ksummit-discuss mailing list
> Ksummit-discuss@lists.linuxfoundation.org
> https://lists.linuxfoundation.org/mailman/listinfo/ksummit-discuss

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [Ksummit-discuss] [TECH TOPIC] How to encourage driver authors to annotate integer endianness properly

[greg]

On Wed, Oct 25, 2017 at 01:24:23PM +0300, Leon Romanovsky wrote:
> On Wed, Oct 25, 2017 at 12:13:55PM +0200, greg@kroah.com wrote:
> > On Wed, Oct 25, 2017 at 10:06:59AM +0000, Bart Van Assche wrote:
> > > On Wed, 2017-10-25 at 11:54 +0200, Greg KH wrote:
> > > > On Wed, Oct 25, 2017 at 09:47:25AM +0000, Bart Van Assche wrote:
> > > > > Hello Ted,
> > > > >
> > > > > As you most likely know endianness annotations like __be32 can be verified
> > > > > by the static source code analyzer called sparse. These annotations are a
> > > > > big help to verify whether endianness conversions in drivers are correct
> > > > > (e.g. be32_to_cpu()). However, many driver authors either are not familiar
> > > > > with sparse or do not use it to verify their work. I think we need a way
> > > > > to encourage driver authors to pay attention to endianness annotations,
> > > > > e.g. by letting the zero-day kernel test infrastructure verify endianness
> > > > > annotations. Please consider to add this topic to the kernel summit agenda.
> > > >
> > > > Driver subsystem maintainers should know this, and sparse reports should
> > > > be simple to run and notice these issues.  If you know of a subsystem
> > > > that is not paying attention to this, please let those maintainers know
> > > > and send patches to resolve the issues :)
> > >
> > > Hi Greg,
> > >
> > > A significant challenge is that for several drivers (e.g. drivers/scsi/qla2xxx
> > > and drivers/infiniband/hw/nes) fixing the endianness annotations is so hard
> > > that only the driver authors can make these drivers endianness clean.
> >
> > Then ask them to do so, and if they will not, then mark the drivers in
> > Kconfig so they do not get built for big endian platforms.
> >
> > > Three challenges that have been encountered while trying to make
> > > drivers endianness clean are:
> > > - Some driver authors use a single variable to store both cpu-endian and big
> > >   endian data.
> > > - Endianness bugs - using a big endian integer where a cpu endian number
> > >   should have been used or vice versa.
> > > - The definitive resource for the endianness format is the firmware
> > >   documentation. For many kernel drivers the firmware documentation is either
> > >   not available or only available under NDA.
> >
> > These problems are not "new" at all, it's been this way for 20+ years now :)
> 
> The major difference between past and current is in the tools, they evolved.

endian support in sparse has been there for over a decade now, right?
This isn't a new issue, and if a driver author/team does not want to
do the work for a specific endian, then disable the drivers from that
build...

greg k-h

Re: [Ksummit-discuss] [TECH TOPIC] How to encourage driver authors to annotate integer endianness properly

[Bart Van Assche]

On Wed, 2017-10-25 at 12:38 +0200, greg@kroah.com wrote:
> endian support in sparse has been there for over a decade now, right?
> This isn't a new issue, and if a driver author/team does not want to
> do the work for a specific endian, then disable the drivers from that
> build...

Hello Greg,

Sorry if I wasn't clear enough. My concern is not about drivers that do
not work for a specific endianness but about drivers that need endianness
conversions because the endianness of the firmware registers differs from
that of the CPU. Such drivers benefit from endianness annotations even
if only one specific CPU endianness type is supported by these drivers.

Bart.

Re: [Ksummit-discuss] [TECH TOPIC] How to encourage driver authors to annotate integer endianness properly

[Leon Romanovsky]

[-- Attachment #1: Type: text/plain, Size: 2301 bytes --]

On Wed, Oct 25, 2017 at 10:06:59AM +0000, Bart Van Assche wrote:
> On Wed, 2017-10-25 at 11:54 +0200, Greg KH wrote:
> > On Wed, Oct 25, 2017 at 09:47:25AM +0000, Bart Van Assche wrote:
> > > Hello Ted,
> > >
> > > As you most likely know endianness annotations like __be32 can be verified
> > > by the static source code analyzer called sparse. These annotations are a
> > > big help to verify whether endianness conversions in drivers are correct
> > > (e.g. be32_to_cpu()). However, many driver authors either are not familiar
> > > with sparse or do not use it to verify their work. I think we need a way
> > > to encourage driver authors to pay attention to endianness annotations,
> > > e.g. by letting the zero-day kernel test infrastructure verify endianness
> > > annotations. Please consider to add this topic to the kernel summit agenda.
> >
> > Driver subsystem maintainers should know this, and sparse reports should
> > be simple to run and notice these issues.  If you know of a subsystem
> > that is not paying attention to this, please let those maintainers know
> > and send patches to resolve the issues :)
>
> Hi Greg,
>
> A significant challenge is that for several drivers (e.g. drivers/scsi/qla2xxx
> and drivers/infiniband/hw/nes) fixing the endianness annotations is so hard
> that only the driver authors can make these drivers endianness clean. Three
> challenges that have been encountered while trying to make drivers endianness
> clean are:
> - Some driver authors use a single variable to store both cpu-endian and big
>   endian data.
> - Endianness bugs - using a big endian integer where a cpu endian number
>   should have been used or vice versa.
> - The definitive resource for the endianness format is the firmware
>   documentation. For many kernel drivers the firmware documentation is either
>   not available or only available under NDA.

It is e very appealing to fix the driver endianness issues without involving
driver authors, but it is not feasible without their cooperation and it is not
always possible especially for not-active drivers.

Thanks

>
> Bart.
> _______________________________________________
> Ksummit-discuss mailing list
> Ksummit-discuss@lists.linuxfoundation.org
> https://lists.linuxfoundation.org/mailman/listinfo/ksummit-discuss

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [Ksummit-discuss] [TECH TOPIC] How to encourage driver authors to annotate integer endianness properly

[Alexey Dobriyan]

On 10/25/17, Bart Van Assche <Bart.VanAssche@wdc.com> wrote:
> I think we need a way to encourage driver authors
> to pay attention to endianness annotations,

typedef struct {
        uint32_t _;
} __le32;

The problems of sparse are:
a) it is not mandatory part of kernel compilation, and
b) it doesn't break compilation.

Re: [Ksummit-discuss] [TECH TOPIC] How to encourage driver authors to annotate integer endianness properly

[David Woodhouse]

[-- Attachment #1: Type: text/plain, Size: 844 bytes --]

On Wed, 2017-10-25 at 14:40 +0200, Alexey Dobriyan wrote:
> On 10/25/17, Bart Van Assche <Bart.VanAssche@wdc.com> wrote:
> > 
> > I think we need a way to encourage driver authors
> > to pay attention to endianness annotations,
> typedef struct {
>         uint32_t _;
> } __le32;

I actually did that in JFFS2 years ago; before sparse could do it. It
was very useful.

I'd really like to have proper compiler support for big-endian and
little-endian integers too. We're half-way there now with
__builtin_bswap32() et al, and compilers can optimise things properly
(instead of the opaque inline asm we had before I cleaned that up).

But being able to just mark a given integer storage as big- or little-
endian and have all loads and stores *automatically* do the right thing
would be extremely useful in many cases.

[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 4938 bytes --]

Re: [Ksummit-discuss] [TECH TOPIC] How to encourage driver authors to annotate integer endianness properly

[Josh Triplett]

On Wed, Oct 25, 2017 at 03:11:48PM +0200, David Woodhouse wrote:
> On Wed, 2017-10-25 at 14:40 +0200, Alexey Dobriyan wrote:
> > On 10/25/17, Bart Van Assche <Bart.VanAssche@wdc.com> wrote:
> > > 
> > > I think we need a way to encourage driver authors
> > > to pay attention to endianness annotations,
> > typedef struct {
> >         uint32_t _;
> > } __le32;
> 
> I actually did that in JFFS2 years ago; before sparse could do it. It
> was very useful.
> 
> I'd really like to have proper compiler support for big-endian and
> little-endian integers too. We're half-way there now with
> __builtin_bswap32() et al, and compilers can optimise things properly
> (instead of the opaque inline asm we had before I cleaned that up).
> 
> But being able to just mark a given integer storage as big- or little-
> endian and have all loads and stores *automatically* do the right thing
> would be extremely useful in many cases.

See https://gcc.gnu.org/bugzilla/show_bug.cgi?id=59852 for a proposal to
implement Sparse's __attribute__((bitwise)) in GCC. The GCC developers
are amenable, if someone's willing to write a patch.

Re: [Ksummit-discuss] [TECH TOPIC] How to encourage driver authors to annotate integer endianness properly

[Bart Van Assche]

On Wed, 2017-10-25 at 09:16 -0700, Josh Triplett wrote:
> See https://gcc.gnu.org/bugzilla/show_bug.cgi?id=59852 for a proposal to
> implement Sparse's __attribute__((bitwise)) in GCC. The GCC developers
> are amenable, if someone's willing to write a patch.

Thanks Josh. I will have a look at that bugzilla ticket.

Bart.