| 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: <20211101172706.GC4698@minyard.net>
Date: Mon, 1 Nov 2021 12:27:06 -0500
From: Corey Minyard <minyard@....org>
To: Oleksij Rempel <o.rempel@...gutronix.de>
Cc: Pengutronix Kernel Team <kernel@...gutronix.de>,
linux-i2c@...r.kernel.org,
Andrew Manley <andrew.manley@...lingtech.com>,
linux-kernel@...r.kernel.org, Corey Minyard <cminyard@...sta.com>
Subject: Re: [PATCH 1/3] i2c:imx: Add timer for handling the stop condition
On Mon, Oct 04, 2021 at 07:32:14PM -0500, minyard@....org wrote:
> From: Corey Minyard <cminyard@...sta.com>
Any comments for this patch series?
-corey
>
> Most IMX I2C interfaces don't generate an interrupt on a stop condition,
> so it won't generate a timely stop event on a slave mode transfer.
> Some users, like IPMB, need a timely stop event to work properly.
>
> So, add a timer and add the proper handling to generate a stop event in
> slave mode if the interface goes idle.
>
> Signed-off-by: Corey Minyard <minyard@....org>
> Tested-by: Andrew Manley <andrew.manley@...lingtech.com>
> Reviewed-by: Andrew Manley <andrew.manley@...lingtech.com>
> ---
> drivers/i2c/busses/i2c-imx.c | 78 +++++++++++++++++++++++++++---------
> 1 file changed, 59 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c
> index 3576b63a6c03..97369fe48b30 100644
> --- a/drivers/i2c/busses/i2c-imx.c
> +++ b/drivers/i2c/busses/i2c-imx.c
> @@ -37,6 +37,8 @@
> #include <linux/io.h>
> #include <linux/iopoll.h>
> #include <linux/kernel.h>
> +#include <linux/spinlock.h>
> +#include <linux/timer.h>
> #include <linux/module.h>
> #include <linux/of.h>
> #include <linux/of_device.h>
> @@ -210,6 +212,10 @@ struct imx_i2c_struct {
> struct imx_i2c_dma *dma;
> struct i2c_client *slave;
> enum i2c_slave_event last_slave_event;
> +
> + /* For checking slave events. */
> + spinlock_t slave_lock;
> + struct timer_list slave_timer;
> };
>
> static const struct imx_i2c_hwdata imx1_i2c_hwdata = {
> @@ -680,7 +686,7 @@ static void i2c_imx_slave_event(struct imx_i2c_struct *i2c_imx,
>
> static void i2c_imx_slave_finish_op(struct imx_i2c_struct *i2c_imx)
> {
> - u8 val;
> + u8 val = 0;
>
> while (i2c_imx->last_slave_event != I2C_SLAVE_STOP) {
> switch (i2c_imx->last_slave_event) {
> @@ -701,10 +707,11 @@ static void i2c_imx_slave_finish_op(struct imx_i2c_struct *i2c_imx)
> }
> }
>
> -static irqreturn_t i2c_imx_slave_isr(struct imx_i2c_struct *i2c_imx,
> - unsigned int status, unsigned int ctl)
> +/* Returns true if the timer should be restarted, false if not. */
> +static irqreturn_t i2c_imx_slave_handle(struct imx_i2c_struct *i2c_imx,
> + unsigned int status, unsigned int ctl)
> {
> - u8 value;
> + u8 value = 0;
>
> if (status & I2SR_IAL) { /* Arbitration lost */
> i2c_imx_clear_irq(i2c_imx, I2SR_IAL);
> @@ -712,6 +719,16 @@ static irqreturn_t i2c_imx_slave_isr(struct imx_i2c_struct *i2c_imx,
> return IRQ_HANDLED;
> }
>
> + if (!(status & I2SR_IBB)) {
> + /* No master on the bus, that could mean a stop condition. */
> + i2c_imx_slave_finish_op(i2c_imx);
> + return IRQ_HANDLED;
> + }
> +
> + if (!(status & I2SR_ICF))
> + /* Data transfer still in progress, ignore this. */
> + goto out;
> +
> if (status & I2SR_IAAS) { /* Addressed as a slave */
> i2c_imx_slave_finish_op(i2c_imx);
> if (status & I2SR_SRW) { /* Master wants to read from us*/
> @@ -737,16 +754,9 @@ static irqreturn_t i2c_imx_slave_isr(struct imx_i2c_struct *i2c_imx,
> imx_i2c_read_reg(i2c_imx, IMX_I2C_I2DR);
> }
> } else if (!(ctl & I2CR_MTX)) { /* Receive mode */
> - if (status & I2SR_IBB) { /* No STOP signal detected */
> - value = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2DR);
> - i2c_imx_slave_event(i2c_imx,
> - I2C_SLAVE_WRITE_RECEIVED, &value);
> - } else { /* STOP signal is detected */
> - dev_dbg(&i2c_imx->adapter.dev,
> - "STOP signal detected");
> - i2c_imx_slave_event(i2c_imx,
> - I2C_SLAVE_STOP, &value);
> - }
> + value = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2DR);
> + i2c_imx_slave_event(i2c_imx,
> + I2C_SLAVE_WRITE_RECEIVED, &value);
> } else if (!(status & I2SR_RXAK)) { /* Transmit mode received ACK */
> ctl |= I2CR_MTX;
> imx_i2c_write_reg(ctl, i2c_imx, IMX_I2C_I2CR);
> @@ -755,15 +765,32 @@ static irqreturn_t i2c_imx_slave_isr(struct imx_i2c_struct *i2c_imx,
> I2C_SLAVE_READ_PROCESSED, &value);
>
> imx_i2c_write_reg(value, i2c_imx, IMX_I2C_I2DR);
> - } else { /* Transmit mode received NAK */
> + } else { /* Transmit mode received NAK, operation is done */
> ctl &= ~I2CR_MTX;
> imx_i2c_write_reg(ctl, i2c_imx, IMX_I2C_I2CR);
> imx_i2c_read_reg(i2c_imx, IMX_I2C_I2DR);
> + i2c_imx_slave_finish_op(i2c_imx);
> + return IRQ_HANDLED;
> }
>
> +out:
> + mod_timer(&i2c_imx->slave_timer, jiffies + 1);
> return IRQ_HANDLED;
> }
>
> +static void i2c_imx_slave_timeout(struct timer_list *t)
> +{
> + struct imx_i2c_struct *i2c_imx = from_timer(i2c_imx, t, slave_timer);
> + unsigned int ctl, status;
> + unsigned long flags;
> +
> + spin_lock_irqsave(&i2c_imx->slave_lock, flags);
> + status = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2SR);
> + ctl = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2CR);
> + i2c_imx_slave_handle(i2c_imx, status, ctl);
> + spin_unlock_irqrestore(&i2c_imx->slave_lock, flags);
> +}
> +
> static void i2c_imx_slave_init(struct imx_i2c_struct *i2c_imx)
> {
> int temp;
> @@ -843,7 +870,9 @@ static irqreturn_t i2c_imx_isr(int irq, void *dev_id)
> {
> struct imx_i2c_struct *i2c_imx = dev_id;
> unsigned int ctl, status;
> + unsigned long flags;
>
> + spin_lock_irqsave(&i2c_imx->slave_lock, flags);
> status = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2SR);
> ctl = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2CR);
>
> @@ -851,14 +880,20 @@ static irqreturn_t i2c_imx_isr(int irq, void *dev_id)
> i2c_imx_clear_irq(i2c_imx, I2SR_IIF);
> if (i2c_imx->slave) {
> if (!(ctl & I2CR_MSTA)) {
> - return i2c_imx_slave_isr(i2c_imx, status, ctl);
> - } else if (i2c_imx->last_slave_event !=
> - I2C_SLAVE_STOP) {
> - i2c_imx_slave_finish_op(i2c_imx);
> + irqreturn_t ret;
> +
> + ret = i2c_imx_slave_handle(i2c_imx,
> + status, ctl);
> + spin_unlock_irqrestore(&i2c_imx->slave_lock,
> + flags);
> + return ret;
> }
> + i2c_imx_slave_finish_op(i2c_imx);
> }
> + spin_unlock_irqrestore(&i2c_imx->slave_lock, flags);
> return i2c_imx_master_isr(i2c_imx, status);
> }
> + spin_unlock_irqrestore(&i2c_imx->slave_lock, flags);
>
> return IRQ_NONE;
> }
> @@ -1378,6 +1413,9 @@ static int i2c_imx_probe(struct platform_device *pdev)
> if (!i2c_imx)
> return -ENOMEM;
>
> + spin_lock_init(&i2c_imx->slave_lock);
> + timer_setup(&i2c_imx->slave_timer, i2c_imx_slave_timeout, 0);
> +
> match = device_get_match_data(&pdev->dev);
> if (match)
> i2c_imx->hwdata = match;
> @@ -1491,6 +1529,8 @@ static int i2c_imx_remove(struct platform_device *pdev)
> if (ret < 0)
> return ret;
>
> + del_timer_sync(&i2c_imx->slave_timer);
> +
> /* remove adapter */
> dev_dbg(&i2c_imx->adapter.dev, "adapter removed\n");
> i2c_del_adapter(&i2c_imx->adapter);
> --
> 2.25.1
>
Powered by blists - more mailing lists