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

[Lukasz Majewski]

Hi Russell,

> On Wed, Jan 25, 2023 at 12:24:12PM +0100, Lukasz Majewski wrote:
> > Hi,
> >   
> > > Hi Russell,
> > >   
> > > > On Fri, Jan 06, 2023 at 11:16:49AM +0100, Lukasz Majewski
> > > > wrote:    
> > > > > Different Marvell DSA switches support different size of max
> > > > > frame bytes to be sent. This value corresponds to the memory
> > > > > allocated in switch to store single frame.
> > > > > 
> > > > > For example mv88e6185 supports max 1632 bytes, which is now
> > > > > in-driver standard value. On the other hand - mv88e6250
> > > > > supports 2048 bytes. To be more interresting - devices
> > > > > supporting jumbo frames - use yet another value (10240 bytes)
> > > > > 
> > > > > As this value is internal and may be different for each
> > > > > switch IC, new entry in struct mv88e6xxx_info has been added
> > > > > to store it.
> > > > > 
> > > > > This commit doesn't change the code functionality - it just
> > > > > provides the max frame size value explicitly - up till now it
> > > > > has been assigned depending on the callback provided by the
> > > > > IC driver (e.g. .set_max_frame_size, .port_set_jumbo_size).
> > > > >    
> > > > 
> > > > I don't think this patch is correct.
> > > > 
> > > > One of the things that mv88e6xxx_setup_port() does when
> > > > initialising each port is:
> > > > 
> > > >         if (chip->info->ops->port_set_jumbo_size) {
> > > >                 err = chip->info->ops->port_set_jumbo_size(chip,
> > > > port, 10218); if (err)
> > > >                         return err;
> > > >         }
> > > > 
> > > > There is one implementation of this, which is
> > > > mv88e6165_port_set_jumbo_size() and that has the effect of
> > > > setting port register 8 to the largest size. So any chip that
> > > > supports the port_set_jumbo_size() method will be programmed on
> > > > initialisation to support this larger size.
> > > > 
> > > > However, you seem to be listing e.g. the 88e6190 (if I'm
> > > > interpreting the horrid mv88e6xxx_table changes correctly)    
> > > 
> > > Those changes were requested by the community. Previous versions
> > > of this patch were just changing things to allow correct
> > > operation of the switch ICs on which I do work (i.e. 88e6020 and
> > > 88e6071).
> > > 
> > > And yes, for 88e6190 the max_frame_size = 10240, but (by mistake)
> > > the same value was not updated for 88e6190X.
> > > 
> > > The question is - how shall I proceed? 
> > > 
> > > After the discussion about this code - it looks like approach
> > > from v3 [1] seems to be the most non-intrusive for other ICs.
> > >   
> > 
> > I would appreciate _any_ hints on how shall I proceed to prepare
> > those patches, so the community will accept them...  
> 

Thanks for a very detailed reply.

> What I'm concerned about, and why I replied, is that setting the
> devices to have a max frame size of 1522 when we program them to use
> a larger frame size means we break those switches for normal sized
> packets.
> 
> The current logic in mv88e6xxx_get_max_mtu() is:
> 
> 	If the chip implements port_set_jumbo_size, then packet sizes
> of up to 10240 are supported.
> 	(ops: 6131, 6141, 6171, 6172, 6175, 6176, 6190, 6190x, 6240,
> 6320, 6321, 6341, 6350, 6351, 6352, 6390, 6390x, 6393x)
> 	If the chip implements set_max_frame_size, then packet sizes
> of up to 1632 are supported.
> 	(ops: 6085, 6095, 6097, 6123, 6161, 6185)
> 	Otherwise, packets of up to 1522 are supported.
> 
> Now, going through the patch, I see:
> 
> 	88e6085 has 10240 but currently has 1632
> 	88e6095 has 1632 (no change)
> 	88e6097 has 1632 (no change)
> 	88e6123 has 10240 but currently has 1632
> 	88e6131 has 10240 (no change)
> 	88e6141 has 10240 (no change)
> 	88e6161 has 1632 but currently has 10240
> 	88e6165 has 1632 but currently has 1522
> 	88e6171 has 1522 but currently has 10240
> 	88e6172 has 10240 (no change)
> 	88e6175 has 1632 but currently has 10240
> 	88e6176 has 10240 (no change)
> 	88e6185 has 1632 (no change)
> 	88e6190 has 10240 (no change)
> 	88e6190x has 10240 (no change)
> 	88e6191 has 10240 but currently has 1522
> 	88e6191x has 1522 but currently has 10240
> 	88e6193x has 1522 but currently has 10240
> 	88e6220 has 2048 but currently has 1522
> 	88e6240 has 10240 (no change)
> 	88e6250 has 2048 but currently has 1522
> 	88e6290 has 10240 but currently has 1522
> 	88e6320 has 10240 (no change)
> 	88e6321 has 10240 (no change)
> 	88e6341 has 10240 (no change)
> 	88e6350 has 10240 (no change)
> 	88e6351 has 10240 (no change)
> 	88e6352 has 10240 (no change)
> 	88e6390 has 1522 but currently has 10240
> 	88e6390x has 1522 but currently has 10240
> 	88e6393x has 1522 but currently has 10240
> 
> My point is that based on the above, there's an awful lot of changes
> that this one patch brings, and I'm not sure many of them are
> intended.

As I only have access to mv88e60{20|71} SoCs I had to base on the code
to deduce which max frame is supported.

> 
> All the ones with "but currently has 10240", it seems they implement
> port_set_jumbo_size() which, although the switch may default to a
> smaller frame size, we configure it to be higher. Maybe these don't
> implement the field that configures those? Maybe your patch is wrong?
> I don't know.
> 
> Similarly for the ones with "but currently has 1632", it seems they
> implement set_max_frame_size(), but this is only called via
> mv88e6xxx_change_mtu(), and I haven't worked out whether that will
> be called during initialisation by the networking layer.
> 
> Now, what really concerns me is the difficulty in making this change.
> As we can see from the above, there's a lot of changes going on here,
> and it's not obvious which are intentional and which may be bugs.

I'm also quite surprised about the impact of this patch.

> 
> So, I think it would be far better to introduce the "max_frame_size"
> field using the existing values, and then verify that value during
> initialisation time for every entry in mv88e6xxx_table[] using the
> rules that mv88e6xxx_get_max_mtu() was using. Boot that kernel, and
> have it run that verification, and state that's what's happened and
> was successful in the commit message.
> 
> In the next commit, change mv88e6xxx_get_max_mtu() to use those
> verified values and remove the verification code.
> 
> Then in the following commit, update the "max_frame_size" values with
> the changes you intend to make.
> 
> Then, we can (a) have confidence that each of the new members were
> properly initialised, and (b) we can also see what changes you're
> intentionally making.
> 

If I understood you correctly - the approach would be to "simulate" and
obtain each max_frame_size assigned in mv88e6xxx_get_max_mtu() to be
sure that we do preserve current (buggy or not) behaviour.

Then I shall just add those two extra values for mv88e60{20|71}.

> Right now, given that at least two of the "max_frame_size" values are
> wrong in this patch, I think we can say for certain that we've proven
> that trying to introduce this new member and use it in a single patch
> is too error prone.

I do agree here - the code for getting max frame size for each
supported SoC is quite convoluted and difficult to deduce the right
value from the outset.

> 
> Thanks.
> 




Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Erika Unter
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@xxxxxxx

Attachment: pgpey_7BjwLMg.pgp
Description: OpenPGP digital signature