* Re: [PATCH v6 7/7] arm64: dts: NS2: add AMAC ethernet support
From: Sergei Shtylyov @ 2016-11-04 13:31 UTC (permalink / raw)
To: Jon Mason, David Miller, Rob Herring, Mark Rutland,
Florian Fainelli
Cc: rafal-g1n6cQUeyibVItvQsEIGlw,
bcm-kernel-feedback-list-dY08KVG/lbpWk0Htik3J/w,
netdev-u79uwXL29TY76Z2rM5mHXA, devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-kernel-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <1478236262-3351-8-git-send-email-jon.mason-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>
Hello.
On 11/4/2016 8:11 AM, Jon Mason wrote:
> Add support for the AMAC ethernet to the Broadcom Northstar2 SoC device
> tree
>
> Signed-off-by: Jon Mason <jon.mason-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>
> ---
> arch/arm64/boot/dts/broadcom/ns2-svk.dts | 5 +++++
> arch/arm64/boot/dts/broadcom/ns2.dtsi | 12 ++++++++++++
> 2 files changed, 17 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/broadcom/ns2-svk.dts b/arch/arm64/boot/dts/broadcom/ns2-svk.dts
> index b09f3bc..c4d5442 100644
> --- a/arch/arm64/boot/dts/broadcom/ns2-svk.dts
> +++ b/arch/arm64/boot/dts/broadcom/ns2-svk.dts
> @@ -56,6 +56,10 @@
> };
> };
>
> +&enet {
> + status = "ok";
The spec dictates it should be "okay" (although "ok" is also recognized).
> +};
> +
> &pci_phy0 {
> status = "ok";
> };
> @@ -174,6 +178,7 @@
> &mdio_mux_iproc {
> mdio@10 {
> gphy0: eth-phy@10 {
> + enet-phy-lane-swap;
> reg = <0x10>;
> };
> };
[...]
MBR, Sergei
--
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 net-next 2/5] ethtool: Implements ETHTOOL_PHY_GTUNABLE/ETHTOOL_PHY_STUNABLE
From: Allan W. Nielsen @ 2016-11-04 13:31 UTC (permalink / raw)
To: Andrew Lunn; +Cc: netdev, f.fainelli, raju.lakkaraju, cphealy, robh
In-Reply-To: <20161104121332.GJ13959@lunn.ch>
On 04/11/16 13:13, Andrew Lunn wrote:
> On Fri, Nov 04, 2016 at 11:35:39AM +0100, Allan W. Nielsen wrote:
> > From: Raju Lakkaraju <Raju.Lakkaraju@microsemi.com>
> >
> > Adding get_tunable/set_tunable function pointer to the phy_driver
> > structure, and uses these function pointers to implement the
> > ETHTOOL_PHY_GTUNABLE/ETHTOOL_PHY_STUNABLE ioctls.
> For consistency, it would also be nice to add code in
> __ethtool_get_strings().
Okay, we add add that, and I assume you also want ethtool to use it when the
"-k|--show-features" options is invoked?
Are there other cases where this should be used?
/Allan
^ permalink raw reply
* Re: [PATCH net] sctp: assign assoc_id earlier in __sctp_connect
From: Neil Horman @ 2016-11-04 13:28 UTC (permalink / raw)
To: Marcelo Ricardo Leitner
Cc: netdev, linux-sctp, Vlad Yasevich, syzkaller, kcc, glider,
edumazet, dvyukov, andreyknvl
In-Reply-To: <9df38dcd0323ad92386eb6851a60dc128dd00b4e.1478199530.git.marcelo.leitner@gmail.com>
On Thu, Nov 03, 2016 at 05:03:41PM -0200, Marcelo Ricardo Leitner wrote:
> sctp_wait_for_connect() currently already holds the asoc to keep it
> alive during the sleep, in case another thread release it. But Andrey
> Konovalov and Dmitry Vyukov reported an use-after-free in such
> situation.
>
> Problem is that __sctp_connect() doesn't get a ref on the asoc and will
> do a read on the asoc after calling sctp_wait_for_connect(), but by then
> another thread may have closed it and the _put on sctp_wait_for_connect
> will actually release it, causing the use-after-free.
>
> Fix is, instead of doing the read after waiting for the connect, do it
> before so, and avoid this issue as the socket is still locked by then.
> There should be no issue on returning the asoc id in case of failure as
> the application shouldn't trust on that number in such situations
> anyway.
>
> This issue doesn't exist in sctp_sendmsg() path.
>
> Reported-by: Dmitry Vyukov <dvyukov@google.com>
> Reported-by: Andrey Konovalov <andreyknvl@google.com>
> Tested-by: Andrey Konovalov <andreyknvl@google.com>
> Signed-off-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
> ---
> net/sctp/socket.c | 7 +++++--
> 1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/net/sctp/socket.c b/net/sctp/socket.c
> index 6cdc61c21438aa9b6dbdad93e70759071a4d6789..be1d9bb98230c9d77f676949db773b2dacd801a4 100644
> --- a/net/sctp/socket.c
> +++ b/net/sctp/socket.c
> @@ -1214,9 +1214,12 @@ static int __sctp_connect(struct sock *sk,
>
> timeo = sock_sndtimeo(sk, f_flags & O_NONBLOCK);
>
> - err = sctp_wait_for_connect(asoc, &timeo);
> - if ((err == 0 || err == -EINPROGRESS) && assoc_id)
> + if (assoc_id)
> *assoc_id = asoc->assoc_id;
> + err = sctp_wait_for_connect(asoc, &timeo);
> + /* Note: the asoc may be freed after the return of
> + * sctp_wait_for_connect.
> + */
>
> /* Don't free association on exit. */
> asoc = NULL;
> --
> 2.7.4
>
>
Acked-by: Neil Horman <nhorman@tuxdriver.com>
^ permalink raw reply
* Re: [PATCH ethtool 2/2] Ethtool: Implements ETHTOOL_PHY_GTUNABLE/ETHTOOL_PHY_STUNABLE and PHY downshift
From: Allan W. Nielsen @ 2016-11-04 13:28 UTC (permalink / raw)
To: Andrew Lunn; +Cc: netdev, f.fainelli, raju.lakkaraju, cphealy, robh
In-Reply-To: <20161104115937.GH13959@lunn.ch>
On 04/11/16 12:59, Andrew Lunn wrote:
> > +++ b/ethtool.8.in
> > @@ -340,6 +340,14 @@ ethtool \- query or control network driver and hardware settings
> > .B2 tx-lpi on off
> > .BN tx-timer
> > .BN advertise
> > +.HP
> > +.B ethtool \-\-set\-phy\-tunable
> > +.I devname
> > +.B3 downshift on off N
> > +.HP
>
> I don't think there is any other option which is on|off|N. The general
> pattern would be
>
> --set-phy-tunable downshift on|off [count N]
>
> With count being optional.
>
> This also allows avoiding the ambiguity of what
>
> --set-phy-tunable downshift 0
>
> means. To me, that means downshift after 0 retries. But it actually
> seems to mean never downshift. You can then error out when somebody
> does
>
> --set-phy-tunable downshift on count 0
> or
> --set-phy-tunable downshift off count 42
Okay, I do not have any strong feelings about the syntax here.
We could not find any other signature which had the same need, so we have been
looking for things that was "close" to...
What is being proposed is similar to "-s [ mdix auto|on|off ]".
What you request is similar to "-e|--eeprom-dump [ raw on|off ] [ offset N ]".
So, unless there are other comments, then we will change this in the next
version.
> Please also add a few sentences about what downshift is, and what N
> means. Is it tries, or retries?
Sure.
We will wait a few days to see what other comments we get.
Thanks for reviewing.
/Allan
^ permalink raw reply
* Re: [PATCH] usbnet: prevent device rpm suspend in usbnet_probe function
From: Alan Stern @ 2016-11-04 13:26 UTC (permalink / raw)
To: Kai-Heng Feng; +Cc: oneukum, netdev, linux-usb, linux-kernel
In-Reply-To: <20161104095735.21071-1-kai.heng.feng@canonical.com>
On Fri, 4 Nov 2016, Kai-Heng Feng wrote:
> Sometimes cdc_mbim failed to probe if runtime pm is enabled:
> [ 9.305626] cdc_mbim: probe of 2-2:1.12 failed with error -22
>
> This can be solved by increase its pm usage counter.
This should not be needed. The USB core increments the PM usage
counter of a device before probing its interfaces.
Alan Stern
> Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
> ---
> drivers/net/usb/usbnet.c | 7 ++++++-
> 1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c
> index d5071e3..f77b4bf 100644
> --- a/drivers/net/usb/usbnet.c
> +++ b/drivers/net/usb/usbnet.c
> @@ -1674,12 +1674,15 @@ usbnet_probe (struct usb_interface *udev, const struct usb_device_id *prod)
> net->watchdog_timeo = TX_TIMEOUT_JIFFIES;
> net->ethtool_ops = &usbnet_ethtool_ops;
>
> + if (usb_autopm_get_interface(dev->intf) < 0)
> + goto out1;
> +
> // allow device-specific bind/init procedures
> // NOTE net->name still not usable ...
> if (info->bind) {
> status = info->bind (dev, udev);
> if (status < 0)
> - goto out1;
> + goto out2;
>
> // heuristic: "usb%d" for links we know are two-host,
> // else "eth%d" when there's reasonable doubt. userspace
> @@ -1772,6 +1775,8 @@ usbnet_probe (struct usb_interface *udev, const struct usb_device_id *prod)
> out3:
> if (info->unbind)
> info->unbind (dev, udev);
> +out2:
> + usb_autopm_put_interface(dev->intf);
> out1:
> /* subdrivers must undo all they did in bind() if they
> * fail it, but we may fail later and a deferred kevent
>
^ permalink raw reply
* Re: net/sctp: use-after-free in __sctp_connect
From: Marcelo Ricardo Leitner @ 2016-11-04 13:03 UTC (permalink / raw)
To: Neil Horman
Cc: Andrey Konovalov, Vlad Yasevich, David S. Miller, linux-sctp,
netdev, LKML, syzkaller, Kostya Serebryany, Alexander Potapenko,
Eric Dumazet, Dmitry Vyukov
In-Reply-To: <20161104125958.GA13691@hmsreliant.think-freely.org>
On Fri, Nov 04, 2016 at 08:59:58AM -0400, Neil Horman wrote:
> On Thu, Nov 03, 2016 at 04:35:33PM -0200, Marcelo Ricardo Leitner wrote:
> > On Thu, Nov 03, 2016 at 07:02:47PM +0100, Andrey Konovalov wrote:
> > > On Thu, Nov 3, 2016 at 6:52 PM, Marcelo Ricardo Leitner
> > > <marcelo.leitner@gmail.com> wrote:
> > > > On Thu, Nov 03, 2016 at 06:11:01PM +0100, Andrey Konovalov wrote:
> > > >> On Wed, Nov 2, 2016 at 11:42 PM, Andrey Konovalov <andreyknvl@google.com> wrote:
> > > >> > On Wed, Oct 19, 2016 at 6:57 PM, Marcelo Ricardo Leitner
> > > >> > <marcelo.leitner@gmail.com> wrote:
> > > >> >> On Wed, Oct 19, 2016 at 02:25:24PM +0200, Andrey Konovalov wrote:
> > > >> >>> Hi,
> > > >> >>>
> > > >> >>> I've got the following error report while running the syzkaller fuzzer:
> > > >> >>>
> > > >> >>> ==================================================================
> > > >> >>> BUG: KASAN: use-after-free in __sctp_connect+0xabe/0xbf0 at addr
> > > >> >>> ffff88006b1dc610
> > > >> >>
> > > >> >> Seems this is the same that Dmitry Vyukov had reported back in Jan 13th.
> > > >> >> So far I couldn't identify the reason.
> > > >> >> "Good" to know it's still there, thanks for reporting it.
> > > >>
> > > >> Hi Marcelo,
> > > >>
> > > >
> > > > Hi
> > > >
> > > >> So I've looked at the code.
> > > >> As far as I understand, the problem is a race condition between
> > > >> setsockopt(SCTP_SOCKOPT_CONNECTX) and shutdown on an sctp socket.
> > > >> setsockopt() calls sctp_wait_for_connect(), which exits the for loop
> > > >> on the sk->sk_shutdown & RCV_SHUTDOWN if clause, and then frees asoc
> > > >> with sctp_association_put() and returns err = 0.
> > > >> Then __sctp_connect() checks that err == 0 and reads asoc->assoc_id
> > > >> from the freed asoc.
> > > >
> > > > Suddenly this seems familiar. Your description makes sense, thanks for
> > > > looking deeper into this, Andrey.
> > > >
> > > > This fix should do it, can you please try it? I'll post it properly
> > > > if it works.
> > >
> > > Yes, it fixes the issue.
> > >
> > > Tested-by: Andrey Konovalov <andreyknvl@google.com>
> > >
> > > Thanks for the fix!
> >
> > Ahm this other fix looks better: do the read before calling
> > sctp_wait_for_connect() as that id won't change in this call and the
> > application shouldn't trust this number if an error is returned, so
> > there should be no issues by returning it in such situation.
> >
> > Can you please confirm this one also works? Thanks!
> >
> > ---8<---
> >
> > diff --git a/net/sctp/socket.c b/net/sctp/socket.c
> > index 6cdc61c21438..be1d9bb98230 100644
> > --- a/net/sctp/socket.c
> > +++ b/net/sctp/socket.c
> > @@ -1214,9 +1214,12 @@ static int __sctp_connect(struct sock *sk,
> >
> > timeo = sock_sndtimeo(sk, f_flags & O_NONBLOCK);
> >
> > - err = sctp_wait_for_connect(asoc, &timeo);
> > - if ((err == 0 || err == -EINPROGRESS) && assoc_id)
> > + if (assoc_id)
> > *assoc_id = asoc->assoc_id;
> > + err = sctp_wait_for_connect(asoc, &timeo);
> > + /* Note: the asoc may be freed after the return of
> > + * sctp_wait_for_connect.
> > + */
> >
> > /* Don't free association on exit. */
> > asoc = NULL;
> >
> Agreed, this version looks better. Please repost it with a proper changelog and
> I'll ack it.
> Neil
It already is. :-)
Please look for
Subject: [PATCH net] sctp: assign assoc_id earlier in __sctp_connect
Thanks,
Marcelo
^ permalink raw reply
* Re: Ethernet not working on a different SoC with same eth HW
From: Mason @ 2016-11-04 13:01 UTC (permalink / raw)
To: netdev
Cc: Andrew Lunn, Mans Rullgard, Florian Fainelli, Timur Tabi,
Sergei Shtylyov, Zefir Kurtisi, Martin Blumenstingl,
Uwe Kleine-Konig, Daniel Mack, Sebastian Frias
In-Reply-To: <58177128.8090403@free.fr>
On 31/10/2016 17:28, Mason wrote:
> On 31/10/2016 16:53, Andrew Lunn wrote:
>
>>> I'll add a log for the request_irq call.
>>
>> And take a look at /proc/interrupts
>
> You're right, there does seem to be something wrong with the interrupts.
Having fixed that, I'm still unable to ping a box on the same
ethernet segment... Still investigating.
I think I may have spotted a potential issue in the Atheros driver.
if (phydev->interface == PHY_INTERFACE_MODE_RGMII_RXID ||
phydev->interface == PHY_INTERFACE_MODE_RGMII_ID) {
ret = at803x_enable_rx_delay(phydev);
if (ret < 0)
return ret;
}
Looking at this code, one might believe that "rgmii rx clock delay"
is only enabled when the user requests it (through DT).
http://www.redeszone.net/app/uploads/2014/04/AR8035.pdf
cf. PDF page 48
*However* this bit is set to 1 at reset (both HW and SW resets).
Thus, "rgmii rx clock delay" is always enabled, whether the user
requests it or not.
Could someone knowledgeable comment on the expected behavior of
enabling rgmii rx (and tx) clock delay?
https://en.wikipedia.org/wiki/Media-independent_interface#Reduced_gigabit_media-independent_interface
Regards.
^ permalink raw reply
* Re: net/sctp: use-after-free in __sctp_connect
From: Neil Horman @ 2016-11-04 12:59 UTC (permalink / raw)
To: Marcelo Ricardo Leitner
Cc: Andrey Konovalov, Vlad Yasevich, David S. Miller, linux-sctp,
netdev, LKML, syzkaller, Kostya Serebryany, Alexander Potapenko,
Eric Dumazet, Dmitry Vyukov
In-Reply-To: <20161103183533.GH8514@localhost.localdomain>
On Thu, Nov 03, 2016 at 04:35:33PM -0200, Marcelo Ricardo Leitner wrote:
> On Thu, Nov 03, 2016 at 07:02:47PM +0100, Andrey Konovalov wrote:
> > On Thu, Nov 3, 2016 at 6:52 PM, Marcelo Ricardo Leitner
> > <marcelo.leitner@gmail.com> wrote:
> > > On Thu, Nov 03, 2016 at 06:11:01PM +0100, Andrey Konovalov wrote:
> > >> On Wed, Nov 2, 2016 at 11:42 PM, Andrey Konovalov <andreyknvl@google.com> wrote:
> > >> > On Wed, Oct 19, 2016 at 6:57 PM, Marcelo Ricardo Leitner
> > >> > <marcelo.leitner@gmail.com> wrote:
> > >> >> On Wed, Oct 19, 2016 at 02:25:24PM +0200, Andrey Konovalov wrote:
> > >> >>> Hi,
> > >> >>>
> > >> >>> I've got the following error report while running the syzkaller fuzzer:
> > >> >>>
> > >> >>> ==================================================================
> > >> >>> BUG: KASAN: use-after-free in __sctp_connect+0xabe/0xbf0 at addr
> > >> >>> ffff88006b1dc610
> > >> >>
> > >> >> Seems this is the same that Dmitry Vyukov had reported back in Jan 13th.
> > >> >> So far I couldn't identify the reason.
> > >> >> "Good" to know it's still there, thanks for reporting it.
> > >>
> > >> Hi Marcelo,
> > >>
> > >
> > > Hi
> > >
> > >> So I've looked at the code.
> > >> As far as I understand, the problem is a race condition between
> > >> setsockopt(SCTP_SOCKOPT_CONNECTX) and shutdown on an sctp socket.
> > >> setsockopt() calls sctp_wait_for_connect(), which exits the for loop
> > >> on the sk->sk_shutdown & RCV_SHUTDOWN if clause, and then frees asoc
> > >> with sctp_association_put() and returns err = 0.
> > >> Then __sctp_connect() checks that err == 0 and reads asoc->assoc_id
> > >> from the freed asoc.
> > >
> > > Suddenly this seems familiar. Your description makes sense, thanks for
> > > looking deeper into this, Andrey.
> > >
> > > This fix should do it, can you please try it? I'll post it properly
> > > if it works.
> >
> > Yes, it fixes the issue.
> >
> > Tested-by: Andrey Konovalov <andreyknvl@google.com>
> >
> > Thanks for the fix!
>
> Ahm this other fix looks better: do the read before calling
> sctp_wait_for_connect() as that id won't change in this call and the
> application shouldn't trust this number if an error is returned, so
> there should be no issues by returning it in such situation.
>
> Can you please confirm this one also works? Thanks!
>
> ---8<---
>
> diff --git a/net/sctp/socket.c b/net/sctp/socket.c
> index 6cdc61c21438..be1d9bb98230 100644
> --- a/net/sctp/socket.c
> +++ b/net/sctp/socket.c
> @@ -1214,9 +1214,12 @@ static int __sctp_connect(struct sock *sk,
>
> timeo = sock_sndtimeo(sk, f_flags & O_NONBLOCK);
>
> - err = sctp_wait_for_connect(asoc, &timeo);
> - if ((err == 0 || err == -EINPROGRESS) && assoc_id)
> + if (assoc_id)
> *assoc_id = asoc->assoc_id;
> + err = sctp_wait_for_connect(asoc, &timeo);
> + /* Note: the asoc may be freed after the return of
> + * sctp_wait_for_connect.
> + */
>
> /* Don't free association on exit. */
> asoc = NULL;
>
Agreed, this version looks better. Please repost it with a proper changelog and
I'll ack it.
Neil
^ permalink raw reply
* Re: [net-next PATCH 3/3] qdisc: catch misconfig of attaching qdisc to tx_queue_len zero device
From: Sergei Shtylyov @ 2016-11-04 12:53 UTC (permalink / raw)
To: Jesper Dangaard Brouer, netdev
Cc: Phil Sutter, Robert Olsson, Jamal Hadi Salim
In-Reply-To: <20161103135611.28737.39840.stgit@firesoul>
Hello.
On 11/3/2016 4:56 PM, Jesper Dangaard Brouer wrote:
> It is a clear misconfiguration to attach a qdisc to a device with
> tx_queue_len zero, because some qdisc's (namely, pfifo, bfifo, gred,
> htb, plug and sfb) inherit/copy this value as their queue length.
>
> Why should the kernel catch such a misconfiguration? Because prior to
> introducing the IFF_NO_QUEUE device flag, userspace found a loophole
> in the qdisc config system that allowed them to achieve the equivalent
> of IFF_NO_QUEUE, which is to remove the qdisc code path entirely from
> a device. The loophole on older kernels is setting tx_queue_len=0,
> *prior* to device qdisc init (the config time is significant, simply
> setting tx_queue_len=0 doesn't trigger the loophole).
>
> This loophole is currently used by Docker[1] to get better performance
> and scalability out of the veth device. The Docker developers were
> warned[1] that they needed to adjust the tx_queue_len if ever
> attaching a qdisc. The OpenShift project didn't remember this warning
> and attached a qdisc, this were caught and fixed in[2].
>
> [1] https://github.com/docker/libcontainer/pull/193
> [2] https://github.com/openshift/origin/pull/11126
>
> Instead of fixing every userspace program that used this loophole, and
> forgot to reset the tx_queue_len, prior to attaching a qdisc. Let's
> catch the misconfiguration on the kernel side.
>
> Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com>
> ---
> net/sched/sch_api.c | 11 +++++++++++
> 1 file changed, 11 insertions(+)
>
> diff --git a/net/sched/sch_api.c b/net/sched/sch_api.c
> index 206dc24add3a..f337f1bdd1d4 100644
> --- a/net/sched/sch_api.c
> +++ b/net/sched/sch_api.c
> @@ -960,6 +960,17 @@ static struct Qdisc *qdisc_create(struct net_device *dev,
>
> sch->handle = handle;
>
> + /* This exist to keep backward compatible with a userspace
"Exists" and "compatibility".
> + * loophole, what allowed userspace to get IFF_NO_QUEUE
> + * facility on older kernels by setting tx_queue_len=0 (prior
> + * to qdisc init), and then forgot to reinit tx_queue_len
> + * before again attaching a qdisc.
> + */
> + if ((dev->priv_flags & IFF_NO_QUEUE) && (dev->tx_queue_len == 0)) {
> + dev->tx_queue_len = DEFAULT_TX_QUEUE_LEN;
> + netdev_info(dev, "Caught tx_queue_len zero misconfig\n");
> + }
> +
> if (!ops->init || (err = ops->init(sch, tca[TCA_OPTIONS])) == 0) {
> if (qdisc_is_percpu_stats(sch)) {
> sch->cpu_bstats =
MBR, Sergei
^ permalink raw reply
* Re: [PATCH net-next v2 2/2] udp: do fwd memory scheduling on dequeue
From: Eric Dumazet @ 2016-11-04 12:38 UTC (permalink / raw)
To: Paolo Abeni
Cc: netdev, David S. Miller, Eric Dumazet, Hannes Frederic Sowa,
linux-nfs
In-Reply-To: <762f142043b48daf282316dd88ff099fbd9729e4.1478254512.git.pabeni@redhat.com>
On Fri, 2016-11-04 at 11:28 +0100, Paolo Abeni wrote:
> A new argument is added to __skb_recv_datagram to provide
> an explicit skb destructor, invoked under the receive queue
> lock.
> The UDP protocol uses such argument to perform memory
> reclaiming on dequeue, so that the UDP protocol does not
> set anymore skb->desctructor.
> Instead explicit memory reclaiming is performed at close() time and
> when skbs are removed from the receive queue.
> The in kernel UDP protocol users now need to call a
> skb_recv_udp() variant instead of skb_recv_datagram() to
> properly perform memory accounting on dequeue.
>
> Overall, this allows acquiring only once the receive queue
> lock on dequeue.
>
> Tested using pktgen with random src port, 64 bytes packet,
> wire-speed on a 10G link as sender and udp_sink as the receiver,
> using an l4 tuple rxhash to stress the contention, and one or more
> udp_sink instances with reuseport.
>
> nr sinks vanilla patched
> 1 440 560
> 3 2150 2300
> 6 3650 3800
> 9 4450 4600
> 12 6250 6450
>
> v1 -> v2:
> - do rmem and allocated memory scheduling under the receive lock
> - do bulk scheduling in first_packet_length() and in udp_destruct_sock()
> - avoid the typdef for the dequeue callback
>
> Suggested-by: Eric Dumazet <edumazet@google.com>
> Acked-by: Hannes Frederic Sowa <hannes@stressinduktion.org>
> Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> ---
> @Eric, please add your signed off by when you feel comfortable with the patch
> as you authored some of the code
SGTM, thanks Paolo and Hannes !
Acked-by: Eric Dumazet <edumazet@google.com>
^ permalink raw reply
* Re: [PATCH net-next v2 1/2] net/sock: add an explicit sk argument for ip_cmsg_recv_offset()
From: Eric Dumazet @ 2016-11-04 12:36 UTC (permalink / raw)
To: Paolo Abeni
Cc: netdev-u79uwXL29TY76Z2rM5mHXA, David S. Miller, Eric Dumazet,
Hannes Frederic Sowa, linux-nfs-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <1b4e5292a0c15494515423245e8eeb7adf2ebcea.1478254512.git.pabeni-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
On Fri, 2016-11-04 at 11:28 +0100, Paolo Abeni wrote:
> So that we can use it even after orphaining the skbuff.
>
> Suggested-by: Eric Dumazet <eric.dumazet-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> Signed-off-by: Paolo Abeni <pabeni-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> ---
> @Eric, please add your signed off by when you feel comfortable with the patch
> as you basically authored it
> -void ip_cmsg_recv_offset(struct msghdr *msg, struct sk_buff *skb,
> - int tlen, int offset)
> +void ip_cmsg_recv_offset(struct msghdr *msg, struct sock *sk,
> + struct sk_buff *skb, int tlen, int offset)
> {
> - struct inet_sock *inet = inet_sk(skb->sk);
> + struct inet_sock *inet = inet_sk(sk);
> unsigned int flags = inet->cmsg_flags;
My final version had :
void ip_cmsg_recv_offset(struct msghdr *msg, const struct sock *sk,
struct sk_buff *skb, int tlen, int offset)
{
unsigned int flags = inet_sk(sk)->cmsg_flags;
(Ie not using "struct inet_sock *inet = ...", and a const pointer for
struct sock)
Other than that minor details :
Signed-off-by: Eric Dumazet <edumazet-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
Thanks !
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" 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] Documentation: networking: dsa: Update tagging protocols
From: Andrew Lunn @ 2016-11-04 12:31 UTC (permalink / raw)
To: Fabian Mewes
Cc: Jonathan Corbet, David S. Miller, Vivien Didelot,
Florian Fainelli, Masanari Iida, linux-doc, linux-kernel, netdev
In-Reply-To: <1478261774-14265-1-git-send-email-architekt@coding4coffee.org>
On Fri, Nov 04, 2016 at 01:16:14PM +0100, Fabian Mewes wrote:
> Add Qualcomm QCA tagging introduced in cafdc45c9 to the
> list of supported protocols.
>
> Signed-off-by: Fabian Mewes <architekt@coding4coffee.org>
Thanks
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Andrew
^ permalink raw reply
* Re: [PATCH net-next 1/5] ethtool: (uapi) Add ETHTOOL_PHY_GTUNABLE and ETHTOOL_PHY_STUNABLE
From: Andrew Lunn @ 2016-11-04 12:29 UTC (permalink / raw)
To: Allan W. Nielsen; +Cc: netdev, f.fainelli, raju.lakkaraju, cphealy, robh
In-Reply-To: <20161104121841.GA5676@microsemi.com>
> It was "just" to mimic how "tunable_type_id/ETHTOOL_TUNABLE_UNSPEC" (and other)
> is done.
Yes, i know. I'm wondering about cult cargo programming...
> The thinking was that we did not want an "ID" of zero do to anything - because
> that could mean the programmer had forgot to set the field...
Seems reasonable. Leave it as is.
Andrew
^ permalink raw reply
* Re: [PATCH net-next 5/5] net: phy: Add downshift get/set support in Microsemi PHYs driver
From: Andrew Lunn @ 2016-11-04 12:27 UTC (permalink / raw)
To: Allan W. Nielsen; +Cc: netdev, f.fainelli, raju.lakkaraju, cphealy, robh
In-Reply-To: <1478255742-25693-6-git-send-email-allan.nielsen@microsemi.com>
Hi Allan
> +static int vsc85xx_downshift_get(struct phy_device *phydev, u8 *count)
> +{
> + int rc;
> + u16 reg_val;
> +
> + mutex_lock(&phydev->lock);
> + rc = vsc85xx_phy_page_set(phydev, MSCC_PHY_PAGE_EXTENDED);
> + if (rc != 0)
> + goto out_unlock;
> +
> + reg_val = phy_read(phydev, MSCC_PHY_ACTIPHY_CNTL);
> + reg_val &= DOWNSHIFT_CNTL_MASK;
> + if (!(reg_val & DOWNSHIFT_EN))
> + *count = 0;
DOWNSHIFT_DEV_DISABLE
> + else
> + *count = ((reg_val & ~DOWNSHIFT_EN) >> DOWNSHIFT_CNTL_POS) + 2;
>
> + rc = vsc85xx_phy_page_set(phydev, MSCC_PHY_PAGE_STANDARD);
> +
> +out_unlock:
> + mutex_unlock(&phydev->lock);
> +
> + return rc;
> +}
> +
> +static int vsc85xx_downshift_set(struct phy_device *phydev, u8 count)
> +{
> + int rc;
> + u16 reg_val;
> +
> + if (count == DOWNSHIFT_DEV_DEFAULT_COUNT) {
> + /* Default downshift count 3 (i.e. Bit3:2 = 0b01) */
> + count = ((1 << DOWNSHIFT_CNTL_POS) | DOWNSHIFT_EN);
> + } else if (count > DOWNSHIFT_COUNT_MAX || count == 1) {
> + phydev_err(phydev, "Invalid downshift count\n");
Maybe include a hint what is valid?
> + return -EINVAL;
ERANGE? I don't know error codes too well, so this needs verifying.
> + } else if (count) {
> + /* Downshift count is either 2,3,4 or 5 */
> + count = (((count - 2) << DOWNSHIFT_CNTL_POS) | DOWNSHIFT_EN);
Ah, now i see why + 2. But this means it never does what you ask it to
do. It would be better to round up < 2 to 2, and leave all the others
as is.
> +static int vsc85xx_get_tunable(struct phy_device *phydev,
> + struct ethtool_tunable *tuna, void *data)
> +{
> + switch (tuna->id) {
> + case ETHTOOL_PHY_DOWNSHIFT:
> + return vsc85xx_downshift_get(phydev, (u8 *)data);
> + default:
> + phydev_err(phydev, "Unsupported PHY tunable id\n");
> + return -EINVAL;
This is not really a error you should complain about. There could be
many tunables, and some your hardware cannot support. So return
-ENOSUPP and no phydev_err().
> + }
> +}
> +
> +static int vsc85xx_set_tunable(struct phy_device *phydev,
> + struct ethtool_tunable *tuna,
> + const void *data)
> +{
> + switch (tuna->id) {
> + case ETHTOOL_PHY_DOWNSHIFT:
> + return vsc85xx_downshift_set(phydev, *(u8 *)data);
> + default:
> + phydev_err(phydev, "Unsupported PHY tunable id\n");
> + return -EINVAL;
Same as above.
Andrew
^ permalink raw reply
* Re: [PATCH net-next 1/5] ethtool: (uapi) Add ETHTOOL_PHY_GTUNABLE and ETHTOOL_PHY_STUNABLE
From: Allan W. Nielsen @ 2016-11-04 12:18 UTC (permalink / raw)
To: Andrew Lunn; +Cc: netdev, f.fainelli, raju.lakkaraju, cphealy, robh
In-Reply-To: <20161104120343.GI13959@lunn.ch>
Hi,
On 04/11/16 13:03, Andrew Lunn wrote:
> On Fri, Nov 04, 2016 at 11:35:38AM +0100, Allan W. Nielsen wrote:
> > From: Raju Lakkaraju <Raju.Lakkaraju@microsemi.com>
> >
> > Defines a generic API to get/set phy tunables. The API is using the
> > existing ethtool_tunable/tunable_type_id types which is already being used
> > for mac level tunables.
> >
> > Signed-off-by: Raju Lakkaraju <Raju.Lakkaraju@microsemi.com>
> > Signed-off-by: Allan W. Nielsen <allan.nielsen@microsemi.com>
> > ---
> > include/uapi/linux/ethtool.h | 7 ++++++-
> > 1 file changed, 6 insertions(+), 1 deletion(-)
> >
> > diff --git a/include/uapi/linux/ethtool.h b/include/uapi/linux/ethtool.h
> > index 8e54723..fd0bd36 100644
> > --- a/include/uapi/linux/ethtool.h
> > +++ b/include/uapi/linux/ethtool.h
> > @@ -248,6 +248,10 @@ struct ethtool_tunable {
> > void *data[0];
> > };
> >
> > +enum phy_tunable_id {
> > + ETHTOOL_PHY_ID_UNSPEC,
> > +};
>
> Do you have any idea what this is for? A grep for
> ETHTOOL_TUNABLE_UNSPEC does not turn up anything.
It is not used...
It was "just" to mimic how "tunable_type_id/ETHTOOL_TUNABLE_UNSPEC" (and other)
is done.
The thinking was that we did not want an "ID" of zero do to anything - because
that could mean the programmer had forgot to set the field...
I have on strong feelings about this, please let us know if you would like this
done in an other way.
/Allan
^ permalink raw reply
* [PATCH] Documentation: networking: dsa: Update tagging protocols
From: Fabian Mewes @ 2016-11-04 12:16 UTC (permalink / raw)
To: Jonathan Corbet
Cc: David S. Miller, Vivien Didelot, Florian Fainelli, Andrew Lunn,
Masanari Iida, linux-doc, linux-kernel, netdev, Fabian Mewes
Add Qualcomm QCA tagging introduced in cafdc45c9 to the
list of supported protocols.
Signed-off-by: Fabian Mewes <architekt@coding4coffee.org>
---
Documentation/networking/dsa/dsa.txt | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/Documentation/networking/dsa/dsa.txt b/Documentation/networking/dsa/dsa.txt
index 6d6c07c..63912ef3 100644
--- a/Documentation/networking/dsa/dsa.txt
+++ b/Documentation/networking/dsa/dsa.txt
@@ -67,13 +67,14 @@ Note that DSA does not currently create network interfaces for the "cpu" and
Switch tagging protocols
------------------------
-DSA currently supports 4 different tagging protocols, and a tag-less mode as
+DSA currently supports 5 different tagging protocols, and a tag-less mode as
well. The different protocols are implemented in:
net/dsa/tag_trailer.c: Marvell's 4 trailer tag mode (legacy)
net/dsa/tag_dsa.c: Marvell's original DSA tag
net/dsa/tag_edsa.c: Marvell's enhanced DSA tag
net/dsa/tag_brcm.c: Broadcom's 4 bytes tag
+net/dsa/tag_qca.c: Qualcomm's 2 bytes tag
The exact format of the tag protocol is vendor specific, but in general, they
all contain something which:
--
1.7.10.4
^ permalink raw reply related
* Re: [PATCH net] r8152: Fix broken RX checksums.
From: Mark Lord @ 2016-11-04 12:13 UTC (permalink / raw)
To: Hayes Wang, David Miller
Cc: nic_swsd, netdev@vger.kernel.org, linux-kernel@vger.kernel.org
In-Reply-To: <008b7d7d-74af-0341-0c16-40aafde5a223@pobox.com>
On 16-11-02 02:29 PM, Mark Lord wrote:
>
> I have poked at it some more, and thus far it appears that it is
> only necessary to disable TCP rx checksums. The system doesn't crash
> when only IP/UDP checksums are enabled, but does when TCP checksums are on.
>
> This happens regardless of whether RX_AGG is disabled or enabled,
> and increasing/decreasing the number of RX URBs (RTL8152_MAX_RX)
> doesn't seem to affect it.
>
..
I noticed that BIT(20) was not defined as anything for "opts3",
and so I added a line to rx_csum to check whether or not that bit
was ever set.
It triggered after a few thousand reset/reboot cycles with opts3
having the rather dubious looking value of an ASCII string: 0x5c7d7852.
So to me, it appears that the rx_desc's are getting mixed-up with data somewhere,
and when using hardware TCP checksums this doesn't get caught. So perhaps the
checksums themselves are fine, but there's another bug (driver or hardware?)
that sneaks through when not doing software checksums.
^ permalink raw reply
* Re: [PATCH net-next 2/5] ethtool: Implements ETHTOOL_PHY_GTUNABLE/ETHTOOL_PHY_STUNABLE
From: Andrew Lunn @ 2016-11-04 12:13 UTC (permalink / raw)
To: Allan W. Nielsen; +Cc: netdev, f.fainelli, raju.lakkaraju, cphealy, robh
In-Reply-To: <1478255742-25693-3-git-send-email-allan.nielsen@microsemi.com>
On Fri, Nov 04, 2016 at 11:35:39AM +0100, Allan W. Nielsen wrote:
> From: Raju Lakkaraju <Raju.Lakkaraju@microsemi.com>
>
> Adding get_tunable/set_tunable function pointer to the phy_driver
> structure, and uses these function pointers to implement the
> ETHTOOL_PHY_GTUNABLE/ETHTOOL_PHY_STUNABLE ioctls.
Hi Allan
For consistency, it would also be nice to add code in
__ethtool_get_strings().
Andrew
^ permalink raw reply
* Re: [net-next PATCH 3/3] qdisc: catch misconfig of attaching qdisc to tx_queue_len zero device
From: Jesper Dangaard Brouer @ 2016-11-04 12:09 UTC (permalink / raw)
To: Phil Sutter; +Cc: netdev, Robert Olsson, Jamal Hadi Salim, brouer
In-Reply-To: <20161104105913.GN5640@orbyte.nwl.cc>
On Fri, 4 Nov 2016 11:59:13 +0100
Phil Sutter <phil@nwl.cc> wrote:
> On Fri, Nov 04, 2016 at 11:10:42AM +0100, Jesper Dangaard Brouer wrote:
> >
> > On Fri, 4 Nov 2016 10:35:26 +0100 Phil Sutter <phil@nwl.cc> wrote:
> >
> > > Hi,
> > >
> > > On Thu, Nov 03, 2016 at 02:56:11PM +0100, Jesper Dangaard Brouer wrote:
> > > [...]
> > > > diff --git a/net/sched/sch_api.c b/net/sched/sch_api.c
> > > > index 206dc24add3a..f337f1bdd1d4 100644
> > > > --- a/net/sched/sch_api.c
> > > > +++ b/net/sched/sch_api.c
> > > > @@ -960,6 +960,17 @@ static struct Qdisc *qdisc_create(struct net_device *dev,
> > > >
> > > > sch->handle = handle;
> > > >
> > > > + /* This exist to keep backward compatible with a userspace
> > > > + * loophole, what allowed userspace to get IFF_NO_QUEUE
> > > > + * facility on older kernels by setting tx_queue_len=0 (prior
> > > > + * to qdisc init), and then forgot to reinit tx_queue_len
> > > > + * before again attaching a qdisc.
> > > > + */
> > > > + if ((dev->priv_flags & IFF_NO_QUEUE) && (dev->tx_queue_len == 0)) {
> > > > + dev->tx_queue_len = DEFAULT_TX_QUEUE_LEN;
> > > > + netdev_info(dev, "Caught tx_queue_len zero misconfig\n");
> > > > + }
> > >
> > > I wonder why this is limited to IFF_NO_QUEUE devices. Do you think there
> > > is a valid use case for physical ones?
> >
> > Hmmm, I cannot come up with a useful use-case for physical devices, but
> > I cannot see why we should save users that had used the loophole on
> > physical devices, as that is clearly a faulty config to begin with.
> > See net_crit_ratelimited warning here:
> > [1] https://github.com/torvalds/linux/blob/v4.9-rc3/net/core/dev.c#L3403
>
> I really feel like nit-picking again,
Perhaps a follow up patch is better? This patch does solve a real
issue.
> but what differs in between
> loophole users of virtual devices (whose broken scripts stopped working)
> and loophole users of physical devices (whose broken scripts stopped
> working as well)?
There is a difference. We basically closed the loophole config, but
fixed that qdisc can be attached to virtual (IFF_NO_QUEUE) devices,
without needing to adjusting tx_queue_len.
Thus, running a loophole-script have no-effect, but for IFF_NO_QUEUE
devices (veth specifically) it looks like it had the desired effect,
thus Docker will/can keep doing that, to work with older kernels, and
on newer kernels it just doesn't have any effect.
The remaining problem is that a "loophole-script" leaves the interface
in a broken state with tx_queue_len==0. Which this patch address.
So, why only catch misconfig for IFF_NO_QUEUE devices? Because a
loophole-script on veth brought it into a valid config, thus valid
use-case, while one a physical into a invalid config (hence the
critical warn[1]).
You could (in a followup patch, please) argue that it is a lot simpler,
just to always catch the misconfig of having tx_queue_len==0 when
attaching a qdisc.
--
Best regards,
Jesper Dangaard Brouer
MSc.CS, Principal Kernel Engineer at Red Hat
Author of http://www.iptv-analyzer.org
LinkedIn: http://www.linkedin.com/in/brouer
^ permalink raw reply
* Re: [PATCH net-next 1/5] ethtool: (uapi) Add ETHTOOL_PHY_GTUNABLE and ETHTOOL_PHY_STUNABLE
From: Andrew Lunn @ 2016-11-04 12:03 UTC (permalink / raw)
To: Allan W. Nielsen; +Cc: netdev, f.fainelli, raju.lakkaraju, cphealy, robh
In-Reply-To: <1478255742-25693-2-git-send-email-allan.nielsen@microsemi.com>
On Fri, Nov 04, 2016 at 11:35:38AM +0100, Allan W. Nielsen wrote:
> From: Raju Lakkaraju <Raju.Lakkaraju@microsemi.com>
>
> Defines a generic API to get/set phy tunables. The API is using the
> existing ethtool_tunable/tunable_type_id types which is already being used
> for mac level tunables.
>
> Signed-off-by: Raju Lakkaraju <Raju.Lakkaraju@microsemi.com>
> Signed-off-by: Allan W. Nielsen <allan.nielsen@microsemi.com>
> ---
> include/uapi/linux/ethtool.h | 7 ++++++-
> 1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/include/uapi/linux/ethtool.h b/include/uapi/linux/ethtool.h
> index 8e54723..fd0bd36 100644
> --- a/include/uapi/linux/ethtool.h
> +++ b/include/uapi/linux/ethtool.h
> @@ -248,6 +248,10 @@ struct ethtool_tunable {
> void *data[0];
> };
>
> +enum phy_tunable_id {
> + ETHTOOL_PHY_ID_UNSPEC,
> +};
Hi Allen
Do you have any idea what this is for? A grep for
ETHTOOL_TUNABLE_UNSPEC does not turn up anything.
Andrew
^ permalink raw reply
* Re: [PATCH ethtool 2/2] Ethtool: Implements ETHTOOL_PHY_GTUNABLE/ETHTOOL_PHY_STUNABLE and PHY downshift
From: Andrew Lunn @ 2016-11-04 11:59 UTC (permalink / raw)
To: Allan W. Nielsen; +Cc: netdev, f.fainelli, raju.lakkaraju, cphealy, robh
In-Reply-To: <1478255805-25823-3-git-send-email-allan.nielsen@microsemi.com>
Hi Allen
> +++ b/ethtool.8.in
> @@ -340,6 +340,14 @@ ethtool \- query or control network driver and hardware settings
> .B2 tx-lpi on off
> .BN tx-timer
> .BN advertise
> +.HP
> +.B ethtool \-\-set\-phy\-tunable
> +.I devname
> +.B3 downshift on off N
> +.HP
I don't think there is any other option which is on|off|N. The general
pattern would be
--set-phy-tunable downshift on|off [count N]
With count being optional.
This also allows avoiding the ambiguity of what
--set-phy-tunable downshift 0
means. To me, that means downshift after 0 retries. But it actually
seems to mean never downshift. You can then error out when somebody
does
--set-phy-tunable downshift on count 0
or
--set-phy-tunable downshift off count 42
Please also add a few sentences about what downshift is, and what N
means. Is it tries, or retries?
Thanks
Andrew
^ permalink raw reply
* (unknown),
From: Amir A. Khanmammadov @ 2016-11-04 10:43 UTC (permalink / raw)
Thanks for your last email response to me.
The information required should include the following-:
Your full names
Your address
Telephone number
Your private email
Occupation
Age
This is to enable my further discussion with you in confidence.
Best regards and wishes to you.
Amir A. Khanmammadov
REPLY TO
khanmammadov@vera.com.uy
amir2016@vera.com.uy
^ permalink raw reply
* Re: [PATCH net-next V4 2/9] liquidio CN23XX: sysfs VF config support
From: kbuild test robot @ 2016-11-04 19:53 UTC (permalink / raw)
To: Raghu Vatsavayi
Cc: kbuild-all, davem, netdev, Raghu Vatsavayi, Raghu Vatsavayi,
Derek Chickles, Satanand Burla, Felix Manlunas
In-Reply-To: <1478208918-8330-3-git-send-email-rvatsavayi@caviumnetworks.com>
[-- Attachment #1: Type: text/plain, Size: 1174 bytes --]
Hi Raghu,
[auto build test ERROR on net-next/master]
url: https://github.com/0day-ci/linux/commits/Raghu-Vatsavayi/liquidio-CN23XX-VF-support/20161104-054117
config: x86_64-randconfig-s2-11041915 (attached as .config)
compiler: gcc-4.4 (Debian 4.4.7-8) 4.4.7
reproduce:
# save the attached .config to linux build tree
make ARCH=x86_64
All errors (new ones prefixed by >>):
drivers/net/ethernet/cavium/liquidio/lio_main.c: In function 'octeon_enable_sriov':
>> drivers/net/ethernet/cavium/liquidio/lio_main.c:4025: error: 'struct pci_dev' has no member named 'physfn'
vim +4025 drivers/net/ethernet/cavium/liquidio/lio_main.c
4019 */
4020 u = 0;
4021 vfdev = pci_get_device(PCI_VENDOR_ID_CAVIUM,
4022 OCTEON_CN23XX_VF_VID, NULL);
4023 while (vfdev) {
4024 if (vfdev->is_virtfn &&
> 4025 (vfdev->physfn == oct->pci_dev)) {
4026 oct->sriov_info.dpiring_to_vfpcidev_lut[u] =
4027 vfdev;
4028 u += oct->sriov_info.rings_per_vf;
---
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: 29371 bytes --]
^ permalink raw reply
* [PATCH net-next v3] cadence: Add LSO support.
From: Rafal Ozieblo @ 2016-11-04 11:40 UTC (permalink / raw)
To: nicolas.ferre, netdev, linux-kernel; +Cc: Rafal Ozieblo
In-Reply-To: <1477397130-24250-1-git-send-email-rafalo@cadence.com>
New Cadence GEM hardware support Large Segment Offload (LSO):
TCP segmentation offload (TSO) as well as UDP fragmentation
offload (UFO). Support for those features was added to the driver.
Signed-off-by: Rafal Ozieblo <rafalo@cadence.com>
---
Changed in v2:
macb_lso_check_compatibility() changed to macb_features_check()
(with little modifications) and bind to .ndo_features_check.
(after Eric Dumazet suggestion)
---
Changed in v3:
Respin to net-next.
---
drivers/net/ethernet/cadence/macb.c | 142 +++++++++++++++++++++++++++++++++---
drivers/net/ethernet/cadence/macb.h | 14 ++++
2 files changed, 144 insertions(+), 12 deletions(-)
diff --git a/drivers/net/ethernet/cadence/macb.c b/drivers/net/ethernet/cadence/macb.c
index e1847ce..1eeaa4b 100644
--- a/drivers/net/ethernet/cadence/macb.c
+++ b/drivers/net/ethernet/cadence/macb.c
@@ -32,7 +32,9 @@
#include <linux/of_gpio.h>
#include <linux/of_mdio.h>
#include <linux/of_net.h>
-
+#include <linux/ip.h>
+#include <linux/udp.h>
+#include <linux/tcp.h>
#include "macb.h"
#define MACB_RX_BUFFER_SIZE 128
@@ -60,10 +62,13 @@
| MACB_BIT(TXERR))
#define MACB_TX_INT_FLAGS (MACB_TX_ERR_FLAGS | MACB_BIT(TCOMP))
-#define MACB_MAX_TX_LEN ((unsigned int)((1 << MACB_TX_FRMLEN_SIZE) - 1))
-#define GEM_MAX_TX_LEN ((unsigned int)((1 << GEM_TX_FRMLEN_SIZE) - 1))
+/* Max length of transmit frame must be a multiple of 8 bytes */
+#define MACB_TX_LEN_ALIGN 8
+#define MACB_MAX_TX_LEN ((unsigned int)((1 << MACB_TX_FRMLEN_SIZE) - 1) & ~((unsigned int)(MACB_TX_LEN_ALIGN - 1)))
+#define GEM_MAX_TX_LEN ((unsigned int)((1 << GEM_TX_FRMLEN_SIZE) - 1) & ~((unsigned int)(MACB_TX_LEN_ALIGN - 1)))
#define GEM_MTU_MIN_SIZE ETH_MIN_MTU
+#define MACB_NETIF_LSO (NETIF_F_TSO | NETIF_F_UFO)
#define MACB_WOL_HAS_MAGIC_PACKET (0x1 << 0)
#define MACB_WOL_ENABLED (0x1 << 1)
@@ -1223,7 +1228,8 @@ static void macb_poll_controller(struct net_device *dev)
static unsigned int macb_tx_map(struct macb *bp,
struct macb_queue *queue,
- struct sk_buff *skb)
+ struct sk_buff *skb,
+ unsigned int hdrlen)
{
dma_addr_t mapping;
unsigned int len, entry, i, tx_head = queue->tx_head;
@@ -1231,14 +1237,27 @@ static unsigned int macb_tx_map(struct macb *bp,
struct macb_dma_desc *desc;
unsigned int offset, size, count = 0;
unsigned int f, nr_frags = skb_shinfo(skb)->nr_frags;
- unsigned int eof = 1;
- u32 ctrl;
+ unsigned int eof = 1, mss_mfs = 0;
+ u32 ctrl, lso_ctrl = 0, seq_ctrl = 0;
+
+ /* LSO */
+ if (skb_shinfo(skb)->gso_size != 0) {
+ if (IPPROTO_UDP == (((struct iphdr *)skb_network_header(skb))->protocol))
+ /* UDP - UFO */
+ lso_ctrl = MACB_LSO_UFO_ENABLE;
+ else
+ /* TCP - TSO */
+ lso_ctrl = MACB_LSO_TSO_ENABLE;
+ }
/* First, map non-paged data */
len = skb_headlen(skb);
+
+ /* first buffer length */
+ size = hdrlen;
+
offset = 0;
while (len) {
- size = min(len, bp->max_tx_length);
entry = macb_tx_ring_wrap(bp, tx_head);
tx_skb = &queue->tx_skb[entry];
@@ -1258,6 +1277,8 @@ static unsigned int macb_tx_map(struct macb *bp,
offset += size;
count++;
tx_head++;
+
+ size = min(len, bp->max_tx_length);
}
/* Then, map paged data from fragments */
@@ -1311,6 +1332,20 @@ static unsigned int macb_tx_map(struct macb *bp,
desc = &queue->tx_ring[entry];
desc->ctrl = ctrl;
+ if (lso_ctrl) {
+ if (lso_ctrl == MACB_LSO_UFO_ENABLE)
+ /* include header and FCS in value given to h/w */
+ mss_mfs = skb_shinfo(skb)->gso_size +
+ skb_transport_offset(skb) + 4;
+ else /* TSO */ {
+ mss_mfs = skb_shinfo(skb)->gso_size;
+ /* TCP Sequence Number Source Select
+ * can be set only for TSO
+ */
+ seq_ctrl = 0;
+ }
+ }
+
do {
i--;
entry = macb_tx_ring_wrap(bp, i);
@@ -1325,6 +1360,16 @@ static unsigned int macb_tx_map(struct macb *bp,
if (unlikely(entry == (bp->tx_ring_size - 1)))
ctrl |= MACB_BIT(TX_WRAP);
+ /* First descriptor is header descriptor */
+ if (i == queue->tx_head) {
+ ctrl |= MACB_BF(TX_LSO, lso_ctrl);
+ ctrl |= MACB_BF(TX_TCP_SEQ_SRC, seq_ctrl);
+ } else
+ /* Only set MSS/MFS on payload descriptors
+ * (second or later descriptor)
+ */
+ ctrl |= MACB_BF(MSS_MFS, mss_mfs);
+
/* Set TX buffer descriptor */
macb_set_addr(desc, tx_skb->mapping);
/* desc->addr must be visible to hardware before clearing
@@ -1350,6 +1395,43 @@ static unsigned int macb_tx_map(struct macb *bp,
return 0;
}
+static netdev_features_t macb_features_check(struct sk_buff *skb,
+ struct net_device *dev,
+ netdev_features_t features)
+{
+ unsigned int nr_frags, f;
+ unsigned int hdrlen;
+
+ /* Validate LSO compatibility */
+
+ /* there is only one buffer */
+ if (!skb_is_nonlinear(skb))
+ return features;
+
+ /* length of header */
+ hdrlen = skb_transport_offset(skb);
+ if (IPPROTO_TCP == (((struct iphdr *)skb_network_header(skb))->protocol))
+ hdrlen += tcp_hdrlen(skb);
+
+ /* For LSO:
+ * When software supplies two or more payload buffers all payload buffers
+ * apart from the last must be a multiple of 8 bytes in size.
+ */
+ if (!IS_ALIGNED(skb_headlen(skb) - hdrlen, MACB_TX_LEN_ALIGN))
+ return features & ~MACB_NETIF_LSO;
+
+ nr_frags = skb_shinfo(skb)->nr_frags;
+ /* No need to check last fragment */
+ nr_frags--;
+ for (f = 0; f < nr_frags; f++) {
+ const skb_frag_t *frag = &skb_shinfo(skb)->frags[f];
+
+ if (!IS_ALIGNED(skb_frag_size(frag), MACB_TX_LEN_ALIGN))
+ return features & ~MACB_NETIF_LSO;
+ }
+ return features;
+}
+
static inline int macb_clear_csum(struct sk_buff *skb)
{
/* no change for packets without checksum offloading */
@@ -1374,7 +1456,27 @@ static int macb_start_xmit(struct sk_buff *skb, struct net_device *dev)
struct macb *bp = netdev_priv(dev);
struct macb_queue *queue = &bp->queues[queue_index];
unsigned long flags;
- unsigned int count, nr_frags, frag_size, f;
+ unsigned int desc_cnt, nr_frags, frag_size, f;
+ unsigned int is_lso = 0, is_udp, hdrlen;
+
+ is_lso = (skb_shinfo(skb)->gso_size != 0);
+
+ if (is_lso) {
+ is_udp = (IPPROTO_UDP == (((struct iphdr *)skb_network_header(skb))->protocol));
+
+ /* length of headers */
+ if (is_udp)
+ /* only queue eth + ip headers separately for UDP */
+ hdrlen = skb_transport_offset(skb);
+ else
+ hdrlen = skb_transport_offset(skb) + tcp_hdrlen(skb);
+ if (skb_headlen(skb) < hdrlen) {
+ netdev_err(bp->dev, "Error - LSO headers fragmented!!!\n");
+ /* if this is required, would need to copy to single buffer */
+ return NETDEV_TX_BUSY;
+ }
+ } else
+ hdrlen = min(skb_headlen(skb), bp->max_tx_length);
#if defined(DEBUG) && defined(VERBOSE_DEBUG)
netdev_vdbg(bp->dev,
@@ -1389,18 +1491,22 @@ static int macb_start_xmit(struct sk_buff *skb, struct net_device *dev)
* socket buffer: skb fragments of jumbo frames may need to be
* split into many buffer descriptors.
*/
- count = DIV_ROUND_UP(skb_headlen(skb), bp->max_tx_length);
+ if (is_lso && (skb_headlen(skb) > hdrlen))
+ /* extra header descriptor if also payload in first buffer */
+ desc_cnt = DIV_ROUND_UP((skb_headlen(skb) - hdrlen), bp->max_tx_length) + 1;
+ else
+ desc_cnt = DIV_ROUND_UP(skb_headlen(skb), bp->max_tx_length);
nr_frags = skb_shinfo(skb)->nr_frags;
for (f = 0; f < nr_frags; f++) {
frag_size = skb_frag_size(&skb_shinfo(skb)->frags[f]);
- count += DIV_ROUND_UP(frag_size, bp->max_tx_length);
+ desc_cnt += DIV_ROUND_UP(frag_size, bp->max_tx_length);
}
spin_lock_irqsave(&bp->lock, flags);
/* This is a hard error, log it. */
if (CIRC_SPACE(queue->tx_head, queue->tx_tail,
- bp->tx_ring_size) < count) {
+ bp->tx_ring_size) < desc_cnt) {
netif_stop_subqueue(dev, queue_index);
spin_unlock_irqrestore(&bp->lock, flags);
netdev_dbg(bp->dev, "tx_head = %u, tx_tail = %u\n",
@@ -1408,13 +1514,19 @@ static int macb_start_xmit(struct sk_buff *skb, struct net_device *dev)
return NETDEV_TX_BUSY;
}
+ if (is_lso) {
+ if (is_udp)
+ /* zero UDP checksum, not calculated by h/w for UFO */
+ udp_hdr(skb)->check = 0;
+ }
+
if (macb_clear_csum(skb)) {
dev_kfree_skb_any(skb);
goto unlock;
}
/* Map socket buffer for DMA transfer */
- if (!macb_tx_map(bp, queue, skb)) {
+ if (!macb_tx_map(bp, queue, skb, hdrlen)) {
dev_kfree_skb_any(skb);
goto unlock;
}
@@ -2354,6 +2466,7 @@ static const struct net_device_ops macb_netdev_ops = {
.ndo_poll_controller = macb_poll_controller,
#endif
.ndo_set_features = macb_set_features,
+ .ndo_features_check = macb_features_check,
};
/* Configure peripheral capabilities according to device tree
@@ -2560,6 +2673,11 @@ static int macb_init(struct platform_device *pdev)
/* Set features */
dev->hw_features = NETIF_F_SG;
+
+ /* Check LSO capability */
+ if (GEM_BFEXT(PBUF_LSO, gem_readl(bp, DCFG6)))
+ dev->hw_features |= MACB_NETIF_LSO;
+
/* Checksum offload is only available on gem with packet buffer */
if (macb_is_gem(bp) && !(bp->caps & MACB_CAPS_FIFO_MODE))
dev->hw_features |= NETIF_F_HW_CSUM | NETIF_F_RXCSUM;
diff --git a/drivers/net/ethernet/cadence/macb.h b/drivers/net/ethernet/cadence/macb.h
index 1216950..d67adad 100644
--- a/drivers/net/ethernet/cadence/macb.h
+++ b/drivers/net/ethernet/cadence/macb.h
@@ -382,6 +382,10 @@
#define GEM_TX_PKT_BUFF_OFFSET 21
#define GEM_TX_PKT_BUFF_SIZE 1
+/* Bitfields in DCFG6. */
+#define GEM_PBUF_LSO_OFFSET 27
+#define GEM_PBUF_LSO_SIZE 1
+
/* Constants for CLK */
#define MACB_CLK_DIV8 0
#define MACB_CLK_DIV16 1
@@ -414,6 +418,10 @@
#define MACB_CAPS_SG_DISABLED 0x40000000
#define MACB_CAPS_MACB_IS_GEM 0x80000000
+/* LSO settings */
+#define MACB_LSO_UFO_ENABLE 0x01
+#define MACB_LSO_TSO_ENABLE 0x02
+
/* Bit manipulation macros */
#define MACB_BIT(name) \
(1 << MACB_##name##_OFFSET)
@@ -545,6 +553,12 @@ struct macb_dma_desc {
#define MACB_TX_LAST_SIZE 1
#define MACB_TX_NOCRC_OFFSET 16
#define MACB_TX_NOCRC_SIZE 1
+#define MACB_MSS_MFS_OFFSET 16
+#define MACB_MSS_MFS_SIZE 14
+#define MACB_TX_LSO_OFFSET 17
+#define MACB_TX_LSO_SIZE 2
+#define MACB_TX_TCP_SEQ_SRC_OFFSET 19
+#define MACB_TX_TCP_SEQ_SRC_SIZE 1
#define MACB_TX_BUF_EXHAUSTED_OFFSET 27
#define MACB_TX_BUF_EXHAUSTED_SIZE 1
#define MACB_TX_UNDERRUN_OFFSET 28
--
2.4.5
^ permalink raw reply related
* Re: [PATCH] virtio-net: drop legacy features in virtio 1 mode
From: Cornelia Huck @ 2016-11-04 11:15 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: netdev, linux-kernel, stable, virtualization
In-Reply-To: <1478256865-29003-1-git-send-email-mst@redhat.com>
On Fri, 4 Nov 2016 12:55:36 +0200
"Michael S. Tsirkin" <mst@redhat.com> wrote:
> Virtio 1.0 spec says VIRTIO_F_ANY_LAYOUT and VIRTIO_NET_F_GSO are
> legacy-only feature bits. Do not negotiate them in virtio 1 mode. Note
> this is a spec violation so we need to backport it to stable/downstream
> kernels.
>
> Cc: stable@vger.kernel.org
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
> drivers/net/virtio_net.c | 30 ++++++++++++++++++++----------
> 1 file changed, 20 insertions(+), 10 deletions(-)
Reviewed-by: Cornelia Huck <cornelia.huck@de.ibm.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