Messages in this thread | | | Date | Mon, 30 Jan 2023 12:57:31 +0100 | From | Lukasz Majewski <> | Subject | Re: [PATCH v4 1/3] dsa: marvell: Provide per device information about max frame size |
| |
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@denx.de [unhandled content-type:application/pgp-signature] | |