Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1656168

Re: [PATCH v8 4/5] i2c: aspeed: added driver for Aspeed I2C

Show all headers | View raw

On Fri, 2017-06-02 at 02:29 -0700, Brendan Higgins wrote:
> > That way you avoid the above lock which is racy, slave activity could
> > get in there and trigger an error interrupt clobbering cmd_err for
> > example no ? Or am I missing something...
> 
> The slave handler does not touch these fields, so that should be fine.
> The only way I can think
> that we could get in this state is if we had some sort of error
> interrupt fire after we handled a
> STOP or previous error; this should not happen, but I think it is
> better to be safe than sorry.
> Nevertheless, I do not think it is necessary to do more than what I
> have already done because
> it would mean the bus is in a pretty bad state anyway. Maybe I should
> just drop the locks here.
> 
> If you disagree, could you elaborate on what you meant by putting
> cmd_err in a separate field?
> Did you just mean having one for xfer and one for everything else (like resets)?

None of that is a big deal and definitely not a blocker for merging,
you can always "improve" things with a latter patch.

I was thinking that the act of "completing" a request could cleaner if
entirely done from the interrupt code, thus clearing bus->msgs and
setting a "final" status code to be returned to the caller.

Now that could be "cmd_err", it's just that today, that is written to
under various circumstances that may or may not related to a command
being in progress and this I worry that with the lock dropping, we
might "lose" the value that actually pertain to the command itself (and
possibly caused it to fail).

This is all quite academic however, as you said, if that happens we are
already probably in a pretty bad shape.

I suspect the only errors that would happen in "normal" circumstances
would be loss of arbitration and nacks, which are I think, already
properly handled, at least from my reading of the code.

Cheers,
Ben.

> > > +     /* If nothing went wrong, return number of messages transferred. */
> > > +     if (ret >= 0)
> > > +             return bus->msgs_index + 1;
> > > +     else
> > > +             return ret;
> > > +}
> > > +
> 
> --
> To unsubscribe from this list: send the line "unsubscribe devicetree" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Back to linux.kernel | Previous | Next — Previous in thread | Next in thread | Find similar | Unroll thread

Thread

[PATCH v8 0/5] i2c: aspeed: added driver for Aspeed I2C Brendan Higgins <brendanhiggins@google.com> - 2017-05-25 09:00 +0200
  [PATCH v8 3/5] i2c: aspeed: added documentation for Aspeed I2C driver Brendan Higgins <brendanhiggins@google.com> - 2017-05-25 09:00 +0200
    Re: [PATCH v8 3/5] i2c: aspeed: added documentation for Aspeed I2C  driver Rob Herring <robh@kernel.org> - 2017-05-30 23:40 +0200
  [PATCH v8 1/5] irqchip/aspeed-i2c-ic: binding docs for Aspeed I2C Interrupt Controller Brendan Higgins <brendanhiggins@google.com> - 2017-05-25 09:00 +0200
    Re: [PATCH v8 1/5] irqchip/aspeed-i2c-ic: binding docs for Aspeed  I2C Interrupt Controller Rob Herring <robh@kernel.org> - 2017-05-30 23:40 +0200
      Re: [PATCH v8 1/5] irqchip/aspeed-i2c-ic: binding docs for Aspeed I2C  Interrupt Controller Brendan Higgins <brendanhiggins@google.com> - 2017-05-30 23:50 +0200
  [PATCH v8 4/5] i2c: aspeed: added driver for Aspeed I2C Brendan Higgins <brendanhiggins@google.com> - 2017-05-25 09:00 +0200
    Re: [PATCH v8 4/5] i2c: aspeed: added driver for Aspeed I2C Benjamin Herrenschmidt <benh@kernel.crashing.org> - 2017-05-31 04:20 +0200
      Re: [PATCH v8 4/5] i2c: aspeed: added driver for Aspeed I2C Brendan Higgins <brendanhiggins@google.com> - 2017-06-02 11:30 +0200
        Re: [PATCH v8 4/5] i2c: aspeed: added driver for Aspeed I2C Benjamin Herrenschmidt <benh@kernel.crashing.org> - 2017-06-02 14:20 +0200
  [PATCH v8 5/5] i2c: aspeed: added slave support for Aspeed I2C driver Brendan Higgins <brendanhiggins@google.com> - 2017-05-25 09:00 +0200
  [PATCH v8 2/5] irqchip/aspeed-i2c-ic: Add I2C IRQ controller for Aspeed Brendan Higgins <brendanhiggins@google.com> - 2017-05-25 09:00 +0200

csiph-web