Messages in this thread | | | Date | Wed, 15 Dec 2021 00:34:17 +0100 | From | Ansuel Smith <> | Subject | Re: [net-next PATCH RFC v6 00/16] Add support for qca8k mdio rw in Ethernet packet |
| |
On Tue, Dec 14, 2021 at 11:43:53PM +0100, Ansuel Smith wrote: > Hi, this is ready but require some additional test on a wider userbase. > > The main reason for this is that we notice some routing problem in the > switch and it seems assisted learning is needed. Considering mdio is > quite slow due to the indirect write using this Ethernet alternative way > seems to be quicker. > > The qca8k switch supports a special way to pass mdio read/write request > using specially crafted Ethernet packet. > This works by putting some defined data in the Ethernet header where the > mac source and dst should be placed. The Ethernet type header is set to qca > header and is set to a mdio read/write type. > This is used to communicate to the switch that this is a special packet > and should be parsed differently. > > Currently we use Ethernet packet for > - MIB counter > - mdio read/write configuration > - phy read/write for each port > > Current implementation of this use completion API to wait for the packet > to be processed by the tagger and has a timeout that fallback to the > legacy mdio way and mutex to enforce one transaction at time. > > We now have connect()/disconnect() ops for the tagger. They are used to > allocate priv data in the dsa priv. The header still has to be put in > global include to make it usable by a dsa driver. > They are called when the tag is connect to the dst and the data is freed > using discconect on tagger change. > > (if someone wonder why the bind function is put at in the general setup > function it's because tag is set in the cpu port where the notifier is > still not available and we require the notifier to sen the > tag_proto_connect() event. > > We now have a tag_proto_connect() for the dsa driver used to put > additional data in the tagger priv (that is actually the dsa priv). > This is called using a switch event DSA_NOTIFIER_TAG_PROTO_CONNECT. > Current use for this is adding handler for the Ethernet packet to keep > the tagger code as dumb as possible. > > The tagger priv implement only the handler for the special packet. All the > other stuff is placed in the qca8k_priv and the tagger has to access > it under lock. > > We use the new API from Vladimir to track if the master port is > operational or not. We had to track many thing to reach a usable state. > Checking if the port is UP is not enough and tracking a NETDEV_CHANGE is > also not enough since it use also for other task. The correct way was > both track for interface UP and if a qdisc was assigned to the > interface. That tells us the port (and the tagger indirectly) is ready > to accept and process packet. > > I tested this with multicpu port and with port6 set as the unique port and > it's sad. > It seems they implemented this feature in a bad way and this is only > supported with cpu port0. When cpu port6 is the unique port, the switch > doesn't send ack packet. With multicpu port, packet ack are not duplicated > and only cpu port0 sends them. This is the same for the MIB counter. > For this reason this feature is enabled only when cpu port0 is enabled and > operational. > > Current concern are: > - Any hint about the naming? Is calling this mdio Ethernet correct? > Should we use a more ""standard""/significant name? (considering also > other switch will implement this) > > v6: > - Fix some error in ethtool handler caused by rebase/cleanup > v5: > - Adapt to new API fixes > - Fix a wrong logic for noop > - Add additional lock for master_state change > - Limit mdio Ethernet to cpu port0 (switch limitation) > - Add priority to these special packet > - Move mdio cache to qca8k_priv > v4: > - Remove duplicate patch sent by mistake. > v3: > - Include MIB with Ethernet packet. > - Include phy read/write with Ethernet packet. > - Reorganize code with new API. > - Introuce master tracking by Vladimir > v2: > - Address all suggestion from Vladimir. > Try to generilize this with connect/disconnect function from the > tagger and tag_proto_connect for the driver. > > Ansuel Smith (12): > net: dsa: tag_qca: convert to FIELD macro > net: dsa: tag_qca: move define to include linux/dsa > net: dsa: tag_qca: enable promisc_on_master flag > net: dsa: tag_qca: add define for handling mdio Ethernet packet > net: dsa: tag_qca: add define for handling MIB packet > net: dsa: tag_qca: add support for handling mdio Ethernet and MIB > packet > net: dsa: qca8k: add tracking state of master port > net: dsa: qca8k: add support for mdio read/write in Ethernet packet > net: dsa: qca8k: add support for mib autocast in Ethernet packet > net: dsa: qca8k: add support for phy read/write with mdio Ethernet > net: dsa: qca8k: move page cache to driver priv > net: dsa: qca8k: cache lo and hi for mdio write > > Vladimir Oltean (4): > net: dsa: provide switch operations for tracking the master state > net: dsa: stop updating master MTU from master.c > net: dsa: hold rtnl_mutex when calling dsa_master_{setup,teardown} > net: dsa: replay master state events in > dsa_tree_{setup,teardown}_master > > drivers/net/dsa/qca8k.c | 600 ++++++++++++++++++++++++++++++++++-- > drivers/net/dsa/qca8k.h | 46 ++- > include/linux/dsa/tag_qca.h | 79 +++++ > include/net/dsa.h | 17 + > net/dsa/dsa2.c | 81 ++++- > net/dsa/dsa_priv.h | 13 + > net/dsa/master.c | 29 +- > net/dsa/slave.c | 32 ++ > net/dsa/switch.c | 15 + > net/dsa/tag_qca.c | 81 +++-- > 10 files changed, 901 insertions(+), 92 deletions(-) > create mode 100644 include/linux/dsa/tag_qca.h > > -- > 2.33.1 >
I just tested if the Documentation is correct about this alternative way being able to read/write 16byte of data at times (instead of 4).
I confirm this work but I need to understand how and if this can be used. I can see I should use the regmap bulk api... But I assume I should change the val_bits. (think that is not acceptable if regmap is already init and would be problematic for the fallback...)
Wonder if I should register a separate regmap for eth and use that... (and create an helper to switch between the mdio and the ethernet one?)
This would be very useful for fib read/write that require 4 read/write to put data in the db. (1 lock, 1 packet instead of 4 lock 4 packet)
Would love any hint if we have other way to handle this but I think the double regmap and the helper seems to be the cleaner way for this.
(obviously this will come later and won't be part of this already big patch)
-- Ansuel
| |