* RE: [PATCH] vsock: only load vmci transport on VMware hypervisor by default
From: Dexuan Cui @ 2017-09-06 19:39 UTC (permalink / raw)
To: Jorgen S. Hansen, Stefan Hajnoczi
Cc: Michal Kubecek, joe@perches.com, olaf@aepfle.de,
Stephen Hemminger, jasowang@redhat.com, netdev@vger.kernel.org,
Haiyang Zhang, Dave Scott, apw@canonical.com,
linux-kernel@vger.kernel.org, Vitaly Kuznetsov, Rolf Neugebauer,
gregkh@linuxfoundation.org, Marcelo Cerri,
devel@linuxdriverproject.org, Asias He, davem@davemloft.net,
George Zhang, Dan Carpenter
In-Reply-To: <8E47FDFD-809F-4EE9-9068-65BE50BE8BEB@vmware.com>
> From: Jorgen S. Hansen [mailto:jhansen@vmware.com]
> Sent: Wednesday, September 6, 2017 7:11 AM
>> ...
> > I'm currently working on NFS over AF_VSOCK and sock_diag support (for
> > ss(8) and netstat-like tools).
> >
> > Multi-transport support is lower priority for me at the moment. I'm
> > happy to review patches though. If there is no progress on this by the
> > end of the year then I will have time to work on it.
> >
>
> I’ll try to find time to write a more coherent proposal in the coming weeks,
> and we can discuss that.
>
> Jorgen
Thank you!
Thanks,
-- Dexuan
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
^ permalink raw reply
* [wireless-testsing2:master 4/5] drivers/net//ethernet/marvell/mvpp2.c:7618:2: warning: passing argument 4 of 'mvpp2_port_copy_mac_addr' from incompatible pointer type
From: kbuild test robot @ 2017-09-06 19:30 UTC (permalink / raw)
To: David S. Miller; +Cc: kbuild-all, netdev, Bob Copeland
[-- Attachment #1: Type: text/plain, Size: 16010 bytes --]
tree: https://git.kernel.org/pub/scm/linux/kernel/git/wireless/wireless-testing.git master
head: d17be7f7503bf9492198a242779a68af93fd92de
commit: 96c03618031bae5e9068b16f9e437b79f98f6482 [4/5] Merge remote-tracking branch 'mac80211-next/master'
config: xtensa-allyesconfig (attached as .config)
compiler: xtensa-linux-gcc (GCC) 4.9.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
git checkout 96c03618031bae5e9068b16f9e437b79f98f6482
# save the attached .config to linux build tree
make.cross ARCH=xtensa
All warnings (new ones prefixed by >>):
drivers/net//ethernet/marvell/mvpp2.c: In function 'mvpp2_port_probe':
>> drivers/net//ethernet/marvell/mvpp2.c:7618:2: warning: passing argument 4 of 'mvpp2_port_copy_mac_addr' from incompatible pointer type
mvpp2_port_copy_mac_addr(dev, priv, port_node, &mac_from);
^
drivers/net//ethernet/marvell/mvpp2.c:7468:13: note: expected 'char **' but argument is of type 'const char **'
static void mvpp2_port_copy_mac_addr(struct net_device *dev, struct mvpp2 *priv,
^
drivers/net//ethernet/marvell/mvpp2.c:7509:7: warning: unused variable 'hw_mac_addr' [-Wunused-variable]
char hw_mac_addr[ETH_ALEN] = {0};
^
drivers/net//ethernet/marvell/mvpp2.c:7507:14: warning: unused variable 'dt_mac_addr' [-Wunused-variable]
const char *dt_mac_addr;
^
vim +/mvpp2_port_copy_mac_addr +7618 drivers/net//ethernet/marvell/mvpp2.c
3ba8c81e1 Antoine Tenart 2017-09-02 7495
3f518509d Marcin Wojtas 2014-07-10 7496 /* Ports initialization */
3f518509d Marcin Wojtas 2014-07-10 7497 static int mvpp2_port_probe(struct platform_device *pdev,
3f518509d Marcin Wojtas 2014-07-10 7498 struct device_node *port_node,
59b9a31ed Thomas Petazzoni 2017-03-07 7499 struct mvpp2 *priv)
3f518509d Marcin Wojtas 2014-07-10 7500 {
3f518509d Marcin Wojtas 2014-07-10 7501 struct device_node *phy_node;
542897d98 Antoine Tenart 2017-08-30 7502 struct phy *comphy;
3f518509d Marcin Wojtas 2014-07-10 7503 struct mvpp2_port *port;
edc660fa0 Marcin Wojtas 2015-08-06 7504 struct mvpp2_port_pcpu *port_pcpu;
3f518509d Marcin Wojtas 2014-07-10 7505 struct net_device *dev;
3f518509d Marcin Wojtas 2014-07-10 7506 struct resource *res;
3f518509d Marcin Wojtas 2014-07-10 7507 const char *dt_mac_addr;
96c036180 Bob Copeland 2017-09-04 7508 const char *mac_from = "";
4c2286826 Antoine Tenart 2017-08-25 7509 char hw_mac_addr[ETH_ALEN] = {0};
09f839755 Thomas Petazzoni 2017-08-03 7510 unsigned int ntxqs, nrxqs;
213f428f5 Thomas Petazzoni 2017-08-03 7511 bool has_tx_irqs;
3f518509d Marcin Wojtas 2014-07-10 7512 u32 id;
3f518509d Marcin Wojtas 2014-07-10 7513 int features;
3f518509d Marcin Wojtas 2014-07-10 7514 int phy_mode;
edc660fa0 Marcin Wojtas 2015-08-06 7515 int err, i, cpu;
3f518509d Marcin Wojtas 2014-07-10 7516
213f428f5 Thomas Petazzoni 2017-08-03 7517 has_tx_irqs = mvpp2_port_has_tx_irqs(priv, port_node);
213f428f5 Thomas Petazzoni 2017-08-03 7518
213f428f5 Thomas Petazzoni 2017-08-03 7519 if (!has_tx_irqs)
213f428f5 Thomas Petazzoni 2017-08-03 7520 queue_mode = MVPP2_QDIST_SINGLE_MODE;
213f428f5 Thomas Petazzoni 2017-08-03 7521
09f839755 Thomas Petazzoni 2017-08-03 7522 ntxqs = MVPP2_MAX_TXQ;
213f428f5 Thomas Petazzoni 2017-08-03 7523 if (priv->hw_version == MVPP22 && queue_mode == MVPP2_QDIST_MULTI_MODE)
213f428f5 Thomas Petazzoni 2017-08-03 7524 nrxqs = MVPP2_DEFAULT_RXQ * num_possible_cpus();
213f428f5 Thomas Petazzoni 2017-08-03 7525 else
09f839755 Thomas Petazzoni 2017-08-03 7526 nrxqs = MVPP2_DEFAULT_RXQ;
09f839755 Thomas Petazzoni 2017-08-03 7527
09f839755 Thomas Petazzoni 2017-08-03 7528 dev = alloc_etherdev_mqs(sizeof(*port), ntxqs, nrxqs);
3f518509d Marcin Wojtas 2014-07-10 7529 if (!dev)
3f518509d Marcin Wojtas 2014-07-10 7530 return -ENOMEM;
3f518509d Marcin Wojtas 2014-07-10 7531
3f518509d Marcin Wojtas 2014-07-10 7532 phy_node = of_parse_phandle(port_node, "phy", 0);
3f518509d Marcin Wojtas 2014-07-10 7533 phy_mode = of_get_phy_mode(port_node);
3f518509d Marcin Wojtas 2014-07-10 7534 if (phy_mode < 0) {
3f518509d Marcin Wojtas 2014-07-10 7535 dev_err(&pdev->dev, "incorrect phy mode\n");
3f518509d Marcin Wojtas 2014-07-10 7536 err = phy_mode;
3f518509d Marcin Wojtas 2014-07-10 7537 goto err_free_netdev;
3f518509d Marcin Wojtas 2014-07-10 7538 }
3f518509d Marcin Wojtas 2014-07-10 7539
542897d98 Antoine Tenart 2017-08-30 7540 comphy = devm_of_phy_get(&pdev->dev, port_node, NULL);
542897d98 Antoine Tenart 2017-08-30 7541 if (IS_ERR(comphy)) {
542897d98 Antoine Tenart 2017-08-30 7542 if (PTR_ERR(comphy) == -EPROBE_DEFER) {
542897d98 Antoine Tenart 2017-08-30 7543 err = -EPROBE_DEFER;
542897d98 Antoine Tenart 2017-08-30 7544 goto err_free_netdev;
542897d98 Antoine Tenart 2017-08-30 7545 }
542897d98 Antoine Tenart 2017-08-30 7546 comphy = NULL;
542897d98 Antoine Tenart 2017-08-30 7547 }
542897d98 Antoine Tenart 2017-08-30 7548
3f518509d Marcin Wojtas 2014-07-10 7549 if (of_property_read_u32(port_node, "port-id", &id)) {
3f518509d Marcin Wojtas 2014-07-10 7550 err = -EINVAL;
3f518509d Marcin Wojtas 2014-07-10 7551 dev_err(&pdev->dev, "missing port-id value\n");
3f518509d Marcin Wojtas 2014-07-10 7552 goto err_free_netdev;
3f518509d Marcin Wojtas 2014-07-10 7553 }
3f518509d Marcin Wojtas 2014-07-10 7554
3f518509d Marcin Wojtas 2014-07-10 7555 dev->tx_queue_len = MVPP2_MAX_TXD;
3f518509d Marcin Wojtas 2014-07-10 7556 dev->watchdog_timeo = 5 * HZ;
3f518509d Marcin Wojtas 2014-07-10 7557 dev->netdev_ops = &mvpp2_netdev_ops;
3f518509d Marcin Wojtas 2014-07-10 7558 dev->ethtool_ops = &mvpp2_eth_tool_ops;
3f518509d Marcin Wojtas 2014-07-10 7559
3f518509d Marcin Wojtas 2014-07-10 7560 port = netdev_priv(dev);
591f4cfab Thomas Petazzoni 2017-08-03 7561 port->dev = dev;
09f839755 Thomas Petazzoni 2017-08-03 7562 port->ntxqs = ntxqs;
09f839755 Thomas Petazzoni 2017-08-03 7563 port->nrxqs = nrxqs;
213f428f5 Thomas Petazzoni 2017-08-03 7564 port->priv = priv;
213f428f5 Thomas Petazzoni 2017-08-03 7565 port->has_tx_irqs = has_tx_irqs;
3f518509d Marcin Wojtas 2014-07-10 7566
591f4cfab Thomas Petazzoni 2017-08-03 7567 err = mvpp2_queue_vectors_init(port, port_node);
591f4cfab Thomas Petazzoni 2017-08-03 7568 if (err)
3f518509d Marcin Wojtas 2014-07-10 7569 goto err_free_netdev;
3f518509d Marcin Wojtas 2014-07-10 7570
fd3651b2a Antoine Tenart 2017-09-01 7571 port->link_irq = of_irq_get_byname(port_node, "link");
fd3651b2a Antoine Tenart 2017-09-01 7572 if (port->link_irq == -EPROBE_DEFER) {
fd3651b2a Antoine Tenart 2017-09-01 7573 err = -EPROBE_DEFER;
fd3651b2a Antoine Tenart 2017-09-01 7574 goto err_deinit_qvecs;
fd3651b2a Antoine Tenart 2017-09-01 7575 }
fd3651b2a Antoine Tenart 2017-09-01 7576 if (port->link_irq <= 0)
fd3651b2a Antoine Tenart 2017-09-01 7577 /* the link irq is optional */
fd3651b2a Antoine Tenart 2017-09-01 7578 port->link_irq = 0;
fd3651b2a Antoine Tenart 2017-09-01 7579
3f518509d Marcin Wojtas 2014-07-10 7580 if (of_property_read_bool(port_node, "marvell,loopback"))
3f518509d Marcin Wojtas 2014-07-10 7581 port->flags |= MVPP2_F_LOOPBACK;
3f518509d Marcin Wojtas 2014-07-10 7582
3f518509d Marcin Wojtas 2014-07-10 7583 port->id = id;
59b9a31ed Thomas Petazzoni 2017-03-07 7584 if (priv->hw_version == MVPP21)
09f839755 Thomas Petazzoni 2017-08-03 7585 port->first_rxq = port->id * port->nrxqs;
59b9a31ed Thomas Petazzoni 2017-03-07 7586 else
59b9a31ed Thomas Petazzoni 2017-03-07 7587 port->first_rxq = port->id * priv->max_port_rxqs;
59b9a31ed Thomas Petazzoni 2017-03-07 7588
3f518509d Marcin Wojtas 2014-07-10 7589 port->phy_node = phy_node;
3f518509d Marcin Wojtas 2014-07-10 7590 port->phy_interface = phy_mode;
542897d98 Antoine Tenart 2017-08-30 7591 port->comphy = comphy;
3f518509d Marcin Wojtas 2014-07-10 7592
a786841df Thomas Petazzoni 2017-03-07 7593 if (priv->hw_version == MVPP21) {
a786841df Thomas Petazzoni 2017-03-07 7594 res = platform_get_resource(pdev, IORESOURCE_MEM, 2 + id);
3f518509d Marcin Wojtas 2014-07-10 7595 port->base = devm_ioremap_resource(&pdev->dev, res);
3f518509d Marcin Wojtas 2014-07-10 7596 if (IS_ERR(port->base)) {
3f518509d Marcin Wojtas 2014-07-10 7597 err = PTR_ERR(port->base);
fd3651b2a Antoine Tenart 2017-09-01 7598 goto err_free_irq;
3f518509d Marcin Wojtas 2014-07-10 7599 }
a786841df Thomas Petazzoni 2017-03-07 7600 } else {
a786841df Thomas Petazzoni 2017-03-07 7601 if (of_property_read_u32(port_node, "gop-port-id",
a786841df Thomas Petazzoni 2017-03-07 7602 &port->gop_id)) {
a786841df Thomas Petazzoni 2017-03-07 7603 err = -EINVAL;
a786841df Thomas Petazzoni 2017-03-07 7604 dev_err(&pdev->dev, "missing gop-port-id value\n");
591f4cfab Thomas Petazzoni 2017-08-03 7605 goto err_deinit_qvecs;
a786841df Thomas Petazzoni 2017-03-07 7606 }
a786841df Thomas Petazzoni 2017-03-07 7607
a786841df Thomas Petazzoni 2017-03-07 7608 port->base = priv->iface_base + MVPP22_GMAC_BASE(port->gop_id);
a786841df Thomas Petazzoni 2017-03-07 7609 }
3f518509d Marcin Wojtas 2014-07-10 7610
3f518509d Marcin Wojtas 2014-07-10 7611 /* Alloc per-cpu stats */
3f518509d Marcin Wojtas 2014-07-10 7612 port->stats = netdev_alloc_pcpu_stats(struct mvpp2_pcpu_stats);
3f518509d Marcin Wojtas 2014-07-10 7613 if (!port->stats) {
3f518509d Marcin Wojtas 2014-07-10 7614 err = -ENOMEM;
fd3651b2a Antoine Tenart 2017-09-01 7615 goto err_free_irq;
3f518509d Marcin Wojtas 2014-07-10 7616 }
3f518509d Marcin Wojtas 2014-07-10 7617
3ba8c81e1 Antoine Tenart 2017-09-02 @7618 mvpp2_port_copy_mac_addr(dev, priv, port_node, &mac_from);
3f518509d Marcin Wojtas 2014-07-10 7619
3f518509d Marcin Wojtas 2014-07-10 7620 port->tx_ring_size = MVPP2_MAX_TXD;
3f518509d Marcin Wojtas 2014-07-10 7621 port->rx_ring_size = MVPP2_MAX_RXD;
3f518509d Marcin Wojtas 2014-07-10 7622 SET_NETDEV_DEV(dev, &pdev->dev);
3f518509d Marcin Wojtas 2014-07-10 7623
3f518509d Marcin Wojtas 2014-07-10 7624 err = mvpp2_port_init(port);
3f518509d Marcin Wojtas 2014-07-10 7625 if (err < 0) {
3f518509d Marcin Wojtas 2014-07-10 7626 dev_err(&pdev->dev, "failed to init port %d\n", id);
3f518509d Marcin Wojtas 2014-07-10 7627 goto err_free_stats;
3f518509d Marcin Wojtas 2014-07-10 7628 }
269758214 Thomas Petazzoni 2017-03-07 7629
269758214 Thomas Petazzoni 2017-03-07 7630 mvpp2_port_periodic_xon_disable(port);
269758214 Thomas Petazzoni 2017-03-07 7631
269758214 Thomas Petazzoni 2017-03-07 7632 if (priv->hw_version == MVPP21)
269758214 Thomas Petazzoni 2017-03-07 7633 mvpp2_port_fc_adv_enable(port);
269758214 Thomas Petazzoni 2017-03-07 7634
269758214 Thomas Petazzoni 2017-03-07 7635 mvpp2_port_reset(port);
3f518509d Marcin Wojtas 2014-07-10 7636
edc660fa0 Marcin Wojtas 2015-08-06 7637 port->pcpu = alloc_percpu(struct mvpp2_port_pcpu);
edc660fa0 Marcin Wojtas 2015-08-06 7638 if (!port->pcpu) {
edc660fa0 Marcin Wojtas 2015-08-06 7639 err = -ENOMEM;
edc660fa0 Marcin Wojtas 2015-08-06 7640 goto err_free_txq_pcpu;
edc660fa0 Marcin Wojtas 2015-08-06 7641 }
edc660fa0 Marcin Wojtas 2015-08-06 7642
213f428f5 Thomas Petazzoni 2017-08-03 7643 if (!port->has_tx_irqs) {
edc660fa0 Marcin Wojtas 2015-08-06 7644 for_each_present_cpu(cpu) {
edc660fa0 Marcin Wojtas 2015-08-06 7645 port_pcpu = per_cpu_ptr(port->pcpu, cpu);
edc660fa0 Marcin Wojtas 2015-08-06 7646
edc660fa0 Marcin Wojtas 2015-08-06 7647 hrtimer_init(&port_pcpu->tx_done_timer, CLOCK_MONOTONIC,
edc660fa0 Marcin Wojtas 2015-08-06 7648 HRTIMER_MODE_REL_PINNED);
edc660fa0 Marcin Wojtas 2015-08-06 7649 port_pcpu->tx_done_timer.function = mvpp2_hr_timer_cb;
edc660fa0 Marcin Wojtas 2015-08-06 7650 port_pcpu->timer_scheduled = false;
edc660fa0 Marcin Wojtas 2015-08-06 7651
213f428f5 Thomas Petazzoni 2017-08-03 7652 tasklet_init(&port_pcpu->tx_done_tasklet,
213f428f5 Thomas Petazzoni 2017-08-03 7653 mvpp2_tx_proc_cb,
edc660fa0 Marcin Wojtas 2015-08-06 7654 (unsigned long)dev);
edc660fa0 Marcin Wojtas 2015-08-06 7655 }
213f428f5 Thomas Petazzoni 2017-08-03 7656 }
edc660fa0 Marcin Wojtas 2015-08-06 7657
186cd4d4e Antoine Tenart 2017-08-23 7658 features = NETIF_F_SG | NETIF_F_IP_CSUM | NETIF_F_TSO;
3f518509d Marcin Wojtas 2014-07-10 7659 dev->features = features | NETIF_F_RXCSUM;
3f518509d Marcin Wojtas 2014-07-10 7660 dev->hw_features |= features | NETIF_F_RXCSUM | NETIF_F_GRO;
3f518509d Marcin Wojtas 2014-07-10 7661 dev->vlan_features |= features;
3f518509d Marcin Wojtas 2014-07-10 7662
5777987e0 Jarod Wilson 2016-10-17 7663 /* MTU range: 68 - 9676 */
5777987e0 Jarod Wilson 2016-10-17 7664 dev->min_mtu = ETH_MIN_MTU;
5777987e0 Jarod Wilson 2016-10-17 7665 /* 9676 == 9700 - 20 and rounding to 8 */
5777987e0 Jarod Wilson 2016-10-17 7666 dev->max_mtu = 9676;
5777987e0 Jarod Wilson 2016-10-17 7667
3f518509d Marcin Wojtas 2014-07-10 7668 err = register_netdev(dev);
3f518509d Marcin Wojtas 2014-07-10 7669 if (err < 0) {
3f518509d Marcin Wojtas 2014-07-10 7670 dev_err(&pdev->dev, "failed to register netdev\n");
edc660fa0 Marcin Wojtas 2015-08-06 7671 goto err_free_port_pcpu;
3f518509d Marcin Wojtas 2014-07-10 7672 }
3f518509d Marcin Wojtas 2014-07-10 7673 netdev_info(dev, "Using %s mac address %pM\n", mac_from, dev->dev_addr);
3f518509d Marcin Wojtas 2014-07-10 7674
3f518509d Marcin Wojtas 2014-07-10 7675 priv->port_list[id] = port;
3f518509d Marcin Wojtas 2014-07-10 7676 return 0;
3f518509d Marcin Wojtas 2014-07-10 7677
edc660fa0 Marcin Wojtas 2015-08-06 7678 err_free_port_pcpu:
edc660fa0 Marcin Wojtas 2015-08-06 7679 free_percpu(port->pcpu);
3f518509d Marcin Wojtas 2014-07-10 7680 err_free_txq_pcpu:
09f839755 Thomas Petazzoni 2017-08-03 7681 for (i = 0; i < port->ntxqs; i++)
3f518509d Marcin Wojtas 2014-07-10 7682 free_percpu(port->txqs[i]->pcpu);
3f518509d Marcin Wojtas 2014-07-10 7683 err_free_stats:
3f518509d Marcin Wojtas 2014-07-10 7684 free_percpu(port->stats);
fd3651b2a Antoine Tenart 2017-09-01 7685 err_free_irq:
fd3651b2a Antoine Tenart 2017-09-01 7686 if (port->link_irq)
fd3651b2a Antoine Tenart 2017-09-01 7687 irq_dispose_mapping(port->link_irq);
591f4cfab Thomas Petazzoni 2017-08-03 7688 err_deinit_qvecs:
591f4cfab Thomas Petazzoni 2017-08-03 7689 mvpp2_queue_vectors_deinit(port);
3f518509d Marcin Wojtas 2014-07-10 7690 err_free_netdev:
ccb80393c Peter Chen 2016-08-01 7691 of_node_put(phy_node);
3f518509d Marcin Wojtas 2014-07-10 7692 free_netdev(dev);
3f518509d Marcin Wojtas 2014-07-10 7693 return err;
3f518509d Marcin Wojtas 2014-07-10 7694 }
3f518509d Marcin Wojtas 2014-07-10 7695
:::::: The code at line 7618 was first introduced by commit
:::::: 3ba8c81e15c11fc396d0b5d11adaf9db6ed39533 net: mvpp2: move the mac retrieval/copy logic into its own function
:::::: TO: Antoine Tenart <antoine.tenart@free-electrons.com>
:::::: CC: David S. Miller <davem@davemloft.net>
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 51119 bytes --]
^ permalink raw reply
* Re: [PATCH net] Revert "net: phy: Correctly process PHY_HALTED in phy_stop_machine()"
From: Florian Fainelli @ 2017-09-06 19:28 UTC (permalink / raw)
To: Mason
Cc: Marc Gonzalez, David Daney, netdev, Geert Uytterhoeven,
David Miller, Andrew Lunn, Mans Rullgard, Thibaud Cornic
In-Reply-To: <927413e9-4f1f-963c-2d3a-5a88de2eac9e@free.fr>
On 09/06/2017 07:55 AM, Mason wrote:
> On 31/08/2017 21:18, Florian Fainelli wrote:
>
>> On 08/31/2017 12:09 PM, Mason wrote:
>>
>>> 1) nb8800_link_reconfigure() calls phy_print_status()
>>> which prints the "Link down" and "Link up" messages
>>> to the console. With the patch reverted, nothing is
>>> printed when the link goes down, and the result is
>>> random when the link comes up. Sometimes, we get
>>> down + up, sometimes just up.
>>
>> Nothing printed when you bring down the network interface as a result of
>> not signaling the link down, there is a small nuance here.
>
> Let me first focus on the "Link down" message.
>
> Do you agree that such a message should be printed when the
> link goes down, not when the link comes up?
The question is not so much about printing the message rather than a)
having the adjust_link callback be called and b) having this particular
callback correctly determine if a "change" has occurred, but let's focus
on the notification too. Printing the message is a consequence of these
two conditions and that's what matters.
There is not unfortunately a hard and fast answer it's clearly a
philosophical problem here.
The link is not physically down, the cable is still plugged so
generating a link down even is not necessarily correct. It would be
convenient for network manager programs, just like it is for network
drivers to treat it as such because that allows them to act like if the
cable was unplugged, which may be a good way to perform a number of
actions including but not limited to: entering a low power state,
re-initialization parts of the Ethernet MAC that need it (DMA, etc.,
etc.). That does not appear to be an absolute requirement for most, if
not all drivers since it changed after 3.4 and no one did bat an eye
about it.
Upon bringing the interface back up again, same thing, if the cable was
not disconnected should we just generate a link UP event, and if we do
that, are we going to confuse any network manager application?
Generating a link transition DOWN -> UP is certainly helpful for any
network application in that they do not need to keep any state just like
it clearly indicates a change was detected.
>
> Perhaps the issue is that the 2 following cases need to be
> handled differently:
> A) operator sets link down on the command-line
This is already handled differently since when you administratively
bring down an interface you call ndo_stop() which will be doing a
phy_stop() + phy_disconnect() which result in stopping the PHY state
machine and disconnecting from the PHY.
> B) asynchronous event makes link go down (peer is dead, cable is cut, etc)
>
> In B) the PHY state machine keeps on running, and eventually
> calls adjust_link()
Correct.
>
> In A) the driver calls phy_stop() and phy_disconnect() and
> therefore adjust_link() will not be called?
That is the current behavior (after the revert) and we can always change
it if deemed necessary, problem is, this broke for two people (one still
being discussed as of right now), so at this point I am very wary of
making any changes without more testing. I really need to get one of
these PHY interrupts wired to one of my boards or create a software
model of such a configuration before accepting new changes in that area.
Thank you
--
Florian
^ permalink raw reply
* Re: [PATCH net] Revert "net: phy: Correctly process PHY_HALTED in phy_stop_machine()"
From: Mason @ 2017-09-06 19:14 UTC (permalink / raw)
To: David Daney, Florian Fainelli
Cc: Marc Gonzalez, netdev, Geert Uytterhoeven, David Miller,
Andrew Lunn, Mans Rullgard
In-Reply-To: <ff070239-28b7-d41b-8abe-c9f810561372@caviumnetworks.com>
On 06/09/2017 20:00, David Daney wrote:
> On 08/31/2017 11:29 AM, Florian Fainelli wrote:
>> On 08/31/2017 11:12 AM, Mason wrote:
>>> On 31/08/2017 19:53, Florian Fainelli wrote:
>>>> On 08/31/2017 10:49 AM, Mason wrote:
>>>>> On 31/08/2017 18:57, Florian Fainelli wrote:
>>>>>> And the race is between phy_detach() setting phydev->attached_dev = NULL
>>>>>> and phy_state_machine() running in PHY_HALTED state and calling
>>>>>> netif_carrier_off().
>>>>>
>>>>> I must be missing something.
>>>>> (Since a thread cannot race against itself.)
>>>>>
>>>>> phy_disconnect calls phy_stop_machine which
>>>>> 1) stops the work queue from running in a separate thread
>>>>> 2) calls phy_state_machine *synchronously*
>>>>> which runs the PHY_HALTED case with everything well-defined
>>>>> end of phy_stop_machine
>>>>>
>>>>> phy_disconnect only then calls phy_detach()
>>>>> which makes future calls of phy_state_machine perilous.
>>>>>
>>>>> This all happens in the same thread, so I'm not yet
>>>>> seeing where the race happens?
>>>>
>>>> The race is as described in David's earlier email, so let's recap:
>>>>
>>>> Thread 1 Thread 2
>>>> phy_disconnect()
>>>> phy_stop_interrupts()
>>>> phy_stop_machine()
>>>> phy_state_machine()
>>>> -> queue_delayed_work()
>>>> phy_detach()
>>>> phy_state_machine()
>>>> -> netif_carrier_off()
>>>>
>>>> If phy_detach() finishes earlier than the workqueue had a chance to be
>>>> scheduled and process PHY_HALTED again, then we trigger the NULL pointer
>>>> de-reference.
>>>>
>>>> workqueues are not tasklets, the CPU scheduling them gets no guarantee
>>>> they will run on the same CPU.
>>>
>>> Something does not add up.
>>>
>>> The synchronous call to phy_state_machine() does:
>>>
>>> case PHY_HALTED:
>>> if (phydev->link) {
>>> phydev->link = 0;
>>> netif_carrier_off(phydev->attached_dev);
>>> phy_adjust_link(phydev);
>>> do_suspend = true;
>>> }
>>>
>>> then sets phydev->link = 0; therefore subsequent calls to
>>> phy_state_machin() will be no-op.
>>
>> Actually you are right, once phydev->link is set to 0 these would become
>> no-ops. Still scratching my head as to what happens for David then...
>>
>>>
>>> Also, queue_delayed_work() is only called in polling mode.
>>> David stated that he's using interrupt mode.
>
> Did you see what I wrote?
>
> phy_disconnect() calls phy_stop_interrupts() which puts it into polling
> mode. So the polling work gets queued unconditionally.
I did address that remark in
https://www.mail-archive.com/netdev@vger.kernel.org/msg186336.html
int phy_stop_interrupts(struct phy_device *phydev)
{
int err = phy_disable_interrupts(phydev);
if (err)
phy_error(phydev);
free_irq(phydev->irq, phydev);
/* If work indeed has been cancelled, disable_irq() will have
* been left unbalanced from phy_interrupt() and enable_irq()
* has to be called so that other devices on the line work.
*/
while (atomic_dec_return(&phydev->irq_disable) >= 0)
enable_irq(phydev->irq);
return err;
}
Which part of this function changes phydev->irq to PHY_POLL?
Perhaps phydev->drv->config_intr?
What PHY are you using?
Regards.
^ permalink raw reply
* Re: [pull request][net-next 0/3] Mellanox, mlx5 GRE tunnel offloads
From: Tom Herbert @ 2017-09-06 19:01 UTC (permalink / raw)
To: Alexander Duyck
Cc: Hannes Frederic Sowa, Saeed Mahameed, Saeed Mahameed,
David S. Miller, Linux Netdev List
In-Reply-To: <CAKgT0UeK0njpy1unJUV+=-JGOHTEZCJu0mNd-UuX8aO2G2zKmQ@mail.gmail.com>
On Wed, Sep 6, 2017 at 10:43 AM, Alexander Duyck
<alexander.duyck@gmail.com> wrote:
> On Wed, Sep 6, 2017 at 9:17 AM, Tom Herbert <tom@herbertland.com> wrote:
>> On Tue, Sep 5, 2017 at 8:06 PM, Alexander Duyck
>> <alexander.duyck@gmail.com> wrote:
>>> On Tue, Sep 5, 2017 at 2:13 PM, Tom Herbert <tom@herbertland.com> wrote:
>>>>> The situation with encapsulation is even more complicated:
>>>>>
>>>>> We are basically only interested in the UDP/vxlan/Ethernet/IP/UDP
>>>>> constellation. If we do the fragmentation inside the vxlan tunnel and
>>>>> carry over the skb hash to all resulting UDP/vxlan packets source ports,
>>>>> we are fine and reordering on the receiver NIC won't happen in this
>>>>> case. If the fragmentation happens on the outer UDP header, this will
>>>>> result in reordering of the inner L2 flow. Unfortunately this depends on
>>>>> how the vxlan tunnel was set up, how other devices do that and (I
>>>>> believe so) on the kernel version.
>>>>>
>>>> This really isn't that complicated. The assumption that an IP network
>>>> always delivers packets in order is simply wrong. The inventors of
>>>> VXLAN must have know full well that when you use IP, packets can and
>>>> eventually will be delivered out of order. This isn't just because of
>>>> fragmentation, there are many other reasons that packets can be
>>>> delivered OOO. This also must have been known when IP/GRE and any
>>>> other protocol that carries L2 over IP was invented. If OOO is an
>>>> issue for these protocols then they need to be fixed-- this is not a
>>>> concern with IP protocol nor the stack.
>>>>
>>>> Tom
>>>
>>> As far as a little background on the original patch I believe the
>>> issue that was fixed by the patch was a video streaming application
>>> that was sending/receiving a mix of fragmented and non-fragmented
>>> packets. Receiving them out of order due to the fragmentation was
>>> causing issues with stutters in the video and so we ended up disabling
>>> UDP by default in the NICs listed. We decided to go that way as UDP
>>> RSS was viewed as a performance optimization, while the out-of-order
>>> problems were viewed as a functionality issue.
>>>
>> Hi Alex,
>>
>> Thanks for the details! Were you able to find the root cause for this?
>> In particular, it would be interesting to know if it is the kernel or
>> device that introduced the jitter, or if it's the application that
>> doesn't handle OOO well...
>>
>> Tom
>
> It is hard to say since my memory of the events from 7 years ago is
> pretty vague at this point, but I'm pretty sure it was the
> application. Basically getting the frames out of order was causing
> them to have to drop video data if I recall correctly.
>
Oh, I didn't notice that patch was from 2010. Maybe the application
has been fixed by now! :-)
Perhaps, it's time to try to turn UDP hashing on again by default?
Even if NICs aren't doing this, there are network devices that are
looking at UDP for ECMP and that doesn't seem to be causing widespread
problems currently...
Tom
^ permalink raw reply
* Re: [PATCH net] Revert "net: phy: Correctly process PHY_HALTED in phy_stop_machine()"
From: Florian Fainelli @ 2017-09-06 18:59 UTC (permalink / raw)
To: David Daney, Mason
Cc: Marc Gonzalez, netdev, Geert Uytterhoeven, David Miller,
Andrew Lunn, Mans Rullgard
In-Reply-To: <ff070239-28b7-d41b-8abe-c9f810561372@caviumnetworks.com>
On 09/06/2017 11:00 AM, David Daney wrote:
> On 08/31/2017 11:29 AM, Florian Fainelli wrote:
>> On 08/31/2017 11:12 AM, Mason wrote:
>>> On 31/08/2017 19:53, Florian Fainelli wrote:
>>>> On 08/31/2017 10:49 AM, Mason wrote:
>>>>> On 31/08/2017 18:57, Florian Fainelli wrote:
>>>>>> And the race is between phy_detach() setting phydev->attached_dev
>>>>>> = NULL
>>>>>> and phy_state_machine() running in PHY_HALTED state and calling
>>>>>> netif_carrier_off().
>>>>>
>>>>> I must be missing something.
>>>>> (Since a thread cannot race against itself.)
>>>>>
>>>>> phy_disconnect calls phy_stop_machine which
>>>>> 1) stops the work queue from running in a separate thread
>>>>> 2) calls phy_state_machine *synchronously*
>>>>> which runs the PHY_HALTED case with everything well-defined
>>>>> end of phy_stop_machine
>>>>>
>>>>> phy_disconnect only then calls phy_detach()
>>>>> which makes future calls of phy_state_machine perilous.
>>>>>
>>>>> This all happens in the same thread, so I'm not yet
>>>>> seeing where the race happens?
>>>>
>>>> The race is as described in David's earlier email, so let's recap:
>>>>
>>>> Thread 1 Thread 2
>>>> phy_disconnect()
>>>> phy_stop_interrupts()
>>>> phy_stop_machine()
>>>> phy_state_machine()
>>>> -> queue_delayed_work()
>>>> phy_detach()
>>>> phy_state_machine()
>>>> -> netif_carrier_off()
>>>>
>>>> If phy_detach() finishes earlier than the workqueue had a chance to be
>>>> scheduled and process PHY_HALTED again, then we trigger the NULL
>>>> pointer
>>>> de-reference.
>>>>
>>>> workqueues are not tasklets, the CPU scheduling them gets no guarantee
>>>> they will run on the same CPU.
>>>
>>> Something does not add up.
>>>
>>> The synchronous call to phy_state_machine() does:
>>>
>>> case PHY_HALTED:
>>> if (phydev->link) {
>>> phydev->link = 0;
>>> netif_carrier_off(phydev->attached_dev);
>>> phy_adjust_link(phydev);
>>> do_suspend = true;
>>> }
>>>
>>> then sets phydev->link = 0; therefore subsequent calls to
>>> phy_state_machin() will be no-op.
>>
>> Actually you are right, once phydev->link is set to 0 these would become
>> no-ops. Still scratching my head as to what happens for David then...
>>
>>>
>>> Also, queue_delayed_work() is only called in polling mode.
>>> David stated that he's using interrupt mode.
>
> Did you see what I wrote?
Still not following, see below.
>
> phy_disconnect() calls phy_stop_interrupts() which puts it into polling
> mode. So the polling work gets queued unconditionally.
What part of phy_stop_interrupts() is responsible for changing
phydev->irq to PHY_POLL? free_irq() cannot touch phydev->irq otherwise
subsequent request_irq() calls won't work anymore.
phy_disable_interrupts() only calls back into the PHY driver to
acknowledge and clear interrupts.
If we were using a PHY with PHY_POLL, as Marc said, the first
synchronous call to phy_state_machine() would have acted on PHY_HALTED
and even if we incorrectly keep re-scheduling the state machine from
PHY_HALTED to PHY_HALTED the second time around nothing can happen.
What are we missing here?
>
>
>
>>
>> Right that's confusing too now. David can you check if you tree has:
>>
>> 49d52e8108a21749dc2114b924c907db43358984 ("net: phy: handle state
>> correctly in phy_stop_machine")
>>
>
> Yes, I am using the 4.9 stable branch, and that commit was also present.
Thanks for checking that.
--
Florian
^ permalink raw reply
* Re: [PATCH net-next] xdp: implement xdp_redirect_map for generic XDP
From: Daniel Borkmann @ 2017-09-06 18:51 UTC (permalink / raw)
To: John Fastabend, Jesper Dangaard Brouer
Cc: netdev, David S. Miller, Andy Gospodarek
In-Reply-To: <59B0418A.5090609@gmail.com>
On 09/06/2017 08:42 PM, John Fastabend wrote:
> On 09/06/2017 11:18 AM, Jesper Dangaard Brouer wrote:
>> On Wed, 06 Sep 2017 18:24:07 +0200
>> Daniel Borkmann <daniel@iogearbox.net> wrote:
>>> On 09/06/2017 05:26 PM, Jesper Dangaard Brouer wrote:
>>>> Using bpf_redirect_map is allowed for generic XDP programs, but the
>>>> appropriate map lookup was never performed in xdp_do_generic_redirect().
>>>>
>>>> Instead the map-index is directly used as the ifindex. For the
>>>
>>> Good point, but ...
>>>
>>> [...]
>>>> net/core/filter.c | 29 +++++++++++++++++++++++++++++
>>>> 1 file changed, 29 insertions(+)
>>>>
>>>> diff --git a/net/core/filter.c b/net/core/filter.c
>>>> index 5912c738a7b2..6a4745bf2c9f 100644
>>>> --- a/net/core/filter.c
>>>> +++ b/net/core/filter.c
>>>> @@ -2562,6 +2562,32 @@ int xdp_do_redirect(struct net_device *dev, struct xdp_buff *xdp,
>>>> }
>>>> EXPORT_SYMBOL_GPL(xdp_do_redirect);
>>>>
>>>> +static int xdp_do_generic_redirect_map(struct net_device *dev,
>>>> + struct sk_buff *skb,
>>>> + struct bpf_prog *xdp_prog)
>>>> +{
>>>> + struct redirect_info *ri = this_cpu_ptr(&redirect_info);
>>>> + struct bpf_map *map = ri->map;
>>>> + u32 index = ri->ifindex;
>>>> + struct net_device *fwd;
>>>> + int err;
>>>> +
>>>> + ri->ifindex = 0;
>>>> + ri->map = NULL;
>>>> +
>>>> + fwd = __dev_map_lookup_elem(map, index);
>>>> + if (!fwd) {
>>>> + err = -EINVAL;
>>>> + goto err;
>>>> + }
>>>> + skb->dev = fwd;
>>>> + _trace_xdp_redirect_map(dev, xdp_prog, fwd, map, index);
>>>> + return 0;
>>>> +err:
>>>> + _trace_xdp_redirect_map_err(dev, xdp_prog, fwd, map, index, err);
>>>> + return err;
>>>> +}
>>>> +
>>>> int xdp_do_generic_redirect(struct net_device *dev, struct sk_buff *skb,
>>>> struct bpf_prog *xdp_prog)
>>>> {
>>>> @@ -2571,6 +2597,9 @@ int xdp_do_generic_redirect(struct net_device *dev, struct sk_buff *skb,
>>>> unsigned int len;
>>>> int err = 0;
>>>>
>>>> + if (ri->map)
>>>> + return xdp_do_generic_redirect_map(dev, skb, xdp_prog);
>>>
>>> This is not quite correct. Really, the only thing you want
>>> to do here is more or less ...
>>>
>>> int xdp_do_generic_redirect(struct net_device *dev, struct sk_buff *skb,
>>> struct bpf_prog *xdp_prog)
>>> {
>>> struct redirect_info *ri = this_cpu_ptr(&redirect_info);
>>> struct bpf_map *map = ri->map;
>>> u32 index = ri->ifindex;
>>> struct net_device *fwd;
>>> unsigned int len;
>>> int err = 0;
>>>
>>> ri->ifindex = 0;
>>> ri->map = NULL;
>>>
>>> if (map)
>>> fwd = __dev_map_lookup_elem(map, index);
>>> else
>>> fwd = dev_get_by_index_rcu(dev_net(dev), index);
>>> if (unlikely(!fwd)) {
>>> err = -EINVAL;
>>> goto err;
>>> }
>>> [...]
>>>
>>> ... such that you have a common path to also do the IFF_UP
>>> and MTU checks that are done here, but otherwise omitted in
>>> your patch.
>>
>> Ah, yes. My patch miss the IFF_UP and MTU check. (I was too inspired by
>> xdp_do_redirect_map).
>>
>>> Otherwise it looks good, but note that it also doesn't really
>>> resolve the issue you mention wrt stale map pointers by the
>>> way. [...]
>>
>> I actually discovered more cases where we can crash the kernel :-(
>>
>> E.g. driver not supporting XDP_REDIRECT, are still allowed to load an
>> XDP bpf_prog that call bpf_redirect_map() and set the ->map pointer,
>> but it will never call xdp_do_redirect() (which is responsible for
>> clearing/consuming ->map pointer).
>>
>> Another case: You can also call bpf_redirect_map() and then NOT return
>> XDP_REDIRECT (it is obviously strange, but the bpf-helper API allows it).
>
> I think we can cover both these cases with previous suggestion to check
> prog pointers. Working up a patch now.
Yep, they would both be covered.
^ permalink raw reply
* Re: [PATCH net-next] xdp: implement xdp_redirect_map for generic XDP
From: John Fastabend @ 2017-09-06 18:42 UTC (permalink / raw)
To: Jesper Dangaard Brouer, Daniel Borkmann
Cc: netdev, David S. Miller, Andy Gospodarek
In-Reply-To: <20170906201835.7bc26f4f@redhat.com>
On 09/06/2017 11:18 AM, Jesper Dangaard Brouer wrote:
> On Wed, 06 Sep 2017 18:24:07 +0200
> Daniel Borkmann <daniel@iogearbox.net> wrote:
>
>> On 09/06/2017 05:26 PM, Jesper Dangaard Brouer wrote:
>>> Using bpf_redirect_map is allowed for generic XDP programs, but the
>>> appropriate map lookup was never performed in xdp_do_generic_redirect().
>>>
>>> Instead the map-index is directly used as the ifindex. For the
>>
>> Good point, but ...
>>
>> [...]
>>> net/core/filter.c | 29 +++++++++++++++++++++++++++++
>>> 1 file changed, 29 insertions(+)
>>>
>>> diff --git a/net/core/filter.c b/net/core/filter.c
>>> index 5912c738a7b2..6a4745bf2c9f 100644
>>> --- a/net/core/filter.c
>>> +++ b/net/core/filter.c
>>> @@ -2562,6 +2562,32 @@ int xdp_do_redirect(struct net_device *dev, struct xdp_buff *xdp,
>>> }
>>> EXPORT_SYMBOL_GPL(xdp_do_redirect);
>>>
>>> +static int xdp_do_generic_redirect_map(struct net_device *dev,
>>> + struct sk_buff *skb,
>>> + struct bpf_prog *xdp_prog)
>>> +{
>>> + struct redirect_info *ri = this_cpu_ptr(&redirect_info);
>>> + struct bpf_map *map = ri->map;
>>> + u32 index = ri->ifindex;
>>> + struct net_device *fwd;
>>> + int err;
>>> +
>>> + ri->ifindex = 0;
>>> + ri->map = NULL;
>>> +
>>> + fwd = __dev_map_lookup_elem(map, index);
>>> + if (!fwd) {
>>> + err = -EINVAL;
>>> + goto err;
>>> + }
>>> + skb->dev = fwd;
>>> + _trace_xdp_redirect_map(dev, xdp_prog, fwd, map, index);
>>> + return 0;
>>> +err:
>>> + _trace_xdp_redirect_map_err(dev, xdp_prog, fwd, map, index, err);
>>> + return err;
>>> +}
>>> +
>>> int xdp_do_generic_redirect(struct net_device *dev, struct sk_buff *skb,
>>> struct bpf_prog *xdp_prog)
>>> {
>>> @@ -2571,6 +2597,9 @@ int xdp_do_generic_redirect(struct net_device *dev, struct sk_buff *skb,
>>> unsigned int len;
>>> int err = 0;
>>>
>>> + if (ri->map)
>>> + return xdp_do_generic_redirect_map(dev, skb, xdp_prog);
>>
>> This is not quite correct. Really, the only thing you want
>> to do here is more or less ...
>>
>> int xdp_do_generic_redirect(struct net_device *dev, struct sk_buff *skb,
>> struct bpf_prog *xdp_prog)
>> {
>> struct redirect_info *ri = this_cpu_ptr(&redirect_info);
>> struct bpf_map *map = ri->map;
>> u32 index = ri->ifindex;
>> struct net_device *fwd;
>> unsigned int len;
>> int err = 0;
>>
>> ri->ifindex = 0;
>> ri->map = NULL;
>>
>> if (map)
>> fwd = __dev_map_lookup_elem(map, index);
>> else
>> fwd = dev_get_by_index_rcu(dev_net(dev), index);
>> if (unlikely(!fwd)) {
>> err = -EINVAL;
>> goto err;
>> }
>> [...]
>>
>> ... such that you have a common path to also do the IFF_UP
>> and MTU checks that are done here, but otherwise omitted in
>> your patch.
>
> Ah, yes. My patch miss the IFF_UP and MTU check. (I was too inspired by
> xdp_do_redirect_map).
>
>
>> Otherwise it looks good, but note that it also doesn't really
>> resolve the issue you mention wrt stale map pointers by the
>> way. [...]
>
> I actually discovered more cases where we can crash the kernel :-(
>
> E.g. driver not supporting XDP_REDIRECT, are still allowed to load an
> XDP bpf_prog that call bpf_redirect_map() and set the ->map pointer,
> but it will never call xdp_do_redirect() (which is responsible for
> clearing/consuming ->map pointer).
>
> Another case: You can also call bpf_redirect_map() and then NOT return
> XDP_REDIRECT (it is obviously strange, but the bpf-helper API allows it).
>
I think we can cover both these cases with previous suggestion to check
prog pointers. Working up a patch now.
^ permalink raw reply
* Re: [PATCH v2 rfc 0/8] IGMP snooping for local traffic
From: Vivien Didelot @ 2017-09-06 18:29 UTC (permalink / raw)
To: Andrew Lunn; +Cc: netdev, jiri, nikolay, Florian Fainelli
In-Reply-To: <20170906170120.GF15315@lunn.ch>
Hi Andrew,
Andrew Lunn <andrew@lunn.ch> writes:
>> I don't understand why SWITCHDEV_OBJ_ID_PORT_MDB cannot be used. Isn't
>> setting the obj->orig_dev to the bridge device itself enough to
>> distinguish br0 from a switch port?
>
> I did consider this. But conceptually, it seems wrong.
> SWITCHDEV_OBJ_ID_PORT_MDB has always been about egress. I don't like
> adding a special case for ingress. Adding a new switchdev object
> avoids this special case.
SWITCHDEV_OBJ_ID_PORT_MDB means manipulating an MDB entry on a port.
When one want to add an MDB entry to br0, SWITCHDEV_OBJ_ID_PORT_MDB is
used. If the device is br->dev, the lower devices (bridged ports) get
recursively notified and switchdev users can program themselves
accordingly. In the case of DSA, a slave port will program its
associated CPU port (port->cpu_dp) if orig_dev is a bridge.
This seems totally correct to me. I don't see any reason for adding and
maintaining a new switchdev object. What do switchdev guys think?
>> The main problem is that we will soon want support for multiple CPU
>> ports.
>
> We keep saying that, but it never actually happens. We should solve
> multiple CPU problems when we actually add support for multiple CPUs.
> Probably at that point, we need to push SWITCHDEV_OBJ_ID_PORT_HOST_MDB
> right down to a port, so that port can setup what needs setting up so
> its frames goes to its CPU port.
This is not correct. I am currently making this easier, i.e. the
dsa_master patchset I sent before net-next closed.
I do understand your point however and even if this multi-CPU feature
takes time to arrive, we can always find a proper design which makes it
easy. Assuming that each port has its dedicated CPU port is the correct
concept to follow.
>> So adding the MDB entry to all CPU ports as you did in a patch 5/8 in
>> dsa_switch_host_mdb_*() is not correct because you can have CPU port
>> sw0p0 being a member of br0 and CPU port sw0p10 being a member of br1.
>
> Be careful, you are making assumptions about mapping cpu ports to
> bridges. That is not been defined yet.
I am not making any assumptions, but you did because you assume that all
CPU ports will be part of the same bridge.
We need to handle things at the DSA core level the following way:
If one programs the bridge device itself, a switchdev object is
notified, and each DSA slave devices member of the bridge will call the
correct dsa_port_* function (agnostic to the port type) with its cpu_dp.
This covers cleanly all cases. You also don't need any new DSA notifier.
You only need your 6/8 patch. To acheive that, there is two ways:
1) either we keep SWITCHDEV_OBJ_ID_PORT_MDB for the bridge device itself
and the slave devices are notified accordingly with a correct orig_dev:
static int dsa_slave_port_obj_add(struct net_device *dev,
const struct switchdev_obj *obj,
struct switchdev_trans *trans)
{
struct dsa_slave_priv *p = netdev_priv(dev);
struct dsa_port *port = p->dp;
/* Program the CPU port if the target is the bridge itself */
if (obj->orig_dev == port->br)
port = port->cpu_dp;
switch (obj->id) {
case SWITCHDEV_OBJ_ID_PORT_MDB:
return dsa_port_mdb_add(port, obj, trans);
...
}
}
2) or you introduce a switchdev object which, by definition, targets the
CPU ports of our bridge members:
static int dsa_slave_port_obj_add(struct net_device *dev,
const struct switchdev_obj *obj,
struct switchdev_trans *trans)
{
struct dsa_slave_priv *p = netdev_priv(dev);
struct dsa_port *port = p->dp;
switch (obj->id) {
case SWITCHDEV_OBJ_ID_PORT_HOST_MDB:
port = port->cpu_dp;
/* fall through... */
case SWITCHDEV_OBJ_ID_PORT_MDB:
return dsa_port_mdb_add(port, obj, trans);
...
}
}
You basically just need that and your 6/8 patch for the DSA part.
Thanks,
Vivien
^ permalink raw reply
* Re: [PATCH net-next] xdp: implement xdp_redirect_map for generic XDP
From: Jesper Dangaard Brouer @ 2017-09-06 18:18 UTC (permalink / raw)
To: Daniel Borkmann
Cc: netdev, David S. Miller, John Fastabend, Andy Gospodarek, brouer
In-Reply-To: <59B02127.5020904@iogearbox.net>
On Wed, 06 Sep 2017 18:24:07 +0200
Daniel Borkmann <daniel@iogearbox.net> wrote:
> On 09/06/2017 05:26 PM, Jesper Dangaard Brouer wrote:
> > Using bpf_redirect_map is allowed for generic XDP programs, but the
> > appropriate map lookup was never performed in xdp_do_generic_redirect().
> >
> > Instead the map-index is directly used as the ifindex. For the
>
> Good point, but ...
>
> [...]
> > net/core/filter.c | 29 +++++++++++++++++++++++++++++
> > 1 file changed, 29 insertions(+)
> >
> > diff --git a/net/core/filter.c b/net/core/filter.c
> > index 5912c738a7b2..6a4745bf2c9f 100644
> > --- a/net/core/filter.c
> > +++ b/net/core/filter.c
> > @@ -2562,6 +2562,32 @@ int xdp_do_redirect(struct net_device *dev, struct xdp_buff *xdp,
> > }
> > EXPORT_SYMBOL_GPL(xdp_do_redirect);
> >
> > +static int xdp_do_generic_redirect_map(struct net_device *dev,
> > + struct sk_buff *skb,
> > + struct bpf_prog *xdp_prog)
> > +{
> > + struct redirect_info *ri = this_cpu_ptr(&redirect_info);
> > + struct bpf_map *map = ri->map;
> > + u32 index = ri->ifindex;
> > + struct net_device *fwd;
> > + int err;
> > +
> > + ri->ifindex = 0;
> > + ri->map = NULL;
> > +
> > + fwd = __dev_map_lookup_elem(map, index);
> > + if (!fwd) {
> > + err = -EINVAL;
> > + goto err;
> > + }
> > + skb->dev = fwd;
> > + _trace_xdp_redirect_map(dev, xdp_prog, fwd, map, index);
> > + return 0;
> > +err:
> > + _trace_xdp_redirect_map_err(dev, xdp_prog, fwd, map, index, err);
> > + return err;
> > +}
> > +
> > int xdp_do_generic_redirect(struct net_device *dev, struct sk_buff *skb,
> > struct bpf_prog *xdp_prog)
> > {
> > @@ -2571,6 +2597,9 @@ int xdp_do_generic_redirect(struct net_device *dev, struct sk_buff *skb,
> > unsigned int len;
> > int err = 0;
> >
> > + if (ri->map)
> > + return xdp_do_generic_redirect_map(dev, skb, xdp_prog);
>
> This is not quite correct. Really, the only thing you want
> to do here is more or less ...
>
> int xdp_do_generic_redirect(struct net_device *dev, struct sk_buff *skb,
> struct bpf_prog *xdp_prog)
> {
> struct redirect_info *ri = this_cpu_ptr(&redirect_info);
> struct bpf_map *map = ri->map;
> u32 index = ri->ifindex;
> struct net_device *fwd;
> unsigned int len;
> int err = 0;
>
> ri->ifindex = 0;
> ri->map = NULL;
>
> if (map)
> fwd = __dev_map_lookup_elem(map, index);
> else
> fwd = dev_get_by_index_rcu(dev_net(dev), index);
> if (unlikely(!fwd)) {
> err = -EINVAL;
> goto err;
> }
> [...]
>
> ... such that you have a common path to also do the IFF_UP
> and MTU checks that are done here, but otherwise omitted in
> your patch.
Ah, yes. My patch miss the IFF_UP and MTU check. (I was too inspired by
xdp_do_redirect_map).
> Otherwise it looks good, but note that it also doesn't really
> resolve the issue you mention wrt stale map pointers by the
> way. [...]
I actually discovered more cases where we can crash the kernel :-(
E.g. driver not supporting XDP_REDIRECT, are still allowed to load an
XDP bpf_prog that call bpf_redirect_map() and set the ->map pointer,
but it will never call xdp_do_redirect() (which is responsible for
clearing/consuming ->map pointer).
Another case: You can also call bpf_redirect_map() and then NOT return
XDP_REDIRECT (it is obviously strange, but the bpf-helper API allows it).
--
Best regards,
Jesper Dangaard Brouer
MSc.CS, Principal Kernel Engineer at Red Hat
LinkedIn: http://www.linkedin.com/in/brouer
^ permalink raw reply
* Re: [PATCH net-next RFC 2/2] tun: enable napi_gro_frags() for TUN/TAP driver
From: Eric Dumazet @ 2017-09-06 18:01 UTC (permalink / raw)
To: Petar Penkov
Cc: netdev, Eric Dumazet, Mahesh Bandewar, Willem de Bruijn, davem,
ppenkov
In-Reply-To: <20170905223551.27925-3-ppenkov@google.com>
On Tue, 2017-09-05 at 15:35 -0700, Petar Penkov wrote:
> Add a TUN/TAP receive mode that exercises the napi_gro_frags()
> interface. This mode is available only in TAP mode, as the interface
> expects packets with Ethernet headers.
>
Hi Petar, thanks a lot for this work.
I must confess I have to retract one feedback I gave while reviewing
your patches.
> + local_bh_disable();
> + data = napi_alloc_frag(fragsz);
> + local_bh_enable();
> + if (!data) {
> + err = -ENOMEM;
> + goto free;
> + }
> +
> + page = virt_to_page(data);
> + offset = offset_in_page(data);
These two lines above indeed trigger too many problems in the kernel.
(Like the one you tried to cover here
https://patchwork.kernel.org/patch/9927927/ )
Please use for your next submission the code you originally had :
page = virt_to_head_page(data);
offset = data - page_address(page);
Thanks !
^ permalink raw reply
* Re: [PATCH net] Revert "net: phy: Correctly process PHY_HALTED in phy_stop_machine()"
From: David Daney @ 2017-09-06 18:00 UTC (permalink / raw)
To: Florian Fainelli, Mason
Cc: Marc Gonzalez, netdev, Geert Uytterhoeven, David Miller,
Andrew Lunn, Mans Rullgard
In-Reply-To: <4ea8b432-4968-1616-eff9-48a2689dd3ce@gmail.com>
On 08/31/2017 11:29 AM, Florian Fainelli wrote:
> On 08/31/2017 11:12 AM, Mason wrote:
>> On 31/08/2017 19:53, Florian Fainelli wrote:
>>> On 08/31/2017 10:49 AM, Mason wrote:
>>>> On 31/08/2017 18:57, Florian Fainelli wrote:
>>>>> And the race is between phy_detach() setting phydev->attached_dev = NULL
>>>>> and phy_state_machine() running in PHY_HALTED state and calling
>>>>> netif_carrier_off().
>>>>
>>>> I must be missing something.
>>>> (Since a thread cannot race against itself.)
>>>>
>>>> phy_disconnect calls phy_stop_machine which
>>>> 1) stops the work queue from running in a separate thread
>>>> 2) calls phy_state_machine *synchronously*
>>>> which runs the PHY_HALTED case with everything well-defined
>>>> end of phy_stop_machine
>>>>
>>>> phy_disconnect only then calls phy_detach()
>>>> which makes future calls of phy_state_machine perilous.
>>>>
>>>> This all happens in the same thread, so I'm not yet
>>>> seeing where the race happens?
>>>
>>> The race is as described in David's earlier email, so let's recap:
>>>
>>> Thread 1 Thread 2
>>> phy_disconnect()
>>> phy_stop_interrupts()
>>> phy_stop_machine()
>>> phy_state_machine()
>>> -> queue_delayed_work()
>>> phy_detach()
>>> phy_state_machine()
>>> -> netif_carrier_off()
>>>
>>> If phy_detach() finishes earlier than the workqueue had a chance to be
>>> scheduled and process PHY_HALTED again, then we trigger the NULL pointer
>>> de-reference.
>>>
>>> workqueues are not tasklets, the CPU scheduling them gets no guarantee
>>> they will run on the same CPU.
>>
>> Something does not add up.
>>
>> The synchronous call to phy_state_machine() does:
>>
>> case PHY_HALTED:
>> if (phydev->link) {
>> phydev->link = 0;
>> netif_carrier_off(phydev->attached_dev);
>> phy_adjust_link(phydev);
>> do_suspend = true;
>> }
>>
>> then sets phydev->link = 0; therefore subsequent calls to
>> phy_state_machin() will be no-op.
>
> Actually you are right, once phydev->link is set to 0 these would become
> no-ops. Still scratching my head as to what happens for David then...
>
>>
>> Also, queue_delayed_work() is only called in polling mode.
>> David stated that he's using interrupt mode.
Did you see what I wrote?
phy_disconnect() calls phy_stop_interrupts() which puts it into polling
mode. So the polling work gets queued unconditionally.
>
> Right that's confusing too now. David can you check if you tree has:
>
> 49d52e8108a21749dc2114b924c907db43358984 ("net: phy: handle state
> correctly in phy_stop_machine")
>
Yes, I am using the 4.9 stable branch, and that commit was also present.
David.
^ permalink raw reply
* Re: [PATCH net] Revert "net: phy: Correctly process PHY_HALTED in phy_stop_machine()"
From: David Daney @ 2017-09-06 17:53 UTC (permalink / raw)
To: Mason, Florian Fainelli
Cc: Marc Gonzalez, netdev, Geert Uytterhoeven, David Miller,
Andrew Lunn, Mans Rullgard, Thibaud Cornic
In-Reply-To: <6721135d-8c3f-57a0-f423-9d18cd6e0947@free.fr>
On 09/06/2017 07:33 AM, Mason wrote:
> On 31/08/2017 20:29, Florian Fainelli wrote:
>> On 08/31/2017 11:12 AM, Mason wrote:
>>> On 31/08/2017 19:53, Florian Fainelli wrote:
>>>> On 08/31/2017 10:49 AM, Mason wrote:
>>>>> On 31/08/2017 18:57, Florian Fainelli wrote:
>>>>>> And the race is between phy_detach() setting phydev->attached_dev
>>>>>> = NULL
>>>>>> and phy_state_machine() running in PHY_HALTED state and calling
>>>>>> netif_carrier_off().
>>>>>
>>>>> I must be missing something.
>>>>> (Since a thread cannot race against itself.)
>>>>>
>>>>> phy_disconnect calls phy_stop_machine which
>>>>> 1) stops the work queue from running in a separate thread
>>>>> 2) calls phy_state_machine *synchronously*
>>>>> which runs the PHY_HALTED case with everything well-defined
>>>>> end of phy_stop_machine
>>>>>
>>>>> phy_disconnect only then calls phy_detach()
>>>>> which makes future calls of phy_state_machine perilous.
>>>>>
>>>>> This all happens in the same thread, so I'm not yet
>>>>> seeing where the race happens?
>>>>
>>>> The race is as described in David's earlier email, so let's recap:
>>>>
>>>> Thread 1 Thread 2
>>>> phy_disconnect()
>>>> phy_stop_interrupts()
>>>> phy_stop_machine()
>>>> phy_state_machine()
>>>> -> queue_delayed_work()
>>>> phy_detach()
>>>> phy_state_machine()
>>>> -> netif_carrier_off()
>>>>
>>>> If phy_detach() finishes earlier than the workqueue had a chance to be
>>>> scheduled and process PHY_HALTED again, then we trigger the NULL
>>>> pointer
>>>> de-reference.
>>>>
>>>> workqueues are not tasklets, the CPU scheduling them gets no guarantee
>>>> they will run on the same CPU.
>>>
>>> Something does not add up.
>>>
>>> The synchronous call to phy_state_machine() does:
>>>
>>> case PHY_HALTED:
>>> if (phydev->link) {
>>> phydev->link = 0;
>>> netif_carrier_off(phydev->attached_dev);
>>> phy_adjust_link(phydev);
>>> do_suspend = true;
>>> }
>>>
>>> then sets phydev->link = 0; therefore subsequent calls to
>>> phy_state_machin() will be no-op.
>>
>> Actually you are right, once phydev->link is set to 0 these would become
>> no-ops. Still scratching my head as to what happens for David then...
>>
>>>
>>> Also, queue_delayed_work() is only called in polling mode.
>>> David stated that he's using interrupt mode.
>>
>> Right that's confusing too now. David can you check if you tree has:
>>
>> 49d52e8108a21749dc2114b924c907db43358984 ("net: phy: handle state
>> correctly in phy_stop_machine")
>
> Hello David,
>
> A week ago, you wrote about my patch:
> "This is broken. Please revert."
>
> I assume you tested the revert locally, and that reverting did make
> the crash disappear. Is that correct?
>
Yes, I always test things before making this type of assertion.
> The reason I ask is because the analysis you provided contains some
> flaws, as noted above. But, if reverting my patch did fix your issue,
> then perhaps understanding *why* is unimportant.
I didn't want to take the time to generate calling sequence traces to
verify each step of my analysis, but I believe the overall concept is
essentially correct.
Once the polling work is canceled and we set a bunch of essential
pointers to NULL, you cannot go blindly restarting the polling.
>
> I'm a bit baffled that it took less than 90 minutes for your request
> to be approved, and the patch reverted in all branches, before I even
> had a chance to comment.
>
o The last chance for patches to v4.13 was fast approaching.
o There were multiple reports of failures caused by the patch.
o The patch was clearly stand-alone.
The kernel maintainers are a model of efficiency, there was no reason to
delay.
^ permalink raw reply
* Re: [pull request][net-next 0/3] Mellanox, mlx5 GRE tunnel offloads
From: Alexander Duyck @ 2017-09-06 17:43 UTC (permalink / raw)
To: Tom Herbert
Cc: Hannes Frederic Sowa, Saeed Mahameed, Saeed Mahameed,
David S. Miller, Linux Netdev List
In-Reply-To: <CALx6S37eY4eWEtBhfGqqRxtr3B4T5qpuBThUa0yH0CKCj6j4_w@mail.gmail.com>
On Wed, Sep 6, 2017 at 9:17 AM, Tom Herbert <tom@herbertland.com> wrote:
> On Tue, Sep 5, 2017 at 8:06 PM, Alexander Duyck
> <alexander.duyck@gmail.com> wrote:
>> On Tue, Sep 5, 2017 at 2:13 PM, Tom Herbert <tom@herbertland.com> wrote:
>>>> The situation with encapsulation is even more complicated:
>>>>
>>>> We are basically only interested in the UDP/vxlan/Ethernet/IP/UDP
>>>> constellation. If we do the fragmentation inside the vxlan tunnel and
>>>> carry over the skb hash to all resulting UDP/vxlan packets source ports,
>>>> we are fine and reordering on the receiver NIC won't happen in this
>>>> case. If the fragmentation happens on the outer UDP header, this will
>>>> result in reordering of the inner L2 flow. Unfortunately this depends on
>>>> how the vxlan tunnel was set up, how other devices do that and (I
>>>> believe so) on the kernel version.
>>>>
>>> This really isn't that complicated. The assumption that an IP network
>>> always delivers packets in order is simply wrong. The inventors of
>>> VXLAN must have know full well that when you use IP, packets can and
>>> eventually will be delivered out of order. This isn't just because of
>>> fragmentation, there are many other reasons that packets can be
>>> delivered OOO. This also must have been known when IP/GRE and any
>>> other protocol that carries L2 over IP was invented. If OOO is an
>>> issue for these protocols then they need to be fixed-- this is not a
>>> concern with IP protocol nor the stack.
>>>
>>> Tom
>>
>> As far as a little background on the original patch I believe the
>> issue that was fixed by the patch was a video streaming application
>> that was sending/receiving a mix of fragmented and non-fragmented
>> packets. Receiving them out of order due to the fragmentation was
>> causing issues with stutters in the video and so we ended up disabling
>> UDP by default in the NICs listed. We decided to go that way as UDP
>> RSS was viewed as a performance optimization, while the out-of-order
>> problems were viewed as a functionality issue.
>>
> Hi Alex,
>
> Thanks for the details! Were you able to find the root cause for this?
> In particular, it would be interesting to know if it is the kernel or
> device that introduced the jitter, or if it's the application that
> doesn't handle OOO well...
>
> Tom
It is hard to say since my memory of the events from 7 years ago is
pretty vague at this point, but I'm pretty sure it was the
application. Basically getting the frames out of order was causing
them to have to drop video data if I recall correctly.
- Alex
^ permalink raw reply
* Re: [patch net] net: sched: fix memleak for chain zero
From: Cong Wang @ 2017-09-06 17:40 UTC (permalink / raw)
To: Jiri Pirko
Cc: Linux Kernel Network Developers, David Miller, Jamal Hadi Salim,
Jakub Kicinski, mlxsw
In-Reply-To: <20170906111419.5115-1-jiri@resnulli.us>
On Wed, Sep 6, 2017 at 4:14 AM, Jiri Pirko <jiri@resnulli.us> wrote:
> From: Jiri Pirko <jiri@mellanox.com>
>
> There's a memleak happening for chain 0. The thing is, chain 0 needs to
> be always present, not created on demand. Therefore tcf_block_get upon
> creation of block calls the tcf_chain_create function directly. The
> chain is created with refcnt == 1, which is not correct in this case and
> causes the memleak. So move the refcnt increment into tcf_chain_get
> function even for the case when chain needs to be created.
>
Your approach could work but you just make the code even
uglier than it is now:
1. The current code is already ugly for special-casing chain 0:
if (--chain->refcnt == 0 && !chain->filter_chain && chain->index != 0)
tcf_chain_destroy(chain);
2. With your patch, chain 0 has a different _initial_ refcnt with others.
3. Allowing an object (chain 0) exists with refcnt==0
Compare it with my patch:
1. No special-case for chain 0, the above ugly part is removed
2. Every chain is equal and created with refcnt==1
3. Any chain with refcnt==0 is destroyed
^ permalink raw reply
* Re: [patch net] net: sched: fix memleak for chain zero
From: Jakub Kicinski @ 2017-09-06 17:38 UTC (permalink / raw)
To: Jiri Pirko; +Cc: netdev, davem, jhs, xiyou.wangcong, mlxsw
In-Reply-To: <20170906111419.5115-1-jiri@resnulli.us>
On Wed, 6 Sep 2017 13:14:19 +0200, Jiri Pirko wrote:
> From: Jiri Pirko <jiri@mellanox.com>
>
> There's a memleak happening for chain 0. The thing is, chain 0 needs to
> be always present, not created on demand. Therefore tcf_block_get upon
> creation of block calls the tcf_chain_create function directly. The
> chain is created with refcnt == 1, which is not correct in this case and
> causes the memleak. So move the refcnt increment into tcf_chain_get
> function even for the case when chain needs to be created.
>
> Reported-by: Jakub Kicinski <kubakici@wp.pl>
> Fixes: 5bc1701881e3 ("net: sched: introduce multichain support for filters")
> Signed-off-by: Jiri Pirko <jiri@mellanox.com>
Tested-by: Jakub Kicinski <jakub.kicinski@netronome.com>
^ permalink raw reply
* RE: [PATCH net 3/4] lan78xx: Fix for eeprom read/write when device autosuspend
From: Nisar.Sayed @ 2017-09-06 17:34 UTC (permalink / raw)
To: andrew; +Cc: davem, UNGLinuxDriver, netdev
In-Reply-To: <20170906140605.GD11864@lunn.ch>
Thanks, will make separate patch.
> Hi Nisar
>
> > + else if ((ee->magic == LAN78XX_EEPROM_MAGIC) &&
> > + (ee->offset >= 0 && ee->offset < MAX_EEPROM_SIZE) &&
> > + (ee->len > 0 && (ee->offset + ee->len) <=
> MAX_EEPROM_SIZE))
> > + ret = lan78xx_write_raw_eeprom(dev, ee->offset, ee->len,
> data);
>
> This change does not appear to have anything to do with auto suspend.
> Please make it a separate patch.
>
> Andrew
^ permalink raw reply
* RE: [PATCH net 2/4] lan78xx: Add fixed_phy device support for LAN7801 device
From: Nisar.Sayed @ 2017-09-06 17:34 UTC (permalink / raw)
To: andrew; +Cc: davem, UNGLinuxDriver, netdev
In-Reply-To: <20170906140233.GC11864@lunn.ch>
Thanks Andrew, will try to change as suggested.
> On Wed, Sep 06, 2017 at 10:51:44AM +0000, Nisar.Sayed@microchip.com
> wrote:
> > From: Nisar Sayed <Nisar.Sayed@microchip.com>
> >
> > Add fixed_phy device support for LAN7801 device
> >
> > When LAN7801 device connected to PHY Device which does not have
> > MDIO/MDC access, fixex_phy device will be added.
>
> Please try to find a way to do this without all the #ifdefs. They can be
> acceptable in header files, but should be avoided in .c files.
>
> Andrew
^ permalink raw reply
* RE: [PATCH net 1/4] lan78xx: Fix for crash associated with System suspend
From: Nisar.Sayed @ 2017-09-06 17:34 UTC (permalink / raw)
To: andrew; +Cc: davem, UNGLinuxDriver, netdev
In-Reply-To: <20170906135908.GB11864@lunn.ch>
Thanks Andrew inputs.
> On Wed, Sep 06, 2017 at 10:51:31AM +0000, Nisar.Sayed@microchip.com
> wrote:
> > From: Nisar Sayed <Nisar.Sayed@microchip.com>
> >
> > Fix for crash associated with System suspend
> >
> > Since ndo_stop removes phydev which makes phydev NULL.
> > Whenever system suspend is initiated or after "ifconfig <interface>
> > down", if set_wol or get_wol is triggered phydev is NULL leads system
> crash.
> > Hence phy_start/phy_stop for ndo_start/ndo_stop fixes the issues
> > instead of adding/removing phydevice
>
> Looking at this patch, there apears to be lots of different things going on.
> Please can you split it up into multiple patches.
Sure will split it up.
>
> > Signed-off-by: Nisar Sayed <Nisar.Sayed@microchip.com>
> > ---
> > drivers/net/usb/lan78xx.c | 44
> > ++++++++++++++++++++++++++++----------------
> > 1 file changed, 28 insertions(+), 16 deletions(-)
> >
> > diff --git a/drivers/net/usb/lan78xx.c b/drivers/net/usb/lan78xx.c
> > index b99a7fb..955ab3b 100644
> > --- a/drivers/net/usb/lan78xx.c
> > +++ b/drivers/net/usb/lan78xx.c
> > @@ -2024,6 +2024,8 @@ static int lan78xx_phy_init(struct lan78xx_net
> *dev)
> > lan8835_fixup);
> > if (ret < 0) {
> > netdev_err(dev->net, "fail to register fixup\n");
> > + phy_unregister_fixup_for_uid(PHY_KSZ9031RNX,
> > + 0xfffffff0);
>
> goto error; would be better. phy_unregister_fixup_for_uid() does not care if
> you try to unregister something which has not been registered.
>
> Also, this should be a separate patch.
Ok, will make it as separate patch
>
> > return ret;
> > }
> > /* add more external PHY fixup here if needed */ @@ -
> 2031,8 +2033,7
> > @@ static int lan78xx_phy_init(struct lan78xx_net *dev)
> > phydev->is_internal = false;
> > } else {
> > netdev_err(dev->net, "unknown ID found\n");
> > - ret = -EIO;
> > - goto error;
> > + return -EIO;
> > }
> >
> > /* if phyirq is not set, use polling mode in phylib */ @@ -2051,7
> > +2052,10 @@ static int lan78xx_phy_init(struct lan78xx_net *dev)
> > if (ret) {
> > netdev_err(dev->net, "can't attach PHY to %s\n",
> > dev->mdiobus->id);
> > - return -EIO;
> > + ret = -EIO;
> > + if (dev->chipid == ID_REV_CHIP_ID_7801_)
> > + goto error;
> > + return ret;
>
> Why not add the if (dev->chipid == ID_REV_CHIP_ID_7801_) after the
> error: label?
>
>
Yes, will correct it
>
> > }
> >
> > /* MAC doesn't support 1000T Half */ @@ -2067,8 +2071,6 @@ static
> > int lan78xx_phy_init(struct lan78xx_net *dev)
> >
> > dev->fc_autoneg = phydev->autoneg;
> >
> > - phy_start(phydev);
> > -
> > netif_dbg(dev, ifup, dev->net, "phy initialised successfully");
> >
> > return 0;
> > @@ -2497,9 +2499,9 @@ static int lan78xx_open(struct net_device *net)
> > if (ret < 0)
> > goto done;
> >
> > - ret = lan78xx_phy_init(dev);
> > - if (ret < 0)
> > - goto done;
> > + if (dev->domain_data.phyirq > 0)
> > + phy_start_interrupts(dev->net->phydev);
>
> This is unusual. I don't see any other MAC driver starting interrupts.
> This needs explaining.
>
> Andrew
Since "lan78xx_open" calls "lan78xx_reset" (Device reset) it is required to start/enable interrupt back
Initially when "phydev->state = PHY_READY" state "phy_start" will not enable interrupts,
However after "lan78xx_stop" when "phy_stop" makes "phydev->state = PHY_HALTED"
Subsequent call to "phy_start" will enable interrupt.
Hence "phy_start_interrupts" used after "lan78xx_reset"
- Nisar
^ permalink raw reply
* Re: [Patch net] net_sched: fix a memory leak of filter chain
From: Cong Wang @ 2017-09-06 17:25 UTC (permalink / raw)
To: Jiri Pirko; +Cc: Linux Kernel Network Developers, Jakub Kicinski, Jiri Pirko
In-Reply-To: <20170906073839.GD2523@nanopsycho>
On Wed, Sep 6, 2017 at 12:38 AM, Jiri Pirko <jiri@resnulli.us> wrote:
> Wed, Sep 06, 2017 at 07:03:10AM CEST, xiyou.wangcong@gmail.com wrote:
>>tcf_chain_destroy() is called by tcf_block_put() and tcf_chain_put().
>>tcf_chain_put() is refcn'ed and paired with tcf_chain_get(),
>>but tcf_block_put() is not, it should be paired with tcf_block_get()
>>and we still need to decrease the refcnt. However, tcf_block_put()
>>is special, it stores the chains too, we have to detach them if
>>it is not the last user.
>
> You don't describe the original issue, or I am missing that from your
> description.
The original issue is the mismatch of tcf_block_put() and tcf_block_get()
w.r.t. refcnt. Think it in this way: if you call tcf_bock_put() immediately
after tcf_block_get(), would you get effectively a nop?
>
>
>>
>>What's more, index 0 is not special at all, it should be treated
>>like other chains. This also makes the code more readable.
>
> [...]
>
>
>>@@ -246,10 +246,7 @@ EXPORT_SYMBOL(tcf_chain_get);
>>
>> void tcf_chain_put(struct tcf_chain *chain)
>> {
>>- /* Destroy unused chain, with exception of chain 0, which is the
>>- * default one and has to be always present.
>>- */
>>- if (--chain->refcnt == 0 && !chain->filter_chain && chain->index != 0)
>>+ if (--chain->refcnt == 0)
>
> The refcounting is only done for actions holding reference to the chain.
> You still need to check is the filter chain is not empty.
> See tc_ctl_tfilter.
With my patch refcnt is done for block too, if you notice the
tcf_chain_put() in tcf_block_put().
>
> Also, chain 0 is created by default on a block creation. It has to be
> present always for a reason. Please see tcf_block_get. The pointer to
> chain 0 is assigned to the qdisc filter list pointer.
Sure, this is why block holds a refcnt to chain (not just chain 0) with
my patch, aka why the initial refcnt is 1 rather than 0.
^ permalink raw reply
* Re: [PATCH] DSA support for Micrel KSZ8895
From: Andrew Lunn @ 2017-09-06 17:09 UTC (permalink / raw)
To: Tristram.Ha
Cc: muvarov, pavel, Woojung.Huh, nathan.leigh.conrad, vivien.didelot,
f.fainelli, netdev, linux-kernel
In-Reply-To: <93AF473E2DA327428DE3D46B72B1E9FD41120E4D@CHN-SV-EXMX02.mchp-main.com>
> The patches are under review internally and will need to be updated
> and approved by Woojung before formal submission. Problem is
> although KSZ8795 and KSZ8895 drivers are new code and will be
> submitted as RFC, they depend on the change of KSZ9477 driver
> currently in the kernel, which require more rigorous review.
Please be aware that they will also go though review when you post
them. This can be anything from great, nice job, to throw them away
and start again. Since you are submitting RFCs we understand it is
early code, issues still to be solved, and we can make suggestions how
to solve those issues.
Post early, post often...
Andrew
^ permalink raw reply
* Re: [PATCH net-next] xdp: implement xdp_redirect_map for generic XDP
From: Daniel Borkmann @ 2017-09-06 17:02 UTC (permalink / raw)
To: Jesper Dangaard Brouer, netdev, David S. Miller
Cc: John Fastabend, Andy Gospodarek
In-Reply-To: <59B02127.5020904@iogearbox.net>
On 09/06/2017 06:24 PM, Daniel Borkmann wrote:
[...]
> Otherwise it looks good, but note that it also doesn't really
> resolve the issue you mention wrt stale map pointers by the
> way. This would need a different way to clear out the pointers
> from redirect_info, I'm thinking when we have devmap dismantle
> time after RCU grace period we should check whether there are
> still stale pointers from this map around and clear them under
> disabled preemption, but need to brainstorm a bit more on that
> first.
Scratch that approach, doesn't work. So thinking bit more on
this, what we could do here is the following: verifier knows
we called bpf_xdp_redirect_map() helper, so it could do a small
insn rewrite in the sense that it fills R4 with a pointer to
the bpf_prog. We have that at verification time anyway and R4
is allowed to be populated since we scratch it per convention.
Then, the helper would store the prog pointer in struct redirect_info.
Later in xdp_do_*_redirect() we check whether the redirect_info's
prog pointer is the same as passed xdp_prog pointer, and if
that's the case then all good, since the prog holds a ref on
the map anyway, if they are not equal in the unlikely case, it
means stale pointer, so we bail out right there. That would
work imo, will see to code it up and check it out.
^ permalink raw reply
* Re: [PATCH v2 rfc 0/8] IGMP snooping for local traffic
From: Andrew Lunn @ 2017-09-06 17:01 UTC (permalink / raw)
To: Vivien Didelot; +Cc: netdev, jiri, nikolay, Florian Fainelli
In-Reply-To: <874lsfg87t.fsf@weeman.i-did-not-set--mail-host-address--so-tickle-me>
On Wed, Sep 06, 2017 at 11:25:26AM -0400, Vivien Didelot wrote:
> Hi Andrew, Nikolay,
>
> Andrew Lunn <andrew@lunn.ch> writes:
>
> > Then starts the work passing down to the hardware that the host has
> > joined/left a group. The existing switchdev mdb object cannot be used,
> > since the semantics are different. The existing
> > SWITCHDEV_OBJ_ID_PORT_MDB is used to indicate a specific multicast
> > group should be forwarded out that port of the switch. However here we
> > require the exact opposite. We want multicast frames for the group
> > received on the port to the forwarded to the host. Hence add a new
> > object SWITCHDEV_OBJ_ID_HOST_MDB, a multicast database entry to
> > forward to the host. This new object is then propagated through the
> > DSA layers. No DSA driver changes should be needed, this should just
> > work...
>
> I'm not sure if you already explained that, if so, sorry in advance.
>
> I don't understand why SWITCHDEV_OBJ_ID_PORT_MDB cannot be used. Isn't
> setting the obj->orig_dev to the bridge device itself enough to
> distinguish br0 from a switch port?
Hi Vivien
I did consider this. But conceptually, it seems wrong.
SWITCHDEV_OBJ_ID_PORT_MDB has always been about egress. I don't like
adding a special case for ingress. Adding a new switchdev object
avoids this special case.
> static int dsa_slave_port_obj_add(struct net_device *dev,
> const struct switchdev_obj *obj,
> struct switchdev_trans *trans)
> {
> struct dsa_slave_priv *p = netdev_priv(dev);
> struct dsa_port *port = p->dp;
>
> /* Is the target port the bridge device itself? */
> if (obj->orig_dev == port->br)
> port = port->cpu_dp;
>
> return dsa_port_mdb_add(port, obj, trans);
> }
>
> The main problem is that we will soon want support for multiple CPU
> ports.
We keep saying that, but it never actually happens. We should solve
multiple CPU problems when we actually add support for multiple CPUs.
Probably at that point, we need to push SWITCHDEV_OBJ_ID_PORT_HOST_MDB
right down to a port, so that port can setup what needs setting up so
its frames goes to its CPU port.
> So adding the MDB entry to all CPU ports as you did in a patch 5/8 in
> dsa_switch_host_mdb_*() is not correct because you can have CPU port
> sw0p0 being a member of br0 and CPU port sw0p10 being a member of br1.
Be careful, you are making assumptions about mapping cpu ports to
bridges. That is not been defined yet.
Andrew
^ permalink raw reply
* [iproute PATCH] utils: Review strlcpy() and strlcat()
From: Phil Sutter @ 2017-09-06 16:51 UTC (permalink / raw)
To: Stephen Hemminger; +Cc: netdev
In-Reply-To: <063D6719AE5E284EB5DD2968C1650D6DD006EC26@AcuExch.aculab.com>
As David Laight correctly pointed out, the first version of strlcpy()
modified dst buffer behind the string copied into it. Fix this by
writing NUL to the byte immediately following src string instead of to
the last byte in dst. Doing so also allows to reduce overhead by using
memcpy().
Improve strlcat() by avoiding the call to strlcpy() if dst string is
already full, not just as sanity check.
Signed-off-by: Phil Sutter <phil@nwl.cc>
---
lib/utils.c | 12 ++++++++----
1 file changed, 8 insertions(+), 4 deletions(-)
diff --git a/lib/utils.c b/lib/utils.c
index 330ab073c2068..bbd3cbc46a0e5 100644
--- a/lib/utils.c
+++ b/lib/utils.c
@@ -1233,18 +1233,22 @@ int get_real_family(int rtm_type, int rtm_family)
size_t strlcpy(char *dst, const char *src, size_t size)
{
+ size_t srclen = strlen(src);
+
if (size) {
- strncpy(dst, src, size - 1);
- dst[size - 1] = '\0';
+ size_t minlen = min(srclen, size - 1);
+
+ memcpy(dst, src, minlen);
+ dst[minlen] = '\0';
}
- return strlen(src);
+ return srclen;
}
size_t strlcat(char *dst, const char *src, size_t size)
{
size_t dlen = strlen(dst);
- if (dlen > size)
+ if (dlen >= size)
return dlen + strlen(src);
return dlen + strlcpy(dst + dlen, src, size - dlen);
--
2.13.1
^ permalink raw reply related
* Re: [PATCH net v2] rds: Fix incorrect statistics counting
From: Santosh Shilimkar @ 2017-09-06 16:48 UTC (permalink / raw)
To: Håkon Bugge, David S . Miller
Cc: netdev, linux-rdma, rds-devel, linux-kernel, knut.omang
In-Reply-To: <20170906163551.20387-1-Haakon.Bugge@oracle.com>
On 9/6/2017 9:35 AM, Håkon Bugge wrote:
> In rds_send_xmit() there is logic to batch the sends. However, if
> another thread has acquired the lock and has incremented the send_gen,
> it is considered a race and we yield. The code incrementing the
> s_send_lock_queue_raced statistics counter did not count this event
> correctly.
>
> This commit counts the race condition correctly.
>
> Changes from v1:
> - Removed check for *someone_on_xmit()*
> - Fixed incorrect indentation
>
> Signed-off-by: Håkon Bugge <haakon.bugge@oracle.com>
> Reviewed-by: Knut Omang <knut.omang@oracle.com>
> ---
Thanks for the update.
Acked-by: Santosh Shilimkar <santosh.shilimkar@oracle.com>
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox