i2c: xiic: Support disabling multi-master in DT
diff mbox series

Message ID 20200218135627.24739-1-ext-jaakko.laine@vaisala.com
State Under Review
Headers show
Series
  • i2c: xiic: Support disabling multi-master in DT
Related show

Commit Message

Laine Jaakko EXT Feb. 18, 2020, 1:58 p.m. UTC
I2C master operating in multimaster mode can get stuck
indefinitely if I2C start is detected on bus, but no master
has a transaction going.

This is a weakness in I2C standard, which defines no way
to recover, since all masters are indefinitely disallowed
from interrupting the currently operating master. A start
condition can be created for example by an electromagnetic
discharge applied near physical I2C lines. Or a already
operating master could get reset immediately after sending
a start.

If it is known during device tree creation that only a single
I2C master will be present on the bus, this deadlock of the
I2C bus could be avoided in the driver by ignoring the
bus_is_busy register of the xiic, since bus can never be
reserved by any other master.

This patch adds this support for detecting multi-master flag
in device tree and when not provided, improves I2C reliability
by ignoring the therefore unnecessary xiic bus_is_busy register.

Error can be reproduced by pulling I2C SDA -line temporarily low
by shorting it to ground, while linux I2C master is operating on
it using the xiic driver. The application using the bus will
start receiving linux error code 16: "Device or resource busy"
indefinitely:

kernel: pca953x 0-0020: failed writing register
app: Error writing file, error: 16

With multi-master disabled device will instead receive error
code 5: "I/O error" while SDA is grounded, but recover normal
operation once short is removed.

kernel: pca953x 0-0020: failed reading register
app: Error reading file, error: 5

Signed-off-by: Jaakko Laine <ext-jaakko.laine@vaisala.com>
---

Applies against Linux 5.6-rc1 from master in
https://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git

I would like to point out that that since this patch disables
multimaster mode based on the standard I2C multimaster property
in device tree (as it propably should) and since the driver has
previously supported multimaster even when this property doesn't
exist in device tree, there is a possible backwards
compatibility issue:

If there are devices relying on the multimaster mode to work
without defining the property in device tree, their I2C bus
might not work without issues anymore after this patch, since
the driver will asume it is the only master on bus and could
therefore interrupt the communication of some other master on
same bus.

Please suggest some alternative fix if this is not acceptable
as is. On the other hand supporting multimaster even on a bus
with only a single master does currently cause some
reliability issues since the bus can get indefinitely stuck.
I don't think there exists a I2C protocol compatible way to
resolve the deadlock on multimaster bus.

 drivers/i2c/busses/i2c-xiic.c | 52 +++++++++++++++++++++--------------
 1 file changed, 32 insertions(+), 20 deletions(-)

Comments

Michal Simek Feb. 24, 2020, 10:13 a.m. UTC | #1
On 18. 02. 20 14:58, Laine Jaakko EXT wrote:
> I2C master operating in multimaster mode can get stuck
> indefinitely if I2C start is detected on bus, but no master
> has a transaction going.
> 
> This is a weakness in I2C standard, which defines no way
> to recover, since all masters are indefinitely disallowed
> from interrupting the currently operating master. A start
> condition can be created for example by an electromagnetic
> discharge applied near physical I2C lines. Or a already
> operating master could get reset immediately after sending
> a start.
> 
> If it is known during device tree creation that only a single
> I2C master will be present on the bus, this deadlock of the
> I2C bus could be avoided in the driver by ignoring the
> bus_is_busy register of the xiic, since bus can never be
> reserved by any other master.
> 
> This patch adds this support for detecting multi-master flag
> in device tree and when not provided, improves I2C reliability
> by ignoring the therefore unnecessary xiic bus_is_busy register.
> 
> Error can be reproduced by pulling I2C SDA -line temporarily low
> by shorting it to ground, while linux I2C master is operating on
> it using the xiic driver. The application using the bus will
> start receiving linux error code 16: "Device or resource busy"
> indefinitely:
> 
> kernel: pca953x 0-0020: failed writing register
> app: Error writing file, error: 16
> 
> With multi-master disabled device will instead receive error
> code 5: "I/O error" while SDA is grounded, but recover normal
> operation once short is removed.
> 
> kernel: pca953x 0-0020: failed reading register
> app: Error reading file, error: 5
> 
> Signed-off-by: Jaakko Laine <ext-jaakko.laine@vaisala.com>
> ---
> 
> Applies against Linux 5.6-rc1 from master in
> https://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git
> 
> I would like to point out that that since this patch disables
> multimaster mode based on the standard I2C multimaster property
> in device tree (as it propably should) and since the driver has
> previously supported multimaster even when this property doesn't
> exist in device tree, there is a possible backwards
> compatibility issue:
> 
> If there are devices relying on the multimaster mode to work
> without defining the property in device tree, their I2C bus
> might not work without issues anymore after this patch, since
> the driver will asume it is the only master on bus and could
> therefore interrupt the communication of some other master on
> same bus.
> 
> Please suggest some alternative fix if this is not acceptable
> as is. On the other hand supporting multimaster even on a bus
> with only a single master does currently cause some
> reliability issues since the bus can get indefinitely stuck.
> I don't think there exists a I2C protocol compatible way to
> resolve the deadlock on multimaster bus.

Wolfram: I don't think this feature is used on this driver a lot but
clearly this breaks compatibility. Not sure how to handle this properly
and I am fine with this solution.

Shubhrajyoti: Any comment?

> 
>  drivers/i2c/busses/i2c-xiic.c | 52 +++++++++++++++++++++--------------
>  1 file changed, 32 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-xiic.c b/drivers/i2c/busses/i2c-xiic.c
> index 90c1c362394d..37f8d6ee0577 100644
> --- a/drivers/i2c/busses/i2c-xiic.c
> +++ b/drivers/i2c/busses/i2c-xiic.c
> @@ -46,19 +46,20 @@ enum xiic_endian {
>  
>  /**
>   * struct xiic_i2c - Internal representation of the XIIC I2C bus
> - * @dev:	Pointer to device structure
> - * @base:	Memory base of the HW registers
> - * @wait:	Wait queue for callers
> - * @adap:	Kernel adapter representation
> - * @tx_msg:	Messages from above to be sent
> - * @lock:	Mutual exclusion
> - * @tx_pos:	Current pos in TX message
> - * @nmsgs:	Number of messages in tx_msg
> - * @state:	See STATE_
> - * @rx_msg:	Current RX message
> - * @rx_pos:	Position within current RX message
> - * @endianness: big/little-endian byte order
> - * @clk:	Pointer to AXI4-lite input clock
> + * @dev:		Pointer to device structure
> + * @base:		Memory base of the HW registers
> + * @wait:		Wait queue for callers
> + * @adap:		Kernel adapter representation
> + * @tx_msg:		Messages from above to be sent
> + * @lock:		Mutual exclusion
> + * @tx_pos:		Current pos in TX message
> + * @nmsgs:		Number of messages in tx_msg
> + * @state:		See STATE_
> + * @rx_msg:		Current RX message
> + * @rx_pos:		Position within current RX message
> + * @endianness:		big/little-endian byte order
> + * @multimaster:	Indicates bus has multiple masters
> + * @clk:		Pointer to AXI4-lite input clock

nit: I can't see reason for these changes above. I would do it in
separate patch if you want to align.

Thanks,
Michal
Laine Jaakko EXT Feb. 25, 2020, 2:08 p.m. UTC | #2
-----Original Message-----
From: Michal Simek <michal.simek@xilinx.com> 
Sent: Monday, 24 February, 2020 12:14
To: Laine Jaakko EXT <ext-jaakko.laine@vaisala.com>; wsa@the-dreams.de
Cc: linux-i2c@vger.kernel.org; michal.simek@xilinx.com; linux-arm-kernel@lists.infradead.org; Shubhrajyoti Datta <shubhraj@xilinx.com>
Subject: Re: [PATCH] i2c: xiic: Support disabling multi-master in DT

> nit: I can't see reason for these changes above. I would do it in
> separate patch if you want to align.

I tried to preserve the original authors' intention of having the
lines aligned. My new parameter name was 1 character too
long to fit properly in the original space. I don't have strong
preference over aligned vs not.

I will make V2 without aligning new parameter as suggested
if this is otherwise ok. This will reduce changed line count.

Thank you for review,
Jaakko
Wolfram Sang Feb. 25, 2020, 2:13 p.m. UTC | #3
> I will make V2 without aligning new parameter as suggested
> if this is otherwise ok. This will reduce changed line count.

My favourite is to change alignment to be just one space. Then, we have
a bit of overhead now, but never again in the future.
Laine Jaakko EXT Feb. 25, 2020, 2:27 p.m. UTC | #4
-----Original Message-----
From: Wolfram Sang <wsa@the-dreams.de> 
Sent: Tuesday, 25 February, 2020 16:14
To: Laine Jaakko EXT <ext-jaakko.laine@vaisala.com>
Cc: Michal Simek <michal.simek@xilinx.com>; linux-i2c@vger.kernel.org; linux-arm-kernel@lists.infradead.org; Shubhrajyoti Datta <shubhraj@xilinx.com>
Subject: Re: [PATCH] i2c: xiic: Support disabling multi-master in DT

> My favourite is to change alignment to be just one space. Then, we have
> a bit of overhead now, but never again in the future.

Ok, I will add that change as separate patch to V2 patch series.

Thanks,
Jaakko
Wolfram Sang Feb. 25, 2020, 2:32 p.m. UTC | #5
> > My favourite is to change alignment to be just one space. Then, we have
> > a bit of overhead now, but never again in the future.
> 
> Ok, I will add that change as separate patch to V2 patch series.

Perfect, thanks!
Michal Simek Feb. 25, 2020, 2:37 p.m. UTC | #6
On 25. 02. 20 15:32, Wolfram Sang wrote:
> 
>>> My favourite is to change alignment to be just one space. Then, we have
>>> a bit of overhead now, but never again in the future.
>>
>> Ok, I will add that change as separate patch to V2 patch series.
> 
> Perfect, thanks!
> 

Wolfram: Any option about that multi-master property?

M
Wolfram Sang Feb. 25, 2020, 2:41 p.m. UTC | #7
On Tue, Feb 25, 2020 at 03:37:21PM +0100, Michal Simek wrote:
> On 25. 02. 20 15:32, Wolfram Sang wrote:
> > 
> >>> My favourite is to change alignment to be just one space. Then, we have
> >>> a bit of overhead now, but never again in the future.
> >>
> >> Ok, I will add that change as separate patch to V2 patch series.
> > 
> > Perfect, thanks!
> > 
> 
> Wolfram: Any option about that multi-master property?

Not yet.
Michal Simek Feb. 25, 2020, 2:44 p.m. UTC | #8
On 25. 02. 20 15:41, Wolfram Sang wrote:
> On Tue, Feb 25, 2020 at 03:37:21PM +0100, Michal Simek wrote:
>> On 25. 02. 20 15:32, Wolfram Sang wrote:
>>>
>>>>> My favourite is to change alignment to be just one space. Then, we have
>>>>> a bit of overhead now, but never again in the future.
>>>>
>>>> Ok, I will add that change as separate patch to V2 patch series.
>>>
>>> Perfect, thanks!
>>>
>>
>> Wolfram: Any option about that multi-master property?
> 
> Not yet.

What does that mean? Do you need time to dig into it or you don't mind?

Thanks,
Michal
Wolfram Sang Feb. 25, 2020, 2:48 p.m. UTC | #9
> >> Wolfram: Any option about that multi-master property?
> > 
> > Not yet.
> 
> What does that mean? Do you need time to dig into it or you don't mind?

Need more time.
Michal Simek Feb. 25, 2020, 2:49 p.m. UTC | #10
On 25. 02. 20 15:48, Wolfram Sang wrote:
> 
>>>> Wolfram: Any option about that multi-master property?
>>>
>>> Not yet.
>>
>> What does that mean? Do you need time to dig into it or you don't mind?
> 
> Need more time.
> 

ok. Thx.

Thanks,
Michal
Shubhrajyoti Datta March 18, 2020, 12:20 p.m. UTC | #11
HI Laine,

On Tue, Feb 18, 2020 at 7:29 PM Laine Jaakko EXT
<ext-jaakko.laine@vaisala.com> wrote:
>
> I2C master operating in multimaster mode can get stuck
> indefinitely if I2C start is detected on bus, but no master
> has a transaction going.
>
> This is a weakness in I2C standard, which defines no way
> to recover, since all masters are indefinitely disallowed
> from interrupting the currently operating master. A start
> condition can be created for example by an electromagnetic
> discharge applied near physical I2C lines. Or a already
> operating master could get reset immediately after sending
> a start.
>
> If it is known during device tree creation that only a single
> I2C master will be present on the bus, this deadlock of the
> I2C bus could be avoided in the driver by ignoring the
> bus_is_busy register of the xiic, since bus can never be
> reserved by any other master.
>
> This patch adds this support for detecting multi-master flag
> in device tree and when not provided, improves I2C reliability
> by ignoring the therefore unnecessary xiic bus_is_busy register.
>
> Error can be reproduced by pulling I2C SDA -line temporarily low
> by shorting it to ground, while linux I2C master is operating on
> it using the xiic driver. The application using the bus will
> start receiving linux error code 16: "Device or resource busy"
> indefinitely:
>
> kernel: pca953x 0-0020: failed writing register
> app: Error writing file, error: 16
>
> With multi-master disabled device will instead receive error
> code 5: "I/O error" while SDA is grounded, but recover normal
> operation once short is removed.
>
> kernel: pca953x 0-0020: failed reading register
> app: Error reading file, error: 5
>
> Signed-off-by: Jaakko Laine <ext-jaakko.laine@vaisala.com>
> ---
>
> Applies against Linux 5.6-rc1 from master in
> https://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git
>
> I would like to point out that that since this patch disables
> multimaster mode based on the standard I2C multimaster property
> in device tree (as it propably should) and since the driver has
> previously supported multimaster even when this property doesn't
> exist in device tree, there is a possible backwards
> compatibility issue:
>
> If there are devices relying on the multimaster mode to work
> without defining the property in device tree, their I2C bus
> might not work without issues anymore after this patch, since
> the driver will asume it is the only master on bus and could
> therefore interrupt the communication of some other master on
> same bus.
>
> Please suggest some alternative fix if this is not acceptable
> as is. On the other hand supporting multimaster even on a bus
> with only a single master does currently cause some
> reliability issues since the bus can get indefinitely stuck.
> I don't think there exists a I2C protocol compatible way to
> resolve the deadlock on multimaster bus.
>
>  drivers/i2c/busses/i2c-xiic.c | 52 +++++++++++++++++++++--------------
>  1 file changed, 32 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-xiic.c b/drivers/i2c/busses/i2c-xiic.c
> index 90c1c362394d..37f8d6ee0577 100644
> --- a/drivers/i2c/busses/i2c-xiic.c
> +++ b/drivers/i2c/busses/i2c-xiic.c
> @@ -46,19 +46,20 @@ enum xiic_endian {
>
>  /**
>   * struct xiic_i2c - Internal representation of the XIIC I2C bus
> - * @dev:       Pointer to device structure
> - * @base:      Memory base of the HW registers
> - * @wait:      Wait queue for callers
> - * @adap:      Kernel adapter representation
> - * @tx_msg:    Messages from above to be sent
> - * @lock:      Mutual exclusion
> - * @tx_pos:    Current pos in TX message
> - * @nmsgs:     Number of messages in tx_msg
> - * @state:     See STATE_
> - * @rx_msg:    Current RX message
> - * @rx_pos:    Position within current RX message
> - * @endianness: big/little-endian byte order
> - * @clk:       Pointer to AXI4-lite input clock
> + * @dev:               Pointer to device structure
> + * @base:              Memory base of the HW registers
> + * @wait:              Wait queue for callers
> + * @adap:              Kernel adapter representation
> + * @tx_msg:            Messages from above to be sent
> + * @lock:              Mutual exclusion
> + * @tx_pos:            Current pos in TX message
> + * @nmsgs:             Number of messages in tx_msg
> + * @state:             See STATE_
> + * @rx_msg:            Current RX message
> + * @rx_pos:            Position within current RX message
> + * @endianness:                big/little-endian byte order
> + * @multimaster:       Indicates bus has multiple masters
> + * @clk:               Pointer to AXI4-lite input clock
>   */
>  struct xiic_i2c {
>         struct device           *dev;
> @@ -73,6 +74,7 @@ struct xiic_i2c {
>         struct i2c_msg          *rx_msg;
>         int                     rx_pos;
>         enum xiic_endian        endianness;
> +       bool                    multimaster;
>         struct clk *clk;
>  };
>
> @@ -521,19 +523,26 @@ static int xiic_bus_busy(struct xiic_i2c *i2c)
>  static int xiic_busy(struct xiic_i2c *i2c)
>  {
>         int tries = 3;
> -       int err;
> +       int err = 0;
>
>         if (i2c->tx_msg)
>                 return -EBUSY;
>
> -       /* for instance if previous transfer was terminated due to TX error
> -        * it might be that the bus is on it's way to become available
> -        * give it at most 3 ms to wake
> +       /* In single master mode bus can only be busy, when in use by this
> +        * driver. If the register indicates bus being busy for some reason we
> +        * should ignore it, since bus will never be released and i2c will be
> +        * stuck forever.
>          */

the other thing i was thinking how will multithreading .
Should we have a lock here.

> -       err = xiic_bus_busy(i2c);
> -       while (err && tries--) {
> -               msleep(1);
> +       if (i2c->multimaster) {
> +               /* for instance if previous transfer was terminated due to TX
> +                * error it might be that the bus is on it's way to become
> +                * available give it at most 3 ms to wake
> +                */
>                 err = xiic_bus_busy(i2c);
> +               while (err && tries--) {
> +                       msleep(1);
> +                       err = xiic_bus_busy(i2c);
> +               }
>         }
>
>         return err;
> @@ -811,6 +820,9 @@ static int xiic_i2c_probe(struct platform_device *pdev)
>                 goto err_clk_dis;
>         }
>
> +       i2c->multimaster =
> +               of_property_read_bool(pdev->dev.of_node, "multi-master");
> +
Current will default to mustimaster is 0.
May be the default should be 1 if not specified.
>         /*
>          * Detect endianness
>          * Try to reset the TX FIFO. Then check the EMPTY flag. If it is not
> --
> 2.19.1
>
Laine Jaakko EXT March 18, 2020, 4:49 p.m. UTC | #12
Hello,

>> @@ -521,19 +523,26 @@ static int xiic_bus_busy(struct xiic_i2c *i2c)
>>  static int xiic_busy(struct xiic_i2c *i2c)
>>  {
>>         int tries = 3;
>> -       int err;
>> +       int err = 0;
>>
>>         if (i2c->tx_msg)
>>                 return -EBUSY;
>>
>> -       /* for instance if previous transfer was terminated due to TX error
>> -        * it might be that the bus is on it's way to become available
>> -        * give it at most 3 ms to wake
>> +       /* In single master mode bus can only be busy, when in use by this
>> +        * driver. If the register indicates bus being busy for some reason we
>> +        * should ignore it, since bus will never be released and i2c will be
>> +        * stuck forever.
>>          */
>
>the other thing i was thinking how will multithreading .
>Should we have a lock here.
>
>> -       err = xiic_bus_busy(i2c);
>> -       while (err && tries--) {
>> -               msleep(1);
>> +       if (i2c->multimaster) {
>> +               /* for instance if previous transfer was terminated due to TX
>> +                * error it might be that the bus is on it's way to become
>> +                * available give it at most 3 ms to wake
>> +                */
>>                 err = xiic_bus_busy(i2c);
>> +               while (err && tries--) {
>> +                       msleep(1);
>> +                       err = xiic_bus_busy(i2c);
>> +               }
>>         }
>>
>>         return err;

Which resource specifically are you worried about needing locking here?

I don't think this patch introduces any new need for locking. Only new parameter, which wasn't accessed already is i2c->multimaster, which is a constant that is never changed after driver is loaded.
If i2c->multimaster, needed locking i2c->tx_msg would have needed it already before, since it is a parameter in the same struct and can actually get changed by some other thread.
In this section the only variables written to are local to the function. Shared variables are only read from, which seems pretty safe to me if considering this function alone.

However, now that you mention it multiple threads could be checking i2c->tx_msg at the same time inside this function or waiting for xiic_bus_busy(i2c) to not be busy anymore.
Since in "static int xiic_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)" i2c->tx_msg is written with data before any locking, multiple threads could exit "xiic_busy(struct xiic_i2c *i2c)" and write their stuff to i2c->tx_msg, since buffer being empty was checked before anyone had a chance to write to it. If this happens, some data to be transmitted could be lost when i2c->tx_msg gets overwritten multiple times before data gets transmitted. This issue did already exist before, but it looks like it should be fixed to me.

Fixing would need locking here, but the possible msleep(1) -calls inside xiic_busy seem like an issue, so some more changes needed:
// lock here
err = xiic_busy(i2c);
if (err)
              // unlock here
	goto out;
i2c->tx_msg = msgs;
i2c->nmsgs = num;
// unlock here

>> +       i2c->multimaster =
>> +               of_property_read_bool(pdev->dev.of_node, "multi-master");
>> +
>Current will default to mustimaster is 0.
>May be the default should be 1 if not specified.

The multi-master -binding is documented here as boolean and encodes a Boolean by either existing or not existing in device tree.
It is also used in other drivers so I couldn't do much about it missing meaning False.
https://github.com/torvalds/linux/blob/master/Documentation/devicetree/bindings/i2c/i2c.txt
I originally had a custom device tree entry where the default was for multi-master to be enabled before I noticed the pre-existing binding.

Maybe if the multi-master binding was changed from Boolean to for example a string property (multi-master = "ON" / multi-master = "OFF"), code could still just check the existence with "of_property_read_bool()" first, where property missing means "OFF" and property existing means "ON"(like before) if there is no text associated. Xiic driver would then only disable multimaster, if device tree explicitly contains multi-master = "OFF".

This should be able to maintain driver backwards compatibility with old device trees, but requires binding documentation change and all drivers should likely be updated to also accept the new style of multi-master property to be consistent. This is also not as clean as the old Boolean property in my opinion.

Thank you for comments,
Jaakko
Shubhrajyoti Datta March 19, 2020, 9:34 a.m. UTC | #13
Hi Jakko,

On Wed, Mar 18, 2020 at 10:19 PM Laine Jaakko EXT
<ext-jaakko.laine@vaisala.com> wrote:
>
> Hello,
>
> >> @@ -521,19 +523,26 @@ static int xiic_bus_busy(struct xiic_i2c *i2c)
> >>  static int xiic_busy(struct xiic_i2c *i2c)
> >>  {
> >>         int tries = 3;
> >> -       int err;
> >> +       int err = 0;
> >>
> >>         if (i2c->tx_msg)
> >>                 return -EBUSY;
> >>
> >> -       /* for instance if previous transfer was terminated due to TX error
> >> -        * it might be that the bus is on it's way to become available
> >> -        * give it at most 3 ms to wake
> >> +       /* In single master mode bus can only be busy, when in use by this
> >> +        * driver. If the register indicates bus being busy for some reason we
> >> +        * should ignore it, since bus will never be released and i2c will be
> >> +        * stuck forever.
> >>          */
> >
> >the other thing i was thinking how will multithreading .
> >Should we have a lock here.
> >
> >> -       err = xiic_bus_busy(i2c);
> >> -       while (err && tries--) {
> >> -               msleep(1);
> >> +       if (i2c->multimaster) {
> >> +               /* for instance if previous transfer was terminated due to TX
> >> +                * error it might be that the bus is on it's way to become
> >> +                * available give it at most 3 ms to wake
> >> +                */
> >>                 err = xiic_bus_busy(i2c);
> >> +               while (err && tries--) {
> >> +                       msleep(1);
> >> +                       err = xiic_bus_busy(i2c);
> >> +               }
> >>         }
> >>
> >>         return err;
>
> Which resource specifically are you worried about needing locking here?
>
Earlier multiple threads on the same processor will wait for bus busy.

Now my concern was

thread1 -> makes a transaction

thread2  -> this will not wait for bus busy and access.
Laine Jaakko EXT March 19, 2020, 10:26 a.m. UTC | #14
> > >
> > >the other thing i was thinking how will multithreading .
> > >Should we have a lock here.
> > >
> > >> -       err = xiic_bus_busy(i2c);
> > >> -       while (err && tries--) {
> > >> -               msleep(1);
> > >> +       if (i2c->multimaster) {
> > >> +               /* for instance if previous transfer was terminated due to TX
> > >> +                * error it might be that the bus is on it's way to become
> > >> +                * available give it at most 3 ms to wake
> > >> +                */
> > >>                 err = xiic_bus_busy(i2c);
> > >> +               while (err && tries--) {
> > >> +                       msleep(1);
> > >> +                       err = xiic_bus_busy(i2c);
> > >> +               }
> > >>         }
> > >>
> > >>         return err;
> >
> > Which resource specifically are you worried about needing locking here?
> >
> Earlier multiple threads on the same processor will wait for bus busy.
>
> Now my concern was
>
> thread1 -> makes a transaction
>
> thread2  -> this will not wait for bus busy and access.

Since i2c->tx_msg is set before anything is sent to FPGA and only returned to NULL after transaction has finished,
I think thread2 would already exit with -EBUSY before xiic_bus_busy(i2c) is called because off:
if (i2c->tx_msg)
	return -EBUSY;
in same function.

This is why my understanding is that xiic_bus_busy(i2c) only practically guards against other masters operating on bus.
In my understanding xiic_bus_busy(i2c) reads the register on FPGA, which can't change state before thread1 is already so far into transmitting its data that FPGA has received something to send and has reserved the bus. This would leave an interval of time between checking xiic_bus_busy and its register value changing during which thread2 could also check xiic_bus_busy and proceed to transmit at the same time with thread1. (Until hitting a transaction lock later, but only after it has already overwritten the pointer to transmit buffer i2c->tx_msg, and possibly messed up the transmissions for thread1).

Now it seems to me that even with i2c->tx_msg being checked, thread2 could get past it before thread1 has set it to not NULL, since thread performs no locking between checking it and setting it, like I mentioned in previous reply. This issue has apparently already existed for some time though and is probably quite unlikely, since it has been there for some time.

-Jaakko

Patch
diff mbox series

diff --git a/drivers/i2c/busses/i2c-xiic.c b/drivers/i2c/busses/i2c-xiic.c
index 90c1c362394d..37f8d6ee0577 100644
--- a/drivers/i2c/busses/i2c-xiic.c
+++ b/drivers/i2c/busses/i2c-xiic.c
@@ -46,19 +46,20 @@  enum xiic_endian {
 
 /**
  * struct xiic_i2c - Internal representation of the XIIC I2C bus
- * @dev:	Pointer to device structure
- * @base:	Memory base of the HW registers
- * @wait:	Wait queue for callers
- * @adap:	Kernel adapter representation
- * @tx_msg:	Messages from above to be sent
- * @lock:	Mutual exclusion
- * @tx_pos:	Current pos in TX message
- * @nmsgs:	Number of messages in tx_msg
- * @state:	See STATE_
- * @rx_msg:	Current RX message
- * @rx_pos:	Position within current RX message
- * @endianness: big/little-endian byte order
- * @clk:	Pointer to AXI4-lite input clock
+ * @dev:		Pointer to device structure
+ * @base:		Memory base of the HW registers
+ * @wait:		Wait queue for callers
+ * @adap:		Kernel adapter representation
+ * @tx_msg:		Messages from above to be sent
+ * @lock:		Mutual exclusion
+ * @tx_pos:		Current pos in TX message
+ * @nmsgs:		Number of messages in tx_msg
+ * @state:		See STATE_
+ * @rx_msg:		Current RX message
+ * @rx_pos:		Position within current RX message
+ * @endianness:		big/little-endian byte order
+ * @multimaster:	Indicates bus has multiple masters
+ * @clk:		Pointer to AXI4-lite input clock
  */
 struct xiic_i2c {
 	struct device		*dev;
@@ -73,6 +74,7 @@  struct xiic_i2c {
 	struct i2c_msg		*rx_msg;
 	int			rx_pos;
 	enum xiic_endian	endianness;
+	bool			multimaster;
 	struct clk *clk;
 };
 
@@ -521,19 +523,26 @@  static int xiic_bus_busy(struct xiic_i2c *i2c)
 static int xiic_busy(struct xiic_i2c *i2c)
 {
 	int tries = 3;
-	int err;
+	int err = 0;
 
 	if (i2c->tx_msg)
 		return -EBUSY;
 
-	/* for instance if previous transfer was terminated due to TX error
-	 * it might be that the bus is on it's way to become available
-	 * give it at most 3 ms to wake
+	/* In single master mode bus can only be busy, when in use by this
+	 * driver. If the register indicates bus being busy for some reason we
+	 * should ignore it, since bus will never be released and i2c will be
+	 * stuck forever.
 	 */
-	err = xiic_bus_busy(i2c);
-	while (err && tries--) {
-		msleep(1);
+	if (i2c->multimaster) {
+		/* for instance if previous transfer was terminated due to TX
+		 * error it might be that the bus is on it's way to become
+		 * available give it at most 3 ms to wake
+		 */
 		err = xiic_bus_busy(i2c);
+		while (err && tries--) {
+			msleep(1);
+			err = xiic_bus_busy(i2c);
+		}
 	}
 
 	return err;
@@ -811,6 +820,9 @@  static int xiic_i2c_probe(struct platform_device *pdev)
 		goto err_clk_dis;
 	}
 
+	i2c->multimaster =
+		of_property_read_bool(pdev->dev.of_node, "multi-master");
+
 	/*
 	 * Detect endianness
 	 * Try to reset the TX FIFO. Then check the EMPTY flag. If it is not