Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1210034 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH] powerpc/hvsi: Fix endianness issues in the HVSI driver Laurent Dufour <ldufour@linux.vnet.ibm.com> - 2015-08-19 23:40 +0200
    Re: [PATCH] powerpc/hvsi: Fix endianness issues in the HVSI driver Benjamin Herrenschmidt <benh@kernel.crashing.org> - 2015-08-20 00:00 +0200

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

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.

Cheers,
Laurent.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1210046

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.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web