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

[Köry Maincent]

On Tue, 16 Jan 2024 14:18:04 +0100
Andrew Lunn <andrew@xxxxxxx> 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