| 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: <f5e7bb0d-205b-4c10-8c31-bf60e1e42b73@kernel.org>
Date: Mon, 20 Oct 2025 11:38:38 +0200
From: Hans Verkuil <hverkuil+cisco@...nel.org>
To: Benoît Monin <benoit.monin@...tlin.com>,
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>,
Mika Westerberg <mika.westerberg@...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>
Cc: 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 2/3] i2c: designware: Enable transfer with different
target addresses
Hi Benoît,
On 17/10/2025 16:59, Benoît Monin wrote:
> When i2c_dw_xfer() is called with more than one message, it sets the
> target address according to the first message. If any of the following
> messages have a different target address, the transfer finishes with
> an error.
>
> Instead, if the next message has a different target address, wait until
> all previous messages are sent and the STOP condition is detected. This
> will complete the current part of the transfer. The next part is then
> handled by looping in i2c_dw_xfer(), calling i2c_dw_xfer_init() and
> i2c_dw_wait_transfer() until all messages of the transfer have been
> processed, or an error is detected.
>
> The RESTART bit is now set after the first message of each part of the
> transfer, instead of just after the very first message of the whole
> transfer.
>
> For each address change, i2c_dw_xfer_init() is called, which takes care
> of disabling the adapter before changing the target address register,
> then re-enabling it. Given that we cannot know the value of the
> I2C_DYNAMIC_TAR_UPDATE parameter, this is the only sure way to change
> the target address.
I have the problem described here:
https://lore.kernel.org/linux-i2c/ee6afdd7-3117-43cd-831f-e0ec5ee46f46@kernel.org/
And it looks like this patch is intended to solve that problem (one transaction
with two writes to different target addresses).
I tried this patch, but it doesn't work. Instead I get a time out:
[ 111.695238] i2c_designware 1f00074000.i2c: controller timed out
Is it indeed meant to solve the problem I have or is it addressing another
issue?
I'm happy to help test patches.
Regards,
Hans
>
> 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-master.c | 58 ++++++++++++++++--------------
> 1 file changed, 31 insertions(+), 27 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-designware-master.c b/drivers/i2c/busses/i2c-designware-master.c
> index c7a72c28786c2..f9a180b145da8 100644
> --- a/drivers/i2c/busses/i2c-designware-master.c
> +++ b/drivers/i2c/busses/i2c-designware-master.c
> @@ -436,6 +436,7 @@ i2c_dw_xfer_msg(struct dw_i2c_dev *dev)
> u8 *buf = dev->tx_buf;
> bool need_restart = false;
> unsigned int flr;
> + int first_idx = dev->msg_write_idx;
>
> intr_mask = DW_IC_INTR_MASTER_MASK;
>
> @@ -446,11 +447,11 @@ i2c_dw_xfer_msg(struct dw_i2c_dev *dev)
> * 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) {
> - dev_err(dev->dev,
> - "%s: invalid target address\n", __func__);
> - dev->msg_err = -EINVAL;
> + intr_mask &= ~DW_IC_INTR_TX_EMPTY;
> break;
> }
>
> @@ -465,7 +466,7 @@ i2c_dw_xfer_msg(struct dw_i2c_dev *dev)
> * set restart bit between messages.
> */
> if ((dev->master_cfg & DW_IC_CON_RESTART_EN) &&
> - (dev->msg_write_idx > 0))
> + (dev->msg_write_idx > first_idx))
> need_restart = true;
> }
>
> @@ -822,7 +823,6 @@ i2c_dw_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
> break;
> }
>
> - reinit_completion(&dev->cmd_complete);
> dev->msgs = msgs;
> dev->msgs_num = num;
> dev->cmd_err = 0;
> @@ -841,18 +841,33 @@ i2c_dw_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
> if (ret < 0)
> goto done;
>
> - /* Start the transfers */
> - i2c_dw_xfer_init(dev);
> + do {
> + reinit_completion(&dev->cmd_complete);
>
> - /* Wait for tx to complete */
> - ret = i2c_dw_wait_transfer(dev);
> - if (ret) {
> - dev_err(dev->dev, "controller timed out\n");
> - /* i2c_dw_init_master() implicitly disables the adapter */
> - i2c_recover_bus(&dev->adapter);
> - i2c_dw_init_master(dev);
> - goto done;
> - }
> + /* Start the transfers */
> + i2c_dw_xfer_init(dev);
> +
> + /* Wait for tx to complete */
> + ret = i2c_dw_wait_transfer(dev);
> + if (ret) {
> + dev_err(dev->dev, "controller timed out\n");
> + /* i2c_dw_init_master() implicitly disables the adapter */
> + i2c_recover_bus(&dev->adapter);
> + i2c_dw_init_master(dev);
> + goto done;
> + }
> +
> + if (dev->msg_err) {
> + ret = dev->msg_err;
> + goto done;
> + }
> +
> + /* We have an error */
> + if (dev->cmd_err == DW_IC_ERR_TX_ABRT) {
> + ret = i2c_dw_handle_tx_abort(dev);
> + goto done;
> + }
> + } while (dev->msg_write_idx < num);
>
> /*
> * This happens rarely (~1:500) and is hard to reproduce. Debug trace
> @@ -874,23 +889,12 @@ i2c_dw_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
> */
> __i2c_dw_disable_nowait(dev);
>
> - if (dev->msg_err) {
> - ret = dev->msg_err;
> - goto done;
> - }
> -
> /* No error */
> if (likely(!dev->cmd_err && !dev->status)) {
> ret = num;
> goto done;
> }
>
> - /* We have an error */
> - if (dev->cmd_err == DW_IC_ERR_TX_ABRT) {
> - ret = i2c_dw_handle_tx_abort(dev);
> - goto done;
> - }
> -
> if (dev->status)
> dev_err(dev->dev,
> "transfer terminated early - interrupt latency too high?\n");
>
Powered by blists - more mailing lists