[v3,2/3] i2c: smbus: use get/put_unaligned_le16 when working with word data
diff mbox series

Message ID 20191112203132.163306-3-dmitry.torokhov@gmail.com
State Under Review
Headers show
Series
  • Use void pointers instead of char in I2C transfer APIs
Related show

Commit Message

Dmitry Torokhov Nov. 12, 2019, 8:31 p.m. UTC
It is potentially more performant, and also shows intent more clearly,
to use get_unaligned_le16() and put_unaligned_le16() when working with
word data.

Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>

---

Changes in v3:
- split put_unaligned_le16 into a separate patch
- more call sites converted to get/put_unaligned_le16

 drivers/i2c/i2c-core-smbus.c | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

Comments

Luca Ceresoli Nov. 13, 2019, 9:47 a.m. UTC | #1
Hi Dmitry,

On 12/11/19 21:31, Dmitry Torokhov wrote:
> It is potentially more performant, and also shows intent more clearly,
> to use get_unaligned_le16() and put_unaligned_le16() when working with
> word data.
> 
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> 
> ---
> 
> Changes in v3:
> - split put_unaligned_le16 into a separate patch
> - more call sites converted to get/put_unaligned_le16
> 
>  drivers/i2c/i2c-core-smbus.c | 12 +++++-------
>  1 file changed, 5 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/i2c/i2c-core-smbus.c b/drivers/i2c/i2c-core-smbus.c
> index f8708409b4dbc..7b4e2270eeda1 100644
> --- a/drivers/i2c/i2c-core-smbus.c
> +++ b/drivers/i2c/i2c-core-smbus.c
> @@ -15,6 +15,7 @@
>  #include <linux/i2c.h>
>  #include <linux/i2c-smbus.h>
>  #include <linux/slab.h>
> +#include <asm/unaligned.h>
>  
>  #include "i2c-core.h"
>  
> @@ -370,8 +371,7 @@ static s32 i2c_smbus_xfer_emulated(struct i2c_adapter *adapter, u16 addr,
>  			msg[1].len = 2;
>  		else {
>  			msg[0].len = 3;
> -			msgbuf0[1] = data->word & 0xff;
> -			msgbuf0[2] = data->word >> 8;
> +			put_unaligned_le16(data->word, msgbuf0 + 1);
>  		}
>  		break;
>  	case I2C_SMBUS_PROC_CALL:
> @@ -379,8 +379,7 @@ static s32 i2c_smbus_xfer_emulated(struct i2c_adapter *adapter, u16 addr,
>  		read_write = I2C_SMBUS_READ;
>  		msg[0].len = 3;
>  		msg[1].len = 2;
> -		msgbuf0[1] = data->word & 0xff;
> -		msgbuf0[2] = data->word >> 8;
> +		put_unaligned_le16(data->word, msgbuf0 + 1);
>  		break;
>  	case I2C_SMBUS_BLOCK_DATA:
>  		if (read_write == I2C_SMBUS_READ) {
> @@ -487,7 +486,7 @@ static s32 i2c_smbus_xfer_emulated(struct i2c_adapter *adapter, u16 addr,
>  			break;
>  		case I2C_SMBUS_WORD_DATA:
>  		case I2C_SMBUS_PROC_CALL:
> -			data->word = msgbuf1[0] | (msgbuf1[1] << 8);
> +			data->word = get_unaligned_le16(msgbuf1);

Well, msgbuf1 cannot be unaligned, so it looks like you just need to
convert little endian to host endian. Perhaps __le16_to_cpu(msgbuf1) is
better (and equally or more efficient).

> @@ -648,8 +647,7 @@ s32 i2c_smbus_read_i2c_block_data_or_emulated(const struct i2c_client *client,
>  			status = i2c_smbus_read_word_data(client, command + i);
>  			if (status < 0)
>  				return status;
> -			bytes[i] = status & 0xff;
> -			bytes[i + 1] = status >> 8;
> +			put_unaligned_le16(status, values + i);
>  			i += 2;
>  		}

I've been pondering on this one, because 'i' is always an even number.
But, depending on the caller, 'values' could be unaligned, thus
put_unaligned_le16() is OK here.
Dmitry Torokhov Nov. 13, 2019, 7:39 p.m. UTC | #2
Hi Luca,

On Wed, Nov 13, 2019 at 10:47:42AM +0100, Luca Ceresoli wrote:
> Hi Dmitry,
> 
> On 12/11/19 21:31, Dmitry Torokhov wrote:
> > It is potentially more performant, and also shows intent more clearly,
> > to use get_unaligned_le16() and put_unaligned_le16() when working with
> > word data.
> > 
> > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> > 
> > ---
> > 
> > Changes in v3:
> > - split put_unaligned_le16 into a separate patch
> > - more call sites converted to get/put_unaligned_le16
> > 
> >  drivers/i2c/i2c-core-smbus.c | 12 +++++-------
> >  1 file changed, 5 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/i2c/i2c-core-smbus.c b/drivers/i2c/i2c-core-smbus.c
> > index f8708409b4dbc..7b4e2270eeda1 100644
> > --- a/drivers/i2c/i2c-core-smbus.c
> > +++ b/drivers/i2c/i2c-core-smbus.c
> > @@ -15,6 +15,7 @@
> >  #include <linux/i2c.h>
> >  #include <linux/i2c-smbus.h>
> >  #include <linux/slab.h>
> > +#include <asm/unaligned.h>
> >  
> >  #include "i2c-core.h"
> >  
> > @@ -370,8 +371,7 @@ static s32 i2c_smbus_xfer_emulated(struct i2c_adapter *adapter, u16 addr,
> >  			msg[1].len = 2;
> >  		else {
> >  			msg[0].len = 3;
> > -			msgbuf0[1] = data->word & 0xff;
> > -			msgbuf0[2] = data->word >> 8;
> > +			put_unaligned_le16(data->word, msgbuf0 + 1);
> >  		}
> >  		break;
> >  	case I2C_SMBUS_PROC_CALL:
> > @@ -379,8 +379,7 @@ static s32 i2c_smbus_xfer_emulated(struct i2c_adapter *adapter, u16 addr,
> >  		read_write = I2C_SMBUS_READ;
> >  		msg[0].len = 3;
> >  		msg[1].len = 2;
> > -		msgbuf0[1] = data->word & 0xff;
> > -		msgbuf0[2] = data->word >> 8;
> > +		put_unaligned_le16(data->word, msgbuf0 + 1);
> >  		break;
> >  	case I2C_SMBUS_BLOCK_DATA:
> >  		if (read_write == I2C_SMBUS_READ) {
> > @@ -487,7 +486,7 @@ static s32 i2c_smbus_xfer_emulated(struct i2c_adapter *adapter, u16 addr,
> >  			break;
> >  		case I2C_SMBUS_WORD_DATA:
> >  		case I2C_SMBUS_PROC_CALL:
> > -			data->word = msgbuf1[0] | (msgbuf1[1] << 8);
> > +			data->word = get_unaligned_le16(msgbuf1);
> 
> Well, msgbuf1 cannot be unaligned, so it looks like you just need to
> convert little endian to host endian. Perhaps __le16_to_cpu(msgbuf1) is
> better (and equally or more efficient).

Well, msgbuf1 (as is any other variable unless adjusted by special
alignment attribute) is naturally aligned for its own data type (which
for u8 means it can start at any address), but that does not mean that
is is aligned for the purpose of storing word data. In fact, the
preceding msgbuf0 variable is 32 + 3 = 35 bytes long, which means that
msgbuf1 is starting on an odd address, and is definitely not aligned for
word access and using __le16_to_cpu to fetch that word will trap on some
architectures.

Thanks.
Luca Ceresoli Nov. 14, 2019, 9:23 a.m. UTC | #3
Hi Dmitry,

On 13/11/19 20:39, Dmitry Torokhov wrote:
> Hi Luca,
> 
> On Wed, Nov 13, 2019 at 10:47:42AM +0100, Luca Ceresoli wrote:
>> Hi Dmitry,
>>
>> On 12/11/19 21:31, Dmitry Torokhov wrote:
>>> It is potentially more performant, and also shows intent more clearly,
>>> to use get_unaligned_le16() and put_unaligned_le16() when working with
>>> word data.
>>>
>>> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
>>>
>>> ---
>>>
>>> Changes in v3:
>>> - split put_unaligned_le16 into a separate patch
>>> - more call sites converted to get/put_unaligned_le16
>>>
>>>  drivers/i2c/i2c-core-smbus.c | 12 +++++-------
>>>  1 file changed, 5 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/drivers/i2c/i2c-core-smbus.c b/drivers/i2c/i2c-core-smbus.c
>>> index f8708409b4dbc..7b4e2270eeda1 100644
>>> --- a/drivers/i2c/i2c-core-smbus.c
>>> +++ b/drivers/i2c/i2c-core-smbus.c
>>> @@ -15,6 +15,7 @@
>>>  #include <linux/i2c.h>
>>>  #include <linux/i2c-smbus.h>
>>>  #include <linux/slab.h>
>>> +#include <asm/unaligned.h>
>>>  
>>>  #include "i2c-core.h"
>>>  
>>> @@ -370,8 +371,7 @@ static s32 i2c_smbus_xfer_emulated(struct i2c_adapter *adapter, u16 addr,
>>>  			msg[1].len = 2;
>>>  		else {
>>>  			msg[0].len = 3;
>>> -			msgbuf0[1] = data->word & 0xff;
>>> -			msgbuf0[2] = data->word >> 8;
>>> +			put_unaligned_le16(data->word, msgbuf0 + 1);
>>>  		}
>>>  		break;
>>>  	case I2C_SMBUS_PROC_CALL:
>>> @@ -379,8 +379,7 @@ static s32 i2c_smbus_xfer_emulated(struct i2c_adapter *adapter, u16 addr,
>>>  		read_write = I2C_SMBUS_READ;
>>>  		msg[0].len = 3;
>>>  		msg[1].len = 2;
>>> -		msgbuf0[1] = data->word & 0xff;
>>> -		msgbuf0[2] = data->word >> 8;
>>> +		put_unaligned_le16(data->word, msgbuf0 + 1);
>>>  		break;
>>>  	case I2C_SMBUS_BLOCK_DATA:
>>>  		if (read_write == I2C_SMBUS_READ) {
>>> @@ -487,7 +486,7 @@ static s32 i2c_smbus_xfer_emulated(struct i2c_adapter *adapter, u16 addr,
>>>  			break;
>>>  		case I2C_SMBUS_WORD_DATA:
>>>  		case I2C_SMBUS_PROC_CALL:
>>> -			data->word = msgbuf1[0] | (msgbuf1[1] << 8);
>>> +			data->word = get_unaligned_le16(msgbuf1);
>>
>> Well, msgbuf1 cannot be unaligned, so it looks like you just need to
>> convert little endian to host endian. Perhaps __le16_to_cpu(msgbuf1) is
>> better (and equally or more efficient).
> 
> Well, msgbuf1 (as is any other variable unless adjusted by special
> alignment attribute) is naturally aligned for its own data type (which
> for u8 means it can start at any address), but that does not mean that
> is is aligned for the purpose of storing word data. In fact, the
> preceding msgbuf0 variable is 32 + 3 = 35 bytes long, which means that
> msgbuf1 is starting on an odd address, and is definitely not aligned for
> word access and using __le16_to_cpu to fetch that word will trap on some
> architectures.

Uhm, you're obviously right. And I was probably drunk when I wrote the
opposite! Sorry about that...

Reviewed-by: Luca Ceresoli <luca@lucaceresoli.net>
Uwe Kleine-K├Ânig Nov. 18, 2019, 7:36 a.m. UTC | #4
Hello Dmitry,

On Tue, Nov 12, 2019 at 12:31:31PM -0800, Dmitry Torokhov wrote:
> It is potentially more performant, and also shows intent more clearly,
> to use get_unaligned_le16() and put_unaligned_le16() when working with
> word data.
> 
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> 
> ---
> 
> Changes in v3:
> - split put_unaligned_le16 into a separate patch
> - more call sites converted to get/put_unaligned_le16
> 
>  drivers/i2c/i2c-core-smbus.c | 12 +++++-------
>  1 file changed, 5 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/i2c/i2c-core-smbus.c b/drivers/i2c/i2c-core-smbus.c
> index f8708409b4dbc..7b4e2270eeda1 100644
> --- a/drivers/i2c/i2c-core-smbus.c
> +++ b/drivers/i2c/i2c-core-smbus.c
> @@ -15,6 +15,7 @@
>  #include <linux/i2c.h>
>  #include <linux/i2c-smbus.h>
>  #include <linux/slab.h>
> +#include <asm/unaligned.h>
>  
>  #include "i2c-core.h"
>  
> @@ -370,8 +371,7 @@ static s32 i2c_smbus_xfer_emulated(struct i2c_adapter *adapter, u16 addr,
>  			msg[1].len = 2;
>  		else {
>  			msg[0].len = 3;
> -			msgbuf0[1] = data->word & 0xff;
> -			msgbuf0[2] = data->word >> 8;
> +			put_unaligned_le16(data->word, msgbuf0 + 1);

You claim this was clearer. For me it is not. With the explicit
assignment to msgbuf0[1] and msbbuf0[2] it is immediatly obvious to me
what happens.  Even though the endianness is explicitly mentioned in
put_unaligned_le16, it takes a bit longer for me to understand what it
does and which part of data->word ends up in which byte.

Concerning the "potentially more performant" part: I wonder if this is
backed by numbers and if it is indeed benificial on some platforms if
this is a compiler problem.

Best regards
Uwe

Patch
diff mbox series

diff --git a/drivers/i2c/i2c-core-smbus.c b/drivers/i2c/i2c-core-smbus.c
index f8708409b4dbc..7b4e2270eeda1 100644
--- a/drivers/i2c/i2c-core-smbus.c
+++ b/drivers/i2c/i2c-core-smbus.c
@@ -15,6 +15,7 @@ 
 #include <linux/i2c.h>
 #include <linux/i2c-smbus.h>
 #include <linux/slab.h>
+#include <asm/unaligned.h>
 
 #include "i2c-core.h"
 
@@ -370,8 +371,7 @@  static s32 i2c_smbus_xfer_emulated(struct i2c_adapter *adapter, u16 addr,
 			msg[1].len = 2;
 		else {
 			msg[0].len = 3;
-			msgbuf0[1] = data->word & 0xff;
-			msgbuf0[2] = data->word >> 8;
+			put_unaligned_le16(data->word, msgbuf0 + 1);
 		}
 		break;
 	case I2C_SMBUS_PROC_CALL:
@@ -379,8 +379,7 @@  static s32 i2c_smbus_xfer_emulated(struct i2c_adapter *adapter, u16 addr,
 		read_write = I2C_SMBUS_READ;
 		msg[0].len = 3;
 		msg[1].len = 2;
-		msgbuf0[1] = data->word & 0xff;
-		msgbuf0[2] = data->word >> 8;
+		put_unaligned_le16(data->word, msgbuf0 + 1);
 		break;
 	case I2C_SMBUS_BLOCK_DATA:
 		if (read_write == I2C_SMBUS_READ) {
@@ -487,7 +486,7 @@  static s32 i2c_smbus_xfer_emulated(struct i2c_adapter *adapter, u16 addr,
 			break;
 		case I2C_SMBUS_WORD_DATA:
 		case I2C_SMBUS_PROC_CALL:
-			data->word = msgbuf1[0] | (msgbuf1[1] << 8);
+			data->word = get_unaligned_le16(msgbuf1);
 			break;
 		case I2C_SMBUS_I2C_BLOCK_DATA:
 			for (i = 0; i < data->block[0]; i++)
@@ -648,8 +647,7 @@  s32 i2c_smbus_read_i2c_block_data_or_emulated(const struct i2c_client *client,
 			status = i2c_smbus_read_word_data(client, command + i);
 			if (status < 0)
 				return status;
-			bytes[i] = status & 0xff;
-			bytes[i + 1] = status >> 8;
+			put_unaligned_le16(status, values + i);
 			i += 2;
 		}
 	}