| 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: <20190123014042.6d64ee29@dimatab>
Date: Wed, 23 Jan 2019 01:40:42 +0300
From: Dmitry Osipenko <digetx@...il.com>
To: Sowjanya Komatineni <skomatineni@...dia.com>
Cc: "thierry.reding@...il.com" <thierry.reding@...il.com>,
Jonathan Hunter <jonathanh@...dia.com>,
Mantravadi Karthik <mkarthik@...dia.com>,
Shardar Mohammed <smohammed@...dia.com>,
Timo Alho <talho@...dia.com>,
"linux-tegra@...r.kernel.org" <linux-tegra@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"linux-i2c@...r.kernel.org" <linux-i2c@...r.kernel.org>
Subject: Re: [PATCH V3] i2c: tegra: Add Bus Clear Master Support
В Tue, 22 Jan 2019 22:13:53 +0000
Sowjanya Komatineni <skomatineni@...dia.com> пишет:
> >> Bus clear feature of tegra i2c controller helps to recover from
> >> bus hang when i2c master loses the bus arbitration due to the
> >> slave device holding SDA LOW continuously for some unknown reasons.
> >>
> >> Per I2C specification, the device that held the bus LOW should
> >> release it within 9 clock pulses.
> >>
> >> During bus clear operation, Tegra I2C controller sends 9 clock
> >> pulses and terminates the transaction with STOP condition.
> >> Upon successful bus clear operation, bus goes to idle state and
> >> driver retries the transaction.
> >>
> >> Signed-off-by: Sowjanya Komatineni <skomatineni@...dia.com>
> >> ---
> >> [V3]: Updated comments and commit message to be clear on the
> >> change [V2]: Same as V1 rebased to 5.0-rc1
> >>
> >> drivers/i2c/busses/i2c-tegra.c | 70
> >> ++++++++++++++++++++++++++++++++++++++++++
> >> 1 file changed, 70 insertions(+)
> >>
> >> diff --git a/drivers/i2c/busses/i2c-tegra.c
> >> b/drivers/i2c/busses/i2c-tegra.c index e417ebf7628c..b1b920b4a203
> >> 100644
> >> --- a/drivers/i2c/busses/i2c-tegra.c
> >> +++ b/drivers/i2c/busses/i2c-tegra.c
> >> @@ -54,6 +54,7 @@
> >> #define I2C_FIFO_STATUS_RX_SHIFT 0
> >> #define I2C_INT_MASK 0x064
> >> #define I2C_INT_STATUS 0x068
> >> +#define I2C_INT_BUS_CLR_DONE BIT(11)
> >> #define I2C_INT_PACKET_XFER_COMPLETE BIT(7)
> >> #define I2C_INT_ALL_PACKETS_XFER_COMPLETE BIT(6)
> >> #define I2C_INT_TX_FIFO_OVERFLOW BIT(5)
> >> @@ -96,6 +97,15 @@
> >> #define I2C_HEADER_MASTER_ADDR_SHIFT 12
> >> #define I2C_HEADER_SLAVE_ADDR_SHIFT 1
> >>
> >> +#define I2C_BUS_CLEAR_CNFG 0x084
> >> +#define I2C_BC_SCLK_THRESHOLD 9
> >> +#define I2C_BC_SCLK_THRESHOLD_SHIFT 16
> >> +#define I2C_BC_STOP_COND BIT(2)
> >> +#define I2C_BC_TERMINATE BIT(1)
> >> +#define I2C_BC_ENABLE BIT(0)
> >> +#define I2C_BUS_CLEAR_STATUS 0x088
> >> +#define I2C_BC_STATUS BIT(0)
> >> +
> >> #define I2C_CONFIG_LOAD 0x08C
> >> #define I2C_MSTR_CONFIG_LOAD BIT(0)
> >> #define I2C_SLV_CONFIG_LOAD BIT(1)
> >> @@ -155,6 +165,8 @@ enum msg_end_type {
> >> * @has_mst_fifo: The I2C controller contains the new MST FIFO
> >> interface that
> >> * provides additional features and allows for
> >> longer messages to
> >> * be transferred in one go.
> >> + * @supports_bus_clear: Bus Clear support to recover from bus
> >> hang during
> >> + * SDA stuck low from device for some unknown
> >> reasons. */
> >> struct tegra_i2c_hw_feature {
> >> bool has_continue_xfer_support;
> >> @@ -167,6 +179,7 @@ struct tegra_i2c_hw_feature {
> >> bool has_multi_master_mode;
> >> bool has_slcg_override_reg;
> >> bool has_mst_fifo;
> >> + bool supports_bus_clear;
> >> };
> >>
> >> /**
> >> @@ -639,6 +652,12 @@ static irqreturn_t tegra_i2c_isr(int irq,
> >> void *dev_id) i2c_dev->msg_err |= I2C_ERR_ARBITRATION_LOST;
> >> goto err;
> >> }
> >> + /*
> >> + * I2C transfer is terminated during the bus clear so skip
> >> + * processing the other interrupts.
> >> + */
> >> + if (i2c_dev->hw->supports_bus_clear && (status &
> >> I2C_INT_BUS_CLR_DONE))
> >> + goto err;
> >>
> >> if (i2c_dev->msg_read && (status &
> >> I2C_INT_RX_FIFO_DATA_REQ)) { if (i2c_dev->msg_buf_remaining)
> >> @@ -668,6 +687,8 @@ static irqreturn_t tegra_i2c_isr(int irq, void
> >> *dev_id) tegra_i2c_mask_irq(i2c_dev, I2C_INT_NO_ACK |
> >> I2C_INT_ARBITRATION_LOST | I2C_INT_PACKET_XFER_COMPLETE |
> >> I2C_INT_TX_FIFO_DATA_REQ | I2C_INT_RX_FIFO_DATA_REQ);
> >> + if (i2c_dev->hw->supports_bus_clear)
> >> + tegra_i2c_mask_irq(i2c_dev, I2C_INT_BUS_CLR_DONE);
> >> i2c_writel(i2c_dev, status, I2C_INT_STATUS);
> >> if (i2c_dev->is_dvc)
> >> dvc_writel(i2c_dev, DVC_STATUS_I2C_DONE_INTR,
> >> DVC_STATUS); @@ -678,6 +699,43 @@ static irqreturn_t
> >> tegra_i2c_isr(int irq, void *dev_id) return IRQ_HANDLED;
> >> }
> >>
> >> +static int tegra_i2c_issue_bus_clear(struct tegra_i2c_dev
> >> *i2c_dev) {
> >> + int err;
> >> + unsigned long time_left;
> >> + u32 reg;
> >> +
> >> + if (i2c_dev->hw->supports_bus_clear) {
> >> + reinit_completion(&i2c_dev->msg_complete);
> >> + reg = (I2C_BC_SCLK_THRESHOLD <<
> >> I2C_BC_SCLK_THRESHOLD_SHIFT) |
> >> + I2C_BC_STOP_COND | I2C_BC_TERMINATE;
> >> + i2c_writel(i2c_dev, reg, I2C_BUS_CLEAR_CNFG);
> >> + if (i2c_dev->hw->has_config_load_reg) {
> >> + err =
> >> tegra_i2c_wait_for_config_load(i2c_dev);
> >> + if (err)
> >> + return err;
> >> + }
> >> + reg |= I2C_BC_ENABLE;
> >> + i2c_writel(i2c_dev, reg, I2C_BUS_CLEAR_CNFG);
> >> + tegra_i2c_unmask_irq(i2c_dev,
> >> I2C_INT_BUS_CLR_DONE); +
> >> + time_left =
> >> wait_for_completion_timeout(&i2c_dev->msg_complete,
> >> +
> >> TEGRA_I2C_TIMEOUT);
> >> + if (time_left == 0) {
> >> + dev_err(i2c_dev->dev, "timed out for bus
> >> clear\n");
> >> + return -ETIMEDOUT;
> >> + }
> >> + reg = i2c_readl(i2c_dev, I2C_BUS_CLEAR_STATUS);
> >> + if (!(reg & I2C_BC_STATUS)) {
> >> + dev_err(i2c_dev->dev,
> >> + "Un-recovered arbitration
> >> lost\n");
> >> + return -EIO;
> >> + }
> >> + }
> >> +
> >> + return -EAGAIN;
> >> +}
> >> +
> >> static int tegra_i2c_xfer_msg(struct tegra_i2c_dev *i2c_dev,
> >> struct i2c_msg *msg, enum msg_end_type end_state) { @@
> >> -759,6 +817,12 @@ static int tegra_i2c_xfer_msg(struct
> >> tegra_i2c_dev *i2c_dev, return 0;
> >>
> >> tegra_i2c_init(i2c_dev);
> >> + /* start recovery upon arbitration loss in single master
> >> mode */
> >> + if (i2c_dev->msg_err == I2C_ERR_ARBITRATION_LOST) {
> >> + if (!i2c_dev->is_multimaster_mode)
> >> + return tegra_i2c_issue_bus_clear(i2c_dev);
> >> + return -EAGAIN;
> >
> >This changes the returned errno from -EIO to -EAGAIN for the
> >supports_bus_clear=false case, is it okay and intentional?
> >
>
> Yes EAGAIN is intentional to allow for transfer retry.
> During single master mode, ARBITRATION LOST notification happens when
> 1. I2C Master sees the bus is occupied by some other device when a
> transfer is initiated 2. I2C Master lost the bus during arbitration
> incase if slave device pulls SDA line low continuously for some
> unknown reason If arbitration lost is due to cause 1, retry helps to
> continue with transfer once bus is released by the slave and it just
> added delay in communication due to bus release delay by slave. In
> case of 2nd cause, retry never succeeds in cases where bus clear is
> not supported.
It's unclear whether the "never succeeds retry" may fail with the
EAGAIN, causing an endless retry-loop. Could you please clarify this
moment?
Powered by blists - more mailing lists