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 <[email protected]>
---
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
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 <[email protected]>
> ---
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
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 <[email protected]>
>> ---
>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?+???????+%?????ݶ??w??{.n?+????{??G?????{ay? ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?
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 <[email protected]>
> >> ---
>
> >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
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.
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 <[email protected]>
Thanks,
Charles
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.