Re: [PATCH net-next v2 2/2] net: ethtool: Add support for writing firmware blocks using EPL payload

[Andrew Lunn <andrew@lunn.ch>]

On Wed, Sep 11, 2024 at 08:32:34AM +0100, Simon Horman wrote:
> + Andrew Lunn
> 
> On Tue, Sep 10, 2024 at 12:02:17PM +0300, Danielle Ratson wrote:
> > In the CMIS specification for pluggable modules, LPL (Local Payload) and
> > EPL (Extended Payload) are two types of data payloads used for managing
> > various functions and features of the module.
> > 
> > EPL payloads are used for more complex and extensive management
> > functions that require a larger amount of data, so writing firmware
> > blocks using EPL is much more efficient.
> > 
> > Currently, only LPL payload is supported for writing firmware blocks to
> > the module.
> > 
> > Add support for writing firmware block using EPL payload, both to
> > support modules that supports only EPL write mechanism, and to optimize
> > the flashing process of modules that support LPL and EPL.
> > 
> > Signed-off-by: Danielle Ratson <danieller@nvidia.com>
> > Reviewed-by: Petr Machata <petrm@nvidia.com>
> > ---
> > 
> > Notes:
> >     v2:
> >     	* Initialize the variable 'bytes_written' before the first
> >     	  iteration.
> 
> Hi Danielle,
> 
> Thanks for the update. From a doing-what-it-says-on-the-wrapper
> perspective, this looks good to me.
> 
> Reviewed-by: Simon Horman <horms@kernel.org>
> 
> I do note that there were some questions from Andrew Lunn (CCed) in v1
> regarding the size of transfers over the bus. I see that you responded to
> that. Thanks! But I do wonder if he has any further comments.

I do wounder where the speedup comes from.

> The LPL contains ap to 128 bytes, and the EPL up to 2048

Are 128 consecutive 16 byte transfers per block really faster than 8
consecutive transfers per block? If you have an efficient I2C master,
both should be doing 400Kbp. If the master is inefficient, you end up
with the same amount of dead time on the wire.

And does the standard really say you can fragment the block at the I2C
layer?

I suspect in the end we will have an API between the core and the
driver to ask it what size of block it actually supports. But we can
probably add that when we need it.

	Andrew