| Message ID | 20191112203132.163306-3-dmitry.torokhov@gmail.com |
|---|---|
| State | Rejected |
| Headers | show |
| Series | Use void pointers instead of char in I2C transfer APIs | expand |
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.
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.
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>
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
> 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. Seems like I am on Uwe's side again. For me, the current way is also more readable.
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; } }
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(-)