| Message ID | 20160906131643.1930011-1-arnd@arndb.de (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
Hi Arnd, On Tue, Sep 06, 2016 at 03:16:33PM +0200, Arnd Bergmann wrote: > After the change to use the gpio descriptor interface, we get a warning if > -Wmaybe-uninitialized is added back to the build flags (it is currently > disabled: > > drivers/power/supply/sbs-battery.c: In function 'sbs_probe': > drivers/power/supply/sbs-battery.c:760:28: error: 'pdata' may be used uninitialized in this function [-Werror=maybe-uninitialized] > > The problem is that if neither the DT properties nor a platform_data > pointer are provided, the chip->pdata pointer gets set to an uninitialized > value. > > Looking at the code some more, I found that the sbs_of_populate_pdata > function is more complex than necessary and has confusing calling > conventions of possibly returning a valid pointer, a NULL pointer > or an ERR_PTR pointer (in addition to the uninitialized pointer). > > To fix all of that, this gets rid of the chip->pdata pointer and > simply moves the two integers into the sbs_info structure. This > makes it much clearer from reading sbs_probe() what the precedence > of the three possible values are (pdata, DT, hardcoded defaults) > and completely avoids the #ifdef CONFIG_OF guards as > of_property_read_u32() gets replaced with a compile-time stub > when that is disabled, and returns an error if passed a NULL of_node > pointer. > > Signed-off-by: Arnd Bergmann <arnd@arndb.de> > Fixes: 3b5dd3a49496 ("power: supply: sbs-battery: Use gpio_desc and sleeping calls for battery detect") Patch looks fine to me. Actually I already asked Phil to implement your change [0]. I just queued it to power-supply's for-next branch. [0] https://marc.info/?l=linux-pm&m=147273382018953&w=2 -- Sebastian
On Wednesday, September 7, 2016 1:55:23 AM CEST Sebastian Reichel wrote: > > Patch looks fine to me. Actually I already asked Phil to > implement your change [0]. I just queued it to power-supply's > for-next branch. > > [0] https://marc.info/?l=linux-pm&m=147273382018953&w=2 Glad to see we had the same thought. ;-) I see that we went for different defaults for chip->poll_retry_count: I concluded that '0' would be the correct default, while you suggested '1' in your reply. Can you double-check the code and see which one is right? Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, On Wed, Sep 07, 2016 at 09:58:10AM +0200, Arnd Bergmann wrote: > On Wednesday, September 7, 2016 1:55:23 AM CEST Sebastian Reichel wrote: > > > > Patch looks fine to me. Actually I already asked Phil to > > implement your change [0]. I just queued it to power-supply's > > for-next branch. > > > > [0] https://marc.info/?l=linux-pm&m=147273382018953&w=2 > > Glad to see we had the same thought. ;-) > > I see that we went for different defaults for chip->poll_retry_count: > I concluded that '0' would be the correct default, while you suggested '1' > in your reply. > > Can you double-check the code and see which one is right? 0 is correct. It will be incremented to 1 later. -- Sebastian
G'day Arnd, Sorry for the delay. Been distracted and just catching up on mail I was just getting back to implementing basically this same thing as requested by Sebastian when I saw your change. Couple of comments below, I haven't tested it. On 6/09/2016 21:16, Arnd Bergmann wrote: > After the change to use the gpio descriptor interface, we get a warning if > -Wmaybe-uninitialized is added back to the build flags (it is currently > disabled: > > drivers/power/supply/sbs-battery.c: In function 'sbs_probe': > drivers/power/supply/sbs-battery.c:760:28: error: 'pdata' may be used uninitialized in this function [-Werror=maybe-uninitialized] > > The problem is that if neither the DT properties nor a platform_data > pointer are provided, the chip->pdata pointer gets set to an uninitialized > value. > > Looking at the code some more, I found that the sbs_of_populate_pdata > function is more complex than necessary and has confusing calling > conventions of possibly returning a valid pointer, a NULL pointer > or an ERR_PTR pointer (in addition to the uninitialized pointer). > > To fix all of that, this gets rid of the chip->pdata pointer and > simply moves the two integers into the sbs_info structure. This > makes it much clearer from reading sbs_probe() what the precedence > of the three possible values are (pdata, DT, hardcoded defaults) > and completely avoids the #ifdef CONFIG_OF guards as > of_property_read_u32() gets replaced with a compile-time stub > when that is disabled, and returns an error if passed a NULL of_node > pointer. > > Signed-off-by: Arnd Bergmann <arnd@arndb.de> > Fixes: 3b5dd3a49496 ("power: supply: sbs-battery: Use gpio_desc and sleeping calls for battery detect") > --- > drivers/power/supply/sbs-battery.c | 98 ++++++++++++-------------------------- > 1 file changed, 31 insertions(+), 67 deletions(-) > > diff --git a/drivers/power/supply/sbs-battery.c b/drivers/power/supply/sbs-battery.c > index 8b4c6a8681b0..248a5dd75e22 100644 > --- a/drivers/power/supply/sbs-battery.c > +++ b/drivers/power/supply/sbs-battery.c > @@ -169,6 +169,8 @@ struct sbs_info { > bool enable_detection; > int last_state; > int poll_time; > + int i2c_retry_count; > + int poll_retry_count; > struct delayed_work work; > int ignore_changes; > }; Done we need to remove pdata from here now. It's not set in the probe funciton anymore. > @@ -184,7 +186,7 @@ static int sbs_read_word_data(struct i2c_client *client, u8 address) > int retries = 1; > > if (chip->pdata) > - retries = max(chip->pdata->i2c_retry_count + 1, 1); > + retries = max(chip->i2c_retry_count + 1, 1); I don't think this is quite right. as pdata is never set, condition needs to removed. > > while (retries > 0) { > ret = i2c_smbus_read_word_data(client, address); > @@ -212,8 +214,8 @@ static int sbs_read_string_data(struct i2c_client *client, u8 address, > u8 block_buffer[I2C_SMBUS_BLOCK_MAX + 1]; > > if (chip->pdata) { > - retries_length = max(chip->pdata->i2c_retry_count + 1, 1); > - retries_block = max(chip->pdata->i2c_retry_count + 1, 1); > + retries_length = max(chip->i2c_retry_count + 1, 1); > + retries_block = max(chip->i2c_retry_count + 1, 1); > } ditto > > /* Adapter needs to support these two functions */ > @@ -279,7 +281,7 @@ static int sbs_write_word_data(struct i2c_client *client, u8 address, > int retries = 1; > > if (chip->pdata) > - retries = max(chip->pdata->i2c_retry_count + 1, 1); > + retries = max(chip->i2c_retry_count + 1, 1); ditto > > while (retries > 0) { > ret = i2c_smbus_write_word_data(client, address, > @@ -741,61 +743,6 @@ static void sbs_delayed_work(struct work_struct *work) > } > } > > -#if defined(CONFIG_OF) > - > -#include <linux/of_device.h> > -#include <linux/of_gpio.h> > - > -static const struct of_device_id sbs_dt_ids[] = { > - { .compatible = "sbs,sbs-battery" }, > - { .compatible = "ti,bq20z75" }, > - { } > -}; > -MODULE_DEVICE_TABLE(of, sbs_dt_ids); > - > -static struct sbs_platform_data *sbs_of_populate_pdata(struct sbs_info *chip) > -{ > - struct i2c_client *client = chip->client; > - struct device_node *of_node = client->dev.of_node; > - struct sbs_platform_data *pdata; > - int rc; > - u32 prop; > - > - /* verify this driver matches this device */ > - if (!of_node) > - return NULL; > - > - /* first make sure at least one property is set, otherwise > - * it won't change behavior from running without pdata. > - */ > - if (!of_get_property(of_node, "sbs,i2c-retry-count", NULL) && > - !of_get_property(of_node, "sbs,poll-retry-count", NULL)) > - goto of_out; > - > - pdata = devm_kzalloc(&client->dev, sizeof(struct sbs_platform_data), > - GFP_KERNEL); > - if (!pdata) > - return ERR_PTR(-ENOMEM); > - > - rc = of_property_read_u32(of_node, "sbs,i2c-retry-count", &prop); > - if (!rc) > - pdata->i2c_retry_count = prop; > - > - rc = of_property_read_u32(of_node, "sbs,poll-retry-count", &prop); > - if (!rc) > - pdata->poll_retry_count = prop; > - > -of_out: > - return pdata; > -} > -#else > -static struct sbs_platform_data *sbs_of_populate_pdata( > - struct sbs_info *client) > -{ > - return ERR_PTR(-ENOSYS); > -} > -#endif > - > static const struct power_supply_desc sbs_default_desc = { > .type = POWER_SUPPLY_TYPE_BATTERY, > .properties = sbs_properties, > @@ -838,13 +785,23 @@ static int sbs_probe(struct i2c_client *client, > chip->ignore_changes = 1; > chip->last_state = POWER_SUPPLY_STATUS_UNKNOWN; > > - if (!pdata) > - pdata = sbs_of_populate_pdata(chip); > - > - if (IS_ERR(pdata)) > - return PTR_ERR(pdata); > - > - chip->pdata = pdata; No longer set here. > + /* use pdata if available, fall back to DT properties, > + * or hardcoded defaults if not > + */ > + rc = of_property_read_u32(client->dev.of_node, "sbs,i2c-retry-count", > + &chip->i2c_retry_count); > + if (rc) > + chip->i2c_retry_count = 1; > + > + rc = of_property_read_u32(client->dev.of_node, "sbs,poll-retry-count", > + &chip->poll_retry_count); > + if (rc) > + chip->poll_retry_count = 0; > + > + if (pdata) { > + chip->poll_retry_count = pdata->poll_retry_count; > + chip->i2c_retry_count = pdata->i2c_retry_count; > + } > > chip->gpio_detect = devm_gpiod_get_optional(&client->dev, > "sbs,battery-detect", GPIOD_IN); > @@ -953,13 +910,20 @@ static const struct i2c_device_id sbs_id[] = { > }; > MODULE_DEVICE_TABLE(i2c, sbs_id); > > +static const struct of_device_id sbs_dt_ids[] = { > + { .compatible = "sbs,sbs-battery" }, > + { .compatible = "ti,bq20z75" }, > + { } > +}; > +MODULE_DEVICE_TABLE(of, sbs_dt_ids); > + > static struct i2c_driver sbs_battery_driver = { > .probe = sbs_probe, > .remove = sbs_remove, > .id_table = sbs_id, > .driver = { > .name = "sbs-battery", > - .of_match_table = of_match_ptr(sbs_dt_ids), > + .of_match_table = sbs_dt_ids, > .pm = SBS_PM_OPS, > }, > }; > Also sbs_external_power_changed is still referencing chip->pdata->poll_retry_count. Which I think will result in null pointer dereference now.
diff --git a/drivers/power/supply/sbs-battery.c b/drivers/power/supply/sbs-battery.c index 8b4c6a8681b0..248a5dd75e22 100644 --- a/drivers/power/supply/sbs-battery.c +++ b/drivers/power/supply/sbs-battery.c @@ -169,6 +169,8 @@ struct sbs_info { bool enable_detection; int last_state; int poll_time; + int i2c_retry_count; + int poll_retry_count; struct delayed_work work; int ignore_changes; }; @@ -184,7 +186,7 @@ static int sbs_read_word_data(struct i2c_client *client, u8 address) int retries = 1; if (chip->pdata) - retries = max(chip->pdata->i2c_retry_count + 1, 1); + retries = max(chip->i2c_retry_count + 1, 1); while (retries > 0) { ret = i2c_smbus_read_word_data(client, address); @@ -212,8 +214,8 @@ static int sbs_read_string_data(struct i2c_client *client, u8 address, u8 block_buffer[I2C_SMBUS_BLOCK_MAX + 1]; if (chip->pdata) { - retries_length = max(chip->pdata->i2c_retry_count + 1, 1); - retries_block = max(chip->pdata->i2c_retry_count + 1, 1); + retries_length = max(chip->i2c_retry_count + 1, 1); + retries_block = max(chip->i2c_retry_count + 1, 1); } /* Adapter needs to support these two functions */ @@ -279,7 +281,7 @@ static int sbs_write_word_data(struct i2c_client *client, u8 address, int retries = 1; if (chip->pdata) - retries = max(chip->pdata->i2c_retry_count + 1, 1); + retries = max(chip->i2c_retry_count + 1, 1); while (retries > 0) { ret = i2c_smbus_write_word_data(client, address, @@ -741,61 +743,6 @@ static void sbs_delayed_work(struct work_struct *work) } } -#if defined(CONFIG_OF) - -#include <linux/of_device.h> -#include <linux/of_gpio.h> - -static const struct of_device_id sbs_dt_ids[] = { - { .compatible = "sbs,sbs-battery" }, - { .compatible = "ti,bq20z75" }, - { } -}; -MODULE_DEVICE_TABLE(of, sbs_dt_ids); - -static struct sbs_platform_data *sbs_of_populate_pdata(struct sbs_info *chip) -{ - struct i2c_client *client = chip->client; - struct device_node *of_node = client->dev.of_node; - struct sbs_platform_data *pdata; - int rc; - u32 prop; - - /* verify this driver matches this device */ - if (!of_node) - return NULL; - - /* first make sure at least one property is set, otherwise - * it won't change behavior from running without pdata. - */ - if (!of_get_property(of_node, "sbs,i2c-retry-count", NULL) && - !of_get_property(of_node, "sbs,poll-retry-count", NULL)) - goto of_out; - - pdata = devm_kzalloc(&client->dev, sizeof(struct sbs_platform_data), - GFP_KERNEL); - if (!pdata) - return ERR_PTR(-ENOMEM); - - rc = of_property_read_u32(of_node, "sbs,i2c-retry-count", &prop); - if (!rc) - pdata->i2c_retry_count = prop; - - rc = of_property_read_u32(of_node, "sbs,poll-retry-count", &prop); - if (!rc) - pdata->poll_retry_count = prop; - -of_out: - return pdata; -} -#else -static struct sbs_platform_data *sbs_of_populate_pdata( - struct sbs_info *client) -{ - return ERR_PTR(-ENOSYS); -} -#endif - static const struct power_supply_desc sbs_default_desc = { .type = POWER_SUPPLY_TYPE_BATTERY, .properties = sbs_properties, @@ -838,13 +785,23 @@ static int sbs_probe(struct i2c_client *client, chip->ignore_changes = 1; chip->last_state = POWER_SUPPLY_STATUS_UNKNOWN; - if (!pdata) - pdata = sbs_of_populate_pdata(chip); - - if (IS_ERR(pdata)) - return PTR_ERR(pdata); - - chip->pdata = pdata; + /* use pdata if available, fall back to DT properties, + * or hardcoded defaults if not + */ + rc = of_property_read_u32(client->dev.of_node, "sbs,i2c-retry-count", + &chip->i2c_retry_count); + if (rc) + chip->i2c_retry_count = 1; + + rc = of_property_read_u32(client->dev.of_node, "sbs,poll-retry-count", + &chip->poll_retry_count); + if (rc) + chip->poll_retry_count = 0; + + if (pdata) { + chip->poll_retry_count = pdata->poll_retry_count; + chip->i2c_retry_count = pdata->i2c_retry_count; + } chip->gpio_detect = devm_gpiod_get_optional(&client->dev, "sbs,battery-detect", GPIOD_IN); @@ -953,13 +910,20 @@ static const struct i2c_device_id sbs_id[] = { }; MODULE_DEVICE_TABLE(i2c, sbs_id); +static const struct of_device_id sbs_dt_ids[] = { + { .compatible = "sbs,sbs-battery" }, + { .compatible = "ti,bq20z75" }, + { } +}; +MODULE_DEVICE_TABLE(of, sbs_dt_ids); + static struct i2c_driver sbs_battery_driver = { .probe = sbs_probe, .remove = sbs_remove, .id_table = sbs_id, .driver = { .name = "sbs-battery", - .of_match_table = of_match_ptr(sbs_dt_ids), + .of_match_table = sbs_dt_ids, .pm = SBS_PM_OPS, }, };
After the change to use the gpio descriptor interface, we get a warning if -Wmaybe-uninitialized is added back to the build flags (it is currently disabled: drivers/power/supply/sbs-battery.c: In function 'sbs_probe': drivers/power/supply/sbs-battery.c:760:28: error: 'pdata' may be used uninitialized in this function [-Werror=maybe-uninitialized] The problem is that if neither the DT properties nor a platform_data pointer are provided, the chip->pdata pointer gets set to an uninitialized value. Looking at the code some more, I found that the sbs_of_populate_pdata function is more complex than necessary and has confusing calling conventions of possibly returning a valid pointer, a NULL pointer or an ERR_PTR pointer (in addition to the uninitialized pointer). To fix all of that, this gets rid of the chip->pdata pointer and simply moves the two integers into the sbs_info structure. This makes it much clearer from reading sbs_probe() what the precedence of the three possible values are (pdata, DT, hardcoded defaults) and completely avoids the #ifdef CONFIG_OF guards as of_property_read_u32() gets replaced with a compile-time stub when that is disabled, and returns an error if passed a NULL of_node pointer. Signed-off-by: Arnd Bergmann <arnd@arndb.de> Fixes: 3b5dd3a49496 ("power: supply: sbs-battery: Use gpio_desc and sleeping calls for battery detect") --- drivers/power/supply/sbs-battery.c | 98 ++++++++++++-------------------------- 1 file changed, 31 insertions(+), 67 deletions(-)