* Re: [PATCH] i2c: i2c-mux-pca954x: retry updating the mux selection on failure
From: Wolfram Sang @ 2016-09-15 19:52 UTC (permalink / raw)
To: Peter Rosin; +Cc: open list:I2C MUXES, open list
In-Reply-To: <1473859452-8069-1-git-send-email-peda@axentia.se>
[-- Attachment #1: Type: text/plain, Size: 312 bytes --]
On Wed, Sep 14, 2016 at 03:24:12PM +0200, Peter Rosin wrote:
> The cached value of the last selected channel prevents retries on the
> next call, even on failure to update the selected channel. Fix that.
>
> Signed-off-by: Peter Rosin <peda@axentia.se>
Looks good. Is this 4.8 material or rather 4.9?
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [PATCH 0/3] LEGO MINDSTORMS I2C support
From: Wolfram Sang @ 2016-09-15 20:00 UTC (permalink / raw)
To: David Lechner; +Cc: linux-i2c, linux-kernel
In-Reply-To: <1473108014-30787-1-git-send-email-david@lechnology.com>
[-- Attachment #1: Type: text/plain, Size: 969 bytes --]
On Mon, Sep 05, 2016 at 03:40:11PM -0500, David Lechner wrote:
> I'm working on getting LEGO MINDSTORMS[1] support in the Linux kernel.
>
> They have a system of modular sensors that are hot-plugable, some of which use
> I2C communications. Unfortunately, these don't necessary follow standard I2C
> conventions, but they do have a well-defined register layout, so they are
> easy to detect.
>
> This set of patches addresses the hot-plugability of the sensors.
>
> [1]: http://mindstorms.lego.com
Thanks!
For a review, I'd need users of this functionality. In this case, a
master driver and probably a sensor driver. Can any regular I2C master
talk to these devices if they support talking to addresses 0x00-0x7f? My
first impression is that using a class is not the proper approach
because classes were used to limit access to devices, not to extend.
However, I'd need to see more code to check if that holds true.
Thanks,
Wolfram
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [PATCH] MAINTAINERS: Remove myself from PA Semi entries
From: David Miller @ 2016-09-16 5:50 UTC (permalink / raw)
To: mpe; +Cc: olof, netdev, linux-i2c, jdelvare
In-Reply-To: <87k2eeoo30.fsf@concordia.ellerman.id.au>
From: Michael Ellerman <mpe@ellerman.id.au>
Date: Wed, 14 Sep 2016 18:57:55 +1000
> Olof Johansson <olof@lixom.net> writes:
>
>> Jean, Dave,
>>
>> I was hoping to have Michael merge this since the bulk of the platform is under him,
>> cc:ing you mostly to be aware that I am orphaning a driver in your subsystems.
>
> I'll merge it unless I hear otherwise from Dave.
Feel free to merge this.
Thanks.
^ permalink raw reply
* Re: [PATCH] i2c: i2c-mux-pca954x: retry updating the mux selection on failure
From: Peter Rosin @ 2016-09-16 7:01 UTC (permalink / raw)
To: Wolfram Sang; +Cc: linux-i2c, linux-kernel
In-Reply-To: <20160915195235.GD7863@katana>
[Resend, with lists added back...]
On 2016-09-15 21:52, Wolfram Sang wrote:
> On Wed, Sep 14, 2016 at 03:24:12PM +0200, Peter Rosin wrote:
>> The cached value of the last selected channel prevents retries on the
>> next call, even on failure to update the selected channel. Fix that.
>>
>> Signed-off-by: Peter Rosin <peda@axentia.se>
>
> Looks good. Is this 4.8 material or rather 4.9?
I have no real-world event or report backing the change, but it is
simple enough and fixes problems that are potentially very hard to
detect/reproduce. Certainly fodder for 4.8 and probably stable as
well (if you care). But no biggie, so personally I don't mind if
you wait until 4.9, but then again I don't depend on the driver...
Cheers,
Peter
^ permalink raw reply
* [PATCH] gpio: pca953x: fix an incorrect lockdep warning
From: Bartosz Golaszewski @ 2016-09-16 14:18 UTC (permalink / raw)
To: Linus Walleij, Alexandre Courbot, Andy Shevchenko, Vignesh R,
Yong Li, Geert Uytterhoeven, Peter Zijlstra, Ingo Molnar,
Wolfram Sang
Cc: linux-i2c, linux-gpio, LKML, Bartosz Golaszewski
If an I2C GPIO multiplexer is driven by a GPIO provided by an expander
when there's a second expander using the same device driver on one of
the I2C bus segments, lockdep prints a deadlock warning when trying to
set the direction or the value of the GPIOs provided by the second
expander.
The below diagram presents the setup:
- - - - -
------- --------- Bus segment 1 | |
| | | |--------------- Devices
| | SCL/SDA | | | |
| Linux |-----------| I2C MUX | - - - - -
| | | | | Bus segment 2
| | | | |-------------------
------- | --------- |
| | - - - - -
------------ | MUX GPIO | |
| | | Devices
| GPIO | | | |
| Expander 1 |---- - - - - -
| | |
------------ | SCL/SDA
|
------------
| |
| GPIO |
| Expander 2 |
| |
------------
The reason for lockdep warning is that we take the chip->i2c_lock in
pca953x_gpio_set_value() or pca953x_gpio_direction_output() and then
come right back to pca953x_gpio_set_value() when the GPIO mux kicks
in. The locks actually protect different expanders, but for lockdep
both are of the same class, so it says:
Possible unsafe locking scenario:
CPU0
----
lock(&chip->i2c_lock);
lock(&chip->i2c_lock);
*** DEADLOCK ***
May be due to missing lock nesting notation
In order to get rid of the warning, check if the i2c adapter of the
expander is multiplexed (by checking if it has a parent adapter) and,
if so, set a different lock subclass for chip->i2c_lock.
Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
Note: a similar issue would occur with other gpio expanders under
similar circumstances. If this patch get's merged, I'll prepare
a common solution for all gpio drivers which use an internal i2c lock.
drivers/gpio/gpio-pca953x.c | 12 ++++++++++++
1 file changed, 12 insertions(+)
diff --git a/drivers/gpio/gpio-pca953x.c b/drivers/gpio/gpio-pca953x.c
index 02f2a56..2d49b25 100644
--- a/drivers/gpio/gpio-pca953x.c
+++ b/drivers/gpio/gpio-pca953x.c
@@ -787,6 +787,18 @@ static int pca953x_probe(struct i2c_client *client,
mutex_init(&chip->i2c_lock);
+ /*
+ * If the i2c adapter we're connected to is multiplexed (which is
+ * indicated by it having a parent adapter) we need to use a
+ * different lock subclass. It's caused by the fact that in a rare
+ * case of using an i2c-gpio multiplexer controlled by a gpio
+ * provided by an expander using the same driver, lockdep would
+ * incorrectly detect a deadlock, since we'd take a second lock
+ * of the same class without releasing the first one.
+ */
+ if (i2c_parent_is_i2c_adapter(client->adapter))
+ lockdep_set_subclass(&chip->i2c_lock, SINGLE_DEPTH_NESTING);
+
/* initialize cached registers from their original values.
* we can't share this chip with another i2c master.
*/
--
2.7.4
^ permalink raw reply related
* Re: [PATCH] gpio: pca953x: fix an incorrect lockdep warning
From: Bartosz Golaszewski @ 2016-09-16 14:40 UTC (permalink / raw)
To: Peter Rosin
Cc: Linus Walleij, Alexandre Courbot, Andy Shevchenko, Vignesh R,
Yong Li, Geert Uytterhoeven, Peter Zijlstra, Ingo Molnar,
Wolfram Sang, linux-i2c, linux-gpio, LKML
In-Reply-To: <d1f3c65c-0294-19fc-8de5-fa3e79a9f505@axentia.se>
2016-09-16 16:33 GMT+02:00 Peter Rosin <peda@axentia.se>:
>
> If this is to be fixed this even for crazy setups where the pattern is
> repeated for more levels, you can look into drivers/i2c/i2c-core.c
> i2c_adapter_depth() and how it's used (i.e. for this exact purpose).
> Maybe it's time to export that function?
>
Hi Peter,
thanks for the heads up. I was not aware of this function. Lockdep
only allows us to specify up to 8 subclasses, but I can't possibly
imagine a setup where more would be needed. I'll submit a series
exporting this function and using it in pca953x.
Best regards,
Bartosz Golaszewski
^ permalink raw reply
* [PATCH v2 2/4] lockdep: make MAX_LOCKDEP_SUBCLASSES unconditionally visible
From: Bartosz Golaszewski @ 2016-09-16 16:02 UTC (permalink / raw)
To: Linus Walleij, Alexandre Courbot, Andy Shevchenko, Vignesh R,
Yong Li, Geert Uytterhoeven, Peter Zijlstra, Ingo Molnar,
Wolfram Sang, Peter Rosin
Cc: linux-i2c, linux-gpio, LKML, Bartosz Golaszewski
In-Reply-To: <1474041765-17818-1-git-send-email-bgolaszewski@baylibre.com>
This define is needed by i2c_adapter_depth() to detect if we don't
exceed the maximum number of lock subclasses. Make it visible even
if lockdep is disabled.
Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
include/linux/lockdep.h | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/include/linux/lockdep.h b/include/linux/lockdep.h
index eabe013..c1458fe 100644
--- a/include/linux/lockdep.h
+++ b/include/linux/lockdep.h
@@ -16,6 +16,8 @@ struct lockdep_map;
extern int prove_locking;
extern int lock_stat;
+#define MAX_LOCKDEP_SUBCLASSES 8UL
+
#ifdef CONFIG_LOCKDEP
#include <linux/linkage.h>
@@ -29,8 +31,6 @@ extern int lock_stat;
*/
#define XXX_LOCK_USAGE_STATES (1+3*4)
-#define MAX_LOCKDEP_SUBCLASSES 8UL
-
/*
* NR_LOCKDEP_CACHING_CLASSES ... Number of classes
* cached in the instance of lockdep_map
--
2.7.4
^ permalink raw reply related
* [PATCH v2 0/4] gpio: fix an incorrect lockdep warning
From: Bartosz Golaszewski @ 2016-09-16 16:02 UTC (permalink / raw)
To: Linus Walleij, Alexandre Courbot, Andy Shevchenko, Vignesh R,
Yong Li, Geert Uytterhoeven, Peter Zijlstra, Ingo Molnar,
Wolfram Sang, Peter Rosin
Cc: linux-i2c, linux-gpio, LKML, Bartosz Golaszewski
If an I2C GPIO multiplexer is driven by a GPIO provided by an expander
when there's a second expander using the same device driver on one of
the I2C bus segments, lockdep prints a deadlock warning when trying to
set the direction or the value of the GPIOs provided by the second
expander.
This series exports an already existing function from i2c-core as
public API and reuses it in pca953x to pass a correct lock subclass
to lockdep.
Note: if this series gets merged, I'll prepare follow-up patches for
other expanders for which a similar problem could potentially occur.
Tested with the following setup:
------- --------- Bus segment 1 | |
| | | |--------------- Devices
| | SCL/SDA | | | |
| Linux |-----------| I2C MUX | - - - - -
| | | | | Bus segment 2
| | | | |-------------------
------- | --------- |
| | - - - - -
------------ | MUX GPIO | |
| | | Devices
| GPIO | | | |
| Expander 1 |---- - - - - -
| | |
------------ | SCL/SDA
|
------------
| |
| GPIO |
| Expander 2 |
| |
------------
where expander 1 is a pca9534 and expander 2 is a pca9535.
v1 -> v2:
- added patches 1/4, 2/4 & 3/4
- used i2c_adapter_depth() in patch 4/4 in order to detect multiple
adapter nesting
Bartosz Golaszewski (4):
i2c: export i2c_adapter_depth()
lockdep: make MAX_LOCKDEP_SUBCLASSES unconditionally visible
i2c: add a warning to i2c_adapter_depth()
gpio: pca953x: fix an incorrect lockdep warning
drivers/gpio/gpio-pca953x.c | 2 ++
drivers/i2c/i2c-core.c | 12 +++++-------
include/linux/i2c.h | 1 +
include/linux/lockdep.h | 4 ++--
4 files changed, 10 insertions(+), 9 deletions(-)
--
2.7.4
^ permalink raw reply
* [PATCH v2 1/4] i2c: export i2c_adapter_depth()
From: Bartosz Golaszewski @ 2016-09-16 16:02 UTC (permalink / raw)
To: Linus Walleij, Alexandre Courbot, Andy Shevchenko, Vignesh R,
Yong Li, Geert Uytterhoeven, Peter Zijlstra, Ingo Molnar,
Wolfram Sang, Peter Rosin
Cc: linux-i2c, linux-gpio, LKML, Bartosz Golaszewski
In-Reply-To: <1474041765-17818-1-git-send-email-bgolaszewski@baylibre.com>
For crazy setups in which an i2c gpio expander is behind an i2c gpio
multiplexer controlled by a gpio provided a second expander using the
same device driver we need to explicitly tell lockdep how to handle
nested locking.
Export i2c_adapter_depth() as public API to be reused outside of i2c
core code.
Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
drivers/i2c/i2c-core.c | 9 ++-------
include/linux/i2c.h | 1 +
2 files changed, 3 insertions(+), 7 deletions(-)
diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
index da3a02e..c9b8df8 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -1335,13 +1335,7 @@ static void i2c_adapter_dev_release(struct device *dev)
complete(&adap->dev_released);
}
-/*
- * This function is only needed for mutex_lock_nested, so it is never
- * called unless locking correctness checking is enabled. Thus we
- * make it inline to avoid a compiler warning. That's what gcc ends up
- * doing anyway.
- */
-static inline unsigned int i2c_adapter_depth(struct i2c_adapter *adapter)
+unsigned int i2c_adapter_depth(struct i2c_adapter *adapter)
{
unsigned int depth = 0;
@@ -1350,6 +1344,7 @@ static inline unsigned int i2c_adapter_depth(struct i2c_adapter *adapter)
return depth;
}
+EXPORT_SYMBOL_GPL(i2c_adapter_depth);
/*
* Let users instantiate I2C devices through sysfs. This can be used when
diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index fffdc27..cfacc03 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -673,6 +673,7 @@ extern void i2c_clients_command(struct i2c_adapter *adap,
extern struct i2c_adapter *i2c_get_adapter(int nr);
extern void i2c_put_adapter(struct i2c_adapter *adap);
+extern unsigned int i2c_adapter_depth(struct i2c_adapter *adapter);
void i2c_parse_fw_timings(struct device *dev, struct i2c_timings *t, bool use_defaults);
--
2.7.4
^ permalink raw reply related
* [PATCH v2 4/4] gpio: pca953x: fix an incorrect lockdep warning
From: Bartosz Golaszewski @ 2016-09-16 16:02 UTC (permalink / raw)
To: Linus Walleij, Alexandre Courbot, Andy Shevchenko, Vignesh R,
Yong Li, Geert Uytterhoeven, Peter Zijlstra, Ingo Molnar,
Wolfram Sang, Peter Rosin
Cc: linux-i2c, linux-gpio, LKML, Bartosz Golaszewski
In-Reply-To: <1474041765-17818-1-git-send-email-bgolaszewski@baylibre.com>
If an I2C GPIO multiplexer is driven by a GPIO provided by an expander
when there's a second expander using the same device driver on one of
the I2C bus segments, lockdep prints a deadlock warning when trying to
set the direction or the value of the GPIOs provided by the second
expander.
The below diagram presents the setup:
- - - - -
------- --------- Bus segment 1 | |
| | | |--------------- Devices
| | SCL/SDA | | | |
| Linux |-----------| I2C MUX | - - - - -
| | | | | Bus segment 2
| | | | |-------------------
------- | --------- |
| | - - - - -
------------ | MUX GPIO | |
| | | Devices
| GPIO | | | |
| Expander 1 |---- - - - - -
| | |
------------ | SCL/SDA
|
------------
| |
| GPIO |
| Expander 2 |
| |
------------
The reason for lockdep warning is that we take the chip->i2c_lock in
pca953x_gpio_set_value() or pca953x_gpio_direction_output() and then
come right back to pca953x_gpio_set_value() when the GPIO mux kicks
in. The locks actually protect different expanders, but for lockdep
both are of the same class, so it says:
Possible unsafe locking scenario:
CPU0
----
lock(&chip->i2c_lock);
lock(&chip->i2c_lock);
*** DEADLOCK ***
May be due to missing lock nesting notation
In order to get rid of the warning, retrieve the adapter nesting depth
and use it as lockdep subclass for chip->i2c_lock.
Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
drivers/gpio/gpio-pca953x.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/gpio/gpio-pca953x.c b/drivers/gpio/gpio-pca953x.c
index 02f2a56..892dc04 100644
--- a/drivers/gpio/gpio-pca953x.c
+++ b/drivers/gpio/gpio-pca953x.c
@@ -786,6 +786,8 @@ static int pca953x_probe(struct i2c_client *client,
chip->chip_type = PCA_CHIP_TYPE(chip->driver_data);
mutex_init(&chip->i2c_lock);
+ lockdep_set_subclass(&chip->i2c_lock,
+ i2c_adapter_depth(client->adapter));
/* initialize cached registers from their original values.
* we can't share this chip with another i2c master.
--
2.7.4
^ permalink raw reply related
* [PATCH v2 3/4] i2c: add a warning to i2c_adapter_depth()
From: Bartosz Golaszewski @ 2016-09-16 16:02 UTC (permalink / raw)
To: Linus Walleij, Alexandre Courbot, Andy Shevchenko, Vignesh R,
Yong Li, Geert Uytterhoeven, Peter Zijlstra, Ingo Molnar,
Wolfram Sang, Peter Rosin
Cc: linux-i2c, linux-gpio, LKML, Bartosz Golaszewski
In-Reply-To: <1474041765-17818-1-git-send-email-bgolaszewski@baylibre.com>
This routine is only used together with lockdep for nested locking.
The number of lock subclasses is limited to 8 as defined in lockdep.h
Emit a warning if the adapter depth exceeds the maximum number of
lockdep subclasses.
Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
drivers/i2c/i2c-core.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
index c9b8df8..75cefa8 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -1342,6 +1342,9 @@ unsigned int i2c_adapter_depth(struct i2c_adapter *adapter)
while ((adapter = i2c_parent_is_i2c_adapter(adapter)))
depth++;
+ WARN_ONCE(depth >= MAX_LOCKDEP_SUBCLASSES,
+ "adapter depth exceeds lockdep subclass limit\n");
+
return depth;
}
EXPORT_SYMBOL_GPL(i2c_adapter_depth);
--
2.7.4
^ permalink raw reply related
* Re: [PATCH] gpio: pca953x: fix an incorrect lockdep warning
From: Bartosz Golaszewski @ 2016-09-16 16:04 UTC (permalink / raw)
To: Linus Walleij, Alexandre Courbot, Andy Shevchenko, Vignesh R,
Yong Li, Geert Uytterhoeven, Peter Zijlstra, Ingo Molnar,
Wolfram Sang, Peter Rosin
Cc: linux-i2c, linux-gpio, LKML, Bartosz Golaszewski
In-Reply-To: <1474035484-25134-1-git-send-email-bgolaszewski@baylibre.com>
2016-09-16 16:18 GMT+02:00 Bartosz Golaszewski <bgolaszewski@baylibre.com>:
> If an I2C GPIO multiplexer is driven by a GPIO provided by an expander
> when there's a second expander using the same device driver on one of
> the I2C bus segments, lockdep prints a deadlock warning when trying to
> set the direction or the value of the GPIOs provided by the second
> expander.
[snip]
Superseded by v2.
Thanks,
Bartosz
^ permalink raw reply
* Re: [PATCH v2 0/4] gpio: fix an incorrect lockdep warning
From: Wolfram Sang @ 2016-09-16 17:26 UTC (permalink / raw)
To: Bartosz Golaszewski
Cc: Linus Walleij, Alexandre Courbot, Andy Shevchenko, Vignesh R,
Yong Li, Geert Uytterhoeven, Peter Zijlstra, Ingo Molnar,
Peter Rosin, linux-i2c, linux-gpio, LKML
In-Reply-To: <1474041765-17818-1-git-send-email-bgolaszewski@baylibre.com>
[-- Attachment #1: Type: text/plain, Size: 747 bytes --]
On Fri, Sep 16, 2016 at 06:02:41PM +0200, Bartosz Golaszewski wrote:
> If an I2C GPIO multiplexer is driven by a GPIO provided by an expander
> when there's a second expander using the same device driver on one of
> the I2C bus segments, lockdep prints a deadlock warning when trying to
> set the direction or the value of the GPIOs provided by the second
> expander.
>
> This series exports an already existing function from i2c-core as
> public API and reuses it in pca953x to pass a correct lock subclass
> to lockdep.
Looks good from my POV, but will wait for Peter to comment.
If accepted, I'd think this should go via my I2C tree and I would like
to ask Linus to ack patch 4. D'accord, everyone?
Thanks,
Wolfram
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [PATCH v2 0/4] gpio: fix an incorrect lockdep warning
From: Peter Rosin @ 2016-09-16 17:45 UTC (permalink / raw)
To: Wolfram Sang, Bartosz Golaszewski
Cc: Linus Walleij, Alexandre Courbot, Andy Shevchenko, Vignesh R,
Yong Li, Geert Uytterhoeven, Peter Zijlstra, Ingo Molnar,
linux-i2c, linux-gpio, LKML
In-Reply-To: <20160916172621.GA1426@katana>
On 2016-09-16 19:26, Wolfram Sang wrote:
> On Fri, Sep 16, 2016 at 06:02:41PM +0200, Bartosz Golaszewski wrote:
>> If an I2C GPIO multiplexer is driven by a GPIO provided by an expander
>> when there's a second expander using the same device driver on one of
>> the I2C bus segments, lockdep prints a deadlock warning when trying to
>> set the direction or the value of the GPIOs provided by the second
>> expander.
>>
>> This series exports an already existing function from i2c-core as
>> public API and reuses it in pca953x to pass a correct lock subclass
>> to lockdep.
>
> Looks good from my POV, but will wait for Peter to comment.
>
> If accepted, I'd think this should go via my I2C tree and I would like
> to ask Linus to ack patch 4. D'accord, everyone?
Since it is not clear if "Peter" is me or PeterZ (I suspect PeterZ...),
I'm just adding that it all looks fine by me as well, just to prevent
this from being held up by a misunderstanding.
It does unconditionally add a new function to i2c-core that is only
ever used if lockdep is enabled, but it is tiny and I'm not bothered
by that memory waste.
Cheers,
Peter
^ permalink raw reply
* Re: [PATCH v2 0/4] gpio: fix an incorrect lockdep warning
From: Wolfram Sang @ 2016-09-16 17:58 UTC (permalink / raw)
To: Peter Rosin
Cc: Bartosz Golaszewski, Linus Walleij, Alexandre Courbot,
Andy Shevchenko, Vignesh R, Yong Li, Geert Uytterhoeven,
Peter Zijlstra, Ingo Molnar, linux-i2c, linux-gpio, LKML
In-Reply-To: <deaa654a-9afc-eae6-6bc8-75f2c3960970@axentia.se>
[-- Attachment #1: Type: text/plain, Size: 880 bytes --]
> > Looks good from my POV, but will wait for Peter to comment.
> >
> > If accepted, I'd think this should go via my I2C tree and I would like
> > to ask Linus to ack patch 4. D'accord, everyone?
>
> Since it is not clear if "Peter" is me or PeterZ (I suspect PeterZ...),
Nope, I meant you :) I really value your input, it especially helps me
on topics like locking, nesting, muxing... etc. Much appreciated, thanks
a lot for doing that!
> I'm just adding that it all looks fine by me as well, just to prevent
> this from being held up by a misunderstanding.
OK. I read this as Acked-by.
> It does unconditionally add a new function to i2c-core that is only
> ever used if lockdep is enabled, but it is tiny and I'm not bothered
> by that memory waste.
Same here. And if it prevents us from false positive lockdep reports, I
am all for fixing it.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: RESEND:i2c-eg20t: fix race between i2c init and interrupt enable
From: Wolfram Sang @ 2016-09-16 20:04 UTC (permalink / raw)
To: Yadi Hu; +Cc: jdelvare, linux-i2c
In-Reply-To: <1473240337-5694-1-git-send-email-yadi.hu@windriver.com>
[-- Attachment #1: Type: text/plain, Size: 545 bytes --]
> after testing last patch, replacing request_irq() location is not a
> good idea,it might produce a little time windows ,in which the interrupts
> occurring inside will be omitted.
>
> the new patch adds a check point on interrupt handler in case field
> 'pch_base_address' has not been initialed.
What about using two for-loops in probe like this pseudo code?
for (all_channels)
do_initialization
request_irq()
for (all_channels)
register_adapter
This seems to me the correct order and most readable solution.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [PATCH 0/3] LEGO MINDSTORMS I2C support
From: David Lechner @ 2016-09-16 20:05 UTC (permalink / raw)
To: Wolfram Sang; +Cc: linux-i2c, linux-kernel
In-Reply-To: <20160915200045.GE7863@katana>
On 09/15/2016 03:00 PM, Wolfram Sang wrote:
> On Mon, Sep 05, 2016 at 03:40:11PM -0500, David Lechner wrote:
>> I'm working on getting LEGO MINDSTORMS[1] support in the Linux kernel.
>>
>> They have a system of modular sensors that are hot-plugable, some of which use
>> I2C communications. Unfortunately, these don't necessary follow standard I2C
>> conventions, but they do have a well-defined register layout, so they are
>> easy to detect.
>>
>> This set of patches addresses the hot-plugability of the sensors.
>>
>> [1]: http://mindstorms.lego.com
>
> Thanks!
>
> For a review, I'd need users of this functionality. In this case, a
> master driver and probably a sensor driver.
Work in progress drivers:
master:
https://github.com/ev3dev/lego-linux-drivers/blob/master/ev3/legoev3_i2c.c
master:
https://github.com/ev3dev/lego-linux-drivers/blob/master/evb/evb_pru_i2c.c
sensor:
https://github.com/ev3dev/lego-linux-drivers/blob/master/sensors/nxt_i2c_sensor_core.c
> Can any regular I2C master
> talk to these devices if they support talking to addresses 0x00-0x7f?
Yes. I've tested this with the SoC I2C on Raspberry Pi. There are
several 3rd party LEGO MINDSTORMS compatible addon boards for RPi that
use the SoC I2C with LEGO sensors, which is part of my motivation for
this patchset.
> My first impression is that using a class is not the proper approach
> because classes were used to limit access to devices, not to extend.
> However, I'd need to see more code to check if that holds true.
>
I'm certainly open to other suggestions. This just seemed like the
obvious way to me.
^ permalink raw reply
* Re: [PATCH] gpio: pca953x: fix an incorrect lockdep warning
From: Peter Rosin @ 2016-09-16 14:33 UTC (permalink / raw)
To: Bartosz Golaszewski, Linus Walleij, Alexandre Courbot,
Andy Shevchenko, Vignesh R, Yong Li, Geert Uytterhoeven,
Peter Zijlstra, Ingo Molnar, Wolfram Sang
Cc: linux-i2c, linux-gpio, LKML
In-Reply-To: <1474035484-25134-1-git-send-email-bgolaszewski@baylibre.com>
On 2016-09-16 16:18, Bartosz Golaszewski wrote:
> If an I2C GPIO multiplexer is driven by a GPIO provided by an expander
> when there's a second expander using the same device driver on one of
> the I2C bus segments, lockdep prints a deadlock warning when trying to
> set the direction or the value of the GPIOs provided by the second
> expander.
>
> The below diagram presents the setup:
>
> - - - - -
> ------- --------- Bus segment 1 | |
> | | | |--------------- Devices
> | | SCL/SDA | | | |
> | Linux |-----------| I2C MUX | - - - - -
> | | | | | Bus segment 2
> | | | | |-------------------
> ------- | --------- |
> | | - - - - -
> ------------ | MUX GPIO | |
> | | | Devices
> | GPIO | | | |
> | Expander 1 |---- - - - - -
> | | |
> ------------ | SCL/SDA
> |
> ------------
> | |
> | GPIO |
> | Expander 2 |
> | |
> ------------
>
> The reason for lockdep warning is that we take the chip->i2c_lock in
> pca953x_gpio_set_value() or pca953x_gpio_direction_output() and then
> come right back to pca953x_gpio_set_value() when the GPIO mux kicks
> in. The locks actually protect different expanders, but for lockdep
> both are of the same class, so it says:
>
> Possible unsafe locking scenario:
>
> CPU0
> ----
> lock(&chip->i2c_lock);
> lock(&chip->i2c_lock);
>
> *** DEADLOCK ***
>
> May be due to missing lock nesting notation
>
> In order to get rid of the warning, check if the i2c adapter of the
> expander is multiplexed (by checking if it has a parent adapter) and,
> if so, set a different lock subclass for chip->i2c_lock.
>
> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> ---
> Note: a similar issue would occur with other gpio expanders under
> similar circumstances. If this patch get's merged, I'll prepare
> a common solution for all gpio drivers which use an internal i2c lock.
>
> drivers/gpio/gpio-pca953x.c | 12 ++++++++++++
> 1 file changed, 12 insertions(+)
>
> diff --git a/drivers/gpio/gpio-pca953x.c b/drivers/gpio/gpio-pca953x.c
> index 02f2a56..2d49b25 100644
> --- a/drivers/gpio/gpio-pca953x.c
> +++ b/drivers/gpio/gpio-pca953x.c
> @@ -787,6 +787,18 @@ static int pca953x_probe(struct i2c_client *client,
>
> mutex_init(&chip->i2c_lock);
>
> + /*
> + * If the i2c adapter we're connected to is multiplexed (which is
> + * indicated by it having a parent adapter) we need to use a
> + * different lock subclass. It's caused by the fact that in a rare
> + * case of using an i2c-gpio multiplexer controlled by a gpio
> + * provided by an expander using the same driver, lockdep would
> + * incorrectly detect a deadlock, since we'd take a second lock
> + * of the same class without releasing the first one.
> + */
> + if (i2c_parent_is_i2c_adapter(client->adapter))
> + lockdep_set_subclass(&chip->i2c_lock, SINGLE_DEPTH_NESTING);
> +
> /* initialize cached registers from their original values.
> * we can't share this chip with another i2c master.
> */
>
If this is to be fixed this even for crazy setups where the pattern is
repeated for more levels, you can look into drivers/i2c/i2c-core.c
i2c_adapter_depth() and how it's used (i.e. for this exact purpose).
Maybe it's time to export that function?
Cheers,
Peter
^ permalink raw reply
* [PATCH] i2c: axxia: disable clks in case of failure in probe
From: Alexey Khoroshilov @ 2016-09-16 20:30 UTC (permalink / raw)
To: Wolfram Sang; +Cc: Alexey Khoroshilov, linux-i2c, linux-kernel, ldv-project
axxia_i2c_probe() does not disable clock in case of failure
in i2c_add_adapter(). Also it ignores returned value from
clk_prepare_enable().
Found by Linux Driver Verification project (linuxtesting.org).
Signed-off-by: Alexey Khoroshilov <khoroshilov@ispras.ru>
---
drivers/i2c/busses/i2c-axxia.c | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/drivers/i2c/busses/i2c-axxia.c b/drivers/i2c/busses/i2c-axxia.c
index c335cc7852f9..5c05b3050393 100644
--- a/drivers/i2c/busses/i2c-axxia.c
+++ b/drivers/i2c/busses/i2c-axxia.c
@@ -545,7 +545,11 @@ static int axxia_i2c_probe(struct platform_device *pdev)
return ret;
}
- clk_prepare_enable(idev->i2c_clk);
+ ret = clk_prepare_enable(idev->i2c_clk);
+ if (ret) {
+ dev_err(&pdev->dev, "failed to enable clock\n");
+ return ret;
+ }
i2c_set_adapdata(&idev->adapter, idev);
strlcpy(idev->adapter.name, pdev->name, sizeof(idev->adapter.name));
@@ -561,6 +565,7 @@ static int axxia_i2c_probe(struct platform_device *pdev)
ret = i2c_add_adapter(&idev->adapter);
if (ret) {
dev_err(&pdev->dev, "failed to add adapter\n");
+ clk_disable_unprepare(idev->i2c_clk);
return ret;
}
--
2.7.4
^ permalink raw reply related
* Re: [PATCH 3/3] i2c: expose adapter probe and remove probed functions
From: Wolfram Sang @ 2016-09-16 21:03 UTC (permalink / raw)
To: David Lechner; +Cc: linux-i2c, linux-kernel
In-Reply-To: <1473108014-30787-4-git-send-email-david@lechnology.com>
[-- Attachment #1: Type: text/plain, Size: 371 bytes --]
> The intended use of these functions is for LEGO MINDSTORMS sensors. These
> are hot-plugable I2C devices. When a sensor is connected, i2c_adapter_probe()
> is called to automatically configure the sensor. When the sensor is
> disconnected, i2c_adapter_remove_probed() is called to remove the
> automatically configured device.
How do you detect the hot-plug events?
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [PATCH 3/3] i2c: expose adapter probe and remove probed functions
From: David Lechner @ 2016-09-16 21:09 UTC (permalink / raw)
To: Wolfram Sang; +Cc: linux-i2c, linux-kernel
In-Reply-To: <20160916210315.GB2216@katana>
On 09/16/2016 04:03 PM, Wolfram Sang wrote:
>
>> The intended use of these functions is for LEGO MINDSTORMS sensors. These
>> are hot-plugable I2C devices. When a sensor is connected, i2c_adapter_probe()
>> is called to automatically configure the sensor. When the sensor is
>> disconnected, i2c_adapter_remove_probed() is called to remove the
>> automatically configured device.
>
> How do you detect the hot-plug events?
>
There are "port" drivers that do this. An I2C adapter is assigned to
each port via device-tree or platform data struct.
Port driver code if you are interested:
-
https://github.com/ev3dev/lego-linux-drivers/blob/master/ev3/legoev3_ports_in.c
-
https://github.com/ev3dev/lego-linux-drivers/blob/master/evb/evb_ports_in.c
In particular, here is where we call the new i2c_adapter_probe()
function:
https://github.com/ev3dev/lego-linux-drivers/blob/master/evb/evb_ports_in.c#L543
^ permalink raw reply
* Re: [PATCH v2 0/4] gpio: fix an incorrect lockdep warning
From: Peter Zijlstra @ 2016-09-17 1:19 UTC (permalink / raw)
To: Bartosz Golaszewski
Cc: Linus Walleij, Alexandre Courbot, Andy Shevchenko, Vignesh R,
Yong Li, Geert Uytterhoeven, Ingo Molnar, Wolfram Sang,
Peter Rosin, linux-i2c, linux-gpio, LKML
In-Reply-To: <1474041765-17818-1-git-send-email-bgolaszewski@baylibre.com>
On Fri, Sep 16, 2016 at 06:02:41PM +0200, Bartosz Golaszewski wrote:
> If an I2C GPIO multiplexer is driven by a GPIO provided by an expander
> when there's a second expander using the same device driver on one of
> the I2C bus segments, lockdep prints a deadlock warning when trying to
> set the direction or the value of the GPIOs provided by the second
> expander.
>
> This series exports an already existing function from i2c-core as
> public API and reuses it in pca953x to pass a correct lock subclass
> to lockdep.
Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> Note: if this series gets merged, I'll prepare follow-up patches for
> other expanders for which a similar problem could potentially occur.
We can't push this annotation into the i2c core, can we? Since the mutex
is in driver specific code, not more generic...
^ permalink raw reply
* Re: [PATCH v2 0/4] gpio: fix an incorrect lockdep warning
From: Wolfram Sang @ 2016-09-17 10:18 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Bartosz Golaszewski, Linus Walleij, Alexandre Courbot,
Andy Shevchenko, Vignesh R, Yong Li, Geert Uytterhoeven,
Ingo Molnar, Peter Rosin, linux-i2c, linux-gpio, LKML
In-Reply-To: <20160917011952.GK5016@twins.programming.kicks-ass.net>
[-- Attachment #1: Type: text/plain, Size: 1040 bytes --]
On Sat, Sep 17, 2016 at 03:19:52AM +0200, Peter Zijlstra wrote:
> On Fri, Sep 16, 2016 at 06:02:41PM +0200, Bartosz Golaszewski wrote:
> > If an I2C GPIO multiplexer is driven by a GPIO provided by an expander
> > when there's a second expander using the same device driver on one of
> > the I2C bus segments, lockdep prints a deadlock warning when trying to
> > set the direction or the value of the GPIOs provided by the second
> > expander.
> >
> > This series exports an already existing function from i2c-core as
> > public API and reuses it in pca953x to pass a correct lock subclass
> > to lockdep.
>
> Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Oops, I overlooked that I need your ack as well :) Thanks!
> > Note: if this series gets merged, I'll prepare follow-up patches for
> > other expanders for which a similar problem could potentially occur.
>
> We can't push this annotation into the i2c core, can we? Since the mutex
> is in driver specific code, not more generic...
Afraid so.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [PATCH v2 0/4] gpio: fix an incorrect lockdep warning
From: Bartosz Golaszewski @ 2016-09-17 18:59 UTC (permalink / raw)
To: Wolfram Sang
Cc: Peter Zijlstra, Linus Walleij, Alexandre Courbot, Andy Shevchenko,
Vignesh R, Yong Li, Geert Uytterhoeven, Ingo Molnar, Peter Rosin,
linux-i2c, linux-gpio, LKML
In-Reply-To: <20160917101821.GB4753@katana>
2016-09-17 12:18 GMT+02:00 Wolfram Sang <wsa@the-dreams.de>:
> On Sat, Sep 17, 2016 at 03:19:52AM +0200, Peter Zijlstra wrote:
>> On Fri, Sep 16, 2016 at 06:02:41PM +0200, Bartosz Golaszewski wrote:
>> > If an I2C GPIO multiplexer is driven by a GPIO provided by an expander
>> > when there's a second expander using the same device driver on one of
>> > the I2C bus segments, lockdep prints a deadlock warning when trying to
>> > set the direction or the value of the GPIOs provided by the second
>> > expander.
>> >
>> > This series exports an already existing function from i2c-core as
>> > public API and reuses it in pca953x to pass a correct lock subclass
>> > to lockdep.
>>
>> Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
>
> Oops, I overlooked that I need your ack as well :) Thanks!
>
>> > Note: if this series gets merged, I'll prepare follow-up patches for
>> > other expanders for which a similar problem could potentially occur.
>>
>> We can't push this annotation into the i2c core, can we? Since the mutex
>> is in driver specific code, not more generic...
>
> Afraid so.
>
Most i2c controlled GPIO expanders could potentially be converted to
using the regmap framework which unfortunately suffers from the same
locking issue in its current form.
I'm already working on adding adapter depth dependent subclasses for
i2c regmaps - maybe I'll be able to submit it for 4.9 next week.
Using regmap would allow us to drop the separate mutexes in each gpio
driver and instead use the regmap-internal locking.
Best regards,
Bartosz Golaszewski
^ permalink raw reply
* Re: [PATCH] i2c-eg20t: use dynamically registered adapter number
From: Wolfram Sang @ 2016-09-17 21:49 UTC (permalink / raw)
To: Jean Delvare; +Cc: Yadi Hu, linux-i2c
In-Reply-To: <20160826173019.1f939ce5@endymion>
[-- Attachment #1: Type: text/plain, Size: 2412 bytes --]
> > The eg20t driver uses i2c_add_numbered_adapter() to register adapter:
> >
> > pch_adap->nr = i;
> > ret = i2c_add_numbered_adapter(pch_adap);
> >
> > Variable i is assigned to 0, it means that i2c_eg20t is the first adapter
> > by default. if another adapter registers before eg20t, above code return
> > error for index conflict:
> >
> > i2c_eg20t 0000:05:0c.2: pch_i2c_probe :i2c_add_adapter[ch:0] FAILED
> > i2c_eg20t: probe of 0000:05:0c.2 failed with error -16
> >
> > So, we can replace i2c_add_numbered_adapter() with i2c_add_adapter()
> > interface.since it dynamically allocates the index number.
>
> This does the exact opposite of:
>
> commit 07e8a51ff68353e01d795cceafbac9f54c49132b
> Author: Feng Tang <feng.tang@intel.com>
> Date: Thu Jan 12 15:38:02 2012 +0800
>
> i2c-eg20t: use i2c_add_numbered_adapter to get a fixed bus number
We have the same problem with i2c-pasemi:
http://www.spinics.net/lists/linux-i2c/msg25761.html
PCI + i2c_add_numbered_adapter + another device adding i2c busses
And we cannot simply change back to i2c_add_adapter() because it will
regress on those systems which originally wanted to have
i2c_add_numbered_adapter().
> Looking at
>
> commit 03bde7c31a360f814ca42101d60563b1b803dca1
> Author: Wolfram Sang <wsa+renesas@sang-engineering.com>
> Date: Thu Mar 12 17:17:59 2015 +0100
>
> i2c: busses with dynamic ids should start after fixed ids for DT
>
> it could be that you need to set some OF attribute to reserve i2c bus
> numbers <= 1 for static usage. Assuming you use OF. Or is it automatic,
> Wolfram?
No, you need to define an alias in the devicetree. I don't think this
platform is a DT user, though?
> If not, it may make sense to add a helper function exposing
> __i2c_first_dynamic_bus_num to drivers (something like
> i2c_is_dynamic_bus_num().) After all, i2c_add_numbered_adapter() mostly
> makes sense if static i2c device definitions exist. If not,
> i2c_add_adapter() is just as good. So something like:
>
> if (i2c_is_dynamic_bus_num(i))
> ret = i2c_add_adapter(pch_adap);
> else {
> pch_adap->nr = i;
> ret = i2c_add_numbered_adapter(pch_adap);
> }
>
> may make sense. Unless someone has a better idea.
Interesting idea. Need to think about it some more. I didn't have a
proper solution so far...
Thanks,
Wolfram
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox