Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20220304173256.39059-1-andriy.shevchenko@linux.intel.com> <8afcf0b81f78ffdda8bcac5f0fd1d4c40dc4f8d6.camel@gmail.com>
In-Reply-To: <8afcf0b81f78ffdda8bcac5f0fd1d4c40dc4f8d6.camel@gmail.com>
From:   Andy Shevchenko <andy.shevchenko@gmail.com>
Date:   Sat, 5 Mar 2022 23:46:46 +0200
Message-ID: <CAHp75Vd7QcujZKEtx2_PT-QxoAh7LAykkV3k_OOO5Q0tRQKrBA@mail.gmail.com>
Subject: Re: [PATCH v2 1/1] device property: Allow error pointer to be passed
 to fwnode APIs
To:     =?UTF-8?B?TnVubyBTw6E=?= <noname.nuno@gmail.com>
Cc:     Andy Shevchenko <andriy.shevchenko@linux.intel.com>,
        "Rafael J. Wysocki" <rafael.j.wysocki@intel.com>,
        ACPI Devel Maling List <linux-acpi@vger.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        Daniel Scally <djrscally@gmail.com>,
        Heikki Krogerus <heikki.krogerus@linux.intel.com>,
        Sakari Ailus <sakari.ailus@linux.intel.com>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        "Rafael J. Wysocki" <rafael@kernel.org>,
        Len Brown <lenb@kernel.org>,
        =?UTF-8?B?TnVubyBTw6E=?= <nuno.sa@analog.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Sat, Mar 5, 2022 at 10:28 PM Nuno S=C3=A1 <noname.nuno@gmail.com> wrote:
>
> On Fri, 2022-03-04 at 19:32 +0200, Andy Shevchenko wrote:
> > Some of the fwnode APIs might return an error pointer instead of NULL
> > or valid fwnode handle. The result of such API call may be considered
> > optional and hence the test for it is usually done in a form of
> >
> >         fwnode =3D fwnode_find_reference(...);
> >         if (IS_ERR_OR_NULL(fwnode))
> >                 ...error handling...
> >
> > Nevertheless the resulting fwnode may have bumped reference count and
> > hence caller of the above API is obliged to call fwnode_handle_put().
> > Since fwnode may be not valid either as NULL or error pointer the
> > check
> > has to be performed there. This approach uglifies the code and adds
> > a point of making a mistake, i.e. forgetting about error point case.
> >
> > To prevent this allow error pointer to be passed to the fwnode APIs.

...

> >         ret =3D fwnode_call_int_op(fwnode, property_read_string_array,
> > propname,
> >                                  val, nval);
> > -       if (ret =3D=3D -EINVAL && !IS_ERR_OR_NULL(fwnode) &&
> > -           !IS_ERR_OR_NULL(fwnode->secondary))
> > +       if (ret =3D=3D -EINVAL && !IS_ERR_OR_NULL(fwnode->secondary))
> >                 ret =3D fwnode_call_int_op(fwnode->secondary,
> >                                          property_read_string_array,
>
> Isn't !IS_ERR_OR_NULL(fwnode->secondary)) redundant? AFAIU,
> fwnode_call_int_op() will already check the fwnode and only call the op
> if the pointer is valid...

It will shadow the error code, but it seems currently it's the same error c=
ode.
So, the question here is if we hide something important with that change.

I dunno what is the best approach here (esp. taking into account that
this is a fix), but ideally we should open code those macros to avoid
double test for fwnode being valid. Because it seems that validation
of fwnode and validation of the operation of the fwnode are orthogonal
and here we mix them. I made this way for the sake of easier
backporting and kicking off a discussion (as you already did). TL;DR:
I think the introduction of the macros was a controversial move, for
which I see pros and cons.


--=20
With Best Regards,
Andy Shevchenko