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
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
