Re: [PATCH v6 08/24] iio: imu: inv_mpu6050: convert to use an explicit i2c mux core
,Arnd Bergmann ,Tommi Rantala ,linux-...@vger.kernel.org,linux-...@vger.kernel.org,linux-...@vger.kernel.org,linux-media@vger.kernel.org,devicet...@vger.kernel.org Message-ID: On April 19, 2016 5:58:11 PM CEST, Crestez Dan Leonard wrote: > On 04/03/2016 11:52 AM, Peter Rosin wrote: > > From: Peter Rosin > > > > Allocate an explicit i2c mux core to handle parent and child > adapters > > etc. Update the select/deselect ops to be in terms of the i2c mux > core > > instead of the child adapter. > > > > --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_acpi.c > > +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_acpi.c > > @@ -136,16 +133,15 @@ static int inv_mpu_probe(struct i2c_client > *client, > > return result; > > > > st = iio_priv(dev_get_drvdata(&client->dev)); > > - st->mux_adapter = i2c_add_mux_adapter(client->adapter, > > - &client->dev, > > - client, > > - 0, 0, 0, > > - inv_mpu6050_select_bypass, > > - inv_mpu6050_deselect_bypass); > > - if (!st->mux_adapter) { > > - result = -ENODEV; > > + st->muxc = i2c_mux_one_adapter(client->adapter, &client->dev, 0, > 0, > > + 0, 0, 0, > > + inv_mpu6050_select_bypass, > > + inv_mpu6050_deselect_bypass); > > + if (IS_ERR(st->muxc)) { > > + result = PTR_ERR(st->muxc); > > goto out_unreg_device; > > } > > + st->muxc->priv = client; > > I just tested this patch on mpu9150 (combo mpu6050+ak8975) and I got a > crash on probe which looks sort of like this: > > [5.565374] RIP: 0010:[] [] > mutex_lock+0xd/0x30 > [5.565374] Call Trace: > [5.565374] [] > inv_mpu6050_select_bypass+0x1c/0xa0 > ... > [5.565374] [] i2c_transfer+0x51/0x90 > ... > [5.565374] [] > i2c_smbus_read_i2c_block_data+0x45/0xd0 > [5.565374] [] ak8975_probe+0x14a/0x5c0 > ... > [5.565374] [] i2c_new_device+0x178/0x1e0 > [5.565374] [] of_i2c_register_device+0xdd/0x1a0 > [5.565374] [] of_i2c_register_devices+0x3b/0x60 > [5.565374] [] i2c_register_adapter+0x178/0x410 > [5.565374] [] i2c_add_adapter+0x73/0x90 > [5.565374] [] i2c_mux_add_adapter+0x2ed/0x400 > [5.565374] [] i2c_mux_one_adapter+0x41/0x70 > [5.565374] [] inv_mpu_probe+0xba/0x1a0 > > This happens because muxc->priv is not initialized when probing > devices > behind the mux as described by devicetree. This can be easily fixed by > using muxc->dev instead of muxc->priv, or perhaps by calling > i2c_mux_alloc, initializing priv and later doing i2c_mux_add_adapter. > > These fixes are driver-specific and other drivers might experience > similar issues. Perhaps i2c_mux_one_adapter should take void *priv > just > like old i2c_add_mux_adapter? > > After I fix this locally the deadlock when reading from both devices > no > longer happens. > > -- > Regards, > Leonard Duh. Obvious now that you point it out. Thanks for catching this! I will just remove the i2c_mux_one_adapter interface for v7 and have the affected drivers do i2c_mux_alloc as a separate step (which was one of your suggested paths forward...) Thanks again, and sorry for the inconvenience, Peter (Written on my phone, sorry for crappy formatting) -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v6 08/24] iio: imu: inv_mpu6050: convert to use an explicit i2c mux core
On 04/03/2016 11:52 AM, Peter Rosin wrote: > From: Peter Rosin > > Allocate an explicit i2c mux core to handle parent and child adapters > etc. Update the select/deselect ops to be in terms of the i2c mux core > instead of the child adapter. > > --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_acpi.c > +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_acpi.c > @@ -136,16 +133,15 @@ static int inv_mpu_probe(struct i2c_client *client, > return result; > > st = iio_priv(dev_get_drvdata(&client->dev)); > - st->mux_adapter = i2c_add_mux_adapter(client->adapter, > - &client->dev, > - client, > - 0, 0, 0, > - inv_mpu6050_select_bypass, > - inv_mpu6050_deselect_bypass); > - if (!st->mux_adapter) { > - result = -ENODEV; > + st->muxc = i2c_mux_one_adapter(client->adapter, &client->dev, 0, 0, > +0, 0, 0, > +inv_mpu6050_select_bypass, > +inv_mpu6050_deselect_bypass); > + if (IS_ERR(st->muxc)) { > + result = PTR_ERR(st->muxc); > goto out_unreg_device; > } > + st->muxc->priv = client; I just tested this patch on mpu9150 (combo mpu6050+ak8975) and I got a crash on probe which looks sort of like this: [5.565374] RIP: 0010:[] [] mutex_lock+0xd/0x30 [5.565374] Call Trace: [5.565374] [] inv_mpu6050_select_bypass+0x1c/0xa0 ... [5.565374] [] i2c_transfer+0x51/0x90 ... [5.565374] [] i2c_smbus_read_i2c_block_data+0x45/0xd0 [5.565374] [] ak8975_probe+0x14a/0x5c0 ... [5.565374] [] i2c_new_device+0x178/0x1e0 [5.565374] [] of_i2c_register_device+0xdd/0x1a0 [5.565374] [] of_i2c_register_devices+0x3b/0x60 [5.565374] [] i2c_register_adapter+0x178/0x410 [5.565374] [] i2c_add_adapter+0x73/0x90 [5.565374] [] i2c_mux_add_adapter+0x2ed/0x400 [5.565374] [] i2c_mux_one_adapter+0x41/0x70 [5.565374] [] inv_mpu_probe+0xba/0x1a0 This happens because muxc->priv is not initialized when probing devices behind the mux as described by devicetree. This can be easily fixed by using muxc->dev instead of muxc->priv, or perhaps by calling i2c_mux_alloc, initializing priv and later doing i2c_mux_add_adapter. These fixes are driver-specific and other drivers might experience similar issues. Perhaps i2c_mux_one_adapter should take void *priv just like old i2c_add_mux_adapter? After I fix this locally the deadlock when reading from both devices no longer happens. -- Regards, Leonard -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v6 08/24] iio: imu: inv_mpu6050: convert to use an explicit i2c mux core
On 03/04/16 12:51, Peter Rosin wrote: > On 2016-04-03 12:51, Jonathan Cameron wrote: >> On 03/04/16 09:52, Peter Rosin wrote: >>> From: Peter Rosin >>> >>> Allocate an explicit i2c mux core to handle parent and child adapters >>> etc. Update the select/deselect ops to be in terms of the i2c mux core >>> instead of the child adapter. >>> >>> Signed-off-by: Peter Rosin >> I'm mostly fine with this (though one unrelated change seems to have snuck >> in). However, I'm not set up to test it - hence other than fixing the change >> you can have my ack, but ideal would be a tested by from someone with >> relevant hardware... However, it looks to be a fairly mechanical change so >> if no one is currently setup to test it, then don't let it hold up the >> series too long! >> >> Acked-by: Jonathan Cameron > > Thanks for your acks! > >> Jonathan >>> --- >>> drivers/iio/imu/inv_mpu6050/inv_mpu_acpi.c | 2 +- >>> drivers/iio/imu/inv_mpu6050/inv_mpu_core.c | 1 - >>> drivers/iio/imu/inv_mpu6050/inv_mpu_i2c.c | 32 >>> +- >>> drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h | 3 ++- >>> 4 files changed, 17 insertions(+), 21 deletions(-) >>> >>> diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_acpi.c >>> b/drivers/iio/imu/inv_mpu6050/inv_mpu_acpi.c >>> index 2771106fd650..f62b8bd9ad7e 100644 >>> --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_acpi.c >>> +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_acpi.c >>> @@ -183,7 +183,7 @@ int inv_mpu_acpi_create_mux_client(struct i2c_client >>> *client) >>> } else >>> return 0; /* no secondary addr, which is OK */ >>> } >>> - st->mux_client = i2c_new_device(st->mux_adapter, &info); >>> + st->mux_client = i2c_new_device(st->muxc->adapter[0], &info); >>> if (!st->mux_client) >>> return -ENODEV; >>> } >>> diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c >>> b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c >>> index d192953e9a38..0c2bded2b5b7 100644 >>> --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c >>> +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c >>> @@ -23,7 +23,6 @@ >>> #include >>> #include >>> #include >>> -#include >>> #include >>> #include "inv_mpu_iio.h" >>> >>> diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_i2c.c >>> b/drivers/iio/imu/inv_mpu6050/inv_mpu_i2c.c >>> index f581256d9d4c..0d429d788106 100644 >>> --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_i2c.c >>> +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_i2c.c >>> @@ -15,7 +15,6 @@ >>> #include >>> #include >>> #include >>> -#include >>> #include >>> #include >>> #include "inv_mpu_iio.h" >>> @@ -52,10 +51,9 @@ static int inv_mpu6050_write_reg_unlocked(struct >>> i2c_client *client, >>> return 0; >>> } >>> >>> -static int inv_mpu6050_select_bypass(struct i2c_adapter *adap, void >>> *mux_priv, >>> -u32 chan_id) >>> +static int inv_mpu6050_select_bypass(struct i2c_mux_core *muxc, u32 >>> chan_id) >>> { >>> - struct i2c_client *client = mux_priv; >>> + struct i2c_client *client = i2c_mux_priv(muxc); >>> struct iio_dev *indio_dev = dev_get_drvdata(&client->dev); > > Here, the existing code uses drv_get_drvdata to get from i2c_client to > iio_dev... > >>> struct inv_mpu6050_state *st = iio_priv(indio_dev); >>> int ret = 0; >>> @@ -84,10 +82,9 @@ write_error: >>> return ret; >>> } >>> >>> -static int inv_mpu6050_deselect_bypass(struct i2c_adapter *adap, >>> - void *mux_priv, u32 chan_id) >>> +static int inv_mpu6050_deselect_bypass(struct i2c_mux_core *muxc, u32 >>> chan_id) >>> { >>> - struct i2c_client *client = mux_priv; >>> + struct i2c_client *client = i2c_mux_priv(muxc); >>> struct iio_dev *indio_dev = dev_get_drvdata(&client->dev); > > ...and here too... > >>> struct inv_mpu6050_state *st = iio_priv(indio_dev); >>> >>> @@ -136,16 +133,15 @@ static int inv_mpu_probe(struct i2c_client *client, >>> return result; >>> >>> st = iio_priv(dev_get_drvdata(&client->dev)); >>> - st->mux_adapter = i2c_add_mux_adapter(client->adapter, >>> - &client->dev, >>> - client, >>> - 0, 0, 0, >>> - inv_mpu6050_select_bypass, >>> - inv_mpu6050_deselect_bypass); >>> - if (!st->mux_adapter) { >>> - result = -ENODEV; >>> + st->muxc = i2c_mux_one_adapter(client->adapter, &client->dev, 0, 0, >>> + 0, 0, 0, >>> + inv_mpu6050_select_bypass, >>> + inv_mpu6050_deselect_bypass); >>> + if (IS_ERR(st->muxc)) { >>> + result = PTR_ERR(st->muxc); >>> goto out_unreg_device; >>> } >>> + st->muxc->priv = client; >>> >>>
Re: [PATCH v6 08/24] iio: imu: inv_mpu6050: convert to use an explicit i2c mux core
On 2016-04-03 12:51, Jonathan Cameron wrote: > On 03/04/16 09:52, Peter Rosin wrote: >> From: Peter Rosin >> >> Allocate an explicit i2c mux core to handle parent and child adapters >> etc. Update the select/deselect ops to be in terms of the i2c mux core >> instead of the child adapter. >> >> Signed-off-by: Peter Rosin > I'm mostly fine with this (though one unrelated change seems to have snuck > in). However, I'm not set up to test it - hence other than fixing the change > you can have my ack, but ideal would be a tested by from someone with > relevant hardware... However, it looks to be a fairly mechanical change so > if no one is currently setup to test it, then don't let it hold up the > series too long! > > Acked-by: Jonathan Cameron Thanks for your acks! > Jonathan >> --- >> drivers/iio/imu/inv_mpu6050/inv_mpu_acpi.c | 2 +- >> drivers/iio/imu/inv_mpu6050/inv_mpu_core.c | 1 - >> drivers/iio/imu/inv_mpu6050/inv_mpu_i2c.c | 32 >> +- >> drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h | 3 ++- >> 4 files changed, 17 insertions(+), 21 deletions(-) >> >> diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_acpi.c >> b/drivers/iio/imu/inv_mpu6050/inv_mpu_acpi.c >> index 2771106fd650..f62b8bd9ad7e 100644 >> --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_acpi.c >> +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_acpi.c >> @@ -183,7 +183,7 @@ int inv_mpu_acpi_create_mux_client(struct i2c_client >> *client) >> } else >> return 0; /* no secondary addr, which is OK */ >> } >> -st->mux_client = i2c_new_device(st->mux_adapter, &info); >> +st->mux_client = i2c_new_device(st->muxc->adapter[0], &info); >> if (!st->mux_client) >> return -ENODEV; >> } >> diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c >> b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c >> index d192953e9a38..0c2bded2b5b7 100644 >> --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c >> +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c >> @@ -23,7 +23,6 @@ >> #include >> #include >> #include >> -#include >> #include >> #include "inv_mpu_iio.h" >> >> diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_i2c.c >> b/drivers/iio/imu/inv_mpu6050/inv_mpu_i2c.c >> index f581256d9d4c..0d429d788106 100644 >> --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_i2c.c >> +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_i2c.c >> @@ -15,7 +15,6 @@ >> #include >> #include >> #include >> -#include >> #include >> #include >> #include "inv_mpu_iio.h" >> @@ -52,10 +51,9 @@ static int inv_mpu6050_write_reg_unlocked(struct >> i2c_client *client, >> return 0; >> } >> >> -static int inv_mpu6050_select_bypass(struct i2c_adapter *adap, void >> *mux_priv, >> - u32 chan_id) >> +static int inv_mpu6050_select_bypass(struct i2c_mux_core *muxc, u32 chan_id) >> { >> -struct i2c_client *client = mux_priv; >> +struct i2c_client *client = i2c_mux_priv(muxc); >> struct iio_dev *indio_dev = dev_get_drvdata(&client->dev); Here, the existing code uses drv_get_drvdata to get from i2c_client to iio_dev... >> struct inv_mpu6050_state *st = iio_priv(indio_dev); >> int ret = 0; >> @@ -84,10 +82,9 @@ write_error: >> return ret; >> } >> >> -static int inv_mpu6050_deselect_bypass(struct i2c_adapter *adap, >> - void *mux_priv, u32 chan_id) >> +static int inv_mpu6050_deselect_bypass(struct i2c_mux_core *muxc, u32 >> chan_id) >> { >> -struct i2c_client *client = mux_priv; >> +struct i2c_client *client = i2c_mux_priv(muxc); >> struct iio_dev *indio_dev = dev_get_drvdata(&client->dev); ...and here too... >> struct inv_mpu6050_state *st = iio_priv(indio_dev); >> >> @@ -136,16 +133,15 @@ static int inv_mpu_probe(struct i2c_client *client, >> return result; >> >> st = iio_priv(dev_get_drvdata(&client->dev)); >> -st->mux_adapter = i2c_add_mux_adapter(client->adapter, >> - &client->dev, >> - client, >> - 0, 0, 0, >> - inv_mpu6050_select_bypass, >> - inv_mpu6050_deselect_bypass); >> -if (!st->mux_adapter) { >> -result = -ENODEV; >> +st->muxc = i2c_mux_one_adapter(client->adapter, &client->dev, 0, 0, >> + 0, 0, 0, >> + inv_mpu6050_select_bypass, >> + inv_mpu6050_deselect_bypass); >> +if (IS_ERR(st->muxc)) { >> +result = PTR_ERR(st->muxc); >> goto out_unreg_device; >> } >> +st->muxc->priv = client; >> >> result = inv_mpu_acpi_create_mux_client(client); >> if (result) >> @@ -154,7 +150,7 @@ static int inv_mpu_probe(struct i2c_client *cl
Re: [PATCH v6 08/24] iio: imu: inv_mpu6050: convert to use an explicit i2c mux core
On 03/04/16 09:52, Peter Rosin wrote: > From: Peter Rosin > > Allocate an explicit i2c mux core to handle parent and child adapters > etc. Update the select/deselect ops to be in terms of the i2c mux core > instead of the child adapter. > > Signed-off-by: Peter Rosin I'm mostly fine with this (though one unrelated change seems to have snuck in). However, I'm not set up to test it - hence other than fixing the change you can have my ack, but ideal would be a tested by from someone with relevant hardware... However, it looks to be a fairly mechanical change so if no one is currently setup to test it, then don't let it hold up the series too long! Acked-by: Jonathan Cameron Jonathan > --- > drivers/iio/imu/inv_mpu6050/inv_mpu_acpi.c | 2 +- > drivers/iio/imu/inv_mpu6050/inv_mpu_core.c | 1 - > drivers/iio/imu/inv_mpu6050/inv_mpu_i2c.c | 32 > +- > drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h | 3 ++- > 4 files changed, 17 insertions(+), 21 deletions(-) > > diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_acpi.c > b/drivers/iio/imu/inv_mpu6050/inv_mpu_acpi.c > index 2771106fd650..f62b8bd9ad7e 100644 > --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_acpi.c > +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_acpi.c > @@ -183,7 +183,7 @@ int inv_mpu_acpi_create_mux_client(struct i2c_client > *client) > } else > return 0; /* no secondary addr, which is OK */ > } > - st->mux_client = i2c_new_device(st->mux_adapter, &info); > + st->mux_client = i2c_new_device(st->muxc->adapter[0], &info); > if (!st->mux_client) > return -ENODEV; > } > diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c > b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c > index d192953e9a38..0c2bded2b5b7 100644 > --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c > +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c > @@ -23,7 +23,6 @@ > #include > #include > #include > -#include > #include > #include "inv_mpu_iio.h" > > diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_i2c.c > b/drivers/iio/imu/inv_mpu6050/inv_mpu_i2c.c > index f581256d9d4c..0d429d788106 100644 > --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_i2c.c > +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_i2c.c > @@ -15,7 +15,6 @@ > #include > #include > #include > -#include > #include > #include > #include "inv_mpu_iio.h" > @@ -52,10 +51,9 @@ static int inv_mpu6050_write_reg_unlocked(struct > i2c_client *client, > return 0; > } > > -static int inv_mpu6050_select_bypass(struct i2c_adapter *adap, void > *mux_priv, > - u32 chan_id) > +static int inv_mpu6050_select_bypass(struct i2c_mux_core *muxc, u32 chan_id) > { > - struct i2c_client *client = mux_priv; > + struct i2c_client *client = i2c_mux_priv(muxc); > struct iio_dev *indio_dev = dev_get_drvdata(&client->dev); > struct inv_mpu6050_state *st = iio_priv(indio_dev); > int ret = 0; > @@ -84,10 +82,9 @@ write_error: > return ret; > } > > -static int inv_mpu6050_deselect_bypass(struct i2c_adapter *adap, > -void *mux_priv, u32 chan_id) > +static int inv_mpu6050_deselect_bypass(struct i2c_mux_core *muxc, u32 > chan_id) > { > - struct i2c_client *client = mux_priv; > + struct i2c_client *client = i2c_mux_priv(muxc); > struct iio_dev *indio_dev = dev_get_drvdata(&client->dev); > struct inv_mpu6050_state *st = iio_priv(indio_dev); > > @@ -136,16 +133,15 @@ static int inv_mpu_probe(struct i2c_client *client, > return result; > > st = iio_priv(dev_get_drvdata(&client->dev)); > - st->mux_adapter = i2c_add_mux_adapter(client->adapter, > - &client->dev, > - client, > - 0, 0, 0, > - inv_mpu6050_select_bypass, > - inv_mpu6050_deselect_bypass); > - if (!st->mux_adapter) { > - result = -ENODEV; > + st->muxc = i2c_mux_one_adapter(client->adapter, &client->dev, 0, 0, > +0, 0, 0, > +inv_mpu6050_select_bypass, > +inv_mpu6050_deselect_bypass); > + if (IS_ERR(st->muxc)) { > + result = PTR_ERR(st->muxc); > goto out_unreg_device; > } > + st->muxc->priv = client; > > result = inv_mpu_acpi_create_mux_client(client); > if (result) > @@ -154,7 +150,7 @@ static int inv_mpu_probe(struct i2c_client *client, > return 0; > > out_del_mux: > - i2c_del_mux_adapter(st->mux_adapter); > + i2c_mux_del_adapters(st->muxc); > out_unreg_device: > inv_mpu_core_remove(&client->dev); > return result; > @@ -162,11 +158,11 @@ out_unreg_device: