| Message ID | 1371491257-23791-2-git-send-email-ttynkkynen@nvidia.com |
|---|---|
| State | Not Applicable |
| Headers | show |
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; -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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) -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
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; > -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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);
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(-)