| Message ID | c772d1d8-eaae-43c2-c206-7c349c134db8@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
On Wed, 2017-03-08 at 07:47 +0100, Heiner Kallweit wrote: > If state is STATE_IDLE no interrupt should occur. Detect this case and > warn. > In addition move resetting REG_CTRL_START bit to the start of the > interrupt handler and remove a unneeded REG_CTRL_START bit reset > in meson_i2c_xfer_msg. > > Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> > --- > drivers/i2c/busses/i2c-meson.c | 16 +++++++++------- > 1 file changed, 9 insertions(+), 7 deletions(-) > > diff --git a/drivers/i2c/busses/i2c-meson.c b/drivers/i2c/busses/i2c-meson.c > index 81304840..b3b881f9 100644 > --- a/drivers/i2c/busses/i2c-meson.c > +++ b/drivers/i2c/busses/i2c-meson.c > @@ -233,7 +233,15 @@ static irqreturn_t meson_i2c_irq(int irqno, void *dev_id) > dev_dbg(i2c->dev, "irq: state %d, pos %d, count %d, ctrl %08x\n", > i2c->state, i2c->pos, i2c->count, ctrl); > > - if (ctrl & REG_CTRL_ERROR && i2c->state != STATE_IDLE) { > + meson_i2c_set_mask(i2c, REG_CTRL, REG_CTRL_START, 0); > + > + if (i2c->state == STATE_IDLE) { > + dev_notice(i2c->dev, "spurious interrupt detected\n"); > + spin_unlock(&i2c->lock); > + return IRQ_NONE; > + } Does it really happen ? Did you see any specific issue you'd like to share ? If not, I'm not a big fan of this change. In any case, if it can be handled gracefully, I'd drop the trace in the interrupt handler. > + > + if (ctrl & REG_CTRL_ERROR) { > /* > * The bit is set when the IGNORE_NAK bit is cleared > * and the device didn't respond. In this case, the > @@ -276,15 +284,12 @@ static irqreturn_t meson_i2c_irq(int irqno, void > *dev_id) > i2c->state = STATE_IDLE; > complete(&i2c->done); > break; > - case STATE_IDLE: > - break; > } > > out: > if (i2c->state != STATE_IDLE) { > /* Restart the processing */ > meson_i2c_write_tokens(i2c); > - meson_i2c_set_mask(i2c, REG_CTRL, REG_CTRL_START, 0); > meson_i2c_set_mask(i2c, REG_CTRL, REG_CTRL_START, > REG_CTRL_START); > } > @@ -344,9 +349,6 @@ static int meson_i2c_xfer_msg(struct meson_i2c *i2c, > struct i2c_msg *msg, > */ > spin_lock_irqsave(&i2c->lock, flags); > > - /* Abort any active operation */ > - meson_i2c_set_mask(i2c, REG_CTRL, REG_CTRL_START, 0); > - > if (!time_left) { > i2c->state = STATE_IDLE; > ret = -ETIMEDOUT;
Am 08.03.2017 um 10:50 schrieb Jerome Brunet: > On Wed, 2017-03-08 at 07:47 +0100, Heiner Kallweit wrote: >> If state is STATE_IDLE no interrupt should occur. Detect this case and >> warn. >> In addition move resetting REG_CTRL_START bit to the start of the >> interrupt handler and remove a unneeded REG_CTRL_START bit reset >> in meson_i2c_xfer_msg. >> >> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> >> --- >> drivers/i2c/busses/i2c-meson.c | 16 +++++++++------- >> 1 file changed, 9 insertions(+), 7 deletions(-) >> >> diff --git a/drivers/i2c/busses/i2c-meson.c b/drivers/i2c/busses/i2c-meson.c >> index 81304840..b3b881f9 100644 >> --- a/drivers/i2c/busses/i2c-meson.c >> +++ b/drivers/i2c/busses/i2c-meson.c >> @@ -233,7 +233,15 @@ static irqreturn_t meson_i2c_irq(int irqno, void *dev_id) >> dev_dbg(i2c->dev, "irq: state %d, pos %d, count %d, ctrl %08x\n", >> i2c->state, i2c->pos, i2c->count, ctrl); >> >> - if (ctrl & REG_CTRL_ERROR && i2c->state != STATE_IDLE) { >> + meson_i2c_set_mask(i2c, REG_CTRL, REG_CTRL_START, 0); >> + >> + if (i2c->state == STATE_IDLE) { >> + dev_notice(i2c->dev, "spurious interrupt detected\n"); >> + spin_unlock(&i2c->lock); >> + return IRQ_NONE; >> + } > > Does it really happen ? Did you see any specific issue you'd like to share ? > If not, I'm not a big fan of this change. > No,it did not happen. It's just that in few parts of the interrupt handler we deal with this situation that should never happen. And if it would happen on some system, we wouldn't know because it's silently ignored. I'm fine with dropping the error message. Still I would prefer to at least return IRQ_NONE instead of IRQ_HANDLED in this potential error case. It's like other drivers checking in the interrupt handler whether any interrupt source is enabled and returning IRQ_NONE if not. > In any case, if it can be handled gracefully, I'd drop the trace in the > interrupt handler. > > >> + >> + if (ctrl & REG_CTRL_ERROR) { >> /* >> * The bit is set when the IGNORE_NAK bit is cleared >> * and the device didn't respond. In this case, the >> @@ -276,15 +284,12 @@ static irqreturn_t meson_i2c_irq(int irqno, void >> *dev_id) >> i2c->state = STATE_IDLE; >> complete(&i2c->done); >> break; >> - case STATE_IDLE: >> - break; >> } >> >> out: >> if (i2c->state != STATE_IDLE) { >> /* Restart the processing */ >> meson_i2c_write_tokens(i2c); >> - meson_i2c_set_mask(i2c, REG_CTRL, REG_CTRL_START, 0); >> meson_i2c_set_mask(i2c, REG_CTRL, REG_CTRL_START, >> REG_CTRL_START); >> } >> @@ -344,9 +349,6 @@ static int meson_i2c_xfer_msg(struct meson_i2c *i2c, >> struct i2c_msg *msg, >> */ >> spin_lock_irqsave(&i2c->lock, flags); >> >> - /* Abort any active operation */ >> - meson_i2c_set_mask(i2c, REG_CTRL, REG_CTRL_START, 0); >> - >> if (!time_left) { >> i2c->state = STATE_IDLE; >> ret = -ETIMEDOUT; >
Am 08.03.2017 um 22:02 schrieb Heiner Kallweit: > Am 08.03.2017 um 10:50 schrieb Jerome Brunet: >> On Wed, 2017-03-08 at 07:47 +0100, Heiner Kallweit wrote: >>> If state is STATE_IDLE no interrupt should occur. Detect this case and >>> warn. >>> In addition move resetting REG_CTRL_START bit to the start of the >>> interrupt handler and remove a unneeded REG_CTRL_START bit reset >>> in meson_i2c_xfer_msg. >>> >>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> >>> --- >>> drivers/i2c/busses/i2c-meson.c | 16 +++++++++------- >>> 1 file changed, 9 insertions(+), 7 deletions(-) >>> >>> diff --git a/drivers/i2c/busses/i2c-meson.c b/drivers/i2c/busses/i2c-meson.c >>> index 81304840..b3b881f9 100644 >>> --- a/drivers/i2c/busses/i2c-meson.c >>> +++ b/drivers/i2c/busses/i2c-meson.c >>> @@ -233,7 +233,15 @@ static irqreturn_t meson_i2c_irq(int irqno, void *dev_id) >>> dev_dbg(i2c->dev, "irq: state %d, pos %d, count %d, ctrl %08x\n", >>> i2c->state, i2c->pos, i2c->count, ctrl); >>> >>> - if (ctrl & REG_CTRL_ERROR && i2c->state != STATE_IDLE) { >>> + meson_i2c_set_mask(i2c, REG_CTRL, REG_CTRL_START, 0); >>> + >>> + if (i2c->state == STATE_IDLE) { >>> + dev_notice(i2c->dev, "spurious interrupt detected\n"); >>> + spin_unlock(&i2c->lock); >>> + return IRQ_NONE; >>> + } >> >> Does it really happen ? Did you see any specific issue you'd like to share ? >> If not, I'm not a big fan of this change. >> > No,it did not happen. It's just that in few parts of the interrupt handler > we deal with this situation that should never happen. > And if it would happen on some system, we wouldn't know because it's > silently ignored. > > I'm fine with dropping the error message. Still I would prefer to at least > return IRQ_NONE instead of IRQ_HANDLED in this potential error case. > > It's like other drivers checking in the interrupt handler whether any > interrupt source is enabled and returning IRQ_NONE if not. > >> In any case, if it can be handled gracefully, I'd drop the trace in the >> interrupt handler. >> >> >>> + >>> + if (ctrl & REG_CTRL_ERROR) { >>> /* >>> * The bit is set when the IGNORE_NAK bit is cleared >>> * and the device didn't respond. In this case, the >>> @@ -276,15 +284,12 @@ static irqreturn_t meson_i2c_irq(int irqno, void >>> *dev_id) >>> i2c->state = STATE_IDLE; >>> complete(&i2c->done); >>> break; >>> - case STATE_IDLE: >>> - break; >>> } >>> >>> out: >>> if (i2c->state != STATE_IDLE) { >>> /* Restart the processing */ >>> meson_i2c_write_tokens(i2c); >>> - meson_i2c_set_mask(i2c, REG_CTRL, REG_CTRL_START, 0); >>> meson_i2c_set_mask(i2c, REG_CTRL, REG_CTRL_START, >>> REG_CTRL_START); >>> } >>> @@ -344,9 +349,6 @@ static int meson_i2c_xfer_msg(struct meson_i2c *i2c, >>> struct i2c_msg *msg, >>> */ >>> spin_lock_irqsave(&i2c->lock, flags); >>> >>> - /* Abort any active operation */ >>> - meson_i2c_set_mask(i2c, REG_CTRL, REG_CTRL_START, 0); >>> - Just recognized that we need to keep this statement to properly handle the timeout case. So I'll make this part of a v3 together with potential changes based on further review feedback. >>> if (!time_left) { >>> i2c->state = STATE_IDLE; >>> ret = -ETIMEDOUT; >> >
diff --git a/drivers/i2c/busses/i2c-meson.c b/drivers/i2c/busses/i2c-meson.c index 81304840..b3b881f9 100644 --- a/drivers/i2c/busses/i2c-meson.c +++ b/drivers/i2c/busses/i2c-meson.c @@ -233,7 +233,15 @@ static irqreturn_t meson_i2c_irq(int irqno, void *dev_id) dev_dbg(i2c->dev, "irq: state %d, pos %d, count %d, ctrl %08x\n", i2c->state, i2c->pos, i2c->count, ctrl); - if (ctrl & REG_CTRL_ERROR && i2c->state != STATE_IDLE) { + meson_i2c_set_mask(i2c, REG_CTRL, REG_CTRL_START, 0); + + if (i2c->state == STATE_IDLE) { + dev_notice(i2c->dev, "spurious interrupt detected\n"); + spin_unlock(&i2c->lock); + return IRQ_NONE; + } + + if (ctrl & REG_CTRL_ERROR) { /* * The bit is set when the IGNORE_NAK bit is cleared * and the device didn't respond. In this case, the @@ -276,15 +284,12 @@ static irqreturn_t meson_i2c_irq(int irqno, void *dev_id) i2c->state = STATE_IDLE; complete(&i2c->done); break; - case STATE_IDLE: - break; } out: if (i2c->state != STATE_IDLE) { /* Restart the processing */ meson_i2c_write_tokens(i2c); - meson_i2c_set_mask(i2c, REG_CTRL, REG_CTRL_START, 0); meson_i2c_set_mask(i2c, REG_CTRL, REG_CTRL_START, REG_CTRL_START); } @@ -344,9 +349,6 @@ static int meson_i2c_xfer_msg(struct meson_i2c *i2c, struct i2c_msg *msg, */ spin_lock_irqsave(&i2c->lock, flags); - /* Abort any active operation */ - meson_i2c_set_mask(i2c, REG_CTRL, REG_CTRL_START, 0); - if (!time_left) { i2c->state = STATE_IDLE; ret = -ETIMEDOUT;
If state is STATE_IDLE no interrupt should occur. Detect this case and warn. In addition move resetting REG_CTRL_START bit to the start of the interrupt handler and remove a unneeded REG_CTRL_START bit reset in meson_i2c_xfer_msg. Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> --- drivers/i2c/busses/i2c-meson.c | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-)