diff mbox series

[v2] HID: i2c-hid: Use block reads when possible to save power

Message ID 20200616154951.3050-1-sultan@kerneltoast.com
State New
Headers show
Series [v2] HID: i2c-hid: Use block reads when possible to save power | expand

Commit Message

Sultan Alsawaf June 16, 2020, 3:49 p.m. UTC 
From: Sultan Alsawaf <sultan@kerneltoast.com>

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>
---
Jarkko, could you try this?
 drivers/hid/i2c-hid/i2c-hid-core.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

Comments

Andy Shevchenko June 16, 2020, 4:41 p.m. UTC | #1 
On Tue, Jun 16, 2020 at 08:49:51AM -0700, Sultan Alsawaf wrote:
> From: Sultan Alsawaf <sultan@kerneltoast.com>
> 
> 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.

> +	/* 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;

AFAIR SMBus specification tells about 256. Why 255?

Andi, am I correct?
Andi Shyti June 16, 2020, 5:18 p.m. UTC | #2 
Hi Andy,

> > 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.
> 
> > +	/* 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;
> 
> AFAIR SMBus specification tells about 256. Why 255?
> 
> Andi, am I correct?

Actually the SMBUS 3.0 protocol from 2015[*] says 255:

"
D.6 255 Bytes in Process Call

The maximum number of bytes allowed in the Block Write-Block Read
Process Call (Section 6.5.8) was increased from 32 to 255.
"

But why does it matter... I see the patch is detatching itself
from smbus.

And, actually, I wonder if this is the right way to fix it, isn't
it better to fix smbus instead?

I have a patch ready that fixes the smbus transfer size, perhaps
I should rebase, test and send it.

Andi

[*] http://smbus.org/specs/SMBus_3_0_20141220.pdf
Sultan Alsawaf June 16, 2020, 5:32 p.m. UTC | #3 
On Tue, Jun 16, 2020 at 08:18:54PM +0300, Andi Shyti wrote:
> Hi Andy,
> 
> > > 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.
> > 
> > > +	/* 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;
> > 
> > AFAIR SMBus specification tells about 256. Why 255?
> > 
> > Andi, am I correct?
> 
> Actually the SMBUS 3.0 protocol from 2015[*] says 255:
> 
> "
> D.6 255 Bytes in Process Call
> 
> The maximum number of bytes allowed in the Block Write-Block Read
> Process Call (Section 6.5.8) was increased from 32 to 255.
> "
> 
> But why does it matter... I see the patch is detatching itself
> from smbus.
> 
> And, actually, I wonder if this is the right way to fix it, isn't
> it better to fix smbus instead?

I think the best solution would be to modify the i2c api to allow passing in a
function pointer and a payload size length, to specify how to interpret the size
of the incoming payload, so the adapter could handle both the HID over i2c
transfer spec and SMBus block reads without needing to read more bytes than
needed.

For example, for an SMBus block read, the payload size is specified in the first
byte and it is limited to 32 bytes. However, for HID over i2c, the payload size
is specified in the first two bytes, and there are also some device quirks
involved to reinterpret the reported size.

A nice solution would be to pass in how many bytes the i2c payload size can
contain, as well as a function pointer to evaluate the reported payload size in
a way that the caller wants. This would require modifying every i2c adapter
driver to add this functionality, but it would fix the efficiency problem faced
by i2c-hid and perhaps others.

> I have a patch ready that fixes the smbus transfer size, perhaps
> I should rebase, test and send it.

For the i2c-hid driver?

Sultan
Andi Shyti June 16, 2020, 6:02 p.m. UTC | #4 
Hi Sultan,

> > > > 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.
> > > 
> > > > +	/* 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;
> > > 
> > > AFAIR SMBus specification tells about 256. Why 255?
> > > 
> > > Andi, am I correct?
> > 
> > Actually the SMBUS 3.0 protocol from 2015[*] says 255:
> > 
> > "
> > D.6 255 Bytes in Process Call
> > 
> > The maximum number of bytes allowed in the Block Write-Block Read
> > Process Call (Section 6.5.8) was increased from 32 to 255.
> > "
> > 
> > But why does it matter... I see the patch is detatching itself
> > from smbus.
> > 
> > And, actually, I wonder if this is the right way to fix it, isn't
> > it better to fix smbus instead?
> 
> I think the best solution would be to modify the i2c api to allow passing in a
> function pointer and a payload size length, to specify how to interpret the size
> of the incoming payload, so the adapter could handle both the HID over i2c
> transfer spec and SMBus block reads without needing to read more bytes than
> needed.

Can't you do that by specifying the xfer function?

When you use smbus_read/write in block or byte or whatever, smbus
always checks if there is an xfer function specified and uses
that.

If it's not specified it uses the default smbus functions with
the limitations that come with it.

> For example, for an SMBus block read, the payload size is specified in the first
> byte and it is limited to 32 bytes. However, for HID over i2c, the payload size
> is specified in the first two bytes, and there are also some device quirks
> involved to reinterpret the reported size.

which is wrong. The 32 bytes limitation is outdated: in the link
that I gave before (i.e.  this one [*]), the new SMBUS specifies
255 maximum for read/write block.

> A nice solution would be to pass in how many bytes the i2c payload size can
> contain, as well as a function pointer to evaluate the reported payload size in
> a way that the caller wants. This would require modifying every i2c adapter
> driver to add this functionality, but it would fix the efficiency problem faced
> by i2c-hid and perhaps others.
> 
> > I have a patch ready that fixes the smbus transfer size, perhaps
> > I should rebase, test and send it.
> 
> For the i2c-hid driver?

No, sorry, for smbus.

Now... here you are replacing "i2c_master_recv" with
"i2c_transfer_buffer_flags". I do not really like this change,
although I understand it's necessary, because we are bypassing
the real issue that is that the smbus implementation is outdated.

I have a patch for that that for a matter of time I never sent.

Andi
Sultan Alsawaf June 16, 2020, 6:17 p.m. UTC | #5 
On Tue, Jun 16, 2020 at 09:02:54PM +0300, Andi Shyti wrote:
> Hi Sultan,
> 
> > > > > 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.
> > > > 
> > > > > +	/* 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;
> > > > 
> > > > AFAIR SMBus specification tells about 256. Why 255?
> > > > 
> > > > Andi, am I correct?
> > > 
> > > Actually the SMBUS 3.0 protocol from 2015[*] says 255:
> > > 
> > > "
> > > D.6 255 Bytes in Process Call
> > > 
> > > The maximum number of bytes allowed in the Block Write-Block Read
> > > Process Call (Section 6.5.8) was increased from 32 to 255.
> > > "
> > > 
> > > But why does it matter... I see the patch is detatching itself
> > > from smbus.
> > > 
> > > And, actually, I wonder if this is the right way to fix it, isn't
> > > it better to fix smbus instead?
> > 
> > I think the best solution would be to modify the i2c api to allow passing in a
> > function pointer and a payload size length, to specify how to interpret the size
> > of the incoming payload, so the adapter could handle both the HID over i2c
> > transfer spec and SMBus block reads without needing to read more bytes than
> > needed.
> 
> Can't you do that by specifying the xfer function?
> 
> When you use smbus_read/write in block or byte or whatever, smbus
> always checks if there is an xfer function specified and uses
> that.
> 
> If it's not specified it uses the default smbus functions with
> the limitations that come with it.

The xfer functions are specified on a per-adapter basis. In the case of i2c-hid,
we need to tell the adapter to interpret the payload size in a specific way,
which I *think* is only specific to HID over i2c (i.e., using 16 bits to store
the length and then checking it for device quirks).

> > For example, for an SMBus block read, the payload size is specified in the first
> > byte and it is limited to 32 bytes. However, for HID over i2c, the payload size
> > is specified in the first two bytes, and there are also some device quirks
> > involved to reinterpret the reported size.
> 
> which is wrong. The 32 bytes limitation is outdated: in the link
> that I gave before (i.e.  this one [*]), the new SMBUS specifies
> 255 maximum for read/write block.

Oops. But still, for SMBus block reads, the size is limited to 8 bits. For HID
over i2c, it can be 16 bits. I don't see how we can handle this without some api
cooperation to tell the adapter what the caller is expecting to see.

> > A nice solution would be to pass in how many bytes the i2c payload size can
> > contain, as well as a function pointer to evaluate the reported payload size in
> > a way that the caller wants. This would require modifying every i2c adapter
> > driver to add this functionality, but it would fix the efficiency problem faced
> > by i2c-hid and perhaps others.
> > 
> > > I have a patch ready that fixes the smbus transfer size, perhaps
> > > I should rebase, test and send it.
> > 
> > For the i2c-hid driver?
> 
> No, sorry, for smbus.
> 
> Now... here you are replacing "i2c_master_recv" with
> "i2c_transfer_buffer_flags". I do not really like this change,
> although I understand it's necessary, because we are bypassing
> the real issue that is that the smbus implementation is outdated.
> 
> I have a patch for that that for a matter of time I never sent.

Can it handle block reads (8 bit size) and HID over i2c (16 bit size)?

Sultan
Jarkko Nikula June 17, 2020, 11:17 a.m. UTC | #6 
On 6/16/20 6:49 PM, Sultan Alsawaf wrote:
> From: Sultan Alsawaf <sultan@kerneltoast.com>
> 
> 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>
> ---
> Jarkko, could you try this?
>   drivers/hid/i2c-hid/i2c-hid-core.c | 5 ++++-
>   1 file changed, 4 insertions(+), 1 deletion(-)
> 
> 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;

This still causes a regression for me.

[    9.457656] i2c_hid i2c-ELAN221D:00: Fetching the HID descriptor
[    9.457663] i2c_hid i2c-ELAN221D:00: __i2c_hid_command: cmd=01 00
[    9.458591] i2c_hid i2c-ELAN221D:00: HID Descriptor: 1e 00 00 01 31 
02 02 00 03 00 43 00 04 00 ff 00 05 00 06 00 f3 04 1d 22 10 56 00 00 00 00
[    9.459519] i2c_hid i2c-ELAN221D:00: entering i2c_hid_parse
[    9.459526] i2c_hid i2c-ELAN221D:00: i2c_hid_hwreset
[    9.459576] i2c_hid i2c-ELAN221D:00: i2c_hid_set_power
[    9.459591] i2c_hid i2c-ELAN221D:00: __i2c_hid_command: cmd=05 00 00 08
[    9.464070] i2c_hid i2c-ELAN221D:00: resetting...
[    9.464078] i2c_hid i2c-ELAN221D:00: __i2c_hid_command: cmd=05 00 00 01
[    9.464346] i2c_hid i2c-ELAN221D:00: __i2c_hid_command: waiting...
[   10.497169] i2c_designware i2c_designware.3: controller timed out
[   10.533940] i2c_designware i2c_designware.3: timeout in disabling adapter
[   14.528677] i2c_hid i2c-ELAN221D:00: __i2c_hid_command: finished.
[   14.528695] i2c_hid i2c-ELAN221D:00: failed to reset device.
[   14.536125] i2c_hid i2c-ELAN221D:00: i2c_hid_set_power
[   14.536141] i2c_hid i2c-ELAN221D:00: __i2c_hid_command: cmd=05 00 01 08
[   14.556335] i2c_designware i2c_designware.3: timeout waiting for bus 
ready
[   14.565086] i2c_hid i2c-ELAN221D:00: failed to change power setting.
[   15.584374] i2c_hid i2c-ELAN221D:00: i2c_hid_hwreset
[   15.584395] i2c_hid i2c-ELAN221D:00: i2c_hid_set_power
[   15.584410] i2c_hid i2c-ELAN221D:00: __i2c_hid_command: cmd=05 00 00 08
[   15.605683] i2c_designware i2c_designware.3: timeout waiting for bus 
ready
[   15.614304] i2c_hid i2c-ELAN221D:00: failed to change power setting.
...
Sultan Alsawaf June 29, 2020, 5:43 p.m. UTC | #7 
On Wed, Jun 17, 2020 at 02:17:19PM +0300, Jarkko Nikula wrote:
> On 6/16/20 6:49 PM, Sultan Alsawaf wrote:
> > From: Sultan Alsawaf <sultan@kerneltoast.com>
> > 
> > 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>
> > ---
> > Jarkko, could you try this?
> >   drivers/hid/i2c-hid/i2c-hid-core.c | 5 ++++-
> >   1 file changed, 4 insertions(+), 1 deletion(-)
> > 
> > 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;
> 
> This still causes a regression for me.

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...

Sultan
Jarkko Nikula July 1, 2020, 8:04 a.m. UTC | #8 
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:

[    9.330514] i2c_hid i2c-ELAN221D:00: Fetching the HID descriptor
[    9.334761] i2c_hid i2c-ELAN221D:00: __i2c_hid_command: cmd=01 00
[    9.335716] i2c_hid i2c-ELAN221D:00: HID Descriptor: 1e 00 00 01 31 
02 02 00 03 00 43 00 04 00 ff 00 05 00 06 00 f3 04 1d 22 10 56 00 00 00 00
[    9.353408] i2c_hid i2c-ELAN221D:00: entering i2c_hid_parse
[    9.353416] i2c_hid i2c-ELAN221D:00: i2c_hid_hwreset
[    9.353502] i2c_hid i2c-ELAN221D:00: i2c_hid_set_power
[    9.353520] i2c_hid i2c-ELAN221D:00: __i2c_hid_command: cmd=05 00 00 08
[    9.362304] i2c_hid i2c-ELAN221D:00: resetting...
[    9.370585] i2c_hid i2c-ELAN221D:00: __i2c_hid_command: cmd=05 00 00 01
[    9.389175] i2c_hid i2c-ELAN221D:00: __i2c_hid_command: waiting...
[   10.416458] i2c_designware i2c_designware.3: controller timed out
[   10.476853] i2c_designware i2c_designware.3: timeout in disabling adapter
[   11.983806] [<00000000fac753ed>] i2c_dw_isr [i2c_designware_core]
[   14.544499] i2c_hid i2c-ELAN221D:00: __i2c_hid_command: finished.
[   14.552123] i2c_hid i2c-ELAN221D:00: failed to reset device.
[   14.559263] i2c_hid i2c-ELAN221D:00: i2c_hid_set_power
[   14.565822] i2c_hid i2c-ELAN221D:00: __i2c_hid_command: cmd=05 00 01 08
[   14.600256] i2c_designware i2c_designware.3: timeout waiting for bus 
ready
[   14.608800] i2c_hid i2c-ELAN221D:00: failed to change power setting.
[   15.632103] i2c_hid i2c-ELAN221D:00: i2c_hid_hwreset
[   15.638460] i2c_hid i2c-ELAN221D:00: i2c_hid_set_power
[   15.646422] i2c_hid i2c-ELAN221D:00: __i2c_hid_command: cmd=05 00 00 08
...
Sultan Alsawaf July 1, 2020, 3 p.m. UTC | #9 
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:

--- a/drivers/i2c/busses/i2c-designware-master.c
+++ b/drivers/i2c/busses/i2c-designware-master.c
@@ -394,10 +394,12 @@ i2c_dw_read(struct dw_i2c_dev *dev)
 			u32 flags = msgs[dev->msg_read_idx].flags;
 
 			*buf = dw_readl(dev, DW_IC_DATA_CMD);
-			/* Ensure length byte is a valid value */
-			if (flags & I2C_M_RECV_LEN &&
-				*buf <= I2C_SMBUS_BLOCK_MAX && *buf > 0) {
-				len = i2c_dw_recv_len(dev, *buf);
+			if (flags & I2C_M_RECV_LEN) {
+				/* Ensure length byte is a valid value */
+				if (*buf <= I2C_SMBUS_BLOCK_MAX && *buf > 0)
+					len = i2c_dw_recv_len(dev, *buf);
+				else
+					len = i2c_dw_recv_len(dev, len);
 			}
 			buf++;
 			dev->rx_outstanding--;
Jarkko Nikula July 3, 2020, 11:18 a.m. UTC | #10 
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
diff mbox series

Patch

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;