Re: [PATCH] i2c: davinci: Fix null dereference bug in i2c_davinci_calc_clk_dividers
Hi Mr. Dooks, On 5/2/2011 5:36 AM, Ben Dooks wrote: > On Sat, Apr 23, 2011 at 01:31:35PM -0400, Michael Williamson wrote: >> The davinci-i2c bus driver currently supports being probed with a NULL >> platform_data structure by using a default configuration when none is >> provided. However, the i2c_davinci_calc_clk_dividers does not >> check the provided platform_data for NULL prior to using it, so >> the support is incomplete. >> >> Rather than have each runtime call check for NULL and avoid future >> problems, copy a pointer to the default data to the device structure >> during the probe. This should be OK, as the information in this >> structure is not modified by the driver. >> >> Tested on a MityDSP-L138 module (OMAP-L138 based SOM). >> >> Signed-off-by: Micahel Williamson >> --- >> drivers/i2c/busses/i2c-davinci.c | 12 +--- >> 1 files changed, 5 insertions(+), 7 deletions(-) >> >> diff --git a/drivers/i2c/busses/i2c-davinci.c >> b/drivers/i2c/busses/i2c-davinci.c >> index 5795c83..1db8fc9 100644 >> --- a/drivers/i2c/busses/i2c-davinci.c >> +++ b/drivers/i2c/busses/i2c-davinci.c >> @@ -163,8 +163,7 @@ static void i2c_recover_bus(struct davinci_i2c_dev *dev) >> flag |= DAVINCI_I2C_MDR_NACK; >> /* write the data into mode register */ >> davinci_i2c_write_reg(dev, DAVINCI_I2C_MDR_REG, flag); >> -if (pdata) >> -generic_i2c_clock_pulse(pdata->scl_pin); >> +generic_i2c_clock_pulse(pdata->scl_pin); > > This looks like a change in the behaviour of the code, since if there was > no pdata supplied we now can try generic_i2c_clock_pulse() with what is > probably gpio pin 0. > - There is a check in generic_i2c_clock_pulse() that does not allow for scl_pin pin = 0. I didn't realize gpio pin 0 was a valid gpio when looking at it. Seems like this is an issue as well? - I think this code is always getting called. I don't think there are any platforms that would work using a NULL pdata as i2c_davinci_calc_clk_dividers would bomb. I don't see any platforms in mach-davinci that provide NULL i2c platform_data. Perhaps I'm not looking hard enough. >> /* Send STOP */ >> flag = davinci_i2c_read_reg(dev, DAVINCI_I2C_MDR_REG); >> flag |= DAVINCI_I2C_MDR_STP; >> @@ -235,10 +234,11 @@ static void i2c_davinci_calc_clk_dividers(struct >> davinci_i2c_dev *dev) >> */ >> static int i2c_davinci_init(struct davinci_i2c_dev *dev) >> { >> -struct davinci_i2c_platform_data *pdata = dev->dev->platform_data; >> +struct davinci_i2c_platform_data *pdata; >> >> -if (!pdata) >> -pdata = &davinci_i2c_platform_data_default; >> +if (!dev->dev->platform_data) >> +dev->dev->platform_data = &davinci_i2c_platform_data_default; >> +pdata = dev->dev->platform_data; > > Hmm, not going to work well if the driver is a module, as you'll unload > the module, leave the default pointer in and then re-load later at a > possibly different address with the old pointer in. > I hadn't thought of (or tested) the load->unload->reload scenario. Subtle (well, to me anyway). Thanks for pointing that out. Appreciate your cycles on review. >> /* put I2C into reset */ >> davinci_i2c_reset_ctrl(dev, 0); >> @@ -313,8 +313,6 @@ i2c_davinci_xfer_msg(struct i2c_adapter *adap, struct >> i2c_msg *msg, int stop) >> u16 w; >> int r; >> >> -if (!pdata) >> -pdata = &davinci_i2c_platform_data_default; >> /* Introduce a delay, required for some boards (e.g Davinci EVM) */ >> if (pdata->bus_delay) >> udelay(pdata->bus_delay); >> -- >> 1.7.0.4 >> >> -- >> To unsubscribe from this list: send the line "unsubscribe linux-i2c" in >> the body of a message to majord...@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html > -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] i2c: davinci: Fix null dereference bug in i2c_davinci_calc_clk_dividers
The davinci-i2c bus driver currently supports being probed with a NULL platform_data structure by using a default configuration when none is provided. However, the i2c_davinci_calc_clk_dividers does not check the provided platform_data for NULL prior to using it, so the support is incomplete. Rather than have each runtime call check for NULL and avoid future problems, copy a pointer to the default data to the device structure during the probe. This should be OK, as the information in this structure is not modified by the driver. Tested on a MityDSP-L138 module (OMAP-L138 based SOM). Signed-off-by: Micahel Williamson --- drivers/i2c/busses/i2c-davinci.c | 12 +--- 1 files changed, 5 insertions(+), 7 deletions(-) diff --git a/drivers/i2c/busses/i2c-davinci.c b/drivers/i2c/busses/i2c-davinci.c index 5795c83..1db8fc9 100644 --- a/drivers/i2c/busses/i2c-davinci.c +++ b/drivers/i2c/busses/i2c-davinci.c @@ -163,8 +163,7 @@ static void i2c_recover_bus(struct davinci_i2c_dev *dev) flag |= DAVINCI_I2C_MDR_NACK; /* write the data into mode register */ davinci_i2c_write_reg(dev, DAVINCI_I2C_MDR_REG, flag); - if (pdata) - generic_i2c_clock_pulse(pdata->scl_pin); + generic_i2c_clock_pulse(pdata->scl_pin); /* Send STOP */ flag = davinci_i2c_read_reg(dev, DAVINCI_I2C_MDR_REG); flag |= DAVINCI_I2C_MDR_STP; @@ -235,10 +234,11 @@ static void i2c_davinci_calc_clk_dividers(struct davinci_i2c_dev *dev) */ static int i2c_davinci_init(struct davinci_i2c_dev *dev) { - struct davinci_i2c_platform_data *pdata = dev->dev->platform_data; + struct davinci_i2c_platform_data *pdata; - if (!pdata) - pdata = &davinci_i2c_platform_data_default; + if (!dev->dev->platform_data) + dev->dev->platform_data = &davinci_i2c_platform_data_default; + pdata = dev->dev->platform_data; /* put I2C into reset */ davinci_i2c_reset_ctrl(dev, 0); @@ -313,8 +313,6 @@ i2c_davinci_xfer_msg(struct i2c_adapter *adap, struct i2c_msg *msg, int stop) u16 w; int r; - if (!pdata) - pdata = &davinci_i2c_platform_data_default; /* Introduce a delay, required for some boards (e.g Davinci EVM) */ if (pdata->bus_delay) udelay(pdata->bus_delay); -- 1.7.0.4 -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] i2c-davinci: Fix use of default platform data if none supplied.
On 09/21/2010 08:24 PM, Ben Dooks wrote: > On Sat, Sep 04, 2010 at 12:07:48PM -0400, Michael Williamson wrote: >> There is a bug in the i2c-davinci device init routine that attempts >> to use default platform data if none is supplied (e.g., is NULL). >> This patch fixes the bug. >> >> Signed-off-by: Michael Williamson >> --- >> drivers/i2c/busses/i2c-davinci.c |8 +--- >> 1 files changed, 5 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/i2c/busses/i2c-davinci.c >> b/drivers/i2c/busses/i2c-davinci.c >> index c87..6d4eeeb 100644 >> --- a/drivers/i2c/busses/i2c-davinci.c >> +++ b/drivers/i2c/busses/i2c-davinci.c >> @@ -235,10 +235,12 @@ static void i2c_davinci_calc_clk_dividers(struct >> davinci_i2c_dev *dev) >> */ >> static int i2c_davinci_init(struct davinci_i2c_dev *dev) >> { >> -struct davinci_i2c_platform_data *pdata = dev->dev->platform_data; >> +struct davinci_i2c_platform_data *pdata; >> >> -if (!pdata) >> -pdata = &davinci_i2c_platform_data_default; >> +if (!dev->dev->platform_data) >> +dev->dev->platform_data = &davinci_i2c_platform_data_default; >> + >> +pdata = dev->dev->platform_data; > > At first glance this looks like a code shift, however is the platform > data used later in the driver by referencing the device.platform_data > field? > Yes. The problem call is i2c_davinci_calc_clk_dividers(), which is called pretty early in the i2c_davinci_init() routine. There is a reference to pdata->bus_freq without a check to see if pdata is valid. I think my proposed fix is not the right approach as you have pointed out. Perhaps a better patch would be (?): --- diff --git a/drivers/i2c/busses/i2c-davinci.c b/drivers/i2c/busses/i2c-davinci.c index c87..15d0cea 100644 --- a/drivers/i2c/busses/i2c-davinci.c +++ b/drivers/i2c/busses/i2c-davinci.c @@ -195,6 +195,9 @@ static void i2c_davinci_calc_clk_dividers(struct davinci_i2c_dev *dev) u32 clkl; u32 input_clock = clk_get_rate(dev->clk); + if (!pdata) + pdata = &davinci_i2c_platform_data_default; + /* NOTE: I2C Clock divider programming info * As per I2C specs the following formulas provide prescaler * and low/high divider values --- > Could you rewrite the header to show this, and if possible note any > actual oops report that has been seen with this. > I found this because I was told (while proposing a new platform) to use NULL as platform data as the parameters I was using matched the default data. I didn't get an oops, it just hung (it's pretty early in the board init). Our boot strapper/load resets the DDR where the kernel buffer is, so getting any oops info is a bit time consuming. The above three lines (and the original patch) resolved the problem. I can update the header to indicate a default parameter set if you like. New patch OK? -Mike -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] i2c-davinci: Fix use of default platform data if none supplied.
There is a bug in the i2c-davinci device init routine that attempts to use default platform data if none is supplied (e.g., is NULL). This patch fixes the bug. Signed-off-by: Michael Williamson --- drivers/i2c/busses/i2c-davinci.c |8 +--- 1 files changed, 5 insertions(+), 3 deletions(-) diff --git a/drivers/i2c/busses/i2c-davinci.c b/drivers/i2c/busses/i2c-davinci.c index c87..6d4eeeb 100644 --- a/drivers/i2c/busses/i2c-davinci.c +++ b/drivers/i2c/busses/i2c-davinci.c @@ -235,10 +235,12 @@ static void i2c_davinci_calc_clk_dividers(struct davinci_i2c_dev *dev) */ static int i2c_davinci_init(struct davinci_i2c_dev *dev) { - struct davinci_i2c_platform_data *pdata = dev->dev->platform_data; + struct davinci_i2c_platform_data *pdata; - if (!pdata) - pdata = &davinci_i2c_platform_data_default; + if (!dev->dev->platform_data) + dev->dev->platform_data = &davinci_i2c_platform_data_default; + + pdata = dev->dev->platform_data; /* put I2C into reset */ davinci_i2c_reset_ctrl(dev, 0); -- 1.7.0.4 -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html