Re: [PATCH] powerpc/hvsi: Fix endianness issues in the HVSI driver

[Benjamin Herrenschmidt <benh@kernel.crashing.org>]

On Wed, 2015-08-19 at 23:37 +0200, Laurent Dufour wrote:
> On 03/08/2015 13:00, David Laight wrote:
From: Laurent Dufour
> > > Sent: 31 July 2015 10:30
> > > This patch fixes several endianness issues detected when running
> > > the HVSI
> > > driver in little endian mode.
> > > 
> > > These issues are raised in little endian mode because the data
> > > exchanged in
> > > memory between the kernel and the hypervisor has to be in big
> > > endian
> > > format.
> > ...
> > > diff --git a/drivers/tty/hvc/hvsi.c b/drivers/tty/hvc/hvsi.c
> > > index 41901997c0d6..a75146f600cb 100644
> > > --- a/drivers/tty/hvc/hvsi.c
> > > +++ b/drivers/tty/hvc/hvsi.c
> > > @@ -240,9 +240,9 @@ static void hvsi_recv_control(struct
> > > hvsi_struct *hp, uint8_t *packet,
> > >  {
> > >  	struct hvsi_control *header = (struct hvsi_control
> > > *)packet;
> > > 
> > > -	switch (header->verb) {
> > > +	switch (be16_to_cpu(header->verb)) {
> > >  		case VSV_MODEM_CTL_UPDATE:
> > > -			if ((header->word & HVSI_TSCD) == 0) {
> > > +			if ((be32_to_cpu(header->word) &
> > > HVSI_TSCD) == 0) {
> > 
> > It is generally best to byteswap constants.
> > 
> > 	David
> 
> Thanks David for your review.
> Regarding the byte swapping of the constants, I'm wondering if this 
>  the
> best way here.
> For instance, Benjamin wrote a similar patch to fix another
> endianness
> issue (99fc1d91b8fc) and he doesn't convert the constant neither.
> It think that byte swapping the constant value will impact more code,
> and may not ease code reading.

Right, I disagree with byteswapping the constants at their definition
point, however maybe he meant using cpu_to_be16(CONSTANT) ?

In any case, it's pretty moot as we have the lhbrx instruction which
will do the load and byteswap and for HVSI, even if it was a tad slower
than a normal load, it would not make a noticeable difference.

Ben.