Re: [PATCH] NTB: ntb_perf: fix cast to restricted __le32

[Serge Semin]

On Tue, Jan 23, 2018 at 11:18:06PM -0500, Jon Mason  wrote:
> On Fri, Jan 19, 2018 at 10:26:37PM +0100, Arnd Bergmann wrote:
> > On Fri, Jan 19, 2018 at 10:03 PM, Serge Semin  
> > wrote:
> > >
> > > Actually the provided patch is the best solution I could come up with.
> > > The thing is, that the methods can't be changed. Those functions are
> > > the part of the NTB API methods used by many drivers. So basically they
> > > are like pci_{read,write}_config_{byte,word,dword}() methods. We can't
> > > change their prototypes only because it's suit some driver. The methods
> > > give an access to the NTB device dummy u32-sized registers, nothing
> > > else. So endianness is the transmitted data settings in this case.
> > >
> > > NTB is the technology to interconnect some two systems with possibly
> > > different endianness (unlike PCI, which interconnect CPU with LE devices).
> > > In this case I'd need to set some agreement up between two systems about
> > > the endianness of the exchanged data like host and network types in
> > > Linux networking. I've chosen the network data to be little-endian,
> > > that's why I needed first to convert them from CPU to le32, then on
> > > remote side convert them back from le32 to CPU.
> > >
> > > If you have any better suggestion how the warning can be fixed, I'd
> > > be glad to stick to it.
> > 
> > I don't think your description matches what you actually do: The
> > underlying ntb hardware drivers (amd, idt, intel, mscc) all treat the
> > incoming data as CPU-endian and convert it to little-endian on
> > the register side, so the framework already assumes that whatever
> > you do here uses a little-endian wire-level protocol.
> > 
> > On a little-endian kernel/CPU, nothing is ever swapped here, neither
> > in the ntb_perf front-end nor in the back-ends. On a big-endian
> > kernel/CPU, they both swap, so you end up with CPU-endian
> > data on the wire, so it should be impossible for a big-endian
> > system to talk to a little-endian one. Have you actually tried that
> > combination with the current code?
> 
> I do not believe anyone has every tried NTB on a big endien system,
> let alone tried it with one side LE and the other BE.  To my
> knowledge, this has only ever been used on x86 to x86.
> 
> > If my interpretation is correct, then the best solution would be to
> > completely remove the cpu_to_le32/le32_to_cpu conversions
> > from ntb_perf, and just define that it works like any other PCI
> > device, exchanging little-endian data.
> 
> Yes, this would be the best solution.  Thank you for the insight.
> 
> Serge, when you get a chance, please make this change and resumbit.
> 

Ok. I'll do it in an hour.

Regards,
-Sergey

> Thanks,
> Jon
> 
> 
> > 
> > There are two interesting cases to consider though:
> > 
> > - if someone wants to implement an NTB based protocol
> >   using big-endian data on the wire, you probably want to add
> >   a ntb_peer_spad_read_be()/ntb_peer_msg_write_be()
> >   set of interfaces, to go along with ioread32_be()/iowrite32_be()
> >   the same way that ntb_peer_spad_read()/ntb_peer_msg_write()
> >   ends up doing ioread32()/iowrite32() with the implied little-endian
> >   behavior.
> > 
> > - memcpy_toio()/memcpy_fromio() and ioread32_rep()/iowrite32_rep
> >   importantly do not do any byteswap, they are meant to
> >   transfer byte streams.
> > 
> > Arnd

Re: [PATCH] NTB: ntb_perf: fix cast to restricted __le32

[Serge Semin]

On Tue, Jan 23, 2018 at 11:18:06PM -0500, Jon Mason  wrote:
> On Fri, Jan 19, 2018 at 10:26:37PM +0100, Arnd Bergmann wrote:
> > On Fri, Jan 19, 2018 at 10:03 PM, Serge Semin  
> > wrote:
> > >
> > > Actually the provided patch is the best solution I could come up with.
> > > The thing is, that the methods can't be changed. Those functions are
> > > the part of the NTB API methods used by many drivers. So basically they
> > > are like pci_{read,write}_config_{byte,word,dword}() methods. We can't
> > > change their prototypes only because it's suit some driver. The methods
> > > give an access to the NTB device dummy u32-sized registers, nothing
> > > else. So endianness is the transmitted data settings in this case.
> > >
> > > NTB is the technology to interconnect some two systems with possibly
> > > different endianness (unlike PCI, which interconnect CPU with LE devices).
> > > In this case I'd need to set some agreement up between two systems about
> > > the endianness of the exchanged data like host and network types in
> > > Linux networking. I've chosen the network data to be little-endian,
> > > that's why I needed first to convert them from CPU to le32, then on
> > > remote side convert them back from le32 to CPU.
> > >
> > > If you have any better suggestion how the warning can be fixed, I'd
> > > be glad to stick to it.
> > 
> > I don't think your description matches what you actually do: The
> > underlying ntb hardware drivers (amd, idt, intel, mscc) all treat the
> > incoming data as CPU-endian and convert it to little-endian on
> > the register side, so the framework already assumes that whatever
> > you do here uses a little-endian wire-level protocol.
> > 
> > On a little-endian kernel/CPU, nothing is ever swapped here, neither
> > in the ntb_perf front-end nor in the back-ends. On a big-endian
> > kernel/CPU, they both swap, so you end up with CPU-endian
> > data on the wire, so it should be impossible for a big-endian
> > system to talk to a little-endian one. Have you actually tried that
> > combination with the current code?
> 
> I do not believe anyone has every tried NTB on a big endien system,
> let alone tried it with one side LE and the other BE.  To my
> knowledge, this has only ever been used on x86 to x86.
> 
> > If my interpretation is correct, then the best solution would be to
> > completely remove the cpu_to_le32/le32_to_cpu conversions
> > from ntb_perf, and just define that it works like any other PCI
> > device, exchanging little-endian data.
> 
> Yes, this would be the best solution.  Thank you for the insight.
> 
> Serge, when you get a chance, please make this change and resumbit.
> 

Ok. I'll do it in an hour.

Regards,
-Sergey

> Thanks,
> Jon
> 
> 
> > 
> > There are two interesting cases to consider though:
> > 
> > - if someone wants to implement an NTB based protocol
> >   using big-endian data on the wire, you probably want to add
> >   a ntb_peer_spad_read_be()/ntb_peer_msg_write_be()
> >   set of interfaces, to go along with ioread32_be()/iowrite32_be()
> >   the same way that ntb_peer_spad_read()/ntb_peer_msg_write()
> >   ends up doing ioread32()/iowrite32() with the implied little-endian
> >   behavior.
> > 
> > - memcpy_toio()/memcpy_fromio() and ioread32_rep()/iowrite32_rep
> >   importantly do not do any byteswap, they are meant to
> >   transfer byte streams.
> > 
> > Arnd

Re: [PATCH] NTB: ntb_perf: fix cast to restricted __le32

[Jon Mason]

On Fri, Jan 19, 2018 at 10:26:37PM +0100, Arnd Bergmann wrote:
> On Fri, Jan 19, 2018 at 10:03 PM, Serge Semin  wrote:
> >
> > Actually the provided patch is the best solution I could come up with.
> > The thing is, that the methods can't be changed. Those functions are
> > the part of the NTB API methods used by many drivers. So basically they
> > are like pci_{read,write}_config_{byte,word,dword}() methods. We can't
> > change their prototypes only because it's suit some driver. The methods
> > give an access to the NTB device dummy u32-sized registers, nothing
> > else. So endianness is the transmitted data settings in this case.
> >
> > NTB is the technology to interconnect some two systems with possibly
> > different endianness (unlike PCI, which interconnect CPU with LE devices).
> > In this case I'd need to set some agreement up between two systems about
> > the endianness of the exchanged data like host and network types in
> > Linux networking. I've chosen the network data to be little-endian,
> > that's why I needed first to convert them from CPU to le32, then on
> > remote side convert them back from le32 to CPU.
> >
> > If you have any better suggestion how the warning can be fixed, I'd
> > be glad to stick to it.
> 
> I don't think your description matches what you actually do: The
> underlying ntb hardware drivers (amd, idt, intel, mscc) all treat the
> incoming data as CPU-endian and convert it to little-endian on
> the register side, so the framework already assumes that whatever
> you do here uses a little-endian wire-level protocol.
> 
> On a little-endian kernel/CPU, nothing is ever swapped here, neither
> in the ntb_perf front-end nor in the back-ends. On a big-endian
> kernel/CPU, they both swap, so you end up with CPU-endian
> data on the wire, so it should be impossible for a big-endian
> system to talk to a little-endian one. Have you actually tried that
> combination with the current code?

I do not believe anyone has every tried NTB on a big endien system,
let alone tried it with one side LE and the other BE.  To my
knowledge, this has only ever been used on x86 to x86.

> If my interpretation is correct, then the best solution would be to
> completely remove the cpu_to_le32/le32_to_cpu conversions
> from ntb_perf, and just define that it works like any other PCI
> device, exchanging little-endian data.

Yes, this would be the best solution.  Thank you for the insight.

Serge, when you get a chance, please make this change and resumbit.

Thanks,
Jon


> 
> There are two interesting cases to consider though:
> 
> - if someone wants to implement an NTB based protocol
>   using big-endian data on the wire, you probably want to add
>   a ntb_peer_spad_read_be()/ntb_peer_msg_write_be()
>   set of interfaces, to go along with ioread32_be()/iowrite32_be()
>   the same way that ntb_peer_spad_read()/ntb_peer_msg_write()
>   ends up doing ioread32()/iowrite32() with the implied little-endian
>   behavior.
> 
> - memcpy_toio()/memcpy_fromio() and ioread32_rep()/iowrite32_rep
>   importantly do not do any byteswap, they are meant to
>   transfer byte streams.
> 
> Arnd

Re: [PATCH] NTB: ntb_perf: fix cast to restricted __le32

[Jon Mason]

On Fri, Jan 19, 2018 at 10:26:37PM +0100, Arnd Bergmann wrote:
> On Fri, Jan 19, 2018 at 10:03 PM, Serge Semin  wrote:
> >
> > Actually the provided patch is the best solution I could come up with.
> > The thing is, that the methods can't be changed. Those functions are
> > the part of the NTB API methods used by many drivers. So basically they
> > are like pci_{read,write}_config_{byte,word,dword}() methods. We can't
> > change their prototypes only because it's suit some driver. The methods
> > give an access to the NTB device dummy u32-sized registers, nothing
> > else. So endianness is the transmitted data settings in this case.
> >
> > NTB is the technology to interconnect some two systems with possibly
> > different endianness (unlike PCI, which interconnect CPU with LE devices).
> > In this case I'd need to set some agreement up between two systems about
> > the endianness of the exchanged data like host and network types in
> > Linux networking. I've chosen the network data to be little-endian,
> > that's why I needed first to convert them from CPU to le32, then on
> > remote side convert them back from le32 to CPU.
> >
> > If you have any better suggestion how the warning can be fixed, I'd
> > be glad to stick to it.
> 
> I don't think your description matches what you actually do: The
> underlying ntb hardware drivers (amd, idt, intel, mscc) all treat the
> incoming data as CPU-endian and convert it to little-endian on
> the register side, so the framework already assumes that whatever
> you do here uses a little-endian wire-level protocol.
> 
> On a little-endian kernel/CPU, nothing is ever swapped here, neither
> in the ntb_perf front-end nor in the back-ends. On a big-endian
> kernel/CPU, they both swap, so you end up with CPU-endian
> data on the wire, so it should be impossible for a big-endian
> system to talk to a little-endian one. Have you actually tried that
> combination with the current code?

I do not believe anyone has every tried NTB on a big endien system,
let alone tried it with one side LE and the other BE.  To my
knowledge, this has only ever been used on x86 to x86.

> If my interpretation is correct, then the best solution would be to
> completely remove the cpu_to_le32/le32_to_cpu conversions
> from ntb_perf, and just define that it works like any other PCI
> device, exchanging little-endian data.

Yes, this would be the best solution.  Thank you for the insight.

Serge, when you get a chance, please make this change and resumbit.

Thanks,
Jon


> 
> There are two interesting cases to consider though:
> 
> - if someone wants to implement an NTB based protocol
>   using big-endian data on the wire, you probably want to add
>   a ntb_peer_spad_read_be()/ntb_peer_msg_write_be()
>   set of interfaces, to go along with ioread32_be()/iowrite32_be()
>   the same way that ntb_peer_spad_read()/ntb_peer_msg_write()
>   ends up doing ioread32()/iowrite32() with the implied little-endian
>   behavior.
> 
> - memcpy_toio()/memcpy_fromio() and ioread32_rep()/iowrite32_rep
>   importantly do not do any byteswap, they are meant to
>   transfer byte streams.
> 
> Arnd

Re: [PATCH] NTB: ntb_perf: fix cast to restricted __le32

[Arnd Bergmann]

On Fri, Jan 19, 2018 at 10:03 PM, Serge Semin  wrote:
>
> Actually the provided patch is the best solution I could come up with.
> The thing is, that the methods can't be changed. Those functions are
> the part of the NTB API methods used by many drivers. So basically they
> are like pci_{read,write}_config_{byte,word,dword}() methods. We can't
> change their prototypes only because it's suit some driver. The methods
> give an access to the NTB device dummy u32-sized registers, nothing
> else. So endianness is the transmitted data settings in this case.
>
> NTB is the technology to interconnect some two systems with possibly
> different endianness (unlike PCI, which interconnect CPU with LE devices).
> In this case I'd need to set some agreement up between two systems about
> the endianness of the exchanged data like host and network types in
> Linux networking. I've chosen the network data to be little-endian,
> that's why I needed first to convert them from CPU to le32, then on
> remote side convert them back from le32 to CPU.
>
> If you have any better suggestion how the warning can be fixed, I'd
> be glad to stick to it.

I don't think your description matches what you actually do: The
underlying ntb hardware drivers (amd, idt, intel, mscc) all treat the
incoming data as CPU-endian and convert it to little-endian on
the register side, so the framework already assumes that whatever
you do here uses a little-endian wire-level protocol.

On a little-endian kernel/CPU, nothing is ever swapped here, neither
in the ntb_perf front-end nor in the back-ends. On a big-endian
kernel/CPU, they both swap, so you end up with CPU-endian
data on the wire, so it should be impossible for a big-endian
system to talk to a little-endian one. Have you actually tried that
combination with the current code?

If my interpretation is correct, then the best solution would be to
completely remove the cpu_to_le32/le32_to_cpu conversions
from ntb_perf, and just define that it works like any other PCI
device, exchanging little-endian data.

There are two interesting cases to consider though:

- if someone wants to implement an NTB based protocol
  using big-endian data on the wire, you probably want to add
  a ntb_peer_spad_read_be()/ntb_peer_msg_write_be()
  set of interfaces, to go along with ioread32_be()/iowrite32_be()
  the same way that ntb_peer_spad_read()/ntb_peer_msg_write()
  ends up doing ioread32()/iowrite32() with the implied little-endian
  behavior.

- memcpy_toio()/memcpy_fromio() and ioread32_rep()/iowrite32_rep
  importantly do not do any byteswap, they are meant to
  transfer byte streams.

Arnd

Re: [PATCH] NTB: ntb_perf: fix cast to restricted __le32

[Arnd Bergmann]

On Fri, Jan 19, 2018 at 10:03 PM, Serge Semin  wrote:
>
> Actually the provided patch is the best solution I could come up with.
> The thing is, that the methods can't be changed. Those functions are
> the part of the NTB API methods used by many drivers. So basically they
> are like pci_{read,write}_config_{byte,word,dword}() methods. We can't
> change their prototypes only because it's suit some driver. The methods
> give an access to the NTB device dummy u32-sized registers, nothing
> else. So endianness is the transmitted data settings in this case.
>
> NTB is the technology to interconnect some two systems with possibly
> different endianness (unlike PCI, which interconnect CPU with LE devices).
> In this case I'd need to set some agreement up between two systems about
> the endianness of the exchanged data like host and network types in
> Linux networking. I've chosen the network data to be little-endian,
> that's why I needed first to convert them from CPU to le32, then on
> remote side convert them back from le32 to CPU.
>
> If you have any better suggestion how the warning can be fixed, I'd
> be glad to stick to it.

I don't think your description matches what you actually do: The
underlying ntb hardware drivers (amd, idt, intel, mscc) all treat the
incoming data as CPU-endian and convert it to little-endian on
the register side, so the framework already assumes that whatever
you do here uses a little-endian wire-level protocol.

On a little-endian kernel/CPU, nothing is ever swapped here, neither
in the ntb_perf front-end nor in the back-ends. On a big-endian
kernel/CPU, they both swap, so you end up with CPU-endian
data on the wire, so it should be impossible for a big-endian
system to talk to a little-endian one. Have you actually tried that
combination with the current code?

If my interpretation is correct, then the best solution would be to
completely remove the cpu_to_le32/le32_to_cpu conversions
from ntb_perf, and just define that it works like any other PCI
device, exchanging little-endian data.

There are two interesting cases to consider though:

- if someone wants to implement an NTB based protocol
  using big-endian data on the wire, you probably want to add
  a ntb_peer_spad_read_be()/ntb_peer_msg_write_be()
  set of interfaces, to go along with ioread32_be()/iowrite32_be()
  the same way that ntb_peer_spad_read()/ntb_peer_msg_write()
  ends up doing ioread32()/iowrite32() with the implied little-endian
  behavior.

- memcpy_toio()/memcpy_fromio() and ioread32_rep()/iowrite32_rep
  importantly do not do any byteswap, they are meant to
  transfer byte streams.

Arnd

Re: [PATCH] NTB: ntb_perf: fix cast to restricted __le32

[Serge Semin]

On Sat, Jan 20, 2018 at 12:03:10AM +0300, Serge Semin  
wrote:
> On Fri, Jan 19, 2018 at 09:42:17PM +0100, Arnd Bergmann  wrote:
> > On Fri, Jan 19, 2018 at 6:30 PM, Serge Semin  
> > wrote:
> > > Sparse is whining about the u32 and __le32 mixed usage in the
> > > driver.
> > >
> > > drivers/ntb/test/ntb_perf.c:288:21: warning: cast to restricted __le32
> > > drivers/ntb/test/ntb_perf.c:295:37: warning: incorrect type in argument 4 
> > > (different base types)
> > > drivers/ntb/test/ntb_perf.c:295:37:expected unsigned int [unsigned] 
> > > [usertype] val
> > > drivers/ntb/test/ntb_perf.c:295:37:got restricted __le32 [usertype] 
> > > 
> > > ...
> > >
> > > The NTB API can't be changed so ntb_spad_*() methods
> > > would return either pure __le32 or __be32, since the scratchpad
> > > data can have arbitrary endianness in general. In this case we
> > > need to forcibly cast all the u32 to be __le32 and vise-versa
> > > where it's supposed to be in accordance with the driver logic.
> > >
> > 
> > There's got to be a better way to do this than sprinkling lots of __force
> > typecasts throughout the code.
> > 
> > It looks like all those casts are about
> > ntb_peer_spad_read()/ntb_peer_msg_write() calls, so why not change
> > those function prototypes to work on __le32 types?
> > 
> > There should also be some form of documentation regarding why you
> > need to swap the data twice, since all the ntb drivers later end up
> > doing another cpu_to_le32() on the little-endian data.
> > 
> >Arnd
> 
> Actually the provided patch is the best solution I could come up with.
> The thing is, that the methods can't be changed. Those functions are
> the part of the NTB API methods used by many drivers. So basically they
> are like pci_{read,write}_config_{byte,word,dword}() methods. We can't
> change their prototypes only because it's suit some driver. The methods
> give an access to the NTB device dummy u32-sized registers, nothing
> else. So endianness is the transmitted data settings in this case.
> 
> NTB is the technology to interconnect some two systems with possibly
> different endianness (unlike PCI, which interconnect CPU with LE devices).
> In this case I'd need to set some agreement up between two systems about
> the endianness of the exchanged data like host and network types in
> Linux networking. I've chosen the network data to be little-endian,
> that's why I needed first to convert them from CPU to le32, then on
> remote side convert them back from le32 to CPU.
> 
> If you have any better suggestion how the warning can be fixed, I'd
> be glad to stick to it.
> 
> -Sergey
> 

I meant, everything depends on the NTB hardware driver hidden behind the
API. If it does back and forth conversions writing/reading data to/from
scratchpad registers (using IO methods like write32/read32), then
I don't need to worry about data endianness at all and should have
discarded le32_to_cpu()/cpu_to_l32() usage. But if it doesn't do it,
then ntb_perf driver will be in trouble. So I sticked with the safest
solution. Although the final decision is after the subsystem maintainer.

-Sergey

Re: [PATCH] NTB: ntb_perf: fix cast to restricted __le32

[Serge Semin]

On Sat, Jan 20, 2018 at 12:03:10AM +0300, Serge Semin  
wrote:
> On Fri, Jan 19, 2018 at 09:42:17PM +0100, Arnd Bergmann  wrote:
> > On Fri, Jan 19, 2018 at 6:30 PM, Serge Semin  
> > wrote:
> > > Sparse is whining about the u32 and __le32 mixed usage in the
> > > driver.
> > >
> > > drivers/ntb/test/ntb_perf.c:288:21: warning: cast to restricted __le32
> > > drivers/ntb/test/ntb_perf.c:295:37: warning: incorrect type in argument 4 
> > > (different base types)
> > > drivers/ntb/test/ntb_perf.c:295:37:expected unsigned int [unsigned] 
> > > [usertype] val
> > > drivers/ntb/test/ntb_perf.c:295:37:got restricted __le32 [usertype] 
> > > 
> > > ...
> > >
> > > The NTB API can't be changed so ntb_spad_*() methods
> > > would return either pure __le32 or __be32, since the scratchpad
> > > data can have arbitrary endianness in general. In this case we
> > > need to forcibly cast all the u32 to be __le32 and vise-versa
> > > where it's supposed to be in accordance with the driver logic.
> > >
> > 
> > There's got to be a better way to do this than sprinkling lots of __force
> > typecasts throughout the code.
> > 
> > It looks like all those casts are about
> > ntb_peer_spad_read()/ntb_peer_msg_write() calls, so why not change
> > those function prototypes to work on __le32 types?
> > 
> > There should also be some form of documentation regarding why you
> > need to swap the data twice, since all the ntb drivers later end up
> > doing another cpu_to_le32() on the little-endian data.
> > 
> >Arnd
> 
> Actually the provided patch is the best solution I could come up with.
> The thing is, that the methods can't be changed. Those functions are
> the part of the NTB API methods used by many drivers. So basically they
> are like pci_{read,write}_config_{byte,word,dword}() methods. We can't
> change their prototypes only because it's suit some driver. The methods
> give an access to the NTB device dummy u32-sized registers, nothing
> else. So endianness is the transmitted data settings in this case.
> 
> NTB is the technology to interconnect some two systems with possibly
> different endianness (unlike PCI, which interconnect CPU with LE devices).
> In this case I'd need to set some agreement up between two systems about
> the endianness of the exchanged data like host and network types in
> Linux networking. I've chosen the network data to be little-endian,
> that's why I needed first to convert them from CPU to le32, then on
> remote side convert them back from le32 to CPU.
> 
> If you have any better suggestion how the warning can be fixed, I'd
> be glad to stick to it.
> 
> -Sergey
> 

I meant, everything depends on the NTB hardware driver hidden behind the
API. If it does back and forth conversions writing/reading data to/from
scratchpad registers (using IO methods like write32/read32), then
I don't need to worry about data endianness at all and should have
discarded le32_to_cpu()/cpu_to_l32() usage. But if it doesn't do it,
then ntb_perf driver will be in trouble. So I sticked with the safest
solution. Although the final decision is after the subsystem maintainer.

-Sergey

Re: [PATCH] NTB: ntb_perf: fix cast to restricted __le32

[Serge Semin]

On Fri, Jan 19, 2018 at 09:42:17PM +0100, Arnd Bergmann  wrote:
> On Fri, Jan 19, 2018 at 6:30 PM, Serge Semin  wrote:
> > Sparse is whining about the u32 and __le32 mixed usage in the
> > driver.
> >
> > drivers/ntb/test/ntb_perf.c:288:21: warning: cast to restricted __le32
> > drivers/ntb/test/ntb_perf.c:295:37: warning: incorrect type in argument 4 
> > (different base types)
> > drivers/ntb/test/ntb_perf.c:295:37:expected unsigned int [unsigned] 
> > [usertype] val
> > drivers/ntb/test/ntb_perf.c:295:37:got restricted __le32 [usertype] 
> > 
> > ...
> >
> > The NTB API can't be changed so ntb_spad_*() methods
> > would return either pure __le32 or __be32, since the scratchpad
> > data can have arbitrary endianness in general. In this case we
> > need to forcibly cast all the u32 to be __le32 and vise-versa
> > where it's supposed to be in accordance with the driver logic.
> >
> 
> There's got to be a better way to do this than sprinkling lots of __force
> typecasts throughout the code.
> 
> It looks like all those casts are about
> ntb_peer_spad_read()/ntb_peer_msg_write() calls, so why not change
> those function prototypes to work on __le32 types?
> 
> There should also be some form of documentation regarding why you
> need to swap the data twice, since all the ntb drivers later end up
> doing another cpu_to_le32() on the little-endian data.
> 
>Arnd

Actually the provided patch is the best solution I could come up with.
The thing is, that the methods can't be changed. Those functions are
the part of the NTB API methods used by many drivers. So basically they
are like pci_{read,write}_config_{byte,word,dword}() methods. We can't
change their prototypes only because it's suit some driver. The methods
give an access to the NTB device dummy u32-sized registers, nothing
else. So endianness is the transmitted data settings in this case.

NTB is the technology to interconnect some two systems with possibly
different endianness (unlike PCI, which interconnect CPU with LE devices).
In this case I'd need to set some agreement up between two systems about
the endianness of the exchanged data like host and network types in
Linux networking. I've chosen the network data to be little-endian,
that's why I needed first to convert them from CPU to le32, then on
remote side convert them back from le32 to CPU.

If you have any better suggestion how the warning can be fixed, I'd
be glad to stick to it.

-Sergey

Re: [PATCH] NTB: ntb_perf: fix cast to restricted __le32

[Serge Semin]

On Fri, Jan 19, 2018 at 09:42:17PM +0100, Arnd Bergmann  wrote:
> On Fri, Jan 19, 2018 at 6:30 PM, Serge Semin  wrote:
> > Sparse is whining about the u32 and __le32 mixed usage in the
> > driver.
> >
> > drivers/ntb/test/ntb_perf.c:288:21: warning: cast to restricted __le32
> > drivers/ntb/test/ntb_perf.c:295:37: warning: incorrect type in argument 4 
> > (different base types)
> > drivers/ntb/test/ntb_perf.c:295:37:expected unsigned int [unsigned] 
> > [usertype] val
> > drivers/ntb/test/ntb_perf.c:295:37:got restricted __le32 [usertype] 
> > 
> > ...
> >
> > The NTB API can't be changed so ntb_spad_*() methods
> > would return either pure __le32 or __be32, since the scratchpad
> > data can have arbitrary endianness in general. In this case we
> > need to forcibly cast all the u32 to be __le32 and vise-versa
> > where it's supposed to be in accordance with the driver logic.
> >
> 
> There's got to be a better way to do this than sprinkling lots of __force
> typecasts throughout the code.
> 
> It looks like all those casts are about
> ntb_peer_spad_read()/ntb_peer_msg_write() calls, so why not change
> those function prototypes to work on __le32 types?
> 
> There should also be some form of documentation regarding why you
> need to swap the data twice, since all the ntb drivers later end up
> doing another cpu_to_le32() on the little-endian data.
> 
>Arnd

Actually the provided patch is the best solution I could come up with.
The thing is, that the methods can't be changed. Those functions are
the part of the NTB API methods used by many drivers. So basically they
are like pci_{read,write}_config_{byte,word,dword}() methods. We can't
change their prototypes only because it's suit some driver. The methods
give an access to the NTB device dummy u32-sized registers, nothing
else. So endianness is the transmitted data settings in this case.

NTB is the technology to interconnect some two systems with possibly
different endianness (unlike PCI, which interconnect CPU with LE devices).
In this case I'd need to set some agreement up between two systems about
the endianness of the exchanged data like host and network types in
Linux networking. I've chosen the network data to be little-endian,
that's why I needed first to convert them from CPU to le32, then on
remote side convert them back from le32 to CPU.

If you have any better suggestion how the warning can be fixed, I'd
be glad to stick to it.

-Sergey

Re: [PATCH] NTB: ntb_perf: fix cast to restricted __le32

[Arnd Bergmann]

On Fri, Jan 19, 2018 at 6:30 PM, Serge Semin  wrote:
> Sparse is whining about the u32 and __le32 mixed usage in the
> driver.
>
> drivers/ntb/test/ntb_perf.c:288:21: warning: cast to restricted __le32
> drivers/ntb/test/ntb_perf.c:295:37: warning: incorrect type in argument 4 
> (different base types)
> drivers/ntb/test/ntb_perf.c:295:37:expected unsigned int [unsigned] 
> [usertype] val
> drivers/ntb/test/ntb_perf.c:295:37:got restricted __le32 [usertype] 
> 
> ...
>
> The NTB API can't be changed so ntb_spad_*() methods
> would return either pure __le32 or __be32, since the scratchpad
> data can have arbitrary endianness in general. In this case we
> need to forcibly cast all the u32 to be __le32 and vise-versa
> where it's supposed to be in accordance with the driver logic.
>

There's got to be a better way to do this than sprinkling lots of __force
typecasts throughout the code.

It looks like all those casts are about
ntb_peer_spad_read()/ntb_peer_msg_write() calls, so why not change
those function prototypes to work on __le32 types?

There should also be some form of documentation regarding why you
need to swap the data twice, since all the ntb drivers later end up
doing another cpu_to_le32() on the little-endian data.

   Arnd

Re: [PATCH] NTB: ntb_perf: fix cast to restricted __le32

[Arnd Bergmann]

On Fri, Jan 19, 2018 at 6:30 PM, Serge Semin  wrote:
> Sparse is whining about the u32 and __le32 mixed usage in the
> driver.
>
> drivers/ntb/test/ntb_perf.c:288:21: warning: cast to restricted __le32
> drivers/ntb/test/ntb_perf.c:295:37: warning: incorrect type in argument 4 
> (different base types)
> drivers/ntb/test/ntb_perf.c:295:37:expected unsigned int [unsigned] 
> [usertype] val
> drivers/ntb/test/ntb_perf.c:295:37:got restricted __le32 [usertype] 
> 
> ...
>
> The NTB API can't be changed so ntb_spad_*() methods
> would return either pure __le32 or __be32, since the scratchpad
> data can have arbitrary endianness in general. In this case we
> need to forcibly cast all the u32 to be __le32 and vise-versa
> where it's supposed to be in accordance with the driver logic.
>

There's got to be a better way to do this than sprinkling lots of __force
typecasts throughout the code.

It looks like all those casts are about
ntb_peer_spad_read()/ntb_peer_msg_write() calls, so why not change
those function prototypes to work on __le32 types?

There should also be some form of documentation regarding why you
need to swap the data twice, since all the ntb drivers later end up
doing another cpu_to_le32() on the little-endian data.

   Arnd