Re: [PATCH 1/7] dsa: marvell: Provide per device information about max frame size

[Vladimir Oltean]

On Fri, Mar 10, 2023 at 12:25:59PM +0000, Russell King (Oracle) wrote:
> This is similar analysis to what I did for a previous patch set, and
> came to the conclusion that we need code in the driver to validate
> that the addition of these values is in fact correct. See my previous
> reviews and my recommendations on how to structure these patch sets,
> so we as reviewers don't _have_ to go to this level of verification.

Ok, I haven't read other patches or comments except for 1/7 yet.

> > I guess I will have to fix this now, since you haven't done it.
> 
> I'm sorry, but why is this Lukasz's problem to fix? If it's broken today
> when using mv88e6xxx with this PHY, and Lukasz doesn't have this PHY,
> why does Lukasz have to solve this?

Well, in principle no one has to solve it. It would be good to not move
around broken code if we know it's broken, is what I'm saying. This is
because eventually, someone who *is* affected *will* want to fix it, and
that fix will conflict with the refactoring. Lukasz would have had the
interest in fixing it because he's touching that code. Again, I will do
this when I find the time.

> > > +	/* Max Frame Size.
> > > +	 * This value corresponds to the memory allocated in switch internal
> > > +	 * memory to store single frame.
> > > +	 */
> > 
> > What is the source of this definition?
> > 
> > I'm asking because I know of other switches where the internal memory
> > allocation scheme has nothing to do with the frame size. Instead, there
> > are SRAM cells of fixed and small size (say 60 octets) chained together.
> 
> The switch documentation only really talks about maximum frame sizes
> that the switch can handle, with a few bits that configure what the
> maximum frame size is. We also know how large the SRAM is, but how
> the SRAM is allocated to packets is for Marvell engineers to know
> and not us mere mortals.
> 
> So, the base definition for this is the information provided in the
> switch documentation.

Agree with the "mere mortals" comment. I was trying to suggest that the
given definition tries to make it appear that we know more about the
switch internal memory allocation than we really do. "This value
corresponds to the memory allocated in switch internal memory to store
single frame" -> how do we know that it corresponds?