| 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: <20251103104330.GG2912318@black.igk.intel.com>
Date: Mon, 3 Nov 2025 11:43:30 +0100
From: Mika Westerberg <mika.westerberg@...ux.intel.com>
To: Benoît Monin <benoit.monin@...tlin.com>
Cc: Andi Shyti <andi.shyti@...nel.org>, Rob Herring <robh@...nel.org>,
Krzysztof Kozlowski <krzk+dt@...nel.org>,
Conor Dooley <conor+dt@...nel.org>,
Jarkko Nikula <jarkko.nikula@...ux.intel.com>,
Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
Jan Dabros <jsd@...ihalf.com>,
Sebastian Andrzej Siewior <bigeasy@...utronix.de>,
Clark Williams <clrkwllms@...nel.org>,
Steven Rostedt <rostedt@...dmis.org>,
Thomas Petazzoni <thomas.petazzoni@...tlin.com>,
Gregory CLEMENT <gregory.clement@...tlin.com>,
Théo Lebrun <theo.lebrun@...tlin.com>,
Tawfik Bayouk <tawfik.bayouk@...ileye.com>,
Vladimir Kondratiev <vladimir.kondratiev@...ileye.com>,
Dmitry Guzman <dmitry.guzman@...ileye.com>,
linux-i2c@...r.kernel.org, devicetree@...r.kernel.org,
linux-kernel@...r.kernel.org, linux-rt-devel@...ts.linux.dev
Subject: Re: [PATCH v2 5/5] i2c: designware: Support of controller with
IC_EMPTYFIFO_HOLD_MASTER disabled
On Fri, Oct 31, 2025 at 03:35:43PM +0100, Benoît Monin wrote:
> If IC_EMPTYFIFO_HOLD_MASTER_EN parameter is 0, "Stop" and "Repeated Start"
> bits in command register does not exist, thus it is impossible to send
> several consecutive write messages in a single hardware batch. The
> existing implementation worked with such configuration incorrectly:
> all consecutive write messages are joined into a single message without
> any Start/Stop or Repeated Start conditions. For example, the following
> command:
>
> i2ctransfer -y 0 w1@...5 0x00 w1@...5 0x01
>
> does the same as
>
> i2ctransfer -y 0 w2@...5 0x00 0x01
>
> In i2c_dw_xfer(), we ensure that we do not have such sequence of messages
> requiring a RESTART, aborting the transfer on controller that cannot
> emit them explicitly.
>
> This behavior is activated by compatible entries because the state of
> the IC_EMPTYFIFO_HOLD_MASTER_EN parameter cannot be detected at runtime.
> Add the compatible entry for Mobileye SoCs needing the workaround.
>
> There is another possible problem with this controller configuration:
> When the CPU is putting commands to the FIFO, this process must not be
> interrupted because if FIFO buffer gets empty, the controller finishes
> the I2C transaction and generates STOP condition on the bus.
>
> If we continue writing the remainder of the message to the FIFO, the
> controller will start emitting a new transaction with those data. This
> turns a single a single message into multiple I2C transactions. To
> ensure that we do not keep processing a message after a FIFO underrun,
> checks are added in two places.
>
> First in i2c_dw_xfer_msg() we check the raw interrupt status register to
> see if a STOP condition was detected while filling the FIFO, and abort
> if so. This can happen with threaded interrupt on a PREEMPT_RT kernel
> if we are preempted during the processing of each bytes of the message.
>
> Second in i2c_dw_process_transfer(), we abort if a STOP is detected
> while a read or a write is in progress. This can occur when processing
> a message larger than the FIFO. In that case the message is processed in
> parts, and rely on the TW EMPTY interrupt to refill the FIFO when it gets
> below a threshold. If servicing this interrupt is delayed for too long,
> it can trigger a FIFO underrun, thus an unwanted STOP.
>
> Originally-by: Dmitry Guzman <dmitry.guzman@...ileye.com>
> Signed-off-by: Benoît Monin <benoit.monin@...tlin.com>
> ---
> drivers/i2c/busses/i2c-designware-core.h | 1 +
> drivers/i2c/busses/i2c-designware-master.c | 51 +++++++++++++++++++++++++++++
> drivers/i2c/busses/i2c-designware-platdrv.c | 1 +
> 3 files changed, 53 insertions(+)
>
> diff --git a/drivers/i2c/busses/i2c-designware-core.h b/drivers/i2c/busses/i2c-designware-core.h
> index 347843b4f5dd..a31a8698e511 100644
> --- a/drivers/i2c/busses/i2c-designware-core.h
> +++ b/drivers/i2c/busses/i2c-designware-core.h
> @@ -311,6 +311,7 @@ struct dw_i2c_dev {
> #define ACCESS_NO_IRQ_SUSPEND BIT(1)
> #define ARBITRATION_SEMAPHORE BIT(2)
> #define ACCESS_POLLING BIT(3)
> +#define NO_EMPTYFIFO_HOLD_MASTER BIT(4)
>
> #define MODEL_MSCC_OCELOT BIT(8)
> #define MODEL_BAIKAL_BT1 BIT(9)
> diff --git a/drivers/i2c/busses/i2c-designware-master.c b/drivers/i2c/busses/i2c-designware-master.c
> index da1963d25def..329bb69485f4 100644
> --- a/drivers/i2c/busses/i2c-designware-master.c
> +++ b/drivers/i2c/busses/i2c-designware-master.c
> @@ -463,6 +463,7 @@ i2c_dw_xfer_msg(struct dw_i2c_dev *dev)
> rx_limit = dev->rx_fifo_depth - flr;
>
> while (buf_len > 0 && tx_limit > 0 && rx_limit > 0) {
> + unsigned int raw_stat;
> u32 cmd = 0;
>
> /*
> @@ -487,6 +488,21 @@ i2c_dw_xfer_msg(struct dw_i2c_dev *dev)
> need_restart = false;
> }
>
> + /*
> + * With threaded interrupt on a PREEMPT-RT kernel, we may
> + * be interrupted while filling the FIFO. Abort the
> + * transfer in case of a FIFO underrun on controller that
> + * emits a STOP in that case.
> + */
> + if (dev->flags & NO_EMPTYFIFO_HOLD_MASTER) {
> + regmap_read(dev->map, DW_IC_RAW_INTR_STAT,
> + &raw_stat);
> + if (raw_stat & DW_IC_INTR_STOP_DET) {
> + dev->msg_err = -EIO;
> + goto done;
> + }
> + }
> +
> if (msgs[dev->msg_write_idx].flags & I2C_M_RD) {
>
> /* Avoid rx buffer overrun */
> @@ -526,6 +542,7 @@ i2c_dw_xfer_msg(struct dw_i2c_dev *dev)
> dev->status &= ~STATUS_WRITE_IN_PROGRESS;
> }
>
> +done:
> /*
> * If i2c_msg index search is completed, we don't need TX_EMPTY
> * interrupt any more.
> @@ -706,6 +723,14 @@ static void i2c_dw_process_transfer(struct dw_i2c_dev *dev, unsigned int stat)
> if (stat & DW_IC_INTR_TX_EMPTY)
> i2c_dw_xfer_msg(dev);
>
> + /* Abort if we detect a STOP in the middle of a read or a write */
> + if ((stat & DW_IC_INTR_STOP_DET) &&
> + (dev->status & (STATUS_READ_IN_PROGRESS | STATUS_WRITE_IN_PROGRESS))) {
> + dev_err(dev->dev, "spurious STOP detected\n");
> + dev->rx_outstanding = 0;
> + dev->msg_err = -EIO;
> + }
> +
> /*
> * No need to modify or disable the interrupt mask here.
> * i2c_dw_xfer_msg() will take care of it according to
> @@ -872,6 +897,21 @@ __i2c_dw_xfer_unlocked(struct dw_i2c_dev *dev, struct i2c_msg msgs[], int num)
> return ret;
> }
>
> +/*
> + * Return true if the message needs an explicit RESTART before being sent.
> + * Without an explicit RESTART, two consecutive messages in the same direction
> + * will be merged into a single transfer.
> + * The adapter always emits a RESTART when the direction changes.
> + */
> +static inline bool i2c_dw_msg_need_restart(struct i2c_msg msgs[], int idx)
This can take const parameters.
> +{
Please move the dev->flags & NO_EMPTYFIFO_HOLD_MASTER here too.
> + /* No need for a RESTART on the first message */
> + if (idx == 0)
> + return false;
That's
if (!idx)
But why not pass the actual message instead of the index?
> +
> + return (msgs[idx - 1].flags & I2C_M_RD) == (msgs[idx].flags & I2C_M_RD);
You don't need the outer parens.
> +}
> +
> static int
> i2c_dw_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
> {
> @@ -918,6 +958,17 @@ i2c_dw_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
> goto done;
> }
>
> + /*
> + * Make sure we don't need explicit RESTART for
> + * controllers that cannot emit them.
> + */
> + if (dev->flags & NO_EMPTYFIFO_HOLD_MASTER &&
> + i2c_dw_msg_need_restart(msg, cnt - 1)) {
> + dev_err(dev->dev, "cannot emit RESTART\n");
> + ret = -EINVAL;
> + goto done;
> + }
> +
> if ((msg[cnt - 1].flags & I2C_M_STOP) ||
> (msg + cnt == msgs + num))
> break;
> diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
> index d7d764f7554d..4aad3dc51fbc 100644
> --- a/drivers/i2c/busses/i2c-designware-platdrv.c
> +++ b/drivers/i2c/busses/i2c-designware-platdrv.c
> @@ -346,6 +346,7 @@ static void dw_i2c_plat_remove(struct platform_device *pdev)
>
> static const struct of_device_id dw_i2c_of_match[] = {
> { .compatible = "baikal,bt1-sys-i2c", .data = (void *)MODEL_BAIKAL_BT1 },
> + { .compatible = "mobileye,eyeq6lplus-i2c", .data = (void *)NO_EMPTYFIFO_HOLD_MASTER },
> { .compatible = "mscc,ocelot-i2c", .data = (void *)MODEL_MSCC_OCELOT },
> { .compatible = "snps,designware-i2c" },
> {}
>
> --
> 2.51.1
Powered by blists - more mailing lists