| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <Z_6TfD-jzdLPtpYN@blossom>
Date: Tue, 15 Apr 2025 13:12:28 -0400
From: Alyssa Rosenzweig <alyssa@...enzweig.io>
To: sven@...npeter.dev
Cc: Janne Grunau <j@...nau.net>, Madhavan Srinivasan <maddy@...ux.ibm.com>,
Michael Ellerman <mpe@...erman.id.au>,
Nicholas Piggin <npiggin@...il.com>,
Christophe Leroy <christophe.leroy@...roup.eu>,
Naveen N Rao <naveen@...nel.org>,
Andi Shyti <andi.shyti@...nel.org>, Neal Gompa <neal@...pa.dev>,
Hector Martin <marcan@...can.st>, linuxppc-dev@...ts.ozlabs.org,
asahi@...ts.linux.dev, linux-arm-kernel@...ts.infradead.org,
linux-i2c@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 4/6] i2c: pasemi: Improve error recovery
Reviewed-by: Alyssa Rosenzweig <alyssa@...enzweig.io>
Le Tue , Apr 15, 2025 at 03:36:58PM +0000, Sven Peter via B4 Relay a écrit :
> From: Hector Martin <marcan@...can.st>
>
> The hardware (supposedly) has a 25ms timeout for clock stretching
> and the driver uses 100ms which should be plenty. The error
> reocvery itself is however lacking.
>
> Add handling for all the missing error condition, and better recovery in
> pasemi_smb_clear(). Also move the timeout to a #define since it's used
> in multiple places now.
>
> Signed-off-by: Hector Martin <marcan@...can.st>
> [sven: adjusted commit message after splitting the commit into two]
> Signed-off-by: Sven Peter <sven@...npeter.dev>
> ---
> drivers/i2c/busses/i2c-pasemi-core.c | 75 +++++++++++++++++++++++++++++++-----
> 1 file changed, 65 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-pasemi-core.c b/drivers/i2c/busses/i2c-pasemi-core.c
> index 9b611dbdfef23e78a4ea75ac0311938d52b6ba96..2f31f039bedfd7f78d6572fe925125b1a6d0724b 100644
> --- a/drivers/i2c/busses/i2c-pasemi-core.c
> +++ b/drivers/i2c/busses/i2c-pasemi-core.c
> @@ -9,6 +9,7 @@
> #include <linux/delay.h>
> #include <linux/i2c.h>
> #include <linux/io.h>
> +#include <linux/iopoll.h>
> #include <linux/kernel.h>
> #include <linux/module.h>
> #include <linux/pci.h>
> @@ -52,6 +53,12 @@
> #define CTL_UJM BIT(8)
> #define CTL_CLK_M GENMASK(7, 0)
>
> +/*
> + * The hardware (supposedly) has a 25ms timeout for clock stretching, thus
> + * use 100ms here which should be plenty.
> + */
> +#define TRANSFER_TIMEOUT_MS 100
> +
> static inline void reg_write(struct pasemi_smbus *smbus, int reg, int val)
> {
> dev_dbg(smbus->dev, "smbus write reg %x val %08x\n", reg, val);
> @@ -80,24 +87,44 @@ static void pasemi_reset(struct pasemi_smbus *smbus)
> reinit_completion(&smbus->irq_completion);
> }
>
> -static void pasemi_smb_clear(struct pasemi_smbus *smbus)
> +static int pasemi_smb_clear(struct pasemi_smbus *smbus)
> {
> unsigned int status;
> + int ret;
>
> - status = reg_read(smbus, REG_SMSTA);
> + /* First wait for the bus to go idle */
> + ret = readx_poll_timeout(ioread32, smbus->ioaddr + REG_SMSTA,
> + status, !(status & (SMSTA_XIP | SMSTA_JAM)),
> + USEC_PER_MSEC, USEC_PER_MSEC * TRANSFER_TIMEOUT_MS);
> +
> + if (ret < 0) {
> + dev_err(smbus->dev, "Bus is still stuck (status 0x%08x)\n", status);
> + return -EIO;
> + }
> +
> + /* If any badness happened or there is data in the FIFOs, reset the FIFOs */
> + if ((status & (SMSTA_MRNE | SMSTA_JMD | SMSTA_MTO | SMSTA_TOM | SMSTA_MTN | SMSTA_MTA)) ||
> + !(status & SMSTA_MTE))
> + pasemi_reset(smbus);
> +
> + /* Clear the flags */
> reg_write(smbus, REG_SMSTA, status);
> +
> + return 0;
> }
>
> static int pasemi_smb_waitready(struct pasemi_smbus *smbus)
> {
> - int timeout = 100;
> + int timeout = TRANSFER_TIMEOUT_MS;
> int ret;
> unsigned int status;
>
> if (smbus->use_irq) {
> reinit_completion(&smbus->irq_completion);
> - reg_write(smbus, REG_IMASK, SMSTA_XEN | SMSTA_MTN);
> - ret = wait_for_completion_timeout(&smbus->irq_completion, msecs_to_jiffies(100));
> + /* XEN should be set when a transaction terminates, whether due to error or not */
> + reg_write(smbus, REG_IMASK, SMSTA_XEN);
> + ret = wait_for_completion_timeout(&smbus->irq_completion,
> + msecs_to_jiffies(timeout));
> reg_write(smbus, REG_IMASK, 0);
> status = reg_read(smbus, REG_SMSTA);
>
> @@ -123,9 +150,35 @@ static int pasemi_smb_waitready(struct pasemi_smbus *smbus)
> }
> }
>
> + /* Controller timeout? */
> + if (status & SMSTA_TOM) {
> + dev_warn(smbus->dev, "Controller timeout, status 0x%08x\n", status);
> + return -EIO;
> + }
> +
> + /* Peripheral timeout? */
> + if (status & SMSTA_MTO) {
> + dev_warn(smbus->dev, "Peripheral timeout, status 0x%08x\n", status);
> + return -ETIME;
> + }
> +
> + /* Still stuck in a transaction? */
> + if (status & SMSTA_XIP) {
> + dev_warn(smbus->dev, "Bus stuck, status 0x%08x\n", status);
> + return -EIO;
> + }
> +
> + /* Arbitration loss? */
> + if (status & SMSTA_MTA) {
> + dev_warn(smbus->dev, "Arbitration loss, status 0x%08x\n", status);
> + return -EBUSY;
> + }
> +
> /* Got NACK? */
> - if (status & SMSTA_MTN)
> + if (status & SMSTA_MTN) {
> + dev_warn(smbus->dev, "NACK, status 0x%08x\n", status);
> return -ENXIO;
> + }
>
> /* Clear XEN */
> reg_write(smbus, REG_SMSTA, SMSTA_XEN);
> @@ -187,9 +240,9 @@ static int pasemi_i2c_xfer(struct i2c_adapter *adapter,
> struct pasemi_smbus *smbus = adapter->algo_data;
> int ret, i;
>
> - pasemi_smb_clear(smbus);
> -
> - ret = 0;
> + ret = pasemi_smb_clear(smbus);
> + if (ret)
> + return ret;
>
> for (i = 0; i < num && !ret; i++)
> ret = pasemi_i2c_xfer_msg(adapter, &msgs[i], (i == (num - 1)));
> @@ -210,7 +263,9 @@ static int pasemi_smb_xfer(struct i2c_adapter *adapter,
> addr <<= 1;
> read_flag = read_write == I2C_SMBUS_READ;
>
> - pasemi_smb_clear(smbus);
> + err = pasemi_smb_clear(smbus);
> + if (err)
> + return err;
>
> switch (size) {
> case I2C_SMBUS_QUICK:
>
> --
> 2.34.1
>
>
Powered by blists - more mailing lists