linux-kernel - Re: [PATCH] [media] staging/imx: remove confusing IS_ERR_OR

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Tue, 11 Jul 2017 15:23:36 +0200
From:   Arnd Bergmann <arnd@...db.de>
To:     Philipp Zabel <p.zabel@...gutronix.de>
Cc:     Steve Longerbeam <slongerbeam@...il.com>,
        Mauro Carvalho Chehab <mchehab@...nel.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Hans Verkuil <hans.verkuil@...co.com>,
        Marek Vasut <marex@...x.de>,
        Russell King <rmk+kernel@...linux.org.uk>,
        Linux Media Mailing List <linux-media@...r.kernel.org>,
        devel@...verdev.osuosl.org,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] [media] staging/imx: remove confusing IS_ERR_OR_NULL usage

On Thu, Jun 29, 2017 at 11:13 AM, Philipp Zabel <p.zabel@...gutronix.de> wrote:

>> @@ -134,23 +134,26 @@ static void csi_idmac_put_ipu_resources(struct csi_priv *priv)
>>  static int csi_idmac_get_ipu_resources(struct csi_priv *priv)
>>  {
>>       int ch_num, ret;
>> +     struct ipu_smfc *smfc, *idmac_ch;
>
> This should be
>
> +       struct ipuv3_channel *idmac_ch;
> +       struct ipu_smfc *smfc;
>
> instead.

Fixed in v2 now.

>
> ... this changes behaviour:
>
>     imx-media: imx_media_of_parse failed with -17
>     imx-media: probe of capture-subsystem failed with error -17
>
> We must continue to return NULL here if imxsd == -EEXIST:
>
> -               return imxsd;
> +               return PTR_ERR(imxsd) == -EEXIST ? NULL : imxsd;
>
> or change the code where of_parse_subdev is called (from
> imx_media_of_parse, and recursively from of_parse_subdev) to not handle
> the -EEXIST return value as an error.
>
> With those fixed,
>
> Reviewed-by: Philipp Zabel <p.zabel@...gutronix.de>
> Tested-by: Philipp Zabel <p.zabel@...gutronix.de>

I thought about it some more and tried to find a better solution for this
function, which is now a bit different, so I did not add your tags.

Can you have another look at v2? This time, of_parse_subdev separates
the return code from the pointer, which seems less confusing in a function
like that. There are in fact two cases where we return NULL and it's
not clear if the caller should treat that as success or failure. I've left
the current behavior the same but added comments there.

     Arnd