Re: [PATCH v8 03/13] squash! max9286: Fix cleanup path from GPIO powerdown

From: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
To: Geert Uytterhoeven <geert@linux-m68k.org>,
	Jacopo Mondi <jacopo@jmondi.org>
Cc: Linux-Renesas <linux-renesas-soc@vger.kernel.org>,
	"Laurent Pinchart" <laurent.pinchart@ideasonboard.com>,
	"Niklas Söderlund" <niklas.soderlund@ragnatech.se>,
	"Hyun Kwon" <hyunk@xilinx.com>,
	"Manivannan Sadhasivam" <manivannan.sadhasivam@linaro.org>
Subject: Re: [PATCH v8 03/13] squash! max9286: Fix cleanup path from GPIO powerdown
Date: Fri, 10 Apr 2020 08:41:18 +0100	[thread overview]
Message-ID: <58c87cbf-cc94-996f-3abc-2c13ef9c6eb1@ideasonboard.com> (raw)
In-Reply-To: <CAMuHMdUYk92CX7NDNA3QEFG7fPC0Gwu702A9RU26fQGpBu=ugA@mail.gmail.com>

On 10/04/2020 08:34, Geert Uytterhoeven wrote:
> Hi Jacopo,
> 
> On Thu, Apr 9, 2020 at 7:07 PM Jacopo Mondi <jacopo@jmondi.org> wrote:
>> On Thu, Apr 09, 2020 at 01:11:52PM +0100, Kieran Bingham wrote:
>>>  - Fix up cleanup path from GPIO PowerDown registration
>>> ---
>>>  drivers/media/i2c/max9286.c | 14 ++++++++------
>>>  1 file changed, 8 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
>>> index 0a43137b8112..cc99740b34c5 100644
>>> --- a/drivers/media/i2c/max9286.c
>>> +++ b/drivers/media/i2c/max9286.c
>>> @@ -1171,8 +1171,10 @@ static int max9286_probe(struct i2c_client *client)
>>>
>>>       priv->gpiod_pwdn = devm_gpiod_get_optional(&client->dev, "enable",
>>>                                                  GPIOD_OUT_HIGH);
>>> -     if (IS_ERR(priv->gpiod_pwdn))
>>> -             return PTR_ERR(priv->gpiod_pwdn);
>>> +     if (IS_ERR(priv->gpiod_pwdn)) {
>>> +             ret = PTR_ERR(priv->gpiod_pwdn);
>>> +             goto err_cleanup_dt;
>>> +     }
>>>
>>>       gpiod_set_consumer_name(priv->gpiod_pwdn, "max9286-pwdn");
>>>       gpiod_set_value_cansleep(priv->gpiod_pwdn, 1);
>>
>> As we get_optional(), shouldn't this be protected by an
>>         if (priv->gpiod_pwdn)
>>
>>  ?
> 
> When passed a NULL desc, validate_desc() just returns 0, without printing
> an error message, so both functions become no-ops.

Aha! I knew I'd tested that code path :-)

Thanks for highlighting that, I missed it just 10 minutes ago when I
rechecked because:

 	VALIDATE_DESC(desc);

has a *HIDDEN RETURN* dammit:

	if (__valid <= 0) \
		return __valid; \
	} while (0)

Honestly - I thought we didn't do that in the kernel for exactly this
reason. Grrrrrrrrrr ... oh and grrrrr again !

They could have at least named it:

 VALIDATE_DESC_OR_RETURN(desc)

> 
> Gr{oetje,eeting}s,
> 
>                         Geert
> 