All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/2] Fix kernel panics with certain I2C tps6* chips
@ 2013-06-17 17:47 ` Tuomas Tynkkynen
  0 siblings, 0 replies; 19+ messages in thread
From: Tuomas Tynkkynen @ 2013-06-17 17:47 UTC (permalink / raw)
  To: Samuel Ortiz, Mark Brown
  Cc: linux-tegra, linux-kernel, linux-i2c, Linus Walleij, Tuomas Tynkkynen

Hi,

Latest linux-next head (next-20130617) seems to have some backwards-incompatible
changes to the i2c core, which breaks the tps* drivers in our boards and cause
panics on boot.

ttynkkynen@ttynkkynen-lnx:~/upstream/kernel$ git bisect bad
c80f52847c50109ca248c22efbf71ff10553dca4 is the first bad commit
commit c80f52847c50109ca248c22efbf71ff10553dca4
Author: Linus Walleij <linus.walleij@linaro.org>
Date:   Mon May 13 22:18:21 2013 +0200

    i2c: core: make it possible to match a pure device tree driver
    
    This tries to address an issue found when writing an MFD driver
    for the Nomadik STw481x PMICs: as the platform is using device
    tree exclusively I want to specify the driver matching like
    this:
    
    static const struct of_device_id stw481x_match[] = {
    	{ .compatible = "st,stw4810", },
    	{ .compatible = "st,stw4811", },
    	{},
    };
    
    static struct i2c_driver stw481x_driver = {
    	.driver = {
    		.name	= "stw481x",
    		.of_match_table = stw481x_match,
    	},
    	.probe		= stw481x_probe,
    	.remove		= stw481x_remove,
    };
    
    However that turns out not to be possible: the I2C probe code
    is written so that the probe() call is always passed a match
    from i2c_match_id() using non-devicetree matches.
    
    This is probably why most devices using device tree for I2C
    clients currently will pass no .of_match_table *at all* but
    instead just use .id_table from struct i2c_driver to match
    the device. As you realize that means that the whole idea with
    compatible strings is discarded, and that is why we find strange
    device tree I2C device compatible strings like "product" instead
    of "vendor,product" as you could expect.
    
    Let's figure out how to fix this before the mess spreads. This
    patch will allow probeing devices with only an of_match_table
    as per above, and will pass NULL as the second argument to the
    probe() function. If the driver wants to deduce secondary info
    from the struct of_device_id .data field, it has to call
    of_match_device() on its own match table in the probe function
    device tree probe path.
    
    If drivers define both an .of_match_table *AND* a i2c_driver
    .id_table, the .of_match_table will take precedence, just
    as is done in the i2c_device_match() function in i2c-core.c.
    
    I2C devices probed from device tree should subsequently be
    fixed to handle the case where of_match_table() is
    used (I think none of them do that today), and platforms should
    fix their device trees to use compatible strings for I2C devices
    instead of setting the name to Linux device driver names as is
    done in multiple cases today.
    
    Cc: Rob Herring <rob.herring@calxeda.com>
    Cc: Grant Likely <grant.likely@linaro.org>
    Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
    Signed-off-by: Wolfram Sang <wsa@the-dreams.de>

Tuomas Tynkkynen (2):
  mfd: tps65910: Fix crash in i2c_driver .probe
  regulator: tps62360: Fix crash in i2c_driver .probe

 drivers/mfd/tps65910.c                 | 6 ++++--
 drivers/regulator/tps62360-regulator.c | 8 ++++++--
 2 files changed, 10 insertions(+), 4 deletions(-)

-- 
1.8.1.5

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

* [PATCH 0/2] Fix kernel panics with certain I2C tps6* chips
@ 2013-06-17 17:47 ` Tuomas Tynkkynen
  0 siblings, 0 replies; 19+ messages in thread
From: Tuomas Tynkkynen @ 2013-06-17 17:47 UTC (permalink / raw)
  To: Samuel Ortiz, Mark Brown
  Cc: linux-tegra, linux-kernel, linux-i2c, Linus Walleij, Tuomas Tynkkynen

Hi,

Latest linux-next head (next-20130617) seems to have some backwards-incompatible
changes to the i2c core, which breaks the tps* drivers in our boards and cause
panics on boot.

ttynkkynen@ttynkkynen-lnx:~/upstream/kernel$ git bisect bad
c80f52847c50109ca248c22efbf71ff10553dca4 is the first bad commit
commit c80f52847c50109ca248c22efbf71ff10553dca4
Author: Linus Walleij <linus.walleij@linaro.org>
Date:   Mon May 13 22:18:21 2013 +0200

    i2c: core: make it possible to match a pure device tree driver
    
    This tries to address an issue found when writing an MFD driver
    for the Nomadik STw481x PMICs: as the platform is using device
    tree exclusively I want to specify the driver matching like
    this:
    
    static const struct of_device_id stw481x_match[] = {
    	{ .compatible = "st,stw4810", },
    	{ .compatible = "st,stw4811", },
    	{},
    };
    
    static struct i2c_driver stw481x_driver = {
    	.driver = {
    		.name	= "stw481x",
    		.of_match_table = stw481x_match,
    	},
    	.probe		= stw481x_probe,
    	.remove		= stw481x_remove,
    };
    
    However that turns out not to be possible: the I2C probe code
    is written so that the probe() call is always passed a match
    from i2c_match_id() using non-devicetree matches.
    
    This is probably why most devices using device tree for I2C
    clients currently will pass no .of_match_table *at all* but
    instead just use .id_table from struct i2c_driver to match
    the device. As you realize that means that the whole idea with
    compatible strings is discarded, and that is why we find strange
    device tree I2C device compatible strings like "product" instead
    of "vendor,product" as you could expect.
    
    Let's figure out how to fix this before the mess spreads. This
    patch will allow probeing devices with only an of_match_table
    as per above, and will pass NULL as the second argument to the
    probe() function. If the driver wants to deduce secondary info
    from the struct of_device_id .data field, it has to call
    of_match_device() on its own match table in the probe function
    device tree probe path.
    
    If drivers define both an .of_match_table *AND* a i2c_driver
    .id_table, the .of_match_table will take precedence, just
    as is done in the i2c_device_match() function in i2c-core.c.
    
    I2C devices probed from device tree should subsequently be
    fixed to handle the case where of_match_table() is
    used (I think none of them do that today), and platforms should
    fix their device trees to use compatible strings for I2C devices
    instead of setting the name to Linux device driver names as is
    done in multiple cases today.
    
    Cc: Rob Herring <rob.herring@calxeda.com>
    Cc: Grant Likely <grant.likely@linaro.org>
    Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
    Signed-off-by: Wolfram Sang <wsa@the-dreams.de>

Tuomas Tynkkynen (2):
  mfd: tps65910: Fix crash in i2c_driver .probe
  regulator: tps62360: Fix crash in i2c_driver .probe

 drivers/mfd/tps65910.c                 | 6 ++++--
 drivers/regulator/tps62360-regulator.c | 8 ++++++--
 2 files changed, 10 insertions(+), 4 deletions(-)

-- 
1.8.1.5


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

* [PATCH 1/2] mfd: tps65910: Fix crash in i2c_driver .probe
  2013-06-17 17:47 ` Tuomas Tynkkynen
@ 2013-06-17 17:47     ` Tuomas Tynkkynen
  -1 siblings, 0 replies; 19+ messages in thread
From: Tuomas Tynkkynen @ 2013-06-17 17:47 UTC (permalink / raw)
  To: Samuel Ortiz, Mark Brown
  Cc: linux-tegra-u79uwXL29TY76Z2rM5mHXA,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA,
	linux-i2c-u79uwXL29TY76Z2rM5mHXA, Linus Walleij,
	Tuomas Tynkkynen

Commit "i2c: core: make it possible to match a pure device tree driver"
changed semantics of the i2c probing for device tree devices.
Device tree probed devices now get a NULL i2c_device_id pointer.
This caused kernel panics due to NULL dereference.

Signed-off-by: Tuomas Tynkkynen <ttynkkynen-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
 drivers/mfd/tps65910.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/mfd/tps65910.c b/drivers/mfd/tps65910.c
index d792772..a62c30d 100644
--- a/drivers/mfd/tps65910.c
+++ b/drivers/mfd/tps65910.c
@@ -461,16 +461,18 @@ static int tps65910_i2c_probe(struct i2c_client *i2c,
 	struct tps65910_board *of_pmic_plat_data = NULL;
 	struct tps65910_platform_data *init_data;
 	int ret = 0;
-	int chip_id = id->driver_data;
+	int chip_id = -1;
 
 	pmic_plat_data = dev_get_platdata(&i2c->dev);
 
-	if (!pmic_plat_data && i2c->dev.of_node) {
+	if (id) {
+		chip_id = id->driver_data;
+	} else if (i2c->dev.of_node) {
 		pmic_plat_data = tps65910_parse_dt(i2c, &chip_id);
 		of_pmic_plat_data = pmic_plat_data;
 	}
 
-	if (!pmic_plat_data)
+	if (!pmic_plat_data || chip_id < 0)
 		return -EINVAL;
 
 	init_data = devm_kzalloc(&i2c->dev, sizeof(*init_data), GFP_KERNEL);
-- 
1.8.1.5

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

* [PATCH 1/2] mfd: tps65910: Fix crash in i2c_driver .probe
@ 2013-06-17 17:47     ` Tuomas Tynkkynen
  0 siblings, 0 replies; 19+ messages in thread
From: Tuomas Tynkkynen @ 2013-06-17 17:47 UTC (permalink / raw)
  To: Samuel Ortiz, Mark Brown
  Cc: linux-tegra, linux-kernel, linux-i2c, Linus Walleij, Tuomas Tynkkynen

Commit "i2c: core: make it possible to match a pure device tree driver"
changed semantics of the i2c probing for device tree devices.
Device tree probed devices now get a NULL i2c_device_id pointer.
This caused kernel panics due to NULL dereference.

Signed-off-by: Tuomas Tynkkynen <ttynkkynen@nvidia.com>
---
 drivers/mfd/tps65910.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/mfd/tps65910.c b/drivers/mfd/tps65910.c
index d792772..a62c30d 100644
--- a/drivers/mfd/tps65910.c
+++ b/drivers/mfd/tps65910.c
@@ -461,16 +461,18 @@ static int tps65910_i2c_probe(struct i2c_client *i2c,
 	struct tps65910_board *of_pmic_plat_data = NULL;
 	struct tps65910_platform_data *init_data;
 	int ret = 0;
-	int chip_id = id->driver_data;
+	int chip_id = -1;
 
 	pmic_plat_data = dev_get_platdata(&i2c->dev);
 
-	if (!pmic_plat_data && i2c->dev.of_node) {
+	if (id) {
+		chip_id = id->driver_data;
+	} else if (i2c->dev.of_node) {
 		pmic_plat_data = tps65910_parse_dt(i2c, &chip_id);
 		of_pmic_plat_data = pmic_plat_data;
 	}
 
-	if (!pmic_plat_data)
+	if (!pmic_plat_data || chip_id < 0)
 		return -EINVAL;
 
 	init_data = devm_kzalloc(&i2c->dev, sizeof(*init_data), GFP_KERNEL);
-- 
1.8.1.5


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

* [PATCH 2/2] regulator: tps62360: Fix crash in i2c_driver .probe
  2013-06-17 17:47 ` Tuomas Tynkkynen
@ 2013-06-17 17:47     ` Tuomas Tynkkynen
  -1 siblings, 0 replies; 19+ messages in thread
From: Tuomas Tynkkynen @ 2013-06-17 17:47 UTC (permalink / raw)
  To: Samuel Ortiz, Mark Brown
  Cc: linux-tegra-u79uwXL29TY76Z2rM5mHXA,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA,
	linux-i2c-u79uwXL29TY76Z2rM5mHXA, Linus Walleij,
	Tuomas Tynkkynen

Commit "i2c: core: make it possible to match a pure device tree driver"
changed semantics of the i2c probing for device tree devices.
Device tree probed devices now get a NULL i2c_device_id pointer.
This caused kernel panics due to NULL dereference.

Signed-off-by: Tuomas Tynkkynen <ttynkkynen-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
 drivers/regulator/tps62360-regulator.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/regulator/tps62360-regulator.c b/drivers/regulator/tps62360-regulator.c
index 612919c..a490d5b 100644
--- a/drivers/regulator/tps62360-regulator.c
+++ b/drivers/regulator/tps62360-regulator.c
@@ -351,7 +351,6 @@ static int tps62360_probe(struct i2c_client *client,
 	int chip_id;
 
 	pdata = client->dev.platform_data;
-	chip_id = id->driver_data;
 
 	if (client->dev.of_node) {
 		const struct of_device_id *match;
@@ -364,6 +363,11 @@ static int tps62360_probe(struct i2c_client *client,
 		chip_id = (int)match->data;
 		if (!pdata)
 			pdata = of_get_tps62360_platform_data(&client->dev);
+	} else if (id) {
+		chip_id = id->driver_data;
+	} else {
+		dev_err(&client->dev, "No device tree match or id table match found\n");
+		return -ENODEV;
 	}
 
 	if (!pdata) {
@@ -402,7 +406,7 @@ static int tps62360_probe(struct i2c_client *client,
 		return -ENODEV;
 	}
 
-	tps->desc.name = id->name;
+	tps->desc.name = client->name;
 	tps->desc.id = 0;
 	tps->desc.ops = &tps62360_dcdc_ops;
 	tps->desc.type = REGULATOR_VOLTAGE;
-- 
1.8.1.5

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

* [PATCH 2/2] regulator: tps62360: Fix crash in i2c_driver .probe
@ 2013-06-17 17:47     ` Tuomas Tynkkynen
  0 siblings, 0 replies; 19+ messages in thread
From: Tuomas Tynkkynen @ 2013-06-17 17:47 UTC (permalink / raw)
  To: Samuel Ortiz, Mark Brown
  Cc: linux-tegra, linux-kernel, linux-i2c, Linus Walleij, Tuomas Tynkkynen

Commit "i2c: core: make it possible to match a pure device tree driver"
changed semantics of the i2c probing for device tree devices.
Device tree probed devices now get a NULL i2c_device_id pointer.
This caused kernel panics due to NULL dereference.

Signed-off-by: Tuomas Tynkkynen <ttynkkynen@nvidia.com>
---
 drivers/regulator/tps62360-regulator.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/regulator/tps62360-regulator.c b/drivers/regulator/tps62360-regulator.c
index 612919c..a490d5b 100644
--- a/drivers/regulator/tps62360-regulator.c
+++ b/drivers/regulator/tps62360-regulator.c
@@ -351,7 +351,6 @@ static int tps62360_probe(struct i2c_client *client,
 	int chip_id;
 
 	pdata = client->dev.platform_data;
-	chip_id = id->driver_data;
 
 	if (client->dev.of_node) {
 		const struct of_device_id *match;
@@ -364,6 +363,11 @@ static int tps62360_probe(struct i2c_client *client,
 		chip_id = (int)match->data;
 		if (!pdata)
 			pdata = of_get_tps62360_platform_data(&client->dev);
+	} else if (id) {
+		chip_id = id->driver_data;
+	} else {
+		dev_err(&client->dev, "No device tree match or id table match found\n");
+		return -ENODEV;
 	}
 
 	if (!pdata) {
@@ -402,7 +406,7 @@ static int tps62360_probe(struct i2c_client *client,
 		return -ENODEV;
 	}
 
-	tps->desc.name = id->name;
+	tps->desc.name = client->name;
 	tps->desc.id = 0;
 	tps->desc.ops = &tps62360_dcdc_ops;
 	tps->desc.type = REGULATOR_VOLTAGE;
-- 
1.8.1.5


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

* Re: [PATCH 1/2] mfd: tps65910: Fix crash in i2c_driver .probe
  2013-06-17 17:47     ` Tuomas Tynkkynen
@ 2013-06-17 18:07         ` Stephen Warren
  -1 siblings, 0 replies; 19+ messages in thread
From: Stephen Warren @ 2013-06-17 18:07 UTC (permalink / raw)
  To: Tuomas Tynkkynen
  Cc: Samuel Ortiz, Mark Brown, linux-tegra-u79uwXL29TY76Z2rM5mHXA,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA,
	linux-i2c-u79uwXL29TY76Z2rM5mHXA, Linus Walleij

On 06/17/2013 11:47 AM, Tuomas Tynkkynen wrote:
> Commit "i2c: core: make it possible to match a pure device tree driver"
> changed semantics of the i2c probing for device tree devices.
> Device tree probed devices now get a NULL i2c_device_id pointer.
> This caused kernel panics due to NULL dereference.

> diff --git a/drivers/mfd/tps65910.c b/drivers/mfd/tps65910.c

>  	pmic_plat_data = dev_get_platdata(&i2c->dev);
>  
> -	if (!pmic_plat_data && i2c->dev.of_node) {
> +	if (id) {
> +		chip_id = id->driver_data;
> +	} else if (i2c->dev.of_node) {
>  		pmic_plat_data = tps65910_parse_dt(i2c, &chip_id);

That over-writes pmic_plat_data even if it was already set above. This
should really only happen if the earlier assignment didn't find any
pdata, i.e. if (!pmic_plat_data) here.

Looking at patch 2/2, the structure in that driver is correct, and
perhaps could be implemented the same or similarly here?

>  		of_pmic_plat_data = pmic_plat_data;

Or just swap those assignments:

of_pmic_plat_data = tps65910_parse_dt(...);
if (!pmic_plat_data)
    pmic_plat_data = of_pmic_plat_data;

(although there's perhaps little point parsing the pdata from DT if it's
already provided through the device object)

>  	}
>  
> -	if (!pmic_plat_data)
> +	if (!pmic_plat_data || chip_id < 0)
>  		return -EINVAL;

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

* Re: [PATCH 1/2] mfd: tps65910: Fix crash in i2c_driver .probe
@ 2013-06-17 18:07         ` Stephen Warren
  0 siblings, 0 replies; 19+ messages in thread
From: Stephen Warren @ 2013-06-17 18:07 UTC (permalink / raw)
  To: Tuomas Tynkkynen
  Cc: Samuel Ortiz, Mark Brown, linux-tegra, linux-kernel, linux-i2c,
	Linus Walleij

On 06/17/2013 11:47 AM, Tuomas Tynkkynen wrote:
> Commit "i2c: core: make it possible to match a pure device tree driver"
> changed semantics of the i2c probing for device tree devices.
> Device tree probed devices now get a NULL i2c_device_id pointer.
> This caused kernel panics due to NULL dereference.

> diff --git a/drivers/mfd/tps65910.c b/drivers/mfd/tps65910.c

>  	pmic_plat_data = dev_get_platdata(&i2c->dev);
>  
> -	if (!pmic_plat_data && i2c->dev.of_node) {
> +	if (id) {
> +		chip_id = id->driver_data;
> +	} else if (i2c->dev.of_node) {
>  		pmic_plat_data = tps65910_parse_dt(i2c, &chip_id);

That over-writes pmic_plat_data even if it was already set above. This
should really only happen if the earlier assignment didn't find any
pdata, i.e. if (!pmic_plat_data) here.

Looking at patch 2/2, the structure in that driver is correct, and
perhaps could be implemented the same or similarly here?

>  		of_pmic_plat_data = pmic_plat_data;

Or just swap those assignments:

of_pmic_plat_data = tps65910_parse_dt(...);
if (!pmic_plat_data)
    pmic_plat_data = of_pmic_plat_data;

(although there's perhaps little point parsing the pdata from DT if it's
already provided through the device object)

>  	}
>  
> -	if (!pmic_plat_data)
> +	if (!pmic_plat_data || chip_id < 0)
>  		return -EINVAL;


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

* Re: [PATCH 2/2] regulator: tps62360: Fix crash in i2c_driver .probe
  2013-06-17 17:47     ` Tuomas Tynkkynen
  (?)
@ 2013-06-17 18:16     ` Stephen Warren
  -1 siblings, 0 replies; 19+ messages in thread
From: Stephen Warren @ 2013-06-17 18:16 UTC (permalink / raw)
  To: Tuomas Tynkkynen
  Cc: Samuel Ortiz, Mark Brown, linux-tegra, linux-kernel, linux-i2c,
	Linus Walleij

On 06/17/2013 11:47 AM, Tuomas Tynkkynen wrote:
> Commit "i2c: core: make it possible to match a pure device tree driver"
> changed semantics of the i2c probing for device tree devices.
> Device tree probed devices now get a NULL i2c_device_id pointer.
> This caused kernel panics due to NULL dereference.

Tested-by: Stephen Warren <swarren@nvidia.com>
Reviewed-by: Stephen Warren <swarren@nvidia.com>

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

* Re: [PATCH 1/2] mfd: tps65910: Fix crash in i2c_driver .probe
  2013-06-17 17:47     ` Tuomas Tynkkynen
@ 2013-06-17 18:16         ` Stephen Warren
  -1 siblings, 0 replies; 19+ messages in thread
From: Stephen Warren @ 2013-06-17 18:16 UTC (permalink / raw)
  To: Tuomas Tynkkynen
  Cc: Samuel Ortiz, Mark Brown, linux-tegra-u79uwXL29TY76Z2rM5mHXA,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA,
	linux-i2c-u79uwXL29TY76Z2rM5mHXA, Linus Walleij

On 06/17/2013 11:47 AM, Tuomas Tynkkynen wrote:
> Commit "i2c: core: make it possible to match a pure device tree driver"
> changed semantics of the i2c probing for device tree devices.
> Device tree probed devices now get a NULL i2c_device_id pointer.
> This caused kernel panics due to NULL dereference.

Tested-by: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>

(although I imagine I'd need to retest if there was a v2 to address my
previous comments)

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

* Re: [PATCH 1/2] mfd: tps65910: Fix crash in i2c_driver .probe
@ 2013-06-17 18:16         ` Stephen Warren
  0 siblings, 0 replies; 19+ messages in thread
From: Stephen Warren @ 2013-06-17 18:16 UTC (permalink / raw)
  To: Tuomas Tynkkynen
  Cc: Samuel Ortiz, Mark Brown, linux-tegra, linux-kernel, linux-i2c,
	Linus Walleij

On 06/17/2013 11:47 AM, Tuomas Tynkkynen wrote:
> Commit "i2c: core: make it possible to match a pure device tree driver"
> changed semantics of the i2c probing for device tree devices.
> Device tree probed devices now get a NULL i2c_device_id pointer.
> This caused kernel panics due to NULL dereference.

Tested-by: Stephen Warren <swarren@nvidia.com>

(although I imagine I'd need to retest if there was a v2 to address my
previous comments)

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

* Re: [PATCH 0/2] Fix kernel panics with certain I2C tps6* chips
  2013-06-17 17:47 ` Tuomas Tynkkynen
@ 2013-06-18  7:52     ` Linus Walleij
  -1 siblings, 0 replies; 19+ messages in thread
From: Linus Walleij @ 2013-06-18  7:52 UTC (permalink / raw)
  To: Tuomas Tynkkynen, Wolfram Sang
  Cc: Samuel Ortiz, Mark Brown, linux-tegra-u79uwXL29TY76Z2rM5mHXA,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA,
	linux-i2c-u79uwXL29TY76Z2rM5mHXA

On Mon, Jun 17, 2013 at 7:47 PM, Tuomas Tynkkynen <ttynkkynen-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> wrote:

> Hi,
>
> Latest linux-next head (next-20130617) seems to have some backwards-incompatible
> changes to the i2c core, which breaks the tps* drivers in our boards and cause
> panics on boot.

You should probably put Wolfram (the i2c maintainer) on the To: line.

> Tuomas Tynkkynen (2):
>   mfd: tps65910: Fix crash in i2c_driver .probe
>   regulator: tps62360: Fix crash in i2c_driver .probe

Nice :-)

Yours,
Linus Walleij

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

* Re: [PATCH 0/2] Fix kernel panics with certain I2C tps6* chips
@ 2013-06-18  7:52     ` Linus Walleij
  0 siblings, 0 replies; 19+ messages in thread
From: Linus Walleij @ 2013-06-18  7:52 UTC (permalink / raw)
  To: Tuomas Tynkkynen, Wolfram Sang
  Cc: Samuel Ortiz, Mark Brown, linux-tegra, linux-kernel, linux-i2c

On Mon, Jun 17, 2013 at 7:47 PM, Tuomas Tynkkynen <ttynkkynen@nvidia.com> wrote:

> Hi,
>
> Latest linux-next head (next-20130617) seems to have some backwards-incompatible
> changes to the i2c core, which breaks the tps* drivers in our boards and cause
> panics on boot.

You should probably put Wolfram (the i2c maintainer) on the To: line.

> Tuomas Tynkkynen (2):
>   mfd: tps65910: Fix crash in i2c_driver .probe
>   regulator: tps62360: Fix crash in i2c_driver .probe

Nice :-)

Yours,
Linus Walleij

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

* Re: [PATCH 1/2] mfd: tps65910: Fix crash in i2c_driver .probe
  2013-06-17 18:16         ` Stephen Warren
@ 2013-06-18  9:16             ` Samuel Ortiz
  -1 siblings, 0 replies; 19+ messages in thread
From: Samuel Ortiz @ 2013-06-18  9:16 UTC (permalink / raw)
  To: Stephen Warren
  Cc: Tuomas Tynkkynen, Mark Brown, linux-tegra-u79uwXL29TY76Z2rM5mHXA,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA,
	linux-i2c-u79uwXL29TY76Z2rM5mHXA, Linus Walleij

On Mon, Jun 17, 2013 at 12:16:33PM -0600, Stephen Warren wrote:
> On 06/17/2013 11:47 AM, Tuomas Tynkkynen wrote:
> > Commit "i2c: core: make it possible to match a pure device tree driver"
> > changed semantics of the i2c probing for device tree devices.
> > Device tree probed devices now get a NULL i2c_device_id pointer.
> > This caused kernel panics due to NULL dereference.
> 
> Tested-by: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> 
> (although I imagine I'd need to retest if there was a v2 to address my
> previous comments)
I would prefer seeing a v2, especially to address the pmic_plat_data
overwriting issue.

Cheers,
Samuel.

-- 
Intel Open Source Technology Centre
http://oss.intel.com/

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

* Re: [PATCH 1/2] mfd: tps65910: Fix crash in i2c_driver .probe
@ 2013-06-18  9:16             ` Samuel Ortiz
  0 siblings, 0 replies; 19+ messages in thread
From: Samuel Ortiz @ 2013-06-18  9:16 UTC (permalink / raw)
  To: Stephen Warren
  Cc: Tuomas Tynkkynen, Mark Brown, linux-tegra, linux-kernel,
	linux-i2c, Linus Walleij

On Mon, Jun 17, 2013 at 12:16:33PM -0600, Stephen Warren wrote:
> On 06/17/2013 11:47 AM, Tuomas Tynkkynen wrote:
> > Commit "i2c: core: make it possible to match a pure device tree driver"
> > changed semantics of the i2c probing for device tree devices.
> > Device tree probed devices now get a NULL i2c_device_id pointer.
> > This caused kernel panics due to NULL dereference.
> 
> Tested-by: Stephen Warren <swarren@nvidia.com>
> 
> (although I imagine I'd need to retest if there was a v2 to address my
> previous comments)
I would prefer seeing a v2, especially to address the pmic_plat_data
overwriting issue.

Cheers,
Samuel.

-- 
Intel Open Source Technology Centre
http://oss.intel.com/

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

* Re: [PATCH 1/2] mfd: tps65910: Fix crash in i2c_driver .probe
  2013-06-17 18:07         ` Stephen Warren
@ 2013-06-18  9:35             ` Tuomas Tynkkynen
  -1 siblings, 0 replies; 19+ messages in thread
From: Tuomas Tynkkynen @ 2013-06-18  9:35 UTC (permalink / raw)
  To: Stephen Warren
  Cc: Samuel Ortiz, Mark Brown, linux-tegra-u79uwXL29TY76Z2rM5mHXA,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA,
	linux-i2c-u79uwXL29TY76Z2rM5mHXA, Linus Walleij

On 06/17/2013 09:07 PM, Stephen Warren wrote:
> On 06/17/2013 11:47 AM, Tuomas Tynkkynen wrote:
>> Commit "i2c: core: make it possible to match a pure device tree driver"
>> changed semantics of the i2c probing for device tree devices.
>> Device tree probed devices now get a NULL i2c_device_id pointer.
>> This caused kernel panics due to NULL dereference.
> 
>> diff --git a/drivers/mfd/tps65910.c b/drivers/mfd/tps65910.c
> 
>>  	pmic_plat_data = dev_get_platdata(&i2c->dev);
>>  
>> -	if (!pmic_plat_data && i2c->dev.of_node) {
>> +	if (id) {
>> +		chip_id = id->driver_data;
>> +	} else if (i2c->dev.of_node) {
>>  		pmic_plat_data = tps65910_parse_dt(i2c, &chip_id);
> 
> That over-writes pmic_plat_data even if it was already set above. This
> should really only happen if the earlier assignment didn't find any
> pdata, i.e. if (!pmic_plat_data) here.

That would cause the probe() to fail since it doesn't have a chip_id.

> 
> Looking at patch 2/2, the structure in that driver is correct, and
> perhaps could be implemented the same or similarly here?
> 

This seems to be the best way, I'll change it that way.

>>  		of_pmic_plat_data = pmic_plat_data;
> 
> Or just swap those assignments:
> 
> of_pmic_plat_data = tps65910_parse_dt(...);
> if (!pmic_plat_data)
>     pmic_plat_data = of_pmic_plat_data;
> 
> (although there's perhaps little point parsing the pdata from DT if it's
> already provided through the device object)

Yeah, especially since tps65910_parse_dt can dev_warn().

> 
>>  	}
>>  
>> -	if (!pmic_plat_data)
>> +	if (!pmic_plat_data || chip_id < 0)
>>  		return -EINVAL;
> 

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

* Re: [PATCH 1/2] mfd: tps65910: Fix crash in i2c_driver .probe
@ 2013-06-18  9:35             ` Tuomas Tynkkynen
  0 siblings, 0 replies; 19+ messages in thread
From: Tuomas Tynkkynen @ 2013-06-18  9:35 UTC (permalink / raw)
  To: Stephen Warren
  Cc: Samuel Ortiz, Mark Brown, linux-tegra, linux-kernel, linux-i2c,
	Linus Walleij

On 06/17/2013 09:07 PM, Stephen Warren wrote:
> On 06/17/2013 11:47 AM, Tuomas Tynkkynen wrote:
>> Commit "i2c: core: make it possible to match a pure device tree driver"
>> changed semantics of the i2c probing for device tree devices.
>> Device tree probed devices now get a NULL i2c_device_id pointer.
>> This caused kernel panics due to NULL dereference.
> 
>> diff --git a/drivers/mfd/tps65910.c b/drivers/mfd/tps65910.c
> 
>>  	pmic_plat_data = dev_get_platdata(&i2c->dev);
>>  
>> -	if (!pmic_plat_data && i2c->dev.of_node) {
>> +	if (id) {
>> +		chip_id = id->driver_data;
>> +	} else if (i2c->dev.of_node) {
>>  		pmic_plat_data = tps65910_parse_dt(i2c, &chip_id);
> 
> That over-writes pmic_plat_data even if it was already set above. This
> should really only happen if the earlier assignment didn't find any
> pdata, i.e. if (!pmic_plat_data) here.

That would cause the probe() to fail since it doesn't have a chip_id.

> 
> Looking at patch 2/2, the structure in that driver is correct, and
> perhaps could be implemented the same or similarly here?
> 

This seems to be the best way, I'll change it that way.

>>  		of_pmic_plat_data = pmic_plat_data;
> 
> Or just swap those assignments:
> 
> of_pmic_plat_data = tps65910_parse_dt(...);
> if (!pmic_plat_data)
>     pmic_plat_data = of_pmic_plat_data;
> 
> (although there's perhaps little point parsing the pdata from DT if it's
> already provided through the device object)

Yeah, especially since tps65910_parse_dt can dev_warn().

> 
>>  	}
>>  
>> -	if (!pmic_plat_data)
>> +	if (!pmic_plat_data || chip_id < 0)
>>  		return -EINVAL;
> 


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

* Re: [PATCH 0/2] Fix kernel panics with certain I2C tps6* chips
  2013-06-17 17:47 ` Tuomas Tynkkynen
@ 2013-06-18 15:58     ` Wolfram Sang
  -1 siblings, 0 replies; 19+ messages in thread
From: Wolfram Sang @ 2013-06-18 15:58 UTC (permalink / raw)
  To: Tuomas Tynkkynen
  Cc: Samuel Ortiz, Mark Brown, linux-tegra-u79uwXL29TY76Z2rM5mHXA,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA,
	linux-i2c-u79uwXL29TY76Z2rM5mHXA, Linus Walleij

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

On Mon, Jun 17, 2013 at 08:47:35PM +0300, Tuomas Tynkkynen wrote:
> Hi,
> 
> Latest linux-next head (next-20130617) seems to have some backwards-incompatible
> changes to the i2c core, which breaks the tps* drivers in our boards and cause
> panics on boot.

Thanks for pointing out. I am going to revert the commit in hope for a
better solution.

So, this series is not needed.

Regards,

   Wolfram

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

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

* Re: [PATCH 0/2] Fix kernel panics with certain I2C tps6* chips
@ 2013-06-18 15:58     ` Wolfram Sang
  0 siblings, 0 replies; 19+ messages in thread
From: Wolfram Sang @ 2013-06-18 15:58 UTC (permalink / raw)
  To: Tuomas Tynkkynen
  Cc: Samuel Ortiz, Mark Brown, linux-tegra, linux-kernel, linux-i2c,
	Linus Walleij

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

On Mon, Jun 17, 2013 at 08:47:35PM +0300, Tuomas Tynkkynen wrote:
> Hi,
> 
> Latest linux-next head (next-20130617) seems to have some backwards-incompatible
> changes to the i2c core, which breaks the tps* drivers in our boards and cause
> panics on boot.

Thanks for pointing out. I am going to revert the commit in hope for a
better solution.

So, this series is not needed.

Regards,

   Wolfram

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

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

end of thread, other threads:[~2013-06-18 15:58 UTC | newest]

Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-06-17 17:47 [PATCH 0/2] Fix kernel panics with certain I2C tps6* chips Tuomas Tynkkynen
2013-06-17 17:47 ` Tuomas Tynkkynen
     [not found] ` <1371491257-23791-1-git-send-email-ttynkkynen-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
2013-06-17 17:47   ` [PATCH 1/2] mfd: tps65910: Fix crash in i2c_driver .probe Tuomas Tynkkynen
2013-06-17 17:47     ` Tuomas Tynkkynen
     [not found]     ` <1371491257-23791-2-git-send-email-ttynkkynen-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
2013-06-17 18:07       ` Stephen Warren
2013-06-17 18:07         ` Stephen Warren
     [not found]         ` <51BF5060.8060001-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org>
2013-06-18  9:35           ` Tuomas Tynkkynen
2013-06-18  9:35             ` Tuomas Tynkkynen
2013-06-17 18:16       ` Stephen Warren
2013-06-17 18:16         ` Stephen Warren
     [not found]         ` <51BF5281.2030105-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org>
2013-06-18  9:16           ` Samuel Ortiz
2013-06-18  9:16             ` Samuel Ortiz
2013-06-17 17:47   ` [PATCH 2/2] regulator: tps62360: " Tuomas Tynkkynen
2013-06-17 17:47     ` Tuomas Tynkkynen
2013-06-17 18:16     ` Stephen Warren
2013-06-18  7:52   ` [PATCH 0/2] Fix kernel panics with certain I2C tps6* chips Linus Walleij
2013-06-18  7:52     ` Linus Walleij
2013-06-18 15:58   ` Wolfram Sang
2013-06-18 15:58     ` Wolfram Sang

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.