On 10/04/2020 08:34, Geert Uytterhoeven wrote:
> Hi Jacopo,
>
> On Thu, Apr 9, 2020 at 7:07 PM Jacopo Mondi <jacopo@xxxxxxxxxx> 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
>
