netdev - Re: [PATCH net-next v2 8/8] net: pse-pd: Add PD692x0 PSE controller driver

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]

Message-ID: <20240116151205.0bf44181@kmaincent-XPS-13-7390>
Date: Tue, 16 Jan 2024 15:12:05 +0100
From: Köry Maincent <kory.maincent@...tlin.com>
To: Andrew Lunn <andrew@...n.ch>
Cc: "David S. Miller" <davem@...emloft.net>, Eric Dumazet
 <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>, Paolo Abeni
 <pabeni@...hat.com>, Jonathan Corbet <corbet@....net>, Luis Chamberlain
 <mcgrof@...nel.org>, Russ Weight <russ.weight@...ux.dev>, Greg
 Kroah-Hartman <gregkh@...uxfoundation.org>, "Rafael J. Wysocki"
 <rafael@...nel.org>, Rob Herring <robh+dt@...nel.org>, Krzysztof Kozlowski
 <krzysztof.kozlowski+dt@...aro.org>, Conor Dooley <conor+dt@...nel.org>,
 Oleksij Rempel <o.rempel@...gutronix.de>, Thomas Petazzoni
 <thomas.petazzoni@...tlin.com>, netdev@...r.kernel.org,
 linux-kernel@...r.kernel.org, linux-doc@...r.kernel.org,
 devicetree@...r.kernel.org, Dent Project <dentproject@...uxfoundation.org>
Subject: Re: [PATCH net-next v2 8/8] net: pse-pd: Add PD692x0 PSE controller
 driver

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

> >   
> > > > +static int pd692x0_fw_get_next_line(const u8 *data,
> > > > +				    char *line, size_t size)
> > > > +{
> > > > +	size_t line_size;
> > > > +	int i;
> > > > +
> > > > +	line_size = min_t(size_t, size,
> > > > (size_t)PD692X0_FW_LINE_MAX_SZ); +
> > > > +	memset(line, 0, PD692X0_FW_LINE_MAX_SZ);
> > > > +	for (i = 0; i < line_size - 1; i++) {
> > > > +		if (*data == '\r' && *(data + 1) == '\n') {
> > > > +			line[i] = '\r';
> > > > +			line[i + 1] = '\n';
> > > > +			return i + 2;
> > > > +		}    
> > > 
> > > 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.   
> > 
> > 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?   
> 
> 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.

> 
> > > > +static enum fw_upload_err pd692x0_fw_poll_complete(struct fw_upload
> > > > *fwl) +{
> > > > +	struct pd692x0_priv *priv = fwl->dd_handle;
> > > > +	const struct i2c_client *client = priv->client;
> > > > +	struct pd692x0_msg_ver ver;
> > > > +	int ret;
> > > > +
> > > > +	priv->fw_state = PD692X0_FW_COMPLETE;
> > > > +
> > > > +	ret = pd692x0_fw_reset(client);
> > > > +	if (ret)
> > > > +		return ret;
> > > > +
> > > > +	ver = pd692x0_get_sw_version(priv);
> > > > +	if (ver.maj_sw_ver != PD692X0_FW_MAJ_VER) {    
> > > 
> > > 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?  
> > 
> > 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 change
> > in the i2c messages content and would need a driver update that's why I
> > used this strong condition.  
> 
> 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.
> 
> 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!

-- 
Köry Maincent, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com