* Re: Performance regression with virtio_net
From: Michael S. Tsirkin @ 2017-07-27 21:30 UTC (permalink / raw)
To: Seth Forshee; +Cc: netdev, virtualization
In-Reply-To: <20170727211430.viki2jcxbzk2u62c@ubuntu-hedt>
On Thu, Jul 27, 2017 at 04:14:30PM -0500, Seth Forshee wrote:
> On Thu, Jul 27, 2017 at 11:38:52PM +0300, Michael S. Tsirkin wrote:
> > On Thu, Jul 27, 2017 at 12:09:42PM -0500, Seth Forshee wrote:
> > > I'm seeing a performance regression with virtio_net that looks to have
> > > started in 4.12-rc1. I only see it in one context though, downloading
> > > snap packages from the Ubuntu snap store. For example:
> > >
> > > https://api.snapcraft.io/api/v1/snaps/download/b8X2psL1ryVrPt5WEmpYiqfr5emixTd7_1797.snap
> > >
> > > which redirects to Internap's CDN. Normally this downloads in a few
> > > seconds at ~10 MB/s, but with 4.12 and 4.13 it takes minutes with a rate
> > > of ~150 KB/s. Everything else I've tried downloads as normal speeds.
> >
> > So just wget that URL should be enough?
>
> Yes. Note that sometimes it starts out faster then slows down.
> > > I bisected this to 680557cf79f8 "virtio_net: rework mergeable buffer
> > > handling". If I revert this on top of 4.13-rc2 (along with other changes
> > > needed to successfully revert it) speeds return to normal.
> > >
> > > Thanks,
> > > Seth
> >
> >
> > Interesting. A more likely suspect would be
> > e377fcc8486d40867c6c217077ad0fa40977e060 - could you please try
> > reverting that one instead?
>
> I tried it, and I still get slow download speeds. I did test at
> 680557cf79f82623e2c4fd42733077d60a843513 during the bisect so I'm
> reasonably confident that this is the one where things went bad.
> > Also, could you please look at mergeable_rx_buffer_size in sysfs with
> > and without the change?
>
> In all cases (stock 4.13-rc2, 680557cf79f8 reverted, and e377fcc8486d
> reverted) mergeable_rx_buffer_size was 1536.
>
> Thanks,
> Seth
Do you see any error counters incrementing after it slows down?
--
MST
^ permalink raw reply
* Re: Performance regression with virtio_net
From: Seth Forshee @ 2017-07-27 21:14 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: Jason Wang, virtualization, netdev
In-Reply-To: <20170727233244-mutt-send-email-mst@kernel.org>
On Thu, Jul 27, 2017 at 11:38:52PM +0300, Michael S. Tsirkin wrote:
> On Thu, Jul 27, 2017 at 12:09:42PM -0500, Seth Forshee wrote:
> > I'm seeing a performance regression with virtio_net that looks to have
> > started in 4.12-rc1. I only see it in one context though, downloading
> > snap packages from the Ubuntu snap store. For example:
> >
> > https://api.snapcraft.io/api/v1/snaps/download/b8X2psL1ryVrPt5WEmpYiqfr5emixTd7_1797.snap
> >
> > which redirects to Internap's CDN. Normally this downloads in a few
> > seconds at ~10 MB/s, but with 4.12 and 4.13 it takes minutes with a rate
> > of ~150 KB/s. Everything else I've tried downloads as normal speeds.
>
> So just wget that URL should be enough?
Yes. Note that sometimes it starts out faster then slows down.
> > I bisected this to 680557cf79f8 "virtio_net: rework mergeable buffer
> > handling". If I revert this on top of 4.13-rc2 (along with other changes
> > needed to successfully revert it) speeds return to normal.
> >
> > Thanks,
> > Seth
>
>
> Interesting. A more likely suspect would be
> e377fcc8486d40867c6c217077ad0fa40977e060 - could you please try
> reverting that one instead?
I tried it, and I still get slow download speeds. I did test at
680557cf79f82623e2c4fd42733077d60a843513 during the bisect so I'm
reasonably confident that this is the one where things went bad.
> Also, could you please look at mergeable_rx_buffer_size in sysfs with
> and without the change?
In all cases (stock 4.13-rc2, 680557cf79f8 reverted, and e377fcc8486d
reverted) mergeable_rx_buffer_size was 1536.
Thanks,
Seth
^ permalink raw reply
* Re: [PATCH v2 2/4] can: fixed-transceiver: Add documentation for CAN fixed transceiver bindings
From: Franklin S Cooper Jr @ 2017-07-27 21:10 UTC (permalink / raw)
To: Oliver Hartkopp, Andrew Lunn
Cc: linux-kernel, devicetree, netdev, linux-can, wg, mkl, robh+dt,
quentin.schulz, dev.kurt, sergei.shtylyov
In-Reply-To: <fe99189e-d077-ba65-4dfc-a4d8beee62b3@hartkopp.net>
On 07/27/2017 01:47 PM, Oliver Hartkopp wrote:
> On 07/26/2017 08:29 PM, Franklin S Cooper Jr wrote:
>>
>
>> I'm fine with switching to using bitrate instead of speed. Kurk was
>> originally the one that suggested to use the term arbitration and data
>> since thats how the spec refers to it. Which I do agree with. But your
>> right that in the drivers (struct can_priv) we just use bittiming and
>> data_bittiming (CAN-FD timings). I don't think adding "fd" into the
>> property name makes sense unless we are calling it something like
>> "max-canfd-bitrate" which I would agree is the easiest to understand.
>>
>> So what is the preference if we end up sticking with two properties?
>> Option 1 or 2?
>>
>> 1)
>> max-bitrate
>> max-data-bitrate
>>
>> 2)
>> max-bitrate
>> max-canfd-bitrate
>>
>>
>
> 1
>
>>> A CAN transceiver is limited in bandwidth. But you only have one RX and
>>> one TX line between the CAN controller and the CAN transceiver. The
>>> transceiver does not know about CAN FD - it has just a physical(!) layer
>>> with a limited bandwidth. This is ONE limitation.
>>>
>>> So I tend to specify only ONE 'max-bitrate' property for the
>>> fixed-transceiver binding.
>>>
>>> The fact whether the CAN controller is CAN FD capable or not is provided
>>> by the netlink configuration interface for CAN controllers.
>>
>> Part of the reasoning to have two properties is to indicate that you
>> don't support CAN FD while limiting the "arbitration" bit rate.
>
> ??
>
> It's a physical layer device which only has a bandwidth limitation.
> The transceiver does not know about CAN FD.
>
>> With one
>> property you can not determine this and end up having to make some
>> assumptions that can quickly end up biting people.
>
> Despite the fact that the transceiver does not know anything about ISO
> layer 2 (CAN/CAN FD) the properties should look like
>
> max-bitrate
> canfd-capable
>
> then.
>
> But when the tranceiver is 'canfd-capable' agnostic, why provide a
> property for it?
>
> Maybe I'm wrong but I still can't follow your argumentation ideas.
Your right. I spoke to our CAN transceiver team and I finally get your
points.
So yes using "max-bitrate" alone is all we need. Sorry for the confusion
and I'll create a new rev using this approach.
>
> Regards,
> Oliver
^ permalink raw reply
* Re: [PATCH net] xgene: Don't fail probe, if there is no clk resource for SGMII interfaces
From: Laura Abbott @ 2017-07-27 21:03 UTC (permalink / raw)
To: Thomas Bogendoerfer, isubramanian, kchudgar, qnguyen, netdev,
linux-kernel
In-Reply-To: <20170713092144.31751508094@solo.franken.de>
On 07/13/2017 01:57 AM, Thomas Bogendoerfer wrote:
> From: Thomas Bogendoerfer <tbogendoerfer@suse.de>
>
> This change fixes following problem
>
> [ 1.827940] xgene-enet: probe of 1f210030.ethernet failed with error -2
>
> which leads to a missing ethernet interface (reproducable at least on
> Gigabyte MP30-AR0 and APM Mustang systems).
>
> The check for a valid clk resource fails, because DT doesn't provide a
> clock for sgenet1. But the driver doesn't use this clk, if the ethernet
> port is connected via SGMII. Therefore this patch avoids probing for clk
> on SGMII interfaces.
>
> Fixes: 9aea7779b764 drivers: net: xgene: Fix crash on DT systems
> Signed-off-by: Thomas Bogendoerfer <tbogendoerfer@suse.de>
> ---
> drivers/net/ethernet/apm/xgene/xgene_enet_main.c | 22 ++++++++++++----------
> 1 file changed, 12 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/net/ethernet/apm/xgene/xgene_enet_main.c b/drivers/net/ethernet/apm/xgene/xgene_enet_main.c
> index d3906f6b01bd..86058a9f3417 100644
> --- a/drivers/net/ethernet/apm/xgene/xgene_enet_main.c
> +++ b/drivers/net/ethernet/apm/xgene/xgene_enet_main.c
> @@ -1785,16 +1785,18 @@ static int xgene_enet_get_resources(struct xgene_enet_pdata *pdata)
>
> xgene_enet_gpiod_get(pdata);
>
> - pdata->clk = devm_clk_get(&pdev->dev, NULL);
> - if (IS_ERR(pdata->clk)) {
> - /* Abort if the clock is defined but couldn't be retrived.
> - * Always abort if the clock is missing on DT system as
> - * the driver can't cope with this case.
> - */
> - if (PTR_ERR(pdata->clk) != -ENOENT || dev->of_node)
> - return PTR_ERR(pdata->clk);
> - /* Firmware may have set up the clock already. */
> - dev_info(dev, "clocks have been setup already\n");
> + if (pdata->phy_mode != PHY_INTERFACE_MODE_SGMII) {
> + pdata->clk = devm_clk_get(&pdev->dev, NULL);
> + if (IS_ERR(pdata->clk)) {
> + /* Abort if the clock is defined but couldn't be
> + * retrived. Always abort if the clock is missing on
> + * DT system as the driver can't cope with this case.
> + */
> + if (PTR_ERR(pdata->clk) != -ENOENT || dev->of_node)
> + return PTR_ERR(pdata->clk);
> + /* Firmware may have set up the clock already. */
> + dev_info(dev, "clocks have been setup already\n");
> + }
> }
>
> if (pdata->phy_mode != PHY_INTERFACE_MODE_XGMII)
>
This change causes boot failures for me on my APM Mustang system running
Fedora rawhide:
[ 16.669089] Synchronous External Abort: synchronous external abort (0x9600000
[ 16.669099] Internal error: : 96000010 [#1] SMP
[ 16.669103] Modules linked in: xgene_enet(+) at803x realtek mdio_xgene xgenes
[ 16.669127] CPU: 2 PID: 534 Comm: systemd-udevd Not tainted 4.13.0-0.rc1.git1
[ 16.669128] Hardware name: AppliedMicro X-Gene Mustang Board/X-Gene Mustang 6
[ 16.669131] task: ffff8003e6f8ce00 task.stack: ffff8003e4fd8000
[ 16.669144] PC is at xgene_enet_wr_mac+0xa0/0x128 [xgene_enet]
[ 16.669152] LR is at xgene_enet_wr_mac+0x64/0x128 [xgene_enet]
<snip>
[ 16.669345] [<ffff000000de19f0>] xgene_enet_wr_mac+0xa0/0x128 [xgene_enet]
[ 16.669354] [<ffff000000de2fc0>] xgene_sgmac_reset+0x28/0x48 [xgene_enet]
[ 16.669362] [<ffff000000de3b50>] xgene_sgmac_init+0x1e0/0x2e8 [xgene_enet]
[ 16.669370] [<ffff000000de8564>] xgene_enet_probe+0xfa4/0x1368 [xgene_enet]
[ 16.669376] [<ffff0000086f14f0>] platform_drv_probe+0x60/0xc0
[ 16.669379] [<ffff0000086ee86c>] driver_probe_device+0x31c/0x458
[ 16.669381] [<ffff0000086eea8c>] __driver_attach+0xe4/0x130
[ 16.669384] [<ffff0000086ebfd4>] bus_for_each_dev+0x5c/0xa8
[ 16.669386] [<ffff0000086edf30>] driver_attach+0x30/0x40
[ 16.669388] [<ffff0000086ed8d0>] bus_add_driver+0x220/0x2c0
[ 16.669390] [<ffff0000086efd04>] driver_register+0x6c/0x118
[ 16.669392] [<ffff0000086f142c>] __platform_driver_register+0x54/0x60
[ 16.669400] [<ffff000000e05014>] xgene_enet_driver_init+0x14/0x1000 [xgene_e]
[ 16.669404] [<ffff000008083834>] do_one_initcall+0x44/0x138
[ 16.669408] [<ffff0000081a327c>] do_init_module+0x64/0x1d0
[ 16.669410] [<ffff0000081a1dc4>] load_module+0x151c/0x1770
[ 16.669413] [<ffff0000081a2330>] SyS_finit_module+0xd8/0xf0
[ 16.669415] [<ffff00000808364c>] __sys_trace_return+0x0/0x4
[ 16.669418] Code: 14000004 d503201f d28218e0 95f24031 (b94002a2)
I suspect the clock is actually needed on some systems.
Thanks,
Laura
^ permalink raw reply
* RE: [PATCH V4 net-next 7/8] net: hns3: Add Ethtool support to HNS3 driver
From: Salil Mehta @ 2017-07-27 20:46 UTC (permalink / raw)
To: Florian Fainelli, davem-fT/PcQaiUtIeIZ0/mPfg9Q@public.gmane.org
Cc: Zhuangyuzeng (Yisen), huangdaode, lipeng (Y),
mehta.salil.lnk-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org,
netdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Linuxarm
In-Reply-To: <0fa12c08-8d20-5bb2-5e56-c083c57922e2-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Hi Florian,
> -----Original Message-----
> From: Florian Fainelli [mailto:f.fainelli@gmail.com]
> Sent: Thursday, July 27, 2017 7:05 PM
> To: Salil Mehta; davem@davemloft.net
> Cc: Zhuangyuzeng (Yisen); huangdaode; lipeng (Y);
> mehta.salil.lnk@gmail.com; netdev@vger.kernel.org; linux-
> kernel@vger.kernel.org; linux-rdma@vger.kernel.org; Linuxarm
> Subject: Re: [PATCH V4 net-next 7/8] net: hns3: Add Ethtool support to
> HNS3 driver
>
> On 07/27/2017 11:01 AM, Salil Mehta wrote:
> > Hi Florian,
> >
> >> -----Original Message-----
> >> From: Florian Fainelli [mailto:f.fainelli@gmail.com]
> >> Sent: Sunday, July 23, 2017 6:05 PM
> >> To: Salil Mehta; davem@davemloft.net
> >> Cc: Zhuangyuzeng (Yisen); huangdaode; lipeng (Y);
> >> mehta.salil.lnk@gmail.com; netdev@vger.kernel.org; linux-
> >> kernel@vger.kernel.org; linux-rdma@vger.kernel.org; Linuxarm
> >> Subject: Re: [PATCH V4 net-next 7/8] net: hns3: Add Ethtool support
> to
> >> HNS3 driver
> >>
> >>
> >>
> >> On 07/22/2017 03:09 PM, Salil Mehta wrote:
> >>> This patch adds the support of the Ethtool interface to
> >>> the HNS3 Ethernet driver. Various commands to read the
> >>> statistics, configure the offloading, loopback selftest etc.
> >>> are supported.
> >>>
> >>> Signed-off-by: Daode Huang <huangdaode@hisilicon.com>
> >>> Signed-off-by: lipeng <lipeng321@huawei.com>
> >>> Signed-off-by: Salil Mehta <salil.mehta@huawei.com>
> >>> Signed-off-by: Yisen Zhuang <yisen.zhuang@huawei.com>
> >>> ---
> >>> Patch V4: addressed below comments
> >>> 1. Andrew Lunn
> >>> Removed the support of loop PHY back for now
> >>> Patch V3: Address below comments
> >>> 1. Stephen Hemminger
> >>> https://lkml.org/lkml/2017/6/13/974
> >>> 2. Andrew Lunn
> >>> https://lkml.org/lkml/2017/6/13/1037
> >>> Patch V2: No change
> >>> Patch V1: Initial Submit
> >>> ---
> >>> .../ethernet/hisilicon/hns3/hns3pf/hns3_ethtool.c | 543
> >> +++++++++++++++++++++
> >>> 1 file changed, 543 insertions(+)
> >>> create mode 100644
> >> drivers/net/ethernet/hisilicon/hns3/hns3pf/hns3_ethtool.c
> >>>
> >>> diff --git
> >> a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hns3_ethtool.c
> >> b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hns3_ethtool.c
> >>> new file mode 100644
> >>> index 000000000000..82b0d4d829f8
> >>> --- /dev/null
> >>> +++ b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hns3_ethtool.c
> >>> @@ -0,0 +1,543 @@
> >>> +/*
> >>> + * Copyright (c) 2016~2017 Hisilicon Limited.
> >>> + *
> >>> + * This program is free software; you can redistribute it and/or
> >> modify
> >>> + * it under the terms of the GNU General Public License as
> published
> >> by
> >>> + * the Free Software Foundation; either version 2 of the License,
> or
> >>> + * (at your option) any later version.
> >>> + */
> >>> +
> >>> +#include <linux/etherdevice.h>
> >>> +#include "hns3_enet.h"
> >>> +
> >>> +struct hns3_stats {
> >>> + char stats_string[ETH_GSTRING_LEN];
> >>> + int stats_size;
> >>> + int stats_offset;
> >>> +};
> >>> +
> >>> +/* netdev related stats */
> >>> +#define HNS3_NETDEV_STAT(_string, _member) \
> >>> + { _string, \
> >>> + FIELD_SIZEOF(struct rtnl_link_stats64, _member), \
> >>> + offsetof(struct rtnl_link_stats64, _member), \
> >>> + }
> >>
> >> Can you make this macro use named initializers?
> > Can you please explain bit more or point out some
> > example. This would be very handy.
>
> .stat_string = _string,
> .stats_size = FIELD_SIZEOF(struct rtnl_link_stat64, _member),
> .stats_offset = offsetof(struct rtnl_link_stats64, _member),
>
> https://gcc.gnu.org/onlinedocs/gcc/Designated-Inits.html
Ok got it, thanks!
> --
> Florian
^ permalink raw reply
* RE: [PATCH V4 net-next 1/8] net: hns3: Add support of HNS3 Ethernet Driver for hip08 SoC
From: Salil Mehta @ 2017-07-27 20:44 UTC (permalink / raw)
To: Florian Fainelli, davem@davemloft.net
Cc: Zhuangyuzeng (Yisen), huangdaode, lipeng (Y),
mehta.salil.lnk@gmail.com, netdev@vger.kernel.org,
linux-kernel@vger.kernel.org, linux-rdma@vger.kernel.org,
Linuxarm
In-Reply-To: <79a3a952-acae-2a51-0d49-46acaf92cf3b@gmail.com>
Hi Florian,
> -----Original Message-----
> From: Florian Fainelli [mailto:f.fainelli@gmail.com]
> Sent: Sunday, July 23, 2017 6:24 PM
> To: Salil Mehta; davem@davemloft.net
> Cc: Zhuangyuzeng (Yisen); huangdaode; lipeng (Y);
> mehta.salil.lnk@gmail.com; netdev@vger.kernel.org; linux-
> kernel@vger.kernel.org; linux-rdma@vger.kernel.org; Linuxarm
> Subject: Re: [PATCH V4 net-next 1/8] net: hns3: Add support of HNS3
> Ethernet Driver for hip08 SoC
>
>
>
> On 07/22/2017 03:09 PM, Salil Mehta wrote:
> > This patch adds the support of Hisilicon Network Subsystem 3
> > Ethernet driver to hip08 family of SoCs.
> >
> > This driver includes basic Rx/Tx functionality. It also includes
> > the client registration code with the HNAE3(Hisilicon Network
> > Acceleration Engine 3) framework.
> >
> > This work provides the initial support to the hip08 SoC and
> > would incrementally add features or enhancements.
> >
> > Signed-off-by: Daode Huang <huangdaode@hisilicon.com>
> > Signed-off-by: lipeng <lipeng321@huawei.com>
> > Signed-off-by: Salil Mehta <salil.mehta@huawei.com>
> > Signed-off-by: Yisen Zhuang <yisen.zhuang@huawei.com>
> > ---
> > Patch V4: addressed comments by:
> > 1. Andrew Lunn:
> > https://lkml.org/lkml/2017/6/17/222
> > https://lkml.org/lkml/2017/6/17/232
> > 2. Bo Yu:
> > https://lkml.org/lkml/2017/6/18/110
> > https://lkml.org/lkml/2017/6/18/115
> > Patch V3: Addresed below comments:
> > 1. Stephen Hemminger:
> > https://lkml.org/lkml/2017/6/13/972
> > 2. Yuval Mintz:
> > https://lkml.org/lkml/2017/6/14/151
> > Patch V2: Addressed below comments:
> > 1. Kbuild:
> > https://lkml.org/lkml/2017/6/11/73
> > 2. Yuval Mintz:
> > https://lkml.org/lkml/2017/6/10/78
> > Patch V1: Initial Submit
> > ---
> > .../net/ethernet/hisilicon/hns3/hns3pf/hns3_enet.c | 2894
> ++++++++++++++++++++
> > .../net/ethernet/hisilicon/hns3/hns3pf/hns3_enet.h | 598 ++++
> > 2 files changed, 3492 insertions(+)
> > create mode 100644
> drivers/net/ethernet/hisilicon/hns3/hns3pf/hns3_enet.c
> > create mode 100644
> drivers/net/ethernet/hisilicon/hns3/hns3pf/hns3_enet.h
> >
> > diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hns3_enet.c
> b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hns3_enet.c
> > new file mode 100644
> > index 000000000000..6e0e2967db42
> > --- /dev/null
> > +++ b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hns3_enet.c
> > @@ -0,0 +1,2894 @@
> > +/*
> > + * Copyright (c) 2016~2017 Hisilicon Limited.
> > + *
> > + * This program is free software; you can redistribute it and/or
> modify
> > + * it under the terms of the GNU General Public License as published
> by
> > + * the Free Software Foundation; either version 2 of the License, or
> > + * (at your option) any later version.
> > + */
> > +
> > +#include <linux/dma-mapping.h>
> > +#include <linux/etherdevice.h>
> > +#include <net/gre.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/if_vlan.h>
> > +#include <linux/ip.h>
> > +#include <linux/ipv6.h>
> > +#include <linux/module.h>
> > +#include <linux/pci.h>
> > +#include <linux/skbuff.h>
> > +#include <linux/sctp.h>
> > +#include <net/vxlan.h>
> > +
> > +#include "hnae3.h"
> > +#include "hns3_enet.h"
> > +
> > +const char hns3_driver_name[] = "hns3";
> > +static const char hns3_driver_string[] =
> > + "Hisilicon Ethernet Network Driver for Hi162x
> Family";
> > +static const char hns3_copyright[] = "Copyright (c) 2017 Huawei
> Corporation.";
> > +static struct hnae3_client client;
> > +
> > +/* hns3_pci_tbl - PCI Device ID Table
> > + *
> > + * Last entry must be all 0s
> > + *
> > + * { Vendor ID, Device ID, SubVendor ID, SubDevice ID,
> > + * Class, Class Mask, private data (not used) }
> > + */
> > +static const struct pci_device_id hns3_pci_tbl[] = {
> > + {PCI_VDEVICE(HUAWEI, HNAE3_DEV_ID_GE), 0},
> > + {PCI_VDEVICE(HUAWEI, HNAE3_DEV_ID_25GE), 0},
> > + {PCI_VDEVICE(HUAWEI, HNAE3_DEV_ID_25GE_RDMA), 0},
> > + {PCI_VDEVICE(HUAWEI, HNAE3_DEV_ID_25GE_RDMA_MACSEC), 0},
> > + {PCI_VDEVICE(HUAWEI, HNAE3_DEV_ID_50GE_RDMA), 0},
> > + {PCI_VDEVICE(HUAWEI, HNAE3_DEV_ID_50GE_RDMA_MACSEC), 0},
> > + {PCI_VDEVICE(HUAWEI, HNAE3_DEV_ID_100G_RDMA_MACSEC), 0},
> > + /* required last entry */
> > + {0, }
> > +};
> > +MODULE_DEVICE_TABLE(pci, hns3_pci_tbl);
> > +
> > +static irqreturn_t hns3_irq_handle(int irq, void *dev)
> > +{
> > + struct hns3_enet_tqp_vector *tqp_vector = dev;
> > +
> > + napi_schedule(&tqp_vector->napi);
> > +
> > + return IRQ_HANDLED;
> > +}
> > +
> > +static int hns3_nic_init_irq(struct hns3_nic_priv *priv)
> > +{
> > + struct pci_dev *pdev = priv->ae_handle->pdev;
> > + struct hns3_enet_tqp_vector *tqp_vectors;
> > + int txrx_int_idx = 0;
> > + int rx_int_idx = 0;
> > + int tx_int_idx = 0;
> > + int ret;
> > + int i;
>
> unsigned int i
Ok.
>
> > +
> > + for (i = 0; i < priv->vector_num; i++) {
> > + tqp_vectors = &priv->tqp_vector[i];
> > +
> > + if (tqp_vectors->irq_init_flag == HNS3_VECTOR_INITED)
> > + continue;
> > +
> > + if (tqp_vectors->tx_group.ring && tqp_vectors-
> >rx_group.ring) {
> > + snprintf(tqp_vectors->name, HNAE3_INT_NAME_LEN - 1,
> > + "%s-%s-%d", priv->netdev->name, "TxRx",
> > + txrx_int_idx++);
> > + txrx_int_idx++;
> > + } else if (tqp_vectors->rx_group.ring) {
> > + snprintf(tqp_vectors->name, HNAE3_INT_NAME_LEN - 1,
> > + "%s-%s-%d", priv->netdev->name, "Rx",
> > + rx_int_idx++);
> > + } else if (tqp_vectors->tx_group.ring) {
> > + snprintf(tqp_vectors->name, HNAE3_INT_NAME_LEN - 1,
> > + "%s-%s-%d", priv->netdev->name, "Tx",
> > + tx_int_idx++);
> > + } else {
> > + /* Skip this unused q_vector */
> > + continue;
> > + }
> > +
> > + tqp_vectors->name[HNAE3_INT_NAME_LEN - 1] = '\0';
> > +
> > + ret = devm_request_irq(&pdev->dev, tqp_vectors->vector_irq,
> > + hns3_irq_handle, 0, tqp_vectors->name,
> > + tqp_vectors);
> > + if (ret) {
> > + netdev_err(priv->netdev, "request irq(%d) fail\n",
> > + tqp_vectors->vector_irq);
> > + return ret;
> > + }
> > + disable_irq(tqp_vectors->vector_irq);
>
> So why do the request_irq() just yet then?
Yes, agreed. This is bit weird logic. We do not need it here.
Will fix it.
>
> > +
> > + tqp_vectors->irq_init_flag = HNS3_VECTOR_INITED;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static void hns3_mask_vector_irq(struct hns3_enet_tqp_vector
> *tqp_vector,
> > + u32 mask_en)
> > +{
> > + writel(mask_en, tqp_vector->mask_addr);
> > +}
> > +
> > +static void hns3_vector_enable(struct hns3_enet_tqp_vector
> *tqp_vector)
> > +{
> > + napi_enable(&tqp_vector->napi);
> > + enable_irq(tqp_vector->vector_irq);
> > +
> > + /* Enable vector */
> > + hns3_mask_vector_irq(tqp_vector, 1);
> > +}
> > +
> > +static void hns3_vector_disable(struct hns3_enet_tqp_vector
> *tqp_vector)
> > +{
> > + /* Disable vector */
> > + hns3_mask_vector_irq(tqp_vector, 0);
> > +
> > + disable_irq(tqp_vector->vector_irq);
> > + napi_disable(&tqp_vector->napi);
> > +}
> > +
> > +static void hns3_set_vector_coalesc_gl(struct hns3_enet_tqp_vector
> *tqp_vector,
> > + u32 gl_value)
> > +{
> > + /* this defines the configuration for GL (Interrupt Gap Limiter)
> > + * GL defines inter interrupt gap.
> > + * GL and RL(Rate Limiter) are 2 ways to acheive interrupt
> coalescing
> > + */
> > + writel(gl_value, tqp_vector->mask_addr + HNS3_VECTOR_GL0_OFFSET);
> > + writel(gl_value, tqp_vector->mask_addr + HNS3_VECTOR_GL1_OFFSET);
> > + writel(gl_value, tqp_vector->mask_addr + HNS3_VECTOR_GL2_OFFSET);
> > +}
> > +
> > +static void hns3_set_vector_coalesc_rl(struct hns3_enet_tqp_vector
> *tqp_vector,
> > + u32 rl_value)
> > +{
> > + /* this defines the configuration for RL (Interrupt Rate
> Limiter).
> > + * Rl defines rate of interrupts i.e. number of interrupts-per-
> second
> > + * GL and RL(Rate Limiter) are 2 ways to acheive interrupt
> coalescing
> > + */
> > + writel(rl_value, tqp_vector->mask_addr + HNS3_VECTOR_RL_OFFSET);
> > +}
> > +
> > +static void hns3_vector_gl_rl_init(struct hns3_enet_tqp_vector
> *tqp_vector)
> > +{
> > + /* initialize the configuration for interrupt coalescing.
> > + * 1. GL (Interrupt Gap Limiter)
> > + * 2. RL (Interrupt Rate Limiter)
> > + */
> > +
> > + /* Default :enable interrupt coalesce */
> > + tqp_vector->rx_group.int_gl = HNS3_INT_GL_50K;
> > + tqp_vector->tx_group.int_gl = HNS3_INT_GL_50K;
> > + hns3_set_vector_coalesc_gl(tqp_vector, HNS3_INT_GL_50K);
> > + /* for now we are disabling Interrupt RL - we
> > + * will re-enable later
> > + */
> > + hns3_set_vector_coalesc_rl(tqp_vector, 0);
> > + tqp_vector->rx_group.flow_level = HNS3_FLOW_LOW;
> > + tqp_vector->tx_group.flow_level = HNS3_FLOW_LOW;
> > +}
> > +
> > +static int hns3_nic_net_up(struct net_device *netdev)
> > +{
> > + struct hns3_nic_priv *priv = netdev_priv(netdev);
> > + struct hnae3_handle *h = priv->ae_handle;
> > + int i, j;
>
> unsigned int i, j
ok.
>
> > + int ret;
> > +
> > + ret = hns3_nic_init_irq(priv);
> > + if (ret) {
> > + netdev_err(netdev, "hns init irq failed! ret=%d\n", ret);
> > + return ret;
> > + }
> > +
> > + for (i = 0; i < priv->vector_num; i++)
> > + hns3_vector_enable(&priv->tqp_vector[i]);
> > +
> > + ret = h->ae_algo->ops->start ? h->ae_algo->ops->start(h) : 0;
> > + if (ret)
> > + goto out_start_err;
> > +
> > + return 0;
> > +
> > +out_start_err:
> > + netif_stop_queue(netdev);
> > +
> > + for (j = i - 1; j >= 0; j--)
> > + hns3_vector_disable(&priv->tqp_vector[j]);
> > +
> > + return ret;
> > +}
> > +
> > +static int hns3_nic_net_open(struct net_device *netdev)
> > +{
> > + struct hns3_nic_priv *priv = netdev_priv(netdev);
> > + struct hnae3_handle *h = priv->ae_handle;
> > + int ret;
> > +
> > + netif_carrier_off(netdev);
> > +
> > + ret = netif_set_real_num_tx_queues(netdev, h->kinfo.num_tqps);
> > + if (ret) {
> > + netdev_err(netdev, "netif_set_real_num_tx_queues fail,
> ret=%d!\n",
> > + ret);
> > + return ret;
> > + }
> > +
> > + ret = netif_set_real_num_rx_queues(netdev, h->kinfo.num_tqps);
> > + if (ret) {
> > + netdev_err(netdev,
> > + "netif_set_real_num_rx_queues fail, ret=%d!\n",
> ret);
> > + return ret;
> > + }
> > +
> > + ret = hns3_nic_net_up(netdev);
> > + if (ret) {
> > + netdev_err(netdev,
> > + "hns net up fail, ret=%d!\n", ret);
> > + return ret;
> > + }
> > +
> > + netif_carrier_on(netdev);
> > + netif_tx_wake_all_queues(netdev);
> > +
> > + return 0;
> > +}
> > +
> > +static void hns3_nic_net_down(struct net_device *netdev)
> > +{
> > + struct hns3_nic_priv *priv = netdev_priv(netdev);
> > + const struct hnae3_ae_ops *ops;
> > + int i;
> > +
> > + netif_tx_stop_all_queues(netdev);
> > + netif_carrier_off(netdev);
> > +
> > + ops = priv->ae_handle->ae_algo->ops;
> > +
> > + if (ops->stop)
> > + ops->stop(priv->ae_handle);
> > +
> > + for (i = 0; i < priv->vector_num; i++)
> > + hns3_vector_disable(&priv->tqp_vector[i]);
> > +}
> > +
> > +static int hns3_nic_net_stop(struct net_device *netdev)
> > +{
> > + hns3_nic_net_down(netdev);
> > +
> > + return 0;
> > +}
> > +
> > +void hns3_set_multicast_list(struct net_device *netdev)
> > +{
> > + struct hns3_nic_priv *priv = netdev_priv(netdev);
> > + struct hnae3_handle *h = priv->ae_handle;
> > + struct netdev_hw_addr *ha = NULL;
> > +
> > + if (!h) {
> > + netdev_err(netdev, "hnae handle is null\n");
> > + return;
> > + }
>
> This is not supposed to happen, and if it does surely you have a bug in
> how the initialization is done.
Yes, agreed. Removed.
>
>
> > +
> > +static int hns3_set_tso(struct sk_buff *skb, u32 *paylen,
> > + u16 *mss, u32 *type_cs_vlan_tso)
> > +{
> > + u32 l4_offset, hdr_len;
> > + union l3_hdr_info l3;
> > + union l4_hdr_info l4;
> > + u32 l4_paylen;
> > + int ret;
> > +
> > + if (skb_is_gso(skb)) {
>
> Reduce the indentation by testing for !skb_is_gso() first and exit if
> that's the case.
Sure.
>
>
> > +static int hns3_fill_desc(struct hns3_enet_ring *ring, void *priv,
> > + int size, dma_addr_t dma, int frag_end,
> > + enum hns_desc_type type)
> > +{
> > + struct hns3_desc_cb *desc_cb = &ring->desc_cb[ring->next_to_use];
> > + struct hns3_desc *desc = &ring->desc[ring->next_to_use];
> > + u32 ol_type_vlan_len_msec = 0;
> > + u16 bdtp_fe_sc_vld_ra_ri = 0;
> > + u32 type_cs_vlan_tso = 0;
> > + struct sk_buff *skb;
> > + u32 paylen = 0;
> > + u16 mss = 0;
> > + __be16 protocol;
> > + u8 ol4_proto;
> > + u8 il4_proto;
> > + int ret;
> > +
> > + /* The txbd's baseinfo of DESC_TYPE_PAGE & DESC_TYPE_SKB */
> > + desc_cb->priv = priv;
> > + desc_cb->length = size;
> > + desc_cb->dma = dma;
> > + desc_cb->type = type;
> > +
> > + /* now, fill the descriptor */
> > + desc->addr = cpu_to_le64(dma);
> > + desc->tx.send_size = cpu_to_le16((u16)size);
>
> Jut pass an u16 size then?
Maybe, can we live with this for now? :)
>
> > + hns3_set_txbd_baseinfo(&bdtp_fe_sc_vld_ra_ri, frag_end);
> > + desc->tx.bdtp_fe_sc_vld_ra_ri =
> cpu_to_le16(bdtp_fe_sc_vld_ra_ri);
> > +
> > + if (type == DESC_TYPE_SKB) {
> > + skb = (struct sk_buff *)priv;
> > + paylen = cpu_to_le16(skb->len);
> > +
> > + if (skb->ip_summed == CHECKSUM_PARTIAL) {
> > + skb_reset_mac_len(skb);
> > + protocol = skb->protocol;
> > +
> > + /* vlan packet*/
> > + if (protocol == htons(ETH_P_8021Q)) {
> > + protocol = vlan_get_protocol(skb);
> > + skb->protocol = protocol;
>
> What is this assignment doing exactly?
>
>
> > +static int hns3_fill_desc_tso(struct hns3_enet_ring *ring, void
> *priv,
> > + int size, dma_addr_t dma, int frag_end,
> > + enum hns_desc_type type)
> > +{
> > + int frag_buf_num;
> > + int sizeoflast;
> > + int ret, k;
>
> unsigned int k, frag_buf_num;
Ok.
>
> > +
> > + frag_buf_num = (size + HNS3_MAX_BD_SIZE - 1) / HNS3_MAX_BD_SIZE;
> > + sizeoflast = size % HNS3_MAX_BD_SIZE;
> > + sizeoflast = sizeoflast ? sizeoflast : HNS3_MAX_BD_SIZE;
> > +
> > + /* When the frag size is bigger than hardware, split this frag */
> > + for (k = 0; k < frag_buf_num; k++) {
> > + ret = hns3_fill_desc(ring, priv,
> > + (k == frag_buf_num - 1) ?
> > + sizeoflast : HNS3_MAX_BD_SIZE,
> > + dma + HNS3_MAX_BD_SIZE * k,
> > + frag_end && (k == frag_buf_num - 1) ? 1 : 0,
> > + (type == DESC_TYPE_SKB && !k) ?
> > + DESC_TYPE_SKB : DESC_TYPE_PAGE);
> > + if (ret)
> > + return ret;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static int hns3_nic_maybe_stop_tso(struct sk_buff **out_skb, int
> *bnum,
> > + struct hns3_enet_ring *ring)
> > +{
> > + struct sk_buff *skb = *out_skb;
> > + struct skb_frag_struct *frag;
> > + int bdnum_for_frag;
> > + int frag_num;
> > + int buf_num;
> > + int size;
> > + int i;
> > +
> > + size = skb_headlen(skb);
> > + buf_num = (size + HNS3_MAX_BD_SIZE - 1) / HNS3_MAX_BD_SIZE;
> > +
> > + frag_num = skb_shinfo(skb)->nr_frags;
> > + for (i = 0; i < frag_num; i++) {
> > + frag = &skb_shinfo(skb)->frags[i];
> > + size = skb_frag_size(frag);
> > + bdnum_for_frag =
> > + (size + HNS3_MAX_BD_SIZE - 1) / HNS3_MAX_BD_SIZE;
> > + if (bdnum_for_frag > HNS3_MAX_BD_PER_FRAG)
> > + return -ENOMEM;
> > +
> > + buf_num += bdnum_for_frag;
> > + }
> > +
> > + if (buf_num > ring_space(ring))
> > + return -EBUSY;
>
> -ENOSPC rather?
Most of the drivers including intel, mellanox etc return same.
>
> > +
> > + *bnum = buf_num;
> > + return 0;
> > +}
> > +
> > +static int hns3_nic_maybe_stop_tx(struct sk_buff **out_skb, int
> *bnum,
> > + struct hns3_enet_ring *ring)
> > +{
> > + struct sk_buff *skb = *out_skb;
> > + int buf_num;
> > +
> > + /* No. of segments (plus a header) */
> > + buf_num = skb_shinfo(skb)->nr_frags + 1;
> > +
> > + if (buf_num > ring_space(ring))
> > + return -EBUSY;
>
> Same here?
Most of the drivers including intel, mellanox etc return same.
>
> > +
> > + *bnum = buf_num;
> > +
> > + return 0;
> > +}
> > +
> > +static void hns_nic_dma_unmap(struct hns3_enet_ring *ring, int
> next_to_use_orig)
> > +{
> > + struct device *dev = ring_to_dev(ring);
> > +
> > + while (1) {
>
> This sounds dangerous, can you come up with an exit condition somehow?
>
> > + /* check if this is where we started */
> > + if (ring->next_to_use == next_to_use_orig)
> > + break;
> > +
> > + /* unmap the descriptor dma address */
> > + if (ring->desc_cb[ring->next_to_use].type == DESC_TYPE_SKB)
> > + dma_unmap_single(dev,
> > + ring->desc_cb[ring->next_to_use].dma,
> > + ring->desc_cb[ring->next_to_use].length,
> > + DMA_TO_DEVICE);
> > + else
> > + dma_unmap_page(dev,
> > + ring->desc_cb[ring->next_to_use].dma,
> > + ring->desc_cb[ring->next_to_use].length,
> > + DMA_TO_DEVICE);
> > +
> > + /* rollback one */
> > + ring_ptr_move_bw(ring, next_to_use);
> > + }
> > +}
> > +
> > +int hns3_nic_net_xmit_hw(struct net_device *netdev,
> > + struct sk_buff *skb,
> > + struct hns3_nic_ring_data *ring_data)
> > +{
> > + struct hns3_nic_priv *priv = netdev_priv(netdev);
> > + struct hns3_enet_ring *ring = ring_data->ring;
> > + struct device *dev = priv->dev;
> > + struct netdev_queue *dev_queue;
> > + struct skb_frag_struct *frag;
> > + int next_to_use_head;
> > + int next_to_use_frag;
> > + dma_addr_t dma;
> > + int buf_num;
> > + int seg_num;
> > + int size;
> > + int ret;
> > + int i;
> > +
> > + if (!skb || !ring)
> > + return -ENOMEM;
>
> Really?
Ok. Checking for skb=NULL at this stage is definitely an overkill in anycase.
Will remove :)
>
> > +
> > + /* Prefetch the data used later */
> > + prefetch(skb->data);
> > +
> > + switch (priv->ops.maybe_stop_tx(&skb, &buf_num, ring)) {
> > + case -EBUSY:
> > + u64_stats_update_begin(&ring->syncp);
> > + ring->stats.tx_busy++;
> > + u64_stats_update_end(&ring->syncp);
> > +
> > + goto out_net_tx_busy;
> > + case -ENOMEM:
> > + u64_stats_update_begin(&ring->syncp);
> > + ring->stats.sw_err_cnt++;
> > + u64_stats_update_end(&ring->syncp);
> > + netdev_err(netdev, "no memory to xmit!\n");
> > +
> > + goto out_err_tx_ok;
> > + default:
> > + break;
> > + }
>
> Move the u64_stats_update_begin() and u64_stats_update_end() out of the
> switch case?
Wouldn't it be inefficient? There is a maybe_stop_tx() which gets
called and this is a live data-path? For most of the cases we will end
up taking and releasing lock un-necessarily which does not seems to be
correct to me and might impact performance of data-path.
>
> > +
> > + /* No. of segments (plus a header) */
> > + seg_num = skb_shinfo(skb)->nr_frags + 1;
> > + /* Fill the first part */
> > + size = skb_headlen(skb);
> > +
> > + next_to_use_head = ring->next_to_use;
> > +
> > + dma = dma_map_single(dev, skb->data, size, DMA_TO_DEVICE);
> > + if (dma_mapping_error(dev, dma)) {
> > + netdev_err(netdev, "TX head DMA map failed\n");
> > + ring->stats.sw_err_cnt++;
> > + goto out_err_tx_ok;
> > + }
> > +
> > + ret = priv->ops.fill_desc(ring, skb, size, dma, seg_num == 1 ? 1
> : 0,
> > + DESC_TYPE_SKB);
> > + if (ret)
> > + goto head_dma_map_err;
> > +
> > + next_to_use_frag = ring->next_to_use;
> > + /* Fill the fragments */
> > + for (i = 1; i < seg_num; i++) {
> > + frag = &skb_shinfo(skb)->frags[i - 1];
> > + size = skb_frag_size(frag);
> > + dma = skb_frag_dma_map(dev, frag, 0, size, DMA_TO_DEVICE);
> > + if (dma_mapping_error(dev, dma)) {
> > + netdev_err(netdev, "TX frag(%d) DMA map failed\n",
> i);
> > + ring->stats.sw_err_cnt++;
> > + goto frag_dma_map_err;
> > + }
> > + ret = priv->ops.fill_desc(ring, skb_frag_page(frag), size,
> dma,
> > + seg_num - 1 == i ? 1 : 0,
> > + DESC_TYPE_PAGE);
> > +
> > + if (ret)
> > + goto frag_dma_map_err;
> > + }
> > +
> > + /* Complete translate all packets */
> > + dev_queue = netdev_get_tx_queue(netdev, ring_data->queue_index);
> > + netdev_tx_sent_queue(dev_queue, skb->len);
> > +
> > + wmb(); /* Commit all data before submit */
>
> dma_wmb()?
wmb --> DSB on AMR64 --> dmb(read-writes, system-wide)
dma_wmb --->DMB on ARM64--> dmb(reads, outer-shareable-domain)
you mean dma_wmb is more optimized? Will this be okay if
data was modified during TX path leg? Please correct me if I
am having gap in my understanding here.
Thanks
Salil
>
> > +
> > + hnae_queue_xmit(ring->tqp, buf_num);
> > +
> > + u64_stats_update_begin(&ring->syncp);
> > + ring->stats.tx_pkts++;
> > + ring->stats.tx_bytes += skb->len;
>
> This can cause use after free bugs, past the point where you tell the
> HW
> to transmit, you can have you reclaiming process run and free your SKBs
> in e.g: hard IRQ -> soft IRQ context, so this is not safe at all.
>
> Besides that, you should always update your TX statistics in the
> reclaiming/completion process because only then do you know for sure
> that the HW has actually transmitted the packets. There is no such
> guarantee here yet.
Yes, well identified. Removed from here and placed in TX completion leg.
Thanks
>
> > + u64_stats_update_end(&ring->syncp);
> > +
> > + return NETDEV_TX_OK;
> > +
> > +frag_dma_map_err:
> > + hns_nic_dma_unmap(ring, next_to_use_frag);
> > +
> > +head_dma_map_err:
> > + hns_nic_dma_unmap(ring, next_to_use_head);
> > +
> > +out_err_tx_ok:
> > + dev_kfree_skb_any(skb);
> > + return NETDEV_TX_OK;
> > +
> > +out_net_tx_busy:
> > + netif_stop_subqueue(netdev, ring_data->queue_index);
> > + smp_mb(); /* Commit all data before submit */
> > +
> > + return NETDEV_TX_BUSY;
> > +}
> > +
> > +static netdev_tx_t hns3_nic_net_xmit(struct sk_buff *skb,
> > + struct net_device *netdev)
> > +{
> > + struct hns3_nic_priv *priv = netdev_priv(netdev);
> > + int ret;
> > +
> > + ret = hns3_nic_net_xmit_hw(netdev, skb,
> > + &tx_ring_data(priv, skb->queue_mapping));
> > + if (ret == NETDEV_TX_OK) {
> > + netif_trans_update(netdev);
> > + netdev->stats.tx_bytes += skb->len;
> > + netdev->stats.tx_packets++;
> > + }
>
> Certainly not, this is an even bigger use after free.
I agree. Changed. Thanks for identifying.
>
> > +
> > + return (netdev_tx_t)ret;
>
> Just inline hns3_nic_net_xmit_hw() in this function, or remove
> nic_net_xmit() and have them be consistent in returning netdev_tx_t.
>
> > +}
> > +
> > +static int hns3_nic_net_set_mac_address(struct net_device *netdev,
> void *p)
> > +{
> > + struct hns3_nic_priv *priv = netdev_priv(netdev);
> > + struct hnae3_handle *h = priv->ae_handle;
> > + struct sockaddr *mac_addr = p;
> > + int ret;
> > +
> > + if (!mac_addr || !is_valid_ether_addr((const u8 *)mac_addr-
> >sa_data))
> > + return -EADDRNOTAVAIL;
> > +
> > + ret = h->ae_algo->ops->set_mac_addr(h, mac_addr->sa_data);
> > + if (ret) {
> > + netdev_err(netdev, "set_mac_address fail, ret=%d!\n", ret);
> > + return ret;
> > + }
> > +
> > + ether_addr_copy(netdev->dev_addr, mac_addr->sa_data);
> > +
> > + return 0;
> > +}
> > +
> > +static int hns3_nic_set_features(struct net_device *netdev,
> > + netdev_features_t features)
> > +{
> > + struct hns3_nic_priv *priv = netdev_priv(netdev);
> > +
> > + if (features & (NETIF_F_TSO | NETIF_F_TSO6)) {
> > + priv->ops.fill_desc = hns3_fill_desc_tso;
>
> Is this safe against pending transmissions? How do we not end-up with
> some packes being submitting with the TSO descriptors and some with the
> normal descriptors?
This pointer get initialized early to correct function or if we
change the TSO status to something else. Hope I understood your question
correctly here?
>
> > + priv->ops.maybe_stop_tx = hns3_nic_maybe_stop_tso;
> > + } else {
> > + priv->ops.fill_desc = hns3_fill_desc;
> > + priv->ops.maybe_stop_tx = hns3_nic_maybe_stop_tx;
> > + }
> > +
> > + netdev->features = features;
> > + return 0;
> > +}
> > +
> > +static void
> > +hns3_nic_get_stats64(struct net_device *netdev, struct
> rtnl_link_stats64 *stats)
> > +{
> > + struct hns3_nic_priv *priv = netdev_priv(netdev);
> > + int queue_num = priv->ae_handle->kinfo.num_tqps;
> > + struct hns3_enet_ring *ring;
> > + unsigned int start;
> > + u64 tx_bytes = 0;
> > + u64 rx_bytes = 0;
> > + u64 tx_pkts = 0;
> > + u64 rx_pkts = 0;
> > + int idx;
>
> unsigned int idx
Ok.
>
>
> > +static int hns3_setup_tc(struct net_device *netdev, u8 tc)
> > +{
> > + struct hns3_nic_priv *priv = netdev_priv(netdev);
> > + struct hnae3_handle *h = priv->ae_handle;
> > + struct hnae3_knic_private_info *kinfo = &h->kinfo;
> > + int i, ret;
>
> unsigned int i;
Ok.
>
> > +
> > + if (tc > HNAE3_MAX_TC)
> > + return -EINVAL;
>
> Should not you be testing for the supported TC offloads that you are
> being asked to offload somehow?
In a wrapper above.
>
> > +
> > + if (kinfo->num_tc == tc)
> > + return 0;
> > +
> > + if (!netdev)
> > + return -EINVAL;
> > +
> > + if (!tc) {
> > + netdev_reset_tc(netdev);
> > + return 0;
> > + }
> > +
> > + /* Set num_tc for netdev */
> > + ret = netdev_set_num_tc(netdev, tc);
> > + if (ret)
> > + return ret;
> > +
> > + /* Set per TC queues for the VSI */
> > + for (i = 0; i < HNAE3_MAX_TC; i++) {
> > + if (kinfo->tc_info[i].enable)
> > + netdev_set_tc_queue(netdev,
> > + kinfo->tc_info[i].tc,
> > + kinfo->tc_info[i].tqp_count,
> > + kinfo->tc_info[i].tqp_offset);
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static int hns3_nic_setup_tc(struct net_device *dev, u32 handle,
> > + u32 chain_index, __be16 protocol,
> > + struct tc_to_netdev *tc)
> > +{
> > + if (handle != TC_H_ROOT || tc->type != TC_SETUP_MQPRIO)
> > + return -EINVAL;
>
> Oh ok, we are testing it here, can you just inline hns3_setup_tc() in
> this function body then?
Function hns3_setup_tc() too bug to be inlined. Did you meant the
Wrapper function to it i.e. hns3_nic_setup_tc()?
As such, compiler can do optimizations for such code itself.
>
>
>
>
> > +int hns3_clean_tx_ring(struct hns3_enet_ring *ring, int budget)
> > +{
> > + struct net_device *netdev = ring->tqp->handle->kinfo.netdev;
> > + struct netdev_queue *dev_queue;
> > + int bytes, pkts;
> > + int head;
> > +
> > + head = readl_relaxed(ring->tqp->io_base +
> HNS3_RING_TX_RING_HEAD_REG);
> > + rmb(); /* Make sure head is ready before touch any data */
> > +
> > + if (is_ring_empty(ring) || head == ring->next_to_clean)
> > + return 0; /* no data to poll */
> > +
> > + if (!is_valid_clean_head(ring, head)) {
> > + netdev_err(netdev, "wrong head (%d, %d-%d)\n", head,
> > + ring->next_to_use, ring->next_to_clean);
> > +
> > + u64_stats_update_begin(&ring->syncp);
> > + ring->stats.io_err_cnt++;
> > + u64_stats_update_end(&ring->syncp);
> > + return -EIO;
> > + }
> > +
> > + bytes = 0;
> > + pkts = 0;
> > + while (head != ring->next_to_clean && budget) {
>
> TX completion should not be bound to the NAPI budget, just clean the
> entire ring.
If number of descriptors per-ring are large then even TX desc reclaiming
can be an overhead. I referred to other drivers like Intel i40e/ixgbe/mlx
etc. all have similar budget driven TX desc. reclaiming.
>
> > + hns3_nic_reclaim_one_desc(ring, &bytes, &pkts);
> > + /* Issue prefetch for next Tx descriptor */
> > + prefetch(&ring->desc_cb[ring->next_to_clean]);
> > + budget--;
> > + }
> > +
> > + ring->tqp_vector->tx_group.total_bytes += bytes;
> > + ring->tqp_vector->tx_group.total_packets += pkts;
> > +
> > + dev_queue = netdev_get_tx_queue(netdev, ring->tqp->tqp_index);
> > + netdev_tx_completed_queue(dev_queue, pkts, bytes);
>
> Where is flow control happening? Should not you wake the transmit queue
> if you had to stop it somehow?
Forgive me, I could not get this part. Flow control of what?
>
> I kind of stopped reviewing here.
> --
> Florian
^ permalink raw reply
* Re: Performance regression with virtio_net
From: Michael S. Tsirkin @ 2017-07-27 20:38 UTC (permalink / raw)
To: Seth Forshee; +Cc: Jason Wang, virtualization, netdev
In-Reply-To: <20170727170942.ub24lwgrl3x2outp@ubuntu-hedt>
On Thu, Jul 27, 2017 at 12:09:42PM -0500, Seth Forshee wrote:
> I'm seeing a performance regression with virtio_net that looks to have
> started in 4.12-rc1. I only see it in one context though, downloading
> snap packages from the Ubuntu snap store. For example:
>
> https://api.snapcraft.io/api/v1/snaps/download/b8X2psL1ryVrPt5WEmpYiqfr5emixTd7_1797.snap
>
> which redirects to Internap's CDN. Normally this downloads in a few
> seconds at ~10 MB/s, but with 4.12 and 4.13 it takes minutes with a rate
> of ~150 KB/s. Everything else I've tried downloads as normal speeds.
So just wget that URL should be enough?
> I bisected this to 680557cf79f8 "virtio_net: rework mergeable buffer
> handling". If I revert this on top of 4.13-rc2 (along with other changes
> needed to successfully revert it) speeds return to normal.
>
> Thanks,
> Seth
Interesting. A more likely suspect would be
e377fcc8486d40867c6c217077ad0fa40977e060 - could you please try
reverting that one instead?
Also, could you please look at mergeable_rx_buffer_size in sysfs with
and without the change?
--
MST
^ permalink raw reply
* Re: [PATCH V2 net] Revert "vhost: cache used event for better performance"
From: Michael S. Tsirkin @ 2017-07-27 20:29 UTC (permalink / raw)
To: Jason Wang; +Cc: kvm, virtualization, netdev, linux-kernel
In-Reply-To: <1501125725-24529-1-git-send-email-jasowang@redhat.com>
On Thu, Jul 27, 2017 at 11:22:05AM +0800, Jason Wang wrote:
> This reverts commit 809ecb9bca6a9424ccd392d67e368160f8b76c92. Since it
> was reported to break vhost_net. We want to cache used event and use
> it to check for notification. The assumption was that guest won't move
> the event idx back, but this could happen in fact when 16 bit index
> wraps around after 64K entries.
>
> Signed-off-by: Jason Wang <jasowang@redhat.com>
Acked-by: Michael S. Tsirkin <mst@redhat.com>
> ---
> - Changes from V1: tweak commit log
> - The patch is needed for -stable.
> ---
> drivers/vhost/vhost.c | 28 ++++++----------------------
> drivers/vhost/vhost.h | 3 ---
> 2 files changed, 6 insertions(+), 25 deletions(-)
>
> diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
> index e4613a3..9cb3f72 100644
> --- a/drivers/vhost/vhost.c
> +++ b/drivers/vhost/vhost.c
> @@ -308,7 +308,6 @@ static void vhost_vq_reset(struct vhost_dev *dev,
> vq->avail = NULL;
> vq->used = NULL;
> vq->last_avail_idx = 0;
> - vq->last_used_event = 0;
> vq->avail_idx = 0;
> vq->last_used_idx = 0;
> vq->signalled_used = 0;
> @@ -1402,7 +1401,7 @@ long vhost_vring_ioctl(struct vhost_dev *d, int ioctl, void __user *argp)
> r = -EINVAL;
> break;
> }
> - vq->last_avail_idx = vq->last_used_event = s.num;
> + vq->last_avail_idx = s.num;
> /* Forget the cached index value. */
> vq->avail_idx = vq->last_avail_idx;
> break;
> @@ -2241,6 +2240,10 @@ static bool vhost_notify(struct vhost_dev *dev, struct vhost_virtqueue *vq)
> __u16 old, new;
> __virtio16 event;
> bool v;
> + /* Flush out used index updates. This is paired
> + * with the barrier that the Guest executes when enabling
> + * interrupts. */
> + smp_mb();
>
> if (vhost_has_feature(vq, VIRTIO_F_NOTIFY_ON_EMPTY) &&
> unlikely(vq->avail_idx == vq->last_avail_idx))
> @@ -2248,10 +2251,6 @@ static bool vhost_notify(struct vhost_dev *dev, struct vhost_virtqueue *vq)
>
> if (!vhost_has_feature(vq, VIRTIO_RING_F_EVENT_IDX)) {
> __virtio16 flags;
> - /* Flush out used index updates. This is paired
> - * with the barrier that the Guest executes when enabling
> - * interrupts. */
> - smp_mb();
> if (vhost_get_avail(vq, flags, &vq->avail->flags)) {
> vq_err(vq, "Failed to get flags");
> return true;
> @@ -2266,26 +2265,11 @@ static bool vhost_notify(struct vhost_dev *dev, struct vhost_virtqueue *vq)
> if (unlikely(!v))
> return true;
>
> - /* We're sure if the following conditions are met, there's no
> - * need to notify guest:
> - * 1) cached used event is ahead of new
> - * 2) old to new updating does not cross cached used event. */
> - if (vring_need_event(vq->last_used_event, new + vq->num, new) &&
> - !vring_need_event(vq->last_used_event, new, old))
> - return false;
> -
> - /* Flush out used index updates. This is paired
> - * with the barrier that the Guest executes when enabling
> - * interrupts. */
> - smp_mb();
> -
> if (vhost_get_avail(vq, event, vhost_used_event(vq))) {
> vq_err(vq, "Failed to get used event idx");
> return true;
> }
> - vq->last_used_event = vhost16_to_cpu(vq, event);
> -
> - return vring_need_event(vq->last_used_event, new, old);
> + return vring_need_event(vhost16_to_cpu(vq, event), new, old);
> }
>
> /* This actually signals the guest, using eventfd. */
> diff --git a/drivers/vhost/vhost.h b/drivers/vhost/vhost.h
> index f720958..bb7c29b 100644
> --- a/drivers/vhost/vhost.h
> +++ b/drivers/vhost/vhost.h
> @@ -115,9 +115,6 @@ struct vhost_virtqueue {
> /* Last index we used. */
> u16 last_used_idx;
>
> - /* Last used evet we've seen */
> - u16 last_used_event;
> -
> /* Used flags */
> u16 used_flags;
>
> --
> 2.7.4
^ permalink raw reply
* Re: [PATCH v3 1/3] ptp: introduce ptp auxiliary worker
From: Richard Cochran @ 2017-07-27 20:08 UTC (permalink / raw)
To: Grygorii Strashko
Cc: David S. Miller, netdev, Sekhar Nori, linux-kernel, linux-omap,
Wingman Kwok, Ivan Khoronzhuk, John Stultz, Thomas Gleixner
In-Reply-To: <20170726221138.12986-2-grygorii.strashko@ti.com>
On Wed, Jul 26, 2017 at 05:11:36PM -0500, Grygorii Strashko wrote:
> @@ -217,6 +231,19 @@ struct ptp_clock *ptp_clock_register(struct ptp_clock_info *info,
> mutex_init(&ptp->pincfg_mux);
> init_waitqueue_head(&ptp->tsev_wq);
>
> + if (ptp->info->do_aux_work) {
> + char *worker_name = kasprintf(GFP_KERNEL, "ptp%d", ptp->index);
This string is allocated but never freed.
> + kthread_init_delayed_work(&ptp->aux_work, ptp_aux_kworker);
> + ptp->kworker = kthread_create_worker(0, worker_name ?
> + worker_name : info->name);
> + if (IS_ERR(ptp->kworker)) {
> + err = PTR_ERR(ptp->kworker);
> + pr_err("failed to create ptp aux_worker %d\n", err);
> + goto kworker_err;
> + }
> + }
> +
Thanks,
Richard
^ permalink raw reply
* Re: [PATCHv4 net] ipv6: no need to check rt->dst.error when get route info
From: Roopa Prabhu @ 2017-07-27 19:52 UTC (permalink / raw)
To: Hangbin Liu; +Cc: netdev@vger.kernel.org, Cong Wang, David Ahern
In-Reply-To: <1501172736-13685-1-git-send-email-liuhangbin@gmail.com>
On Thu, Jul 27, 2017 at 9:25 AM, Hangbin Liu <liuhangbin@gmail.com> wrote:
> After commit 18c3a61c4264 ("net: ipv6: RTM_GETROUTE: return matched fib
> result when requested"). When we get a prohibit ertry, we will return
> -EACCES directly instead of dump route info.
>
> Fix it by remove the rt->dst.error check.
>
> Before fix:
> \# ip -6 route add prohibit 2003::/64 dev eth1
> \# ip -6 route get fibmatch 2003::1
> RTNETLINK answers: Permission denied
> \# ip -6 route add unreachable 2004::/64 dev eth1
> \# ip -6 route get fibmatch 2004::1
> RTNETLINK answers: No route to host
>
> After fix:
> \# ip -6 route add prohibit 2003::/64 dev eth1
> \# ip -6 route get fibmatch 2003::1
> prohibit 2003::/64 dev lo metric 1024 error -13 pref medium
> \# ip -6 route add unreachable 2004::/64 dev eth1
> \# ip -6 route get fibmatch 2004::1
> unreachable 2004::/64 dev lo metric 1024 error -113 pref medium
>
> Fixes: 18c3a61c4264 ("net: ipv6: RTM_GETROUTE: return matched fib...")
> Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
> ---
Acked-by: Roopa Prabhu <roopa@cumulusnetworks.com>
^ permalink raw reply
* [PATCH net-next] liquidio: bump up driver version to match newer NIC firmware
From: Felix Manlunas @ 2017-07-27 19:32 UTC (permalink / raw)
To: davem; +Cc: netdev, raghu.vatsavayi, derek.chickles, satananda.burla
Bump up driver version to match newer NIC firmware. Also update
nic_rx_stats (a struct common to host driver and firmware) by adding a new
field: fw_total_fwd_bytes.
Signed-off-by: Felix Manlunas <felix.manlunas@cavium.com>
Signed-off-by: Raghu Vatsavayi <raghu.vatsavayi@cavium.com>
---
drivers/net/ethernet/cavium/liquidio/liquidio_common.h | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/net/ethernet/cavium/liquidio/liquidio_common.h b/drivers/net/ethernet/cavium/liquidio/liquidio_common.h
index 53aaf41..3b9e364 100644
--- a/drivers/net/ethernet/cavium/liquidio/liquidio_common.h
+++ b/drivers/net/ethernet/cavium/liquidio/liquidio_common.h
@@ -27,8 +27,8 @@
#define LIQUIDIO_PACKAGE ""
#define LIQUIDIO_BASE_MAJOR_VERSION 1
-#define LIQUIDIO_BASE_MINOR_VERSION 5
-#define LIQUIDIO_BASE_MICRO_VERSION 1
+#define LIQUIDIO_BASE_MINOR_VERSION 6
+#define LIQUIDIO_BASE_MICRO_VERSION 0
#define LIQUIDIO_BASE_VERSION __stringify(LIQUIDIO_BASE_MAJOR_VERSION) "." \
__stringify(LIQUIDIO_BASE_MINOR_VERSION)
#define LIQUIDIO_MICRO_VERSION "." __stringify(LIQUIDIO_BASE_MICRO_VERSION)
@@ -768,6 +768,7 @@ struct nic_rx_stats {
/* firmware stats */
u64 fw_total_rcvd;
u64 fw_total_fwd;
+ u64 fw_total_fwd_bytes;
u64 fw_err_pko;
u64 fw_err_link;
u64 fw_err_drop;
^ permalink raw reply related
* [PATCH net] bpf: don't indicate success when copy_from_user fails
From: Daniel Borkmann @ 2017-07-27 19:02 UTC (permalink / raw)
To: davem; +Cc: kafai, alexei.starovoitov, netdev, Daniel Borkmann
err in bpf_prog_get_info_by_fd() still holds 0 at that time from prior
check_uarg_tail_zero() check. Explicitly return -EFAULT instead, so
user space can be notified of buggy behavior.
Fixes: 1e2709769086 ("bpf: Add BPF_OBJ_GET_INFO_BY_FD")
Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
---
kernel/bpf/syscall.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index 045646d..84bb399 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -1289,7 +1289,7 @@ static int bpf_prog_get_info_by_fd(struct bpf_prog *prog,
info_len = min_t(u32, sizeof(info), info_len);
if (copy_from_user(&info, uinfo, info_len))
- return err;
+ return -EFAULT;
info.type = prog->type;
info.id = prog->aux->id;
--
1.9.3
^ permalink raw reply related
* Re: [PATCH v2 2/4] can: fixed-transceiver: Add documentation for CAN fixed transceiver bindings
From: Oliver Hartkopp @ 2017-07-27 18:47 UTC (permalink / raw)
To: Franklin S Cooper Jr, Andrew Lunn
Cc: linux-kernel-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA, netdev-u79uwXL29TY76Z2rM5mHXA,
linux-can-u79uwXL29TY76Z2rM5mHXA, wg-5Yr1BZd7O62+XT7JhA+gdA,
mkl-bIcnvbaLZ9MEGnE8C9+IrQ, robh+dt-DgEjT+Ai2ygdnm+yROfE0A,
quentin.schulz-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8,
dev.kurt-yI9piX4KPfawT/RRk36CISFp6vIno51x,
sergei.shtylyov-M4DtvfQ/ZS1MRgGoP+s0PdBPR1lH4CV8
In-Reply-To: <a77fe395-33c7-9405-b51a-5d3372e5c58b-l0cyMroinI0@public.gmane.org>
On 07/26/2017 08:29 PM, Franklin S Cooper Jr wrote:
>
> I'm fine with switching to using bitrate instead of speed. Kurk was
> originally the one that suggested to use the term arbitration and data
> since thats how the spec refers to it. Which I do agree with. But your
> right that in the drivers (struct can_priv) we just use bittiming and
> data_bittiming (CAN-FD timings). I don't think adding "fd" into the
> property name makes sense unless we are calling it something like
> "max-canfd-bitrate" which I would agree is the easiest to understand.
>
> So what is the preference if we end up sticking with two properties?
> Option 1 or 2?
>
> 1)
> max-bitrate
> max-data-bitrate
>
> 2)
> max-bitrate
> max-canfd-bitrate
>
>
1
>> A CAN transceiver is limited in bandwidth. But you only have one RX and
>> one TX line between the CAN controller and the CAN transceiver. The
>> transceiver does not know about CAN FD - it has just a physical(!) layer
>> with a limited bandwidth. This is ONE limitation.
>>
>> So I tend to specify only ONE 'max-bitrate' property for the
>> fixed-transceiver binding.
>>
>> The fact whether the CAN controller is CAN FD capable or not is provided
>> by the netlink configuration interface for CAN controllers.
>
> Part of the reasoning to have two properties is to indicate that you
> don't support CAN FD while limiting the "arbitration" bit rate.
??
It's a physical layer device which only has a bandwidth limitation.
The transceiver does not know about CAN FD.
> With one
> property you can not determine this and end up having to make some
> assumptions that can quickly end up biting people.
Despite the fact that the transceiver does not know anything about ISO
layer 2 (CAN/CAN FD) the properties should look like
max-bitrate
canfd-capable
then.
But when the tranceiver is 'canfd-capable' agnostic, why provide a
property for it?
Maybe I'm wrong but I still can't follow your argumentation ideas.
Regards,
Oliver
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH RFC 11/13] phylink: add support for MII ioctl access to Clause 45 PHYs
From: Andrew Lunn @ 2017-07-27 18:47 UTC (permalink / raw)
To: Russell King; +Cc: Florian Fainelli, netdev
In-Reply-To: <E1da0Qq-0007MB-U1@rmk-PC.armlinux.org.uk>
On Tue, Jul 25, 2017 at 03:03:28PM +0100, Russell King wrote:
> Add support for reading and writing the clause 45 MII registers.
>
> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Andrew
^ permalink raw reply
* Re: [PATCH v7 2/3] PCI: Enable PCIe Relaxed Ordering if supported
From: Raj, Ashok @ 2017-07-27 18:42 UTC (permalink / raw)
To: Casey Leedom
Cc: Ding Tianhong, Alexander Duyck, Netdev, Bjorn Helgaas,
linux-arm-kernel@lists.infradead.org, David.Laight@aculab.com,
Alex Williamson, l.stach@pengutronix.de,
Suravee.Suthikulpanit@amd.com, catalin.marinas@arm.com,
linux-pci@vger.kernel.org, will.deacon@arm.com, Sinan Kaya,
robin.murphy@arm.com, linux-kernel@vger.kernel.org,
"davem@davemloft.n
In-Reply-To: <MWHPR12MB1600556FFDCE3EC810D97591C8BE0@MWHPR12MB1600.namprd12.prod.outlook.com>
Hi Casey
> | Still no Intel and AMD guys has ack this, this is what I am worried about,
> | should I ping some man again ?
I can ack the patch set for Intel specific changes. Now that the doc is made
public :-).
Can you/Ding resend the patch series, i do have the most recent v7, some
of the commit message wasn't easy to ready. Seems like this patch has
gotten bigger than originally intended, but seems to be for the overall
good :-).
Sorry for staying silent up until now.
Cheers,
Ashok
^ permalink raw reply
* RE: [Intel-wired-lan] [PATCH] igb: support BCM54616 PHY
From: Wyborny, Carolyn @ 2017-07-27 18:35 UTC (permalink / raw)
To: Florian Fainelli, Andrew Lunn, Brown, Aaron F
Cc: John W. Linville, netdev@vger.kernel.org,
intel-wired-lan@lists.osuosl.org
In-Reply-To: <76c8ea56-8e4e-a85c-abe5-695b46c99308@gmail.com>
> -----Original Message-----
> From: netdev-owner@vger.kernel.org [mailto:netdev-
> owner@vger.kernel.org] On Behalf Of Florian Fainelli
> Sent: Thursday, July 27, 2017 10:58 AM
> To: Andrew Lunn <andrew@lunn.ch>; Brown, Aaron F
> <aaron.f.brown@intel.com>
> Cc: John W. Linville <linville@tuxdriver.com>; netdev@vger.kernel.org; intel-
> wired-lan@lists.osuosl.org
> Subject: Re: [Intel-wired-lan] [PATCH] igb: support BCM54616 PHY
>
> On 07/27/2017 08:37 AM, Andrew Lunn wrote:
> > On Thu, Jul 27, 2017 at 12:40:01AM +0000, Brown, Aaron F wrote:
> >>> From: Intel-wired-lan [mailto:intel-wired-lan-bounces@osuosl.org] On
> Behalf
> >>> Of John W. Linville
> >>> Sent: Friday, July 21, 2017 11:12 AM
> >>> To: netdev@vger.kernel.org
> >>> Cc: intel-wired-lan@lists.osuosl.org; John W. Linville
> >>> <linville@tuxdriver.com>
> >>> Subject: [Intel-wired-lan] [PATCH] igb: support BCM54616 PHY
> >>>
> >>> The management port on an Edgecore AS7712-32 switch uses an igb MAC,
> >>> but
> >>> it uses a BCM54616 PHY. Without a patch like this, loading the igb
> >>> module produces dmesg output like this:
> >>>
> >>> [ 3.439125] igb: Copyright (c) 2007-2014 Intel Corporation.
> >>> [ 3.439866] igb: probe of 0000:00:14.0 failed with error -2
> >>>
> >>> Signed-off-by: John W. Linville <linville@tuxdriver.com>
> >>> Cc: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
> >>> ---
> >>> drivers/net/ethernet/intel/igb/e1000_82575.c | 6 ++++++
> >>> drivers/net/ethernet/intel/igb/e1000_defines.h | 1 +
> >>> drivers/net/ethernet/intel/igb/e1000_hw.h | 1 +
> >>> 3 files changed, 8 insertions(+)
> >>
> >> I do not have the specific hardware (Edgecore switch) but as far as
> regression tests go this works fine.
> >> Tested-by: Aaron Brown <aaron.f.brown@intel.com>
> >
> > Sorry, missed the initial post, so replying to a reply.
> >
> > Linux has supported the BCM54616 PHY since April 2015. If the Intel
> > drivers used the Linux PHY drivers, you would not of had this problem.
> >
> > It would be good if somebody spent the time to migrate these MAC
> > drivers to use the common Linux PHY infrastructure.
>
> I suspect there is a design pattern within the Intel drivers to share as
> much low-level code as possible between OSes and only have some
> Linux-ism where necessary (e.g: net_device, ethtool etc.).
>
> PHY code is a pain in general, especially if you are serious about
> testing interoperability (which is where you can spend tons of $$$ with
> little reward but just say: yes it works), so it may make sense to share
> it across different OSes.
>
> I too, wish there was more sharing, but considering that this works for
> the Intel driver, there is little incentive in doing this I suppose...
> --
> Florian
Yes Florian you are correct generally, but future implementations have not been ruled out. With this driver, we had our custom phy init code tested and released for several products before phylib existed. I began research on a phylib implementation for it at one point, but internal product decisions ended up lowering the priority of that work.
Thanks,
Carolyn
Carolyn Wyborny
Linux Development
Networking Division
Intel Corporation
^ permalink raw reply
* Re: [PATCHv2 iproute2] utils: return default family when rtm_family is not RTNL_FAMILY_IPMR/IP6MR
From: Stephen Hemminger @ 2017-07-27 18:28 UTC (permalink / raw)
To: Phil Sutter; +Cc: Hangbin Liu, netdev, Nikolay Aleksandrov
In-Reply-To: <20170727100302.GP2235@orbyte.nwl.cc>
On Thu, 27 Jul 2017 12:03:02 +0200
Phil Sutter <phil@nwl.cc> wrote:
> On Thu, Jul 27, 2017 at 05:44:15PM +0800, Hangbin Liu wrote:
> > When we get a multicast route, the rtm_type is RTN_MULTICAST, but the
> > rtm_family may be AF_INET. If we only check the type with RTNL_FAMILY_IPMR,
> > we will get malformed address. e.g.
> >
> > + ip -4 route add multicast 172.111.1.1 dev em1 table main
> >
> > Before fix:
> > + ip route list type multicast table main
> > multicast ac6f:101:800:400:400:0:3c00:0 dev em1 scope link
> >
> > After fix:
> > + ip route list type multicast table main
> > multicast 172.111.1.1 dev em1 scope link
> >
> > Fixes: 56e3eb4c3400 ("ip: route: fix multicast route dumps")
> > Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
>
> Acked-by: Phil Sutter <phil@nwl.cc>
Applied, thanks.
^ permalink raw reply
* Re: [PATCH] netns: more input validation
From: Stephen Hemminger @ 2017-07-27 18:26 UTC (permalink / raw)
To: Matteo Croce; +Cc: netdev
In-Reply-To: <20170725133031.7735-1-mcroce@redhat.com>
On Tue, 25 Jul 2017 15:30:31 +0200
Matteo Croce <mcroce@redhat.com> wrote:
> ip netns accepts invalid input as namespace name like an empty string or a
> string longer than the maximum file name length.
> Check that the netns name is not empty and less than or equal to NAME_MAX.
>
> Signed-off-by: Matteo Croce <mcroce@redhat.com>
Sure, applied.
^ permalink raw reply
* Re: [PATCH iproute2] geneve: support for modifying geneve device
From: Stephen Hemminger @ 2017-07-27 18:24 UTC (permalink / raw)
To: Girish Moodalbail; +Cc: netdev
In-Reply-To: <1501035103-3090-1-git-send-email-girish.moodalbail@oracle.com>
On Tue, 25 Jul 2017 19:11:43 -0700
Girish Moodalbail <girish.moodalbail@oracle.com> wrote:
> Ability to change geneve device attributes was added to kernel through
> commit 5b861f6baa3a ("geneve: add rtnl changelink support"), however one
> cannot do the same through ip-link(8) command. Changing the allowed
> geneve device attributes using 'ip link set <geneve_name> type geneve id
> <geneve_id> <allowed_attributes>' currently fails with 'operation not
> supported' error. This patch adds support for it.
>
> Signed-off-by: Girish Moodalbail <girish.moodalbail@oracle.com>
Looks good, applied.
Please send a follow on patch to update usage help and man page.
^ permalink raw reply
* Re: [Intel-wired-lan] [PATCH] igb: support BCM54616 PHY
From: Andrew Lunn @ 2017-07-27 18:15 UTC (permalink / raw)
To: Jonathan Toppins
Cc: Brown, Aaron F, John W. Linville, netdev@vger.kernel.org,
intel-wired-lan@lists.osuosl.org
In-Reply-To: <211b5efd-f8d5-cf44-4e0d-dc661c144b7d@redhat.com>
> I recall someone mentioning that there were plans to have phylib support
> [Q]SFP[+] modules as well. I am very interested in that work, if someone
> has patches/plans I would like to take a look.
Hi Jon
The first part was posted yesterday:
https://www.spinics.net/lists/netdev/msg445767.html
Andrew
^ permalink raw reply
* [PATCH v2] net: inet: diag: expose sockets cgroup classid
From: Levin, Alexander (Sasha Levin) @ 2017-07-27 18:11 UTC (permalink / raw)
To: davem@davemloft.net
Cc: netdev@vger.kernel.org, linux-kernel@vger.kernel.org,
xiyou.wangcong@gmail.com, Levin, Alexander (Sasha Levin)
This is useful for directly looking up a task based on class id rather than
having to scan through all open file descriptors.
Signed-off-by: Sasha Levin <alexander.levin@verizon.com>
---
Changes in V2:
- Addressed comments from Cong Wang (use nla_put_u32())
include/uapi/linux/inet_diag.h | 1 +
net/ipv4/inet_diag.c | 10 ++++++++++
2 files changed, 11 insertions(+)
diff --git a/include/uapi/linux/inet_diag.h b/include/uapi/linux/inet_diag.h
index bbe201047df6..678496897a68 100644
--- a/include/uapi/linux/inet_diag.h
+++ b/include/uapi/linux/inet_diag.h
@@ -142,6 +142,7 @@ enum {
INET_DIAG_PAD,
INET_DIAG_MARK,
INET_DIAG_BBRINFO,
+ INET_DIAG_CLASS_ID,
__INET_DIAG_MAX,
};
diff --git a/net/ipv4/inet_diag.c b/net/ipv4/inet_diag.c
index 3828b3a805cd..2c2445d4bb58 100644
--- a/net/ipv4/inet_diag.c
+++ b/net/ipv4/inet_diag.c
@@ -274,6 +274,16 @@ int inet_sk_diag_fill(struct sock *sk, struct inet_connection_sock *icsk,
goto errout;
}
+ if (ext & (1 << (INET_DIAG_CLASS_ID - 1))) {
+ u32 classid = 0;
+
+#ifdef CONFIG_SOCK_CGROUP_DATA
+ classid = sock_cgroup_classid(&sk->sk_cgrp_data);
+#endif
+
+ nla_put_u32(skb, INET_DIAG_CLASS_ID, classid);
+ }
+
out:
nlmsg_end(skb, nlh);
return 0;
--
2.11.0
^ permalink raw reply related
* Re: [PATCH V4 net-next 7/8] net: hns3: Add Ethtool support to HNS3 driver
From: Florian Fainelli @ 2017-07-27 18:04 UTC (permalink / raw)
To: Salil Mehta, davem-fT/PcQaiUtIeIZ0/mPfg9Q@public.gmane.org
Cc: Zhuangyuzeng (Yisen), huangdaode, lipeng (Y),
mehta.salil.lnk-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org,
netdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Linuxarm
In-Reply-To: <F4CC6FACFEB3C54C9141D49AD221F7F93B8283E1-WFPaWmAhWqtUuCJht5byYAK1hpo4iccwjNknBlVQO8k@public.gmane.org>
On 07/27/2017 11:01 AM, Salil Mehta wrote:
> Hi Florian,
>
>> -----Original Message-----
>> From: Florian Fainelli [mailto:f.fainelli-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org]
>> Sent: Sunday, July 23, 2017 6:05 PM
>> To: Salil Mehta; davem-fT/PcQaiUtIeIZ0/mPfg9Q@public.gmane.org
>> Cc: Zhuangyuzeng (Yisen); huangdaode; lipeng (Y);
>> mehta.salil.lnk-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org; netdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; linux-
>> kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; linux-rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; Linuxarm
>> Subject: Re: [PATCH V4 net-next 7/8] net: hns3: Add Ethtool support to
>> HNS3 driver
>>
>>
>>
>> On 07/22/2017 03:09 PM, Salil Mehta wrote:
>>> This patch adds the support of the Ethtool interface to
>>> the HNS3 Ethernet driver. Various commands to read the
>>> statistics, configure the offloading, loopback selftest etc.
>>> are supported.
>>>
>>> Signed-off-by: Daode Huang <huangdaode-C8/M+/jPZTeaMJb+Lgu22Q@public.gmane.org>
>>> Signed-off-by: lipeng <lipeng321-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
>>> Signed-off-by: Salil Mehta <salil.mehta-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
>>> Signed-off-by: Yisen Zhuang <yisen.zhuang-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
>>> ---
>>> Patch V4: addressed below comments
>>> 1. Andrew Lunn
>>> Removed the support of loop PHY back for now
>>> Patch V3: Address below comments
>>> 1. Stephen Hemminger
>>> https://lkml.org/lkml/2017/6/13/974
>>> 2. Andrew Lunn
>>> https://lkml.org/lkml/2017/6/13/1037
>>> Patch V2: No change
>>> Patch V1: Initial Submit
>>> ---
>>> .../ethernet/hisilicon/hns3/hns3pf/hns3_ethtool.c | 543
>> +++++++++++++++++++++
>>> 1 file changed, 543 insertions(+)
>>> create mode 100644
>> drivers/net/ethernet/hisilicon/hns3/hns3pf/hns3_ethtool.c
>>>
>>> diff --git
>> a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hns3_ethtool.c
>> b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hns3_ethtool.c
>>> new file mode 100644
>>> index 000000000000..82b0d4d829f8
>>> --- /dev/null
>>> +++ b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hns3_ethtool.c
>>> @@ -0,0 +1,543 @@
>>> +/*
>>> + * Copyright (c) 2016~2017 Hisilicon Limited.
>>> + *
>>> + * This program is free software; you can redistribute it and/or
>> modify
>>> + * it under the terms of the GNU General Public License as published
>> by
>>> + * the Free Software Foundation; either version 2 of the License, or
>>> + * (at your option) any later version.
>>> + */
>>> +
>>> +#include <linux/etherdevice.h>
>>> +#include "hns3_enet.h"
>>> +
>>> +struct hns3_stats {
>>> + char stats_string[ETH_GSTRING_LEN];
>>> + int stats_size;
>>> + int stats_offset;
>>> +};
>>> +
>>> +/* netdev related stats */
>>> +#define HNS3_NETDEV_STAT(_string, _member) \
>>> + { _string, \
>>> + FIELD_SIZEOF(struct rtnl_link_stats64, _member), \
>>> + offsetof(struct rtnl_link_stats64, _member), \
>>> + }
>>
>> Can you make this macro use named initializers?
> Can you please explain bit more or point out some
> example. This would be very handy.
.stat_string = _string,
.stats_size = FIELD_SIZEOF(struct rtnl_link_stat64, _member),
.stats_offset = offsetof(struct rtnl_link_stats64, _member),
https://gcc.gnu.org/onlinedocs/gcc/Designated-Inits.html
--
Florian
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCHv4 net] ipv6: no need to check rt->dst.error when get route info
From: David Ahern @ 2017-07-27 18:03 UTC (permalink / raw)
To: Hangbin Liu, netdev; +Cc: Cong Wang, Roopa Prabhu
In-Reply-To: <1501172736-13685-1-git-send-email-liuhangbin@gmail.com>
On 7/27/17 10:25 AM, Hangbin Liu wrote:
> After commit 18c3a61c4264 ("net: ipv6: RTM_GETROUTE: return matched fib
> result when requested"). When we get a prohibit ertry, we will return
> -EACCES directly instead of dump route info.
>
> Fix it by remove the rt->dst.error check.
>
> Before fix:
> \# ip -6 route add prohibit 2003::/64 dev eth1
> \# ip -6 route get fibmatch 2003::1
> RTNETLINK answers: Permission denied
> \# ip -6 route add unreachable 2004::/64 dev eth1
> \# ip -6 route get fibmatch 2004::1
> RTNETLINK answers: No route to host
>
> After fix:
> \# ip -6 route add prohibit 2003::/64 dev eth1
> \# ip -6 route get fibmatch 2003::1
> prohibit 2003::/64 dev lo metric 1024 error -13 pref medium
> \# ip -6 route add unreachable 2004::/64 dev eth1
> \# ip -6 route get fibmatch 2004::1
> unreachable 2004::/64 dev lo metric 1024 error -113 pref medium
>
> Fixes: 18c3a61c4264 ("net: ipv6: RTM_GETROUTE: return matched fib...")
> Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
> ---
> net/ipv6/route.c | 6 ------
> 1 file changed, 6 deletions(-)
>
Acked-by: David Ahern <dsahern@gmail.com>
^ permalink raw reply
* RE: [PATCH V4 net-next 7/8] net: hns3: Add Ethtool support to HNS3 driver
From: Salil Mehta @ 2017-07-27 18:01 UTC (permalink / raw)
To: Florian Fainelli, davem-fT/PcQaiUtIeIZ0/mPfg9Q@public.gmane.org
Cc: Zhuangyuzeng (Yisen), huangdaode, lipeng (Y),
mehta.salil.lnk-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org,
netdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Linuxarm
In-Reply-To: <23ddbe00-8bef-a09b-5783-3a5438086bd6-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Hi Florian,
> -----Original Message-----
> From: Florian Fainelli [mailto:f.fainelli-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org]
> Sent: Sunday, July 23, 2017 6:05 PM
> To: Salil Mehta; davem-fT/PcQaiUtIeIZ0/mPfg9Q@public.gmane.org
> Cc: Zhuangyuzeng (Yisen); huangdaode; lipeng (Y);
> mehta.salil.lnk-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org; netdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; linux-
> kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; linux-rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; Linuxarm
> Subject: Re: [PATCH V4 net-next 7/8] net: hns3: Add Ethtool support to
> HNS3 driver
>
>
>
> On 07/22/2017 03:09 PM, Salil Mehta wrote:
> > This patch adds the support of the Ethtool interface to
> > the HNS3 Ethernet driver. Various commands to read the
> > statistics, configure the offloading, loopback selftest etc.
> > are supported.
> >
> > Signed-off-by: Daode Huang <huangdaode-C8/M+/jPZTeaMJb+Lgu22Q@public.gmane.org>
> > Signed-off-by: lipeng <lipeng321-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
> > Signed-off-by: Salil Mehta <salil.mehta-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
> > Signed-off-by: Yisen Zhuang <yisen.zhuang-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
> > ---
> > Patch V4: addressed below comments
> > 1. Andrew Lunn
> > Removed the support of loop PHY back for now
> > Patch V3: Address below comments
> > 1. Stephen Hemminger
> > https://lkml.org/lkml/2017/6/13/974
> > 2. Andrew Lunn
> > https://lkml.org/lkml/2017/6/13/1037
> > Patch V2: No change
> > Patch V1: Initial Submit
> > ---
> > .../ethernet/hisilicon/hns3/hns3pf/hns3_ethtool.c | 543
> +++++++++++++++++++++
> > 1 file changed, 543 insertions(+)
> > create mode 100644
> drivers/net/ethernet/hisilicon/hns3/hns3pf/hns3_ethtool.c
> >
> > diff --git
> a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hns3_ethtool.c
> b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hns3_ethtool.c
> > new file mode 100644
> > index 000000000000..82b0d4d829f8
> > --- /dev/null
> > +++ b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hns3_ethtool.c
> > @@ -0,0 +1,543 @@
> > +/*
> > + * Copyright (c) 2016~2017 Hisilicon Limited.
> > + *
> > + * This program is free software; you can redistribute it and/or
> modify
> > + * it under the terms of the GNU General Public License as published
> by
> > + * the Free Software Foundation; either version 2 of the License, or
> > + * (at your option) any later version.
> > + */
> > +
> > +#include <linux/etherdevice.h>
> > +#include "hns3_enet.h"
> > +
> > +struct hns3_stats {
> > + char stats_string[ETH_GSTRING_LEN];
> > + int stats_size;
> > + int stats_offset;
> > +};
> > +
> > +/* netdev related stats */
> > +#define HNS3_NETDEV_STAT(_string, _member) \
> > + { _string, \
> > + FIELD_SIZEOF(struct rtnl_link_stats64, _member), \
> > + offsetof(struct rtnl_link_stats64, _member), \
> > + }
>
> Can you make this macro use named initializers?
Can you please explain bit more or point out some
example. This would be very handy.
Thanks
>
> > +
> > +static const struct hns3_stats hns3_netdev_stats[] = {
> > + /* misc. Rx/Tx statistics */
> > + HNS3_NETDEV_STAT("rx_packets", rx_packets),
> > + HNS3_NETDEV_STAT("tx_packets", tx_packets),
> > + HNS3_NETDEV_STAT("rx_bytes", rx_bytes),
> > + HNS3_NETDEV_STAT("tx_bytes", tx_bytes),
> > + HNS3_NETDEV_STAT("rx_errors", rx_errors),
> > + HNS3_NETDEV_STAT("tx_errors", tx_errors),
> > + HNS3_NETDEV_STAT("rx_dropped", rx_dropped),
> > + HNS3_NETDEV_STAT("tx_dropped", tx_dropped),
> > + HNS3_NETDEV_STAT("multicast", multicast),
> > + HNS3_NETDEV_STAT("collisions", collisions),
> > +
> > + /* detailed Rx errors */
> > + HNS3_NETDEV_STAT("rx_length_errors", rx_length_errors),
> > + HNS3_NETDEV_STAT("rx_over_errors", rx_over_errors),
> > + HNS3_NETDEV_STAT("rx_crc_errors", rx_crc_errors),
> > + HNS3_NETDEV_STAT("rx_frame_errors", rx_frame_errors),
> > + HNS3_NETDEV_STAT("rx_fifo_errors", rx_fifo_errors),
> > + HNS3_NETDEV_STAT("rx_missed_errors", rx_missed_errors),
> > +
> > + /* detailed Tx errors */
> > + HNS3_NETDEV_STAT("tx_aborted_errors", tx_aborted_errors),
> > + HNS3_NETDEV_STAT("tx_carrier_errors", tx_carrier_errors),
> > + HNS3_NETDEV_STAT("tx_fifo_errors", tx_fifo_errors),
> > + HNS3_NETDEV_STAT("tx_heartbeat_errors", tx_heartbeat_errors),
> > + HNS3_NETDEV_STAT("tx_window_errors", tx_window_errors),
> > +
> > + /* for cslip etc */
> > + HNS3_NETDEV_STAT("rx_compressed", rx_compressed),
> > + HNS3_NETDEV_STAT("tx_compressed", tx_compressed),
> > +};
> > +
> > +#define HNS3_NETDEV_STATS_COUNT ARRAY_SIZE(hns3_netdev_stats)
> > +
> > +/* tqp related stats */
> > +#define HNS3_TQP_STAT(_string, _member) \
> > + { _string, \
> > + FIELD_SIZEOF(struct ring_stats, _member), \
> > + offsetof(struct hns3_enet_ring, stats), \
> > + }
> > +
>
> Same here.
Ok.
>
> > +static const struct hns3_stats hns3_txq_stats[] = {
> > + /* Tx per-queue statistics */
> > + HNS3_TQP_STAT("tx_io_err_cnt", io_err_cnt),
> > + HNS3_TQP_STAT("tx_sw_err_cnt", sw_err_cnt),
> > + HNS3_TQP_STAT("tx_seg_pkt_cnt", seg_pkt_cnt),
> > + HNS3_TQP_STAT("tx_pkts", tx_pkts),
> > + HNS3_TQP_STAT("tx_bytes", tx_bytes),
> > + HNS3_TQP_STAT("tx_err_cnt", tx_err_cnt),
> > + HNS3_TQP_STAT("tx_restart_queue", restart_queue),
> > + HNS3_TQP_STAT("tx_busy", tx_busy),
> > +};
> > +
> > +#define HNS3_TXQ_STATS_COUNT ARRAY_SIZE(hns3_txq_stats)
> > +
> > +static const struct hns3_stats hns3_rxq_stats[] = {
> > + /* Rx per-queue statistics */
> > + HNS3_TQP_STAT("rx_io_err_cnt", io_err_cnt),
> > + HNS3_TQP_STAT("rx_sw_err_cnt", sw_err_cnt),
> > + HNS3_TQP_STAT("rx_seg_pkt_cnt", seg_pkt_cnt),
> > + HNS3_TQP_STAT("rx_pkts", rx_pkts),
> > + HNS3_TQP_STAT("rx_bytes", rx_bytes),
> > + HNS3_TQP_STAT("rx_err_cnt", rx_err_cnt),
> > + HNS3_TQP_STAT("rx_reuse_pg_cnt", reuse_pg_cnt),
> > + HNS3_TQP_STAT("rx_err_pkt_len", err_pkt_len),
> > + HNS3_TQP_STAT("rx_non_vld_descs", non_vld_descs),
> > + HNS3_TQP_STAT("rx_err_bd_num", err_bd_num),
> > + HNS3_TQP_STAT("rx_l2_err", l2_err),
> > + HNS3_TQP_STAT("rx_l3l4_csum_err", l3l4_csum_err),
> > +};
> > +
> > +#define HNS3_RXQ_STATS_COUNT ARRAY_SIZE(hns3_rxq_stats)
> > +
> > +#define HNS3_TQP_STATS_COUNT (HNS3_TXQ_STATS_COUNT +
> HNS3_RXQ_STATS_COUNT)
> > +
> > +struct hns3_link_mode_mapping {
> > + u32 hns3_link_mode;
> > + u32 ethtool_link_mode;
> > +};
> > +
> > +static const struct hns3_link_mode_mapping hns3_lm_map[] = {
> > + {HNS3_LM_FIBRE_BIT, ETHTOOL_LINK_MODE_FIBRE_BIT},
> > + {HNS3_LM_AUTONEG_BIT, ETHTOOL_LINK_MODE_Autoneg_BIT},
> > + {HNS3_LM_TP_BIT, ETHTOOL_LINK_MODE_TP_BIT},
> > + {HNS3_LM_PAUSE_BIT, ETHTOOL_LINK_MODE_Pause_BIT},
> > + {HNS3_LM_BACKPLANE_BIT, ETHTOOL_LINK_MODE_Backplane_BIT},
> > + {HNS3_LM_10BASET_HALF_BIT, ETHTOOL_LINK_MODE_10baseT_Half_BIT},
> > + {HNS3_LM_10BASET_FULL_BIT, ETHTOOL_LINK_MODE_10baseT_Full_BIT},
> > + {HNS3_LM_100BASET_HALF_BIT, ETHTOOL_LINK_MODE_100baseT_Half_BIT},
> > + {HNS3_LM_100BASET_FULL_BIT, ETHTOOL_LINK_MODE_100baseT_Full_BIT},
> > + {HNS3_LM_1000BASET_FULL_BIT,
> ETHTOOL_LINK_MODE_1000baseT_Full_BIT},
> > +};
> > +
> > +#define HNS3_DRV_TO_ETHTOOL_CAPS(caps, lk_ksettings, name) \
> > +{ \
> > + int i; \
> > + \
> > + for (i = 0; i < ARRAY_SIZE(hns3_lm_map); i++) { \
> > + if ((caps) & hns3_lm_map[i].hns3_link_mode) \
> > + __set_bit(hns3_lm_map[i].ethtool_link_mode,\
> > + (lk_ksettings)->link_modes.name); \
> > + } \
> > +}
>
> How about making this an inline function such that you would get proper
> type checking?
Sure will try doing that.
Thanks
>
> > +
> > +static int hns3_get_sset_count(struct net_device *netdev, int
> stringset)
> > +{
> > + struct hns3_nic_priv *priv = netdev_priv(netdev);
> > + struct hnae3_handle *h = priv->ae_handle;
> > + const struct hnae3_ae_ops *ops = h->ae_algo->ops;
> > +
> > + if (!ops->get_sset_count) {
> > + netdev_err(netdev, "could not get string set count\n");
> > + return -EOPNOTSUPP;
> > + }
>
> Is it really worth the error message? This might be called several
> times
> during the driver's lifecycle.
Fine, will remove.
>
> > +
> > + switch (stringset) {
> > + case ETH_SS_STATS:
> > + return (HNS3_NETDEV_STATS_COUNT +
> > + (HNS3_TQP_STATS_COUNT * h->kinfo.num_tqps) +
> > + ops->get_sset_count(h, stringset));
> > +
> > + case ETH_SS_TEST:
> > + return ops->get_sset_count(h, stringset);
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static u8 *hns3_get_strings_netdev(u8 *data)
> > +{
> > + int i;
>
> unsigned int i.
Ok.
>
> > +
> > + for (i = 0; i < HNS3_NETDEV_STATS_COUNT; i++) {
> > + memcpy(data, hns3_netdev_stats[i].stats_string,
> > + ETH_GSTRING_LEN);
> > + data += ETH_GSTRING_LEN;
> > + }
> > +
> > + return data;
> > +}
> > +
> > +static u8 *hns3_get_strings_tqps(struct hnae3_handle *handle, u8
> *data)
> > +{
> > + struct hnae3_knic_private_info *kinfo = &handle->kinfo;
> > + int i, j;
>
> Likewise, unsigned int i, j.
Ok.
>
> > +
> > + /* get strings for Tx */
> > + for (i = 0; i < kinfo->num_tqps; i++) {
> > + for (j = 0; j < HNS3_TXQ_STATS_COUNT; j++) {
> > + u8 gstr[ETH_GSTRING_LEN];
>
> char gstr[ETH_GSTRING_LEN] and you can move it out of the loop since it
> is just a temporary buffer for both loops here.
Fine.
>
> > +
> > + sprintf(gstr, "rcb_q%d_", i);
>
> snprintf() just to be on the safe side.
>
> > + strcat(gstr, hns3_txq_stats[j].stats_string);
> > +
> > + memcpy(data, gstr, ETH_GSTRING_LEN);
>
> What ensures that you are NULL terminating this string?
Yes, point! Will fix this.
Thanks
>
> > + data += ETH_GSTRING_LEN;
> > + }
> > + }
> > +
> > + /* get strings for Rx */
> > + for (i = 0; i < kinfo->num_tqps; i++) {
> > + for (j = 0; j < HNS3_RXQ_STATS_COUNT; j++) {
> > + u8 gstr[ETH_GSTRING_LEN];
> > +
> > + sprintf(gstr, "rcb_q%d_", i);
> > + strcat(gstr, hns3_rxq_stats[j].stats_string);
> > +
> > + memcpy(data, gstr, ETH_GSTRING_LEN);
> > + data += ETH_GSTRING_LEN;
> > + }
> > + }
> > +
> > + return data;
> > +}
> > +
> > +static void hns3_get_strings(struct net_device *netdev, u32
> stringset, u8 *data)
> > +{
> > + struct hns3_nic_priv *priv = netdev_priv(netdev);
> > + struct hnae3_handle *h = priv->ae_handle;
> > + const struct hnae3_ae_ops *ops = h->ae_algo->ops;
> > + char *buff = (char *)data;
> > +
> > + if (!ops->get_strings) {
> > + netdev_err(netdev, "could not get strings!\n");
> > + return;
> > + }
>
> Same here, does not sound like something you would want to print more
> than once?
Sure. Will remove.
>
> > +
> > + switch (stringset) {
> > + case ETH_SS_STATS:
> > + buff = hns3_get_strings_netdev(buff);
> > + buff = hns3_get_strings_tqps(h, buff);
> > + h->ae_algo->ops->get_strings(h, stringset, (u8 *)buff);
> > + break;
> > + case ETH_SS_TEST:
> > + ops->get_strings(h, stringset, data);
> > + break;
> > + }
> > +}
> > +
> > +static u64 *hns3_get_stats_netdev(struct net_device *netdev, u64
> *data)
> > +{
>
> You should implement the 64-bit version of this.
This is already taking care of 64bit stats?
>
> > + const struct rtnl_link_stats64 *net_stats;
> > + struct rtnl_link_stats64 temp;
> > + u8 *stat;
> > + int i;
>
> unsigned int i
Ok.
>
> > +
> > + net_stats = dev_get_stats(netdev, &temp);
> > +
> > + for (i = 0; i < HNS3_NETDEV_STATS_COUNT; i++) {
> > + stat = (u8 *)net_stats + hns3_netdev_stats[i].stats_offset;
> > + *data++ = *(u64 *)stat;
> > + }
> > +
> > + return data;
> > +}
> > +
> > +static u64 *hns3_get_stats_tqps(struct hnae3_handle *handle, u64
> *data)
> > +{
> > + struct hns3_nic_priv *nic_priv = (struct hns3_nic_priv *)handle-
> >priv;
> > + struct hnae3_knic_private_info *kinfo = &handle->kinfo;
> > + struct hns3_enet_ring *ring;
> > + u8 *stat;
> > + int i;
>
> Same here.
Ok.
>
> > +
> > + /* get stats for Tx */
> > + for (i = 0; i < kinfo->num_tqps; i++) {
> > + ring = nic_priv->ring_data[i].ring;
> > + for (i = 0; i < HNS3_TXQ_STATS_COUNT; i++) {
> > + stat = (u8 *)ring + hns3_txq_stats[i].stats_offset;
> > + *data++ = *(u64 *)stat;
> > + }
> > + }
> > +
> > + /* get stats for Rx */
> > + for (i = 0; i < kinfo->num_tqps; i++) {
> > + ring = nic_priv->ring_data[i + kinfo->num_tqps].ring;
> > + for (i = 0; i < HNS3_RXQ_STATS_COUNT; i++) {
> > + stat = (u8 *)ring + hns3_rxq_stats[i].stats_offset;
> > + *data++ = *(u64 *)stat;
> > + }
> > + }
> > +
> > + return data;
> > +}
> > +
> > +/* hns3_get_stats - get detail statistics.
> > + * @netdev: net device
> > + * @stats: statistics info.
> > + * @data: statistics data.
> > + */
> > +void hns3_get_stats(struct net_device *netdev, struct ethtool_stats
> *stats,
> > + u64 *data)
> > +{
> > + struct hns3_nic_priv *priv = netdev_priv(netdev);
> > + struct hnae3_handle *h = priv->ae_handle;
> > + u64 *p = data;
> > +
> > + if (!h->ae_algo->ops->get_stats || !h->ae_algo->ops-
> >update_stats) {
> > + netdev_err(netdev, "could not get any statistics\n");
> > + return;
> > + }
> > +
> > + h->ae_algo->ops->update_stats(h, &netdev->stats);
> > +
> > + /* get netdev related stats */
> > + p = hns3_get_stats_netdev(netdev, p);
> > +
> > + /* get per-queue stats */
> > + p = hns3_get_stats_tqps(h, p);
> > +
> > + /* get MAC & other misc hardware stats */
> > + h->ae_algo->ops->get_stats(h, p);
> > +}
> > +
> > +static void hns3_get_drvinfo(struct net_device *netdev,
> > + struct ethtool_drvinfo *drvinfo)
> > +{
> > + struct hns3_nic_priv *priv = netdev_priv(netdev);
> > + struct hnae3_handle *h = priv->ae_handle;
> > +
> > + strncpy(drvinfo->version, HNAE_DRIVER_VERSION,
> > + sizeof(drvinfo->version));
> > + drvinfo->version[sizeof(drvinfo->version) - 1] = '\0';
>
> strlcpy() would probably do that for you.
Not changing this for now.
>
> > +
> > + strncpy(drvinfo->driver, HNAE_DRIVER_NAME, sizeof(drvinfo-
> >driver));
> > + drvinfo->driver[sizeof(drvinfo->driver) - 1] = '\0';
>
> Same here
>
> > +
> > + strncpy(drvinfo->bus_info, priv->dev->bus->name,
> > + sizeof(drvinfo->bus_info));> + drvinfo-
> >bus_info[ETHTOOL_BUSINFO_LEN - 1] = '\0';
>
> And here.
>
> The rest looked fine.
Sure thanks
> --
> Florian
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [Intel-wired-lan] [PATCH] igb: support BCM54616 PHY
From: Florian Fainelli @ 2017-07-27 17:57 UTC (permalink / raw)
To: Andrew Lunn, Brown, Aaron F
Cc: John W. Linville, netdev@vger.kernel.org,
intel-wired-lan@lists.osuosl.org
In-Reply-To: <20170727153713.GG18666@lunn.ch>
On 07/27/2017 08:37 AM, Andrew Lunn wrote:
> On Thu, Jul 27, 2017 at 12:40:01AM +0000, Brown, Aaron F wrote:
>>> From: Intel-wired-lan [mailto:intel-wired-lan-bounces@osuosl.org] On Behalf
>>> Of John W. Linville
>>> Sent: Friday, July 21, 2017 11:12 AM
>>> To: netdev@vger.kernel.org
>>> Cc: intel-wired-lan@lists.osuosl.org; John W. Linville
>>> <linville@tuxdriver.com>
>>> Subject: [Intel-wired-lan] [PATCH] igb: support BCM54616 PHY
>>>
>>> The management port on an Edgecore AS7712-32 switch uses an igb MAC,
>>> but
>>> it uses a BCM54616 PHY. Without a patch like this, loading the igb
>>> module produces dmesg output like this:
>>>
>>> [ 3.439125] igb: Copyright (c) 2007-2014 Intel Corporation.
>>> [ 3.439866] igb: probe of 0000:00:14.0 failed with error -2
>>>
>>> Signed-off-by: John W. Linville <linville@tuxdriver.com>
>>> Cc: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
>>> ---
>>> drivers/net/ethernet/intel/igb/e1000_82575.c | 6 ++++++
>>> drivers/net/ethernet/intel/igb/e1000_defines.h | 1 +
>>> drivers/net/ethernet/intel/igb/e1000_hw.h | 1 +
>>> 3 files changed, 8 insertions(+)
>>
>> I do not have the specific hardware (Edgecore switch) but as far as regression tests go this works fine.
>> Tested-by: Aaron Brown <aaron.f.brown@intel.com>
>
> Sorry, missed the initial post, so replying to a reply.
>
> Linux has supported the BCM54616 PHY since April 2015. If the Intel
> drivers used the Linux PHY drivers, you would not of had this problem.
>
> It would be good if somebody spent the time to migrate these MAC
> drivers to use the common Linux PHY infrastructure.
I suspect there is a design pattern within the Intel drivers to share as
much low-level code as possible between OSes and only have some
Linux-ism where necessary (e.g: net_device, ethtool etc.).
PHY code is a pain in general, especially if you are serious about
testing interoperability (which is where you can spend tons of $$$ with
little reward but just say: yes it works), so it may make sense to share
it across different OSes.
I too, wish there was more sharing, but considering that this works for
the Intel driver, there is little incentive in doing this I suppose...
--
Florian
^ 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