All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] i2c: xiic: Support disabling multi-master in DT
@ 2020-02-18 13:58 ` Laine Jaakko EXT
  0 siblings, 0 replies; 44+ messages in thread
From: Laine Jaakko EXT @ 2020-02-18 13:58 UTC (permalink / raw)
  To: wsa; +Cc: linux-i2c, michal.simek, linux-arm-kernel, Laine Jaakko EXT

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.
 	 */
-	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
-- 
2.19.1

^ permalink raw reply related	[flat|nested] 44+ messages in thread

* [PATCH] i2c: xiic: Support disabling multi-master in DT
@ 2020-02-18 13:58 ` Laine Jaakko EXT
  0 siblings, 0 replies; 44+ messages in thread
From: Laine Jaakko EXT @ 2020-02-18 13:58 UTC (permalink / raw)
  To: wsa; +Cc: Laine Jaakko EXT, linux-i2c, linux-arm-kernel, michal.simek

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.
 	 */
-	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
-- 
2.19.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply related	[flat|nested] 44+ messages in thread

* Re: [PATCH] i2c: xiic: Support disabling multi-master in DT
  2020-02-18 13:58 ` Laine Jaakko EXT
@ 2020-02-24 10:13   ` Michal Simek
  -1 siblings, 0 replies; 44+ messages in thread
From: Michal Simek @ 2020-02-24 10:13 UTC (permalink / raw)
  To: Laine Jaakko EXT, wsa
  Cc: linux-i2c, michal.simek, linux-arm-kernel, Shubhrajyoti Datta

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

^ permalink raw reply	[flat|nested] 44+ messages in thread

* Re: [PATCH] i2c: xiic: Support disabling multi-master in DT
@ 2020-02-24 10:13   ` Michal Simek
  0 siblings, 0 replies; 44+ messages in thread
From: Michal Simek @ 2020-02-24 10:13 UTC (permalink / raw)
  To: Laine Jaakko EXT, wsa
  Cc: Shubhrajyoti Datta, linux-i2c, linux-arm-kernel, michal.simek

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 44+ messages in thread

* RE: [PATCH] i2c: xiic: Support disabling multi-master in DT
  2020-02-24 10:13   ` Michal Simek
@ 2020-02-25 14:08     ` Laine Jaakko EXT
  -1 siblings, 0 replies; 44+ messages in thread
From: Laine Jaakko EXT @ 2020-02-25 14:08 UTC (permalink / raw)
  To: Michal Simek, wsa; +Cc: linux-i2c, linux-arm-kernel, Shubhrajyoti Datta

-----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

^ permalink raw reply	[flat|nested] 44+ messages in thread

* RE: [PATCH] i2c: xiic: Support disabling multi-master in DT
@ 2020-02-25 14:08     ` Laine Jaakko EXT
  0 siblings, 0 replies; 44+ messages in thread
From: Laine Jaakko EXT @ 2020-02-25 14:08 UTC (permalink / raw)
  To: Michal Simek, wsa; +Cc: linux-i2c, linux-arm-kernel, Shubhrajyoti Datta

-----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
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 44+ messages in thread

* Re: [PATCH] i2c: xiic: Support disabling multi-master in DT
  2020-02-25 14:08     ` Laine Jaakko EXT
@ 2020-02-25 14:13       ` Wolfram Sang
  -1 siblings, 0 replies; 44+ messages in thread
From: Wolfram Sang @ 2020-02-25 14:13 UTC (permalink / raw)
  To: Laine Jaakko EXT
  Cc: Michal Simek, linux-i2c, linux-arm-kernel, Shubhrajyoti Datta

[-- Attachment #1: Type: text/plain, Size: 254 bytes --]


> 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.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 44+ messages in thread

* Re: [PATCH] i2c: xiic: Support disabling multi-master in DT
@ 2020-02-25 14:13       ` Wolfram Sang
  0 siblings, 0 replies; 44+ messages in thread
From: Wolfram Sang @ 2020-02-25 14:13 UTC (permalink / raw)
  To: Laine Jaakko EXT
  Cc: Shubhrajyoti Datta, Michal Simek, linux-arm-kernel, linux-i2c


[-- Attachment #1.1: Type: text/plain, Size: 254 bytes --]


> 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.


[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 44+ messages in thread

* RE: [PATCH] i2c: xiic: Support disabling multi-master in DT
  2020-02-25 14:13       ` Wolfram Sang
@ 2020-02-25 14:27         ` Laine Jaakko EXT
  -1 siblings, 0 replies; 44+ messages in thread
From: Laine Jaakko EXT @ 2020-02-25 14:27 UTC (permalink / raw)
  To: Wolfram Sang
  Cc: Michal Simek, linux-i2c, linux-arm-kernel, Shubhrajyoti Datta

-----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

^ permalink raw reply	[flat|nested] 44+ messages in thread

* RE: [PATCH] i2c: xiic: Support disabling multi-master in DT
@ 2020-02-25 14:27         ` Laine Jaakko EXT
  0 siblings, 0 replies; 44+ messages in thread
From: Laine Jaakko EXT @ 2020-02-25 14:27 UTC (permalink / raw)
  To: Wolfram Sang
  Cc: Shubhrajyoti Datta, Michal Simek, linux-arm-kernel, linux-i2c

-----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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 44+ messages in thread

* Re: [PATCH] i2c: xiic: Support disabling multi-master in DT
  2020-02-25 14:27         ` Laine Jaakko EXT
@ 2020-02-25 14:32           ` Wolfram Sang
  -1 siblings, 0 replies; 44+ messages in thread
From: Wolfram Sang @ 2020-02-25 14:32 UTC (permalink / raw)
  To: Laine Jaakko EXT
  Cc: Michal Simek, linux-i2c, linux-arm-kernel, Shubhrajyoti Datta

[-- Attachment #1: Type: text/plain, Size: 230 bytes --]


> > 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!

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 44+ messages in thread

* Re: [PATCH] i2c: xiic: Support disabling multi-master in DT
@ 2020-02-25 14:32           ` Wolfram Sang
  0 siblings, 0 replies; 44+ messages in thread
From: Wolfram Sang @ 2020-02-25 14:32 UTC (permalink / raw)
  To: Laine Jaakko EXT
  Cc: Shubhrajyoti Datta, Michal Simek, linux-arm-kernel, linux-i2c


[-- Attachment #1.1: Type: text/plain, Size: 230 bytes --]


> > 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!

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 44+ messages in thread

* Re: [PATCH] i2c: xiic: Support disabling multi-master in DT
  2020-02-25 14:32           ` Wolfram Sang
@ 2020-02-25 14:37             ` Michal Simek
  -1 siblings, 0 replies; 44+ messages in thread
From: Michal Simek @ 2020-02-25 14:37 UTC (permalink / raw)
  To: Wolfram Sang, Laine Jaakko EXT
  Cc: Michal Simek, linux-i2c, linux-arm-kernel, Shubhrajyoti Datta

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

^ permalink raw reply	[flat|nested] 44+ messages in thread

* Re: [PATCH] i2c: xiic: Support disabling multi-master in DT
@ 2020-02-25 14:37             ` Michal Simek
  0 siblings, 0 replies; 44+ messages in thread
From: Michal Simek @ 2020-02-25 14:37 UTC (permalink / raw)
  To: Wolfram Sang, Laine Jaakko EXT
  Cc: Shubhrajyoti Datta, Michal Simek, linux-arm-kernel, linux-i2c

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 44+ messages in thread

* Re: [PATCH] i2c: xiic: Support disabling multi-master in DT
  2020-02-25 14:37             ` Michal Simek
@ 2020-02-25 14:41               ` Wolfram Sang
  -1 siblings, 0 replies; 44+ messages in thread
From: Wolfram Sang @ 2020-02-25 14:41 UTC (permalink / raw)
  To: Michal Simek
  Cc: Laine Jaakko EXT, linux-i2c, linux-arm-kernel, Shubhrajyoti Datta

[-- Attachment #1: Type: text/plain, Size: 439 bytes --]

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.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 44+ messages in thread

* Re: [PATCH] i2c: xiic: Support disabling multi-master in DT
@ 2020-02-25 14:41               ` Wolfram Sang
  0 siblings, 0 replies; 44+ messages in thread
From: Wolfram Sang @ 2020-02-25 14:41 UTC (permalink / raw)
  To: Michal Simek
  Cc: Laine Jaakko EXT, linux-i2c, linux-arm-kernel, Shubhrajyoti Datta


[-- Attachment #1.1: Type: text/plain, Size: 439 bytes --]

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.


[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 44+ messages in thread

* Re: [PATCH] i2c: xiic: Support disabling multi-master in DT
  2020-02-25 14:41               ` Wolfram Sang
@ 2020-02-25 14:44                 ` Michal Simek
  -1 siblings, 0 replies; 44+ messages in thread
From: Michal Simek @ 2020-02-25 14:44 UTC (permalink / raw)
  To: Wolfram Sang, Michal Simek
  Cc: Laine Jaakko EXT, linux-i2c, linux-arm-kernel, Shubhrajyoti Datta

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

^ permalink raw reply	[flat|nested] 44+ messages in thread

* Re: [PATCH] i2c: xiic: Support disabling multi-master in DT
@ 2020-02-25 14:44                 ` Michal Simek
  0 siblings, 0 replies; 44+ messages in thread
From: Michal Simek @ 2020-02-25 14:44 UTC (permalink / raw)
  To: Wolfram Sang, Michal Simek
  Cc: Laine Jaakko EXT, linux-i2c, linux-arm-kernel, Shubhrajyoti Datta

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



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 44+ messages in thread

* Re: [PATCH] i2c: xiic: Support disabling multi-master in DT
  2020-02-25 14:44                 ` Michal Simek
@ 2020-02-25 14:48                   ` Wolfram Sang
  -1 siblings, 0 replies; 44+ messages in thread
From: Wolfram Sang @ 2020-02-25 14:48 UTC (permalink / raw)
  To: Michal Simek
  Cc: Laine Jaakko EXT, linux-i2c, linux-arm-kernel, Shubhrajyoti Datta

[-- Attachment #1: Type: text/plain, Size: 182 bytes --]


> >> 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.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 44+ messages in thread

* Re: [PATCH] i2c: xiic: Support disabling multi-master in DT
@ 2020-02-25 14:48                   ` Wolfram Sang
  0 siblings, 0 replies; 44+ messages in thread
From: Wolfram Sang @ 2020-02-25 14:48 UTC (permalink / raw)
  To: Michal Simek
  Cc: Laine Jaakko EXT, linux-i2c, linux-arm-kernel, Shubhrajyoti Datta


[-- Attachment #1.1: Type: text/plain, Size: 182 bytes --]


> >> 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.


[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 44+ messages in thread

* Re: [PATCH] i2c: xiic: Support disabling multi-master in DT
  2020-02-25 14:48                   ` Wolfram Sang
@ 2020-02-25 14:49                     ` Michal Simek
  -1 siblings, 0 replies; 44+ messages in thread
From: Michal Simek @ 2020-02-25 14:49 UTC (permalink / raw)
  To: Wolfram Sang, Michal Simek
  Cc: Laine Jaakko EXT, linux-i2c, linux-arm-kernel, Shubhrajyoti Datta

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

^ permalink raw reply	[flat|nested] 44+ messages in thread

* Re: [PATCH] i2c: xiic: Support disabling multi-master in DT
@ 2020-02-25 14:49                     ` Michal Simek
  0 siblings, 0 replies; 44+ messages in thread
From: Michal Simek @ 2020-02-25 14:49 UTC (permalink / raw)
  To: Wolfram Sang, Michal Simek
  Cc: Laine Jaakko EXT, linux-i2c, linux-arm-kernel, Shubhrajyoti Datta

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 44+ messages in thread

* Re: [PATCH] i2c: xiic: Support disabling multi-master in DT
  2020-02-18 13:58 ` Laine Jaakko EXT
@ 2020-03-18 12:20   ` Shubhrajyoti Datta
  -1 siblings, 0 replies; 44+ messages in thread
From: Shubhrajyoti Datta @ 2020-03-18 12:20 UTC (permalink / raw)
  To: Laine Jaakko EXT; +Cc: wsa, linux-i2c, michal.simek, linux-arm-kernel

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
>

^ permalink raw reply	[flat|nested] 44+ messages in thread

* Re: [PATCH] i2c: xiic: Support disabling multi-master in DT
@ 2020-03-18 12:20   ` Shubhrajyoti Datta
  0 siblings, 0 replies; 44+ messages in thread
From: Shubhrajyoti Datta @ 2020-03-18 12:20 UTC (permalink / raw)
  To: Laine Jaakko EXT; +Cc: michal.simek, linux-i2c, linux-arm-kernel, wsa

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
>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 44+ messages in thread

* RE: [PATCH] i2c: xiic: Support disabling multi-master in DT
  2020-03-18 12:20   ` Shubhrajyoti Datta
@ 2020-03-18 16:49     ` Laine Jaakko EXT
  -1 siblings, 0 replies; 44+ messages in thread
From: Laine Jaakko EXT @ 2020-03-18 16:49 UTC (permalink / raw)
  To: Shubhrajyoti Datta; +Cc: wsa, linux-i2c, michal.simek, linux-arm-kernel

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

^ permalink raw reply	[flat|nested] 44+ messages in thread

* RE: [PATCH] i2c: xiic: Support disabling multi-master in DT
@ 2020-03-18 16:49     ` Laine Jaakko EXT
  0 siblings, 0 replies; 44+ messages in thread
From: Laine Jaakko EXT @ 2020-03-18 16:49 UTC (permalink / raw)
  To: Shubhrajyoti Datta; +Cc: michal.simek, linux-i2c, linux-arm-kernel, wsa

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
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 44+ messages in thread

* Re: [PATCH] i2c: xiic: Support disabling multi-master in DT
  2020-03-18 16:49     ` Laine Jaakko EXT
@ 2020-03-19  9:34       ` Shubhrajyoti Datta
  -1 siblings, 0 replies; 44+ messages in thread
From: Shubhrajyoti Datta @ 2020-03-19  9:34 UTC (permalink / raw)
  To: Laine Jaakko EXT; +Cc: wsa, linux-i2c, michal.simek, linux-arm-kernel

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.

^ permalink raw reply	[flat|nested] 44+ messages in thread

* Re: [PATCH] i2c: xiic: Support disabling multi-master in DT
@ 2020-03-19  9:34       ` Shubhrajyoti Datta
  0 siblings, 0 replies; 44+ messages in thread
From: Shubhrajyoti Datta @ 2020-03-19  9:34 UTC (permalink / raw)
  To: Laine Jaakko EXT; +Cc: michal.simek, linux-i2c, linux-arm-kernel, wsa

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.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 44+ messages in thread

* RE: [PATCH] i2c: xiic: Support disabling multi-master in DT
  2020-03-19  9:34       ` Shubhrajyoti Datta
@ 2020-03-19 10:26         ` Laine Jaakko EXT
  -1 siblings, 0 replies; 44+ messages in thread
From: Laine Jaakko EXT @ 2020-03-19 10:26 UTC (permalink / raw)
  To: Shubhrajyoti Datta; +Cc: wsa, linux-i2c, michal.simek, linux-arm-kernel

> > >
> > >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

^ permalink raw reply	[flat|nested] 44+ messages in thread

* RE: [PATCH] i2c: xiic: Support disabling multi-master in DT
@ 2020-03-19 10:26         ` Laine Jaakko EXT
  0 siblings, 0 replies; 44+ messages in thread
From: Laine Jaakko EXT @ 2020-03-19 10:26 UTC (permalink / raw)
  To: Shubhrajyoti Datta; +Cc: michal.simek, linux-i2c, linux-arm-kernel, wsa

> > >
> > >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
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 44+ messages in thread

* Re: [PATCH] i2c: xiic: Support disabling multi-master in DT
  2020-03-18 16:49     ` Laine Jaakko EXT
@ 2020-04-01 14:41       ` Wolfram Sang
  -1 siblings, 0 replies; 44+ messages in thread
From: Wolfram Sang @ 2020-04-01 14:41 UTC (permalink / raw)
  To: Laine Jaakko EXT
  Cc: Shubhrajyoti Datta, linux-i2c, michal.simek, linux-arm-kernel

[-- Attachment #1: Type: text/plain, Size: 1678 bytes --]


> 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.

I agree. I don't want to change the old "multi-master" binding like
above because that would be quite intrusive for other drivers and
confusing when trying to understand the binding.

My best bet is to introduce another binding "single-master" which says
clearly that we are the only bus master on that bus.

Both bindings missing means then "unclear".

I think this matches reality best.

Opinions?


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 44+ messages in thread

* Re: [PATCH] i2c: xiic: Support disabling multi-master in DT
@ 2020-04-01 14:41       ` Wolfram Sang
  0 siblings, 0 replies; 44+ messages in thread
From: Wolfram Sang @ 2020-04-01 14:41 UTC (permalink / raw)
  To: Laine Jaakko EXT
  Cc: michal.simek, Shubhrajyoti Datta, linux-arm-kernel, linux-i2c


[-- Attachment #1.1: Type: text/plain, Size: 1678 bytes --]


> 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.

I agree. I don't want to change the old "multi-master" binding like
above because that would be quite intrusive for other drivers and
confusing when trying to understand the binding.

My best bet is to introduce another binding "single-master" which says
clearly that we are the only bus master on that bus.

Both bindings missing means then "unclear".

I think this matches reality best.

Opinions?


[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 44+ messages in thread

* RE: [PATCH] i2c: xiic: Support disabling multi-master in DT
  2020-04-01 14:41       ` Wolfram Sang
@ 2020-04-02  6:16         ` Laine Jaakko EXT
  -1 siblings, 0 replies; 44+ messages in thread
From: Laine Jaakko EXT @ 2020-04-02  6:16 UTC (permalink / raw)
  To: Wolfram Sang
  Cc: Shubhrajyoti Datta, linux-i2c, michal.simek, linux-arm-kernel

> > 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.
>
> I agree. I don't want to change the old "multi-master" binding like
> above because that would be quite intrusive for other drivers and
> confusing when trying to understand the binding.
>
> My best bet is to introduce another binding "single-master" which says
> clearly that we are the only bus master on that bus.
>
> Both bindings missing means then "unclear".
>
> I think this matches reality best.
>
> Opinions?

I agree that this sounds like the best option if original binding can't be used, even though it can also be a bit confusing to have 2 similar bindings.
How would both bindings existing simultaneously be interpreted? Maybe both existing simultaneously should be considered as an invalid configuration, so that it would be enough to just check the one you need? The other option would be to treat both existing similarly to neither existing, which would require the driver to always check both if checking one.

Should the new single-master binding also be a general binding for all I2C drivers or a binding just defined for the XIIC driver? Having it as a general binding for all drivers could help with similar issues rising later with some other driver. It seems possible that a similar issue could happen with some other driver implementing multi-master support, but I have no experience with using or testing them.

- Jaakko

^ permalink raw reply	[flat|nested] 44+ messages in thread

* RE: [PATCH] i2c: xiic: Support disabling multi-master in DT
@ 2020-04-02  6:16         ` Laine Jaakko EXT
  0 siblings, 0 replies; 44+ messages in thread
From: Laine Jaakko EXT @ 2020-04-02  6:16 UTC (permalink / raw)
  To: Wolfram Sang
  Cc: michal.simek, Shubhrajyoti Datta, linux-arm-kernel, linux-i2c

> > 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.
>
> I agree. I don't want to change the old "multi-master" binding like
> above because that would be quite intrusive for other drivers and
> confusing when trying to understand the binding.
>
> My best bet is to introduce another binding "single-master" which says
> clearly that we are the only bus master on that bus.
>
> Both bindings missing means then "unclear".
>
> I think this matches reality best.
>
> Opinions?

I agree that this sounds like the best option if original binding can't be used, even though it can also be a bit confusing to have 2 similar bindings.
How would both bindings existing simultaneously be interpreted? Maybe both existing simultaneously should be considered as an invalid configuration, so that it would be enough to just check the one you need? The other option would be to treat both existing similarly to neither existing, which would require the driver to always check both if checking one.

Should the new single-master binding also be a general binding for all I2C drivers or a binding just defined for the XIIC driver? Having it as a general binding for all drivers could help with similar issues rising later with some other driver. It seems possible that a similar issue could happen with some other driver implementing multi-master support, but I have no experience with using or testing them.

- Jaakko

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 44+ messages in thread

* Re: [PATCH] i2c: xiic: Support disabling multi-master in DT
  2020-04-02  6:16         ` Laine Jaakko EXT
@ 2020-04-02  9:28           ` Wolfram Sang
  -1 siblings, 0 replies; 44+ messages in thread
From: Wolfram Sang @ 2020-04-02  9:28 UTC (permalink / raw)
  To: Laine Jaakko EXT, Rob Herring
  Cc: Shubhrajyoti Datta, linux-i2c, michal.simek, linux-arm-kernel,
	devicetree

[-- Attachment #1: Type: text/plain, Size: 1471 bytes --]

Hi Jaako,

> > My best bet is to introduce another binding "single-master" which says
> > clearly that we are the only bus master on that bus.
> >
> > Both bindings missing means then "unclear".
> >
> > I think this matches reality best.
> >
> > Opinions?

> I agree that this sounds like the best option if original binding
> can't be used, even though it can also be a bit confusing to have 2
> similar bindings.

I think it becomes understandable if we emphasize that "no bindings"
means "unclear". We need to document it.

> How would both bindings existing simultaneously be interpreted? Maybe
> both existing simultaneously should be considered as an invalid
> configuration, so that it would be enough to just check the one you
> need? The other option would be to treat both existing similarly to
> neither existing, which would require the driver to always check both
> if checking one.

I am clearly for saying that this is an illegal combination. I'd hope
this can be expressed in a YAML binding. Yet, my research didn't give me
an answer. Adding Rob and DT list to CC. Question is:

Can we check if the boolean bindings "multi-master" and "single-master"
are not applied at the same time? Any other combination is okay, i.e.
just one of them or none of them.

> Should the new single-master binding also be a general binding for all
> I2C drivers or a binding just defined for the XIIC driver? Having it

It should definately be global.

Thanks,

   Wolfram


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 44+ messages in thread

* Re: [PATCH] i2c: xiic: Support disabling multi-master in DT
@ 2020-04-02  9:28           ` Wolfram Sang
  0 siblings, 0 replies; 44+ messages in thread
From: Wolfram Sang @ 2020-04-02  9:28 UTC (permalink / raw)
  To: Laine Jaakko EXT, Rob Herring
  Cc: devicetree, michal.simek, Shubhrajyoti Datta, linux-arm-kernel,
	linux-i2c


[-- Attachment #1.1: Type: text/plain, Size: 1471 bytes --]

Hi Jaako,

> > My best bet is to introduce another binding "single-master" which says
> > clearly that we are the only bus master on that bus.
> >
> > Both bindings missing means then "unclear".
> >
> > I think this matches reality best.
> >
> > Opinions?

> I agree that this sounds like the best option if original binding
> can't be used, even though it can also be a bit confusing to have 2
> similar bindings.

I think it becomes understandable if we emphasize that "no bindings"
means "unclear". We need to document it.

> How would both bindings existing simultaneously be interpreted? Maybe
> both existing simultaneously should be considered as an invalid
> configuration, so that it would be enough to just check the one you
> need? The other option would be to treat both existing similarly to
> neither existing, which would require the driver to always check both
> if checking one.

I am clearly for saying that this is an illegal combination. I'd hope
this can be expressed in a YAML binding. Yet, my research didn't give me
an answer. Adding Rob and DT list to CC. Question is:

Can we check if the boolean bindings "multi-master" and "single-master"
are not applied at the same time? Any other combination is okay, i.e.
just one of them or none of them.

> Should the new single-master binding also be a general binding for all
> I2C drivers or a binding just defined for the XIIC driver? Having it

It should definately be global.

Thanks,

   Wolfram


[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 44+ messages in thread

* RE: [PATCH] i2c: xiic: Support disabling multi-master in DT
  2020-03-19 10:26         ` Laine Jaakko EXT
@ 2020-04-16 10:42           ` Laine Jaakko EXT
  -1 siblings, 0 replies; 44+ messages in thread
From: Laine Jaakko EXT @ 2020-04-16 10:42 UTC (permalink / raw)
  To: Shubhrajyoti Datta; +Cc: wsa, linux-i2c, michal.simek, linux-arm-kernel

> > > >
> > > >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

Just to update you about the previously mentioned possible multithreading issue, I took some time to try and force the race condition to happen by adding some delay between checking and setting "i2c->tx_msg".
I was planning to fix it in separate patch since the possible issue would have existed already.
I discovered that I am unable to reproduce the threading issue, since the bus is already protected by a mutex in "i2c_transfer()" implemented in "i2c-core-base.c".
I suppose this may have been obvious for someone who has worked longer than me with the kernel i2c -drivers, but it seems that the driver implementations don't have to worry about access from multiple threads, since they are already protected in a more general level.

-Jaakko

^ permalink raw reply	[flat|nested] 44+ messages in thread

* RE: [PATCH] i2c: xiic: Support disabling multi-master in DT
@ 2020-04-16 10:42           ` Laine Jaakko EXT
  0 siblings, 0 replies; 44+ messages in thread
From: Laine Jaakko EXT @ 2020-04-16 10:42 UTC (permalink / raw)
  To: Shubhrajyoti Datta; +Cc: michal.simek, linux-i2c, linux-arm-kernel, wsa

> > > >
> > > >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

Just to update you about the previously mentioned possible multithreading issue, I took some time to try and force the race condition to happen by adding some delay between checking and setting "i2c->tx_msg".
I was planning to fix it in separate patch since the possible issue would have existed already.
I discovered that I am unable to reproduce the threading issue, since the bus is already protected by a mutex in "i2c_transfer()" implemented in "i2c-core-base.c".
I suppose this may have been obvious for someone who has worked longer than me with the kernel i2c -drivers, but it seems that the driver implementations don't have to worry about access from multiple threads, since they are already protected in a more general level.

-Jaakko
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 44+ messages in thread

* RE: [PATCH] i2c: xiic: Support disabling multi-master in DT
  2020-04-02  9:28           ` Wolfram Sang
@ 2020-05-04 12:58             ` Laine Jaakko EXT
  -1 siblings, 0 replies; 44+ messages in thread
From: Laine Jaakko EXT @ 2020-05-04 12:58 UTC (permalink / raw)
  To: Wolfram Sang, Rob Herring
  Cc: Shubhrajyoti Datta, linux-i2c, michal.simek, linux-arm-kernel,
	devicetree

> > How would both bindings existing simultaneously be interpreted? Maybe
> > both existing simultaneously should be considered as an invalid
> > configuration, so that it would be enough to just check the one you
> > need? The other option would be to treat both existing similarly to
> > neither existing, which would require the driver to always check both
> > if checking one.
>
> I am clearly for saying that this is an illegal combination. I'd hope
> this can be expressed in a YAML binding. Yet, my research didn't give me
> an answer. Adding Rob and DT list to CC. Question is:
>
> Can we check if the boolean bindings "multi-master" and "single-master"
> are not applied at the same time? Any other combination is okay, i.e.
> just one of them or none of them.

It seems we have not had any replies by now, but it would be nice to get this thing moving forward,
even though we have this current version of patch already applied and working in our kernel branch
and are not therefore really in hurry in that regard.

The changes required to this patch at XIIC driver from suggested DT changes are pretty minor.
Basically only checking a different property, reversing logic and some naming changes.
I can make these changes already for the driver if this solution is what will be chosen,
or would you prefer to still think about this?

Regarding the device tree changes:
I am not very familiar with the needed documentation changes, YAML bindings or what needs to be done for new bindings in general.
Would you prefer to still consider them and/or get these subsystem level bindings done by someone more familiar with them?
Another option would be for me to try find time to do the suggested bindings changes anyway, but it will likely require some effort
from me to familiarize with device tree bindings changes and schedule the time for it.

Best regards,
Jaakko

^ permalink raw reply	[flat|nested] 44+ messages in thread

* RE: [PATCH] i2c: xiic: Support disabling multi-master in DT
@ 2020-05-04 12:58             ` Laine Jaakko EXT
  0 siblings, 0 replies; 44+ messages in thread
From: Laine Jaakko EXT @ 2020-05-04 12:58 UTC (permalink / raw)
  To: Wolfram Sang, Rob Herring
  Cc: devicetree, michal.simek, Shubhrajyoti Datta, linux-arm-kernel,
	linux-i2c

> > How would both bindings existing simultaneously be interpreted? Maybe
> > both existing simultaneously should be considered as an invalid
> > configuration, so that it would be enough to just check the one you
> > need? The other option would be to treat both existing similarly to
> > neither existing, which would require the driver to always check both
> > if checking one.
>
> I am clearly for saying that this is an illegal combination. I'd hope
> this can be expressed in a YAML binding. Yet, my research didn't give me
> an answer. Adding Rob and DT list to CC. Question is:
>
> Can we check if the boolean bindings "multi-master" and "single-master"
> are not applied at the same time? Any other combination is okay, i.e.
> just one of them or none of them.

It seems we have not had any replies by now, but it would be nice to get this thing moving forward,
even though we have this current version of patch already applied and working in our kernel branch
and are not therefore really in hurry in that regard.

The changes required to this patch at XIIC driver from suggested DT changes are pretty minor.
Basically only checking a different property, reversing logic and some naming changes.
I can make these changes already for the driver if this solution is what will be chosen,
or would you prefer to still think about this?

Regarding the device tree changes:
I am not very familiar with the needed documentation changes, YAML bindings or what needs to be done for new bindings in general.
Would you prefer to still consider them and/or get these subsystem level bindings done by someone more familiar with them?
Another option would be for me to try find time to do the suggested bindings changes anyway, but it will likely require some effort
from me to familiarize with device tree bindings changes and schedule the time for it.

Best regards,
Jaakko

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 44+ messages in thread

* Re: [PATCH] i2c: xiic: Support disabling multi-master in DT
  2020-05-04 12:58             ` Laine Jaakko EXT
@ 2020-05-27 11:07               ` Wolfram Sang
  -1 siblings, 0 replies; 44+ messages in thread
From: Wolfram Sang @ 2020-05-27 11:07 UTC (permalink / raw)
  To: Laine Jaakko EXT
  Cc: Rob Herring, Shubhrajyoti Datta, linux-i2c, michal.simek,
	linux-arm-kernel, devicetree

[-- Attachment #1: Type: text/plain, Size: 1490 bytes --]

Hi Jaako,

> The changes required to this patch at XIIC driver from suggested DT
> changes are pretty minor. Basically only checking a different
> property, reversing logic and some naming changes. I can make these
> changes already for the driver if this solution is what will be
> chosen, or would you prefer to still think about this?

I think we can go forward this way. Although I didn't find an example, I
am quite sure YAML format can have exclusive properties. If not, it
should be added ;) I'll try again to get some information from people
more experienced with YAML. But we don't depend on it.

> Regarding the device tree changes: I am not very familiar with the
> needed documentation changes, YAML bindings or what needs to be done
> for new bindings in general. Would you prefer to still consider them
> and/or get these subsystem level bindings done by someone more
> familiar with them? Another option would be for me to try find time to
> do the suggested bindings changes anyway, but it will likely require
> some effort from me to familiarize with device tree bindings changes
> and schedule the time for it.

No need to. Luckily, the I2C main bindings file is still text, so it is
easy to modify. I will send a patch out in a few minutes, so you can
base your driver code on that. Converting to YAML is a completely
different step, not related to your patch here. (Sidenote: I am looking
for volunteers to do that)

I think this covers it?

Happy hacking,

   Wolfram


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 44+ messages in thread

* Re: [PATCH] i2c: xiic: Support disabling multi-master in DT
@ 2020-05-27 11:07               ` Wolfram Sang
  0 siblings, 0 replies; 44+ messages in thread
From: Wolfram Sang @ 2020-05-27 11:07 UTC (permalink / raw)
  To: Laine Jaakko EXT
  Cc: Rob Herring, Shubhrajyoti Datta, devicetree, michal.simek,
	linux-i2c, linux-arm-kernel


[-- Attachment #1.1: Type: text/plain, Size: 1490 bytes --]

Hi Jaako,

> The changes required to this patch at XIIC driver from suggested DT
> changes are pretty minor. Basically only checking a different
> property, reversing logic and some naming changes. I can make these
> changes already for the driver if this solution is what will be
> chosen, or would you prefer to still think about this?

I think we can go forward this way. Although I didn't find an example, I
am quite sure YAML format can have exclusive properties. If not, it
should be added ;) I'll try again to get some information from people
more experienced with YAML. But we don't depend on it.

> Regarding the device tree changes: I am not very familiar with the
> needed documentation changes, YAML bindings or what needs to be done
> for new bindings in general. Would you prefer to still consider them
> and/or get these subsystem level bindings done by someone more
> familiar with them? Another option would be for me to try find time to
> do the suggested bindings changes anyway, but it will likely require
> some effort from me to familiarize with device tree bindings changes
> and schedule the time for it.

No need to. Luckily, the I2C main bindings file is still text, so it is
easy to modify. I will send a patch out in a few minutes, so you can
base your driver code on that. Converting to YAML is a completely
different step, not related to your patch here. (Sidenote: I am looking
for volunteers to do that)

I think this covers it?

Happy hacking,

   Wolfram


[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 44+ messages in thread

* RE: [PATCH] i2c: xiic: Support disabling multi-master in DT
  2020-05-27 11:07               ` Wolfram Sang
@ 2020-05-27 11:27                 ` Laine Jaakko EXT
  -1 siblings, 0 replies; 44+ messages in thread
From: Laine Jaakko EXT @ 2020-05-27 11:27 UTC (permalink / raw)
  To: Wolfram Sang
  Cc: Rob Herring, Shubhrajyoti Datta, linux-i2c, michal.simek,
	linux-arm-kernel, devicetree

Thanks Wolfram for helping me out with this.

I will make Version 2 patch with proposed changes.

Happy hacking for you too!

-Jaakko

^ permalink raw reply	[flat|nested] 44+ messages in thread

* RE: [PATCH] i2c: xiic: Support disabling multi-master in DT
@ 2020-05-27 11:27                 ` Laine Jaakko EXT
  0 siblings, 0 replies; 44+ messages in thread
From: Laine Jaakko EXT @ 2020-05-27 11:27 UTC (permalink / raw)
  To: Wolfram Sang
  Cc: Rob Herring, Shubhrajyoti Datta, devicetree, michal.simek,
	linux-i2c, linux-arm-kernel

Thanks Wolfram for helping me out with this.

I will make Version 2 patch with proposed changes.

Happy hacking for you too!

-Jaakko

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 44+ messages in thread

end of thread, other threads:[~2020-05-27 11:28 UTC | newest]

Thread overview: 44+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-02-18 13:58 [PATCH] i2c: xiic: Support disabling multi-master in DT Laine Jaakko EXT
2020-02-18 13:58 ` Laine Jaakko EXT
2020-02-24 10:13 ` Michal Simek
2020-02-24 10:13   ` Michal Simek
2020-02-25 14:08   ` Laine Jaakko EXT
2020-02-25 14:08     ` Laine Jaakko EXT
2020-02-25 14:13     ` Wolfram Sang
2020-02-25 14:13       ` Wolfram Sang
2020-02-25 14:27       ` Laine Jaakko EXT
2020-02-25 14:27         ` Laine Jaakko EXT
2020-02-25 14:32         ` Wolfram Sang
2020-02-25 14:32           ` Wolfram Sang
2020-02-25 14:37           ` Michal Simek
2020-02-25 14:37             ` Michal Simek
2020-02-25 14:41             ` Wolfram Sang
2020-02-25 14:41               ` Wolfram Sang
2020-02-25 14:44               ` Michal Simek
2020-02-25 14:44                 ` Michal Simek
2020-02-25 14:48                 ` Wolfram Sang
2020-02-25 14:48                   ` Wolfram Sang
2020-02-25 14:49                   ` Michal Simek
2020-02-25 14:49                     ` Michal Simek
2020-03-18 12:20 ` Shubhrajyoti Datta
2020-03-18 12:20   ` Shubhrajyoti Datta
2020-03-18 16:49   ` Laine Jaakko EXT
2020-03-18 16:49     ` Laine Jaakko EXT
2020-03-19  9:34     ` Shubhrajyoti Datta
2020-03-19  9:34       ` Shubhrajyoti Datta
2020-03-19 10:26       ` Laine Jaakko EXT
2020-03-19 10:26         ` Laine Jaakko EXT
2020-04-16 10:42         ` Laine Jaakko EXT
2020-04-16 10:42           ` Laine Jaakko EXT
2020-04-01 14:41     ` Wolfram Sang
2020-04-01 14:41       ` Wolfram Sang
2020-04-02  6:16       ` Laine Jaakko EXT
2020-04-02  6:16         ` Laine Jaakko EXT
2020-04-02  9:28         ` Wolfram Sang
2020-04-02  9:28           ` Wolfram Sang
2020-05-04 12:58           ` Laine Jaakko EXT
2020-05-04 12:58             ` Laine Jaakko EXT
2020-05-27 11:07             ` Wolfram Sang
2020-05-27 11:07               ` Wolfram Sang
2020-05-27 11:27               ` Laine Jaakko EXT
2020-05-27 11:27                 ` Laine Jaakko EXT

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.