* Re: [PATCH net] bpf: don't select potentially stale ri->map from buggy xdp progs
From: Jesper Dangaard Brouer @ 2017-09-08 13:07 UTC (permalink / raw)
To: Daniel Borkmann; +Cc: andy, davem, ast, john.fastabend, netdev, brouer
In-Reply-To: <59B28E45.40700@iogearbox.net>
On Fri, 08 Sep 2017 14:34:13 +0200
Daniel Borkmann <daniel@iogearbox.net> wrote:
> On 09/08/2017 01:52 PM, Jesper Dangaard Brouer wrote:
> > On Fri, 08 Sep 2017 12:34:28 +0200 Daniel Borkmann <daniel@iogearbox.net> wrote:
> >> On 09/08/2017 07:06 AM, Jesper Dangaard Brouer wrote:
> >>> On Fri, 8 Sep 2017 00:14:51 +0200
> >>> Daniel Borkmann <daniel@iogearbox.net> wrote:
> >>>
> >>>> + /* This is really only caused by a deliberately crappy
> >>>> + * BPF program, normally we would never hit that case,
> >>>> + * so no need to inform someone via tracepoints either,
> >>>> + * just bail out.
> >>>> + */
> >>>> + if (unlikely(map_owner != xdp_prog))
> >>>> + return -EINVAL;
> >>>
> >>> IMHO we do need to call the tracepoint here. It is not just crappy
> >>> BPF-progs that cause this situation, it is also drivers not implementing
> >>> XDP_REDIRECT yet (which is all but ixgbe). Due to the level XDP
> >>> operates at, tracepoints are the only way users can runtime troubleshoot
> >>> their XDP programs.
> >>
> >> Drivers not implementing XDP_REDIRECT don't even get there in
> >> the first place. What they will do is to hit the 'default' case
> >> when they check for the action code from the BPF program. Then
> >> call into bpf_warn_invalid_xdp_action(act), and fall-through
> >> to hit the tracepoint at trace_xdp_exception() which is also
> >> triggered by XDP_ABORTED usually. So when that happens we do
> >> complain loudly and call a tracepoint already. We should probably
> >> tweak the bpf_warn_invalid_xdp_action() message a little to make
> >> it clear that the action could also just be unsupported by the
> >> driver instead of being illegal.
> >
> > Yes. drivers not implementing XDP_REDIRECT will cause a tracepoint
> > trace_xdp_exception() to be called for its _own_ packets.
>
> Yep, plus a big one time warning for the case a user doesn't
> look at tracepoints initially.
>
> > But it will still setup and leave map and map_owner pointer dangling.
> > Another NIC can load an xdp_prog that return XDP_REDIRECT, which will hit
> > above if-statement, and its packets will disappear, without getting
> > recorded by a tracepoint (thus hard to debug!).
>
> If a user wants to reproduce this exact error, he would need
> to go and reload the program on the driver not supporting the
> XDP_REDIRECT in the first place, and then reload his buggy program
> on the other driver supporting XDP_REDIRECT but w/o having called
> bpf_xdp_redirect_map(), so exactly once on the switch from one
> driver to another with this misuse, any subsequent packets will
> trigger _trace_xdp_redirect_err(), same way as if the buggy
> program was loaded to the 2nd driver from the beginning since
> the map and ifindex etc will be zero, hence my comment on this.
We can agree that the second program that experience the side-effect is
also buggy, as just returning XDP_REDIRECT without calling
bpf_xdp_redirect_map() or bpf_xdp_redirect(), is a bug in the bpf
program. Thus, the comment about a "deliberately crappy BPF program"
is not wrong.
You don't have to load and unload xdp programs. My test is simply
having two XDP programs running. 1. xdp_redirect_map on mlx5 which
doesn't implement XDP_REDIRECT, and 2. a "deliberately crappy BPF
program" on ixgbe that just returns XDP_REDIRECT.
In below output I've used -EFAULT == -14 to capture this situation
happening:
ksoftirqd/3 28 [003] 3437.829882: xdp:xdp_redirect_err: prog_id=3 action=REDIRECT ifindex=2 to_ifindex=0 err=-22
swapper 0 [005] 3437.829882: xdp:xdp_redirect_err: prog_id=3 action=REDIRECT ifindex=2 to_ifindex=0 err=-22
ksoftirqd/0 7 [000] 3437.829882: xdp:xdp_exception: prog_id=5 action=REDIRECT ifindex=7
ksoftirqd/4 34 [004] 3437.829882: xdp:xdp_redirect_err: prog_id=3 action=REDIRECT ifindex=2 to_ifindex=0 err=-22
ksoftirqd/2 22 [002] 3437.829882: xdp:xdp_redirect_err: prog_id=3 action=REDIRECT ifindex=2 to_ifindex=0 err=-22
ksoftirqd/3 28 [003] 3437.829882: xdp:xdp_redirect_err: prog_id=3 action=REDIRECT ifindex=2 to_ifindex=0 err=-22
swapper 0 [005] 3437.829882: xdp:xdp_redirect_err: prog_id=3 action=REDIRECT ifindex=2 to_ifindex=0 err=-22
swapper 0 [005] 3437.829882: xdp:xdp_redirect_err: prog_id=3 action=REDIRECT ifindex=2 to_ifindex=0 err=-22
ksoftirqd/3 28 [003] 3437.829882: xdp:xdp_redirect_err: prog_id=3 action=REDIRECT ifindex=2 to_ifindex=0 err=-22
ksoftirqd/2 22 [002] 3437.829882: xdp:xdp_redirect_err: prog_id=3 action=REDIRECT ifindex=2 to_ifindex=0 err=-22
ksoftirqd/4 34 [004] 3437.829882: xdp:xdp_redirect_err: prog_id=3 action=REDIRECT ifindex=2 to_ifindex=0 err=-22
ksoftirqd/3 28 [003] 3437.829882: xdp:xdp_redirect_err: prog_id=3 action=REDIRECT ifindex=2 to_ifindex=0 err=-22
ksoftirqd/2 22 [002] 3437.829882: xdp:xdp_redirect_err: prog_id=3 action=REDIRECT ifindex=2 to_ifindex=0 err=-22
ksoftirqd/0 7 [000] 3437.829882: xdp:xdp_redirect_map_err: prog_id=3 action=REDIRECT ifindex=2 to_ifindex=0 err=-14 map_id=5 map_index=0
swapper 0 [005] 3437.829883: xdp:xdp_redirect_err: prog_id=3 action=REDIRECT ifindex=2 to_ifindex=0 err=-22
ksoftirqd/3 28 [003] 3437.829883: xdp:xdp_redirect_err: prog_id=3 action=REDIRECT ifindex=2 to_ifindex=0 err=-22
And I can see I made a mistake and dereference the map_id ;-)
BTW, just to make it clear, I love the rest of the patch. And I love
how you solved this. Cool trick. You also closed a hole where someone
could set the map in one bpf_prog and cause the next bpf program to
forward using this map (this could be a policy violation).
--
Best regards,
Jesper Dangaard Brouer
MSc.CS, Principal Kernel Engineer at Red Hat
LinkedIn: http://www.linkedin.com/in/brouer
^ permalink raw reply
* Re: [PATCH 2/2] netfilter/libxt_hashlimit: new feature/algorithm for xt_hashlimit
From: Pablo Neira Ayuso @ 2017-09-08 13:10 UTC (permalink / raw)
To: Vishwanath Pai; +Cc: kadlec, netfilter-devel, johunt, fw, netdev, pai.vishwain
In-Reply-To: <1503089946-15996-1-git-send-email-vpai@akamai.com>
On Fri, Aug 18, 2017 at 04:59:06PM -0400, Vishwanath Pai wrote:
> This patch adds a new feature to hashlimit that allows matching on the
> current packet/byte rate without rate limiting. This can be enabled
> with a new flag --hashlimit-rate-match. The match returns true if the
> current rate of packets is above/below the user specified value.
Applied to iptables:
http://git.netfilter.org/iptables/commit/?id=1c32e5606fdf53856cba0cd9bc7b3f8b584b2cc2
Thanks.
^ permalink raw reply
* Re: [PATCH iproute2 2/2] lib/libnetlink: update rtnl_talk to support malloc buff at run time
From: Hangbin Liu @ 2017-09-08 13:26 UTC (permalink / raw)
To: Phil Sutter, netdev, Stephen Hemminger, Michal Kubecek
In-Reply-To: <20170908110652.GU2399@orbyte.nwl.cc>
On Fri, Sep 08, 2017 at 01:06:52PM +0200, Phil Sutter wrote:
> Hi Hangbin,
>
> On Fri, Sep 08, 2017 at 06:14:57PM +0800, Hangbin Liu wrote:
> [...]
> > diff --git a/genl/ctrl.c b/genl/ctrl.c
> > index 448988e..699657b 100644
> > --- a/genl/ctrl.c
> > +++ b/genl/ctrl.c
> > @@ -55,6 +55,7 @@ int genl_ctrl_resolve_family(const char *family)
> > };
> > struct nlmsghdr *nlh = &req.n;
> > struct genlmsghdr *ghdr = &req.g;
> > + struct nlmsghdr *answer = NULL;
>
> I don't think it's necessary to assign NULL here or in any of the other
> cases.
OK
>
> > if (rtnl_open_byproto(&rth, 0, NETLINK_GENERIC) < 0) {
> > fprintf(stderr, "Cannot open generic netlink socket\n");
> > @@ -63,19 +64,19 @@ int genl_ctrl_resolve_family(const char *family)
> >
> > addattr_l(nlh, 128, CTRL_ATTR_FAMILY_NAME, family, strlen(family) + 1);
> >
> > - if (rtnl_talk(&rth, nlh, nlh, sizeof(req)) < 0) {
> > + if (rtnl_talk(&rth, nlh, &answer) < 0) {
>
> I didn't check, but it's likely that at least in some cases 'nlh'
> contains some buffer space to hold the answer (if it will be larger than
> the input). If so, this could be dropped since the buffer is no longer
> reused.
Yes, I will remove the buffer.
Thanks
Hangbin
^ permalink raw reply
* Re: [PATCH v5 10/10] net: stmmac: dwmac-sun8i: Handle integrated/external MDIOs
From: Corentin Labbe @ 2017-09-08 13:26 UTC (permalink / raw)
To: Andrew Lunn
Cc: robh+dt, mark.rutland, maxime.ripard, wens, linux,
catalin.marinas, will.deacon, peppe.cavallaro, alexandre.torgue,
f.fainelli, netdev, devicetree, linux-arm-kernel, linux-kernel
In-Reply-To: <20170908130520.GA11248@lunn.ch>
On Fri, Sep 08, 2017 at 03:05:20PM +0200, Andrew Lunn wrote:
> > +#define DWMAC_sUN8I_MDIO_MUX_INTERNAL_ID 0
> > +#define DWMAC_sUN8I_MDIO_MUX_EXTERNAL_ID 1
> >
> > /* H3/A64 specific bits */
> > #define SYSCON_RMII_EN BIT(13) /* 1: enable RMII (overrides EPIT) */
> > @@ -634,6 +639,76 @@ static int sun8i_dwmac_reset(struct stmmac_priv *priv)
> > return 0;
> > }
> >
> > +/* MDIO multiplexing switch function
> > + * This function is called by the mdio-mux layer when it thinks the mdio bus
> > + * multiplexer needs to switch.
> > + * 'current_child' is the current value of the mux register
> > + * 'desired_child' is the value of the 'reg' property of the target child MDIO
> > + * node.
> > + * The first time this function is called, current_child == -1.
> > + * If current_child == desired_child, then the mux is already set to the
> > + * correct bus.
> > + *
> > + * Note that we do not use reg/mask like mdio-mux-mmioreg because we need to
> > + * know easily which bus is used (reset must be done only for desired bus).
> > + */
> > +static int mdio_mux_syscon_switch_fn(int current_child, int desired_child,
> > + void *data)
> > +{
> > + struct stmmac_priv *priv = data;
> > + struct sunxi_priv_data *gmac = priv->plat->bsp_priv;
> > + u32 reg, val;
> > + int ret = 0;
> > + bool need_reset = false;
> > +
> > + if (current_child ^ desired_child) {
> > + regmap_read(gmac->regmap, SYSCON_EMAC_REG, ®);
> > + switch (desired_child) {
> > + case DWMAC_sUN8I_MDIO_MUX_INTERNAL_ID:
> > + dev_info(priv->device, "Switch mux to internal PHY");
> > + val = (reg & ~H3_EPHY_MUX_MASK) | H3_EPHY_SELECT;
> > + if (gmac->use_internal_phy)
> > + need_reset = true;
> > + break;
>
> This i don't get. Why do you need use_internal_phy? Isn't that
> implicit from DWMAC_sUN8I_MDIO_MUX_INTERNAL_ID? Is it even possible to
> use an external PHY on the internal MDIO bus?
>
On my H3 box with external PHY, the MDIO mux library first select (for scan ?) the internal MDIO.
Without use_internal_phy usage, this board will launch a reset to use the internal MDIO... and this reset timeout/fail.
After the MDIO mux select the external MDIO.
> > + case DWMAC_sUN8I_MDIO_MUX_EXTERNAL_ID:
> > + dev_info(priv->device, "Switch mux to external PHY");
> > + val = (reg & ~H3_EPHY_MUX_MASK) | H3_EPHY_SHUTDOWN;
> > + if (!gmac->use_internal_phy)
> > + need_reset = true;
> > + break;
>
> And is it possible to use the internal PHY on the external bus?
>
I need to check that.
Regards
^ permalink raw reply
* Re: [PATCH RFC 0/6] Modify KSZ9477 DSA driver in preparation to add other KSZ switch drivers.
From: Vivien Didelot @ 2017-09-08 13:30 UTC (permalink / raw)
To: Tristram.Ha, andrew, muvarov, pavel, nathan.leigh.conrad,
f.fainelli, netdev, linux-kernel, Woojung.Huh
In-Reply-To: <93AF473E2DA327428DE3D46B72B1E9FD411219F8@CHN-SV-EXMX02.mchp-main.com>
Hi Tristram,
Tristram.Ha@microchip.com writes:
> From: Tristram Ha <Tristram.Ha@microchip.com>
>
> This series of patches is to modify the original KSZ9477 DSA driver so that other KSZ switch drivers can be added and use the common code.
Please see Documentation/process/submitting-patches.rst. Use a correct
formatting for the messages, and send you patchset as a thread.
> This patch set is against net-next.
The net subsystem is somehow special. Please see
Documentation/networking/netdev-FAQ.txt. The tree must be indicated in
the subject prefix of the patch series.
Thanks,
Vivien
^ permalink raw reply
* Re: [PATCH RFC] Update documentation for KSZ DSA drivers so that new drivers can be added
From: Maxim Uvarov @ 2017-09-08 13:32 UTC (permalink / raw)
To: Andrew Lunn
Cc: Tristram.Ha, Pavel Machek, Nathan Conrad, Vivien Didelot,
Florian Fainelli, netdev, linux-kernel, Woojung.Huh
In-Reply-To: <20170907215417.GU11248@lunn.ch>
2017-09-08 0:54 GMT+03:00 Andrew Lunn <andrew@lunn.ch>:
>> -- compatible: For external switch chips, compatible string must be exactly one
>> - of: "microchip,ksz9477"
>> +- compatible: Should be "microchip,ksz9477" for KSZ9477 chip,
>> + "microchip,ksz8795" for KSZ8795 chip,
>> + "microchip,ksz8794" for KSZ8794 chip,
>> + "microchip,ksz8765" for KSZ8765 chip,
>> + "microchip,ksz8895" for KSZ8895 chip,
>> + "microchip,ksz8864" for KSZ8864 chip,
>> + "microchip,ksz8873" for KSZ8873 chip,
>> + "microchip,ksz8863" for KSZ8863 chip,
>> + "microchip,ksz8463" for KSZ8463 chip
>
all that chips have the same spi access to get chip id on probe(). I
prefer common microship,ksz-spi rather than somebody will always
maintain that list.
Maxim.
> This part of this patch should be in a patch of the series that
> actually adds support for these chips. Don't document chips until you
> actually support them.
>
>> See Documentation/devicetree/bindings/dsa/dsa.txt for a list of additional required and optional properties.
>> @@ -13,60 +20,60 @@ Examples:
>>
>> Ethernet switch connected via SPI to the host, CPU port wired to eth0:
>>
>> - eth0: ethernet@10001000 {
>> - fixed-link {
>> - speed = <1000>;
>> - full-duplex;
>> - };
>> - };
>> + eth0: ethernet@10001000 {
>> + fixed-link {
>> + speed = <1000>;
>> + full-duplex;
>> + };
>> + };
>>
>> - spi1: spi@f8008000 {
>> - pinctrl-0 = <&pinctrl_spi_ksz>;
>> - cs-gpios = <&pioC 25 0>;
>> - id = <1>;
>> - status = "okay";
>> + spi1: spi@f8008000 {
>> + cs-gpios = <&pioC 25 0>;
>> + id = <1>;
>> + status = "okay";
>>
>> - ksz9477: ksz9477@0 {
>> - compatible = "microchip,ksz9477";
>> - reg = <0>;
>> + ksz9477: ksz9477@0 {
>> + compatible = "microchip,ksz9477";
>> + reg = <0>;
>>
>> - spi-max-frequency = <44000000>;
>> - spi-cpha;
>> - spi-cpol;
>> + spi-max-frequency = <44000000>;
>> + spi-cpha;
>> + spi-cpol;
>> +
>> + status = "okay";
>> + ports {
>> + #address-cells = <1>;
>> + #size-cells = <0>;
>> + port@0 {
>> + reg = <0>;
>> + label = "lan1";
>> + };
>> + port@1 {
>> + reg = <1>;
>> + label = "lan2";
>> + };
>> + port@2 {
>> + reg = <2>;
>> + label = "lan3";
>> + };
>> + port@3 {
>> + reg = <3>;
>> + label = "lan4";
>> + };
>> + port@4 {
>> + reg = <4>;
>> + label = "lan5";
>> + };
>> + port@5 {
>> + reg = <5>;
>> + label = "cpu";
>> + ethernet = <ð0>;
>> + fixed-link {
>> + speed = <1000>;
>> + full-duplex;
>> + };
>> + };
>> + };
>> + };
>> + };
>>
>> - status = "okay";
>> - ports {
>> - #address-cells = <1>;
>> - #size-cells = <0>;
>> - port@0 {
>> - reg = <0>;
>> - label = "lan1";
>> - };
>> - port@1 {
>> - reg = <1>;
>> - label = "lan2";
>> - };
>> - port@2 {
>> - reg = <2>;
>> - label = "lan3";
>> - };
>> - port@3 {
>> - reg = <3>;
>> - label = "lan4";
>> - };
>> - port@4 {
>> - reg = <4>;
>> - label = "lan5";
>> - };
>> - port@5 {
>> - reg = <5>;
>> - label = "cpu";
>> - ethernet = <ð0>;
>> - fixed-link {
>> - speed = <1000>;
>> - full-duplex;
>> - };
>> - };
>> - };
>> - };
>> - };
>
> This part however is a nice cleanup. You can submit this patch as a
> separate patch, once netdev has opened again in about 10 days time.
>
> Andrew
--
Best regards,
Maxim Uvarov
^ permalink raw reply
* Re: [RFC net-next 2/5] net/sched: Introduce Credit Based Shaper (CBS) qdisc
From: Henrik Austad @ 2017-09-08 13:43 UTC (permalink / raw)
To: Vinicius Costa Gomes
Cc: netdev, jhs, xiyou.wangcong, jiri, intel-wired-lan, andre.guedes,
ivan.briano, jesus.sanchez-palencia, boon.leong.ong,
richardcochran
In-Reply-To: <20170901012625.14838-3-vinicius.gomes@intel.com>
[-- Attachment #1: Type: text/plain, Size: 11796 bytes --]
On Thu, Aug 31, 2017 at 06:26:22PM -0700, Vinicius Costa Gomes wrote:
> This queueing discipline implements the shaper algorithm defined by
> the 802.1Q-2014 Section 8.6.8.2 and detailed in Annex L.
>
> It's primary usage is to apply some bandwidth reservation to user
> defined traffic classes, which are mapped to different queues via the
> mqprio qdisc.
>
> Initially, it only supports offloading the traffic shaping work to
> supporting controllers.
>
> Later, when a software implementation is added, the current dependency
> on being installed "under" mqprio can be lifted.
>
> Signed-off-by: Vinicius Costa Gomes <vinicius.gomes@intel.com>
> Signed-off-by: Jesus Sanchez-Palencia <jesus.sanchez-palencia@intel.com>
So, I started testing this in my VM to make sure I didn't do anything silly
and it exploded spectacularly as it used the underlying e1000 driver which
does not have a ndo_setup_tc present.
I commented inline below, but as a tl;dr
The cbs_init() would call into cbs_change() that would correctly detect
that ndo_setup_tc is missing and abort. However, at this stage it would try
to desroy the qdisc and in cbs_destroy() there's no such guard leading to a
BUG: unable to handle kernel NULL pointer dereference at 0000000000000010
Fixing that, another NULL would be found when the code walks into
qdisc_destroy(q->qdisc).
Apart from this, it loaded fine after some wrestling with tc :)
Awesome! :D
> ---
> include/linux/netdevice.h | 1 +
> net/sched/Kconfig | 11 ++
> net/sched/Makefile | 1 +
> net/sched/sch_cbs.c | 286 ++++++++++++++++++++++++++++++++++++++++++++++
> 4 files changed, 299 insertions(+)
> create mode 100644 net/sched/sch_cbs.c
>
> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> index 35de8312e0b5..dd9a2ecd0c03 100644
> --- a/include/linux/netdevice.h
> +++ b/include/linux/netdevice.h
> @@ -775,6 +775,7 @@ enum tc_setup_type {
> TC_SETUP_CLSFLOWER,
> TC_SETUP_CLSMATCHALL,
> TC_SETUP_CLSBPF,
> + TC_SETUP_CBS,
> };
>
> /* These structures hold the attributes of xdp state that are being passed
> diff --git a/net/sched/Kconfig b/net/sched/Kconfig
> index e70ed26485a2..c03d86a7775e 100644
> --- a/net/sched/Kconfig
> +++ b/net/sched/Kconfig
> @@ -172,6 +172,17 @@ config NET_SCH_TBF
> To compile this code as a module, choose M here: the
> module will be called sch_tbf.
>
> +config NET_SCH_CBS
Shouldn't this depend on NET_SCH_MQPRIO as it is supposed to hook into
that?
@@ -173,6 +173,7 @@ config NET_SCH_TBF
module will be called sch_tbf.
config NET_SCH_CBS
+ depends on NET_SCH_MQPRIO
tristate "Credit Based Shaper (CBS)"
---help---
Say Y here if you want to use the Credit Based Shaper (CBS) packet
> + tristate "Credit Based Shaper (CBS)"
> + ---help---
> + Say Y here if you want to use the Credit Based Shaper (CBS) packet
> + scheduling algorithm.
> +
> + See the top of <file:net/sched/sch_cbs.c> for more details.
> +
> + To compile this code as a module, choose M here: the
> + module will be called sch_cbs.
> +
> config NET_SCH_GRED
> tristate "Generic Random Early Detection (GRED)"
> ---help---
> diff --git a/net/sched/Makefile b/net/sched/Makefile
> index 7b915d226de7..80c8f92d162d 100644
> --- a/net/sched/Makefile
> +++ b/net/sched/Makefile
> @@ -52,6 +52,7 @@ obj-$(CONFIG_NET_SCH_FQ_CODEL) += sch_fq_codel.o
> obj-$(CONFIG_NET_SCH_FQ) += sch_fq.o
> obj-$(CONFIG_NET_SCH_HHF) += sch_hhf.o
> obj-$(CONFIG_NET_SCH_PIE) += sch_pie.o
> +obj-$(CONFIG_NET_SCH_CBS) += sch_cbs.o
>
> obj-$(CONFIG_NET_CLS_U32) += cls_u32.o
> obj-$(CONFIG_NET_CLS_ROUTE4) += cls_route.o
> diff --git a/net/sched/sch_cbs.c b/net/sched/sch_cbs.c
> new file mode 100644
> index 000000000000..1c86a9e14150
> --- /dev/null
> +++ b/net/sched/sch_cbs.c
> @@ -0,0 +1,286 @@
> +/*
> + * net/sched/sch_cbs.c Credit Based Shaper
> + *
> + * 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.
> + *
> + * Authors: Vininicius Costa Gomes <vinicius.gomes@intel.com>
> + *
> + */
> +
> +#include <linux/module.h>
> +#include <linux/types.h>
> +#include <linux/kernel.h>
> +#include <linux/string.h>
> +#include <linux/errno.h>
> +#include <linux/skbuff.h>
> +#include <net/netlink.h>
> +#include <net/sch_generic.h>
> +#include <net/pkt_sched.h>
> +
> +struct cbs_sched_data {
> + struct Qdisc *qdisc; /* Inner qdisc, default - pfifo queue */
> + s32 queue;
> + s32 locredit;
> + s32 hicredit;
> + s32 sendslope;
> + s32 idleslope;
> +};
> +
> +static int cbs_enqueue(struct sk_buff *skb, struct Qdisc *sch,
> + struct sk_buff **to_free)
> +{
> + struct cbs_sched_data *q = qdisc_priv(sch);
> + int ret;
> +
> + ret = qdisc_enqueue(skb, q->qdisc, to_free);
> + if (ret != NET_XMIT_SUCCESS) {
> + if (net_xmit_drop_count(ret))
> + qdisc_qstats_drop(sch);
> + return ret;
> + }
> +
> + qdisc_qstats_backlog_inc(sch, skb);
> + sch->q.qlen++;
> + return NET_XMIT_SUCCESS;
> +}
> +
> +static struct sk_buff *cbs_dequeue(struct Qdisc *sch)
> +{
> + struct cbs_sched_data *q = qdisc_priv(sch);
> + struct sk_buff *skb;
> +
> + skb = q->qdisc->ops->peek(q->qdisc);
> + if (skb) {
> + skb = qdisc_dequeue_peeked(q->qdisc);
> + if (unlikely(!skb))
> + return NULL;
> +
> + qdisc_qstats_backlog_dec(sch, skb);
> + sch->q.qlen--;
> + qdisc_bstats_update(sch, skb);
> +
> + return skb;
> + }
> + return NULL;
> +}
> +
> +static void cbs_reset(struct Qdisc *sch)
> +{
> + struct cbs_sched_data *q = qdisc_priv(sch);
> +
> + qdisc_reset(q->qdisc);
> +}
> +
> +static const struct nla_policy cbs_policy[TCA_CBS_MAX + 1] = {
> + [TCA_CBS_PARMS] = { .len = sizeof(struct tc_cbs_qopt) },
> +};
> +
> +static int cbs_change(struct Qdisc *sch, struct nlattr *opt)
> +{
> + struct cbs_sched_data *q = qdisc_priv(sch);
> + struct tc_cbs_qopt_offload cbs = { };
> + struct nlattr *tb[TCA_CBS_MAX + 1];
> + const struct net_device_ops *ops;
> + struct tc_cbs_qopt *qopt;
> + struct net_device *dev;
> + int err;
> +
> + err = nla_parse_nested(tb, TCA_CBS_MAX, opt, cbs_policy, NULL);
> + if (err < 0)
> + return err;
> +
> + err = -EINVAL;
> + if (!tb[TCA_CBS_PARMS])
> + goto done;
> +
> + qopt = nla_data(tb[TCA_CBS_PARMS]);
> +
> + dev = qdisc_dev(sch);
> + ops = dev->netdev_ops;
> +
> + /* FIXME: this means that we can only install this qdisc
> + * "under" mqprio. Do we need a more generic way to retrieve
> + * the queue, or do we pass the netdev_queue to the driver?
> + */
> + cbs.queue = TC_H_MIN(sch->parent) - 1 - netdev_get_num_tc(dev);
> +
> + cbs.enable = 1;
> + cbs.hicredit = qopt->hicredit;
> + cbs.locredit = qopt->locredit;
> + cbs.idleslope = qopt->idleslope;
> + cbs.sendslope = qopt->sendslope;
> +
> + err = -ENOTSUPP;
> + if (!ops->ndo_setup_tc)
> + goto done;
This confuses tc a bit, and looking at other qdisc schedulers, it seems
like EOPNOTSUPP is an alternative, this changes the output from
RTNETLINK answers: Unknown error 524
- to
RTNETLINK answers: Operation not supported
which perhaps is more informative.
> +
> + err = dev->netdev_ops->ndo_setup_tc(dev, TC_SETUP_CBS, &cbs);
> + if (err < 0)
> + goto done;
> +
> + q->queue = cbs.queue;
> + q->hicredit = cbs.hicredit;
> + q->locredit = cbs.locredit;
> + q->idleslope = cbs.idleslope;
> + q->sendslope = cbs.sendslope;
> +
> +done:
> + return err;
> +}
> +
> +static int cbs_init(struct Qdisc *sch, struct nlattr *opt)
> +{
> + struct cbs_sched_data *q = qdisc_priv(sch);
> +
> + if (!opt)
> + return -EINVAL;
> +
> + q->qdisc = fifo_create_dflt(sch, &pfifo_qdisc_ops, 1024);
> + qdisc_hash_add(q->qdisc, true);
> +
> + return cbs_change(sch, opt);
> +}
> +
> +static void cbs_destroy(struct Qdisc *sch)
> +{
> + struct cbs_sched_data *q = qdisc_priv(sch);
> + struct tc_cbs_qopt_offload cbs = { };
> + struct net_device *dev;
> + int err;
> +
> + q->hicredit = 0;
> + q->locredit = 0;
> + q->idleslope = 0;
> + q->sendslope = 0;
> +
> + dev = qdisc_dev(sch);
> +
> + cbs.queue = q->queue;
> + cbs.enable = 0;
> +
> + err = dev->netdev_ops->ndo_setup_tc(dev, TC_SETUP_CBS, &cbs);
This can lead to NULL pointer deref if it is not set (tested for in
cbs_change and error there leads us here, which then explodes).
> + if (err < 0)
> + pr_warn("Couldn't reset queue %d to default values\n",
> + cbs.queue);
> +
> + qdisc_destroy(q->qdisc);
Same, q->qdisc was NULL when cbs_init() failed.
> +}
> +
> +static int cbs_dump(struct Qdisc *sch, struct sk_buff *skb)
> +{
> + struct cbs_sched_data *q = qdisc_priv(sch);
> + struct nlattr *nest;
> + struct tc_cbs_qopt opt;
> +
> + sch->qstats.backlog = q->qdisc->qstats.backlog;
> + nest = nla_nest_start(skb, TCA_OPTIONS);
> + if (!nest)
> + goto nla_put_failure;
> +
> + opt.hicredit = q->hicredit;
> + opt.locredit = q->locredit;
> + opt.sendslope = q->sendslope;
> + opt.idleslope = q->idleslope;
> +
> + if (nla_put(skb, TCA_CBS_PARMS, sizeof(opt), &opt))
> + goto nla_put_failure;
> +
> + return nla_nest_end(skb, nest);
> +
> +nla_put_failure:
> + nla_nest_cancel(skb, nest);
> + return -1;
> +}
> +
> +static int cbs_dump_class(struct Qdisc *sch, unsigned long cl,
> + struct sk_buff *skb, struct tcmsg *tcm)
> +{
> + struct cbs_sched_data *q = qdisc_priv(sch);
> +
> + tcm->tcm_handle |= TC_H_MIN(1);
> + tcm->tcm_info = q->qdisc->handle;
> +
> + return 0;
> +}
> +
> +static int cbs_graft(struct Qdisc *sch, unsigned long arg, struct Qdisc *new,
> + struct Qdisc **old)
> +{
> + struct cbs_sched_data *q = qdisc_priv(sch);
> +
> + if (!new)
> + new = &noop_qdisc;
> +
> + *old = qdisc_replace(sch, new, &q->qdisc);
> + return 0;
> +}
> +
> +static struct Qdisc *cbs_leaf(struct Qdisc *sch, unsigned long arg)
> +{
> + struct cbs_sched_data *q = qdisc_priv(sch);
> +
> + return q->qdisc;
> +}
> +
> +static unsigned long cbs_find(struct Qdisc *sch, u32 classid)
> +{
> + return 1;
> +}
> +
> +static int cbs_delete(struct Qdisc *sch, unsigned long arg)
> +{
> + return 0;
> +}
> +
> +static void cbs_walk(struct Qdisc *sch, struct qdisc_walker *walker)
> +{
> + if (!walker->stop) {
> + if (walker->count >= walker->skip)
> + if (walker->fn(sch, 1, walker) < 0) {
> + walker->stop = 1;
> + return;
> + }
> + walker->count++;
> + }
> +}
> +
> +static const struct Qdisc_class_ops cbs_class_ops = {
> + .graft = cbs_graft,
> + .leaf = cbs_leaf,
> + .find = cbs_find,
> + .delete = cbs_delete,
> + .walk = cbs_walk,
> + .dump = cbs_dump_class,
> +};
> +
> +static struct Qdisc_ops cbs_qdisc_ops __read_mostly = {
> + .next = NULL,
> + .cl_ops = &cbs_class_ops,
> + .id = "cbs",
> + .priv_size = sizeof(struct cbs_sched_data),
> + .enqueue = cbs_enqueue,
> + .dequeue = cbs_dequeue,
> + .peek = qdisc_peek_dequeued,
> + .init = cbs_init,
> + .reset = cbs_reset,
> + .destroy = cbs_destroy,
> + .change = cbs_change,
> + .dump = cbs_dump,
> + .owner = THIS_MODULE,
> +};
> +
> +static int __init cbs_module_init(void)
> +{
> + return register_qdisc(&cbs_qdisc_ops);
> +}
> +
> +static void __exit cbs_module_exit(void)
> +{
> + unregister_qdisc(&cbs_qdisc_ops);
> +}
> +module_init(cbs_module_init)
> +module_exit(cbs_module_exit)
> +MODULE_LICENSE("GPL");
> --
> 2.14.1
>
--
Henrik Austad
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 181 bytes --]
^ permalink raw reply
* Re: [PATCH net-next 2/3] net: ethernet: socionext: add AVE ethernet driver
From: Andrew Lunn @ 2017-09-08 13:50 UTC (permalink / raw)
To: Kunihiko Hayashi
Cc: netdev, David S. Miller, Florian Fainelli, Rob Herring,
Mark Rutland, linux-arm-kernel, linux-kernel, devicetree,
Masahiro Yamada, Masami Hiramatsu, Jassi Brar
In-Reply-To: <1504875731-3680-3-git-send-email-hayashi.kunihiko@socionext.com>
> +static int ave_mdio_busywait(struct net_device *ndev)
> +{
> + int ret = 1, loop = 100;
> + u32 mdiosr;
> +
> + /* wait until completion */
> + while (1) {
> + mdiosr = ave_r32(ndev, AVE_MDIOSR);
> + if (!(mdiosr & AVE_MDIOSR_STS))
> + break;
> +
> + usleep_range(10, 20);
> + if (!loop--) {
> + netdev_err(ndev,
> + "failed to read from MDIO (status:0x%08x)\n",
> + mdiosr);
> + ret = 0;
ETIMEDOUT would be better.
> + break;
> + }
> + }
> +
> + return ret;
and then return 0 on success. That is the normal convention for return
values. An error code, and 0.
> +static int ave_mdiobus_write(struct mii_bus *bus,
> + int phyid, int regnum, u16 val)
> +{
> + struct net_device *ndev = bus->priv;
> + u32 mdioctl;
> +
> + /* write address */
> + ave_w32(ndev, AVE_MDIOAR, (phyid << 8) | regnum);
> +
> + /* write data */
> + ave_w32(ndev, AVE_MDIOWDR, val);
> +
> + /* write request */
> + mdioctl = ave_r32(ndev, AVE_MDIOCTR);
> + ave_w32(ndev, AVE_MDIOCTR, mdioctl | AVE_MDIOCTR_WREQ);
> +
> + if (!ave_mdio_busywait(ndev)) {
> + netdev_err(ndev, "phy-%d reg-%x write failed\n",
> + phyid, regnum);
> + return -1;
If ave_mdio_busywait() returns ETIMEDOUT, you can just return
it. Returning -1 is not good.
> + }
> +
> + return 0;
> +}
> +
> +static irqreturn_t ave_interrupt(int irq, void *netdev)
> +{
> + struct net_device *ndev = (struct net_device *)netdev;
> + struct ave_private *priv = netdev_priv(ndev);
> + u32 gimr_val, gisr_val;
> +
> + gimr_val = ave_irq_disable_all(ndev);
> +
> + /* get interrupt status */
> + gisr_val = ave_r32(ndev, AVE_GISR);
> +
> + /* PHY */
> + if (gisr_val & AVE_GI_PHY) {
> + ave_w32(ndev, AVE_GISR, AVE_GI_PHY);
> + if (priv->internal_phy_interrupt)
> + phy_mac_interrupt(ndev->phydev, ndev->phydev->link);
Humm. I don't think this is correct. You are supposed to give it the
new link state, not the old.
What does a PHY interrupt mean here?
> +static void ave_adjust_link(struct net_device *ndev)
> +{
> + struct ave_private *priv = netdev_priv(ndev);
> + struct phy_device *phydev = ndev->phydev;
> + u32 val, txcr, rxcr, rxcr_org;
> +
> + /* set RGMII speed */
> + val = ave_r32(ndev, AVE_TXCR);
> + val &= ~(AVE_TXCR_TXSPD_100 | AVE_TXCR_TXSPD_1G);
> +
> + if (priv->phy_mode == PHY_INTERFACE_MODE_RGMII &&
> + phydev->speed == SPEED_1000)
phy_interface_mode_is_rgmii(), so that you handle all the RGMII modes.
> + val |= AVE_TXCR_TXSPD_1G;
> + else if (phydev->speed == SPEED_100)
> + val |= AVE_TXCR_TXSPD_100;
> +
> + ave_w32(ndev, AVE_TXCR, val);
> +
> + /* set RMII speed (100M/10M only) */
> + if (priv->phy_mode != PHY_INTERFACE_MODE_RGMII) {
Not so safe. It would be better to check for the modes you actually
support.
> + if (phydev->link)
> + netif_carrier_on(ndev);
> + else
> + netif_carrier_off(ndev);
I don't think you need this. The phylib should do it for you.
> +
> + phy_print_status(phydev);
> +}
> +
> +static int ave_init(struct net_device *ndev)
> +{
> + struct ave_private *priv = netdev_priv(ndev);
> + struct device *dev = ndev->dev.parent;
> + struct device_node *phy_node, *np = dev->of_node;
> + struct phy_device *phydev;
> + const void *mac_addr;
> + u32 supported;
> +
> + /* get mac address */
> + mac_addr = of_get_mac_address(np);
> + if (mac_addr)
> + ether_addr_copy(ndev->dev_addr, mac_addr);
> +
> + /* if the mac address is invalid, use random mac address */
> + if (!is_valid_ether_addr(ndev->dev_addr)) {
> + eth_hw_addr_random(ndev);
> + dev_warn(dev, "Using random MAC address: %pM\n",
> + ndev->dev_addr);
> + }
> +
> + /* attach PHY with MAC */
> + phy_node = of_get_next_available_child(np, NULL);
???
Should this not be looking for a phy-handle property?
Documentation/devicetree/binds/net/ethernet.txt:
- phy-handle: phandle, specifies a reference to a node representing a PHY
device; this property is described in the Devicetree Specification and so
preferred;
> + phydev = of_phy_connect(ndev, phy_node,
> + ave_adjust_link, 0, priv->phy_mode);
> + if (!phydev) {
> + dev_err(dev, "could not attach to PHY\n");
> + return -ENODEV;
> + }
> + of_node_put(phy_node);
> +
> + priv->phydev = phydev;
> + phydev->autoneg = AUTONEG_ENABLE;
> + phydev->speed = 0;
> + phydev->duplex = 0;
And this should not be needed.
> +
> + dev_info(dev, "connected to %s phy with id 0x%x\n",
> + phydev->drv->name, phydev->phy_id);
phy_attached_info()
> +
> + if (priv->phy_mode != PHY_INTERFACE_MODE_RGMII) {
Same comment as above.
> + supported = phydev->supported;
> + phydev->supported &= ~PHY_GBIT_FEATURES;
> + phydev->supported |= supported & PHY_BASIC_FEATURES;
> + }
> +
> + /* PHY interrupt stop instruction is needed because the interrupt
> + * continues to assert.
> + */
> + phy_stop_interrupts(phydev);
Could you explain this some more? It sounds like your interrupt
controller is broken.
> +
> + /* When PHY driver can't handle its interrupt directly,
> + * interrupt request always fails and polling method is used
> + * alternatively. In this case, the libphy says
> + * "libphy: uniphier-mdio: Can't get IRQ -1 (PHY)".
> + */
> + phy_start_interrupts(phydev);
-1 is PHY_POLL. So calling phy_start_interrupts() is wrong. In fact,
you should not be calling phy_start_interrupts() at all. No other
Ethernet driver does.
> +
> + phy_start_aneg(phydev);
> +
> + return 0;
> +}
> +
> +static void ave_uninit(struct net_device *ndev)
> +{
> + struct ave_private *priv = netdev_priv(ndev);
> +
> + phy_stop_interrupts(priv->phydev);
And no other Ethernet driver calls phy_stop_interrupts either.
Please take a look at this.
> + phy_disconnect(priv->phydev);
> +}
> +
Andrew
^ permalink raw reply
* Re: [PATCH net-next 3/3] net: phy: realtek: add RTL8201F phy-id and functions
From: Andrew Lunn @ 2017-09-08 13:57 UTC (permalink / raw)
To: Kunihiko Hayashi
Cc: netdev, David S. Miller, Florian Fainelli, Rob Herring,
Mark Rutland, linux-arm-kernel, linux-kernel, devicetree,
Masahiro Yamada, Masami Hiramatsu, Jassi Brar, Jongsung Kim
In-Reply-To: <1504875731-3680-4-git-send-email-hayashi.kunihiko@socionext.com>
On Fri, Sep 08, 2017 at 10:02:11PM +0900, Kunihiko Hayashi wrote:
> From: Jassi Brar <jaswinder.singh@linaro.org>
>
> Add RTL8201F phy-id and the related functions to the driver.
>
> The original patch is as follows:
> https://patchwork.kernel.org/patch/2538341/
>
> Signed-off-by: Jongsung Kim <neidhard.kim@lge.com>
> Signed-off-by: Jassi Brar <jaswinder.singh@linaro.org>
> Signed-off-by: Kunihiko Hayashi <hayashi.kunihiko@socionext.com>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Andrew
^ permalink raw reply
* Re: [PATCH v5 10/10] net: stmmac: dwmac-sun8i: Handle integrated/external MDIOs
From: Andrew Lunn @ 2017-09-08 14:00 UTC (permalink / raw)
To: Corentin Labbe
Cc: robh+dt-DgEjT+Ai2ygdnm+yROfE0A, mark.rutland-5wv7dgnIgG8,
maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8, wens-jdAy2FN1RRM,
linux-I+IVW8TIWO2tmTQ+vhA3Yw, catalin.marinas-5wv7dgnIgG8,
will.deacon-5wv7dgnIgG8, peppe.cavallaro-qxv4g6HH51o,
alexandre.torgue-qxv4g6HH51o, f.fainelli-Re5JQEeQqe8AvxtiuMwx3w,
netdev-u79uwXL29TY76Z2rM5mHXA, devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-kernel-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <20170908132632.GA3037@Red>
> > > +static int mdio_mux_syscon_switch_fn(int current_child, int desired_child,
> > > + void *data)
> > > +{
> > > + struct stmmac_priv *priv = data;
> > > + struct sunxi_priv_data *gmac = priv->plat->bsp_priv;
> > > + u32 reg, val;
> > > + int ret = 0;
> > > + bool need_reset = false;
> > > +
> > > + if (current_child ^ desired_child) {
> > > + regmap_read(gmac->regmap, SYSCON_EMAC_REG, ®);
> > > + switch (desired_child) {
> > > + case DWMAC_sUN8I_MDIO_MUX_INTERNAL_ID:
> > > + dev_info(priv->device, "Switch mux to internal PHY");
> > > + val = (reg & ~H3_EPHY_MUX_MASK) | H3_EPHY_SELECT;
> > > + if (gmac->use_internal_phy)
> > > + need_reset = true;
> > > + break;
> >
> > This i don't get. Why do you need use_internal_phy? Isn't that
> > implicit from DWMAC_sUN8I_MDIO_MUX_INTERNAL_ID? Is it even possible to
> > use an external PHY on the internal MDIO bus?
> >
>
> On my H3 box with external PHY, the MDIO mux library first select (for scan ?) the internal MDIO.
> Without use_internal_phy usage, this board will launch a reset to use the internal MDIO... and this reset timeout/fail.
Do you know why the reset times out/fails?
Andrew
--
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 iproute2 1/2] lib/libnetlink: re malloc buff if size is not enough
From: Hangbin Liu @ 2017-09-08 14:01 UTC (permalink / raw)
To: Phil Sutter, netdev, Stephen Hemminger, Michal Kubecek
In-Reply-To: <20170908110247.GS2399@orbyte.nwl.cc>
Hi Phil,
Thanks for the comments, see replies bellow.
On Fri, Sep 08, 2017 at 01:02:47PM +0200, Phil Sutter wrote:
> Hi Hangbin,
>
> On Fri, Sep 08, 2017 at 06:14:56PM +0800, Hangbin Liu wrote:
> [...]
> > diff --git a/lib/libnetlink.c b/lib/libnetlink.c
> > index be7ac86..37cfb5a 100644
> > --- a/lib/libnetlink.c
> > +++ b/lib/libnetlink.c
> > @@ -402,6 +402,59 @@ static void rtnl_dump_error(const struct rtnl_handle *rth,
> > }
> > }
> >
> > +static int rtnl_recvmsg(int fd, struct msghdr *msg, char **buf)
> > +{
> > + struct iovec *iov;
> > + int len = -1, buf_len = 32768;
> > + char *buffer = *buf;
>
> Isn't it possible to make 'buffer' static instead of the two 'buf'
> variables in rtnl_dump_filter_l() and __rtnl_talk()? Then we would have
> only a single buffer which is shared between both functions instead of
> two which are independently allocated.
I was also thinking of this before. But in function ipaddrlabel_flush()
if (rtnl_dump_filter(&rth, flush_addrlabel, NULL) < 0)
It will cal rtnl_dump_filter_l() first via
rtnl_dump_filter() -> rtnl_dump_filter_nc() -> rtnl_dump_filter_l().
Then call rtnl_talk() later via call back
a->filter(&nladdr, h, a->arg1) -> flush_addrlabel() -> rtnl_talk()
So if we only use one static buffer in rtnl_recvmsg(). Then it will be written
at lease twice.
The path looks like bellow in function rtnl_dump_filter_l()
while (1) {
status = rtnl_recvmsg(rth->fd, &msg, &buf); <== write buf
for (a = arg; a->filter; a++) {
struct nlmsghdr *h = (struct nlmsghdr *)buf; <== assign buf to h
while (NLMSG_OK(h, msglen)) {
if (!rth->dump_fp) {
err = a->filter(&nladdr, h, a->arg1); <== buf changed via rtnl_talk()
}
h = NLMSG_NEXT(h, msglen); <== so h will also be changed
}
}
}
That's why I have to use two static buffers.
>
> > +
> > + int flag = MSG_PEEK | MSG_TRUNC;
> > +
> > + if (buffer == NULL)
> > +re_malloc:
> > + buffer = malloc(buf_len);
>
> I think using realloc() here is more appropriate since there is no need
> to free the buffer in beforehand and calling realloc(NULL, len) is
> equivalent to calling malloc(len). I think 'realloc' is also a better
> name for the goto label.
Good idea.
>
> > + if (buffer == NULL) {
> > + fprintf(stderr, "malloc error: no enough buffer\n");
>
> Minor typo here: s/no/not/
>
> > + return -1;
>
> Return -ENOMEM?
>
> > + }
> > +
> > + iov = msg->msg_iov;
> > + iov->iov_base = buffer;
> > + iov->iov_len = buf_len;
> > +
> > +re_recv:
>
> Just call this 'recv'? (Not really important though.)
>
> > + len = recvmsg(fd, msg, flag);
> > +
> > + if (len < 0) {
> > + if (errno == EINTR || errno == EAGAIN)
> > + return 0;
>
> Instead of returning 0 (which makes callers retry), goto re_recv?
Yes, will fix this.
>
> > + fprintf(stderr, "netlink receive error %s (%d)\n",
> > + strerror(errno), errno);
> > + return len;
> > + }
> > +
> > + if (len == 0) {
> > + fprintf(stderr, "EOF on netlink\n");
> > + return -1;
>
> Return -ENODATA here? (Initially I though about -EOF, but EOF is -1 so
> that would be incorrect).
>
> > + }
> > +
> > + if (len > buf_len) {
> > + free(buffer);
>
> If you use realloc() above, this can be dropped.
Yes.
>
> > + buf_len = len;
>
> For this to work you have to make buf_len static too, otherwise you will
> unnecessarily reallocate the buffer. Oh, and that also requires the
> single buffer (as pointed out above) because you will otherwise use a
> common buf_len for both static buffers passed to this function.
Since we have to use two static bufffers. So how about check like
if (len > strlen(buffer))
>
> > + flag = 0;
> > + goto re_malloc;
> > + }
> > +
> > + if (flag != 0) {
> > + flag = 0;
> > + goto re_recv;
> > + }
> > +
> > + *buf = buffer;
> > + return len;
> > +}
> > +
> > int rtnl_dump_filter_l(struct rtnl_handle *rth,
> > const struct rtnl_dump_filter_arg *arg)
> > {
> > @@ -413,31 +466,20 @@ int rtnl_dump_filter_l(struct rtnl_handle *rth,
> > .msg_iov = &iov,
> > .msg_iovlen = 1,
> > };
> > - char buf[32768];
> > + static char *buf = NULL;
>
> If you keep the static buffer in rtnl_recvmsg(), there is no need to
> assign NULL here.
>
> > int dump_intr = 0;
> >
> > - iov.iov_base = buf;
> > while (1) {
> > int status;
> > const struct rtnl_dump_filter_arg *a;
> > int found_done = 0;
> > int msglen = 0;
> >
> > - iov.iov_len = sizeof(buf);
> > - status = recvmsg(rth->fd, &msg, 0);
> > -
> > - if (status < 0) {
> > - if (errno == EINTR || errno == EAGAIN)
> > - continue;
> > - fprintf(stderr, "netlink receive error %s (%d)\n",
> > - strerror(errno), errno);
> > - return -1;
> > - }
> > -
> > - if (status == 0) {
> > - fprintf(stderr, "EOF on netlink\n");
> > - return -1;
> > - }
> > + status = rtnl_recvmsg(rth->fd, &msg, &buf);
> > + if (status < 0)
> > + return status;
> > + else if (status == 0)
> > + continue;
>
> When retrying inside rtnl_recvmsg(), it won't return 0 anymore. I
> believe the whole 'while (1)' loop could go away then.
>
Like Michal said, there may have multi netlink packets?
Thanks
Hangbin
^ permalink raw reply
* Re: [PATCH net-next v2 01/10] net: dsa: add debugfs interface
From: Vivien Didelot @ 2017-09-08 13:58 UTC (permalink / raw)
To: Greg KH
Cc: netdev, linux-kernel, kernel, David S. Miller, Florian Fainelli,
Andrew Lunn, Egil Hjelmeland, John Crispin, Woojung Huh,
Sean Wang, Nikita Yushchenko, Chris Healy
In-Reply-To: <20170907193434.GA11006@kroah.com>
Hi Greg,
Greg KH <gregkh@linuxfoundation.org> writes:
> I agree you shouldn't be using debugfs for this, but in the future, if
> you do write debugfs code, please take the following review into
> account:
Humm sorry I may not have given enough details. This was really meant
for debug and dev only, because DSA makes it hard to query directly the
hardware (some switch ports are not exposed to userspace as well.)
This is not meant to be used for anything real at all, or even be
compiled-in in a production kernel. That's why I found it appropriate.
So I am still wondering why it doesn't fit here, can you tell me why?
> You should _never_ care about the return value of a debugfs call, and
> you should not need to ever propagate the error upward. The api was
> written to not need this.
>
> Just call the function, and return, that's it. If you need to save the
> return value (i.e. it's a dentry), you also don't care, just save it and
> pass it to some other debugfs call, and all will still be fine. Your
> code should never do anything different if a debugfs call succeeds or
> fails.
Thank for your interesting review! I'll cleanup my out-of-tree patches.
Vivien
^ permalink raw reply
* Re: [PATCH net-next 1/3] dt-bindings: net: add DT bindings for Socionext UniPhier AVE
From: Andrew Lunn @ 2017-09-08 14:03 UTC (permalink / raw)
To: Kunihiko Hayashi
Cc: netdev, David S. Miller, Florian Fainelli, Rob Herring,
Mark Rutland, linux-arm-kernel, linux-kernel, devicetree,
Masahiro Yamada, Masami Hiramatsu, Jassi Brar
In-Reply-To: <1504875731-3680-2-git-send-email-hayashi.kunihiko@socionext.com>
> + eth: ethernet@65000000 {
> + compatible = "socionext,uniphier-ave4";
> + reg = <0x65000000 0x8500>;
> + interrupts = <0 66 4>;
> + pinctrl-names = "default";
> + pinctrl-0 = <&pinctrl_ether_rgmii>;
> + phy-mode = "rgmii";
> + socionext,desc-bits = <64>;
> + local-mac-address = [00 00 00 00 00 00];
> +
> + #address-cells = <1>;
> + #size-cells = <0>;
> + ethphy: ethphy@1 {
> + reg = <1>;
> + };
So you normally have an mdio node, and the phy as a children of that
node.
mdio {
ethphy: ethernet-phy@6 {
reg = <6>;
};
};
Andrew
^ permalink raw reply
* Re: [PATCH v5 10/10] net: stmmac: dwmac-sun8i: Handle integrated/external MDIOs
From: Corentin Labbe @ 2017-09-08 14:08 UTC (permalink / raw)
To: Andrew Lunn
Cc: robh+dt-DgEjT+Ai2ygdnm+yROfE0A, mark.rutland-5wv7dgnIgG8,
maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8, wens-jdAy2FN1RRM,
linux-I+IVW8TIWO2tmTQ+vhA3Yw, catalin.marinas-5wv7dgnIgG8,
will.deacon-5wv7dgnIgG8, peppe.cavallaro-qxv4g6HH51o,
alexandre.torgue-qxv4g6HH51o, f.fainelli-Re5JQEeQqe8AvxtiuMwx3w,
netdev-u79uwXL29TY76Z2rM5mHXA, devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-kernel-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <20170908140020.GC25219-g2DYL2Zd6BY@public.gmane.org>
On Fri, Sep 08, 2017 at 04:00:20PM +0200, Andrew Lunn wrote:
> > > > +static int mdio_mux_syscon_switch_fn(int current_child, int desired_child,
> > > > + void *data)
> > > > +{
> > > > + struct stmmac_priv *priv = data;
> > > > + struct sunxi_priv_data *gmac = priv->plat->bsp_priv;
> > > > + u32 reg, val;
> > > > + int ret = 0;
> > > > + bool need_reset = false;
> > > > +
> > > > + if (current_child ^ desired_child) {
> > > > + regmap_read(gmac->regmap, SYSCON_EMAC_REG, ®);
> > > > + switch (desired_child) {
> > > > + case DWMAC_sUN8I_MDIO_MUX_INTERNAL_ID:
> > > > + dev_info(priv->device, "Switch mux to internal PHY");
> > > > + val = (reg & ~H3_EPHY_MUX_MASK) | H3_EPHY_SELECT;
> > > > + if (gmac->use_internal_phy)
> > > > + need_reset = true;
> > > > + break;
> > >
> > > This i don't get. Why do you need use_internal_phy? Isn't that
> > > implicit from DWMAC_sUN8I_MDIO_MUX_INTERNAL_ID? Is it even possible to
> > > use an external PHY on the internal MDIO bus?
> > >
> >
> > On my H3 box with external PHY, the MDIO mux library first select (for scan ?) the internal MDIO.
> > Without use_internal_phy usage, this board will launch a reset to use the internal MDIO... and this reset timeout/fail.
>
> Do you know why the reset times out/fails?
>
Because there are nothing connected to it.
I got also reset timeout on integrated MDIO when the integrated PHY is not powered.
--
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] Update documentation for KSZ DSA drivers so that new drivers can be added
From: Andrew Lunn @ 2017-09-08 14:12 UTC (permalink / raw)
To: Maxim Uvarov
Cc: Tristram.Ha, Pavel Machek, Nathan Conrad, Vivien Didelot,
Florian Fainelli, netdev, linux-kernel, Woojung.Huh
In-Reply-To: <CAJGZr0+r3UhhRJcGXvBKZGamrSH_XEQY5oo7MTBjTTrDFOiKtg@mail.gmail.com>
On Fri, Sep 08, 2017 at 04:32:35PM +0300, Maxim Uvarov wrote:
> 2017-09-08 0:54 GMT+03:00 Andrew Lunn <andrew@lunn.ch>:
> >> -- compatible: For external switch chips, compatible string must be exactly one
> >> - of: "microchip,ksz9477"
> >> +- compatible: Should be "microchip,ksz9477" for KSZ9477 chip,
> >> + "microchip,ksz8795" for KSZ8795 chip,
> >> + "microchip,ksz8794" for KSZ8794 chip,
> >> + "microchip,ksz8765" for KSZ8765 chip,
> >> + "microchip,ksz8895" for KSZ8895 chip,
> >> + "microchip,ksz8864" for KSZ8864 chip,
> >> + "microchip,ksz8873" for KSZ8873 chip,
> >> + "microchip,ksz8863" for KSZ8863 chip,
> >> + "microchip,ksz8463" for KSZ8463 chip
> >
>
> all that chips have the same spi access to get chip id on probe(). I
> prefer common microship,ksz-spi rather than somebody will always
> maintain that list.
The Marvell DSA driver is similar. The compatibility string tells you
enough to go find the switch ID in the switch itself.
I suppose this comes down to, is there going to be one SPI driver for
all the devices, or lots of drivers? In general, DSA has one driver
for lots of devices. The mv88e6xxx supports around 25 devices. The b53
has around 17, etc.
So i would suggest one driver supporting all the different devices.
Andrew
^ permalink raw reply
* Re: [PATCH v5 10/10] net: stmmac: dwmac-sun8i: Handle integrated/external MDIOs
From: Andrew Lunn @ 2017-09-08 14:17 UTC (permalink / raw)
To: Corentin Labbe
Cc: robh+dt, mark.rutland, maxime.ripard, wens, linux,
catalin.marinas, will.deacon, peppe.cavallaro, alexandre.torgue,
f.fainelli, netdev, devicetree, linux-arm-kernel, linux-kernel
In-Reply-To: <20170908140832.GB3037@Red>
> > Do you know why the reset times out/fails?
> >
>
> Because there are nothing connected to it.
That should not be an issue. A read should just return 0xffff. And it
should return 0xffff fast. The timing of the MDIO protocol is fixed. A
read or a write takes a fixed number of cycles, independent of if
there is a device there or not. The bus data line has a pullup, so if
you try to access a missing device, you automatically read 0xffff.
Andrew
^ permalink raw reply
* Re: [PATCH] net: bonding: Fix transmit load balancing in balance-alb mode if specified by sysfs
From: Kosuke Tatsukawa @ 2017-09-08 14:17 UTC (permalink / raw)
To: Nikolay Aleksandrov
Cc: Jay Vosburgh, Veaceslav Falico, Andy Gospodarek,
netdev@vger.kernel.org, linux-kernel@vger.kernel.org,
Reinis Rozitis
In-Reply-To: <99818f9e-7ee0-4e53-b2be-b61b958f87e7@cumulusnetworks.com>
Hi,
> On 08/09/17 13:10, Nikolay Aleksandrov wrote:
>> On 08/09/17 05:06, Kosuke Tatsukawa wrote:
>>> Hi,
>>>
>>>> On 7.09.2017 01:47, Kosuke Tatsukawa wrote:
>>>>> Commit cbf5ecb30560 ("net: bonding: Fix transmit load balancing in
>>>>> balance-alb mode") tried to fix transmit dynamic load balancing in
>>>>> balance-alb mode, which wasn't working after commit 8b426dc54cf4
>>>>> ("bonding: remove hardcoded value").
>>>>>
>>>>> It turned out that my previous patch only fixed the case when
>>>>> balance-alb was specified as bonding module parameter, and not when
>>>>> balance-alb mode was set using /sys/class/net/*/bonding/mode (the most
>>>>> common usage). In the latter case, tlb_dynamic_lb was set up according
>>>>> to the default mode of the bonding interface, which happens to be
>>>>> balance-rr.
>>>>>
>>>>> This additional patch addresses this issue by setting up tlb_dynamic_lb
>>>>> to 1 if "mode" is set to balance-alb through the sysfs interface.
>>>>>
>>>>> I didn't add code to change tlb_balance_lb back to the default value for
>>>>> other modes, because "mode" is usually set up only once during
>>>>> initialization, and it's not worthwhile to change the static variable
>>>>> bonding_defaults in bond_main.c to a global variable just for this
>>>>> purpose.
>>>>>
>>>>> Commit 8b426dc54cf4 also changes the value of tlb_dynamic_lb for
>>>>> balance-tlb mode if it is set up using the sysfs interface. I didn't
>>>>> change that behavior, because the value of tlb_balance_lb can be changed
>>>>> using the sysfs interface for balance-tlb, and I didn't like changing
>>>>> the default value back and forth for balance-tlb.
>>>>>
>>>>> As for balance-alb, /sys/class/net/*/bonding/tlb_balance_lb cannot be
>>>>> written to. However, I think balance-alb with tlb_dynamic_lb set to 0
>>>>> is not an intended usage, so there is little use making it writable at
>>>>> this moment.
>>>>>
>>>>> Fixes: 8b426dc54cf4 ("bonding: remove hardcoded value")
>>>>> Reported-by: Reinis Rozitis <r@roze.lv>
>>>>> Signed-off-by: Kosuke Tatsukawa <tatsu@ab.jp.nec.com>
>>>>> Cc: stable@vger.kernel.org # v4.12+
>>>>> ---
>>>>> drivers/net/bonding/bond_options.c | 3 +++
>>>>> 1 files changed, 3 insertions(+), 0 deletions(-)
>>>>>
>>>>
>>>> I don't believe this to be the right solution, hardcoding it like this
>>>> changes user-visible behaviour. The issue is that if someone configured
>>>> it to be 0 in tlb mode, suddenly it will become 1 and will silently
>>>> override their config if they switch the mode to alb. Also it robs users
>>>> from their choice.
>>>>
>>>> If you think this should be settable in ALB mode, then IMO you should
>>>> edit tlb_dynamic_lb option's unsuppmodes and allow it to be set in ALB.
>>>> That would also be consistent with how it's handled in TLB mode.
>>>
>>> No, I don't think tlb_dynamic_lb should be settable in balance-alb at
>>> this point. All the current commits regarding tlb_dynamic_lb are for
>>> balance-tlb mode, so I don't think balance-alb with tlb_dynamic_lb set
>>> to 0 is an intended usage.
>>>
>>>
>>>> Going back and looking at your previous fix I'd argue that it is also
>>>> wrong, you should've removed the mode check altogether to return the
>>>> original behaviour where the dynamic_lb is set to 1 (enabled) by
>>>> default and then ALB mode would've had it, of course that would've left
>>>> the case of setting it to 0 in TLB mode and switching to ALB, but that
>>>> is a different issue.
>>>
>>> Maybe balance-alb shouldn't be dependent on tlb_dynamic_lb.
>>> tlb_dynamic_lb is referenced in the following functions.
>>>
>>> + bond_do_alb_xmit() -- Used by both balance-tlb and balance-alb
>>> + bond_tlb_xmit() -- Only used by balance-tlb
>>> + bond_open() -- Used by both balance-tlb and balance-alb
>>> + bond_check_params() -- Used during module initialization
>>> + bond_fill_info() -- Used to get/set value
>>> + bond_option_tlb_dynamic_lb_set() -- Used to get/set value
>>> + bonding_show_tlb_dynamic_lb() -- Used to get/set value
>>> + bond_is_nondyn_tlb() -- Only referenced if balance-tlb mode
>>>
>>> The following untested patch adds code to make balance-alb work as if
>>> tlb_dynamic_lb==1 for the functions which affect balance-alb mode. It
>>> also reverts my previous patch.
>>>
>>> What do you think about this approach?
>>> ---
>>> Kosuke TATSUKAWA | 1st Platform Software Division
>>> | NEC Solution Innovators
>>> | tatsu@ab.jp.nec.com
>>>
>>
>> Logically the approach looks good, that being said it adds unnecessary tests in
>> the fast path, why not just something like the patch below ? That only leaves the
>> problem if it is zeroed in TLB and switched to ALB mode, and that is a one line
>> fix to unsuppmodes just allow it to be set for that specific case. The below
>> returns the default behaviour before the commit in your Fixes tag.
>>
>>
>
> Actually I'm fine with your approach, too. It will fix this regardless of the
> value of tlb_dynamic_lb which sounds good to me for the price of a test in
> the fast path.
If you're concerned about the additional test in the fast path, how
about the patch below. I've added an arguemnt to bond_do_alb_xmit()
to handle both balance-tlb and balance-alb similary.
I'm not sure if this causes any problem if tlb_dynamic_lb is changed
while calling bond_do_alb_xmit() in balance-tlb mode.
---
Kosuke TATSUKAWA | 1st Platform Software Division
| NEC Solution Innovators
| tatsu@ab.jp.nec.com
------------------------------------------------------------------------
drivers/net/bonding/bond_alb.c | 11 ++++++-----
drivers/net/bonding/bond_main.c | 5 +++--
2 files changed, 9 insertions(+), 7 deletions(-)
diff --git a/drivers/net/bonding/bond_alb.c b/drivers/net/bonding/bond_alb.c
index c02cc81..7710f20 100644
--- a/drivers/net/bonding/bond_alb.c
+++ b/drivers/net/bonding/bond_alb.c
@@ -1317,7 +1317,7 @@ void bond_alb_deinitialize(struct bonding *bond)
}
static int bond_do_alb_xmit(struct sk_buff *skb, struct bonding *bond,
- struct slave *tx_slave)
+ struct slave *tx_slave, int tlb_dynamic_lb)
{
struct alb_bond_info *bond_info = &(BOND_ALB_INFO(bond));
struct ethhdr *eth_data = eth_hdr(skb);
@@ -1325,7 +1325,7 @@ static int bond_do_alb_xmit(struct sk_buff *skb, struct bonding *bond,
if (!tx_slave) {
/* unbalanced or unassigned, send through primary */
tx_slave = rcu_dereference(bond->curr_active_slave);
- if (bond->params.tlb_dynamic_lb)
+ if (tlb_dynamic_lb)
bond_info->unbalanced_load += skb->len;
}
@@ -1339,7 +1339,7 @@ static int bond_do_alb_xmit(struct sk_buff *skb, struct bonding *bond,
goto out;
}
- if (tx_slave && bond->params.tlb_dynamic_lb) {
+ if (tx_slave && tlb_dynamic_lb) {
spin_lock(&bond->mode_lock);
__tlb_clear_slave(bond, tx_slave, 0);
spin_unlock(&bond->mode_lock);
@@ -1386,7 +1386,8 @@ int bond_tlb_xmit(struct sk_buff *skb, struct net_device *bond_dev)
break;
}
}
- return bond_do_alb_xmit(skb, bond, tx_slave);
+ return bond_do_alb_xmit(skb, bond, tx_slave,
+ bond->params.tlb_dynamic_lb);
}
int bond_alb_xmit(struct sk_buff *skb, struct net_device *bond_dev)
@@ -1483,7 +1484,7 @@ int bond_alb_xmit(struct sk_buff *skb, struct net_device *bond_dev)
tx_slave = tlb_choose_channel(bond, hash_index, skb->len);
}
- return bond_do_alb_xmit(skb, bond, tx_slave);
+ return bond_do_alb_xmit(skb, bond, tx_slave, 1);
}
void bond_alb_monitor(struct work_struct *work)
diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index fc63992..bcb71e7 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -3305,7 +3305,8 @@ static int bond_open(struct net_device *bond_dev)
*/
if (bond_alb_initialize(bond, (BOND_MODE(bond) == BOND_MODE_ALB)))
return -ENOMEM;
- if (bond->params.tlb_dynamic_lb)
+ if (bond->params.tlb_dynamic_lb ||
+ (BOND_MODE(bond) == BOND_MODE_TLB))
queue_delayed_work(bond->wq, &bond->alb_work, 0);
}
@@ -4601,7 +4602,7 @@ static int bond_check_params(struct bond_params *params)
}
ad_user_port_key = valptr->value;
- if ((bond_mode == BOND_MODE_TLB) || (bond_mode == BOND_MODE_ALB)) {
+ if (bond_mode == BOND_MODE_TLB) {
bond_opt_initstr(&newval, "default");
valptr = bond_opt_parse(bond_opt_get(BOND_OPT_TLB_DYNAMIC_LB),
&newval);
^ permalink raw reply related
* Re: Fwd: DA850-evm MAC Address is random
From: Adam Ford @ 2017-09-08 14:18 UTC (permalink / raw)
To: Sekhar Nori; +Cc: Tony Lindgren, Grygorii Strashko, linux-omap, netdev
In-Reply-To: <37fd2aaa-0802-9fde-61ee-43bdc287ff36@ti.com>
On Thu, Sep 7, 2017 at 3:36 AM, Sekhar Nori <nsekhar@ti.com> wrote:
> On Thursday 07 September 2017 03:11 AM, Adam Ford wrote:
>> On Mon, Sep 4, 2017 at 11:42 PM, Sekhar Nori <nsekhar@ti.com> wrote:
>>> Hi Adam,
>>>
>>> On Wednesday 30 August 2017 11:08 AM, Sekhar Nori wrote:
>>>>> I wonder if U-Boot isn't pushing something to Linux because it doesn't
>>>>> appear to be running some of the da850 specific code even when I run
>>>>> linux-next. Can you tell me what verision of U-Boot you're using?
>>>>> Other than using davinci_all_defconfig, did you change the
>>>>> configuration at all?
>>>
>>>> I am using U-Boot 2017.01. Yes, the kernel was built using
>>>> davinci_all_defconfig and no other config change. Before booting kernel,
>>>> can you confirm that ethaddr is set in U-Boot environment? This is what
>>>> fdt_fixup_ethernet() reads to fixup the FDT before boot.
>>>>
>>>> Here is my complete boot log with environment variable dump.
>>>>
>>>> http://pastebin.ubuntu.com/25430265/
>>>
>>> Were you able to get rid of the random mac address problem?
>>
>> Not yet. I haven't been able to rebuild Arago using TI's instructions
>> on the Wiki. I am not sure if it's a dependency issue or something
>> else. When I run Linux 4.13 using Buildroot as the rootfs, it does
>> not appear to run da850_evm_m25p80_notify_add(). I am going to
>> investigate whether or not da850_evm_init() is getting called. I was
>> wondering if you had some insight as to what calls that function? It
>> looks like it's defined as part of MACHINE_START(DAVINCI_DA850_EVM,
>> "DaVinci DA850/OMAP-L138/AM18x EVM"), but I don't know how it gets
>> called.
>
> These functions are called only when booting using the legacy board file
> method. From your logs before, you are booting using device tree. So
> these functions are irrelevant.
Ok. That makes a lot more sense now. I was really confused why the functions
were not getting called.
> Can you check if the mac address has been populated in the device-tree
> by dumping it from /proc/device-tree/.../local-mac-address? That will
> tell us if U-Boot is updating the mac address or not.
It does not appear to getting called.
# hexdump ./soc@1c00000/ethernet@220000/local-mac-address
0000000 0000 0000 0000
0000006
#
I'll work on something that pulls in the MAC address then inserts it
into the device tree like the recommendation that Tony made.
Thanks for all your help.
adam
>
> Thanks,
> Sekhar
^ permalink raw reply
* Re: [PATCH net-next v2 02/10] net: dsa: debugfs: add tree
From: Vivien Didelot @ 2017-09-08 14:18 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: netdev, linux-kernel, kernel, David S. Miller, Florian Fainelli,
Andrew Lunn, Egil Hjelmeland, John Crispin, Woojung Huh,
Sean Wang, Nikita Yushchenko, Chris Healy
In-Reply-To: <20170828191748.19492-3-vivien.didelot@savoirfairelinux.com>
Hi Greg,
Can I ask for a quick review of this patch as well? It's the one adding
the boilerplate for a single debugfs file, and I'm pretty sure it can be
reduced somehow.
Also more important, you will notice what seems to be a bug to me:
I can read or write a file even if I didn't mask the corresponding mode,
hence the double check in dsa_debugfs_show and dsa_debugfs_write.
Thanks,
Vivien
^ permalink raw reply
* Re: [PATCH v5 10/10] net: stmmac: dwmac-sun8i: Handle integrated/external MDIOs
From: Corentin Labbe @ 2017-09-08 14:28 UTC (permalink / raw)
To: Andrew Lunn
Cc: robh+dt, mark.rutland, maxime.ripard, wens, linux,
catalin.marinas, will.deacon, peppe.cavallaro, alexandre.torgue,
f.fainelli, netdev, devicetree, linux-arm-kernel, linux-kernel
In-Reply-To: <20170908141736.GF25219@lunn.ch>
On Fri, Sep 08, 2017 at 04:17:36PM +0200, Andrew Lunn wrote:
> > > Do you know why the reset times out/fails?
> > >
> >
> > Because there are nothing connected to it.
>
> That should not be an issue. A read should just return 0xffff. And it
> should return 0xffff fast. The timing of the MDIO protocol is fixed. A
> read or a write takes a fixed number of cycles, independent of if
> there is a device there or not. The bus data line has a pullup, so if
> you try to access a missing device, you automatically read 0xffff.
>
Perhaps, but the reality is that with nothing connected to it, the reset of the MAC timeout.
Certainly, the MAC does not support finding no PHY.
So, to prevent an error message, and a "freeze" of the net process, the need_reset trick is necessary.
Regards
Corentin Labbe
^ permalink raw reply
* Re: [PATCH] net: bonding: Fix transmit load balancing in balance-alb mode if specified by sysfs
From: Nikolay Aleksandrov @ 2017-09-08 14:30 UTC (permalink / raw)
To: Kosuke Tatsukawa
Cc: Jay Vosburgh, Veaceslav Falico, Andy Gospodarek,
netdev@vger.kernel.org, linux-kernel@vger.kernel.org,
Reinis Rozitis
In-Reply-To: <17EC94B0A072C34B8DCF0D30AD16044A0298684C@BPXM09GP.gisp.nec.co.jp>
On 08/09/17 17:17, Kosuke Tatsukawa wrote:
> Hi,
>
>> On 08/09/17 13:10, Nikolay Aleksandrov wrote:
>>> On 08/09/17 05:06, Kosuke Tatsukawa wrote:
>>>> Hi,
>>>>
>>>>> On 7.09.2017 01:47, Kosuke Tatsukawa wrote:
>>>>>> Commit cbf5ecb30560 ("net: bonding: Fix transmit load balancing in
>>>>>> balance-alb mode") tried to fix transmit dynamic load balancing in
>>>>>> balance-alb mode, which wasn't working after commit 8b426dc54cf4
>>>>>> ("bonding: remove hardcoded value").
>>>>>>
>>>>>> It turned out that my previous patch only fixed the case when
>>>>>> balance-alb was specified as bonding module parameter, and not when
>>>>>> balance-alb mode was set using /sys/class/net/*/bonding/mode (the most
>>>>>> common usage). In the latter case, tlb_dynamic_lb was set up according
>>>>>> to the default mode of the bonding interface, which happens to be
>>>>>> balance-rr.
>>>>>>
>>>>>> This additional patch addresses this issue by setting up tlb_dynamic_lb
>>>>>> to 1 if "mode" is set to balance-alb through the sysfs interface.
>>>>>>
>>>>>> I didn't add code to change tlb_balance_lb back to the default value for
>>>>>> other modes, because "mode" is usually set up only once during
>>>>>> initialization, and it's not worthwhile to change the static variable
>>>>>> bonding_defaults in bond_main.c to a global variable just for this
>>>>>> purpose.
>>>>>>
>>>>>> Commit 8b426dc54cf4 also changes the value of tlb_dynamic_lb for
>>>>>> balance-tlb mode if it is set up using the sysfs interface. I didn't
>>>>>> change that behavior, because the value of tlb_balance_lb can be changed
>>>>>> using the sysfs interface for balance-tlb, and I didn't like changing
>>>>>> the default value back and forth for balance-tlb.
>>>>>>
>>>>>> As for balance-alb, /sys/class/net/*/bonding/tlb_balance_lb cannot be
>>>>>> written to. However, I think balance-alb with tlb_dynamic_lb set to 0
>>>>>> is not an intended usage, so there is little use making it writable at
>>>>>> this moment.
>>>>>>
>>>>>> Fixes: 8b426dc54cf4 ("bonding: remove hardcoded value")
>>>>>> Reported-by: Reinis Rozitis <r@roze.lv>
>>>>>> Signed-off-by: Kosuke Tatsukawa <tatsu@ab.jp.nec.com>
>>>>>> Cc: stable@vger.kernel.org # v4.12+
>>>>>> ---
>>>>>> drivers/net/bonding/bond_options.c | 3 +++
>>>>>> 1 files changed, 3 insertions(+), 0 deletions(-)
>>>>>>
>>>>>
>>>>> I don't believe this to be the right solution, hardcoding it like this
>>>>> changes user-visible behaviour. The issue is that if someone configured
>>>>> it to be 0 in tlb mode, suddenly it will become 1 and will silently
>>>>> override their config if they switch the mode to alb. Also it robs users
>>>>> from their choice.
>>>>>
>>>>> If you think this should be settable in ALB mode, then IMO you should
>>>>> edit tlb_dynamic_lb option's unsuppmodes and allow it to be set in ALB.
>>>>> That would also be consistent with how it's handled in TLB mode.
>>>>
>>>> No, I don't think tlb_dynamic_lb should be settable in balance-alb at
>>>> this point. All the current commits regarding tlb_dynamic_lb are for
>>>> balance-tlb mode, so I don't think balance-alb with tlb_dynamic_lb set
>>>> to 0 is an intended usage.
>>>>
>>>>
>>>>> Going back and looking at your previous fix I'd argue that it is also
>>>>> wrong, you should've removed the mode check altogether to return the
>>>>> original behaviour where the dynamic_lb is set to 1 (enabled) by
>>>>> default and then ALB mode would've had it, of course that would've left
>>>>> the case of setting it to 0 in TLB mode and switching to ALB, but that
>>>>> is a different issue.
>>>>
>>>> Maybe balance-alb shouldn't be dependent on tlb_dynamic_lb.
>>>> tlb_dynamic_lb is referenced in the following functions.
>>>>
>>>> + bond_do_alb_xmit() -- Used by both balance-tlb and balance-alb
>>>> + bond_tlb_xmit() -- Only used by balance-tlb
>>>> + bond_open() -- Used by both balance-tlb and balance-alb
>>>> + bond_check_params() -- Used during module initialization
>>>> + bond_fill_info() -- Used to get/set value
>>>> + bond_option_tlb_dynamic_lb_set() -- Used to get/set value
>>>> + bonding_show_tlb_dynamic_lb() -- Used to get/set value
>>>> + bond_is_nondyn_tlb() -- Only referenced if balance-tlb mode
>>>>
>>>> The following untested patch adds code to make balance-alb work as if
>>>> tlb_dynamic_lb==1 for the functions which affect balance-alb mode. It
>>>> also reverts my previous patch.
>>>>
>>>> What do you think about this approach?
>>>> ---
>>>> Kosuke TATSUKAWA | 1st Platform Software Division
>>>> | NEC Solution Innovators
>>>> | tatsu@ab.jp.nec.com
>>>>
>>>
>>> Logically the approach looks good, that being said it adds unnecessary tests in
>>> the fast path, why not just something like the patch below ? That only leaves the
>>> problem if it is zeroed in TLB and switched to ALB mode, and that is a one line
>>> fix to unsuppmodes just allow it to be set for that specific case. The below
>>> returns the default behaviour before the commit in your Fixes tag.
>>>
>>>
>>
>> Actually I'm fine with your approach, too. It will fix this regardless of the
>> value of tlb_dynamic_lb which sounds good to me for the price of a test in
>> the fast path.
>
> If you're concerned about the additional test in the fast path, how
> about the patch below. I've added an arguemnt to bond_do_alb_xmit()
> to handle both balance-tlb and balance-alb similary.
>
Even better, looks great! 1 question below though.
> I'm not sure if this causes any problem if tlb_dynamic_lb is changed
> while calling bond_do_alb_xmit() in balance-tlb mode.
The option has the ifdown flag, you shouldn't be able to change it while
the bond dev is up, but even if you could I don't think it will be an issue
for the xmit.
> ---
> Kosuke TATSUKAWA | 1st Platform Software Division
> | NEC Solution Innovators
> | tatsu@ab.jp.nec.com
>
> ------------------------------------------------------------------------
> drivers/net/bonding/bond_alb.c | 11 ++++++-----
> drivers/net/bonding/bond_main.c | 5 +++--
> 2 files changed, 9 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/net/bonding/bond_alb.c b/drivers/net/bonding/bond_alb.c
> index c02cc81..7710f20 100644
> --- a/drivers/net/bonding/bond_alb.c
> +++ b/drivers/net/bonding/bond_alb.c
> @@ -1317,7 +1317,7 @@ void bond_alb_deinitialize(struct bonding *bond)
> }
>
> static int bond_do_alb_xmit(struct sk_buff *skb, struct bonding *bond,
> - struct slave *tx_slave)
> + struct slave *tx_slave, int tlb_dynamic_lb)
> {
> struct alb_bond_info *bond_info = &(BOND_ALB_INFO(bond));
> struct ethhdr *eth_data = eth_hdr(skb);
> @@ -1325,7 +1325,7 @@ static int bond_do_alb_xmit(struct sk_buff *skb, struct bonding *bond,
> if (!tx_slave) {
> /* unbalanced or unassigned, send through primary */
> tx_slave = rcu_dereference(bond->curr_active_slave);
> - if (bond->params.tlb_dynamic_lb)
> + if (tlb_dynamic_lb)
> bond_info->unbalanced_load += skb->len;
> }
>
> @@ -1339,7 +1339,7 @@ static int bond_do_alb_xmit(struct sk_buff *skb, struct bonding *bond,
> goto out;
> }
>
> - if (tx_slave && bond->params.tlb_dynamic_lb) {
> + if (tx_slave && tlb_dynamic_lb) {
> spin_lock(&bond->mode_lock);
> __tlb_clear_slave(bond, tx_slave, 0);
> spin_unlock(&bond->mode_lock);
> @@ -1386,7 +1386,8 @@ int bond_tlb_xmit(struct sk_buff *skb, struct net_device *bond_dev)
> break;
> }
> }
> - return bond_do_alb_xmit(skb, bond, tx_slave);
> + return bond_do_alb_xmit(skb, bond, tx_slave,
> + bond->params.tlb_dynamic_lb);
> }
>
> int bond_alb_xmit(struct sk_buff *skb, struct net_device *bond_dev)
> @@ -1483,7 +1484,7 @@ int bond_alb_xmit(struct sk_buff *skb, struct net_device *bond_dev)
> tx_slave = tlb_choose_channel(bond, hash_index, skb->len);
> }
>
> - return bond_do_alb_xmit(skb, bond, tx_slave);
> + return bond_do_alb_xmit(skb, bond, tx_slave, 1);
> }
>
> void bond_alb_monitor(struct work_struct *work)
> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
> index fc63992..bcb71e7 100644
> --- a/drivers/net/bonding/bond_main.c
> +++ b/drivers/net/bonding/bond_main.c
> @@ -3305,7 +3305,8 @@ static int bond_open(struct net_device *bond_dev)
> */
> if (bond_alb_initialize(bond, (BOND_MODE(bond) == BOND_MODE_ALB)))
> return -ENOMEM;
> - if (bond->params.tlb_dynamic_lb)
> + if (bond->params.tlb_dynamic_lb ||
> + (BOND_MODE(bond) == BOND_MODE_TLB))
mode == tlb ? shouldn't this check be for alb ?
> queue_delayed_work(bond->wq, &bond->alb_work, 0);
> }
>
> @@ -4601,7 +4602,7 @@ static int bond_check_params(struct bond_params *params)
> }
> ad_user_port_key = valptr->value;
>
> - if ((bond_mode == BOND_MODE_TLB) || (bond_mode == BOND_MODE_ALB)) {
> + if (bond_mode == BOND_MODE_TLB) {
> bond_opt_initstr(&newval, "default");
> valptr = bond_opt_parse(bond_opt_get(BOND_OPT_TLB_DYNAMIC_LB),
> &newval);
>
^ permalink raw reply
* [PATCH net] sctp: fix missing wake ups in some situations
From: Marcelo Ricardo Leitner @ 2017-09-08 14:35 UTC (permalink / raw)
To: netdev; +Cc: linux-sctp, Neil Horman, Vlad Yasevich, Harald Welte
Commit fb586f25300f ("sctp: delay calls to sk_data_ready() as much as
possible") minimized the number of wake ups that are triggered in case
the association receives a packet with multiple data chunks on it and/or
when io_events are enabled and then commit 0970f5b36659 ("sctp: signal
sk_data_ready earlier on data chunks reception") moved the wake up to as
soon as possible. It thus relies on the state machine running later to
clean the flag that the event was already generated.
The issue is that there are 2 call paths that calls
sctp_ulpq_tail_event() outside of the state machine, causing the flag to
linger and possibly omitting a needed wake up in the sequence.
One of the call paths is when enabling SCTP_SENDER_DRY_EVENTS via
setsockopt(SCTP_EVENTS), as noticed by Harald Welte. The other is when
partial reliability triggers removal of chunks from the send queue when
the application calls sendmsg().
This commit fixes it by not setting the flag in case the socket is not
owned by the user, as it won't be cleaned later. This works for
user-initiated calls and also for rx path processing.
Fixes: fb586f25300f ("sctp: delay calls to sk_data_ready() as much as possible")
Reported-by: Harald Welte <laforge@gnumonks.org>
Signed-off-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
---
Hi. Please consider this one for -stable. Thanks
net/sctp/ulpqueue.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/net/sctp/ulpqueue.c b/net/sctp/ulpqueue.c
index 0225d62a869f8deff10565c4625df0a10464ce87..a71be33f3afeb0aaaef174ee082c4c547aab1e2d 100644
--- a/net/sctp/ulpqueue.c
+++ b/net/sctp/ulpqueue.c
@@ -265,7 +265,8 @@ int sctp_ulpq_tail_event(struct sctp_ulpq *ulpq, struct sctp_ulpevent *event)
sctp_ulpq_clear_pd(ulpq);
if (queue == &sk->sk_receive_queue && !sp->data_ready_signalled) {
- sp->data_ready_signalled = 1;
+ if (!sock_owned_by_user(sk))
+ sp->data_ready_signalled = 1;
sk->sk_data_ready(sk);
}
return 1;
--
2.13.5
^ permalink raw reply related
* IFA_F_OPTIMISTIC is not supported?
From: soohoon.lee @ 2017-09-08 14:29 UTC (permalink / raw)
To: netdev
rtm_newaddr masks off OPTIMISTIC.
inet6_rtm_newaddr(struct sk_buff *skb, struct nlmsghdr *nlh)
...
/* We ignore other flags so far. */
ifa_flags &= IFA_F_NODAD | IFA_F_HOMEADDRESS | IFA_F_MANAGETEMPADDR |
IFA_F_NOPREFIXROUTE;
Is there any problem or not allowed?
I need to do something like adding a route for the address after adding the address but fails because the address is not useable yet.
I tried NODAD but there's still little delay until IFA_F_TENTATIVE is cleared.
- rtm_newaddr sets IFA_F_TENTATIVE
- set a timer for dad start with delay=0
- some delay
- dad starts but ends immediately because of NODAD flag, and clears TENTATIVE.
And it looks like OPTIMISTIC does what I need but kind of disabled like the code above.
Thanks,
Soohoon.
^ permalink raw reply
* Re: [PATCH net-next v2 02/10] net: dsa: debugfs: add tree
From: Greg Kroah-Hartman @ 2017-09-08 14:40 UTC (permalink / raw)
To: Vivien Didelot
Cc: netdev, linux-kernel, kernel, David S. Miller, Florian Fainelli,
Andrew Lunn, Egil Hjelmeland, John Crispin, Woojung Huh,
Sean Wang, Nikita Yushchenko, Chris Healy
In-Reply-To: <87shfxxoi4.fsf@weeman.i-did-not-set--mail-host-address--so-tickle-me>
On Fri, Sep 08, 2017 at 10:18:27AM -0400, Vivien Didelot wrote:
> Hi Greg,
>
> Can I ask for a quick review of this patch as well? It's the one adding
> the boilerplate for a single debugfs file, and I'm pretty sure it can be
> reduced somehow.
I don't see a patch here :(
> Also more important, you will notice what seems to be a bug to me:
> I can read or write a file even if I didn't mask the corresponding mode,
> hence the double check in dsa_debugfs_show and dsa_debugfs_write.
The mode can be changed by userspace, you shouldn't ever need to check
it in any debugfs calls, right?
thanks,
greg k-h
^ permalink raw reply
* Re: [PATCH net-next 2/3] net: ethernet: socionext: add AVE ethernet driver
From: Masahiro Yamada @ 2017-09-08 14:44 UTC (permalink / raw)
To: Kunihiko Hayashi
Cc: netdev, David S. Miller, Andrew Lunn, Florian Fainelli,
Rob Herring, Mark Rutland, linux-arm-kernel,
Linux Kernel Mailing List, devicetree, Masami Hiramatsu,
Jassi Brar
In-Reply-To: <1504875731-3680-3-git-send-email-hayashi.kunihiko@socionext.com>
2017-09-08 22:02 GMT+09:00 Kunihiko Hayashi <hayashi.kunihiko@socionext.com>:
> diff --git a/drivers/net/ethernet/socionext/Kconfig b/drivers/net/ethernet/socionext/Kconfig
> new file mode 100644
> index 0000000..788f26f
> --- /dev/null
> +++ b/drivers/net/ethernet/socionext/Kconfig
> @@ -0,0 +1,22 @@
> +config NET_VENDOR_SOCIONEXT
> + bool "Socionext ethernet drivers"
> + default y
> + ---help---
> + Option to select ethernet drivers for Socionext platforms.
> +
> + Note that the answer to this question doesn't directly affect the
> + kernel: saying N will just cause the configurator to skip all
> + the questions about Agere devices. If you say Y, you will be asked
> + for your specific card in the following questions.
Agere?
> +
> + dev_info(dev, "Socionext %c%c%c%c Ethernet IP %s (irq=%d, phy=%s)\n",
> + (ave_id >> 24) & 0xff, (ave_id >> 16) & 0xff,
> + (ave_id >> 8) & 0xff, (ave_id >> 0) & 0xff,
> + buf, ndev->irq, phy_modes(phy_mode));
> +
> + return 0;
> +err_netdev_register:
Maybe, a bad label name.
for ex. out_del_napi or whatever.
Documentation/process/coding-style.rst says
"Choose label names which say what the goto does ..."
> + netif_napi_del(&priv->napi_rx);
> + netif_napi_del(&priv->napi_tx);
> + mdiobus_unregister(priv->mdio);
> +err_mdiobus_register:
> +err_mdiobus_alloc:
> +err_req_irq:
These three should be merged, for ex.
out_free_device.
I ran sparse for you.
Please take a look if it is worthwhile.
Seems endianess around mac_addr.
drivers/net/ethernet/socionext/sni_ave.c:423:15: warning: cast to
restricted __le32
drivers/net/ethernet/socionext/sni_ave.c:425:20: warning: cast to
restricted __le16
drivers/net/ethernet/socionext/sni_ave.c:1194:15: warning: cast to
restricted __le32
drivers/net/ethernet/socionext/sni_ave.c:1196:20: warning: cast to
restricted __le16
drivers/net/ethernet/socionext/sni_ave.c:1398:15: warning: cast to
restricted __le32
drivers/net/ethernet/socionext/sni_ave.c:1400:20: warning: cast to
restricted __le16
--
Best Regards
Masahiro Yamada
^ 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