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;
Date:   Fri, 9 Sep 2022 23:00:19 +0200
From:   Jonathan =?utf-8?Q?Neusch=C3=A4fer?= <j.neuschaefer@gmx.net>
To:     Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc:     Jonathan =?utf-8?Q?Neusch=C3=A4fer?= <j.neuschaefer@gmx.net>,
        Linus Walleij <linus.walleij@linaro.org>,
        openbmc@lists.ozlabs.org, linux-gpio@vger.kernel.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH v1 1/1] pinctrl: wpcm450: Correct the fwnode_irq_get()
 return value check
Message-ID: <YxupY5MGWddiY2mq@probook>
References: <20220905191408.73794-1-andriy.shevchenko@linux.intel.com>
 <YxkHaBKtiO9zVuKM@probook>
 <Yxm9fB/5IJS3MXGu@smile.fi.intel.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
        protocol="application/pgp-signature"; boundary="T4S0blP9/51qK21U"
Content-Disposition: inline
In-Reply-To: <Yxm9fB/5IJS3MXGu@smile.fi.intel.com>
Precedence: bulk


--T4S0blP9/51qK21U
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

Hello,


On Thu, Sep 08, 2022 at 01:01:32PM +0300, Andy Shevchenko wrote:
> On Wed, Sep 07, 2022 at 11:04:40PM +0200, Jonathan Neusch=C3=A4fer wrote:
> > On Mon, Sep 05, 2022 at 10:14:08PM +0300, Andy Shevchenko wrote:
> > > fwnode_irq_get() may return all possible signed values, such as Linux
> > > error code. Fix the code to handle this properly.
> >=20
> > It would be good to note explicitly here what a return value of zero
> > means, i.e., as the documentation of of_irq_get says, "IRQ mapping
> > failure", and why it should result in skipping this IRQ.
>=20
> Not that I'm fun of duplicating documentation in the commit message,
> but it won't take much in this case.

My problem with the description is that handling "all possible signed
values" is fairly meaningless: The code arguably did that already, it
did *something* for every possible value. The significant change of
your patch is that the value zero is handled differently.

IOW, what I miss is something along the lines of: "fwnode_irq_get can
return zero to indicate some errors. Handle this case like other errors."

> ...
>=20
> > >  		for (i =3D 0; i < WPCM450_NUM_GPIO_IRQS; i++) {
> > > -			int irq =3D fwnode_irq_get(child, i);
> > > +			int irq;
> > > =20
> > > +			irq =3D fwnode_irq_get(child, i);
>=20
> > (Unneccesary churn, but I'll allow it)
>=20
> The point here is to see that we actually check something that we just got
> from somewhere else. It's slightly better for reading and maintaining the
> code as I explained in [1].
>=20
> And there is a difference to the cases like
>=20
> static int foo(struct platform_device *pdev, ...)
> {
> 	struct device *dev =3D &pdev->dev;
> 	...
> }
>=20
> when we know ahead that if pdev is NULL, something is _so_ wrong that
> it's not even our issue.
>=20
> [1]: https://lore.kernel.org/lkml/CAHp75Vda5KX5pVrNeueQEODoEy405eTb9SYJtt=
s-Lm9jMNocHQ@mail.gmail.com/

Ok, fair enough.


>=20
> > >  			if (irq < 0)
> > >  				break;
> > > +			if (!irq)
> > > +				continue;
> >=20
> > Since irq =3D=3D 0 seems to be an error condition, the following seems =
fine
> > to me, and simpler:
> >=20
> > -			if (irq < 0)
> > +			if (irq <=3D 0)
> >  				break;
>=20
> Not sure it's the same by two reasons:
>  1) break !=3D continue;

Right, hence why I asked for reasoning why zero should be handled
the way you propose to handle it.

>  2) we might need to convert 0 to error if we ever go to report this

Good point.

>=20
> So, to me mapping error shouldn't be fatal to continue, but I would
> like to hear your interpretation since you know this case much better
> than me.

However: In wpcm450_gpio_register, there is currently no reporting for
mapping errors in this loop.

I'm fine with either break or continue.


Thanks,
Jonathan

--T4S0blP9/51qK21U
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQIzBAABCgAdFiEEvHAHGBBjQPVy+qvDCDBEmo7zX9sFAmMbqUMACgkQCDBEmo7z
X9vPTg//W68gSOfDJBQZ2/UeuyNZ5V9lnHuvyi6tBr+qM77qU9qZoqss1lcSbVq9
vhtvGEPdFIvPKb61wU0qYM70Kr3TXHpsFHK9lUo25WZybUq32OJMFZYY0go0IQ2X
FZ8VpUlb7lMOy0jwPu6ARQhyuVN1I5Cr6EnzWs8bhxAIJANqAMeqD85hsXmel3O8
4IM+oa+26i5otyGZLeLQPAJO1ABWAExemevWsmfsghTcbSt1fTc34dbnyCCXrAE7
d09YYaIu9SVHt584aIDMMdGwpXDxF1a4ObaRXvv6atMC07Ezs4zU12hATe+GVGx7
0MCD+bh4RxjSCrAAKRDItN1nJqMFhZTTB0ugmneDAKzORoiV3iDCxCsABxsH1ds4
u+yI+JAulbWdHsUOXfuns/qQqaTDPbLYdXE3asunOIRNlvTw+DwzuyJ5sIJfBclJ
6FuIyv+DjjcEt9YRlKtZzkgMxGw0Z13ZvlVlZb9RS9A176xFO04J1m22Vy2Kj+TF
5UouBkVh0OU9zkLHa+KRmcT88uvVgzK6YqvATSFBvcYGSV2r1Y3v2HlRWCrNxygy
vqsgWN/d9dqeVLyj5vVUz3ksveKlFEJ8mhPAwHGxSiQUyOLQ/s6YS88GiMioBlFG
hRoetTp8LXl9SbQyqmuuVzJsaFocIQ271MtXjOIWJou5+k8YN+o=
=HFeK
-----END PGP SIGNATURE-----

--T4S0blP9/51qK21U--