Netdev List
 help / color / mirror / Atom feed
* Re: What to do when a bridge port gets its pvid deleted?
From: Vladimir Oltean @ 2019-08-19 21:10 UTC (permalink / raw)
  To: Ido Schimmel
  Cc: Florian Fainelli, Roopa Prabhu, nikolay, netdev, Andrew Lunn,
	Vivien Didelot, stephen
In-Reply-To: <20190819201502.GA25207@splinter>

On Mon, 19 Aug 2019 at 23:15, Ido Schimmel <idosch@idosch.org> wrote:
>
> On Mon, Aug 19, 2019 at 08:15:03PM +0300, Vladimir Oltean wrote:
> > On 6/28/19 7:45 PM, Florian Fainelli wrote:
> > > On 6/28/19 5:37 AM, Vladimir Oltean wrote:
> > > > On Fri, 28 Jun 2019 at 15:30, Ido Schimmel <idosch@idosch.org> wrote:
> > > > >
> > > > > On Tue, Jun 25, 2019 at 11:49:29PM +0300, Vladimir Oltean wrote:
> > > > > > A number of DSA drivers (BCM53XX, Microchip KSZ94XX, Mediatek MT7530
> > > > > > at the very least), as well as Mellanox Spectrum (I didn't look at all
> > > > > > the pure switchdev drivers) try to restore the pvid to a default value
> > > > > > on .port_vlan_del.
> > > > >
> > > > > I don't know about DSA drivers, but that's not what mlxsw is doing. If
> > > > > the VLAN that is configured as PVID is deleted from the bridge port, the
> > > > > driver instructs the port to discard untagged and prio-tagged packets.
> > > > > This is consistent with the bridge driver's behavior.
> > > > >
> > > > > We do have a flow the "restores" the PVID, but that's when a port is
> > > > > unlinked from its bridge master. The PVID we set is 4095 which cannot be
> > > > > configured by the 8021q / bridge driver. This is due to the way the
> > > > > underlying hardware works. Even if a port is not bridged and used purely
> > > > > for routing, packets still do L2 lookup first which sends them directly
> > > > > to the router block. If PVID is not configured, untagged packets could
> > > > > not be routed. Obviously, at egress we strip this VLAN.
> > > > >
> > > > > > Sure, the port stops receiving traffic when its pvid is a VLAN ID that
> > > > > > is not installed in its hw filter, but as far as the bridge core is
> > > > > > concerned, this is to be expected:
> > > > > >
> > > > > > # bridge vlan add dev swp2 vid 100 pvid untagged
> > > > > > # bridge vlan
> > > > > > port    vlan ids
> > > > > > swp5     1 PVID Egress Untagged
> > > > > >
> > > > > > swp2     1 Egress Untagged
> > > > > >           100 PVID Egress Untagged
> > > > > >
> > > > > > swp3     1 PVID Egress Untagged
> > > > > >
> > > > > > swp4     1 PVID Egress Untagged
> > > > > >
> > > > > > br0      1 PVID Egress Untagged
> > > > > > # ping 10.0.0.1
> > > > > > PING 10.0.0.1 (10.0.0.1) 56(84) bytes of data.
> > > > > > 64 bytes from 10.0.0.1: icmp_seq=1 ttl=64 time=0.682 ms
> > > > > > 64 bytes from 10.0.0.1: icmp_seq=2 ttl=64 time=0.299 ms
> > > > > > 64 bytes from 10.0.0.1: icmp_seq=3 ttl=64 time=0.251 ms
> > > > > > 64 bytes from 10.0.0.1: icmp_seq=4 ttl=64 time=0.324 ms
> > > > > > 64 bytes from 10.0.0.1: icmp_seq=5 ttl=64 time=0.257 ms
> > > > > > ^C
> > > > > > --- 10.0.0.1 ping statistics ---
> > > > > > 5 packets transmitted, 5 received, 0% packet loss, time 4188ms
> > > > > > rtt min/avg/max/mdev = 0.251/0.362/0.682/0.163 ms
> > > > > > # bridge vlan del dev swp2 vid 100
> > > > > > # bridge vlan
> > > > > > port    vlan ids
> > > > > > swp5     1 PVID Egress Untagged
> > > > > >
> > > > > > swp2     1 Egress Untagged
> > > > > >
> > > > > > swp3     1 PVID Egress Untagged
> > > > > >
> > > > > > swp4     1 PVID Egress Untagged
> > > > > >
> > > > > > br0      1 PVID Egress Untagged
> > > > > >
> > > > > > # ping 10.0.0.1
> > > > > > PING 10.0.0.1 (10.0.0.1) 56(84) bytes of data.
> > > > > > ^C
> > > > > > --- 10.0.0.1 ping statistics ---
> > > > > > 8 packets transmitted, 0 received, 100% packet loss, time 7267ms
> > > > > >
> > > > > > What is the consensus here? Is there a reason why the bridge driver
> > > > > > doesn't take care of this?
> > > > >
> > > > > Take care of what? :) Always maintaining a PVID on the bridge port? It's
> > > > > completely OK not to have a PVID.
> > > > >
> > > >
> > > > Yes, I didn't think it through during the first email. I came to the
> > > > same conclusion in the second one.
> > > >
> > > > > > Do switchdev drivers have to restore the pvid to always be
> > > > > > operational, even if their state becomes inconsistent with the upper
> > > > > > dev? Is it just 'nice to have'? What if VID 1 isn't in the hw filter
> > > > > > either (perfectly legal)?
> > > > >
> > > > > Are you saying that DSA drivers always maintain a PVID on the bridge
> > > > > port and allow untagged traffic to ingress regardless of the bridge
> > > > > driver's configuration? If so, I think this needs to be fixed.
> > > >
> > > > Well, not at the DSA core level.
> > > > But for Microchip:
> > > > https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git/tree/drivers/net/dsa/microchip/ksz9477.c#n576
> > > > For Broadcom:
> > > > https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git/tree/drivers/net/dsa/b53/b53_common.c#n1376
> > > > For Mediatek:
> > > > https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git/tree/drivers/net/dsa/mt7530.c#n1196
> > > >
> > > > There might be others as well.
> > >
> > > That sounds bogus indeed, and I bet that the two other drivers just
> > > followed the b53 driver there. I will have to test this again and come
> > > up with a patch eventually.
> > >
> > > When the port leaves the bridge we do bring it back into a default PVID
> > > (which is different than the bridge's default PVID) so that part should
> > > be okay.
> > >
> >
> > Adding a few more networking people.
> > So my flow is something like this:
> > - Boot a board with a DSA switch
> > - Bring all interfaces up
> > - Enslave all interfaces to br0
> > - Enable vlan_filtering on br0
> >
> > What VIDs should be installed into the ports' hw filters, and what should
> > the pvid be at this point?
> > Should the switch ports pass any traffic?
> > At this point, 'bridge vlan' shows a confusing:
> > port    vlan ids
> > eth0     1 PVID Egress Untagged
> >
> > swp5     1 PVID Egress Untagged
> >
> > swp2     1 PVID Egress Untagged
> >
> > swp3     1 PVID Egress Untagged
> >
> > swp4     1 PVID Egress Untagged
> >
> > br0      1 PVID Egress Untagged
> > for all ports, but the .port_vlan_add callback is nowhere to be found.
>
> The bridge adds a PVID on the port when it is enslaved to the bridge.
> The configuration only takes effect when VLAN filtering is enabled. I'm
> looking at dsa_port_vlan_add() and it seems that it does not propagate
> the VLAN call when VLAN filtering is disabled. This explains why you
> never see the callback.
>

Aha! The offending commit is this:

commit 2ea7a679ca2abd251c1ec03f20508619707e1749
Author: Andrew Lunn <andrew@lunn.ch>
Date:   Tue Nov 7 00:04:24 2017 +0100

    net: dsa: Don't add vlans when vlan filtering is disabled

    The software bridge can be build with vlan filtering support
    included. However, by default it is turned off. In its turned off
    state, it still passes VLANs via switchev, even though they are not to
    be used. Don't pass these VLANs to the hardware. Only do so when vlan
    filtering is enabled.

    This fixes at least one corner case. There are still issues in other
    corners, such as when vlan_filtering is later enabled.

    Signed-off-by: Andrew Lunn <andrew@lunn.ch>
    Signed-off-by: David S. Miller <davem@davemloft.net>

It's good to know that it's there (like you said, it explains some
things) but I can't exactly say that removing it helps in any way.
In fact, removing it only overwrites the dsa_8021q VLANs with 1 during
bridge_join, while not actually doing anything upon a vlan_filtering
toggle.
So the sja1105 driver is in a way shielded by DSA from the bridge, for
the better.
It still appears to be true that the bridge doesn't think it's
necessary to notify through SWITCHDEV_OBJ_ID_PORT_VLAN again. So my
best bet is to restore the pvid on my own.
However I've already stumbled upon an apparent bug while trying to do
that. Does this look off? If it doesn't, I'll submit it as a patch:

commit 788f03991aa576fc0b4b26ca330af0f412c55582
Author: Vladimir Oltean <olteanv@gmail.com>
Date:   Mon Aug 19 22:57:00 2019 +0300

    net: bridge: Keep the BRIDGE_VLAN_INFO_PVID flag in net_bridge_vlan

    Currently this simplified code snippet fails:

            br_vlan_get_pvid(netdev, &pvid);
            br_vlan_get_info(netdev, pvid, &vinfo);
            ASSERT(!(vinfo.flags & BRIDGE_VLAN_INFO_PVID));

    It is intuitive that the pvid of a netdevice should have the
    BRIDGE_VLAN_INFO_PVID flag set.

    However I can't seem to pinpoint a commit where this behavior was
    introduced. It seems like it's been like that since forever.

    Signed-off-by: Vladimir Oltean <olteanv@gmail.com>

diff --git a/net/bridge/br_vlan.c b/net/bridge/br_vlan.c
index 021cc9f66804..f49b2758bcab 100644
--- a/net/bridge/br_vlan.c
+++ b/net/bridge/br_vlan.c
@@ -68,10 +68,13 @@ static bool __vlan_add_flags(struct
net_bridge_vlan *v, u16 flags)
     else
         vg = nbp_vlan_group(v->port);

-    if (flags & BRIDGE_VLAN_INFO_PVID)
+    if (flags & BRIDGE_VLAN_INFO_PVID) {
         ret = __vlan_add_pvid(vg, v->vid);
-    else
+        v->flags |= BRIDGE_VLAN_INFO_PVID;
+    } else {
         ret = __vlan_delete_pvid(vg, v->vid);
+        v->flags &= ~BRIDGE_VLAN_INFO_PVID;
+    }

     if (flags & BRIDGE_VLAN_INFO_UNTAGGED)
         v->flags |= BRIDGE_VLAN_INFO_UNTAGGED;


> I assume that if you configure the bridge with VLAN filtering enabled
> and then enslave a port, then everything works fine.
>
> mlxsw avoids the situation by forbidding the toggling of VLAN filtering
> on the bridge when its ports are enslaved.
>

I can certainly understand where this comes from. However a simpleton
might object that this:

ip link add name br0 type bridge vlan_filtering 1
ip link set dev swp2 master br0

should behave the same as this:

ip link add name br0 type bridge
ip link set dev swp2 master br0
echo 1 > /sys/class/net/br0/bridge/vlan_filtering

I can't disagree with said simpleton.

> >
> > Whose responsibility is it for the switch to pass traffic without any
> > further 'bridge vlan' command? What is the mechanism through which this
> > should work?
> >
> > What if I do:
> > sudo bridge vlan add vid 100 dev swp2 pvid untagged
> > echo 0 | sudo tee /sys/class/net/br0/bridge/vlan_filtering
> > echo 1 | sudo tee /sys/class/net/br0/bridge/vlan_filtering
> > What pvid should there be on swp2 now?
> > 'bridge vlan' shows:
> > port    vlan ids
> > eth0     1 PVID Egress Untagged
> >
> > swp5     1 PVID Egress Untagged
> >
> > swp2     1 Egress Untagged
> >          100 PVID Egress Untagged
> >
> > swp3     1 PVID Egress Untagged
> >
> > swp4     1 PVID Egress Untagged
> >
> > br0      1 PVID Egress Untagged
> > If the 'bridge vlan' output is correct, whose responsibility is it to
> > restore this pvid?
>
> I suggest to follow mlxsw and avoid this mess. You can support both VLAN
> filtering enable / disable without supporting dynamically toggling the
> option.
>
> >
> > More context: the sja1105 driver is somewhat similar to the mlxsw in that
> > VLAN awareness cannot be truly disabled. Arid details aside, in both cases,
> > achieving "VLAN-unaware"-like behavior involves manipulating the pvid in
> > both cases. But it appears that the bridge core does expect:
> > (1) that the driver performs a default VLAN initialization which matches its
> > own, without them ever communicating. But because switchdev/DSA drivers
> > start off in standalone mode, vlan_filtering=0 comes first, hence the
> > non-standard pvid. Through what mechanism is the bridge-expected pvid
> > supposed to get restored upon flipping vlan_filtering?
> > (2) that toggling VLAN filtering off and on has no other state upon the
> > underlying driver than enabling and disabling VLAN awareness. The VLAN hw
> > filter table is assumed to be unchanged. Is this a correct assumption?
> >
> > Thanks,
> > -Vladimir

^ permalink raw reply related

* Re: INFO: task hung in tls_sw_release_resources_tx
From: Jakub Kicinski @ 2019-08-19 21:12 UTC (permalink / raw)
  To: Eric Biggers
  Cc: Steffen Klassert, syzbot, ast, aviadye, borisp, bpf, daniel,
	davejwatson, davem, hdanton, john.fastabend, netdev,
	syzkaller-bugs, herbert, linux-crypto
In-Reply-To: <20190817054743.GE8209@sol.localdomain>

On Fri, 16 Aug 2019 22:47:43 -0700, Eric Biggers wrote:
> [+Steffen, who is the maintainer of pcrypt]
> 
> On Fri, Aug 16, 2019 at 07:02:34PM -0700, Jakub Kicinski wrote:
> > On Thu, 15 Aug 2019 11:06:00 -0700, syzbot wrote:  
> > > syzbot has bisected this bug to:
> > > 
> > > commit 130b392c6cd6b2aed1b7eb32253d4920babb4891
> > > Author: Dave Watson <davejwatson@fb.com>
> > > Date:   Wed Jan 30 21:58:31 2019 +0000
> > > 
> > >      net: tls: Add tls 1.3 support
> > > 
> > > bisection log:  https://syzkaller.appspot.com/x/bisect.txt?x=118e8dee600000
> > > start commit:   6d5afe20 sctp: fix memleak in sctp_send_reset_streams
> > > git tree:       net
> > > final crash:    https://syzkaller.appspot.com/x/report.txt?x=138e8dee600000
> > > console output: https://syzkaller.appspot.com/x/log.txt?x=158e8dee600000
> > > kernel config:  https://syzkaller.appspot.com/x/.config?x=a4c9e9f08e9e8960
> > > dashboard link: https://syzkaller.appspot.com/bug?extid=6a9ff159672dfbb41c95
> > > syz repro:      https://syzkaller.appspot.com/x/repro.syz?x=17cb0502600000
> > > C reproducer:   https://syzkaller.appspot.com/x/repro.c?x=14d5dc22600000
> > > 
> > > Reported-by: syzbot+6a9ff159672dfbb41c95@syzkaller.appspotmail.com
> > > Fixes: 130b392c6cd6 ("net: tls: Add tls 1.3 support")
> > > 
> > > For information about bisection process see: https://goo.gl/tpsmEJ#bisection  
> > 
> > CC Herbert, linux-crypto
> > 
> > This is got to be something in the crypto code :S 
> > 
> > The test case opens a ktls socket and back log writes to it.
> > Then it opens a AF_ALG socket, binds "pcrypt(gcm(aes))" and dies.
> > 
> > The ktls socket upon close waits for async crypto callbacks, but they
> > never come. If I unset CRYPTO_USER_API_AEAD or change the alg to bind
> > to "gcm(aes)" the bug does not trigger.
> > 
> > Any suggestions?  
> 
> Seeing as pcrypt is involved and this is a "task hung" bug, this is probably
> caused by the recursive pcrypt deadlock, which is yet to be fixed.
> 
> See the original thread for more info:
> 
> 	https://groups.google.com/forum/#!msg/syzkaller-bugs/1_CXUd3gBcg/BvsRLH0lAgAJ
> 
> And the syzbot dashboard link:
> 
> 	https://syzkaller.appspot.com/bug?id=178f2528d10720d563091fb51dceb4cb20f75525
> 
> Let's tell syzbot this is a duplicate:
> 
> #syz dup: INFO: task hung in aead_recvmsg

Thanks for the suggestion Eric!

Looks like the dup didn't tickle syzbot the right way. Let me retry
sending this directly to the original report.

^ permalink raw reply

* Re: INFO: task hung in tls_sw_free_resources_tx
From: Jakub Kicinski @ 2019-08-19 21:16 UTC (permalink / raw)
  To: syzbot
  Cc: aviadye, borisp, daniel, davejwatson, davem, john.fastabend,
	linux-kernel, netdev, syzkaller-bugs, Eric Biggers
In-Reply-To: <000000000000cab053057c2e5202@google.com>

On Tue, 04 Dec 2018 00:48:02 -0800, syzbot wrote:
> Hello,
> 
> syzbot found the following crash on:
> 
> HEAD commit:    6915bf3b002b net: phy: don't allow __set_phy_supported to ..
> git tree:       net-next
> console output: https://syzkaller.appspot.com/x/log.txt?x=177085a3400000
> kernel config:  https://syzkaller.appspot.com/x/.config?x=28ecefa8a6e10719
> dashboard link: https://syzkaller.appspot.com/bug?extid=503339bf3c9053b8a7fc
> compiler:       gcc (GCC) 8.0.1 20180413 (experimental)
> syz repro:      https://syzkaller.appspot.com/x/repro.syz?x=11e6996d400000
> C reproducer:   https://syzkaller.appspot.com/x/repro.c?x=117e2125400000
> 
> IMPORTANT: if you fix the bug, please add the following tag to the commit:
> Reported-by: syzbot+503339bf3c9053b8a7fc@syzkaller.appspotmail.com

#syz dup: INFO: task hung in aead_recvmsg

^ permalink raw reply

* Re: [PATCH bpf-next v2 3/3] samples: bpf: syscal_nrs: use mmap2 if defined
From: Jonathan Lemon @ 2019-08-19 21:17 UTC (permalink / raw)
  To: Ivan Khoronzhuk
  Cc: magnus.karlsson, bjorn.topel, davem, hawk, john.fastabend,
	jakub.kicinski, daniel, netdev, bpf, xdp-newbies, linux-kernel,
	yhs, andrii.nakryiko
In-Reply-To: <20190815121356.8848-4-ivan.khoronzhuk@linaro.org>



On 15 Aug 2019, at 5:13, Ivan Khoronzhuk wrote:

> For arm32 xdp sockets mmap2 is preferred, so use it if it's defined.
> Declaration of __NR_mmap can be skipped and it breaks build.
>
> Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org>

Acked-by: Jonathan Lemon <jonathan.lemon@gmail.com>


> ---
>  samples/bpf/syscall_nrs.c  |  6 ++++++
>  samples/bpf/tracex5_kern.c | 13 +++++++++++++
>  2 files changed, 19 insertions(+)
>
> diff --git a/samples/bpf/syscall_nrs.c b/samples/bpf/syscall_nrs.c
> index 516e255cbe8f..88f940052450 100644
> --- a/samples/bpf/syscall_nrs.c
> +++ b/samples/bpf/syscall_nrs.c
> @@ -9,5 +9,11 @@ void syscall_defines(void)
>  	COMMENT("Linux system call numbers.");
>  	SYSNR(__NR_write);
>  	SYSNR(__NR_read);
> +#ifdef __NR_mmap2
> +	SYSNR(__NR_mmap2);
> +#endif
> +#ifdef __NR_mmap
>  	SYSNR(__NR_mmap);
> +#endif
> +
>  }
> diff --git a/samples/bpf/tracex5_kern.c b/samples/bpf/tracex5_kern.c
> index f57f4e1ea1ec..35cb0eed3be5 100644
> --- a/samples/bpf/tracex5_kern.c
> +++ b/samples/bpf/tracex5_kern.c
> @@ -68,12 +68,25 @@ PROG(SYS__NR_read)(struct pt_regs *ctx)
>  	return 0;
>  }
>
> +#ifdef __NR_mmap2
> +PROG(SYS__NR_mmap2)(struct pt_regs *ctx)
> +{
> +	char fmt[] = "mmap2\n";
> +
> +	bpf_trace_printk(fmt, sizeof(fmt));
> +	return 0;
> +}
> +#endif
> +
> +#ifdef __NR_mmap
>  PROG(SYS__NR_mmap)(struct pt_regs *ctx)
>  {
>  	char fmt[] = "mmap\n";
> +
>  	bpf_trace_printk(fmt, sizeof(fmt));
>  	return 0;
>  }
> +#endif
>
>  char _license[] SEC("license") = "GPL";
>  u32 _version SEC("version") = LINUX_VERSION_CODE;
> -- 
> 2.17.1

^ permalink raw reply

* [PATCH 2/2] bpf: clarify description for CONFIG_BPF_EVENTS
From: Peter Wu @ 2019-08-19 21:21 UTC (permalink / raw)
  To: Alexei Starovoitov, Daniel Borkmann; +Cc: netdev, bpf
In-Reply-To: <20190819212122.10286-1-peter@lekensteyn.nl>

PERF_EVENT_IOC_SET_BPF supports uprobes since v4.3, and tracepoints
since v4.7 via commit 04a22fae4cbc ("tracing, perf: Implement BPF
programs attached to uprobes"), and commit 98b5c2c65c29 ("perf, bpf:
allow bpf programs attach to tracepoints") respectively.

Signed-off-by: Peter Wu <peter@lekensteyn.nl>
---
 kernel/trace/Kconfig | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/kernel/trace/Kconfig b/kernel/trace/Kconfig
index 98da8998c25c..b09d7b1ffffd 100644
--- a/kernel/trace/Kconfig
+++ b/kernel/trace/Kconfig
@@ -520,7 +520,8 @@ config BPF_EVENTS
 	bool
 	default y
 	help
-	  This allows the user to attach BPF programs to kprobe events.
+	  This allows the user to attach BPF programs to kprobe, uprobe, and
+	  tracepoint events.
 
 config DYNAMIC_EVENTS
 	def_bool n
-- 
2.22.0


^ permalink raw reply related

* [PATCH 1/2] bpf: fix 'struct pt_reg' typo in documentation
From: Peter Wu @ 2019-08-19 21:21 UTC (permalink / raw)
  To: Alexei Starovoitov, Daniel Borkmann; +Cc: netdev, bpf
In-Reply-To: <20190819212122.10286-1-peter@lekensteyn.nl>

There is no 'struct pt_reg'.

Signed-off-by: Peter Wu <peter@lekensteyn.nl>
---
 include/uapi/linux/bpf.h       | 6 +++---
 tools/include/uapi/linux/bpf.h | 6 +++---
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index fa1c753dcdbc..9ca333c3ce91 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -1013,7 +1013,7 @@ union bpf_attr {
  * 		The realm of the route for the packet associated to *skb*, or 0
  * 		if none was found.
  *
- * int bpf_perf_event_output(struct pt_reg *ctx, struct bpf_map *map, u64 flags, void *data, u64 size)
+ * int bpf_perf_event_output(struct pt_regs *ctx, struct bpf_map *map, u64 flags, void *data, u64 size)
  * 	Description
  * 		Write raw *data* blob into a special BPF perf event held by
  * 		*map* of type **BPF_MAP_TYPE_PERF_EVENT_ARRAY**. This perf
@@ -1075,7 +1075,7 @@ union bpf_attr {
  * 	Return
  * 		0 on success, or a negative error in case of failure.
  *
- * int bpf_get_stackid(struct pt_reg *ctx, struct bpf_map *map, u64 flags)
+ * int bpf_get_stackid(struct pt_regs *ctx, struct bpf_map *map, u64 flags)
  * 	Description
  * 		Walk a user or a kernel stack and return its id. To achieve
  * 		this, the helper needs *ctx*, which is a pointer to the context
@@ -1724,7 +1724,7 @@ union bpf_attr {
  * 	Return
  * 		0 on success, or a negative error in case of failure.
  *
- * int bpf_override_return(struct pt_reg *regs, u64 rc)
+ * int bpf_override_return(struct pt_regs *regs, u64 rc)
  * 	Description
  * 		Used for error injection, this helper uses kprobes to override
  * 		the return value of the probed function, and to set it to *rc*.
diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
index 4e455018da65..8bff70ff5ce9 100644
--- a/tools/include/uapi/linux/bpf.h
+++ b/tools/include/uapi/linux/bpf.h
@@ -1013,7 +1013,7 @@ union bpf_attr {
  * 		The realm of the route for the packet associated to *skb*, or 0
  * 		if none was found.
  *
- * int bpf_perf_event_output(struct pt_reg *ctx, struct bpf_map *map, u64 flags, void *data, u64 size)
+ * int bpf_perf_event_output(struct pt_regs *ctx, struct bpf_map *map, u64 flags, void *data, u64 size)
  * 	Description
  * 		Write raw *data* blob into a special BPF perf event held by
  * 		*map* of type **BPF_MAP_TYPE_PERF_EVENT_ARRAY**. This perf
@@ -1075,7 +1075,7 @@ union bpf_attr {
  * 	Return
  * 		0 on success, or a negative error in case of failure.
  *
- * int bpf_get_stackid(struct pt_reg *ctx, struct bpf_map *map, u64 flags)
+ * int bpf_get_stackid(struct pt_regs *ctx, struct bpf_map *map, u64 flags)
  * 	Description
  * 		Walk a user or a kernel stack and return its id. To achieve
  * 		this, the helper needs *ctx*, which is a pointer to the context
@@ -1721,7 +1721,7 @@ union bpf_attr {
  * 	Return
  * 		0 on success, or a negative error in case of failure.
  *
- * int bpf_override_return(struct pt_reg *regs, u64 rc)
+ * int bpf_override_return(struct pt_regs *regs, u64 rc)
  * 	Description
  * 		Used for error injection, this helper uses kprobes to override
  * 		the return value of the probed function, and to set it to *rc*.
-- 
2.22.0


^ permalink raw reply related

* [PATCH 0/2] Small BPF documentation fixes
From: Peter Wu @ 2019-08-19 21:21 UTC (permalink / raw)
  To: Alexei Starovoitov, Daniel Borkmann; +Cc: netdev, bpf

Hi,

Some fixes for doc issues I ran into while playing with BPF and uprobes.

Kind regards,
Peter

Peter Wu (2):
  bpf: fix 'struct pt_reg' typo in documentation
  bpf: clarify description for CONFIG_BPF_EVENTS

 include/uapi/linux/bpf.h       | 6 +++---
 kernel/trace/Kconfig           | 3 ++-
 tools/include/uapi/linux/bpf.h | 6 +++---
 3 files changed, 8 insertions(+), 7 deletions(-)

-- 
2.22.0


^ permalink raw reply

* Re: KASAN: use-after-free Read in tls_write_space
From: Jakub Kicinski @ 2019-08-19 21:22 UTC (permalink / raw)
  To: syzbot
  Cc: aviadye, borisp, davejwatson, davem, linux-kernel, netdev,
	syzkaller-bugs
In-Reply-To: <0000000000003dab1605704fb71d@google.com>

On Fri, 06 Jul 2018 00:36:02 -0700, syzbot wrote:
> Hello,
> 
> syzbot found the following crash on:
> 
> HEAD commit:    1a84d7fdb5fc Merge branch 'mlxsw-Add-resource-scale-tests'
> git tree:       net-next
> console output: https://syzkaller.appspot.com/x/log.txt?x=17dec75c400000
> kernel config:  https://syzkaller.appspot.com/x/.config?x=a63be0c83e84d370
> dashboard link: https://syzkaller.appspot.com/bug?extid=2134b6b74dec9f8c760f
> compiler:       gcc (GCC) 8.0.1 20180413 (experimental)
> 
> Unfortunately, I don't have any reproducer for this crash yet.
> 
> IMPORTANT: if you fix the bug, please add the following tag to the commit:
> Reported-by: syzbot+2134b6b74dec9f8c760f@syzkaller.appspotmail.com

#syz dup: general protection fault in tls_write_space

^ permalink raw reply

* Re: KASAN: slab-out-of-bounds Read in tls_write_space
From: Jakub Kicinski @ 2019-08-19 21:22 UTC (permalink / raw)
  To: syzbot
  Cc: aviadye, borisp, davejwatson, davem, linux-kernel, netdev,
	syzkaller-bugs
In-Reply-To: <0000000000000a5b840576bad225@google.com>

On Tue, 25 Sep 2018 16:54:03 -0700, syzbot wrote:
> Hello,
> 
> syzbot found the following crash on:
> 
> HEAD commit:    bd4d08daeb95 Merge branch 'net-dsa-b53-SGMII-modes-fixes'
> git tree:       net-next
> console output: https://syzkaller.appspot.com/x/log.txt?x=110b6a1a400000
> kernel config:  https://syzkaller.appspot.com/x/.config?x=e79b9299baeb2298
> dashboard link: https://syzkaller.appspot.com/bug?extid=12638b747fd208f6cff0
> compiler:       gcc (GCC) 8.0.1 20180413 (experimental)
> syz repro:      https://syzkaller.appspot.com/x/repro.syz?x=167d9b9e400000
> C reproducer:   https://syzkaller.appspot.com/x/repro.c?x=15c4003a400000
> 
> IMPORTANT: if you fix the bug, please add the following tag to the commit:
> Reported-by: syzbot+12638b747fd208f6cff0@syzkaller.appspotmail.com

#syz dup: general protection fault in tls_write_space

^ permalink raw reply

* Re: general protection fault in tls_setsockopt
From: Jakub Kicinski @ 2019-08-19 21:29 UTC (permalink / raw)
  To: syzbot
  Cc: ast, aviadye, bhole_prashant_q7, borisp, daniel, davejwatson,
	davem, john.fastabend, linux-kernel, linux-kselftest, netdev,
	shuah, songliubraving, syzkaller-bugs
In-Reply-To: <000000000000d917f4058dd525cf@google.com>

On Tue, 16 Jul 2019 16:58:06 -0700, syzbot wrote:
> Hello,
> 
> syzbot found the following crash on:
> 
> HEAD commit:    a131c2bf Merge tag 'acpi-5.3-rc1-2' of git://git.kernel.or..
> git tree:       net-next
> console output: https://syzkaller.appspot.com/x/log.txt?x=1603e9c0600000
> kernel config:  https://syzkaller.appspot.com/x/.config?x=8bff73c5ba9e876
> dashboard link: https://syzkaller.appspot.com/bug?extid=23d9570edec63669d890
> compiler:       gcc (GCC) 9.0.0 20181231 (experimental)
> syz repro:      https://syzkaller.appspot.com/x/repro.syz?x=13560870600000

#syz fix: net/tls: partially revert fix transition through disconnect with close

^ permalink raw reply

* Re: INFO: task hung in tls_sw_release_resources_tx
From: Jakub Kicinski @ 2019-08-19 21:35 UTC (permalink / raw)
  To: Eric Biggers
  Cc: Steffen Klassert, syzbot, ast, aviadye, borisp, bpf, daniel,
	davejwatson, davem, hdanton, john.fastabend, netdev,
	syzkaller-bugs, herbert, linux-crypto
In-Reply-To: <20190819141255.010a323a@cakuba.netronome.com>

On Mon, 19 Aug 2019 14:12:55 -0700, Jakub Kicinski wrote:
> Looks like the dup didn't tickle syzbot the right way. Let me retry
> sending this directly to the original report.

Oh, no, my bad, there was just a third bug of the same nature.
tls_sw_release_resources_tx got renamed at some point, hence 
the duplicate report.

^ permalink raw reply

* Re: [PATCH net-next] netdevsim: Fix build error without CONFIG_INET
From: Jakub Kicinski @ 2019-08-19 21:59 UTC (permalink / raw)
  To: YueHaibing; +Cc: davem, idosch, jiri, mcroce, linux-kernel, netdev
In-Reply-To: <20190819120825.74460-1-yuehaibing@huawei.com>

On Mon, 19 Aug 2019 20:08:25 +0800, YueHaibing wrote:
> If CONFIG_INET is not set, building fails:
> 
> drivers/net/netdevsim/dev.o: In function `nsim_dev_trap_report_work':
> dev.c:(.text+0x67b): undefined reference to `ip_send_check'
> 
> Add CONFIG_INET Kconfig dependency to fix this.
> 
> Reported-by: Hulk Robot <hulkci@huawei.com>
> Fixes: da58f90f11f5 ("netdevsim: Add devlink-trap support")
> Signed-off-by: YueHaibing <yuehaibing@huawei.com>

Hmm.. I'd rather the test module did not have hard dependencies on
marginally important config options. We have done a pretty good job
so far limiting the requirements though separating the code out at
compilation object level. The more tests depend on netdevsim and the
more bots we have running tests against randconfig - the more important
this is.

This missing reference here is for calculating a checksum over a
constant header.. could we perhaps just hard code the checksum?

^ permalink raw reply

* [PATCH] Fix a double free bug in rsi_91x_deinit
From: Hui Peng @ 2019-08-19 22:02 UTC (permalink / raw)
  To: security
  Cc: Hui Peng, Mathias Payer, Kalle Valo, David S. Miller,
	linux-wireless, netdev, linux-kernel

`dev` (struct rsi_91x_usbdev *) field of adapter
(struct rsi_91x_usbdev *) is allocated  and initialized in
`rsi_init_usb_interface`. If any error is detected in information
read from the device side,  `rsi_init_usb_interface` will be
freed. However, in the higher level error handling code in
`rsi_probe`, if error is detected, `rsi_91x_deinit` is called
again, in which `dev` will be freed again, resulting double free.

This patch fixes the double free by removing the free operation on
`dev` in `rsi_init_usb_interface`, because `rsi_91x_deinit` is also
used in `rsi_disconnect`, in that code path, the `dev` field is not
 (and thus needs to be) freed.

This bug was found in v4.19, but is also present in the latest version
of kernel.

Reported-by: Hui Peng <benquike@gmail.com>
Reported-by: Mathias Payer <mathias.payer@nebelwelt.net>
Signed-off-by: Hui Peng <benquike@gmail.com>
---
 drivers/net/wireless/rsi/rsi_91x_usb.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/net/wireless/rsi/rsi_91x_usb.c b/drivers/net/wireless/rsi/rsi_91x_usb.c
index c0a163e40402..ac917227f708 100644
--- a/drivers/net/wireless/rsi/rsi_91x_usb.c
+++ b/drivers/net/wireless/rsi/rsi_91x_usb.c
@@ -640,7 +640,6 @@ static int rsi_init_usb_interface(struct rsi_hw *adapter,
 	kfree(rsi_dev->tx_buffer);
 
 fail_eps:
-	kfree(rsi_dev);
 
 	return status;
 }
-- 
2.22.1


^ permalink raw reply related

* Re: [PATCH net-next v7 2/3] net: phy: add support for clause 37 auto-negotiation
From: Tao Ren @ 2019-08-19 22:18 UTC (permalink / raw)
  To: Andrew Lunn, Florian Fainelli, Heiner Kallweit, David S . Miller,
	Vladimir Oltean, Arun Parameswaran, Justin Chen,
	netdev@vger.kernel.org, linux-kernel@vger.kernel.org,
	openbmc@lists.ozlabs.org
In-Reply-To: <20190811234010.3673592-1-taoren@fb.com>

On 8/11/19 4:40 PM, Tao Ren wrote:
> From: Heiner Kallweit <hkallweit1@gmail.com>
> 
> This patch adds support for clause 37 1000Base-X auto-negotiation.
> 
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> Signed-off-by: Tao Ren <taoren@fb.com>

A kind reminder: could someone help to review the patch when you have bandwidth?


Cheers,

Tao

> ---
>  Changes in v7:
>   - Update "if (AUTONEG_ENABLE != phydev->autoneg)" to
>     "if (phydev->autoneg != AUTONEG_ENABLE)" so checkpatch.pl is happy.
>  Changes in v6:
>   - add "Signed-off-by: Tao Ren <taoren@fb.com>"
>  Changes in v1-v5:
>   - nothing changed. It's given v5 just to align with the version of
>     patch series.
> 
>  drivers/net/phy/phy_device.c | 139 +++++++++++++++++++++++++++++++++++
>  include/linux/phy.h          |   5 ++
>  2 files changed, 144 insertions(+)
> 
> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> index 252a712d1b2b..301a794b2963 100644
> --- a/drivers/net/phy/phy_device.c
> +++ b/drivers/net/phy/phy_device.c
> @@ -1617,6 +1617,40 @@ static int genphy_config_advert(struct phy_device *phydev)
>  	return changed;
>  }
>  
> +/**
> + * genphy_c37_config_advert - sanitize and advertise auto-negotiation parameters
> + * @phydev: target phy_device struct
> + *
> + * Description: Writes MII_ADVERTISE with the appropriate values,
> + *   after sanitizing the values to make sure we only advertise
> + *   what is supported.  Returns < 0 on error, 0 if the PHY's advertisement
> + *   hasn't changed, and > 0 if it has changed. This function is intended
> + *   for Clause 37 1000Base-X mode.
> + */
> +static int genphy_c37_config_advert(struct phy_device *phydev)
> +{
> +	u16 adv = 0;
> +
> +	/* Only allow advertising what this PHY supports */
> +	linkmode_and(phydev->advertising, phydev->advertising,
> +		     phydev->supported);
> +
> +	if (linkmode_test_bit(ETHTOOL_LINK_MODE_1000baseX_Full_BIT,
> +			      phydev->advertising))
> +		adv |= ADVERTISE_1000XFULL;
> +	if (linkmode_test_bit(ETHTOOL_LINK_MODE_Pause_BIT,
> +			      phydev->advertising))
> +		adv |= ADVERTISE_1000XPAUSE;
> +	if (linkmode_test_bit(ETHTOOL_LINK_MODE_Asym_Pause_BIT,
> +			      phydev->advertising))
> +		adv |= ADVERTISE_1000XPSE_ASYM;
> +
> +	return phy_modify_changed(phydev, MII_ADVERTISE,
> +				  ADVERTISE_1000XFULL | ADVERTISE_1000XPAUSE |
> +				  ADVERTISE_1000XHALF | ADVERTISE_1000XPSE_ASYM,
> +				  adv);
> +}
> +
>  /**
>   * genphy_config_eee_advert - disable unwanted eee mode advertisement
>   * @phydev: target phy_device struct
> @@ -1726,6 +1760,54 @@ int genphy_config_aneg(struct phy_device *phydev)
>  }
>  EXPORT_SYMBOL(genphy_config_aneg);
>  
> +/**
> + * genphy_c37_config_aneg - restart auto-negotiation or write BMCR
> + * @phydev: target phy_device struct
> + *
> + * Description: If auto-negotiation is enabled, we configure the
> + *   advertising, and then restart auto-negotiation.  If it is not
> + *   enabled, then we write the BMCR. This function is intended
> + *   for use with Clause 37 1000Base-X mode.
> + */
> +int genphy_c37_config_aneg(struct phy_device *phydev)
> +{
> +	int err, changed;
> +
> +	if (phydev->autoneg != AUTONEG_ENABLE)
> +		return genphy_setup_forced(phydev);
> +
> +	err = phy_modify(phydev, MII_BMCR, BMCR_SPEED1000 | BMCR_SPEED100,
> +			 BMCR_SPEED1000);
> +	if (err)
> +		return err;
> +
> +	changed = genphy_c37_config_advert(phydev);
> +	if (changed < 0) /* error */
> +		return changed;
> +
> +	if (!changed) {
> +		/* Advertisement hasn't changed, but maybe aneg was never on to
> +		 * begin with?  Or maybe phy was isolated?
> +		 */
> +		int ctl = phy_read(phydev, MII_BMCR);
> +
> +		if (ctl < 0)
> +			return ctl;
> +
> +		if (!(ctl & BMCR_ANENABLE) || (ctl & BMCR_ISOLATE))
> +			changed = 1; /* do restart aneg */
> +	}
> +
> +	/* Only restart aneg if we are advertising something different
> +	 * than we were before.
> +	 */
> +	if (changed > 0)
> +		return genphy_restart_aneg(phydev);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(genphy_c37_config_aneg);
> +
>  /**
>   * genphy_aneg_done - return auto-negotiation status
>   * @phydev: target phy_device struct
> @@ -1864,6 +1946,63 @@ int genphy_read_status(struct phy_device *phydev)
>  }
>  EXPORT_SYMBOL(genphy_read_status);
>  
> +/**
> + * genphy_c37_read_status - check the link status and update current link state
> + * @phydev: target phy_device struct
> + *
> + * Description: Check the link, then figure out the current state
> + *   by comparing what we advertise with what the link partner
> + *   advertises. This function is for Clause 37 1000Base-X mode.
> + */
> +int genphy_c37_read_status(struct phy_device *phydev)
> +{
> +	int lpa, err, old_link = phydev->link;
> +
> +	/* Update the link, but return if there was an error */
> +	err = genphy_update_link(phydev);
> +	if (err)
> +		return err;
> +
> +	/* why bother the PHY if nothing can have changed */
> +	if (phydev->autoneg == AUTONEG_ENABLE && old_link && phydev->link)
> +		return 0;
> +
> +	phydev->duplex = DUPLEX_UNKNOWN;
> +	phydev->pause = 0;
> +	phydev->asym_pause = 0;
> +
> +	if (phydev->autoneg == AUTONEG_ENABLE && phydev->autoneg_complete) {
> +		lpa = phy_read(phydev, MII_LPA);
> +		if (lpa < 0)
> +			return lpa;
> +
> +		linkmode_mod_bit(ETHTOOL_LINK_MODE_Autoneg_BIT,
> +				 phydev->lp_advertising, lpa & LPA_LPACK);
> +		linkmode_mod_bit(ETHTOOL_LINK_MODE_1000baseX_Full_BIT,
> +				 phydev->lp_advertising, lpa & LPA_1000XFULL);
> +		linkmode_mod_bit(ETHTOOL_LINK_MODE_Pause_BIT,
> +				 phydev->lp_advertising, lpa & LPA_1000XPAUSE);
> +		linkmode_mod_bit(ETHTOOL_LINK_MODE_Asym_Pause_BIT,
> +				 phydev->lp_advertising,
> +				 lpa & LPA_1000XPAUSE_ASYM);
> +
> +		phy_resolve_aneg_linkmode(phydev);
> +	} else if (phydev->autoneg == AUTONEG_DISABLE) {
> +		int bmcr = phy_read(phydev, MII_BMCR);
> +
> +		if (bmcr < 0)
> +			return bmcr;
> +
> +		if (bmcr & BMCR_FULLDPLX)
> +			phydev->duplex = DUPLEX_FULL;
> +		else
> +			phydev->duplex = DUPLEX_HALF;
> +	}
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(genphy_c37_read_status);
> +
>  /**
>   * genphy_soft_reset - software reset the PHY via BMCR_RESET bit
>   * @phydev: target phy_device struct
> diff --git a/include/linux/phy.h b/include/linux/phy.h
> index 462b90b73f93..81a2921512ee 100644
> --- a/include/linux/phy.h
> +++ b/include/linux/phy.h
> @@ -1077,6 +1077,11 @@ int genphy_suspend(struct phy_device *phydev);
>  int genphy_resume(struct phy_device *phydev);
>  int genphy_loopback(struct phy_device *phydev, bool enable);
>  int genphy_soft_reset(struct phy_device *phydev);
> +
> +/* Clause 37 */
> +int genphy_c37_config_aneg(struct phy_device *phydev);
> +int genphy_c37_read_status(struct phy_device *phydev);
> +
>  static inline int genphy_no_soft_reset(struct phy_device *phydev)
>  {
>  	return 0;
> 

^ permalink raw reply

* Re: BUG: unable to handle kernel paging request in tls_prots
From: Jakub Kicinski @ 2019-08-19 22:34 UTC (permalink / raw)
  To: syzbot
  Cc: ast, bpf, daniel, davem, edumazet, john.fastabend, kafai, kuznet,
	linux-kernel, netdev, songliubraving, syzkaller-bugs, yhs,
	yoshfuji
In-Reply-To: <000000000000d7bcbb058c3758a1@google.com>

On Wed, 26 Jun 2019 03:17:05 -0700, syzbot wrote:
> Hello,
> 
> syzbot found the following crash on:
> 
> HEAD commit:    904d88d7 qmi_wwan: Fix out-of-bounds read
> git tree:       net
> console output: https://syzkaller.appspot.com/x/log.txt?x=14a8b865a00000
> kernel config:  https://syzkaller.appspot.com/x/.config?x=137ec2016ea3870d
> dashboard link: https://syzkaller.appspot.com/bug?extid=4207c7f3a443366d8aa2
> compiler:       gcc (GCC) 9.0.0 20181231 (experimental)
> syz repro:      https://syzkaller.appspot.com/x/repro.syz?x=15576c71a00000
> 
> IMPORTANT: if you fix the bug, please add the following tag to the commit:
> Reported-by: syzbot+4207c7f3a443366d8aa2@syzkaller.appspotmail.com

That's one of the bugs John fixed:

#syz fix: bpf: sockmap/tls, close can race with map free

^ permalink raw reply

* Re: WARNING: ODEBUG bug in tls_sw_free_resources_tx
From: Jakub Kicinski @ 2019-08-19 22:37 UTC (permalink / raw)
  To: syzbot
  Cc: aviadye, borisp, daniel, davejwatson, davem, john.fastabend,
	linux-kernel, netdev, syzkaller-bugs
In-Reply-To: <00000000000062c5c3057a095d25@google.com>

On Tue, 06 Nov 2018 17:52:03 -0800, syzbot wrote:
> Hello,
> 
> syzbot found the following crash on:
> 
> HEAD commit:    7c6c54b505b8 Merge branch 'i2c/for-next' of git://git.kern..
> git tree:       net-next
> console output: https://syzkaller.appspot.com/x/log.txt?x=1276246d400000
> kernel config:  https://syzkaller.appspot.com/x/.config?x=136ed5b316dbf1d8
> dashboard link: https://syzkaller.appspot.com/bug?extid=70ab6a1f8151888c4ea0
> compiler:       gcc (GCC) 8.0.1 20180413 (experimental)
> 
> Unfortunately, I don't have any reproducer for this crash yet.
> 
> IMPORTANT: if you fix the bug, please add the following tag to the commit:
> Reported-by: syzbot+70ab6a1f8151888c4ea0@syzkaller.appspotmail.com

This was most likely fixed by John:

#syz fix: net/tls: remove close callback sock unlock/lock around TX work flush

^ permalink raw reply

* Re: KASAN: use-after-free Read in tls_push_sg
From: Jakub Kicinski @ 2019-08-19 22:42 UTC (permalink / raw)
  To: syzbot
  Cc: aviadye, borisp, daniel, davejwatson, davem, john.fastabend,
	linux-kernel, netdev, syzkaller-bugs
In-Reply-To: <0000000000000d1491058919b662@google.com>

On Fri, 17 May 2019 11:40:05 -0700, syzbot wrote:
> Hello,
> 
> syzbot found the following crash on:
> 
> HEAD commit:    35c99ffa Merge tag 'for_linus' of git://git.kernel.org/pub..
> git tree:       net-next
> console output: https://syzkaller.appspot.com/x/log.txt?x=10ff3322a00000
> kernel config:  https://syzkaller.appspot.com/x/.config?x=82f0809e8f0a8c87
> dashboard link: https://syzkaller.appspot.com/bug?extid=66fbe4719f6ef22754ee
> compiler:       gcc (GCC) 9.0.0 20181231 (experimental)
> 
> Unfortunately, I don't have any reproducer for this crash yet.
> 
> IMPORTANT: if you fix the bug, please add the following tag to the commit:
> Reported-by: syzbot+66fbe4719f6ef22754ee@syzkaller.appspotmail.com

Most likely:

#syz fix: net/tls: fix page double free on TX cleanup

^ permalink raw reply

* Re: [PATCH net-next 0/1] net: fec: add C45 MDIO read/write support
From: Andrew Lunn @ 2019-08-19 22:54 UTC (permalink / raw)
  To: Marco Hartmann
  Cc: Andy Duan, davem@davemloft.net, netdev@vger.kernel.org,
	linux-kernel@vger.kernel.org, Christian Herber
In-Reply-To: <1566234659-7164-1-git-send-email-marco.hartmann@nxp.com>

On Mon, Aug 19, 2019 at 05:11:14PM +0000, Marco Hartmann wrote:
> As of yet, the Fast Ethernet Controller (FEC) driver only supports Clause 22
> conform MDIO transactions. IEEE 802.3ae Clause 45 defines a modified MDIO
> protocol that uses a two staged access model in order to increase the address
> space.
> 
> This patch adds support for Clause 45 conform MDIO read and write operations to
> the FEC driver.

Hi Marco

Do all versions of the FEC hardware support C45? Or do we need to make
use of the quirk support in this driver to just enable it for some
revisions of FEC?

Thanks
	Andrew

^ permalink raw reply

* Re: What to do when a bridge port gets its pvid deleted?
From: Nikolay Aleksandrov @ 2019-08-19 23:01 UTC (permalink / raw)
  To: Vladimir Oltean, Ido Schimmel
  Cc: Florian Fainelli, Roopa Prabhu, netdev, Andrew Lunn,
	Vivien Didelot, stephen
In-Reply-To: <CA+h21hrt9SXPDZq8i1=dZsa4iPHzKwzHnTGUM+ysXascUoKOpQ@mail.gmail.com>

On 8/20/19 12:10 AM, Vladimir Oltean wrote:
[snip]
> It's good to know that it's there (like you said, it explains some
> things) but I can't exactly say that removing it helps in any way.
> In fact, removing it only overwrites the dsa_8021q VLANs with 1 during
> bridge_join, while not actually doing anything upon a vlan_filtering
> toggle.
> So the sja1105 driver is in a way shielded by DSA from the bridge, for
> the better.
> It still appears to be true that the bridge doesn't think it's
> necessary to notify through SWITCHDEV_OBJ_ID_PORT_VLAN again. So my
> best bet is to restore the pvid on my own.
> However I've already stumbled upon an apparent bug while trying to do
> that. Does this look off? If it doesn't, I'll submit it as a patch:
> 
> commit 788f03991aa576fc0b4b26ca330af0f412c55582
> Author: Vladimir Oltean <olteanv@gmail.com>
> Date:   Mon Aug 19 22:57:00 2019 +0300
> 
>     net: bridge: Keep the BRIDGE_VLAN_INFO_PVID flag in net_bridge_vlan
> 
>     Currently this simplified code snippet fails:
> 
>             br_vlan_get_pvid(netdev, &pvid);
>             br_vlan_get_info(netdev, pvid, &vinfo);
>             ASSERT(!(vinfo.flags & BRIDGE_VLAN_INFO_PVID));
> 
>     It is intuitive that the pvid of a netdevice should have the
>     BRIDGE_VLAN_INFO_PVID flag set.
> 
>     However I can't seem to pinpoint a commit where this behavior was
>     introduced. It seems like it's been like that since forever.
> 
>     Signed-off-by: Vladimir Oltean <olteanv@gmail.com>
> 
> diff --git a/net/bridge/br_vlan.c b/net/bridge/br_vlan.c
> index 021cc9f66804..f49b2758bcab 100644
> --- a/net/bridge/br_vlan.c
> +++ b/net/bridge/br_vlan.c
> @@ -68,10 +68,13 @@ static bool __vlan_add_flags(struct
> net_bridge_vlan *v, u16 flags)
>      else
>          vg = nbp_vlan_group(v->port);
> 
> -    if (flags & BRIDGE_VLAN_INFO_PVID)
> +    if (flags & BRIDGE_VLAN_INFO_PVID) {
>          ret = __vlan_add_pvid(vg, v->vid);
> -    else
> +        v->flags |= BRIDGE_VLAN_INFO_PVID;
> +    } else {
>          ret = __vlan_delete_pvid(vg, v->vid);
> +        v->flags &= ~BRIDGE_VLAN_INFO_PVID;
> +    }
> 
>      if (flags & BRIDGE_VLAN_INFO_UNTAGGED)
>          v->flags |= BRIDGE_VLAN_INFO_UNTAGGED;
> 

Hi Vladimir,
I know it looks logical to have it, but there are a few reasons why we don't
do it, most importantly because we need to have only one visible pvid at any single
time, even if it's stale - it must be just one. Right now that rule will not
be violated  by your change, but people will try using this flag and could see
two pvids simultaneously. You can see that the pvid code is even using memory
barriers to propagate the new value faster and everywhere the pvid is read only once.
That is the reason the flag is set dynamically when dumping entries, too.
A second (weaker) argument against would be given the above we don't want
another way to do the same thing, specifically if it can provide us with
two pvids (e.g. if walking the vlan list) or if it can provide us with a pvid
different from the one set in the vg.
If you do need that flag, you can change br_vlan_get_info() to set the flag like
the other places do - if the vid matches the current vg pvid, or you could change
the whole pvid handling code to this new scheme as long as it can guarantee a single
pvid when walking the vlan list and fast pvid retrieval in the fast-path.

Cheers,
 Nik
 

^ permalink raw reply

* Re: [net-next v2 04/14] ice: fix set pause param autoneg check
From: Jakub Kicinski @ 2019-08-19 23:11 UTC (permalink / raw)
  To: Jeff Kirsher
  Cc: davem, Paul Greenwalt, netdev, nhorman, sassmann, Andrew Bowers
In-Reply-To: <20190819161708.3763-5-jeffrey.t.kirsher@intel.com>

On Mon, 19 Aug 2019 09:16:58 -0700, Jeff Kirsher wrote:
> +	pcaps = devm_kzalloc(&vsi->back->pdev->dev, sizeof(*pcaps),
> +			     GFP_KERNEL);
> +	if (!pcaps)
> +		return -ENOMEM;
> +
> +	/* Get current PHY config */
> +	status = ice_aq_get_phy_caps(pi, false, ICE_AQC_REPORT_SW_CFG, pcaps,
> +				     NULL);
> +	if (status) {
> +		devm_kfree(&vsi->back->pdev->dev, pcaps);
> +		return -EIO;
> +	}
> +
> +	is_an = ((pcaps->caps & ICE_AQC_PHY_AN_MODE) ?
> +			AUTONEG_ENABLE : AUTONEG_DISABLE);
> +
> +	devm_kfree(&vsi->back->pdev->dev, pcaps);

Is it just me or is this use of devm_k*alloc absolutely pointless?

^ permalink raw reply

* Re: What to do when a bridge port gets its pvid deleted?
From: Nikolay Aleksandrov @ 2019-08-19 23:12 UTC (permalink / raw)
  To: Vladimir Oltean, Ido Schimmel
  Cc: Florian Fainelli, Roopa Prabhu, netdev, Andrew Lunn,
	Vivien Didelot, stephen
In-Reply-To: <031521d2-2fb5-dd0f-2cb0-a75daa76022d@cumulusnetworks.com>

On 8/20/19 2:01 AM, Nikolay Aleksandrov wrote:
> On 8/20/19 12:10 AM, Vladimir Oltean wrote:
> [snip]
>> It's good to know that it's there (like you said, it explains some
>> things) but I can't exactly say that removing it helps in any way.
>> In fact, removing it only overwrites the dsa_8021q VLANs with 1 during
>> bridge_join, while not actually doing anything upon a vlan_filtering
>> toggle.
>> So the sja1105 driver is in a way shielded by DSA from the bridge, for
>> the better.
>> It still appears to be true that the bridge doesn't think it's
>> necessary to notify through SWITCHDEV_OBJ_ID_PORT_VLAN again. So my
>> best bet is to restore the pvid on my own.
>> However I've already stumbled upon an apparent bug while trying to do
>> that. Does this look off? If it doesn't, I'll submit it as a patch:
>>
>> commit 788f03991aa576fc0b4b26ca330af0f412c55582
>> Author: Vladimir Oltean <olteanv@gmail.com>
>> Date:   Mon Aug 19 22:57:00 2019 +0300
>>
>>     net: bridge: Keep the BRIDGE_VLAN_INFO_PVID flag in net_bridge_vlan
>>
>>     Currently this simplified code snippet fails:
>>
>>             br_vlan_get_pvid(netdev, &pvid);
>>             br_vlan_get_info(netdev, pvid, &vinfo);
>>             ASSERT(!(vinfo.flags & BRIDGE_VLAN_INFO_PVID));
>>
>>     It is intuitive that the pvid of a netdevice should have the
>>     BRIDGE_VLAN_INFO_PVID flag set.
>>
>>     However I can't seem to pinpoint a commit where this behavior was
>>     introduced. It seems like it's been like that since forever.
>>
>>     Signed-off-by: Vladimir Oltean <olteanv@gmail.com>
>>
>> diff --git a/net/bridge/br_vlan.c b/net/bridge/br_vlan.c
>> index 021cc9f66804..f49b2758bcab 100644
>> --- a/net/bridge/br_vlan.c
>> +++ b/net/bridge/br_vlan.c
>> @@ -68,10 +68,13 @@ static bool __vlan_add_flags(struct
>> net_bridge_vlan *v, u16 flags)
>>      else
>>          vg = nbp_vlan_group(v->port);
>>
>> -    if (flags & BRIDGE_VLAN_INFO_PVID)
>> +    if (flags & BRIDGE_VLAN_INFO_PVID) {
>>          ret = __vlan_add_pvid(vg, v->vid);
>> -    else
>> +        v->flags |= BRIDGE_VLAN_INFO_PVID;
>> +    } else {
>>          ret = __vlan_delete_pvid(vg, v->vid);
>> +        v->flags &= ~BRIDGE_VLAN_INFO_PVID;
>> +    }
>>
>>      if (flags & BRIDGE_VLAN_INFO_UNTAGGED)
>>          v->flags |= BRIDGE_VLAN_INFO_UNTAGGED;
>>
> 
> Hi Vladimir,
> I know it looks logical to have it, but there are a few reasons why we don't
> do it, most importantly because we need to have only one visible pvid at any single
> time, even if it's stale - it must be just one. Right now that rule will not
> be violated  by your change, but people will try using this flag and could see

Obviously, I'm talking about RCU pvid/vlan use cases similar to the dumps.
The locked cases are fine.

> two pvids simultaneously. You can see that the pvid code is even using memory
> barriers to propagate the new value faster and everywhere the pvid is read only once.
> That is the reason the flag is set dynamically when dumping entries, too.
> A second (weaker) argument against would be given the above we don't want
> another way to do the same thing, specifically if it can provide us with

i.e. I would like to avoid explaining why this shouldn't be relied upon without locking

> two pvids (e.g. if walking the vlan list) or if it can provide us with a pvid
> different from the one set in the vg.
> If you do need that flag, you can change br_vlan_get_info() to set the flag like
> the other places do - if the vid matches the current vg pvid, or you could change
> the whole pvid handling code to this new scheme as long as it can guarantee a single
> pvid when walking the vlan list and fast pvid retrieval in the fast-path.
> 
> Cheers,
>  Nik
>  
> 


^ permalink raw reply

* Re: [PATCH net-next v7 2/3] net: phy: add support for clause 37 auto-negotiation
From: René van Dorst @ 2019-08-19 23:15 UTC (permalink / raw)
  To: Tao Ren
  Cc: Andrew Lunn, Florian Fainelli, Heiner Kallweit, David S . Miller,
	Vladimir Oltean, Arun Parameswaran, Justin Chen, netdev,
	linux-kernel, openbmc
In-Reply-To: <3af5d897-7f97-a223-2d7b-56e09b83dcb5@fb.com>

Hi Tao,

Quoting Tao Ren <taoren@fb.com>:

> On 8/11/19 4:40 PM, Tao Ren wrote:
>> From: Heiner Kallweit <hkallweit1@gmail.com>
>>
>> This patch adds support for clause 37 1000Base-X auto-negotiation.
>>
>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
>> Signed-off-by: Tao Ren <taoren@fb.com>
>
> A kind reminder: could someone help to review the patch when you  
> have bandwidth?
>

FWIW: I have a similar setup with my device. MAC -> PHY -> SFP cage.
PHY is a Qualcomm at8031 and is used as a RGMII-to-SerDes converter.
SerDes only support 100Base-FX and 1000Base-X in this converter mode.
PHY also supports a RJ45 port but that is not wired on my device.

I converted [0] at803x driver to make use of the PHYLINK API for SFP cage and
also of these new c37 functions.

In autoneg on and off, it detects the link and can ping a host on the network.
Tested with 1gbit BiDi optical(1000Base-X) and RJ45 module(SGMII).
Both work and both devices detects unplug and plug-in of the cable.

output of ethtool:

Autoneg on
Settings for lan5:
         Supported ports: [ TP MII ]
         Supported link modes:   100baseT/Half 100baseT/Full
                                 1000baseT/Full
                                 1000baseX/Full
         Supported pause frame use: Symmetric Receive-only
         Supports auto-negotiation: Yes
         Supported FEC modes: Not reported
         Advertised link modes:  100baseT/Half 100baseT/Full
                                 1000baseT/Full
                                 1000baseX/Full
         Advertised pause frame use: Symmetric Receive-only
         Advertised auto-negotiation: Yes
         Advertised FEC modes: Not reported
         Link partner advertised link modes:  1000baseX/Full
         Link partner advertised pause frame use: Symmetric Receive-only
         Link partner advertised auto-negotiation: Yes
         Link partner advertised FEC modes: Not reported
         Speed: 1000Mb/s
         Duplex: Full
         Port: MII
         PHYAD: 7
         Transceiver: internal
         Auto-negotiation: on
         Supports Wake-on: g
         Wake-on: d
         Link detected: yes

Autoneg off
Settings for lan5:
         Supported ports: [ TP MII ]
         Supported link modes:   100baseT/Half 100baseT/Full
                                 1000baseT/Full
                                 1000baseX/Full
         Supported pause frame use: Symmetric Receive-only
         Supports auto-negotiation: Yes
         Supported FEC modes: Not reported
         Advertised link modes:  1000baseT/Full
         Advertised pause frame use: Symmetric Receive-only
         Advertised auto-negotiation: No
         Advertised FEC modes: Not reported
         Speed: 1000Mb/s
         Duplex: Full
         Port: MII
         PHYAD: 7
         Transceiver: internal
         Auto-negotiation: off
         Supports Wake-on: g
         Wake-on: d
         Link detected: yes

Tested-by: René van Dorst <opensource@vdorst.com>

Greats,

René

[0]  
https://github.com/vDorst/linux-1/blob/1d8cb01bc8047bda94c076676e47b09d2f31069d/drivers/net/phy/at803x.c

>
> Cheers,
>
> Tao
>
>> ---
>>  Changes in v7:
>>   - Update "if (AUTONEG_ENABLE != phydev->autoneg)" to
>>     "if (phydev->autoneg != AUTONEG_ENABLE)" so checkpatch.pl is happy.
>>  Changes in v6:
>>   - add "Signed-off-by: Tao Ren <taoren@fb.com>"
>>  Changes in v1-v5:
>>   - nothing changed. It's given v5 just to align with the version of
>>     patch series.
>>
>>  drivers/net/phy/phy_device.c | 139 +++++++++++++++++++++++++++++++++++
>>  include/linux/phy.h          |   5 ++
>>  2 files changed, 144 insertions(+)
>>
>> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
>> index 252a712d1b2b..301a794b2963 100644
>> --- a/drivers/net/phy/phy_device.c
>> +++ b/drivers/net/phy/phy_device.c
>> @@ -1617,6 +1617,40 @@ static int genphy_config_advert(struct  
>> phy_device *phydev)
>>  	return changed;
>>  }
>>
>> +/**
>> + * genphy_c37_config_advert - sanitize and advertise  
>> auto-negotiation parameters
>> + * @phydev: target phy_device struct
>> + *
>> + * Description: Writes MII_ADVERTISE with the appropriate values,
>> + *   after sanitizing the values to make sure we only advertise
>> + *   what is supported.  Returns < 0 on error, 0 if the PHY's advertisement
>> + *   hasn't changed, and > 0 if it has changed. This function is intended
>> + *   for Clause 37 1000Base-X mode.
>> + */
>> +static int genphy_c37_config_advert(struct phy_device *phydev)
>> +{
>> +	u16 adv = 0;
>> +
>> +	/* Only allow advertising what this PHY supports */
>> +	linkmode_and(phydev->advertising, phydev->advertising,
>> +		     phydev->supported);
>> +
>> +	if (linkmode_test_bit(ETHTOOL_LINK_MODE_1000baseX_Full_BIT,
>> +			      phydev->advertising))
>> +		adv |= ADVERTISE_1000XFULL;
>> +	if (linkmode_test_bit(ETHTOOL_LINK_MODE_Pause_BIT,
>> +			      phydev->advertising))
>> +		adv |= ADVERTISE_1000XPAUSE;
>> +	if (linkmode_test_bit(ETHTOOL_LINK_MODE_Asym_Pause_BIT,
>> +			      phydev->advertising))
>> +		adv |= ADVERTISE_1000XPSE_ASYM;
>> +
>> +	return phy_modify_changed(phydev, MII_ADVERTISE,
>> +				  ADVERTISE_1000XFULL | ADVERTISE_1000XPAUSE |
>> +				  ADVERTISE_1000XHALF | ADVERTISE_1000XPSE_ASYM,
>> +				  adv);
>> +}
>> +
>>  /**
>>   * genphy_config_eee_advert - disable unwanted eee mode advertisement
>>   * @phydev: target phy_device struct
>> @@ -1726,6 +1760,54 @@ int genphy_config_aneg(struct phy_device *phydev)
>>  }
>>  EXPORT_SYMBOL(genphy_config_aneg);
>>
>> +/**
>> + * genphy_c37_config_aneg - restart auto-negotiation or write BMCR
>> + * @phydev: target phy_device struct
>> + *
>> + * Description: If auto-negotiation is enabled, we configure the
>> + *   advertising, and then restart auto-negotiation.  If it is not
>> + *   enabled, then we write the BMCR. This function is intended
>> + *   for use with Clause 37 1000Base-X mode.
>> + */
>> +int genphy_c37_config_aneg(struct phy_device *phydev)
>> +{
>> +	int err, changed;
>> +
>> +	if (phydev->autoneg != AUTONEG_ENABLE)
>> +		return genphy_setup_forced(phydev);
>> +
>> +	err = phy_modify(phydev, MII_BMCR, BMCR_SPEED1000 | BMCR_SPEED100,
>> +			 BMCR_SPEED1000);
>> +	if (err)
>> +		return err;
>> +
>> +	changed = genphy_c37_config_advert(phydev);
>> +	if (changed < 0) /* error */
>> +		return changed;
>> +
>> +	if (!changed) {
>> +		/* Advertisement hasn't changed, but maybe aneg was never on to
>> +		 * begin with?  Or maybe phy was isolated?
>> +		 */
>> +		int ctl = phy_read(phydev, MII_BMCR);
>> +
>> +		if (ctl < 0)
>> +			return ctl;
>> +
>> +		if (!(ctl & BMCR_ANENABLE) || (ctl & BMCR_ISOLATE))
>> +			changed = 1; /* do restart aneg */
>> +	}
>> +
>> +	/* Only restart aneg if we are advertising something different
>> +	 * than we were before.
>> +	 */
>> +	if (changed > 0)
>> +		return genphy_restart_aneg(phydev);
>> +
>> +	return 0;
>> +}
>> +EXPORT_SYMBOL(genphy_c37_config_aneg);
>> +
>>  /**
>>   * genphy_aneg_done - return auto-negotiation status
>>   * @phydev: target phy_device struct
>> @@ -1864,6 +1946,63 @@ int genphy_read_status(struct phy_device *phydev)
>>  }
>>  EXPORT_SYMBOL(genphy_read_status);
>>
>> +/**
>> + * genphy_c37_read_status - check the link status and update  
>> current link state
>> + * @phydev: target phy_device struct
>> + *
>> + * Description: Check the link, then figure out the current state
>> + *   by comparing what we advertise with what the link partner
>> + *   advertises. This function is for Clause 37 1000Base-X mode.
>> + */
>> +int genphy_c37_read_status(struct phy_device *phydev)
>> +{
>> +	int lpa, err, old_link = phydev->link;
>> +
>> +	/* Update the link, but return if there was an error */
>> +	err = genphy_update_link(phydev);
>> +	if (err)
>> +		return err;
>> +
>> +	/* why bother the PHY if nothing can have changed */
>> +	if (phydev->autoneg == AUTONEG_ENABLE && old_link && phydev->link)
>> +		return 0;
>> +
>> +	phydev->duplex = DUPLEX_UNKNOWN;
>> +	phydev->pause = 0;
>> +	phydev->asym_pause = 0;
>> +
>> +	if (phydev->autoneg == AUTONEG_ENABLE && phydev->autoneg_complete) {
>> +		lpa = phy_read(phydev, MII_LPA);
>> +		if (lpa < 0)
>> +			return lpa;
>> +
>> +		linkmode_mod_bit(ETHTOOL_LINK_MODE_Autoneg_BIT,
>> +				 phydev->lp_advertising, lpa & LPA_LPACK);
>> +		linkmode_mod_bit(ETHTOOL_LINK_MODE_1000baseX_Full_BIT,
>> +				 phydev->lp_advertising, lpa & LPA_1000XFULL);
>> +		linkmode_mod_bit(ETHTOOL_LINK_MODE_Pause_BIT,
>> +				 phydev->lp_advertising, lpa & LPA_1000XPAUSE);
>> +		linkmode_mod_bit(ETHTOOL_LINK_MODE_Asym_Pause_BIT,
>> +				 phydev->lp_advertising,
>> +				 lpa & LPA_1000XPAUSE_ASYM);
>> +
>> +		phy_resolve_aneg_linkmode(phydev);
>> +	} else if (phydev->autoneg == AUTONEG_DISABLE) {
>> +		int bmcr = phy_read(phydev, MII_BMCR);
>> +
>> +		if (bmcr < 0)
>> +			return bmcr;
>> +
>> +		if (bmcr & BMCR_FULLDPLX)
>> +			phydev->duplex = DUPLEX_FULL;
>> +		else
>> +			phydev->duplex = DUPLEX_HALF;
>> +	}
>> +
>> +	return 0;
>> +}
>> +EXPORT_SYMBOL(genphy_c37_read_status);
>> +
>>  /**
>>   * genphy_soft_reset - software reset the PHY via BMCR_RESET bit
>>   * @phydev: target phy_device struct
>> diff --git a/include/linux/phy.h b/include/linux/phy.h
>> index 462b90b73f93..81a2921512ee 100644
>> --- a/include/linux/phy.h
>> +++ b/include/linux/phy.h
>> @@ -1077,6 +1077,11 @@ int genphy_suspend(struct phy_device *phydev);
>>  int genphy_resume(struct phy_device *phydev);
>>  int genphy_loopback(struct phy_device *phydev, bool enable);
>>  int genphy_soft_reset(struct phy_device *phydev);
>> +
>> +/* Clause 37 */
>> +int genphy_c37_config_aneg(struct phy_device *phydev);
>> +int genphy_c37_read_status(struct phy_device *phydev);
>> +
>>  static inline int genphy_no_soft_reset(struct phy_device *phydev)
>>  {
>>  	return 0;
>>




^ permalink raw reply

* Re: [net-next v2 00/14][pull request] 100GbE Intel Wired LAN Driver Updates 2019-08-19
From: Jakub Kicinski @ 2019-08-19 23:16 UTC (permalink / raw)
  To: Jeff Kirsher; +Cc: davem, netdev, nhorman, sassmann
In-Reply-To: <20190819161708.3763-1-jeffrey.t.kirsher@intel.com>

On Mon, 19 Aug 2019 09:16:54 -0700, Jeff Kirsher wrote:
> This series contains updates to ice driver only.

FWIW from API perspective this set LGTM, however, the code doesn't
always seem well thought out ;)

^ permalink raw reply

* Re: [PATCH net-next v7 2/3] net: phy: add support for clause 37 auto-negotiation
From: Tao Ren @ 2019-08-19 23:22 UTC (permalink / raw)
  To: René van Dorst
  Cc: Andrew Lunn, Florian Fainelli, Heiner Kallweit, David S . Miller,
	Vladimir Oltean, Arun Parameswaran, Justin Chen,
	netdev@vger.kernel.org, linux-kernel@vger.kernel.org,
	openbmc@lists.ozlabs.org
In-Reply-To: <20190819231526.Horde.8CjxfcGbCnfBNA-nXmq1PJt@www.vdorst.com>

On 8/19/19 4:15 PM, René van Dorst wrote:
> Hi Tao,
> 
> Quoting Tao Ren <taoren@fb.com>:
> 
>> On 8/11/19 4:40 PM, Tao Ren wrote:
>>> From: Heiner Kallweit <hkallweit1@gmail.com>
>>>
>>> This patch adds support for clause 37 1000Base-X auto-negotiation.
>>>
>>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
>>> Signed-off-by: Tao Ren <taoren@fb.com>
>>
>> A kind reminder: could someone help to review the patch when you have bandwidth?
>>
> 
> FWIW: I have a similar setup with my device. MAC -> PHY -> SFP cage.
> PHY is a Qualcomm at8031 and is used as a RGMII-to-SerDes converter.
> SerDes only support 100Base-FX and 1000Base-X in this converter mode.
> PHY also supports a RJ45 port but that is not wired on my device.
> 
> I converted [0] at803x driver to make use of the PHYLINK API for SFP cage and
> also of these new c37 functions.
> 
> In autoneg on and off, it detects the link and can ping a host on the network.
> Tested with 1gbit BiDi optical(1000Base-X) and RJ45 module(SGMII).
> Both work and both devices detects unplug and plug-in of the cable.
> 
> output of ethtool:
> 
> Autoneg on
> Settings for lan5:
>         Supported ports: [ TP MII ]
>         Supported link modes:   100baseT/Half 100baseT/Full
>                                 1000baseT/Full
>                                 1000baseX/Full
>         Supported pause frame use: Symmetric Receive-only
>         Supports auto-negotiation: Yes
>         Supported FEC modes: Not reported
>         Advertised link modes:  100baseT/Half 100baseT/Full
>                                 1000baseT/Full
>                                 1000baseX/Full
>         Advertised pause frame use: Symmetric Receive-only
>         Advertised auto-negotiation: Yes
>         Advertised FEC modes: Not reported
>         Link partner advertised link modes:  1000baseX/Full
>         Link partner advertised pause frame use: Symmetric Receive-only
>         Link partner advertised auto-negotiation: Yes
>         Link partner advertised FEC modes: Not reported
>         Speed: 1000Mb/s
>         Duplex: Full
>         Port: MII
>         PHYAD: 7
>         Transceiver: internal
>         Auto-negotiation: on
>         Supports Wake-on: g
>         Wake-on: d
>         Link detected: yes
> 
> Autoneg off
> Settings for lan5:
>         Supported ports: [ TP MII ]
>         Supported link modes:   100baseT/Half 100baseT/Full
>                                 1000baseT/Full
>                                 1000baseX/Full
>         Supported pause frame use: Symmetric Receive-only
>         Supports auto-negotiation: Yes
>         Supported FEC modes: Not reported
>         Advertised link modes:  1000baseT/Full
>         Advertised pause frame use: Symmetric Receive-only
>         Advertised auto-negotiation: No
>         Advertised FEC modes: Not reported
>         Speed: 1000Mb/s
>         Duplex: Full
>         Port: MII
>         PHYAD: 7
>         Transceiver: internal
>         Auto-negotiation: off
>         Supports Wake-on: g
>         Wake-on: d
>         Link detected: yes
> 
> Tested-by: René van Dorst <opensource@vdorst.com>
> 
> Greats,
> 
> René

Great. Thank you for the testing, René.


Cheers,

Tao

^ permalink raw reply

* Re: [PATCH v5 00/17] Use MFD framework for SGI IOC3 drivers
From: Jakub Kicinski @ 2019-08-19 23:51 UTC (permalink / raw)
  To: Thomas Bogendoerfer
  Cc: Jonathan Corbet, Ralf Baechle, Paul Burton, James Hogan,
	Dmitry Torokhov, Lee Jones, David S. Miller, Srinivas Kandagatla,
	Alessandro Zummo, Alexandre Belloni, Greg Kroah-Hartman,
	Jiri Slaby, Evgeniy Polyakov, linux-doc, linux-kernel, linux-mips,
	linux-input, netdev, linux-rtc, linux-serial
In-Reply-To: <20190819163144.3478-1-tbogendoerfer@suse.de>

On Mon, 19 Aug 2019 18:31:23 +0200, Thomas Bogendoerfer wrote:
>  - requested by Jakub I've splitted ioc3 ethernet driver changes into
>    more steps to make the transition more visible; 

Thanks a lot for doing that!

^ permalink raw reply


This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox