diff mbox series

[2/2] i2c: core: APCI: Log device not acking errors at dbg loglevel

Message ID 20180422175800.6315-2-hdegoede@redhat.com
State Accepted
Headers show
Series [1/2] i2c: core: APCI: Improve OpRegion read errors | expand

Commit Message

Hans de Goede April 22, 2018, 5:58 p.m. UTC 
Unfortunately some DSDTs issue bogus i2c reads to non existing devices
resulting in -EREMOTEIO errors because the non existing device of course
does not ack.

This happens e.g. from the The Asus T100TA's _BIX method, the DSDT on
the T100TA defines 2 resources on the I2C1 bus:

        Name (EHID, ResourceTemplate ()
        {
            I2cSerialBusV2 (0x005B, ControllerInitiated, 0x00061A80,
                AddressingMode7Bit, "\\_SB.I2C1",
                0x00, ResourceConsumer, , Exclusive,
                )
        })
        OperationRegion (EHOR, GenericSerialBus, Zero, 0x0100)
        Field (EHOR, BufferAcc, NoLock, Preserve)
        {
            Connection (EHID),
            Offset (0x01),
            AccessAs (BufferAcc, AttribBytes (0x10)),
            ABCD,   8
        }

        Name (UMPC, ResourceTemplate ()
        {
            I2cSerialBusV2 (0x0066, ControllerInitiated, 0x00061A80,
                AddressingMode7Bit, "\\_SB.I2C1",
                0x00, ResourceConsumer, , Exclusive,
                )
        })

The _BIX method does a single read (on each BIX() call) from the EHID
device through the ABCD Field, only to completely ignore the result.
This read always fails as there is no i2c client at address 0x5b.

The _BIX method also does several reads from the UMPC device and actually
uses the results of those to provide battery information.

IIRC I've also seen some DSTDs which do an i2c read to detect if a device
is present, also leading to false positive errors being logged.

Esp. the _BIX use is problematic as the _BIX method gets called
periodically to monitor battery status.

This commit stops the logs from filling up with errors like these:

[   57.327858] i2c i2c-0: i2c read 16 bytes from client@0x5b starting at
               reg 0x1 failed, error: -121

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/i2c/i2c-core-acpi.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

Comments

Mika Westerberg April 23, 2018, 10:40 a.m. UTC | #1 
On Sun, Apr 22, 2018 at 07:58:00PM +0200, Hans de Goede wrote:
> Unfortunately some DSDTs issue bogus i2c reads to non existing devices
> resulting in -EREMOTEIO errors because the non existing device of course
> does not ack.
> 
> This happens e.g. from the The Asus T100TA's _BIX method, the DSDT on
> the T100TA defines 2 resources on the I2C1 bus:
> 
>         Name (EHID, ResourceTemplate ()
>         {
>             I2cSerialBusV2 (0x005B, ControllerInitiated, 0x00061A80,
>                 AddressingMode7Bit, "\\_SB.I2C1",
>                 0x00, ResourceConsumer, , Exclusive,
>                 )
>         })
>         OperationRegion (EHOR, GenericSerialBus, Zero, 0x0100)
>         Field (EHOR, BufferAcc, NoLock, Preserve)
>         {
>             Connection (EHID),
>             Offset (0x01),
>             AccessAs (BufferAcc, AttribBytes (0x10)),
>             ABCD,   8
>         }
> 
>         Name (UMPC, ResourceTemplate ()
>         {
>             I2cSerialBusV2 (0x0066, ControllerInitiated, 0x00061A80,
>                 AddressingMode7Bit, "\\_SB.I2C1",
>                 0x00, ResourceConsumer, , Exclusive,
>                 )
>         })
> 
> The _BIX method does a single read (on each BIX() call) from the EHID
> device through the ABCD Field, only to completely ignore the result.
> This read always fails as there is no i2c client at address 0x5b.
> 
> The _BIX method also does several reads from the UMPC device and actually
> uses the results of those to provide battery information.
> 
> IIRC I've also seen some DSTDs which do an i2c read to detect if a device
> is present, also leading to false positive errors being logged.
> 
> Esp. the _BIX use is problematic as the _BIX method gets called
> periodically to monitor battery status.
> 
> This commit stops the logs from filling up with errors like these:
> 
> [   57.327858] i2c i2c-0: i2c read 16 bytes from client@0x5b starting at
>                reg 0x1 failed, error: -121
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>

Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>
Wolfram Sang April 28, 2018, 12:46 p.m. UTC | #2 
> This commit stops the logs from filling up with errors like these:
> 
> [   57.327858] i2c i2c-0: i2c read 16 bytes from client@0x5b starting at
>                reg 0x1 failed, error: -121

This makes it probably 4.17 material?
Hans de Goede April 28, 2018, 6:17 p.m. UTC | #3 
Hi,

On 28-04-18 14:46, Wolfram Sang wrote:
> 
>> This commit stops the logs from filling up with errors like these:
>>
>> [   57.327858] i2c i2c-0: i2c read 16 bytes from client@0x5b starting at
>>                 reg 0x1 failed, error: -121
> 
> This makes it probably 4.17 material?

Those errors have been happening on devices with buggy DSTDs for
years already, so there is no real rush, but I don't see much
of a regression chance either, so getting these 2 patches into 4.17
would be nice. Your choice.

Regards,

Hans
Wolfram Sang April 30, 2018, 8:49 a.m. UTC | #4 
On Sun, Apr 22, 2018 at 07:58:00PM +0200, Hans de Goede wrote:
> Unfortunately some DSDTs issue bogus i2c reads to non existing devices
> resulting in -EREMOTEIO errors because the non existing device of course
> does not ack.
> 
> This happens e.g. from the The Asus T100TA's _BIX method, the DSDT on
> the T100TA defines 2 resources on the I2C1 bus:
> 
>         Name (EHID, ResourceTemplate ()
>         {
>             I2cSerialBusV2 (0x005B, ControllerInitiated, 0x00061A80,
>                 AddressingMode7Bit, "\\_SB.I2C1",
>                 0x00, ResourceConsumer, , Exclusive,
>                 )
>         })
>         OperationRegion (EHOR, GenericSerialBus, Zero, 0x0100)
>         Field (EHOR, BufferAcc, NoLock, Preserve)
>         {
>             Connection (EHID),
>             Offset (0x01),
>             AccessAs (BufferAcc, AttribBytes (0x10)),
>             ABCD,   8
>         }
> 
>         Name (UMPC, ResourceTemplate ()
>         {
>             I2cSerialBusV2 (0x0066, ControllerInitiated, 0x00061A80,
>                 AddressingMode7Bit, "\\_SB.I2C1",
>                 0x00, ResourceConsumer, , Exclusive,
>                 )
>         })
> 
> The _BIX method does a single read (on each BIX() call) from the EHID
> device through the ABCD Field, only to completely ignore the result.
> This read always fails as there is no i2c client at address 0x5b.
> 
> The _BIX method also does several reads from the UMPC device and actually
> uses the results of those to provide battery information.
> 
> IIRC I've also seen some DSTDs which do an i2c read to detect if a device
> is present, also leading to false positive errors being logged.
> 
> Esp. the _BIX use is problematic as the _BIX method gets called
> periodically to monitor battery status.
> 
> This commit stops the logs from filling up with errors like these:
> 
> [   57.327858] i2c i2c-0: i2c read 16 bytes from client@0x5b starting at
>                reg 0x1 failed, error: -121
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>

Fixed the APCI typo in subject ;) and applied to for-current, thanks!
diff mbox series

Patch

diff --git a/drivers/i2c/i2c-core-acpi.c b/drivers/i2c/i2c-core-acpi.c
index 3dc43a009f5d..7c3b4740b94b 100644
--- a/drivers/i2c/i2c-core-acpi.c
+++ b/drivers/i2c/i2c-core-acpi.c
@@ -445,11 +445,17 @@  static int acpi_gsb_i2c_read_bytes(struct i2c_client *client,
 	msgs[1].buf = buffer;
 
 	ret = i2c_transfer(client->adapter, msgs, ARRAY_SIZE(msgs));
-	if (ret < 0)
-		dev_err(&client->adapter->dev, "i2c read %d bytes from client@%#x starting at reg %#x failed, error: %d\n",
-			data_len, client->addr, cmd, ret);
-	else
+	if (ret < 0) {
+		/* Getting a NACK is unfortunately normal with some DSTDs */
+		if (ret == -EREMOTEIO)
+			dev_dbg(&client->adapter->dev, "i2c read %d bytes from client@%#x starting at reg %#x failed, error: %d\n",
+				data_len, client->addr, cmd, ret);
+		else
+			dev_err(&client->adapter->dev, "i2c read %d bytes from client@%#x starting at reg %#x failed, error: %d\n",
+				data_len, client->addr, cmd, ret);
+	} else {
 		memcpy(data, buffer, data_len);
+	}
 
 	kfree(buffer);
 	return ret;