Received-SPF: pass (google.com: domain of linux-kernel+bounces-27456-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Date: Tue, 16 Jan 2024 15:12:05 +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>,
 Oleksij Rempel <o.rempel@pengutronix.de>, Thomas Petazzoni
 <thomas.petazzoni@bootlin.com>, netdev@vger.kernel.org,
 linux-kernel@vger.kernel.org, linux-doc@vger.kernel.org,
 devicetree@vger.kernel.org, Dent Project <dentproject@linuxfoundation.org>
Subject: Re: [PATCH net-next v2 8/8] net: pse-pd: Add PD692x0 PSE controller
 driver
Message-ID: <20240116151205.0bf44181@kmaincent-XPS-13-7390>
In-Reply-To: <64f30166-58cc-409d-ba5b-9ea3fb8ead88@lunn.ch>
References: <20231201-feature_poe-v2-0-56d8cac607fa@bootlin.com>
	<20231201-feature_poe-v2-8-56d8cac607fa@bootlin.com>
	<639c5222-043f-4e27-9efa-ce2a1d73eaba@lunn.ch>
	<20240116104949.12708cd5@kmaincent-XPS-13-7390>
	<64f30166-58cc-409d-ba5b-9ea3fb8ead88@lunn.ch>
Organization: bootlin
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: quoted-printable

On Tue, 16 Jan 2024 14:18:04 +0100
Andrew Lunn <andrew@lunn.ch> wrote:

> >  =20
> > > > +static int pd692x0_fw_get_next_line(const u8 *data,
> > > > +				    char *line, size_t size)
> > > > +{
> > > > +	size_t line_size;
> > > > +	int i;
> > > > +
> > > > +	line_size =3D min_t(size_t, size,
> > > > (size_t)PD692X0_FW_LINE_MAX_SZ); +
> > > > +	memset(line, 0, PD692X0_FW_LINE_MAX_SZ);
> > > > +	for (i =3D 0; i < line_size - 1; i++) {
> > > > +		if (*data =3D=3D '\r' && *(data + 1) =3D=3D '\n') {
> > > > +			line[i] =3D '\r';
> > > > +			line[i + 1] =3D '\n';
> > > > +			return i + 2;
> > > > +		}   =20
> > >=20
> > > Does the Vendor Documentation indicate Windoze line endings will
> > > always be used? Motorola SREC allow both Windows or rest of the world
> > > line endings to be used.  =20
> >=20
> > All the firmware lines end with "\r\n" but indeed it is not specifically
> > written that the firmware content would follow this. IMHO it is implicit
> > that it would be the case as all i2c messages use this line termination.
> > Do you prefer that I add support to the world line endings possibility?=
  =20
>=20
> No need, just hack an SREC file, and test the parser does not explode
> with an opps, and you get an sensible error message about the firmware
> being corrupt. I would not be too surprised if there are some mail
> systems still out there which might convert the line ending.

Ok I will do so.

>=20
> > > > +static enum fw_upload_err pd692x0_fw_poll_complete(struct fw_upload
> > > > *fwl) +{
> > > > +	struct pd692x0_priv *priv =3D fwl->dd_handle;
> > > > +	const struct i2c_client *client =3D priv->client;
> > > > +	struct pd692x0_msg_ver ver;
> > > > +	int ret;
> > > > +
> > > > +	priv->fw_state =3D PD692X0_FW_COMPLETE;
> > > > +
> > > > +	ret =3D pd692x0_fw_reset(client);
> > > > +	if (ret)
> > > > +		return ret;
> > > > +
> > > > +	ver =3D pd692x0_get_sw_version(priv);
> > > > +	if (ver.maj_sw_ver !=3D PD692X0_FW_MAJ_VER) {   =20
> > >=20
> > > That is probably too strong a condition. You need to allow firmware
> > > upgrades, etc. Does it need to be an exact match, or would < be
> > > enough? =20
> >=20
> > The major version is not compatible with the last one, the i2c messages
> > content changed. I supposed a change in major version would imply a cha=
nge
> > in the i2c messages content and would need a driver update that's why I
> > used this strong condition. =20
>=20
> Do you know the next major version will change the message contents?

No.

> Is this documented somewhere? If so add a comment. Otherwise, i would
> allow higher major versions. When the vendor breaks backwards
> compatibility, its going to need code changes anyway, and at that
> point the test can be made more strict.
>=20
> We try to make vendors not make firmware ABI breaking changes, and we
> have pushed back against a number of vendors who do. So i think its
> best we assume they won't break the ABI.

Alright, thanks!

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