On Wed, Jan 04, 2017 at 09:59:27AM +0100, Greg KH wrote:
> On Wed, Jan 04, 2017 at 09:54:53AM +0100, Greg KH wrote:
> > On Tue, Jan 03, 2017 at 10:44:17PM -0800, Eric S. Stone wrote:
> > > On Tue, Jan 03, 2017 at 04:30:58PM +0100, Greg KH wrote:
> > > > On Wed, Dec 28, 2016 at 10:08:43PM -0800, Eric S. Stone wrote:
> > > > > The modified functions do explicit endian checking and conversion. The
> > > > > added forced casts fix these sparse warnings:
> > > > >
> > > > > CHECK drivers/staging/gdm724x/gdm_endian.c
> > > > > drivers/staging/gdm724x/gdm_endian.c:28:24: warning: incorrect type in return expression (different base types)
> > > > > drivers/staging/gdm724x/gdm_endian.c:28:24: expected unsigned short
> > > > > drivers/staging/gdm724x/gdm_endian.c:28:24: got restricted __le16 [usertype] <noident>
> > > > > drivers/staging/gdm724x/gdm_endian.c:30:24: warning: incorrect type in return expression (different base types)
> > > > > drivers/staging/gdm724x/gdm_endian.c:30:24: expected unsigned short
> > > > > drivers/staging/gdm724x/gdm_endian.c:30:24: got restricted __be16 [usertype] <noident>
> > > > > drivers/staging/gdm724x/gdm_endian.c:36:24: warning: cast to restricted __le16
> > > > > drivers/staging/gdm724x/gdm_endian.c:38:24: warning: cast to restricted __be16
> > > > > drivers/staging/gdm724x/gdm_endian.c:44:24: warning: incorrect type in return expression (different base types)
> > > > > drivers/staging/gdm724x/gdm_endian.c:44:24: expected unsigned int
> > > > > drivers/staging/gdm724x/gdm_endian.c:44:24: got restricted __le32 [usertype] <noident>
> > > > > drivers/staging/gdm724x/gdm_endian.c:46:24: warning: incorrect type in return expression (different base types)
> > > > > drivers/staging/gdm724x/gdm_endian.c:46:24: expected unsigned int
> > > > > drivers/staging/gdm724x/gdm_endian.c:46:24: got restricted __be32 [usertype] <noident>
> > > > > drivers/staging/gdm724x/gdm_endian.c:52:24: warning: cast to restricted __le32
> > > > > drivers/staging/gdm724x/gdm_endian.c:54:24: warning: cast to restricted __be32
> > > > >
> > > > > Signed-off-by: Eric S. Stone <esstone@xxxxxxxxx>
> > > > > ---
> > > > > drivers/staging/gdm724x/gdm_endian.c | 16 ++++++++--------
> > > > > 1 file changed, 8 insertions(+), 8 deletions(-)
> > > > >
> > > > > diff --git a/drivers/staging/gdm724x/gdm_endian.c b/drivers/staging/gdm724x/gdm_endian.c
> > > > > index d7144e7..00ae7a8 100644
> > > > > --- a/drivers/staging/gdm724x/gdm_endian.c
> > > > > +++ b/drivers/staging/gdm724x/gdm_endian.c
> > > > > @@ -25,31 +25,31 @@ void gdm_set_endian(struct gdm_endian *ed, u8 dev_endian)
> > > > > u16 gdm_cpu_to_dev16(struct gdm_endian *ed, u16 x)
> > > > > {
> > > > > if (ed->dev_ed == ENDIANNESS_LITTLE)
> > > > > - return cpu_to_le16(x);
> > > > > + return (__force u16)cpu_to_le16(x);
> > > > > else
> > > > > - return cpu_to_be16(x);
> > > > > + return (__force u16)cpu_to_be16(x);
> > > >
> > > > That's crazy, look at what you are writing here, does it really make any
> > > > sense?
> > > >
> > > > Please fix this up properly...
> > > >
> > > > thanks,
> > > >
> > > > greg k-h
> > >
> > > Thanks for the feedback and sorry for the Crazy. I'll resubmit using
> > > cpu_to_le16s(u16 *) and friends, avoiding casts. I was trying to keep
> > > the code using the same non-mutating converters it was already using,
> > > but it was a bad tradeoff since those all use the __bitwise annotated
> > > types and we need u16/u32 here. So instead:
> > >
> > > u16 gdm_cpu_to_dev16(struct gdm_endian *ed, u16 x)
> > > {
> > > if (ed->dev_ed == ENDIANNESS_LITTLE)
> > > cpu_to_le16s(&x);
> > > else
> > > cpu_to_be16s(&x);
> > > return x;
> > > }
> > >
> > > I also tried to alternatively fix the warnings by normalizing the
> > > driver code to one endiannes (getting rid of u16/u32), but since the
> > > different device models have different endianness, conversions of some
> > > sort remain necessary.
> >
> > Ah, ick, sorry, I was a bit hasty on my review (having to review 300+
> > staging patches all at once does that to you...)
> >
> > This code is odd, but not unprecidented, there are other drivers that
> > need this. Let me go see how they handled this type of thing, it might
> > be easier to just copy what they did...
>
> Ah, and you were right with your first try, look at
> drivers/usb/host/ohci.h and the cpu_to_hc16() function. Your use of
> __force is right, the only thing I would suggest is adding a new type,
> __dev16 much like the ohci.h file creates __hc16 to keep things a bit
> more obvious what is going on here.
>
> Can you fix up your original patch much like this?
>
> thanks, and again, sorry for the complaint on your patch, my fault.
>
> greg k-h
Agree typedefs would make this clearer, I'll add them in. With those,
I prefer this v1 casting approach to the v2 mutating converters.
No worries on the initial complaint, and thanks for the second look!
-Eric S. Stone
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
