From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756519AbeASVZT (ORCPT ); Fri, 19 Jan 2018 16:25:19 -0500 Received: from mail-lf0-f67.google.com ([209.85.215.67]:37285 "EHLO mail-lf0-f67.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754118AbeASVZK (ORCPT ); Fri, 19 Jan 2018 16:25:10 -0500 X-Google-Smtp-Source: ACJfBougLxdQ3pJP/nVxKoS2/me/CO8/euEzxudIQbLArxw6Zu3gRHvG88vFxdWTmZg9/YKwiJCQAg== Date: Sat, 20 Jan 2018 00:25:16 +0300 From: Serge Semin To: Arnd Bergmann Cc: jdmason@kudzu.us, Dave Jiang , allenbh@gmail.com, "Hook, Gary" , Sergey.Semin@t-platforms.ru, linux-ntb@googlegroups.com, Linux Kernel Mailing List Subject: Re: [PATCH] NTB: ntb_perf: fix cast to restricted __le32 Message-ID: <20180119212516.GA2386@mobilestation> References: <20180119173044.8013-1-fancer.lancer@gmail.com> <20180119210310.GA2284@mobilestation> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20180119210310.GA2284@mobilestation> User-Agent: Mutt/1.5.24 (2015-08-30) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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