Re: Need some help on "Input: elantech - add LEN2146 to SMBus blacklist for ThinkPad L13 Gen2"

Re: Need some help on "Input: elantech - add LEN2146 to SMBus blacklist for ThinkPad L13 Gen2"

[Nikolai Kostrigin]

Hi, List!

I'd like to draw attention of Elantech and Lenovo engineers to an issue
of a non-working trackpoint.
Earlier this month I've discovered that Lenovo Thinkpad L13 trackpoint
doesn't work on Linux while touchpad does.
Both use elan_i2c + i2c_i801 drivers. The issue is confirmed for 5.4 and
5.10 kernels.

I had a preliminary discussion with Benjamin Tissoires and according to
our agreement I repost it for wider audience.
Blacklisting the device was decided to be a bad idea.
But actually I managed to get touchpad totally operational via SMBus
using a following hack:

providing a parameter to i2c_i801 driver:

modprobe i2c_i801 disable_features=0x2 (i.e. disable the block buffer).


If any additional information is needed I'm ready to provide it.
Some technical details on the issue we've managed to figure out  by now
are here-in-after:

19.02.2021 11:30, Benjamin Tissoires пишет:
> Hi Nikolai,
>
> On Thu, Feb 18, 2021 at 6:05 PM Nikolai Kostrigin <nickel@basealt.ru> wrote:
>> Hi, Benjamin!
>>
>> Sorry for bothering you directly.
>>
>> I have a question concerning the work on the issue we were discussing on
>> linux-input mailing list.
>>
>> https://patchwork.kernel.org/project/linux-input/patch/20210208075205.3784059-1-nickel@altlinux.org/
[...]
>> With elantech-smbus protocol I was getting persistent messages in dmesg:
>>
>> elan_i2c [...] 0000:00:1f.4 failed to read report data: -71
>>
>> I managed to track -71 (-EPROTO) error code to drivers/i2c/busses/i2c-i801.c
>>
>> Actually I managed to get Touchpad totally operational via SMBus using a
>> following hack:
>> providing a parameter to i2c_i801 driver:
>>
>> modprobe i2c_i801 disable_features=0x2 (i.e. disable the block buffer).
> ooh...
>
>> This makes code operating in other way and recover from illegal SMBus
>> block read size (giving warning messages in dmesg though).
>>
>>
>> [ 1270.105103] i801_smbus 0000:00:1f.4: Illegal SMBus block read size
>> from i801_isr_byte_done 93
>> [ 1270.119337] i801_smbus 0000:00:1f.4: Illegal SMBus block read size
>> from i801_isr_byte_done 93
>> [ 1270.133108] i801_smbus 0000:00:1f.4: Illegal SMBus block read size
>> from i801_isr_byte_done 93   <- theese are from trackpoint
>> [ 1270.167344] i801_smbus 0000:00:1f.4: Illegal SMBus block read size
>> from i801_isr_byte_done 93
>> [ 1399.869023] i801_smbus 0000:00:1f.4: SMBus block read size is 32
>> [ 1399.882266] i801_smbus 0000:00:1f.4: SMBus block read size is 32
>> [ 1399.896619] i801_smbus 0000:00:1f.4: SMBus block read size is 32
>> [ 1399.909850] i801_smbus 0000:00:1f.4: SMBus block read size is 32
>> <-  these are from touchpad
>> [ 1399.924117] i801_smbus 0000:00:1f.4: SMBus block read size is 32
>>
>>
>> We end up in this piece of code that has a built-in workaround and makes
>> Trackpoint work with SMBus
>>
>> NB: pay attention to /* FIXME: Recover */ priv->len = I2C_SMBUS_BLOCK_MAX;
>> All code snippets are from drivers/i2c/busses/i2c-i801.c here-in-after
>> (with some debug messages of mine).
>>
>> static void i801_isr_byte_done(struct i801_priv *priv)
>> {
>>         if (priv->is_read) {
>>                 /* For SMBus block reads, length is received with first
>> byte */
>>                 if (((priv->cmd & 0x1c) == I801_BLOCK_DATA) &&
>>                     (priv->count == 0)) {
>>                         priv->len = inb_p(SMBHSTDAT0(priv));
>>                         if (priv->len < 1 || priv->len >
>> I2C_SMBUS_BLOCK_MAX) {
>>                                 dev_err(&priv->pci_dev->dev,
>>                                         "Illegal SMBus block read size
>> from i801_isr_byte_done %d\n",
>>                                         priv->len);
>>                                 /* FIXME: Recover */
>>                                 priv->len = I2C_SMBUS_BLOCK_MAX;
>>                         } else {
>>                                 dev_dbg(&priv->pci_dev->dev,
>>                                         "SMBus block read size is %d\n",
>>                                         priv->len);
>>                         }
>>                         priv->data[-1] = priv->len;
>>                 }
>>
>>
>>
>> But with no param passed to i2c_i801 driver we end up here due to len=93
>> with our trackpoint:
>>
>> static int i801_block_transaction_by_block(struct i801_priv *priv,
>>                                            union i2c_smbus_data *data,
>>                                            char read_write, int command,
>>                                            int hwpec)
>> {
>>
>> [...]
>> status = i801_transaction(priv, xact);
>>         if (status)
>>                 return status;
>>
>>         if (read_write == I2C_SMBUS_READ ||
>>             command == I2C_SMBUS_BLOCK_PROC_CALL) {
>>                 len = inb_p(SMBHSTDAT0(priv));
>>                 if (len < 1 || len > I2C_SMBUS_BLOCK_MAX)
>>                 {
>>                         dev_err(&priv->pci_dev->dev,
>>                                         "!!!! Going to send EPROTO from
>> i801_block_transaction_by_block len= %d\n",
>>                                         len);
>>                         return -EPROTO;
>>                 }
>>                 data->block[0] = len;
>>                 for (i = 0; i < len; i++)
>>                         data->block[i + 1] = inb_p(SMBBLKDAT(priv));
>>         }
>>         return 0;
>>
>>
>> so the question is: whether trackpoint has a buggy firmware which
>> violates SMBus spec;
>>                     or it is really an I2C device which was erroneously
>> wired to SMBus i801 bridge...
> Good question.
> Couple of points:
> - these touchpads can only be used in SMBus as they use the Host
> Notify functionality
> - maybe the trackpoint on it should be handled differently
[...]
>> Please help me to decide what way to choose while developing a workaround.
>>
>> Should I correct trackpoint features list to not mess with 32-byte
>> buffer, or should I force it not to use SMBus at all and use only I2C
>> (if this at all possible).
> I think 2 types of people could help us there:
> - the elan engineers to give us the 'how this trackpoint is supposed
> to behave'. We can involve them in the public thread on linux-input
> - the i2c folks on how we can add a special quirk for these trackpoints.
[...]
> Cheers,
> Benjamin
>
>> --
>> Best regards,
>> Nikolai Kostrigin
>> ALT Linux Team
>>
-- 
Best regards,
Nikolai Kostrigin

Re: Need some help on "Input: elantech - add LEN2146 to SMBus blacklist for ThinkPad L13 Gen2"

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1273 bytes --]

Hi,

> I had a preliminary discussion with Benjamin Tissoires and according to
> our agreement I repost it for wider audience.
> Blacklisting the device was decided to be a bad idea.
> But actually I managed to get touchpad totally operational via SMBus
> using a following hack:
> 
> providing a parameter to i2c_i801 driver:
> 
> modprobe i2c_i801 disable_features=0x2 (i.e. disable the block buffer).

So, from an I2C perspective, there are two things to mention here:

a) I am in the process of extending the I2C core to allow block
transfers > 32 byte. This is a slow process, though, because we need to
pay attention to not break userspace ABI. If this is done *and* the i801
driver supports length > 32 bytes, too, then it would work natively. If
the i801 can do this, this is a question for Jean Delvare.

b) I don't know Elantech HW but there are devices out there which allow
configuration for the block size. Something like a bit specifying if
block transfers > 32 are allowed. Or the SMBus version to support. Block
transfers > 32 are SMBus 3.0+ only. If your HW does not have that,
disabling SMBus is an option, too. Disabling it in the i801 driver is
too much of a hammer, I'd say.

Hope this helps! Happy hacking,

   Wolfram


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: Need some help on "Input: elantech - add LEN2146 to SMBus blacklist for ThinkPad L13 Gen2"

[Nikolai Kostrigin]

Hi,

25.02.2021 12:38, Wolfram Sang пишет:
> Hi,
>
>> I had a preliminary discussion with Benjamin Tissoires and according to
>> our agreement I repost it for wider audience.
>> Blacklisting the device was decided to be a bad idea.
>> But actually I managed to get touchpad totally operational via SMBus
>> using a following hack:
>>
>> providing a parameter to i2c_i801 driver:
>>
>> modprobe i2c_i801 disable_features=0x2 (i.e. disable the block buffer).
> So, from an I2C perspective, there are two things to mention here:
>
> a) I am in the process of extending the I2C core to allow block
> transfers > 32 byte. This is a slow process, though, because we need to
> pay attention to not break userspace ABI. If this is done *and* the i801
> driver supports length > 32 bytes, too, then it would work natively. If
> the i801 can do this, this is a question for Jean Delvare.
>
> b) I don't know Elantech HW but there are devices out there which allow
> configuration for the block size. Something like a bit specifying if
> block transfers > 32 are allowed. Or the SMBus version to support. Block
> transfers > 32 are SMBus 3.0+ only. If your HW does not have that,
> disabling SMBus is an option, too. Disabling it in the i801 driver is
> too much of a hammer, I'd say.
>
> Hope this helps! Happy hacking,
>
>    Wolfram
Thank you for the information, Wolfram!

Finally it turned out that the solution was near me from the very
beginning, but I failed to check mainline code at that moment (which is
now 5.11).
Happily Jingle Wu has pointed me to a couple of  patches of his
(co-authored by Dmitry Torokhov):

https://git.kernel.org/pub/scm/linux/kernel/git/dtor/input.git/commit/?h=next&id=056115daede8d01f71732bc7d778fb85acee8eb6

https://git.kernel.org/pub/scm/linux/kernel/git/dtor/input.git/commit/?h=next&id=e4c9062717feda88900b566463228d1c4910af6d

I applied those to 5.10.17 and trackpoint works like a charm.
So I guess theese patches are worth being backported to the longterm
5.10 branch.
I'm really sorry for the noise.

-- 
Best regards,
Nikolai Kostrigin

Re: Need some help on "Input: elantech - add LEN2146 to SMBus blacklist for ThinkPad L13 Gen2"

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 193 bytes --]


> Happily Jingle Wu has pointed me to a couple of  patches of his
> (co-authored by Dmitry Torokhov):

Yep, this looks like the proper place to fix things. Good it is already
solved.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: Need some help on "Input: elantech - add LEN2146 to SMBus blacklist for ThinkPad L13 Gen2"

[Nikolai Kostrigin]

Hi,
resending this once again, hoping it wouldn't contain any HTML and
wouldn't be filtered by LKML.

03.03.2021 13:11, Nikolai Kostrigin пишет:
> Hi,
> 
> 25.02.2021 12:38, Wolfram Sang пишет:
>> Hi,
>>
>>> I had a preliminary discussion with Benjamin Tissoires and according to
>>> our agreement I repost it for wider audience.
>>> Blacklisting the device was decided to be a bad idea.
>>> But actually I managed to get touchpad totally operational via SMBus
>>> using a following hack:
>>>
>>> providing a parameter to i2c_i801 driver:
>>>
>>> modprobe i2c_i801 disable_features=0x2 (i.e. disable the block buffer).
>> So, from an I2C perspective, there are two things to mention here:
>>
>> a) I am in the process of extending the I2C core to allow block
>> transfers > 32 byte. This is a slow process, though, because we need to
>> pay attention to not break userspace ABI. If this is done *and* the i801
>> driver supports length > 32 bytes, too, then it would work natively. If
>> the i801 can do this, this is a question for Jean Delvare.
>>
>> b) I don't know Elantech HW but there are devices out there which allow
>> configuration for the block size. Something like a bit specifying if
>> block transfers > 32 are allowed. Or the SMBus version to support. Block
>> transfers > 32 are SMBus 3.0+ only. If your HW does not have that,
>> disabling SMBus is an option, too. Disabling it in the i801 driver is
>> too much of a hammer, I'd say.
>>
>> Hope this helps! Happy hacking,
>>
>>    Wolfram
> Thank you for the information, Wolfram!
> 
> Finally it turned out that the solution was near me from the very
> beginning, but I failed to check mainline code at that moment (which is
> now 5.11).
> Happily Jingle Wu has pointed me to a couple of  patches of his
> (co-authored by Dmitry Torokhov):
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/dtor/input.git/commit/?h=next&id=056115daede8d01f71732bc7d778fb85acee8eb6
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/dtor/input.git/commit/?h=next&id=e4c9062717feda88900b566463228d1c4910af6d
> 
> I applied those to 5.10.17 and trackpoint works like a charm.
> So I guess theese patches are worth being backported to the longterm
> 5.10 branch.
> I'm really sorry for the noise.
> 

-- 
Best regards,
Nikolai Kostrigin