Discussion:
[PATCH v2 1/3] gpio: davinci: Use devm_gpiochip_add_data in place of gpiochip_add_data
Keerthy
2017-07-20 09:42:16 UTC
Permalink
Use the devm version of gpiochip_add_data and pass on the
return value. This avoids memory leak due to gpiochip_add_data
in case the driver is unbound.

Signed-off-by: Keerthy <j-***@ti.com>
---

Changes in v2:

* Restoring the previous values of ctrl_num and bank_base in case of an error.

drivers/gpio/gpio-davinci.c | 14 ++++++++++++--
1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/drivers/gpio/gpio-davinci.c b/drivers/gpio/gpio-davinci.c
index 65cb359..27499ec 100644
--- a/drivers/gpio/gpio-davinci.c
+++ b/drivers/gpio/gpio-davinci.c
@@ -166,7 +166,7 @@ static int davinci_gpio_get(struct gpio_chip *chip, unsigned offset)
static int davinci_gpio_probe(struct platform_device *pdev)
{
static int ctrl_num, bank_base;
- int gpio, bank;
+ int gpio, bank, ret = 0;
unsigned ngpio, nbank;
struct davinci_gpio_controller *chips;
struct davinci_gpio_platform_data *pdata;
@@ -232,10 +232,20 @@ static int davinci_gpio_probe(struct platform_device *pdev)
for (gpio = 0, bank = 0; gpio < ngpio; gpio += 32, bank++)
chips->regs[bank] = gpio_base + offset_array[bank];

- gpiochip_add_data(&chips->chip, chips);
+ ret = devm_gpiochip_add_data(dev, &chips->chip, chips);
+ if (ret)
+ goto err;
+
platform_set_drvdata(pdev, chips);
davinci_gpio_irq_setup(pdev);
return 0;
+
+err:
+ /* Revert the static variable increments */
+ ctrl_num--;
+ bank_base -= ngpio;
+
+ return ret;
}

/*--------------------------------------------------------------------------*/
--
1.9.1
Keerthy
2017-07-20 09:42:18 UTC
Permalink
In case of devm_clk_get failure use dev_err instead of printk

Signed-off-by: Keerthy <j-***@ti.com>
---
drivers/gpio/gpio-davinci.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/gpio/gpio-davinci.c b/drivers/gpio/gpio-davinci.c
index d6fb1ce..f75d844 100644
--- a/drivers/gpio/gpio-davinci.c
+++ b/drivers/gpio/gpio-davinci.c
@@ -490,8 +490,7 @@ static int davinci_gpio_irq_setup(struct platform_device *pdev)

clk = devm_clk_get(dev, "gpio");
if (IS_ERR(clk)) {
- printk(KERN_ERR "Error %ld getting gpio clock?\n",
- PTR_ERR(clk));
+ dev_err(dev, "Error %ld getting gpio clock\n", PTR_ERR(clk));
return PTR_ERR(clk);
}
ret = clk_prepare_enable(clk);
--
1.9.1
Keerthy
2017-07-20 09:42:17 UTC
Permalink
Currently davinci_gpio_irq_setup return value is ignored. Handle the
return value appropriately.

Signed-off-by: Keerthy <j-***@ti.com>
---
drivers/gpio/gpio-davinci.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/gpio/gpio-davinci.c b/drivers/gpio/gpio-davinci.c
index 27499ec..d6fb1ce 100644
--- a/drivers/gpio/gpio-davinci.c
+++ b/drivers/gpio/gpio-davinci.c
@@ -237,7 +237,10 @@ static int davinci_gpio_probe(struct platform_device *pdev)
goto err;

platform_set_drvdata(pdev, chips);
- davinci_gpio_irq_setup(pdev);
+ ret = davinci_gpio_irq_setup(pdev);
+ if (ret)
+ goto err;
+
return 0;

err:
--
1.9.1
Keerthy
2017-07-20 09:43:44 UTC
Permalink
The patch series does a couple of important return handling which were missed
earlier in the driver probe.
http://marc.info/?l=linux-arm-kernel&m=150034845427555&w=2
http://marc.info/?l=linux-arm-kernel&m=150034856627618&w=2
My bad! The series has no dependency on the above. I have split the
device tree patches to keep it independent.
* Removed the device tree patch for to keep the series independent.
gpio: davinci: Use devm_gpiochip_add_data in place of
gpiochip_add_data
gpio: davinci: Handle the return value of davinci_gpio_irq_setup
function
gpio: davinci: Convert prinkt to dev_err
drivers/gpio/gpio-davinci.c | 22 +++++++++++++++++-----
1 file changed, 17 insertions(+), 5 deletions(-)
Suman Anna
2017-07-20 19:57:50 UTC
Permalink
Hi Keerthy,
Post by Keerthy
The patch series does a couple of important return handling which were missed
earlier in the driver probe.
http://marc.info/?l=linux-arm-kernel&m=150034845427555&w=2
http://marc.info/?l=linux-arm-kernel&m=150034856627618&w=2
My bad! The series has no dependency on the above. I have split the
device tree patches to keep it independent.
The new patches looks clean. The davinci_gpio_irq_setup() function
though could also do with couple more cleanups. Not all failure paths
are doing the necessary cleanup (there are some non devres functions
also called, but not cleaned up).

regards
Suman
Post by Keerthy
* Removed the device tree patch for to keep the series independent.
gpio: davinci: Use devm_gpiochip_add_data in place of
gpiochip_add_data
gpio: davinci: Handle the return value of davinci_gpio_irq_setup
function
gpio: davinci: Convert prinkt to dev_err
drivers/gpio/gpio-davinci.c | 22 +++++++++++++++++-----
1 file changed, 17 insertions(+), 5 deletions(-)
Loading...