Re: [PATCH v2] regulator: wm831x-dcdc: Use pointer after NULL check

Re: [PATCH v2] regulator: wm831x-dcdc: Use pointer after NULL check

[Maninder Singh]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=windows-1252, Size: 914 bytes --]

Hello charles,
>> pdata is used before NULL check, so it looks misleading.
>> If pdata validation is required then we have to
>> first check for pdata validation, then calculate id,
>> and then second check for pdata->dcdc[id].
>> 
>> and it is better to use !pointer than (pointer == NULL)
>> 
>> Signed-off-by: Maninder Singh <maninder1.s@samsung.com>
>> ---

>The patch basically looks fine, but it feels a bit like needless
>churn. The current code is perfectly correct and feels clear
>enough to me.

Reason for patch is we are checking a pointer which is already dereferenced 
before NULL check, which looks misleading.
If we know code is working correctly, Then we may simply drop NULL check,
rather than these changes. 

Thanks,
Maninder
........ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [PATCH v2] regulator: wm831x-dcdc: Use pointer after NULL check

[Charles Keepax]

On Thu, Jul 02, 2015 at 11:02:54AM +0000, Maninder Singh wrote:
> Hello charles,
> >> pdata is used before NULL check, so it looks misleading.
> >> If pdata validation is required then we have to
> >> first check for pdata validation, then calculate id,
> >> and then second check for pdata->dcdc[id].
> >> 
> >> and it is better to use !pointer than (pointer == NULL)
> >> 
> >> Signed-off-by: Maninder Singh <maninder1.s@samsung.com>
> >> ---
> 
> >The patch basically looks fine, but it feels a bit like needless
> >churn. The current code is perfectly correct and feels clear
> >enough to me.
> 
> Reason for patch is we are checking a pointer which is already dereferenced 
> before NULL check, which looks misleading.
> If we know code is working correctly, Then we may simply drop NULL check,
> rather than these changes. 

You can't drop the NULL check because then we might dereference a
NULL pointer. ARRAY_SIZE(pdata->dcdc) is perfectly save because
sizeof only works on types so no actual dereference takes place.

Thanks,
Charles

[PATCH v2] regulator: wm831x-dcdc: Use pointer after NULL check

[Maninder Singh]

pdata is used before NULL check, so it looks misleading.
If pdata validation is required then we have to
first check for pdata validation, then calculate id,
and then second check for pdata->dcdc[id].

and it is better to use !pointer than (pointer == NULL)

Signed-off-by: Maninder Singh <maninder1.s@samsung.com>
---
v2: indentation fixes

 drivers/regulator/wm831x-dcdc.c |   10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/regulator/wm831x-dcdc.c b/drivers/regulator/wm831x-dcdc.c
index 8cbb82c..04ede43 100644
--- a/drivers/regulator/wm831x-dcdc.c
+++ b/drivers/regulator/wm831x-dcdc.c
@@ -738,15 +738,19 @@ static int wm831x_boostp_probe(struct platform_device *pdev)
 	struct wm831x *wm831x = dev_get_drvdata(pdev->dev.parent);
 	struct wm831x_pdata *pdata = dev_get_platdata(wm831x->dev);
 	struct regulator_config config = { };
-	int id = pdev->id % ARRAY_SIZE(pdata->dcdc);
+	int id;
 	struct wm831x_dcdc *dcdc;
 	struct resource *res;
 	int ret, irq;
 
+	if (!pdata)
+		return -ENODEV;
+
+	id = pdev->id % ARRAY_SIZE(pdata->dcdc);
 	dev_dbg(&pdev->dev, "Probing DCDC%d\n", id + 1);
 
-	if (pdata == NULL || pdata->dcdc[id] == NULL)
-		return -ENODEV;
+	if (!pdata->dcdc[id])
+		return -ENODEV;
 
 	dcdc = devm_kzalloc(&pdev->dev, sizeof(struct wm831x_dcdc), GFP_KERNEL);
 	if (!dcdc)
-- 
1.7.9.5

Re: [PATCH v2] regulator: wm831x-dcdc: Use pointer after NULL check

[Charles Keepax]

On Thu, Jul 02, 2015 at 01:27:09PM +0530, Maninder Singh wrote:
> pdata is used before NULL check, so it looks misleading.
> If pdata validation is required then we have to
> first check for pdata validation, then calculate id,
> and then second check for pdata->dcdc[id].
> 
> and it is better to use !pointer than (pointer == NULL)
> 
> Signed-off-by: Maninder Singh <maninder1.s@samsung.com>
> ---

The patch basically looks fine, but it feels a bit like needless
churn. The current code is perfectly correct and feels clear
enough to me.

Thanks,
Charles

Re: [PATCH v2] regulator: wm831x-dcdc: Use pointer after NULL check

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 464 bytes --]

On Thu, Jul 02, 2015 at 11:18:49AM +0100, Charles Keepax wrote:
> On Thu, Jul 02, 2015 at 01:27:09PM +0530, Maninder Singh wrote:
> > pdata is used before NULL check, so it looks misleading.

> The patch basically looks fine, but it feels a bit like needless
> churn. The current code is perfectly correct and feels clear
> enough to me.

It's a bug - either the validation shouldn't be there or it needs to be
before the use otherwise it does nothing. 

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH v2] regulator: wm831x-dcdc: Use pointer after NULL check

[Charles Keepax]

On Thu, Jul 02, 2015 at 12:14:56PM +0100, Mark Brown wrote:
> On Thu, Jul 02, 2015 at 11:18:49AM +0100, Charles Keepax wrote:
> > On Thu, Jul 02, 2015 at 01:27:09PM +0530, Maninder Singh wrote:
> > > pdata is used before NULL check, so it looks misleading.
> 
> > The patch basically looks fine, but it feels a bit like needless
> > churn. The current code is perfectly correct and feels clear
> > enough to me.
> 
> It's a bug - either the validation shouldn't be there or it needs to be
> before the use otherwise it does nothing. 

ARRAY_SIZE being implemented with sizeof's means that no actual
dereference takes place. But if we have multiple people thinking
this looks buggy then I guess the code probably isn't clear
enough at the moment.

Acked-by: Charles Keepax <ckeepax@opensource.wolfsonmicro.com>

Thanks,
Charles

Re: [PATCH v2] regulator: wm831x-dcdc: Use pointer after NULL check

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 582 bytes --]

On Thu, Jul 02, 2015 at 12:19:50PM +0100, Charles Keepax wrote:
> On Thu, Jul 02, 2015 at 12:14:56PM +0100, Mark Brown wrote:

> > It's a bug - either the validation shouldn't be there or it needs to be
> > before the use otherwise it does nothing. 

> ARRAY_SIZE being implemented with sizeof's means that no actual
> dereference takes place. But if we have multiple people thinking
> this looks buggy then I guess the code probably isn't clear
> enough at the moment.

Ah, I see.  I'd not actually looked at the patch yet only the changelog
quoted in the discussion.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]