Re: [PATCH v4 1/5] i2c:ocores: stop transfer on timeout
> Could you drop these patches and wait for a new version? I don't > think you have pushed it out yet? So it won't be a visible rebase. Yes, I can do that. When resending, just make sure you include the fixes I mentioned when applying. Thanks! signature.asc Description: PGP signature
Re: [PATCH v4 1/5] i2c:ocores: stop transfer on timeout
On Monday, February 11, 2019 3:01:38 PM CET Andrew Lunn wrote: > > Applied to for-next, thanks! > > Hi Wolfram > > Could you drop these patches and wait for a new version? I don't > think you have pushed it out yet? So it won't be a visible rebase. I will wait to send v5: full patch set, or just PATCH 3 and 5 ? > > Thanks > Andrew
Re: [PATCH v4 1/5] i2c:ocores: stop transfer on timeout
> Applied to for-next, thanks! Hi Wolfram Could you drop these patches and wait for a new version? I don't think you have pushed it out yet? So it won't be a visible rebase. Thanks Andrew
Re: [PATCH v4 1/5] i2c:ocores: stop transfer on timeout
On Monday, February 11, 2019 11:44:46 AM CET Peter Rosin wrote: > On 2019-02-11 09:31, Federico Vaga wrote: > > > Detecting a timeout is ok, but we also need to assert a STOP command on > > the bus in order to prevent it from generating interrupts when there are > > no on going transfers. > > > > Example: very long transmission. > > > > 1. ocores_xfer: START a transfer > > 2. ocores_isr : handle byte by byte the transfer > > 3. ocores_xfer: goes in timeout [[bugfix here]] > > 4. ocores_xfer: return to I2C subsystem and to the I2C driver > > 5. I2C driver : it may clean up the i2c_msg memory > > 6. ocores_isr : receives another interrupt (pending bytes to be > > > > transferred) but the i2c_msg memory is invalid now > > > > > > So, since the transfer was too long, we have to detect the timeout and > > STOP the transfer. > > > > Another point is that we have a critical region here. When handling the > > timeout condition we may have a running IRQ handler. For this reason I > > introduce a spinlock. > > > > In order to make easier to understan locking I have: > > - added a new function to handle timeout > > - modified the current ocores_process() function in order to be protected > > > > by the new spinlock > > > > Like this it is obvious at first sight that this locking serializes > > the execution of ocores_process() and ocores_process_timeout() > > > > > *snip* > > > > @@ -184,14 +197,14 @@ static void ocores_process(struct ocores_i2c *i2c) > > > > > > > > oc_setreg(i2c, OCI2C_DATA, addr); > > oc_setreg(i2c, OCI2C_CMD, OCI2C_CMD_START); > > > Didn't checkpatch complain about the double space? Fixing it fits in > patch 5... Apparently not, I will add the fix the checkpatch PATCH > Cheers, > Peter
Re: [PATCH v4 1/5] i2c:ocores: stop transfer on timeout
On 2019-02-11 09:31, Federico Vaga wrote: > Detecting a timeout is ok, but we also need to assert a STOP command on > the bus in order to prevent it from generating interrupts when there are > no on going transfers. > > Example: very long transmission. > > 1. ocores_xfer: START a transfer > 2. ocores_isr : handle byte by byte the transfer > 3. ocores_xfer: goes in timeout [[bugfix here]] > 4. ocores_xfer: return to I2C subsystem and to the I2C driver > 5. I2C driver : it may clean up the i2c_msg memory > 6. ocores_isr : receives another interrupt (pending bytes to be > transferred) but the i2c_msg memory is invalid now > > So, since the transfer was too long, we have to detect the timeout and > STOP the transfer. > > Another point is that we have a critical region here. When handling the > timeout condition we may have a running IRQ handler. For this reason I > introduce a spinlock. > > In order to make easier to understan locking I have: > - added a new function to handle timeout > - modified the current ocores_process() function in order to be protected > by the new spinlock > Like this it is obvious at first sight that this locking serializes > the execution of ocores_process() and ocores_process_timeout() > *snip* > @@ -184,14 +197,14 @@ static void ocores_process(struct ocores_i2c *i2c) > > oc_setreg(i2c, OCI2C_DATA, addr); > oc_setreg(i2c, OCI2C_CMD, OCI2C_CMD_START); Didn't checkpatch complain about the double space? Fixing it fits in patch 5... Cheers, Peter
Re: [PATCH v4 1/5] i2c:ocores: stop transfer on timeout
On Mon, Feb 11, 2019 at 09:31:18AM +0100, Federico Vaga wrote: > Detecting a timeout is ok, but we also need to assert a STOP command on > the bus in order to prevent it from generating interrupts when there are > no on going transfers. > > Example: very long transmission. > > 1. ocores_xfer: START a transfer > 2. ocores_isr : handle byte by byte the transfer > 3. ocores_xfer: goes in timeout [[bugfix here]] > 4. ocores_xfer: return to I2C subsystem and to the I2C driver > 5. I2C driver : it may clean up the i2c_msg memory > 6. ocores_isr : receives another interrupt (pending bytes to be > transferred) but the i2c_msg memory is invalid now > > So, since the transfer was too long, we have to detect the timeout and > STOP the transfer. > > Another point is that we have a critical region here. When handling the > timeout condition we may have a running IRQ handler. For this reason I > introduce a spinlock. > > In order to make easier to understan locking I have: > - added a new function to handle timeout > - modified the current ocores_process() function in order to be protected > by the new spinlock > Like this it is obvious at first sight that this locking serializes > the execution of ocores_process() and ocores_process_timeout() > > Signed-off-by: Federico Vaga > Reviewed-by: Andrew Lunn > Applied to for-next, thanks! signature.asc Description: PGP signature
[PATCH v4 1/5] i2c:ocores: stop transfer on timeout
Detecting a timeout is ok, but we also need to assert a STOP command on the bus in order to prevent it from generating interrupts when there are no on going transfers. Example: very long transmission. 1. ocores_xfer: START a transfer 2. ocores_isr : handle byte by byte the transfer 3. ocores_xfer: goes in timeout [[bugfix here]] 4. ocores_xfer: return to I2C subsystem and to the I2C driver 5. I2C driver : it may clean up the i2c_msg memory 6. ocores_isr : receives another interrupt (pending bytes to be transferred) but the i2c_msg memory is invalid now So, since the transfer was too long, we have to detect the timeout and STOP the transfer. Another point is that we have a critical region here. When handling the timeout condition we may have a running IRQ handler. For this reason I introduce a spinlock. In order to make easier to understan locking I have: - added a new function to handle timeout - modified the current ocores_process() function in order to be protected by the new spinlock Like this it is obvious at first sight that this locking serializes the execution of ocores_process() and ocores_process_timeout() Signed-off-by: Federico Vaga Reviewed-by: Andrew Lunn --- drivers/i2c/busses/i2c-ocores.c | 54 ++--- 1 file changed, 45 insertions(+), 9 deletions(-) diff --git a/drivers/i2c/busses/i2c-ocores.c b/drivers/i2c/busses/i2c-ocores.c index 87f9caa..aa85202 100644 --- a/drivers/i2c/busses/i2c-ocores.c +++ b/drivers/i2c/busses/i2c-ocores.c @@ -25,7 +25,12 @@ #include #include #include +#include +/** + * @process_lock: protect I2C transfer process. + * ocores_process() and ocores_process_timeout() can't run in parallel. + */ struct ocores_i2c { void __iomem *base; u32 reg_shift; @@ -36,6 +41,7 @@ struct ocores_i2c { int pos; int nmsgs; int state; /* see STATE_ */ + spinlock_t process_lock; struct clk *clk; int ip_clock_khz; int bus_clock_khz; @@ -141,19 +147,26 @@ static void ocores_process(struct ocores_i2c *i2c) { struct i2c_msg *msg = i2c->msg; u8 stat = oc_getreg(i2c, OCI2C_STATUS); + unsigned long flags; + + /* +* If we spin here is because we are in timeout, so we are going +* to be in STATE_ERROR. See ocores_process_timeout() +*/ + spin_lock_irqsave(>process_lock, flags); if ((i2c->state == STATE_DONE) || (i2c->state == STATE_ERROR)) { /* stop has been sent */ oc_setreg(i2c, OCI2C_CMD, OCI2C_CMD_IACK); wake_up(>wait); - return; + goto out; } /* error? */ if (stat & OCI2C_STAT_ARBLOST) { i2c->state = STATE_ERROR; oc_setreg(i2c, OCI2C_CMD, OCI2C_CMD_STOP); - return; + goto out; } if ((i2c->state == STATE_START) || (i2c->state == STATE_WRITE)) { @@ -163,7 +176,7 @@ static void ocores_process(struct ocores_i2c *i2c) if (stat & OCI2C_STAT_NACK) { i2c->state = STATE_ERROR; oc_setreg(i2c, OCI2C_CMD, OCI2C_CMD_STOP); - return; + goto out; } } else msg->buf[i2c->pos++] = oc_getreg(i2c, OCI2C_DATA); @@ -184,14 +197,14 @@ static void ocores_process(struct ocores_i2c *i2c) oc_setreg(i2c, OCI2C_DATA, addr); oc_setreg(i2c, OCI2C_CMD, OCI2C_CMD_START); - return; + goto out; } else i2c->state = (msg->flags & I2C_M_RD) ? STATE_READ : STATE_WRITE; } else { i2c->state = STATE_DONE; oc_setreg(i2c, OCI2C_CMD, OCI2C_CMD_STOP); - return; + goto out; } } @@ -202,6 +215,9 @@ static void ocores_process(struct ocores_i2c *i2c) oc_setreg(i2c, OCI2C_DATA, msg->buf[i2c->pos++]); oc_setreg(i2c, OCI2C_CMD, OCI2C_CMD_WRITE); } + +out: + spin_unlock_irqrestore(>process_lock, flags); } static irqreturn_t ocores_isr(int irq, void *dev_id) @@ -213,9 +229,24 @@ static irqreturn_t ocores_isr(int irq, void *dev_id) return IRQ_HANDLED; } +/** + * Process timeout event + * @i2c: ocores I2C device instance + */ +static void ocores_process_timeout(struct ocores_i2c *i2c) +{ + unsigned long flags; + + spin_lock_irqsave(>process_lock, flags); + i2c->state = STATE_ERROR; + oc_setreg(i2c, OCI2C_CMD, OCI2C_CMD_STOP); + spin_unlock_irqrestore(>process_lock, flags); +} + static int ocores_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num) { struct