| Message ID | 1432806373-17372-3-git-send-email-u.kleine-koenig@pengutronix.de (mailing list archive) |
|---|---|
| State | RFC |
| Delegated to: | Johannes Berg |
| Headers | show |
On Thu, 2015-05-28 at 11:46 +0200, Uwe Kleine-König wrote: > When assigning directly to the pointer contained in the driver data the > local variable can be dropped together with the extra assignment to it. I'm not really sure I see the big benefit of this? It doesn't really seem to make the code much easier to read/follow. I also don't really see the (perceived) objection with "dynamic memory" though since that memory is freed pretty much immediately as soon as we return a non-zero value from this function ... the function itself allocates the memory, and clearly we return without it ever being able to use it, so ... Anyway - I might apply this for the few removed lines of code, but only with a better commit log. johannes -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, May 29, 2015 at 01:17:04PM +0200, Johannes Berg wrote: > On Thu, 2015-05-28 at 11:46 +0200, Uwe Kleine-König wrote: > > When assigning directly to the pointer contained in the driver data the > > local variable can be dropped together with the extra assignment to it. > > I'm not really sure I see the big benefit of this? It doesn't really > seem to make the code much easier to read/follow. > > I also don't really see the (perceived) objection with "dynamic memory" > though since that memory is freed pretty much immediately as soon as we > return a non-zero value from this function ... the function itself > allocates the memory, and clearly we return without it ever being able > to use it, so ... > > Anyway - I might apply this for the few removed lines of code, but only > with a better commit log. Alternatively squash it into patch 1/2 and add: While touching the code simplify it a bit to not need a local variable for the gpio descriptors. ? Best regards Uwe
On Fri, 2015-05-29 at 14:42 +0200, Uwe Kleine-König wrote: > On Fri, May 29, 2015 at 01:17:04PM +0200, Johannes Berg wrote: > > On Thu, 2015-05-28 at 11:46 +0200, Uwe Kleine-König wrote: > > > When assigning directly to the pointer contained in the driver data the > > > local variable can be dropped together with the extra assignment to it. > > > > I'm not really sure I see the big benefit of this? It doesn't really > > seem to make the code much easier to read/follow. > > > > I also don't really see the (perceived) objection with "dynamic memory" > > though since that memory is freed pretty much immediately as soon as we > > return a non-zero value from this function ... the function itself > > allocates the memory, and clearly we return without it ever being able > > to use it, so ... > > > > Anyway - I might apply this for the few removed lines of code, but only > > with a better commit log. > Alternatively squash it into patch 1/2 and add: > > While touching the code simplify it a bit to not need a local > variable for the gpio descriptors. Too late, already applied & pushed out the other one. johannes -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello, On Fri, May 29, 2015 at 02:45:10PM +0200, Johannes Berg wrote: > On Fri, 2015-05-29 at 14:42 +0200, Uwe Kleine-König wrote: > > On Fri, May 29, 2015 at 01:17:04PM +0200, Johannes Berg wrote: > > > On Thu, 2015-05-28 at 11:46 +0200, Uwe Kleine-König wrote: > > > > When assigning directly to the pointer contained in the driver data the > > > > local variable can be dropped together with the extra assignment to it. > > > > > > I'm not really sure I see the big benefit of this? It doesn't really > > > seem to make the code much easier to read/follow. > > > > > > I also don't really see the (perceived) objection with "dynamic memory" > > > though since that memory is freed pretty much immediately as soon as we > > > return a non-zero value from this function ... the function itself > > > allocates the memory, and clearly we return without it ever being able > > > to use it, so ... > > > > > > Anyway - I might apply this for the few removed lines of code, but only > > > with a better commit log. > > Alternatively squash it into patch 1/2 and add: > > > > While touching the code simplify it a bit to not need a local > > variable for the gpio descriptors. > > Too late, already applied & pushed out the other one. Ah, I thought you wanted a better commit log, didn't understand that you looked into that yourself. Best regards and thanks Uwe
Hello again, On Fri, May 29, 2015 at 02:46:37PM +0200, Uwe Kleine-König wrote: > On Fri, May 29, 2015 at 02:45:10PM +0200, Johannes Berg wrote: > > On Fri, 2015-05-29 at 14:42 +0200, Uwe Kleine-König wrote: > > > On Fri, May 29, 2015 at 01:17:04PM +0200, Johannes Berg wrote: > > > > On Thu, 2015-05-28 at 11:46 +0200, Uwe Kleine-König wrote: > > > > > When assigning directly to the pointer contained in the driver data the > > > > > local variable can be dropped together with the extra assignment to it. > > > > > > > > I'm not really sure I see the big benefit of this? It doesn't really > > > > seem to make the code much easier to read/follow. > > > > > > > > I also don't really see the (perceived) objection with "dynamic memory" > > > > though since that memory is freed pretty much immediately as soon as we > > > > return a non-zero value from this function ... the function itself > > > > allocates the memory, and clearly we return without it ever being able > > > > to use it, so ... > > > > > > > > Anyway - I might apply this for the few removed lines of code, but only > > > > with a better commit log. > > > Alternatively squash it into patch 1/2 and add: > > > > > > While touching the code simplify it a bit to not need a local > > > variable for the gpio descriptors. > > > > Too late, already applied & pushed out the other one. > Ah, I thought you wanted a better commit log, didn't understand that you > looked into that yourself. Ah, misunderstood once more. Will take a look for a better commit log. Uwe
diff --git a/net/rfkill/rfkill-gpio.c b/net/rfkill/rfkill-gpio.c index d5d58d919552..fd540024c506 100644 --- a/net/rfkill/rfkill-gpio.c +++ b/net/rfkill/rfkill-gpio.c @@ -92,7 +92,6 @@ static int rfkill_gpio_probe(struct platform_device *pdev) { struct rfkill_gpio_platform_data *pdata = pdev->dev.platform_data; struct rfkill_gpio_data *rfkill; - struct gpio_desc *gpio; int ret; rfkill = devm_kzalloc(&pdev->dev, sizeof(*rfkill), GFP_KERNEL); @@ -112,17 +111,15 @@ static int rfkill_gpio_probe(struct platform_device *pdev) rfkill->clk = devm_clk_get(&pdev->dev, NULL); - gpio = devm_gpiod_get_optional(&pdev->dev, "reset", GPIOD_OUT_LOW); - if (IS_ERR(gpio)) - return PTR_ERR(gpio); + rfkill->reset_gpio = devm_gpiod_get_optional(&pdev->dev, "reset", + GPIOD_OUT_LOW); + if (IS_ERR(rfkill->reset_gpio)) + return PTR_ERR(rfkill->reset_gpio); - rfkill->reset_gpio = gpio; - - gpio = devm_gpiod_get_optional(&pdev->dev, "shutdown", GPIOD_OUT_LOW); - if (IS_ERR(gpio)) - return PTR_ERR(gpio); - - rfkill->shutdown_gpio = gpio; + rfkill->shutdown_gpio = devm_gpiod_get_optional(&pdev->dev, "shutdown", + GPIOD_OUT_LOW); + if (IS_ERR(rfkill->shutdown_gpio)) + return PTR_ERR(rfkill->shutdown_gpio); /* Make sure at-least one of the GPIO is defined and that * a name is specified for this instance
When assigning directly to the pointer contained in the driver data the local variable can be dropped together with the extra assignment to it. The downside is that davem doesn't like writing error pointers to dynamically allocated memory as this might result in bugs like: if (driverdata->gpio) free_resources(driverdata->gpio); which is probably wrong for error values. In this case however there is no cleanup code for the gpios as everything is managed by devm_* and furthermore already without this change there is an error path further down in rfkill_gpio_probe that potentially keeps a valid gpio descriptor in driver data which must not be used. For this eventually calling some cleanup function is hardly better than on an error pointer. Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> --- net/rfkill/rfkill-gpio.c | 19 ++++++++----------- 1 file changed, 8 insertions(+), 11 deletions(-)