Message ID | 20210119230749.1178874-16-olteanv@gmail.com |
---|---|
State | New |
Headers | show |
Series | tag_8021q for Ocelot switches | expand |
Hi Vladimir,
I love your patch! Perhaps something to improve:
[auto build test WARNING on net-next/master]
url: https://github.com/0day-ci/linux/commits/Vladimir-Oltean/tag_8021q-for-Ocelot-switches/20210120-145800
base: https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git 7b8fc0103bb51d1d3e1fb5fd67958612e709f883
config: nds32-allyesconfig (attached as .config)
compiler: nds32le-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/0day-ci/linux/commit/c1cb52858ce53bff4293bc96d800bbb0bd22fc74
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Vladimir-Oltean/tag_8021q-for-Ocelot-switches/20210120-145800
git checkout c1cb52858ce53bff4293bc96d800bbb0bd22fc74
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=nds32
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
drivers/net/dsa/ocelot/felix.c: In function 'felix_setup_tag_8021q':
>> drivers/net/dsa/ocelot/felix.c:464:12: warning: variable 'err' set but not used [-Wunused-but-set-variable]
464 | int port, err;
| ^~~
vim +/err +464 drivers/net/dsa/ocelot/felix.c
7bc3a3dc54b2bb6 Vladimir Oltean 2021-01-20 458
7bc3a3dc54b2bb6 Vladimir Oltean 2021-01-20 459 static int felix_setup_tag_8021q(struct dsa_switch *ds, int cpu)
7bc3a3dc54b2bb6 Vladimir Oltean 2021-01-20 460 {
7bc3a3dc54b2bb6 Vladimir Oltean 2021-01-20 461 struct ocelot *ocelot = ds->priv;
7bc3a3dc54b2bb6 Vladimir Oltean 2021-01-20 462 struct felix *felix = ocelot_to_felix(ocelot);
7bc3a3dc54b2bb6 Vladimir Oltean 2021-01-20 463 unsigned long cpu_flood;
7bc3a3dc54b2bb6 Vladimir Oltean 2021-01-20 @464 int port, err;
7bc3a3dc54b2bb6 Vladimir Oltean 2021-01-20 465
7bc3a3dc54b2bb6 Vladimir Oltean 2021-01-20 466 felix_8021q_cpu_port_init(ocelot, cpu);
7bc3a3dc54b2bb6 Vladimir Oltean 2021-01-20 467
7bc3a3dc54b2bb6 Vladimir Oltean 2021-01-20 468 for (port = 0; port < ds->num_ports; port++) {
7bc3a3dc54b2bb6 Vladimir Oltean 2021-01-20 469 if (dsa_is_unused_port(ds, port))
7bc3a3dc54b2bb6 Vladimir Oltean 2021-01-20 470 continue;
7bc3a3dc54b2bb6 Vladimir Oltean 2021-01-20 471
7bc3a3dc54b2bb6 Vladimir Oltean 2021-01-20 472 /* This overwrites ocelot_init():
7bc3a3dc54b2bb6 Vladimir Oltean 2021-01-20 473 * Do not forward BPDU frames to the CPU port module,
7bc3a3dc54b2bb6 Vladimir Oltean 2021-01-20 474 * for 2 reasons:
7bc3a3dc54b2bb6 Vladimir Oltean 2021-01-20 475 * - When these packets are injected from the tag_8021q
7bc3a3dc54b2bb6 Vladimir Oltean 2021-01-20 476 * CPU port, we want them to go out, not loop back
7bc3a3dc54b2bb6 Vladimir Oltean 2021-01-20 477 * into the system.
7bc3a3dc54b2bb6 Vladimir Oltean 2021-01-20 478 * - STP traffic ingressing on a user port should go to
7bc3a3dc54b2bb6 Vladimir Oltean 2021-01-20 479 * the tag_8021q CPU port, not to the hardware CPU
7bc3a3dc54b2bb6 Vladimir Oltean 2021-01-20 480 * port module.
7bc3a3dc54b2bb6 Vladimir Oltean 2021-01-20 481 */
7bc3a3dc54b2bb6 Vladimir Oltean 2021-01-20 482 ocelot_write_gix(ocelot,
7bc3a3dc54b2bb6 Vladimir Oltean 2021-01-20 483 ANA_PORT_CPU_FWD_BPDU_CFG_BPDU_REDIR_ENA(0),
7bc3a3dc54b2bb6 Vladimir Oltean 2021-01-20 484 ANA_PORT_CPU_FWD_BPDU_CFG, port);
7bc3a3dc54b2bb6 Vladimir Oltean 2021-01-20 485 }
7bc3a3dc54b2bb6 Vladimir Oltean 2021-01-20 486
c1cb52858ce53bf Vladimir Oltean 2021-01-20 487 /* In tag_8021q mode, the CPU port module is unused, except for PTP
c1cb52858ce53bf Vladimir Oltean 2021-01-20 488 * frames. So we want to disable flooding of any kind to the CPU port
c1cb52858ce53bf Vladimir Oltean 2021-01-20 489 * module, since packets going there will end in a black hole.
7bc3a3dc54b2bb6 Vladimir Oltean 2021-01-20 490 */
7bc3a3dc54b2bb6 Vladimir Oltean 2021-01-20 491 cpu_flood = ANA_PGID_PGID_PGID(BIT(ocelot->num_phys_ports));
7bc3a3dc54b2bb6 Vladimir Oltean 2021-01-20 492 ocelot_rmw_rix(ocelot, 0, cpu_flood, ANA_PGID_PGID, PGID_UC);
7bc3a3dc54b2bb6 Vladimir Oltean 2021-01-20 493 ocelot_rmw_rix(ocelot, 0, cpu_flood, ANA_PGID_PGID, PGID_MC);
7bc3a3dc54b2bb6 Vladimir Oltean 2021-01-20 494
7bc3a3dc54b2bb6 Vladimir Oltean 2021-01-20 495 ocelot_apply_bridge_fwd_mask(ocelot);
7bc3a3dc54b2bb6 Vladimir Oltean 2021-01-20 496
7bc3a3dc54b2bb6 Vladimir Oltean 2021-01-20 497 felix->dsa_8021q_ctx = kzalloc(sizeof(*felix->dsa_8021q_ctx),
7bc3a3dc54b2bb6 Vladimir Oltean 2021-01-20 498 GFP_KERNEL);
7bc3a3dc54b2bb6 Vladimir Oltean 2021-01-20 499 if (!felix->dsa_8021q_ctx)
7bc3a3dc54b2bb6 Vladimir Oltean 2021-01-20 500 return -ENOMEM;
7bc3a3dc54b2bb6 Vladimir Oltean 2021-01-20 501
7bc3a3dc54b2bb6 Vladimir Oltean 2021-01-20 502 felix->dsa_8021q_ctx->ops = &felix_tag_8021q_ops;
7bc3a3dc54b2bb6 Vladimir Oltean 2021-01-20 503 felix->dsa_8021q_ctx->proto = htons(ETH_P_8021AD);
7bc3a3dc54b2bb6 Vladimir Oltean 2021-01-20 504 felix->dsa_8021q_ctx->ds = ds;
7bc3a3dc54b2bb6 Vladimir Oltean 2021-01-20 505
7bc3a3dc54b2bb6 Vladimir Oltean 2021-01-20 506 rtnl_lock();
7bc3a3dc54b2bb6 Vladimir Oltean 2021-01-20 507 err = dsa_8021q_setup(felix->dsa_8021q_ctx, true);
7bc3a3dc54b2bb6 Vladimir Oltean 2021-01-20 508 rtnl_unlock();
7bc3a3dc54b2bb6 Vladimir Oltean 2021-01-20 509
c1cb52858ce53bf Vladimir Oltean 2021-01-20 510 return felix_setup_mmio_filtering(felix);
7bc3a3dc54b2bb6 Vladimir Oltean 2021-01-20 511 }
7bc3a3dc54b2bb6 Vladimir Oltean 2021-01-20 512
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
On Wed, 20 Jan 2021 01:07:48 +0200 Vladimir Oltean wrote: > From: Vladimir Oltean <vladimir.oltean@nxp.com> > > Since the tag_8021q tagger is software-defined, it has no means by > itself for retrieving hardware timestamps of PTP event messages. > > Because we do want to support PTP on ocelot even with tag_8021q, we need > to use the CPU port module for that. The RX timestamp is present in the > Extraction Frame Header. And because we can't use NPI mode which redirects > the CPU queues to an "external CPU" (meaning the ARM CPU running Linux), > then we need to poll the CPU port module through the MMIO registers to > retrieve TX and RX timestamps. > > Sadly, on NXP LS1028A, the Felix switch was integrated into the SoC > without wiring the extraction IRQ line to the ARM GIC. So, if we want to > be notified of any PTP packets received on the CPU port module, we have > a problem. > > There is a possible workaround, which is to use the Ethernet CPU port as > a notification channel that packets are available on the CPU port module > as well. When a PTP packet is received by the DSA tagger (without timestamp, > of course), we go to the CPU extraction queues, poll for it there, then > we drop the original Ethernet packet and masquerade the packet retrieved > over MMIO (plus the timestamp) as the original when we inject it up the > stack. > > Create a quirk in struct felix is selected by the Felix driver (but not > by Seville, since that doesn't support PTP at all). We want to do this > such that the workaround is minimally invasive for future switches that > don't require this workaround. > > The only traffic for which we need timestamps is PTP traffic, so add a > redirection rule to the CPU port module for this. Currently we only have > the need for PTP over L2, so redirection rules for UDP ports 319 and 320 > are TBD for now. > > Note that for the workaround of matching of PTP-over-Ethernet-port with > PTP-over-MMIO queues to work properly, both channels need to be > absolutely lossless. There are two parts to achieving that: > - We keep flow control enabled on the tag_8021q CPU port > - We put the DSA master interface in promiscuous mode, so it will never > drop a PTP frame (for the profiles we are interested in, these are > sent to the multicast MAC addresses of 01-80-c2-00-00-0e and > 01-1b-19-00-00-00). > > Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com> drivers/net/dsa/ocelot/felix.c:464:12: warning: variable ‘err’ set but not used [-Wunused-but-set-variable] 464 | int port, err; | ^~~ drivers/net/dsa/ocelot/felix.c:265:53: warning: incorrect type in assignment (different base types) drivers/net/dsa/ocelot/felix.c:265:53: expected unsigned short [usertype] drivers/net/dsa/ocelot/felix.c:265:53: got restricted __be16 [usertype] Please build test the patches locally, the patchwork testing thing is not keeping up with the volume, and it's running on the largest VM available thru the provider already :/ I need to add this "don't post your patches to get them build tested or you'll make Kuba very angry" to the netdev FAQ.
On Wed, Jan 20, 2021 at 08:40:42AM -0800, Jakub Kicinski wrote: > drivers/net/dsa/ocelot/felix.c:464:12: warning: variable ‘err’ set but not used [-Wunused-but-set-variable] > 464 | int port, err; > | ^~~ > drivers/net/dsa/ocelot/felix.c:265:53: warning: incorrect type in assignment (different base types) > drivers/net/dsa/ocelot/felix.c:265:53: expected unsigned short [usertype] > drivers/net/dsa/ocelot/felix.c:265:53: got restricted __be16 [usertype] > > > Please build test the patches locally, the patchwork testing thing is > not keeping up with the volume, and it's running on the largest VM > available thru the provider already :/ I updated my compiler now, so that W=1 C=1 builds would not fail. That should hopefully prevent this from happening in the future. > I need to add this "don't post your patches to get them build tested > or you'll make Kuba very angry" to the netdev FAQ. Since I definitely don't want to upset Kuba, how bad is it to exceed the 15 patches per series limit? Do I need to do something about it?
On Wed, 20 Jan 2021 19:32:41 +0200 Vladimir Oltean wrote: > On Wed, Jan 20, 2021 at 08:40:42AM -0800, Jakub Kicinski wrote: > > drivers/net/dsa/ocelot/felix.c:464:12: warning: variable ‘err’ set but not used [-Wunused-but-set-variable] > > 464 | int port, err; > > | ^~~ > > drivers/net/dsa/ocelot/felix.c:265:53: warning: incorrect type in assignment (different base types) > > drivers/net/dsa/ocelot/felix.c:265:53: expected unsigned short [usertype] > > drivers/net/dsa/ocelot/felix.c:265:53: got restricted __be16 [usertype] > > > > > > Please build test the patches locally, the patchwork testing thing is > > not keeping up with the volume, and it's running on the largest VM > > available thru the provider already :/ > > I updated my compiler now, so that W=1 C=1 builds would not fail. > That should hopefully prevent this from happening in the future. Thanks. > > I need to add this "don't post your patches to get them build tested > > or you'll make Kuba very angry" to the netdev FAQ. > > Since I definitely don't want to upset Kuba, :) > how bad is it to exceed the 15 patches per series limit? Do I need to > do something about it? It's not a hard rule IIUC, if you have 16, 17 patches as an atomic series which is hard to split, I'd think that's acceptable from time to time. Especially if the patches themselves are not huge. If you're already splitting a larger effort, keeping it < 15 is best. In general if you can split a smaller logically contained series out that's always preferred. The point is if the series is too large reviewers are likely to postpone reviewing it until they can allocate sufficiently large continuous block of time, which may be never. It's all about efficient code review. At least that's my recollection / understanding. There may be more reasons, we'd have to ask Dave.
On Wed, Jan 20, 2021 at 12:58:13PM -0800, Jakub Kicinski wrote: > > how bad is it to exceed the 15 patches per series limit? Do I need to > > do something about it? > > It's not a hard rule IIUC, if you have 16, 17 patches as an atomic > series which is hard to split, I'd think that's acceptable from time > to time. Especially if the patches themselves are not huge. > If you're already splitting a larger effort, keeping it < 15 is best. > In general if you can split a smaller logically contained series out > that's always preferred. The point is if the series is too large > reviewers are likely to postpone reviewing it until they can allocate > sufficiently large continuous block of time, which may be never. > It's all about efficient code review. > > At least that's my recollection / understanding. There may be more > reasons, we'd have to ask Dave. To be fair this series is abusively large even for me to read, and _is_ easy to split. In v1 I had posted just the first half, but then figured that reviewers might want to have the full picture of where I'm trying to get at. But now that the picture has been given, I'm going back to the split format. Thanks.
diff --git a/drivers/net/dsa/ocelot/felix.c b/drivers/net/dsa/ocelot/felix.c index df43f4a40f83..cb05d795ef05 100644 --- a/drivers/net/dsa/ocelot/felix.c +++ b/drivers/net/dsa/ocelot/felix.c @@ -231,6 +231,120 @@ static const struct dsa_8021q_ops felix_tag_8021q_ops = { .vlan_del = felix_tag_8021q_vlan_del, }; +/* Set up a VCAP IS2 rule for delivering PTP frames to the CPU port module. + * If the quirk_no_xtr_irq is in place, then also copy those PTP frames to the + * tag_8021q CPU port. + */ +static int felix_setup_mmio_filtering(struct felix *felix) +{ + unsigned long user_ports = 0, cpu_ports = 0; + struct ocelot_vcap_filter *redirect_rule; + struct ocelot_vcap_filter *tagging_rule; + struct ocelot *ocelot = &felix->ocelot; + struct dsa_switch *ds = felix->ds; + int port, ret; + + tagging_rule = kzalloc(sizeof(struct ocelot_vcap_filter), GFP_KERNEL); + if (!tagging_rule) + return -ENOMEM; + + redirect_rule = kzalloc(sizeof(struct ocelot_vcap_filter), GFP_KERNEL); + if (!redirect_rule) { + kfree(tagging_rule); + return -ENOMEM; + } + + for (port = 0; port < ocelot->num_phys_ports; port++) { + if (dsa_is_user_port(ds, port)) + user_ports |= BIT(port); + if (dsa_is_cpu_port(ds, port)) + cpu_ports |= BIT(port); + } + + tagging_rule->key_type = OCELOT_VCAP_KEY_ETYPE; + *(u16 *)tagging_rule->key.etype.etype.value = htons(ETH_P_1588); + *(u16 *)tagging_rule->key.etype.etype.mask = 0xffff; + tagging_rule->ingress_port_mask = user_ports; + tagging_rule->prio = 1; + tagging_rule->id.cookie = ocelot->num_phys_ports; + tagging_rule->id.tc_offload = false; + tagging_rule->block_id = VCAP_IS1; + tagging_rule->type = OCELOT_VCAP_FILTER_OFFLOAD; + tagging_rule->lookup = 0; + tagging_rule->action.pag_override_mask = 0xff; + tagging_rule->action.pag_val = ocelot->num_phys_ports; + + ret = ocelot_vcap_filter_add(ocelot, tagging_rule, NULL); + if (ret) { + kfree(tagging_rule); + kfree(redirect_rule); + return ret; + } + + redirect_rule->key_type = OCELOT_VCAP_KEY_ANY; + redirect_rule->ingress_port_mask = user_ports; + redirect_rule->pag = ocelot->num_phys_ports; + redirect_rule->prio = 1; + redirect_rule->id.cookie = ocelot->num_phys_ports; + redirect_rule->id.tc_offload = false; + redirect_rule->block_id = VCAP_IS2; + redirect_rule->type = OCELOT_VCAP_FILTER_OFFLOAD; + redirect_rule->lookup = 0; + redirect_rule->action.cpu_copy_ena = true; + if (felix->info->quirk_no_xtr_irq) { + /* Redirect to the tag_8021q CPU but also copy PTP packets to + * the CPU port module + */ + redirect_rule->action.mask_mode = OCELOT_MASK_MODE_REDIRECT; + redirect_rule->action.port_mask = cpu_ports; + } else { + /* Trap PTP packets only to the CPU port module (which is + * redirected to the NPI port) + */ + redirect_rule->action.mask_mode = OCELOT_MASK_MODE_PERMIT_DENY; + redirect_rule->action.port_mask = 0; + } + + ret = ocelot_vcap_filter_add(ocelot, redirect_rule, NULL); + if (ret) { + ocelot_vcap_filter_del(ocelot, tagging_rule); + kfree(redirect_rule); + return ret; + } + + return 0; +} + +static int felix_teardown_mmio_filtering(struct felix *felix) +{ + struct ocelot_vcap_filter *tagging_rule, *redirect_rule; + struct ocelot_vcap_block *block_vcap_is1; + struct ocelot_vcap_block *block_vcap_is2; + struct ocelot *ocelot = &felix->ocelot; + int err; + + block_vcap_is1 = &ocelot->block[VCAP_IS1]; + block_vcap_is2 = &ocelot->block[VCAP_IS2]; + + tagging_rule = ocelot_vcap_block_find_filter_by_id(block_vcap_is1, + ocelot->num_phys_ports, + false); + if (!tagging_rule) + return -ENOENT; + + err = ocelot_vcap_filter_del(ocelot, tagging_rule); + if (err) + return err; + + redirect_rule = ocelot_vcap_block_find_filter_by_id(block_vcap_is2, + ocelot->num_phys_ports, + false); + if (!redirect_rule) + return -ENOENT; + + return ocelot_vcap_filter_del(ocelot, redirect_rule); +} + static enum dsa_tag_protocol felix_get_tag_protocol(struct dsa_switch *ds, int port, enum dsa_tag_protocol mp) @@ -370,9 +484,9 @@ static int felix_setup_tag_8021q(struct dsa_switch *ds, int cpu) ANA_PORT_CPU_FWD_BPDU_CFG, port); } - /* In tag_8021q mode, the CPU port module is unused. So we - * want to disable flooding of any kind to the CPU port module, - * since packets going there will end in a black hole. + /* In tag_8021q mode, the CPU port module is unused, except for PTP + * frames. So we want to disable flooding of any kind to the CPU port + * module, since packets going there will end in a black hole. */ cpu_flood = ANA_PGID_PGID_PGID(BIT(ocelot->num_phys_ports)); ocelot_rmw_rix(ocelot, 0, cpu_flood, ANA_PGID_PGID, PGID_UC); @@ -393,7 +507,7 @@ static int felix_setup_tag_8021q(struct dsa_switch *ds, int cpu) err = dsa_8021q_setup(felix->dsa_8021q_ctx, true); rtnl_unlock(); - return err; + return felix_setup_mmio_filtering(felix); } static void felix_teardown_tag_8021q(struct dsa_switch *ds, int cpu) @@ -402,6 +516,11 @@ static void felix_teardown_tag_8021q(struct dsa_switch *ds, int cpu) struct felix *felix = ocelot_to_felix(ocelot); int err, port; + err = felix_teardown_mmio_filtering(felix); + if (err) + dev_err(ds->dev, "felix_teardown_mmio_filtering returned %d", + err); + rtnl_lock(); err = dsa_8021q_setup(felix->dsa_8021q_ctx, false); rtnl_unlock(); diff --git a/drivers/net/dsa/ocelot/felix.h b/drivers/net/dsa/ocelot/felix.h index 9d4459f2fffb..eb85eac361db 100644 --- a/drivers/net/dsa/ocelot/felix.h +++ b/drivers/net/dsa/ocelot/felix.h @@ -23,6 +23,19 @@ struct felix_info { int switch_pci_bar; int imdio_pci_bar; const struct ptp_clock_info *ptp_caps; + + /* Some Ocelot switches are integrated into the SoC without the + * extraction IRQ line connected to the ARM GIC. By enabling this + * workaround, the few packets that are delivered to the CPU port + * module (currently only PTP) are copied not only to the hardware CPU + * port module, but also to the 802.1Q Ethernet CPU port, and polling + * the extraction registers is triggered once the DSA tagger sees a PTP + * frame. The Ethernet frame is only used as a notification: it is + * dropped, and the original frame is extracted over MMIO and annotated + * with the RX timestamp. + */ + bool quirk_no_xtr_irq; + int (*mdio_bus_alloc)(struct ocelot *ocelot); void (*mdio_bus_free)(struct ocelot *ocelot); void (*phylink_validate)(struct ocelot *ocelot, int port, diff --git a/drivers/net/dsa/ocelot/felix_vsc9959.c b/drivers/net/dsa/ocelot/felix_vsc9959.c index e944868cc120..38224c0169c0 100644 --- a/drivers/net/dsa/ocelot/felix_vsc9959.c +++ b/drivers/net/dsa/ocelot/felix_vsc9959.c @@ -1379,6 +1379,7 @@ static const struct felix_info felix_info_vsc9959 = { .num_tx_queues = OCELOT_NUM_TC, .switch_pci_bar = 4, .imdio_pci_bar = 0, + .quirk_no_xtr_irq = true, .ptp_caps = &vsc9959_ptp_caps, .mdio_bus_alloc = vsc9959_mdio_bus_alloc, .mdio_bus_free = vsc9959_mdio_bus_free, diff --git a/net/dsa/tag_ocelot_8021q.c b/net/dsa/tag_ocelot_8021q.c index 09e10ade11f7..7bd45b66e9a4 100644 --- a/net/dsa/tag_ocelot_8021q.c +++ b/net/dsa/tag_ocelot_8021q.c @@ -60,6 +60,7 @@ static struct dsa_device_ops ocelot_netdev_ops = { .xmit = ocelot_xmit, .rcv = ocelot_rcv, .overhead = VLAN_HLEN, + .promisc_on_master = true, }; MODULE_LICENSE("GPL v2");