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

[Serge Semin]

On Sat, Jan 20, 2018 at 12:03:10AM +0300, Serge Semin <fancer.lancer@xxxxxxxxx> wrote:
> On Fri, Jan 19, 2018 at 09:42:17PM +0100, Arnd Bergmann <arnd@xxxxxxxx> wrote:
> > On Fri, Jan 19, 2018 at 6:30 PM, Serge Semin <fancer.lancer@xxxxxxxxx> 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] <noident>
> > > ...
> > >
> > > 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