| Message ID | 20200616154951.3050-1-sultan@kerneltoast.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2] HID: i2c-hid: Use block reads when possible to save power | expand |
Hi On 7/1/20 6:00 PM, Sultan Alsawaf wrote: > On Wed, Jul 01, 2020 at 11:04:01AM +0300, Jarkko Nikula wrote: >> On 6/29/20 8:43 PM, Sultan Alsawaf wrote: >>> Hmm, for some reason in 5.8 I get the same problem, but 5.7 is fine. Could you >>> try this on 5.7 and see if it works? >>> >>> In the meantime I'll bisect 5.8 to see why it's causing problems for me... >>> >> I see the same issue on top of v5.7: > > Try reverting my "i2c: designware: Only check the first byte for SMBus block > read length" patch and apply the following change instead: > This combination (the diff and this HID patch) works on top of v5.7. I tried also these other combinations: v5.7 - HID patch + this diff -> ok - HID patch -> not ok - HID + acked i2c-dw patch -> acked i2c-dw patch doesn't apply v5.8-rc3 - acked i2c-dw patch -> ok - HID patch -> nok - HID patch + acked i2c-dw patch -> nok - HID patch + this diff -> diff doesn't apply Hopefully gives some glue. I'll be out of office for a few weeks and unfortunately cannot test patches meanwhile. Jarkko
On Tue, Sep 15, 2020 at 02:55:48PM +0300, Jarkko Nikula wrote: > Hi > > On 9/14/20 3:15 AM, Sultan Alsawaf wrote: > > From: Sultan Alsawaf <sultan@kerneltoast.com> > > > > This is a squash of the following: > > > > i2c: designware: Fix transfer failures for invalid SMBus block reads > > > > SMBus block reads can be broken because the read function will just skip > > over bytes it doesn't like until reaching a byte that conforms to the > > length restrictions for block reads. This is problematic when it isn't > > known if the incoming payload is indeed a conforming block read. > > > > According to the SMBus specification, block reads will only send the > > payload length in the first byte, so we can fix this by only considering > > the first byte in a sequence for block read length purposes. > > > > In addition, when the length byte is invalid, the original transfer > > length still needs to be adjusted to avoid a controller timeout. > > > > Fixes: c3ae106050b9 ("i2c: designware: Implement support for SMBus block read and write") > > Signed-off-by: Sultan Alsawaf <sultan@kerneltoast.com> > > > > i2c: designware: Ensure tx_buf_len is nonzero for SMBus block reads > > > > The point of adding a byte to len in i2c_dw_recv_len() is to make sure > > that tx_buf_len is nonzero, so that i2c_dw_xfer_msg() can let the i2c > > controller know that the i2c transaction can end. Otherwise, the i2c > > controller will think that the transaction can never end for block > > reads, which results in the stop-detection bit never being set and thus > > the transaction timing out. > > > > Adding a byte to len is not a reliable way to do this though; sometimes > > it lets tx_buf_len become zero, which results in the scenario described > > above. Therefore, just directly ensure tx_buf_len cannot be zero to fix > > the issue. > > > > Fixes: c3ae106050b9 ("i2c: designware: Implement support for SMBus block read and write") > > Signed-off-by: Sultan Alsawaf <sultan@kerneltoast.com> > > > > i2c: designware: Allow SMBus block reads up to 255 bytes in length > > > > According to the SMBus 3.0 protocol specification, block transfer limits > > were increased from 32 bytes to 255 bytes. Remove the obsolete 32-byte > > limitation. > > > > Signed-off-by: Sultan Alsawaf <sultan@kerneltoast.com> > > > > HID: i2c-hid: Use block reads when possible to save power > > > > We have no way of knowing how large an incoming payload is going to be, > > so the only strategy available up until now has been to always retrieve > > the maximum possible report length over i2c, which can be quite > > inefficient. For devices that send reports in block read format, the i2c > > controller driver can read the payload length on the fly and terminate > > the i2c transaction early, resulting in considerable power savings. > > > > On a Dell Precision 15 5540 with an i9-9880H, resting my finger on the > > touchpad causes psys power readings to go up by about 4W and hover there > > until I remove my finger. With this patch, my psys readings go from 4.7W > > down to 3.1W, yielding about 1.6W in savings. This is because my > > touchpad's max report length is 60 bytes, but all of the regular reports > > it sends for touch events are only 32 bytes, so the i2c transfer is > > roughly halved for the common case. > > > > Signed-off-by: Sultan Alsawaf <sultan@kerneltoast.com> > > --- > > Hi Jarkko, > > > > Sorry for the delayed response. Life gets in the way of the things that really > > matter, like kernel hacking ;) > > > > I fixed the issue with the i2c block reads on 5.8. I've squashed all 4 of my i2c > > commits into this email for simplicity; please apply this patch on either 5.8 or > > 5.9 (it applies cleanly to both) and let me know if it works with your i2c-hid > > touchscreen. If all is well, I will resubmit these patches individually in one > > patchset, in a new thread. > > > I tested this on top of fc4f28bb3daf ("Merge tag 'for-5.9-rc5-tag' of > git://git.kernel.org/pub/scm/linux/kernel/git/kdave/linux") and seems to be > working fine. What was the key change compared to previous version that was > regressing for me? This change fixed your issue (and my issue with 5.8): --- a/drivers/i2c/busses/i2c-designware-master.c +++ b/drivers/i2c/busses/i2c-designware-master.c @@ -395,8 +395,9 @@ i2c_dw_recv_len(struct dw_i2c_dev *dev, u8 len) * Adjust the buffer length and mask the flag * after receiving the first byte. */ - len += (flags & I2C_CLIENT_PEC) ? 2 : 1; - dev->tx_buf_len = len - min_t(u8, len, dev->rx_outstanding); + if (flags & I2C_CLIENT_PEC) + len++; + dev->tx_buf_len = len - min_t(u8, len - 1, dev->rx_outstanding); msgs[dev->msg_read_idx].len = len; msgs[dev->msg_read_idx].flags &= ~I2C_M_RECV_LEN; I've attributed this change with the following commit message: "i2c: designware: Ensure tx_buf_len is nonzero for SMBus block reads The point of adding a byte to len in i2c_dw_recv_len() is to make sure that tx_buf_len is nonzero, so that i2c_dw_xfer_msg() can let the i2c controller know that the i2c transaction can end. Otherwise, the i2c controller will think that the transaction can never end for block reads, which results in the stop-detection bit never being set and thus the transaction timing out. Adding a byte to len is not a reliable way to do this though; sometimes it lets tx_buf_len become zero, which results in the scenario described above. Therefore, just directly ensure tx_buf_len cannot be zero to fix the issue." Does the patch series look good to submit? Sultan
On Sun, 13 Sep 2020, Sultan Alsawaf wrote: > From: Sultan Alsawaf <sultan@kerneltoast.com> > > This is a squash of the following: > > i2c: designware: Fix transfer failures for invalid SMBus block reads [ .... ] > HID: i2c-hid: Use block reads when possible to save power [ .... ] > --- > Hi Jarkko, > > Sorry for the delayed response. Life gets in the way of the things that really > matter, like kernel hacking ;) > > I fixed the issue with the i2c block reads on 5.8. I've squashed all 4 of my i2c > commits into this email for simplicity; please apply this patch on either 5.8 or > 5.9 (it applies cleanly to both) and let me know if it works with your i2c-hid > touchscreen. If all is well, I will resubmit these patches individually in one > patchset, in a new thread. > > Thanks, > Sultan > drivers/hid/i2c-hid/i2c-hid-core.c | 5 ++++- > drivers/i2c/busses/i2c-designware-master.c | 15 +++++++++------ > 2 files changed, 13 insertions(+), 7 deletions(-) I believe it makes most sense for this to go through i2c tree as a whole, so feel free to add Acked-by: Jiri Kosina <jkosina@suse.cz> for the drivers/hid/i2c-hid/i2c-hid-core.c hunk. Thanks,
On 9/15/20 8:48 PM, Sultan Alsawaf wrote: > On Tue, Sep 15, 2020 at 02:55:48PM +0300, Jarkko Nikula wrote: >> I tested this on top of fc4f28bb3daf ("Merge tag 'for-5.9-rc5-tag' of >> git://git.kernel.org/pub/scm/linux/kernel/git/kdave/linux") and seems to be >> working fine. What was the key change compared to previous version that was >> regressing for me? > > This change fixed your issue (and my issue with 5.8): > --- a/drivers/i2c/busses/i2c-designware-master.c > +++ b/drivers/i2c/busses/i2c-designware-master.c > @@ -395,8 +395,9 @@ i2c_dw_recv_len(struct dw_i2c_dev *dev, u8 len) > * Adjust the buffer length and mask the flag > * after receiving the first byte. > */ > - len += (flags & I2C_CLIENT_PEC) ? 2 : 1; > - dev->tx_buf_len = len - min_t(u8, len, dev->rx_outstanding); > + if (flags & I2C_CLIENT_PEC) > + len++; > + dev->tx_buf_len = len - min_t(u8, len - 1, dev->rx_outstanding); > msgs[dev->msg_read_idx].len = len; > msgs[dev->msg_read_idx].flags &= ~I2C_M_RECV_LEN; > > I've attributed this change with the following commit message: > "i2c: designware: Ensure tx_buf_len is nonzero for SMBus block reads > > The point of adding a byte to len in i2c_dw_recv_len() is to make sure > that tx_buf_len is nonzero, so that i2c_dw_xfer_msg() can let the i2c > controller know that the i2c transaction can end. Otherwise, the i2c > controller will think that the transaction can never end for block > reads, which results in the stop-detection bit never being set and thus > the transaction timing out. > > Adding a byte to len is not a reliable way to do this though; sometimes > it lets tx_buf_len become zero, which results in the scenario described > above. Therefore, just directly ensure tx_buf_len cannot be zero to fix > the issue." > Ok, nice that you found it. > Does the patch series look good to submit? > Yes, go ahead. Jarkko
diff --git a/drivers/hid/i2c-hid/i2c-hid-core.c b/drivers/hid/i2c-hid/i2c-hid-core.c index 294c84e136d7..739dccfc57e1 100644 --- a/drivers/hid/i2c-hid/i2c-hid-core.c +++ b/drivers/hid/i2c-hid/i2c-hid-core.c @@ -472,11 +472,14 @@ static void i2c_hid_get_input(struct i2c_hid *ihid) int ret; u32 ret_size; int size = le16_to_cpu(ihid->hdesc.wMaxInputLength); + u16 flags; if (size > ihid->bufsize) size = ihid->bufsize; - ret = i2c_master_recv(ihid->client, ihid->inbuf, size); + /* Try to do a block read if the size fits in one byte */ + flags = size > 255 ? I2C_M_RD : I2C_M_RD | I2C_M_RECV_LEN; + ret = i2c_transfer_buffer_flags(ihid->client, ihid->inbuf, size, flags); if (ret != size) { if (ret < 0) return;