All of lore.kernel.org
 help / color / mirror / Atom feed
* lockdep: incorrect deadlock warning with two GPIO expanders
@ 2016-09-12 11:51 Bartosz Golaszewski
  2016-09-12 12:09 ` Peter Zijlstra
  0 siblings, 1 reply; 16+ messages in thread
From: Bartosz Golaszewski @ 2016-09-12 11:51 UTC (permalink / raw)
  To: Peter Zijlstra, Ingo Molnar; +Cc: LKML

I'm trying to figure out a way of getting rid of an incorrect lockdep
deadlock warning, but the issue is not trivial.

In our hardware an I2C multiplexer is controlled by a GPIO provided by
an expander. There's a second expander using the same device driver
(pca953x) on one of the I2C bus segments. The diagram below 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 |
                                            |            |
                                             ------------

Lockdep prints a deadlock message when trying to set the direction or
get/set the value of any GPIO provided by the second expander.

The reason for the warning is that we take the chip->i2c_lock in
pca953x_gpio_set/get_value() or pca953x_gpio_direction_input/output()
and then come right back to pca953x_gpio_set_value() when the GPIO mux
kicks in. The call path is as follows (starting from a fixed regulator
controlled by a GPIO provided by the second expander):

  regulator_register()
  |_ _regulator_do_enable()
     |_ pca953x_gpio_set_value() <- mutex_lock
        |_ pca953x_write_single()
           |_ i2c_smbus_write_byte_data()
              |_ i2c_smbus_xfer()
                 |_ i2c_transfer()
                    |_ __i2c_transfer()
                       |_ i2c_mux_master_xfer()
                          |_ i2c_mux_gpio_select()
                             |_ i2c_mux_gpio_set()
                                |_ pca953x_gpio_set_value() <- 2nd mutex_lock

 The locks actually protect different expanders, but lockdep doesn't
see this and says:

  Possible unsafe locking scenario:

        CPU0
        ----
   lock(&chip->i2c_lock);
   lock(&chip->i2c_lock);

  *** DEADLOCK ***

  May be due to missing lock nesting notation

Using mutex_lock_nested(&chip->i2c_lock, SINGLE_DEPTH_NESTING) in
pca953x_gpio_get_value() and pca953x_gpio_direction_input/output()
helps for reading the values or setting the direction, but doesn't do
anything if used in pca953x_gpio_set_value() since we still end up
taking the lock of the same subclass again.

It would require some nasty hacks to figure out that a GPIO is being
used by an I2C mux if we wanted to explicitly provide a different
sublass in this case, but that would not fix the culprit, since the
same problem would occur in other gpio drivers under similar
circumstances.

It seems the problem is with the way lockdep works, but I lack the
knowledge to propose any solution.

Any help & ideas are appreciated.

Best regards,
Bartosz Golaszewski

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

* Re: lockdep: incorrect deadlock warning with two GPIO expanders
  2016-09-12 11:51 lockdep: incorrect deadlock warning with two GPIO expanders Bartosz Golaszewski
@ 2016-09-12 12:09 ` Peter Zijlstra
  2016-09-12 15:16   ` Bartosz Golaszewski
  0 siblings, 1 reply; 16+ messages in thread
From: Peter Zijlstra @ 2016-09-12 12:09 UTC (permalink / raw)
  To: Bartosz Golaszewski; +Cc: Ingo Molnar, LKML

On Mon, Sep 12, 2016 at 01:51:55PM +0200, Bartosz Golaszewski wrote:
> I'm trying to figure out a way of getting rid of an incorrect lockdep
> deadlock warning, but the issue is not trivial.
> 
> In our hardware an I2C multiplexer is controlled by a GPIO provided by
> an expander. There's a second expander using the same device driver
> (pca953x) on one of the I2C bus segments. The diagram below 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 |
>                                             |            |
>                                              ------------

> Using mutex_lock_nested(&chip->i2c_lock, SINGLE_DEPTH_NESTING) in
> pca953x_gpio_get_value() and pca953x_gpio_direction_input/output()
> helps for reading the values or setting the direction, but doesn't do
> anything if used in pca953x_gpio_set_value() since we still end up
> taking the lock of the same subclass again.
> 
> It would require some nasty hacks to figure out that a GPIO is being
> used by an I2C mux if we wanted to explicitly provide a different
> sublass in this case, but that would not fix the culprit, since the
> same problem would occur in other gpio drivers under similar
> circumstances.
> 
> It seems the problem is with the way lockdep works, but I lack the
> knowledge to propose any solution.
> 
> Any help & ideas are appreciated.

So I'm entirely clueless on how the device model works let alone i2c
and/or gpio. So I'm going to need some help as well. What's an SCL/SDA
for instance?

So the 'problem' is that pca953x_probe()'s mutex_init() will collapse
all mutexes it initializes into a single class. It assumes that the
locking rules for all instances will be the same.

This happens to not be true in this case.

The tricky part, and here I have absolutely no clue what so ever, is
being able to tell at pca953x_probe() time that this is so.

Once we can tell, at probe time, there are two different annotations we
could use, depending on need.

I suppose that theoretically you can keep nesting like that ad
infinitum, but I also expect that its uncommon enough, and maybe not
practical, to really nest like this -- seeing this is the first instance
of such issues.

In any case, can you tell at probe time? And how deep a nesting should
we worry about?

Seeing how this lock is specific to the driver, and there is no generic
infrastructure, I don't see how we could solve it other than on a
per-driver basis.

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

* Re: lockdep: incorrect deadlock warning with two GPIO expanders
  2016-09-12 12:09 ` Peter Zijlstra
@ 2016-09-12 15:16   ` Bartosz Golaszewski
  2016-09-12 15:33     ` Peter Zijlstra
  0 siblings, 1 reply; 16+ messages in thread
From: Bartosz Golaszewski @ 2016-09-12 15:16 UTC (permalink / raw)
  To: Peter Zijlstra, Linus Walleij, Alexandre Courbot, Ingo Molnar
  Cc: LKML, linux-gpio

  + Linus, Alexandre, linux-gpio

2016-09-12 14:09 GMT+02:00 Peter Zijlstra <peterz@infradead.org>:
>2016-09-12 13:51 GMT+02:00 Bartosz Golaszewski <bgolaszewski@baylibre.com>:
>> I'm trying to figure out a way of getting rid of an incorrect lockdep
>> deadlock warning, but the issue is not trivial.
>>
>> In our hardware an I2C multiplexer is controlled by a GPIO provided by
>> an expander. There's a second expander using the same device driver
>> (pca953x) on one of the I2C bus segments. The diagram below 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 |
>>                                             |            |
>>                                              ------------
>>
>> Lockdep prints a deadlock message when trying to set the direction or
>> get/set the value of any GPIO provided by the second expander.
>>
>> The reason for the warning is that we take the chip->i2c_lock in
>> pca953x_gpio_set/get_value() or pca953x_gpio_direction_input/output()
>> and then come right back to pca953x_gpio_set_value() when the GPIO mux
>> kicks in. The call path is as follows (starting from a fixed regulator
>> controlled by a GPIO provided by the second expander):
>>
>>   regulator_register()
>>   |_ _regulator_do_enable()
>>      |_ pca953x_gpio_set_value() <- mutex_lock
>>         |_ pca953x_write_single()
>>            |_ i2c_smbus_write_byte_data()
>>               |_ i2c_smbus_xfer()
>>                  |_ i2c_transfer()
>>                     |_ __i2c_transfer()
>>                        |_ i2c_mux_master_xfer()
>>                           |_ i2c_mux_gpio_select()
>>                              |_ i2c_mux_gpio_set()
>>                                 |_ pca953x_gpio_set_value() <- 2nd mutex_lock
>>
>> The locks actually protect different expanders, but lockdep doesn't
>> see this and says:
>>
>>   Possible unsafe locking scenario:
>>
>>         CPU0
>>         ----
>>    lock(&chip->i2c_lock);
>>    lock(&chip->i2c_lock);
>>
>>   *** DEADLOCK ***
>>
>>   May be due to missing lock nesting notation
>>
>> Using mutex_lock_nested(&chip->i2c_lock, SINGLE_DEPTH_NESTING) in
>> pca953x_gpio_get_value() and pca953x_gpio_direction_input/output()
>> helps for reading the values or setting the direction, but doesn't do
>> anything if used in pca953x_gpio_set_value() since we still end up
>> taking the lock of the same subclass again.
>>
>> It would require some nasty hacks to figure out that a GPIO is being
>> used by an I2C mux if we wanted to explicitly provide a different
>> sublass in this case, but that would not fix the culprit, since the
>> same problem would occur in other gpio drivers under similar
>> circumstances.
>>
>> It seems the problem is with the way lockdep works, but I lack the
>> knowledge to propose any solution.
>>
>> Any help & ideas are appreciated.
>
> So I'm entirely clueless on how the device model works let alone i2c
> and/or gpio. So I'm going to need some help as well. What's an SCL/SDA
> for instance?
>

SCL/SDA stands for I2C clock and data lines. It's not important for
the problem, I just used it to mark the physical layer between the SoC
and I2C multiplexer.

> So the 'problem' is that pca953x_probe()'s mutex_init() will collapse
> all mutexes it initializes into a single class. It assumes that the
> locking rules for all instances will be the same.
>
> This happens to not be true in this case.
>

Correct.

> The tricky part, and here I have absolutely no clue what so ever, is
> being able to tell at pca953x_probe() time that this is so.
>

AFAIK there is no clean way to tell that a GPIO is used by an I2C
multiplexer at probe time. Linus, Alexandre could you confirm?

> Once we can tell, at probe time, there are two different annotations we
> could use, depending on need.
>

My current workaround is the following patch:

diff --git a/drivers/gpio/gpio-pca953x.c b/drivers/gpio/gpio-pca953x.c
index 5e3be32..40b96bf 100644
--- a/drivers/gpio/gpio-pca953x.c
+++ b/drivers/gpio/gpio-pca953x.c
@@ -739,6 +739,7 @@ static const struct of_device_id pca953x_dt_ids[];
 static int pca953x_probe(struct i2c_client *client,
    const struct i2c_device_id *id)
 {
+ struct lock_class_key *lock_key = (struct lock_class_key *)id;
  struct pca953x_platform_data *pdata;
  struct pca953x_chip *chip;
  int irq_base = 0;
@@ -783,7 +784,7 @@ static int pca953x_probe(struct i2c_client *client,

  chip->chip_type = PCA_CHIP_TYPE(chip->driver_data);

- mutex_init(&chip->i2c_lock);
+ __mutex_init(&chip->i2c_lock, "chip->i2c_lock", lock_key);

  /* initialize cached registers from their original values.
  * we can't share this chip with another i2c master.

It uses the fact that the two expanders we have are of different type
(pca9534 and pca9535). The id pointer points to per-chip device info
residing in .data which makes it suitable for mutex key.

I don't think such hack is suitable for mainline though.

> I suppose that theoretically you can keep nesting like that ad
> infinitum, but I also expect that its uncommon enough, and maybe not
> practical, to really nest like this -- seeing this is the first instance
> of such issues.
>
> In any case, can you tell at probe time? And how deep a nesting should
> we worry about?
>

In this case we would only need two classes - one for 'regular' GPIOs
and another for the GPIOs used to control an I2C multiplexer.

> Seeing how this lock is specific to the driver, and there is no generic
> infrastructure, I don't see how we could solve it other than on a
> per-driver basis.

I'm planning to convert this driver to using regmap and I'm afraid
we'll run into similar issue (given that regmap doesn't nest the
default mutex nor uses different classes).

Best regards,
Bartosz Golaszewski

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

* Re: lockdep: incorrect deadlock warning with two GPIO expanders
  2016-09-12 15:16   ` Bartosz Golaszewski
@ 2016-09-12 15:33     ` Peter Zijlstra
  2016-09-13 12:29       ` Linus Walleij
  0 siblings, 1 reply; 16+ messages in thread
From: Peter Zijlstra @ 2016-09-12 15:33 UTC (permalink / raw)
  To: Bartosz Golaszewski
  Cc: Linus Walleij, Alexandre Courbot, Ingo Molnar, LKML, linux-gpio

On Mon, Sep 12, 2016 at 05:16:14PM +0200, Bartosz Golaszewski wrote:

> >>                                                - - - - -
> >>  -------             ---------  Bus segment 1 |         |
> >> |       |           |         |---------------  Devices
> >> |       | SCL/SDA   |         |               |         |
> >> | Linux |-----------| I2C MUX |                - - - - -
> >> |       |    |      |         | Bus segment 2
> >> |       |    |      |         |-------------------
> >>  -------     |       ---------                    |
> >>              |           |                    - - - - -
> >>         ------------     | MUX GPIO          |         |
> >>        |            |    |                     Devices
> >>        |    GPIO    |    |                   |         |
> >>        | Expander 1 |----                     - - - - -
> >>        |            |                             |
> >>         ------------                              | SCL/SDA
> >>                                                   |
> >>                                              ------------
> >>                                             |            |
> >>                                             |    GPIO    |
> >>                                             | Expander 2 |
> >>                                             |            |
> >>                                              ------------

> > The tricky part, and here I have absolutely no clue what so ever, is
> > being able to tell at pca953x_probe() time that this is so.
> >
> 
> AFAIK there is no clean way to tell that a GPIO is used by an I2C
> multiplexer at probe time. Linus, Alexandre could you confirm?

You cannot inspect the device tree while probing?

> It uses the fact that the two expanders we have are of different type
> (pca9534 and pca9535). The id pointer points to per-chip device info
> residing in .data which makes it suitable for mutex key.
> 
> I don't think such hack is suitable for mainline though.

Right, that works by accident rather than anything else :-)

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

* Re: lockdep: incorrect deadlock warning with two GPIO expanders
  2016-09-12 15:33     ` Peter Zijlstra
@ 2016-09-13 12:29       ` Linus Walleij
  2016-09-15  7:51         ` Peter Zijlstra
  0 siblings, 1 reply; 16+ messages in thread
From: Linus Walleij @ 2016-09-13 12:29 UTC (permalink / raw)
  To: Peter Zijlstra
  Cc: Bartosz Golaszewski, Alexandre Courbot, Ingo Molnar, LKML, linux-gpio

On Mon, Sep 12, 2016 at 5:33 PM, Peter Zijlstra <peterz@infradead.org> wrote:
> On Mon, Sep 12, 2016 at 05:16:14PM +0200, Bartosz Golaszewski wrote:

>> AFAIK there is no clean way to tell that a GPIO is used by an I2C
>> multiplexer at probe time. Linus, Alexandre could you confirm?

Nominally, the GPIO descriptors are just abstract resources such
as regulators or clocks, they can be used for a lot but just like
a clock, regulator, dma channel etc does not know who is using
it and for what, it does not know this, no.

> You cannot inspect the device tree while probing?

Of course it *can* but we would end up encoding a special
case every time something like this happens, tied to just
device tree, then another bolt-on for ACPI etc.

I have a hard time following the problem really, I'm
afraid I'm simply just not smart enough :(

Yours,
Linus Walleij

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

* Re: lockdep: incorrect deadlock warning with two GPIO expanders
  2016-09-13 12:29       ` Linus Walleij
@ 2016-09-15  7:51         ` Peter Zijlstra
  2016-09-15 12:41           ` Linus Walleij
  0 siblings, 1 reply; 16+ messages in thread
From: Peter Zijlstra @ 2016-09-15  7:51 UTC (permalink / raw)
  To: Linus Walleij
  Cc: Bartosz Golaszewski, Alexandre Courbot, Ingo Molnar, LKML, linux-gpio

On Tue, Sep 13, 2016 at 02:29:24PM +0200, Linus Walleij wrote:
> On Mon, Sep 12, 2016 at 5:33 PM, Peter Zijlstra <peterz@infradead.org> wrote:
> > On Mon, Sep 12, 2016 at 05:16:14PM +0200, Bartosz Golaszewski wrote:
> 
> >> AFAIK there is no clean way to tell that a GPIO is used by an I2C
> >> multiplexer at probe time. Linus, Alexandre could you confirm?
> 
> Nominally, the GPIO descriptors are just abstract resources such
> as regulators or clocks, they can be used for a lot but just like
> a clock, regulator, dma channel etc does not know who is using
> it and for what, it does not know this, no.
> 
> > You cannot inspect the device tree while probing?
> 
> Of course it *can* but we would end up encoding a special
> case every time something like this happens, tied to just
> device tree, then another bolt-on for ACPI etc.
> 
> I have a hard time following the problem really, I'm
> afraid I'm simply just not smart enough :(

Why would this be DT or ACPI specific? Linux itself has a tree/graph of
all busses and devices right? That's what all this drivers/base/ stuff
is on about.

So can't you walk up that and see if you encounter the exact same driver
again?

Something like:

	for (nr = 0, parent = dev->parent; parent; parent = parent->parent) {
		if (parent->device_driver == &pca953x_driver.driver)
			nr++;
	}

Again, I have no clue how any of this works, but that seems like
something that ought to work.

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

* Re: lockdep: incorrect deadlock warning with two GPIO expanders
  2016-09-15  7:51         ` Peter Zijlstra
@ 2016-09-15 12:41           ` Linus Walleij
  2016-09-15 13:20             ` Bartosz Golaszewski
  2016-09-16 14:21             ` Bartosz Golaszewski
  0 siblings, 2 replies; 16+ messages in thread
From: Linus Walleij @ 2016-09-15 12:41 UTC (permalink / raw)
  To: Peter Zijlstra
  Cc: Bartosz Golaszewski, Alexandre Courbot, Ingo Molnar, LKML, linux-gpio

On Thu, Sep 15, 2016 at 9:51 AM, Peter Zijlstra <peterz@infradead.org> wrote:
> On Tue, Sep 13, 2016 at 02:29:24PM +0200, Linus Walleij wrote:
>> On Mon, Sep 12, 2016 at 5:33 PM, Peter Zijlstra <peterz@infradead.org> wrote:
>> > On Mon, Sep 12, 2016 at 05:16:14PM +0200, Bartosz Golaszewski wrote:
>>
>> >> AFAIK there is no clean way to tell that a GPIO is used by an I2C
>> >> multiplexer at probe time. Linus, Alexandre could you confirm?
>>
>> Nominally, the GPIO descriptors are just abstract resources such
>> as regulators or clocks, they can be used for a lot but just like
>> a clock, regulator, dma channel etc does not know who is using
>> it and for what, it does not know this, no.
>>
>> > You cannot inspect the device tree while probing?
>>
>> Of course it *can* but we would end up encoding a special
>> case every time something like this happens, tied to just
>> device tree, then another bolt-on for ACPI etc.
>>
>> I have a hard time following the problem really, I'm
>> afraid I'm simply just not smart enough :(
>
> Why would this be DT or ACPI specific? Linux itself has a tree/graph of
> all busses and devices right? That's what all this drivers/base/ stuff
> is on about.
>
> So can't you walk up that and see if you encounter the exact same driver
> again?
>
> Something like:
>
>         for (nr = 0, parent = dev->parent; parent; parent = parent->parent) {
>                 if (parent->device_driver == &pca953x_driver.driver)
>                         nr++;
>         }

Oh clever. Of course.

Bartosz can you try out this approach?

Yours,
Linus Walleij

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

* Re: lockdep: incorrect deadlock warning with two GPIO expanders
  2016-09-15 12:41           ` Linus Walleij
@ 2016-09-15 13:20             ` Bartosz Golaszewski
  2016-09-15 13:39               ` Peter Zijlstra
  2016-09-16 14:21             ` Bartosz Golaszewski
  1 sibling, 1 reply; 16+ messages in thread
From: Bartosz Golaszewski @ 2016-09-15 13:20 UTC (permalink / raw)
  To: Linus Walleij
  Cc: Peter Zijlstra, Alexandre Courbot, Ingo Molnar, LKML, linux-gpio

2016-09-15 14:41 GMT+02:00 Linus Walleij <linus.walleij@linaro.org>:
> On Thu, Sep 15, 2016 at 9:51 AM, Peter Zijlstra <peterz@infradead.org> wrote:
>> On Tue, Sep 13, 2016 at 02:29:24PM +0200, Linus Walleij wrote:
>>> On Mon, Sep 12, 2016 at 5:33 PM, Peter Zijlstra <peterz@infradead.org> wrote:
>>> > On Mon, Sep 12, 2016 at 05:16:14PM +0200, Bartosz Golaszewski wrote:
>>>
>>> >> AFAIK there is no clean way to tell that a GPIO is used by an I2C
>>> >> multiplexer at probe time. Linus, Alexandre could you confirm?
>>>
>>> Nominally, the GPIO descriptors are just abstract resources such
>>> as regulators or clocks, they can be used for a lot but just like
>>> a clock, regulator, dma channel etc does not know who is using
>>> it and for what, it does not know this, no.
>>>
>>> > You cannot inspect the device tree while probing?
>>>
>>> Of course it *can* but we would end up encoding a special
>>> case every time something like this happens, tied to just
>>> device tree, then another bolt-on for ACPI etc.
>>>
>>> I have a hard time following the problem really, I'm
>>> afraid I'm simply just not smart enough :(
>>
>> Why would this be DT or ACPI specific? Linux itself has a tree/graph of
>> all busses and devices right? That's what all this drivers/base/ stuff
>> is on about.
>>
>> So can't you walk up that and see if you encounter the exact same driver
>> again?
>>
>> Something like:
>>
>>         for (nr = 0, parent = dev->parent; parent; parent = parent->parent) {
>>                 if (parent->device_driver == &pca953x_driver.driver)
>>                         nr++;
>>         }
>
> Oh clever. Of course.
>
> Bartosz can you try out this approach?
>

I think it may be more complicated than that, depending on the hw
topology, but the general idea seems reasonable. I'll try this.

Thanks,
Bartosz Golaszewski

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

* Re: lockdep: incorrect deadlock warning with two GPIO expanders
  2016-09-15 13:20             ` Bartosz Golaszewski
@ 2016-09-15 13:39               ` Peter Zijlstra
  2016-09-15 14:08                 ` Bartosz Golaszewski
  2016-09-16 10:56                 ` Peter Zijlstra
  0 siblings, 2 replies; 16+ messages in thread
From: Peter Zijlstra @ 2016-09-15 13:39 UTC (permalink / raw)
  To: Bartosz Golaszewski
  Cc: Linus Walleij, Alexandre Courbot, Ingo Molnar, LKML, linux-gpio

On Thu, Sep 15, 2016 at 03:20:58PM +0200, Bartosz Golaszewski wrote:
> 2016-09-15 14:41 GMT+02:00 Linus Walleij <linus.walleij@linaro.org>:
> >> So can't you walk up that and see if you encounter the exact same driver
> >> again?
> >>
> >> Something like:
> >>
> >>         for (nr = 0, parent = dev->parent; parent; parent = parent->parent) {
> >>                 if (parent->device_driver == &pca953x_driver.driver)
> >>                         nr++;
> >>         }
> >
> > Oh clever. Of course.
> >
> > Bartosz can you try out this approach?
> >
> 
> I think it may be more complicated than that, depending on the hw
> topology, but the general idea seems reasonable. I'll try this.

Yeah, I figured there might be more to it.

In any case, if this fails, we can always punt and simply count the
total number of instances of this driver on the system and go with that.


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

* Re: lockdep: incorrect deadlock warning with two GPIO expanders
  2016-09-15 13:39               ` Peter Zijlstra
@ 2016-09-15 14:08                 ` Bartosz Golaszewski
  2016-09-15 14:38                   ` Peter Zijlstra
  2016-09-16 10:56                 ` Peter Zijlstra
  1 sibling, 1 reply; 16+ messages in thread
From: Bartosz Golaszewski @ 2016-09-15 14:08 UTC (permalink / raw)
  To: Peter Zijlstra
  Cc: Linus Walleij, Alexandre Courbot, Ingo Molnar, LKML, linux-gpio

2016-09-15 15:39 GMT+02:00 Peter Zijlstra <peterz@infradead.org>:
> On Thu, Sep 15, 2016 at 03:20:58PM +0200, Bartosz Golaszewski wrote:
>> 2016-09-15 14:41 GMT+02:00 Linus Walleij <linus.walleij@linaro.org>:
>> >> So can't you walk up that and see if you encounter the exact same driver
>> >> again?
>> >>
>> >> Something like:
>> >>
>> >>         for (nr = 0, parent = dev->parent; parent; parent = parent->parent) {
>> >>                 if (parent->device_driver == &pca953x_driver.driver)
>> >>                         nr++;
>> >>         }
>> >
>> > Oh clever. Of course.
>> >
>> > Bartosz can you try out this approach?
>> >
>>
>> I think it may be more complicated than that, depending on the hw
>> topology, but the general idea seems reasonable. I'll try this.
>
> Yeah, I figured there might be more to it.
>
> In any case, if this fails, we can always punt and simply count the
> total number of instances of this driver on the system and go with that.
>

But for __mutex_init() to work with the key argument you need to know
it at compile time, right?

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

* Re: lockdep: incorrect deadlock warning with two GPIO expanders
  2016-09-15 14:08                 ` Bartosz Golaszewski
@ 2016-09-15 14:38                   ` Peter Zijlstra
  2016-09-15 15:23                     ` Bartosz Golaszewski
  0 siblings, 1 reply; 16+ messages in thread
From: Peter Zijlstra @ 2016-09-15 14:38 UTC (permalink / raw)
  To: Bartosz Golaszewski
  Cc: Linus Walleij, Alexandre Courbot, Ingo Molnar, LKML, linux-gpio

On Thu, Sep 15, 2016 at 04:08:52PM +0200, Bartosz Golaszewski wrote:
> 2016-09-15 15:39 GMT+02:00 Peter Zijlstra <peterz@infradead.org>:

> > In any case, if this fails, we can always punt and simply count the
> > total number of instances of this driver on the system and go with that.
> >
> 
> But for __mutex_init() to work with the key argument you need to know
> it at compile time, right?

You can do something like:

	mutex_init(&mutex);
	lockdep_set_subclass(&mutex, nr);

which will of course fail at runtime the moment nr >= 8, but is that
really a concern?

Equally you can do:

static struct lock_class_key my_keys[NR];

	mutex_init(&mutex);
	BUG_ON(nr > NR);
	lockdep_set_class(&mutex, my_keys + nr);

and have a bigger limit.

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

* Re: lockdep: incorrect deadlock warning with two GPIO expanders
  2016-09-15 14:38                   ` Peter Zijlstra
@ 2016-09-15 15:23                     ` Bartosz Golaszewski
  0 siblings, 0 replies; 16+ messages in thread
From: Bartosz Golaszewski @ 2016-09-15 15:23 UTC (permalink / raw)
  To: Peter Zijlstra
  Cc: Linus Walleij, Alexandre Courbot, Ingo Molnar, LKML, linux-gpio

2016-09-15 16:38 GMT+02:00 Peter Zijlstra <peterz@infradead.org>:
> On Thu, Sep 15, 2016 at 04:08:52PM +0200, Bartosz Golaszewski wrote:
>> 2016-09-15 15:39 GMT+02:00 Peter Zijlstra <peterz@infradead.org>:
>
>> > In any case, if this fails, we can always punt and simply count the
>> > total number of instances of this driver on the system and go with that.
>> >
>>
>> But for __mutex_init() to work with the key argument you need to know
>> it at compile time, right?
>
> You can do something like:
>
>         mutex_init(&mutex);
>         lockdep_set_subclass(&mutex, nr);
>
> which will of course fail at runtime the moment nr >= 8, but is that
> really a concern?
>
> Equally you can do:
>
> static struct lock_class_key my_keys[NR];
>
>         mutex_init(&mutex);
>         BUG_ON(nr > NR);
>         lockdep_set_class(&mutex, my_keys + nr);
>
> and have a bigger limit.

Thanks, I was not aware of this API.

Best regards,
Bartosz Golaszewski

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

* Re: lockdep: incorrect deadlock warning with two GPIO expanders
  2016-09-15 13:39               ` Peter Zijlstra
  2016-09-15 14:08                 ` Bartosz Golaszewski
@ 2016-09-16 10:56                 ` Peter Zijlstra
  2016-09-16 11:14                   ` Bartosz Golaszewski
  1 sibling, 1 reply; 16+ messages in thread
From: Peter Zijlstra @ 2016-09-16 10:56 UTC (permalink / raw)
  To: Bartosz Golaszewski
  Cc: Linus Walleij, Alexandre Courbot, Ingo Molnar, LKML, linux-gpio

On Thu, Sep 15, 2016 at 03:39:22PM +0200, Peter Zijlstra wrote:

> In any case, if this fails, we can always punt and simply count the
> total number of instances of this driver on the system and go with that.

One caveat with that approach is that if this is s module, someone doing
rmmod, insmod a number of times gets tricky.

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

* Re: lockdep: incorrect deadlock warning with two GPIO expanders
  2016-09-16 10:56                 ` Peter Zijlstra
@ 2016-09-16 11:14                   ` Bartosz Golaszewski
  2016-09-16 12:00                     ` Bartosz Golaszewski
  0 siblings, 1 reply; 16+ messages in thread
From: Bartosz Golaszewski @ 2016-09-16 11:14 UTC (permalink / raw)
  To: Peter Zijlstra
  Cc: Linus Walleij, Alexandre Courbot, Ingo Molnar, LKML, linux-gpio

2016-09-16 12:56 GMT+02:00 Peter Zijlstra <peterz@infradead.org>:
> On Thu, Sep 15, 2016 at 03:39:22PM +0200, Peter Zijlstra wrote:
>
>> In any case, if this fails, we can always punt and simply count the
>> total number of instances of this driver on the system and go with that.
>
> One caveat with that approach is that if this is s module, someone doing
> rmmod, insmod a number of times gets tricky.

The whole thing is a bit more complicated - I'm trying to figure out
the code behind it, but the first expander has four parents whose
driver == pca953x, while the second has five of them. There is no
device name, but kobj->name corresponds with the i2c device
sub-directories on correct addresses and busses (1-0021 for the first
one and 4-0020 for the second on my board).

With both devices having pca953x parents I still can't tell one from
another. Still working on it.

Thanks,
Bartosz Golaszewski

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

* Re: lockdep: incorrect deadlock warning with two GPIO expanders
  2016-09-16 11:14                   ` Bartosz Golaszewski
@ 2016-09-16 12:00                     ` Bartosz Golaszewski
  0 siblings, 0 replies; 16+ messages in thread
From: Bartosz Golaszewski @ 2016-09-16 12:00 UTC (permalink / raw)
  To: Peter Zijlstra
  Cc: Linus Walleij, Alexandre Courbot, Ingo Molnar, LKML, linux-gpio

2016-09-16 13:14 GMT+02:00 Bartosz Golaszewski <bgolaszewski@baylibre.com>:
>
> The whole thing is a bit more complicated - I'm trying to figure out
> the code behind it, but the first expander has four parents whose
> driver == pca953x, while the second has five of them. There is no
> device name, but kobj->name corresponds with the i2c device
> sub-directories on correct addresses and busses (1-0021 for the first
> one and 4-0020 for the second on my board).

Actually nevermind my last e-mail - I did a braindead mistake when
obtaining logs.

Thanks,
Bartosz

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

* Re: lockdep: incorrect deadlock warning with two GPIO expanders
  2016-09-15 12:41           ` Linus Walleij
  2016-09-15 13:20             ` Bartosz Golaszewski
@ 2016-09-16 14:21             ` Bartosz Golaszewski
  1 sibling, 0 replies; 16+ messages in thread
From: Bartosz Golaszewski @ 2016-09-16 14:21 UTC (permalink / raw)
  To: Linus Walleij
  Cc: Peter Zijlstra, Alexandre Courbot, Ingo Molnar, LKML, linux-gpio

2016-09-15 14:41 GMT+02:00 Linus Walleij <linus.walleij@linaro.org>:
>
> Oh clever. Of course.
>
> Bartosz can you try out this approach?
>
> Yours,
> Linus Walleij

Hi Linus,

I submitted a patch reusing a routine already present in i2c.h that
allows to test if an i2c adapter's parent is also an i2c adapter,
which is the case for adapters behind a multiplexer.

Also Cc'ed linux-i2c and Wolfram Sang to let him take a look if the
API is used correctly.

Best regards,
Bartosz Golaszewski

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

end of thread, other threads:[~2016-09-16 14:21 UTC | newest]

Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-09-12 11:51 lockdep: incorrect deadlock warning with two GPIO expanders Bartosz Golaszewski
2016-09-12 12:09 ` Peter Zijlstra
2016-09-12 15:16   ` Bartosz Golaszewski
2016-09-12 15:33     ` Peter Zijlstra
2016-09-13 12:29       ` Linus Walleij
2016-09-15  7:51         ` Peter Zijlstra
2016-09-15 12:41           ` Linus Walleij
2016-09-15 13:20             ` Bartosz Golaszewski
2016-09-15 13:39               ` Peter Zijlstra
2016-09-15 14:08                 ` Bartosz Golaszewski
2016-09-15 14:38                   ` Peter Zijlstra
2016-09-15 15:23                     ` Bartosz Golaszewski
2016-09-16 10:56                 ` Peter Zijlstra
2016-09-16 11:14                   ` Bartosz Golaszewski
2016-09-16 12:00                     ` Bartosz Golaszewski
2016-09-16 14:21             ` Bartosz Golaszewski

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.