| 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: <20251021154036.GR2912318@black.igk.intel.com>
Date: Tue, 21 Oct 2025 17:40:36 +0200
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 3/3] i2c: designware: Support of controller with
IC_EMPTYFIFO_HOLD_MASTER disabled
Hi,
On Fri, Oct 17, 2025 at 04:59:34PM +0200, Benoît Monin wrote:
> If IC_EMPTYFIFO_HOLD_MASTER_EN parameter is 0, "Stop" and "Repeated
> Start" bits in command register doesn't 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
> consequent write messages 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
>
> To fix it, for the controllers that behave this way, if the next message
> to the same slave device has the same direction as the previous one, it
> is sent to the controller only after the previous message is sent and
> STOP_DET IRQ flag is raised by the controller.
>
> 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 entries of Mobileye SoCs needing the work-around and
> sort the entries alphabetically.
>
> 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.
>
> In a PREEMPT-RT kernel, interrupt handlers are by default executed in
> thread and may be interrupted, which can lead to breaking an I2C message
> by inserting an unwanted STOP.
>
> To ensure proper operation on realtime kernel, use IRQF_NO_THREAD flag
> when requesting IRQ.
But even with that, it is still possible that something else is running
with local interrupt disabled so this may still happen, although likelyhood
is smaller.
You could use ACCESS_POLLING but that too can be preempted.
Perhaps best is to fail the transfer and let the caller retry?
> Based on the work of 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 | 45 +++++++++++++++++++++--------
> drivers/i2c/busses/i2c-designware-platdrv.c | 6 ++--
> 3 files changed, 38 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-designware-core.h b/drivers/i2c/busses/i2c-designware-core.h
> index 347843b4f5dd7..a31a8698e511a 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 f9a180b145da8..e5af0439ec832 100644
> --- a/drivers/i2c/busses/i2c-designware-master.c
> +++ b/drivers/i2c/busses/i2c-designware-master.c
> @@ -443,18 +443,6 @@ i2c_dw_xfer_msg(struct dw_i2c_dev *dev)
> for (; dev->msg_write_idx < dev->msgs_num; dev->msg_write_idx++) {
> u32 flags = msgs[dev->msg_write_idx].flags;
>
> - /*
> - * If target address has changed, we need to
> - * reprogram the target address in the I2C
> - * adapter when we are done with this transfer.
> - * This can be done after STOP_DET IRQ flag is raised.
> - * So, disable "TX FIFO empty" interrupt.
> - */
> - if (msgs[dev->msg_write_idx].addr != addr) {
> - intr_mask &= ~DW_IC_INTR_TX_EMPTY;
> - break;
> - }
> -
> if (!(dev->status & STATUS_WRITE_IN_PROGRESS)) {
> /* new i2c_msg */
> buf = msgs[dev->msg_write_idx].buf;
> @@ -470,6 +458,25 @@ i2c_dw_xfer_msg(struct dw_i2c_dev *dev)
> need_restart = true;
> }
>
> + /*
> + * If target address has changed, we need to
> + * reprogram the target address in the I2C
> + * adapter when we are done with this transfer.
> + * This can be done after STOP_DET IRQ flag is raised.
> + * So, disable "TX FIFO empty" interrupt.
> + * Also force a stop-then-start between two messages
> + * in the same direction if we need to restart on a
> + * adapter that does not handle restart.
> + */
> + if (msgs[dev->msg_write_idx].addr != addr ||
> + ((need_restart &&
> + dev->flags & NO_EMPTYFIFO_HOLD_MASTER &&
> + ((msgs[dev->msg_write_idx].flags & I2C_M_RD) ==
> + (msgs[dev->msg_write_idx - 1].flags & I2C_M_RD))))) {
> + intr_mask &= ~DW_IC_INTR_TX_EMPTY;
> + break;
This, if we want to add it even, needs a helper function. The above is
really hard to parse ;-)
> + }
> +
> regmap_read(dev->map, DW_IC_TXFLR, &flr);
> tx_limit = dev->tx_fifo_depth - flr;
>
> @@ -1062,6 +1069,20 @@ int i2c_dw_probe_master(struct dw_i2c_dev *dev)
> irq_flags = IRQF_SHARED | IRQF_COND_SUSPEND;
> }
>
> + /*
> + * The first writing to TX FIFO buffer causes transmission start. If
> + * IC_EMPTYFIFO_HOLD_MASTER_EN is not set, when TX FIFO gets empty, I2C
> + * controller finishes the transaction. If writing to FIFO is
> + * interrupted, FIFO can get empty and the transaction will be finished
> + * prematurely. FIFO buffer is filled in IRQ handler, but in PREEMPT_RT
> + * kernel IRQ handler by default is executed in thread that can be
> + * preempted with another higher priority thread or an interrupt. So,
> + * IRQF_NO_THREAD flag is required in order to prevent any preemption
> + * during filling the FIFO buffer and possible data lost.
> + */
> + if (dev->flags & NO_EMPTYFIFO_HOLD_MASTER)
> + irq_flags |= IRQF_NO_THREAD;
> +
> ret = i2c_dw_acquire_lock(dev);
> if (ret)
> return ret;
> diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
> index 34d881572351c..5e459175dcdb2 100644
> --- a/drivers/i2c/busses/i2c-designware-platdrv.c
> +++ b/drivers/i2c/busses/i2c-designware-platdrv.c
> @@ -345,9 +345,11 @@ static void dw_i2c_plat_remove(struct platform_device *pdev)
> }
>
> static const struct of_device_id dw_i2c_of_match[] = {
> - { .compatible = "snps,designware-i2c", },
> - { .compatible = "mscc,ocelot-i2c", .data = (void *)MODEL_MSCC_OCELOT },
> { .compatible = "baikal,bt1-sys-i2c", .data = (void *)MODEL_BAIKAL_BT1 },
> + { .compatible = "mobileye,eyeq6lplus-i2c", .data = (void *)NO_EMPTYFIFO_HOLD_MASTER },
> + { .compatible = "mobileye,eyeq7h-i2c", .data = (void *)NO_EMPTYFIFO_HOLD_MASTER },
> + { .compatible = "mscc,ocelot-i2c", .data = (void *)MODEL_MSCC_OCELOT },
> + { .compatible = "snps,designware-i2c", },
> {}
> };
> MODULE_DEVICE_TABLE(of, dw_i2c_of_match);
>
> --
> 2.51.0
Powered by blists - more mailing lists