Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:8 as permitted sender) client-ip=2620:137:e000::3:8;
Date:   Wed, 22 Nov 2023 18:16:47 +0100
From:   =?UTF-8?B?S8O2cnk=?= Maincent <kory.maincent@bootlin.com>
To:     Andrew Lunn <andrew@lunn.ch>
Cc:     "David S. Miller" <davem@davemloft.net>,
        Eric Dumazet <edumazet@google.com>,
        Jakub Kicinski <kuba@kernel.org>,
        Paolo Abeni <pabeni@redhat.com>,
        Jonathan Corbet <corbet@lwn.net>,
        Luis Chamberlain <mcgrof@kernel.org>,
        Russ Weight <russ.weight@linux.dev>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        "Rafael J. Wysocki" <rafael@kernel.org>,
        Rob Herring <robh+dt@kernel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        Conor Dooley <conor+dt@kernel.org>,
        Thomas Petazzoni <thomas.petazzoni@bootlin.com>,
        netdev@vger.kernel.org, linux-kernel@vger.kernel.org,
        linux-doc@vger.kernel.org, devicetree@vger.kernel.org
Subject: Re: [PATCH net-next 9/9] net: pse-pd: Add PD692x0 PSE controller
 driver
Message-ID: <20231122181647.06d9c3c9@kmaincent-XPS-13-7390>
In-Reply-To: <04f59e77-134b-45b2-8759-84b8e22c30d5@lunn.ch>
References: <20231116-feature_poe-v1-0-be48044bf249@bootlin.com>
        <20231116-feature_poe-v1-9-be48044bf249@bootlin.com>
        <2ff8bea5-5972-4d1a-a692-34ad27b05446@lunn.ch>
        <20231122171112.59370d21@kmaincent-XPS-13-7390>
        <04f59e77-134b-45b2-8759-84b8e22c30d5@lunn.ch>
Organization: bootlin
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Wed, 22 Nov 2023 17:54:53 +0100
Andrew Lunn <andrew@lunn.ch> wrote:

> > > > +static int pd692x0_sendrecv_msg(struct pd692x0_priv *priv,
> > > > +				struct pd692x0_msg *msg,
> > > > +				struct pd692x0_msg_content *buf)
> > > > +{
> > > > +	struct device *dev =3D &priv->client->dev;
> > > > +	int ret;
> > > > +
> > > > +	ret =3D pd692x0_send_msg(priv, msg);
> > > > +	if (ret)
> > > > +		return ret;
> > > > +
> > > > +	ret =3D pd692x0_recv_msg(priv, msg, buf);   =20
> > >=20
> > > So this function takes at least 10 seconds? =20
> >=20
> > No, on normal communication it takes a bit more than 30ms. =20
>=20
> So i think the first step is to refactor this code to make it clear
> what the normal path is, and what the exception path is, and the
> timing of each.

Ok I will try to refactor it to makes it more readable.

> > > > +	msg.content.sub[2] =3D id;
> > > > +	ret =3D pd692x0_sendrecv_msg(priv, &msg, &buf);   =20
> > >=20
> > > So this is also 10 seconds?=20
> > >=20
> > > Given its name, it looks like this is called via ethtool? Is the
> > > ethtool core holding RTNL? It is generally considered bad to hold RTN=
L for
> > > that long. =20
> >=20
> > Yes it is holding RTNL lock. Should I consider another behavior in case=
 of
> > communication loss to not holding RTNL lock so long? =20
>=20
> How often does it happen? On the scale of its a theoretical
> possibility, through to it happens every N calls? Also, does it happen
> on cold boot and reboot?
>=20
> If its never supposed to happen, i would keep holding RTNL, and add a
> pr_warn() that the PSE has crashed and burned, waiting for it to
> reboot. If this is likely to happen on the first communication with
> the device, we might want to do a dummy transfer during probe to get
> is synchronized before we start using it with the RTNL held.

I would say it never supposed to happen.
I never faced the issue playing with the controller. The first communicatio=
n is
a simple i2c_master_recv of the controller status without entering the
pd692x0_sendrecv_msg function, therefore it won't be an issue.

Another solution could be to raise a flag if I enter in communication loss =
and
release the rtnlock. We would lock again the rtnl when the flags got disabl=
ed.
The controler won't be accessible until the flag is disabled.

Regards,
--=20
K=C3=B6ry Maincent, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com