* Re: [Intel-wired-lan] [PATCH v2 1/1] ixgbe: sync the first fragment unconditionally
From: Alexander Duyck @ 2019-08-07 16:06 UTC (permalink / raw)
To: Maciej Fijalkowski
Cc: Firo Yang, Netdev, LKML, intel-wired-lan, Jacob Wen,
davem@davemloft.net, Alexander Duyck
In-Reply-To: <20190807160853.00001d71@gmail.com>
On Wed, Aug 7, 2019 at 7:09 AM Maciej Fijalkowski
<maciejromanfijalkowski@gmail.com> wrote:
>
> On Wed, 7 Aug 2019 08:38:43 +0000
> Firo Yang <firo.yang@suse.com> wrote:
>
> > The 08/07/2019 15:56, Jacob Wen wrote:
> > > I think the description is not correct. Consider using something like below.
> > Thank you for comments.
> >
> > >
> > > In Xen environment, due to memory fragmentation ixgbe may allocate a 'DMA'
> > > buffer with pages that are not physically contiguous.
> > Actually, I didn't look into the reason why ixgbe got a DMA buffer which
> > was mapped to Xen-swiotlb area.
>
> I think that neither of these descriptions are telling us what was truly
> broken. They lack what Alexander wrote on v1 thread of this patch.
>
> ixgbe_dma_sync_frag is called only on case when the current descriptor has EOP
> bit set, skb was already allocated and you'll be adding a current buffer as a
> frag. DMA unmapping for the first frag was intentionally skipped and we will be
> unmapping it here, in ixgbe_dma_sync_frag. As Alex said, we're using the
> DMA_ATTR_SKIP_CPU_SYNC attribute which obliges us to perform a sync manually
> and it was missing.
>
> So IMHO the commit description should include descriptions from both xen and
> ixgbe worlds, the v2 lacks info about ixgbe case.
>
> BTW Alex, what was the initial reason for holding off with unmapping the first
> buffer? Asking because IIRC the i40e and ice behave a bit different here. We
> don't look there for EOP at all when building/constructing skb and not delaying
> the unmap of non-eop buffers.
>
> Thanks,
> Maciej
The reason why we have to hold off on unmapping the first buffer is
because in the case of Receive Side Coalescing (RSC), also known as Large
Receive Offload (LRO), the header of the packet is updated for each
additional frame that is added. As such you can end up with the device
writing data, header, data, header, data, header where each data write
would update a new descriptor, but the header will only ever update the
first descriptor in the chain. As such if we unmapped it earlier it would
result in an IOMMU fault because the device would be rewriting the header
after it had been unmapped.
The devices supported by the ixgbe driver are the only ones that have
RSC/LRO support. As such this behavior is present for ixgbe, but not for
other Intel NIC drivers including igb, igbvf, ixgbevf, i40e, etc.
- Alex
^ permalink raw reply
* Re: [PATCH v2 0/3] arm/arm64: Add support for function error injection
From: Will Deacon @ 2019-08-07 16:07 UTC (permalink / raw)
To: Leo Yan
Cc: Russell King, Oleg Nesterov, Catalin Marinas,
Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman,
Thomas Gleixner, Ingo Molnar, Borislav Petkov, H. Peter Anvin,
x86, Arnd Bergmann, Alexei Starovoitov, Daniel Borkmann,
Martin KaFai Lau, Song Liu, Yonghong Song, Naveen N. Rao,
linux-arm-kernel, linux-kernel, linuxppc-dev, linux-arch, netdev,
bpf, clang-built-linux, Masami Hiramatsu
In-Reply-To: <20190806100015.11256-1-leo.yan@linaro.org>
On Tue, Aug 06, 2019 at 06:00:12PM +0800, Leo Yan wrote:
> This small patch set is to add support for function error injection;
> this can be used to eanble more advanced debugging feature, e.g.
> CONFIG_BPF_KPROBE_OVERRIDE.
>
> The patch 01/03 is to consolidate the function definition which can be
> suared cross architectures, patches 02,03/03 are used for enabling
> function error injection on arm64 and arm architecture respectively.
>
> I tested on arm64 platform Juno-r2 and one of my laptop with x86
> architecture with below steps; I don't test for Arm architecture so
> only pass compilation.
Thanks. I've queued the first two patches up here:
https://git.kernel.org/pub/scm/linux/kernel/git/arm64/linux.git/log/?h=for-next/error-injection
Will
^ permalink raw reply
* Re: [PATCH net] net: phy: rtl8211f: do a double read to get real time link status
From: Heiner Kallweit @ 2019-08-07 16:47 UTC (permalink / raw)
To: Yonglong Liu, davem, andrew
Cc: netdev, linux-kernel, linuxarm, salil.mehta, yisen.zhuang,
shiju.jose
In-Reply-To: <1565183772-44268-1-git-send-email-liuyonglong@huawei.com>
On 07.08.2019 15:16, Yonglong Liu wrote:
> [ 27.232781] hns3 0000:bd:00.3 eth7: net open
> [ 27.237303] 8021q: adding VLAN 0 to HW filter on device eth7
> [ 27.242972] IPv6: ADDRCONF(NETDEV_CHANGE): eth7: link becomes ready
> [ 27.244449] hns3 0000:bd:00.3: invalid speed (-1)
> [ 27.253904] hns3 0000:bd:00.3 eth7: failed to adjust link.
> [ 27.259379] RTL8211F Gigabit Ethernet mii-0000:bd:00.3:07: PHY state change UP -> RUNNING
> [ 27.924903] hns3 0000:bd:00.3 eth7: link up
> [ 28.280479] RTL8211F Gigabit Ethernet mii-0000:bd:00.3:07: PHY state change RUNNING -> NOLINK
> [ 29.208452] hns3 0000:bd:00.3 eth7: link down
> [ 32.376745] RTL8211F Gigabit Ethernet mii-0000:bd:00.3:07: PHY state change NOLINK -> RUNNING
> [ 33.208448] hns3 0000:bd:00.3 eth7: link up
> [ 35.253821] hns3 0000:bd:00.3 eth7: net stop
> [ 35.258270] hns3 0000:bd:00.3 eth7: link down
>
> When using rtl8211f in polling mode, may get a invalid speed,
> because of reading a fake link up and autoneg complete status
> immediately after starting autoneg:
>
> ifconfig-1176 [007] .... 27.232763: mdio_access: mii-0000:bd:00.3 read phy:0x07 reg:0x00 val:0x1040
> kworker/u257:1-670 [015] .... 27.232805: mdio_access: mii-0000:bd:00.3 read phy:0x07 reg:0x04 val:0x01e1
> kworker/u257:1-670 [015] .... 27.232815: mdio_access: mii-0000:bd:00.3 write phy:0x07 reg:0x04 val:0x05e1
> kworker/u257:1-670 [015] .... 27.232869: mdio_access: mii-0000:bd:00.3 read phy:0x07 reg:0x01 val:0x79ad
> kworker/u257:1-670 [015] .... 27.232904: mdio_access: mii-0000:bd:00.3 read phy:0x07 reg:0x09 val:0x0200
> kworker/u257:1-670 [015] .... 27.232940: mdio_access: mii-0000:bd:00.3 read phy:0x07 reg:0x00 val:0x1040
> kworker/u257:1-670 [015] .... 27.232949: mdio_access: mii-0000:bd:00.3 write phy:0x07 reg:0x00 val:0x1240
> kworker/u257:1-670 [015] .... 27.233003: mdio_access: mii-0000:bd:00.3 read phy:0x07 reg:0x01 val:0x79ad
> kworker/u257:1-670 [015] .... 27.233039: mdio_access: mii-0000:bd:00.3 read phy:0x07 reg:0x0a val:0x3002
> kworker/u257:1-670 [015] .... 27.233074: mdio_access: mii-0000:bd:00.3 read phy:0x07 reg:0x09 val:0x0200
> kworker/u257:1-670 [015] .... 27.233110: mdio_access: mii-0000:bd:00.3 read phy:0x07 reg:0x05 val:0x0000
> kworker/u257:1-670 [000] .... 28.280475: mdio_access: mii-0000:bd:00.3 read phy:0x07 reg:0x01 val:0x7989
> kworker/u257:1-670 [000] .... 29.304471: mdio_access: mii-0000:bd:00.3 read phy:0x07 reg:0x01 val:0x7989
>
> According to the datasheet of rtl8211f, to get the real time
> link status, need to read MII_BMSR twice.
>
> This patch add a read_status hook for rtl8211f, and do a fake
> phy_read before genphy_read_status(), so that can get real link
> status in genphy_read_status().
>
> Signed-off-by: Yonglong Liu <liuyonglong@huawei.com>
> ---
> drivers/net/phy/realtek.c | 13 +++++++++++++
> 1 file changed, 13 insertions(+)
>
Is this an accidental resubmit? Because we discussed this in
https://marc.info/?t=156413509900003&r=1&w=2 and a fix has
been applied already.
Heiner
^ permalink raw reply
* Re: [v3,1/4] tools: bpftool: add net attach command to attach XDP on interface
From: Y Song @ 2019-08-07 16:56 UTC (permalink / raw)
To: Daniel T. Lee; +Cc: Daniel Borkmann, Alexei Starovoitov, netdev
In-Reply-To: <20190807022509.4214-2-danieltimlee@gmail.com>
On Tue, Aug 6, 2019 at 7:25 PM Daniel T. Lee <danieltimlee@gmail.com> wrote:
>
> By this commit, using `bpftool net attach`, user can attach XDP prog on
> interface. New type of enum 'net_attach_type' has been made, as stated at
> cover-letter, the meaning of 'attach' is, prog will be attached on interface.
>
> With 'overwrite' option at argument, attached XDP program could be replaced.
> Added new helper 'net_parse_dev' to parse the network device at argument.
>
> BPF prog will be attached through libbpf 'bpf_set_link_xdp_fd'.
>
> Signed-off-by: Daniel T. Lee <danieltimlee@gmail.com>
Acked-by: Yonghong Song <yhs@fb.com>
^ permalink raw reply
* Re: [PATCH net v2] net/tls: prevent skb_orphan() from leaking TLS plain text with offload
From: Willem de Bruijn @ 2019-08-07 16:59 UTC (permalink / raw)
To: Jakub Kicinski
Cc: David Miller, Network Development, davejwatson, borisp, aviadye,
John Fastabend, Daniel Borkmann, Eric Dumazet, Alexei Starovoitov,
oss-drivers
In-Reply-To: <20190807060612.19397-1-jakub.kicinski@netronome.com>
On Wed, Aug 7, 2019 at 2:06 AM Jakub Kicinski
<jakub.kicinski@netronome.com> wrote:
>
> sk_validate_xmit_skb() and drivers depend on the sk member of
> struct sk_buff to identify segments requiring encryption.
> Any operation which removes or does not preserve the original TLS
> socket such as skb_orphan() or skb_clone() will cause clear text
> leaks.
>
> Make the TCP socket underlying an offloaded TLS connection
> mark all skbs as decrypted, if TLS TX is in offload mode.
> Then in sk_validate_xmit_skb() catch skbs which have no socket
> (or a socket with no validation) and decrypted flag set.
>
> Note that CONFIG_SOCK_VALIDATE_XMIT, CONFIG_TLS_DEVICE and
> sk->sk_validate_xmit_skb are slightly interchangeable right now,
> they all imply TLS offload. The new checks are guarded by
> CONFIG_TLS_DEVICE because that's the option guarding the
> sk_buff->decrypted member.
>
> Second, smaller issue with orphaning is that it breaks
> the guarantee that packets will be delivered to device
> queues in-order. All TLS offload drivers depend on that
> scheduling property. This means skb_orphan_partial()'s
> trick of preserving partial socket references will cause
> issues in the drivers. We need a full orphan, and as a
> result netem delay/throttling will cause all TLS offload
> skbs to be dropped.
>
> Reusing the sk_buff->decrypted flag also protects from
> leaking clear text when incoming, decrypted skb is redirected
> (e.g. by TC).
>
> See commit 0608c69c9a80 ("bpf: sk_msg, sock{map|hash} redirect
> through ULP") for justification why the internal flag is safe.
>
> v2:
> - remove superfluous decrypted mark copy (Willem);
> - remove the stale doc entry (Boris);
> - rely entirely on EOR marking to prevent coalescing (Boris);
> - use an internal sendpages flag instead of marking the socket
> (Boris).
>
> Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
> ---
> Documentation/networking/tls-offload.rst | 18 ------------------
> include/linux/skbuff.h | 8 ++++++++
> include/linux/socket.h | 3 +++
> include/net/sock.h | 10 +++++++++-
> net/core/sock.c | 20 +++++++++++++++-----
> net/ipv4/tcp.c | 3 +++
> net/ipv4/tcp_output.c | 3 +++
> net/tls/tls_device.c | 9 +++++++--
> 8 files changed, 48 insertions(+), 26 deletions(-)
> diff --git a/net/core/sock.c b/net/core/sock.c
> index d57b0cc995a0..0f9619b0892f 100644
> --- a/net/core/sock.c
> +++ b/net/core/sock.c
> @@ -1992,6 +1992,20 @@ void skb_set_owner_w(struct sk_buff *skb, struct sock *sk)
> }
> EXPORT_SYMBOL(skb_set_owner_w);
>
> +static bool can_skb_orphan_partial(const struct sk_buff *skb)
> +{
> +#ifdef CONFIG_TLS_DEVICE
> + /* Drivers depend on in-order delivery for crypto offload,
> + * partial orphan breaks out-of-order-OK logic.
> + */
> + if (skb->decrypted)
> + return false;
> +#endif
> + return (IS_ENABLED(CONFIG_INET) &&
> + skb->destructor == tcp_wfree) ||
Please add parentheses around IS_ENABLED(CONFIG_INET) &&
skb->destructor == tcp_wfree
I was also surprised that this works when tcp_wfree is not defined if
!CONFIG_INET. But apparently it does (at -O2?) :)
> @@ -984,6 +984,9 @@ ssize_t do_tcp_sendpages(struct sock *sk, struct page *page, int offset,
> if (!skb)
> goto wait_for_memory;
>
> +#ifdef CONFIG_TLS_DEVICE
> + skb->decrypted = !!(flags & MSG_SENDPAGE_DECRYPTED);
> +#endif
Nothing is stopping userspace from passing this new flag. In send
(tcp_sendmsg_locked) it is ignored. But can it reach do_tcp_sendpages
through tcp_bpf_sendmsg?
> skb_entail(sk, skb);
> copy = size_goal;
> }
> diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
> index 6e4afc48d7bb..979520e46e33 100644
> --- a/net/ipv4/tcp_output.c
> +++ b/net/ipv4/tcp_output.c
> @@ -1320,6 +1320,7 @@ int tcp_fragment(struct sock *sk, enum tcp_queue tcp_queue,
> buff = sk_stream_alloc_skb(sk, nsize, gfp, true);
> if (!buff)
> return -ENOMEM; /* We'll just try again later. */
> + skb_copy_decrypted(buff, skb);
This code has to copy timestamps, tx_flags, zerocopy state and now
this in three locations. Eventually we'll want a single helper for all
of them..
^ permalink raw reply
* Re: [v3,2/4] tools: bpftool: add net detach command to detach XDP on interface
From: Y Song @ 2019-08-07 17:02 UTC (permalink / raw)
To: Daniel T. Lee; +Cc: Daniel Borkmann, Alexei Starovoitov, netdev
In-Reply-To: <20190807022509.4214-3-danieltimlee@gmail.com>
On Tue, Aug 6, 2019 at 7:25 PM Daniel T. Lee <danieltimlee@gmail.com> wrote:
>
> By this commit, using `bpftool net detach`, the attached XDP prog can
> be detached. Detaching the BPF prog will be done through libbpf
> 'bpf_set_link_xdp_fd' with the progfd set to -1.
>
> Signed-off-by: Daniel T. Lee <danieltimlee@gmail.com>
> ---
> tools/bpf/bpftool/net.c | 42 ++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 41 insertions(+), 1 deletion(-)
>
> diff --git a/tools/bpf/bpftool/net.c b/tools/bpf/bpftool/net.c
> index c05a3fac5cac..7be96acb08e0 100644
> --- a/tools/bpf/bpftool/net.c
> +++ b/tools/bpf/bpftool/net.c
> @@ -343,6 +343,43 @@ static int do_attach(int argc, char **argv)
> return 0;
> }
>
> +static int do_detach(int argc, char **argv)
> +{
> + enum net_attach_type attach_type;
> + int progfd, ifindex, err = 0;
> +
> + /* parse detach args */
> + if (!REQ_ARGS(3))
> + return -EINVAL;
> +
> + attach_type = parse_attach_type(*argv);
> + if (attach_type == max_net_attach_type) {
> + p_err("invalid net attach/detach type");
> + return -EINVAL;
> + }
> +
> + NEXT_ARG();
> + ifindex = net_parse_dev(&argc, &argv);
> + if (ifindex < 1)
> + return -EINVAL;
> +
> + /* detach xdp prog */
> + progfd = -1;
> + if (is_prefix("xdp", attach_type_strings[attach_type]))
> + err = do_attach_detach_xdp(progfd, attach_type, ifindex, NULL);
I found an issue here. This is probably related to do_attach_detach_xdp.
-bash-4.4$ sudo ./bpftool net attach x pinned /sys/fs/bpf/xdp_example dev v1
-bash-4.4$ sudo ./bpftool net
xdp:
v1(4) driver id 1172
tc:
eth0(2) clsact/ingress fbflow_icmp id 29 act []
eth0(2) clsact/egress cls_fg_dscp_section id 27 act []
eth0(2) clsact/egress fbflow_egress id 28
eth0(2) clsact/egress fbflow_sslwall_egress id 35
flow_dissector:
-bash-4.4$ sudo ./bpftool net detach x dev v2
-bash-4.4$ sudo ./bpftool net
xdp:
v1(4) driver id 1172
tc:
eth0(2) clsact/ingress fbflow_icmp id 29 act []
eth0(2) clsact/egress cls_fg_dscp_section id 27 act []
eth0(2) clsact/egress fbflow_egress id 28
eth0(2) clsact/egress fbflow_sslwall_egress id 35
flow_dissector:
-bash-4.4$
Basically detaching may fail due to wrong dev name or wrong type, etc.
But the tool did not return an error. Is this expected?
This may be related to this funciton "bpf_set_link_xdp_fd()".
So this patch itself should be okay.
> +
> + if (err < 0) {
> + p_err("interface %s detach failed",
> + attach_type_strings[attach_type]);
> + return err;
> + }
> +
> + if (json_output)
> + jsonw_null(json_wtr);
> +
> + return 0;
> +}
> +
> static int do_show(int argc, char **argv)
> {
> struct bpf_attach_info attach_info = {};
> @@ -419,6 +456,7 @@ static int do_help(int argc, char **argv)
> fprintf(stderr,
> "Usage: %s %s { show | list } [dev <devname>]\n"
> " %s %s attach ATTACH_TYPE PROG dev <devname> [ overwrite ]\n"
> + " %s %s detach ATTACH_TYPE dev <devname>\n"
> " %s %s help\n"
> "\n"
> " " HELP_SPEC_PROGRAM "\n"
> @@ -429,7 +467,8 @@ static int do_help(int argc, char **argv)
> " to dump program attachments. For program types\n"
> " sk_{filter,skb,msg,reuseport} and lwt/seg6, please\n"
> " consult iproute2.\n",
> - bin_name, argv[-2], bin_name, argv[-2], bin_name, argv[-2]);
> + bin_name, argv[-2], bin_name, argv[-2], bin_name, argv[-2],
> + bin_name, argv[-2]);
>
> return 0;
> }
> @@ -438,6 +477,7 @@ static const struct cmd cmds[] = {
> { "show", do_show },
> { "list", do_show },
> { "attach", do_attach },
> + { "detach", do_detach },
> { "help", do_help },
> { 0 }
> };
> --
> 2.20.1
>
^ permalink raw reply
* [PATCH v5 0/4] net: phy: Add support for DT configuration of PHY LEDs and use it for RTL8211E
From: Matthias Kaehlcke @ 2019-08-07 17:04 UTC (permalink / raw)
To: David S . Miller, Rob Herring, Mark Rutland, Andrew Lunn,
Florian Fainelli, Heiner Kallweit
Cc: netdev, devicetree, linux-kernel, Douglas Anderson,
Matthias Kaehlcke
This series adds a generic binding to configure PHY LEDs through
the device tree, and phylib support for reading the information
from the DT. PHY drivers that support the generic binding should
implement the new hook .config_led.
Enable DT configuration of the RTL8211E LEDs by implementing the
.config_led hook of the driver. Certain registers of the RTL8211E
can only be accessed through a vendor specific extended page
mechanism. Extended pages need to be accessed for the LED
configuration. This series adds helpers to facilitate accessing
extended pages.
(subject updated, was "net: phy: realtek: Enable configuration
of RTL8211E LEDs")
Matthias Kaehlcke (4):
dt-bindings: net: phy: Add subnode for LED configuration
net: phy: Add support for generic LED configuration through the DT
net: phy: realtek: Add helpers for accessing RTL8211x extension pages
net: phy: realtek: Add LED configuration support for RTL8211E
.../devicetree/bindings/net/ethernet-phy.yaml | 59 +++++++
drivers/net/phy/phy_device.c | 72 +++++++++
drivers/net/phy/realtek.c | 148 ++++++++++++++++--
include/linux/phy.h | 22 +++
4 files changed, 286 insertions(+), 15 deletions(-)
--
2.22.0.770.g0f2c4a37fd-goog
^ permalink raw reply
* [PATCH v5 1/4] dt-bindings: net: phy: Add subnode for LED configuration
From: Matthias Kaehlcke @ 2019-08-07 17:04 UTC (permalink / raw)
To: David S . Miller, Rob Herring, Mark Rutland, Andrew Lunn,
Florian Fainelli, Heiner Kallweit
Cc: netdev, devicetree, linux-kernel, Douglas Anderson,
Matthias Kaehlcke
In-Reply-To: <20190807170449.205378-1-mka@chromium.org>
The LED behavior of some Ethernet PHYs is configurable. Add an
optional 'leds' subnode with a child node for each LED to be
configured. The binding aims to be compatible with the common
LED binding (see devicetree/bindings/leds/common.txt).
A LED can be configured to be:
- 'on' when a link is active, some PHYs allow configuration for
certain link speeds
speeds
- 'off'
- blink on RX/TX activity, some PHYs allow configuration for
certain link speeds
For the configuration to be effective it needs to be supported by
the hardware and the corresponding PHY driver.
Suggested-by: Andrew Lunn <andrew@lunn.ch>
Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
---
Changes in v5:
- renamed triggers from 'phy_link_<speed>_active' to 'phy-link-<speed>'
- added entries for 'phy-link-<speed>-activity'
- added 'phy-link' and 'phy-link-activity' for triggers with any link
speed
- added entry for trigger 'none'
Changes in v4:
- patch added to the series
---
.../devicetree/bindings/net/ethernet-phy.yaml | 59 +++++++++++++++++++
1 file changed, 59 insertions(+)
diff --git a/Documentation/devicetree/bindings/net/ethernet-phy.yaml b/Documentation/devicetree/bindings/net/ethernet-phy.yaml
index f70f18ff821f..98ba320f828b 100644
--- a/Documentation/devicetree/bindings/net/ethernet-phy.yaml
+++ b/Documentation/devicetree/bindings/net/ethernet-phy.yaml
@@ -153,6 +153,50 @@ properties:
Delay after the reset was deasserted in microseconds. If
this property is missing the delay will be skipped.
+patternProperties:
+ "^leds$":
+ type: object
+ description:
+ Subnode with configuration of the PHY LEDs.
+
+ patternProperties:
+ "^led@[0-9]+$":
+ type: object
+ description:
+ Subnode with the configuration of a single PHY LED.
+
+ properties:
+ reg:
+ description:
+ The ID number of the LED, typically corresponds to a hardware ID.
+ $ref: "/schemas/types.yaml#/definitions/uint32"
+
+ linux,default-trigger:
+ description:
+ This parameter, if present, is a string specifying the trigger
+ assigned to the LED. Supported triggers are:
+ "none" - LED will be solid off
+ "phy-link" - LED will be solid on when a link is active
+ "phy-link-10m" - LED will be solid on when a 10Mb/s link is active
+ "phy-link-100m" - LED will be solid on when a 100Mb/s link is active
+ "phy-link-1g" - LED will be solid on when a 1Gb/s link is active
+ "phy-link-10g" - LED will be solid on when a 10Gb/s link is active
+ "phy-link-activity" - LED will be on when link is active and blink
+ off with activity.
+ "phy-link-10m-activity" - LED will be on when 10Mb/s link is active
+ and blink off with activity.
+ "phy-link-100m-activity" - LED will be on when 100Mb/s link is
+ active and blink off with activity.
+ "phy-link-1g-activity" - LED will be on when 1Gb/s link is active
+ and blink off with activity.
+ "phy-link-10g-activity" - LED will be on when 10Gb/s link is active
+ and blink off with activity.
+
+ $ref: "/schemas/types.yaml#/definitions/string"
+
+ required:
+ - reg
+
required:
- reg
@@ -173,5 +217,20 @@ examples:
reset-gpios = <&gpio1 4 1>;
reset-assert-us = <1000>;
reset-deassert-us = <2000>;
+
+ leds {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ led@0 {
+ reg = <0>;
+ linux,default-trigger = "phy-link-1g";
+ };
+
+ led@1 {
+ reg = <1>;
+ linux,default-trigger = "phy-link-100m-activity";
+ };
+ };
};
};
--
2.22.0.770.g0f2c4a37fd-goog
^ permalink raw reply related
* [PATCH v5 4/4] net: phy: realtek: Add LED configuration support for RTL8211E
From: Matthias Kaehlcke @ 2019-08-07 17:04 UTC (permalink / raw)
To: David S . Miller, Rob Herring, Mark Rutland, Andrew Lunn,
Florian Fainelli, Heiner Kallweit
Cc: netdev, devicetree, linux-kernel, Douglas Anderson,
Matthias Kaehlcke
In-Reply-To: <20190807170449.205378-1-mka@chromium.org>
Add a .config_led hook which is called by the PHY core when
configuration data for a PHY LED is available. Each LED can be
configured to be solid 'off, solid 'on' for certain (or all)
link speeds or to blink on RX/TX activity.
Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
---
Changes in v5:
- use 'config_leds' driver callback instead of requesting the DT
configuration
- added support for trigger 'none'
- always disable EEE LED mode when a LED is configured. We have no
device data struct to keep track of its state, the number of LEDs
is limited, so the overhead of disabling it multiple times (once for
each LED that is configured) during initialization is negligible
- print warning when disabling EEE LED mode fails
- updated commit message (previous subject was 'net: phy: realtek:
configure RTL8211E LEDs')
Changes in v4:
- use the generic PHY LED binding
- keep default/current configuration if none is specified
- added rtl8211e_disable_eee_led_mode()
- was previously in separate patch, however since we always want to
disable EEE LED mode when a LED configuration is specified it makes
sense to just add the function here.
- don't call phy_restore_page() in rtl8211e_config_leds() if
selection of the extended page failed.
- use phydev_warn() instead of phydev_err() if LED configuration
fails since we don't bail out
- use hex number to specify page for consistency
- add hex number to comment about ext page 44 to facilitate searching
Changes in v3:
- sanity check led-modes values
- set LACR bits in a more readable way
- use phydev_err() instead of dev_err()
- log an error if LED configuration fails
Changes in v2:
- patch added to the series
---
drivers/net/phy/realtek.c | 101 +++++++++++++++++++++++++++++++++++++-
1 file changed, 100 insertions(+), 1 deletion(-)
diff --git a/drivers/net/phy/realtek.c b/drivers/net/phy/realtek.c
index a5b3708dc4d8..5064ad732443 100644
--- a/drivers/net/phy/realtek.c
+++ b/drivers/net/phy/realtek.c
@@ -9,8 +9,9 @@
* Copyright (c) 2004 Freescale Semiconductor, Inc.
*/
#include <linux/bitops.h>
-#include <linux/phy.h>
+#include <linux/bits.h>
#include <linux/module.h>
+#include <linux/phy.h>
#define RTL821x_PHYSR 0x11
#define RTL821x_PHYSR_DUPLEX BIT(13)
@@ -26,6 +27,18 @@
#define RTL821x_EXT_PAGE_SELECT 0x1e
#define RTL821x_PAGE_SELECT 0x1f
+/* RTL8211E page 5 */
+#define RTL8211E_EEE_LED_MODE1 0x05
+#define RTL8211E_EEE_LED_MODE2 0x06
+
+/* RTL8211E extension page 44 (0x2c) */
+#define RTL8211E_LACR 0x1a
+#define RLT8211E_LACR_LEDACTCTRL_SHIFT 4
+#define RTL8211E_LCR 0x1c
+
+#define LACR_MASK(led) BIT(4 + led)
+#define LCR_MASK(led) GENMASK((led * 4) + 2, led * 4)
+
#define RTL8211F_INSR 0x1d
#define RTL8211F_TX_DELAY BIT(8)
@@ -83,6 +96,91 @@ static int rtl8211x_modify_ext_paged(struct phy_device *phydev, int page,
return phy_restore_page(phydev, oldpage, ret);
}
+static void rtl8211e_disable_eee_led_mode(struct phy_device *phydev)
+{
+ int oldpage;
+ int err = 0;
+
+ oldpage = phy_select_page(phydev, 5);
+ if (oldpage < 0)
+ goto out;
+
+ /* write magic values to disable EEE LED mode */
+ err = __phy_write(phydev, RTL8211E_EEE_LED_MODE1, 0x8b82);
+ if (err)
+ goto out;
+
+ err = __phy_write(phydev, RTL8211E_EEE_LED_MODE2, 0x052b);
+
+out:
+ if (err)
+ phydev_warn(phydev, "failed to disable EEE LED mode: %d\n", err);
+
+ phy_restore_page(phydev, oldpage, err);
+}
+
+static int rtl8211e_config_led(struct phy_device *phydev, int led,
+ struct phy_led_config *cfg)
+{
+ u16 lacr_bits = 0, lcr_bits = 0;
+ int oldpage, ret;
+
+ switch (cfg->trigger.t) {
+ case PHY_LED_TRIGGER_LINK:
+ lcr_bits = 7 << (led * 4);
+ break;
+
+ case PHY_LED_TRIGGER_LINK_10M:
+ lcr_bits = 1 << (led * 4);
+ break;
+
+ case PHY_LED_TRIGGER_LINK_100M:
+ lcr_bits = 2 << (led * 4);
+ break;
+
+ case PHY_LED_TRIGGER_LINK_1G:
+ lcr_bits |= 4 << (led * 4);
+ break;
+
+ case PHY_LED_TRIGGER_NONE:
+ break;
+
+ default:
+ phydev_warn(phydev,
+ "unknown trigger for LED%d: %d\n",
+ led, cfg->trigger.t);
+ return -EINVAL;
+ }
+
+ if (cfg->trigger.activity)
+ lacr_bits = BIT(RLT8211E_LACR_LEDACTCTRL_SHIFT + led);
+
+ rtl8211e_disable_eee_led_mode(phydev);
+
+ oldpage = rtl8211x_select_ext_page(phydev, 0x2c);
+ if (oldpage < 0) {
+ phydev_err(phydev, "failed to select extended page: %d\n", oldpage);
+ return oldpage;
+ }
+
+ ret = __phy_modify(phydev, RTL8211E_LACR,
+ LACR_MASK(led), lacr_bits);
+ if (ret) {
+ phydev_err(phydev, "failed to write LACR reg: %d\n",
+ ret);
+ goto err;
+ }
+
+ ret = __phy_modify(phydev, RTL8211E_LCR,
+ LCR_MASK(led), lcr_bits);
+ if (ret)
+ phydev_err(phydev, "failed to write LCR reg: %d\n",
+ ret);
+
+err:
+ return phy_restore_page(phydev, oldpage, ret);
+}
+
static int rtl8201_ack_interrupt(struct phy_device *phydev)
{
int err;
@@ -330,6 +428,7 @@ static struct phy_driver realtek_drvs[] = {
.config_init = &rtl8211e_config_init,
.ack_interrupt = &rtl821x_ack_interrupt,
.config_intr = &rtl8211e_config_intr,
+ .config_led = &rtl8211e_config_led,
.suspend = genphy_suspend,
.resume = genphy_resume,
.read_page = rtl821x_read_page,
--
2.22.0.770.g0f2c4a37fd-goog
^ permalink raw reply related
* [PATCH v5 3/4] net: phy: realtek: Add helpers for accessing RTL8211x extension pages
From: Matthias Kaehlcke @ 2019-08-07 17:04 UTC (permalink / raw)
To: David S . Miller, Rob Herring, Mark Rutland, Andrew Lunn,
Florian Fainelli, Heiner Kallweit
Cc: netdev, devicetree, linux-kernel, Douglas Anderson,
Matthias Kaehlcke
In-Reply-To: <20190807170449.205378-1-mka@chromium.org>
Some RTL8211x PHYs have extension pages, which can be accessed
after selecting a page through a custom method. Add a function to
modify bits in a register of an extension page and a helper for
selecting an ext page. Use rtl8211x_modify_ext_paged() in
rtl8211e_config_init() instead of doing things 'manually'.
Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
---
Changes in v5:
- renamed 'rtl8211e_<action>_ext_page' to 'rtl8211x_<action>_ext_page'
- updated commit message
Changes in v4:
- don't add constant RTL8211E_EXT_PAGE, it's only used once,
use a literal instead
- pass 'oldpage' to phy_restore_page() in rtl8211e_select_ext_page(),
not 'page'
- return 'oldpage' in rtl8211e_select_ext_page()
- use __phy_modify() in rtl8211e_modify_ext_paged() instead of
reimplementing __phy_modify_changed()
- in rtl8211e_modify_ext_paged() return directly when
rtl8211e_select_ext_page() fails
Changes in v3:
- use the new function in rtl8211e_config_init() instead of
doing things 'manually'
- use existing RTL8211E_EXT_PAGE instead of adding a new define
- updated commit message
Changes in v2:
- use phy_select_page() and phy_restore_page(), get rid of
rtl8211e_restore_page()
- s/rtl821e_select_ext_page/rtl8211e_select_ext_page/
- updated commit message
---
drivers/net/phy/realtek.c | 47 +++++++++++++++++++++++++++------------
1 file changed, 33 insertions(+), 14 deletions(-)
diff --git a/drivers/net/phy/realtek.c b/drivers/net/phy/realtek.c
index a669945eb829..a5b3708dc4d8 100644
--- a/drivers/net/phy/realtek.c
+++ b/drivers/net/phy/realtek.c
@@ -53,6 +53,36 @@ static int rtl821x_write_page(struct phy_device *phydev, int page)
return __phy_write(phydev, RTL821x_PAGE_SELECT, page);
}
+static int rtl8211x_select_ext_page(struct phy_device *phydev, int page)
+{
+ int ret, oldpage;
+
+ oldpage = phy_select_page(phydev, 7);
+ if (oldpage < 0)
+ return oldpage;
+
+ ret = __phy_write(phydev, RTL821x_EXT_PAGE_SELECT, page);
+ if (ret)
+ return phy_restore_page(phydev, oldpage, ret);
+
+ return oldpage;
+}
+
+static int rtl8211x_modify_ext_paged(struct phy_device *phydev, int page,
+ u32 regnum, u16 mask, u16 set)
+{
+ int ret = 0;
+ int oldpage;
+
+ oldpage = rtl8211x_select_ext_page(phydev, page);
+ if (oldpage < 0)
+ return oldpage;
+
+ ret = __phy_modify(phydev, regnum, mask, set);
+
+ return phy_restore_page(phydev, oldpage, ret);
+}
+
static int rtl8201_ack_interrupt(struct phy_device *phydev)
{
int err;
@@ -184,7 +214,6 @@ static int rtl8211f_config_init(struct phy_device *phydev)
static int rtl8211e_config_init(struct phy_device *phydev)
{
- int ret = 0, oldpage;
u16 val;
/* enable TX/RX delay for rgmii-* modes, and disable them for rgmii. */
@@ -213,19 +242,9 @@ static int rtl8211e_config_init(struct phy_device *phydev)
* 2 = RX Delay, 1 = TX Delay, 0 = SELRGV (see original PHY datasheet
* for details).
*/
- oldpage = phy_select_page(phydev, 0x7);
- if (oldpage < 0)
- goto err_restore_page;
-
- ret = __phy_write(phydev, RTL821x_EXT_PAGE_SELECT, 0xa4);
- if (ret)
- goto err_restore_page;
-
- ret = __phy_modify(phydev, 0x1c, RTL8211E_TX_DELAY | RTL8211E_RX_DELAY,
- val);
-
-err_restore_page:
- return phy_restore_page(phydev, oldpage, ret);
+ return rtl8211x_modify_ext_paged(phydev, 0xa4, 0x1c,
+ RTL8211E_TX_DELAY | RTL8211E_RX_DELAY,
+ val);
}
static int rtl8211b_suspend(struct phy_device *phydev)
--
2.22.0.770.g0f2c4a37fd-goog
^ permalink raw reply related
* [PATCH v5 2/4] net: phy: Add support for generic LED configuration through the DT
From: Matthias Kaehlcke @ 2019-08-07 17:04 UTC (permalink / raw)
To: David S . Miller, Rob Herring, Mark Rutland, Andrew Lunn,
Florian Fainelli, Heiner Kallweit
Cc: netdev, devicetree, linux-kernel, Douglas Anderson,
Matthias Kaehlcke
In-Reply-To: <20190807170449.205378-1-mka@chromium.org>
For PHYs with a device tree node look for LED trigger configuration
using the generic binding, if it exists try to apply it via the new
driver hook .config_led.
Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
---
Changes in v5:
- add callback to configure a LED to the PHY driver, instead of
having the driver retrieve the DT data
- use new trigger names
- added support for trigger 'none'
- release DT nodes after use
- renamed 'PHY_LED_LINK_*' to 'PHY_LED_TRIGGER_LINK_*'
- added anonymous struct to struct phy_led_config to track
'activity' in a separate flag. this could be changed to 'flags' if
needed/desired.
- updated commit message (previous subject was 'net: phy: Add
function to retrieve LED configuration from the DT')
Changes in v4:
- patch added to the series
---
drivers/net/phy/phy_device.c | 72 ++++++++++++++++++++++++++++++++++++
include/linux/phy.h | 22 +++++++++++
2 files changed, 94 insertions(+)
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 6b5cb87f3866..6f85fdf72af0 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -29,6 +29,7 @@
#include <linux/phy_led_triggers.h>
#include <linux/mdio.h>
#include <linux/io.h>
+#include <linux/of.h>
#include <linux/uaccess.h>
MODULE_DESCRIPTION("PHY library");
@@ -1064,6 +1065,75 @@ static int phy_poll_reset(struct phy_device *phydev)
return 0;
}
+static void of_phy_config_leds(struct phy_device *phydev)
+{
+ struct device_node *np, *child;
+ struct phy_led_config cfg;
+ const char *trigger;
+ int ret;
+
+ if (!IS_ENABLED(CONFIG_OF_MDIO) || !phydev->drv->config_led)
+ return;
+
+ np = of_find_node_by_name(phydev->mdio.dev.of_node, "leds");
+ if (!np)
+ return;
+
+ for_each_child_of_node(np, child) {
+ u32 led;
+
+ if (of_property_read_u32(child, "reg", &led))
+ goto skip_config;
+
+ ret = of_property_read_string(child, "linux,default-trigger",
+ &trigger);
+ if (ret)
+ trigger = "none";
+
+ memset(&cfg, 0, sizeof(cfg));
+
+ if (!strcmp(trigger, "none")) {
+ cfg.trigger.t = PHY_LED_TRIGGER_NONE;
+ } else if (!strcmp(trigger, "phy-link")) {
+ cfg.trigger.t = PHY_LED_TRIGGER_LINK;
+ } else if (!strcmp(trigger, "phy-link-10m")) {
+ cfg.trigger.t = PHY_LED_TRIGGER_LINK_10M;
+ } else if (!strcmp(trigger, "phy-link-100m")) {
+ cfg.trigger.t = PHY_LED_TRIGGER_LINK_100M;
+ } else if (!strcmp(trigger, "phy-link-1g")) {
+ cfg.trigger.t = PHY_LED_TRIGGER_LINK_1G;
+ } else if (!strcmp(trigger, "phy-link-10g")) {
+ cfg.trigger.t = PHY_LED_TRIGGER_LINK_10G;
+ } else if (!strcmp(trigger, "phy-link-activity")) {
+ cfg.trigger.t = PHY_LED_TRIGGER_LINK;
+ cfg.trigger.activity = true;
+ } else if (!strcmp(trigger, "phy-link-10m-activity")) {
+ cfg.trigger.t = PHY_LED_TRIGGER_LINK_10M;
+ cfg.trigger.activity = true;
+ } else if (!strcmp(trigger, "phy-link-100m-activity")) {
+ cfg.trigger.t = PHY_LED_TRIGGER_LINK_100M;
+ cfg.trigger.activity = true;
+ } else if (!strcmp(trigger, "phy-link-1g-activity")) {
+ cfg.trigger.t = PHY_LED_TRIGGER_LINK_1G;
+ cfg.trigger.activity = true;
+ } else if (!strcmp(trigger, "phy-link-10g-activity")) {
+ cfg.trigger.t = PHY_LED_TRIGGER_LINK_10G;
+ cfg.trigger.activity = true;
+ } else {
+ phydev_warn(phydev, "trigger '%s' for LED%d is invalid\n",
+ trigger, led);
+ goto skip_config;
+ }
+
+ phydev->drv->config_led(phydev, led, &cfg);
+
+ skip_config:
+ of_node_put(child);
+ }
+
+ of_node_put(np);
+}
+
int phy_init_hw(struct phy_device *phydev)
{
int ret = 0;
@@ -1087,6 +1157,8 @@ int phy_init_hw(struct phy_device *phydev)
if (phydev->drv->config_init)
ret = phydev->drv->config_init(phydev);
+ of_phy_config_leds(phydev);
+
return ret;
}
EXPORT_SYMBOL(phy_init_hw);
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 462b90b73f93..3a07390fc5e9 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -325,6 +325,24 @@ struct phy_c45_device_ids {
u32 device_ids[8];
};
+/* Triggers for PHY LEDs */
+enum phy_led_trigger {
+ PHY_LED_TRIGGER_NONE,
+ PHY_LED_TRIGGER_LINK,
+ PHY_LED_TRIGGER_LINK_10M,
+ PHY_LED_TRIGGER_LINK_100M,
+ PHY_LED_TRIGGER_LINK_1G,
+ PHY_LED_TRIGGER_LINK_10G,
+};
+
+/* Configuration of a single PHY LED */
+struct phy_led_config {
+ struct {
+ enum phy_led_trigger t;
+ bool activity;
+ } trigger;
+};
+
/* phy_device: An instance of a PHY
*
* drv: Pointer to the driver for this PHY instance
@@ -626,6 +644,10 @@ struct phy_driver {
struct ethtool_tunable *tuna,
const void *data);
int (*set_loopback)(struct phy_device *dev, bool enable);
+
+ /* Configure a PHY LED */
+ int (*config_led)(struct phy_device *dev, int led,
+ struct phy_led_config *cfg);
};
#define to_phy_driver(d) container_of(to_mdio_common_driver(d), \
struct phy_driver, mdiodrv)
--
2.22.0.770.g0f2c4a37fd-goog
^ permalink raw reply related
* Re:[PATCH net-next] net/ncsi: allow to customize BMC MAC Address offset
From: Vijay Khemka @ 2019-08-07 17:36 UTC (permalink / raw)
To: Tao Ren, Samuel Mendoza-Jonas, David S . Miller,
netdev@vger.kernel.org, linux-kernel@vger.kernel.org,
openbmc@lists.ozlabs.org, William Kennington, Joel Stanley
Lgtm except one small comment below.
On 8/6/19, 5:22 PM, "openbmc on behalf of Tao Ren" <openbmc-bounces+vijaykhemka=fb.com@lists.ozlabs.org on behalf of taoren@fb.com> wrote:
Currently BMC's MAC address is calculated by adding 1 to NCSI NIC's base
MAC address when CONFIG_NCSI_OEM_CMD_GET_MAC option is enabled. The logic
doesn't work for platforms with different BMC MAC offset: for example,
Facebook Yamp BMC's MAC address is calculated by adding 2 to NIC's base
MAC address ("BaseMAC + 1" is reserved for Host use).
This patch adds NET_NCSI_MC_MAC_OFFSET config option to customize offset
between NIC's Base MAC address and BMC's MAC address. Its default value is
set to 1 to avoid breaking existing users.
Signed-off-by: Tao Ren <taoren@fb.com>
---
net/ncsi/Kconfig | 8 ++++++++
net/ncsi/ncsi-rsp.c | 15 +++++++++++++--
2 files changed, 21 insertions(+), 2 deletions(-)
diff --git a/net/ncsi/Kconfig b/net/ncsi/Kconfig
index 2f1e5756c03a..be8efe1ed99e 100644
--- a/net/ncsi/Kconfig
+++ b/net/ncsi/Kconfig
@@ -17,3 +17,11 @@ config NCSI_OEM_CMD_GET_MAC
---help---
This allows to get MAC address from NCSI firmware and set them back to
controller.
+config NET_NCSI_MC_MAC_OFFSET
+ int
+ prompt "Offset of Management Controller's MAC Address"
+ depends on NCSI_OEM_CMD_GET_MAC
+ default 1
+ help
+ This defines the offset between Network Controller's (base) MAC
+ address and Management Controller's MAC address.
diff --git a/net/ncsi/ncsi-rsp.c b/net/ncsi/ncsi-rsp.c
index 7581bf919885..24a791f9ebf5 100644
--- a/net/ncsi/ncsi-rsp.c
+++ b/net/ncsi/ncsi-rsp.c
@@ -656,6 +656,11 @@ static int ncsi_rsp_handler_oem_bcm_gma(struct ncsi_request *nr)
struct ncsi_rsp_oem_pkt *rsp;
struct sockaddr saddr;
int ret = 0;
+#ifdef CONFIG_NET_NCSI_MC_MAC_OFFSET
+ int mac_offset = CONFIG_NET_NCSI_MC_MAC_OFFSET;
+#else
+ int mac_offset = 1;
+#endif
/* Get the response header */
rsp = (struct ncsi_rsp_oem_pkt *)skb_network_header(nr->rsp);
@@ -663,8 +668,14 @@ static int ncsi_rsp_handler_oem_bcm_gma(struct ncsi_request *nr)
saddr.sa_family = ndev->type;
ndev->priv_flags |= IFF_LIVE_ADDR_CHANGE;
memcpy(saddr.sa_data, &rsp->data[BCM_MAC_ADDR_OFFSET], ETH_ALEN);
- /* Increase mac address by 1 for BMC's address */
- eth_addr_inc((u8 *)saddr.sa_data);
+
+ /* Management Controller's MAC address is calculated by adding
+ * the offset to Network Controller's (base) MAC address.
+ * Note: negative offset is "ignored", and BMC will use the Base
Just mention negative and zero offset is ignored. As you are ignoring 0 as well.
+ * MAC address in this case.
+ */
+ while (mac_offset-- > 0)
+ eth_addr_inc((u8 *)saddr.sa_data);
if (!is_valid_ether_addr((const u8 *)saddr.sa_data))
return -ENXIO;
--
2.17.1
^ permalink raw reply
* Re: [bpf-next PATCH 1/3] samples/bpf: xdp_fwd rename devmap name to be xdp_tx_ports
From: David Ahern @ 2019-08-07 17:52 UTC (permalink / raw)
To: Jesper Dangaard Brouer, netdev, Daniel Borkmann,
Alexei Starovoitov
Cc: xdp-newbies, a.s.protopopov
In-Reply-To: <156518137803.5636.11766023213864836956.stgit@firesoul>
On 8/7/19 6:36 AM, Jesper Dangaard Brouer wrote:
> The devmap name 'tx_port' came from a copy-paste from xdp_redirect_map
> which only have a single TX port. Change name to xdp_tx_ports
> to make it more descriptive.
>
> Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com>
> ---
> samples/bpf/xdp_fwd_kern.c | 5 +++--
> samples/bpf/xdp_fwd_user.c | 2 +-
> 2 files changed, 4 insertions(+), 3 deletions(-)
>
Reviewed-by: David Ahern <dsahern@gmail.com>
^ permalink raw reply
* Re: [bpf-next PATCH 2/3] samples/bpf: make xdp_fwd more practically usable via devmap lookup
From: David Ahern @ 2019-08-07 17:52 UTC (permalink / raw)
To: Jesper Dangaard Brouer, netdev, Daniel Borkmann,
Alexei Starovoitov
Cc: xdp-newbies, a.s.protopopov
In-Reply-To: <156518138310.5636.13064696265479533742.stgit@firesoul>
On 8/7/19 6:36 AM, Jesper Dangaard Brouer wrote:
> This address the TODO in samples/bpf/xdp_fwd_kern.c, which points out
> that the chosen egress index should be checked for existence in the
> devmap. This can now be done via taking advantage of Toke's work in
> commit 0cdbb4b09a06 ("devmap: Allow map lookups from eBPF").
>
> This change makes xdp_fwd more practically usable, as this allows for
> a mixed environment, where IP-forwarding fallback to network stack, if
> the egress device isn't configured to use XDP.
>
> Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com>
> ---
> samples/bpf/xdp_fwd_kern.c | 20 ++++++++++++++------
> samples/bpf/xdp_fwd_user.c | 36 +++++++++++++++++++++++++-----------
> 2 files changed, 39 insertions(+), 17 deletions(-)
>
Reviewed-by: David Ahern <dsahern@gmail.com>
^ permalink raw reply
* Re: [bpf-next PATCH 1/3] samples/bpf: xdp_fwd rename devmap name to be xdp_tx_ports
From: Y Song @ 2019-08-07 17:52 UTC (permalink / raw)
To: Jesper Dangaard Brouer
Cc: netdev, Daniel Borkmann, Alexei Starovoitov, Xdp, a.s.protopopov,
David Ahern
In-Reply-To: <156518137803.5636.11766023213864836956.stgit@firesoul>
On Wed, Aug 7, 2019 at 5:37 AM Jesper Dangaard Brouer <brouer@redhat.com> wrote:
>
> The devmap name 'tx_port' came from a copy-paste from xdp_redirect_map
> which only have a single TX port. Change name to xdp_tx_ports
> to make it more descriptive.
>
> Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com>
Acked-by: Yonghong Song <yhs@fb.com>
^ permalink raw reply
* Re: [bpf-next PATCH 3/3] samples/bpf: xdp_fwd explain bpf_fib_lookup return codes
From: David Ahern @ 2019-08-07 17:52 UTC (permalink / raw)
To: Jesper Dangaard Brouer, netdev, Daniel Borkmann,
Alexei Starovoitov
Cc: xdp-newbies, a.s.protopopov
In-Reply-To: <156518138817.5636.3626699603179533211.stgit@firesoul>
On 8/7/19 6:36 AM, Jesper Dangaard Brouer wrote:
> Make it clear that this XDP program depend on the network
> stack to do the ARP resolution. This is connected with the
> BPF_FIB_LKUP_RET_NO_NEIGH return code from bpf_fib_lookup().
>
> Another common mistake (seen via XDP-tutorial) is that users
> don't realize that sysctl net.ipv{4,6}.conf.all.forwarding
> setting is honored by bpf_fib_lookup.
>
> Reported-by: Anton Protopopov <a.s.protopopov@gmail.com>
> Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com>
> ---
> samples/bpf/xdp_fwd_kern.c | 19 +++++++++++++++++--
> 1 file changed, 17 insertions(+), 2 deletions(-)
>
Reviewed-by: David Ahern <dsahern@gmail.com>
^ permalink raw reply
* Re: [PATCH net v2] net/tls: prevent skb_orphan() from leaking TLS plain text with offload
From: Jakub Kicinski @ 2019-08-07 18:00 UTC (permalink / raw)
To: Willem de Bruijn
Cc: David Miller, Network Development, davejwatson, borisp, aviadye,
John Fastabend, Daniel Borkmann, Eric Dumazet, Alexei Starovoitov,
oss-drivers
In-Reply-To: <CA+FuTScYkHho4hqrGf9q6=4iao-f2P2s258rjtQTCgn+nF-CYg@mail.gmail.com>
On Wed, 7 Aug 2019 12:59:00 -0400, Willem de Bruijn wrote:
> On Wed, Aug 7, 2019 at 2:06 AM Jakub Kicinski wrote:
> > diff --git a/net/core/sock.c b/net/core/sock.c
> > index d57b0cc995a0..0f9619b0892f 100644
> > --- a/net/core/sock.c
> > +++ b/net/core/sock.c
> > @@ -1992,6 +1992,20 @@ void skb_set_owner_w(struct sk_buff *skb, struct sock *sk)
> > }
> > EXPORT_SYMBOL(skb_set_owner_w);
> >
> > +static bool can_skb_orphan_partial(const struct sk_buff *skb)
> > +{
> > +#ifdef CONFIG_TLS_DEVICE
> > + /* Drivers depend on in-order delivery for crypto offload,
> > + * partial orphan breaks out-of-order-OK logic.
> > + */
> > + if (skb->decrypted)
> > + return false;
> > +#endif
> > + return (IS_ENABLED(CONFIG_INET) &&
> > + skb->destructor == tcp_wfree) ||
>
> Please add parentheses around IS_ENABLED(CONFIG_INET) &&
> skb->destructor == tcp_wfree
Mm.. there are parenthesis around them, maybe I'm being slow,
could you show me how?
> I was also surprised that this works when tcp_wfree is not defined if
> !CONFIG_INET. But apparently it does (at -O2?) :)
I was surprised to but in essence it should work the same as
if (IS_ENABLED(CONFIG_xyz))
call_some_xyz_code();
from compiler's perspective, and we do that a lot. Perhaps kbuild
bot will prove us wrong :)
> > @@ -984,6 +984,9 @@ ssize_t do_tcp_sendpages(struct sock *sk, struct page *page, int offset,
> > if (!skb)
> > goto wait_for_memory;
> >
> > +#ifdef CONFIG_TLS_DEVICE
> > + skb->decrypted = !!(flags & MSG_SENDPAGE_DECRYPTED);
> > +#endif
>
> Nothing is stopping userspace from passing this new flag. In send
> (tcp_sendmsg_locked) it is ignored. But can it reach do_tcp_sendpages
> through tcp_bpf_sendmsg?
Ah, I think you're right, thanks for checking that :( I don't entirely
follow how 0608c69c9a80 ("bpf: sk_msg, sock{map|hash} redirect through
ULP") is safe then.
One option would be to clear the flags kernel would previously ignore
in tcp_bpf_sendmsg(). But I feel like we should just go back to marking
the socket, since we don't need the per-message flexibility of a flag.
WDYT?
> > skb_entail(sk, skb);
> > copy = size_goal;
> > }
> > diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
> > index 6e4afc48d7bb..979520e46e33 100644
> > --- a/net/ipv4/tcp_output.c
> > +++ b/net/ipv4/tcp_output.c
> > @@ -1320,6 +1320,7 @@ int tcp_fragment(struct sock *sk, enum tcp_queue tcp_queue,
> > buff = sk_stream_alloc_skb(sk, nsize, gfp, true);
> > if (!buff)
> > return -ENOMEM; /* We'll just try again later. */
> > + skb_copy_decrypted(buff, skb);
>
> This code has to copy timestamps, tx_flags, zerocopy state and now
> this in three locations. Eventually we'll want a single helper for all
> of them..
Ack, should I take an action on that for net-next or was it a
note-to-self? :)
^ permalink raw reply
* Re: [PATCH v3] mlx5: Use refcount_t for refcount
From: Saeed Mahameed @ 2019-08-07 18:03 UTC (permalink / raw)
To: leon@kernel.org
Cc: jgg@ziepe.ca, dledford@redhat.com, davem@davemloft.net,
linux-kernel@vger.kernel.org, hslester96@gmail.com,
linux-rdma@vger.kernel.org, netdev@vger.kernel.org
In-Reply-To: <20190807031717.GB4832@mtr-leonro.mtl.com>
On Wed, 2019-08-07 at 06:17 +0300, Leon Romanovsky wrote:
> On Tue, Aug 06, 2019 at 08:40:11PM +0000, Saeed Mahameed wrote:
> > On Tue, 2019-08-06 at 09:59 +0800, Chuhong Yuan wrote:
> > > Reference counters are preferred to use refcount_t instead of
> > > atomic_t.
> > > This is because the implementation of refcount_t can prevent
> > > overflows and detect possible use-after-free.
> > > So convert atomic_t ref counters to refcount_t.
> > >
> > > Signed-off-by: Chuhong Yuan <hslester96@gmail.com>
> > > ---
> > > Changes in v3:
> > > - Merge v2 patches together.
> > >
> > > drivers/infiniband/hw/mlx5/srq_cmd.c | 6 +++---
> > > drivers/net/ethernet/mellanox/mlx5/core/qp.c | 6 +++---
> > > include/linux/mlx5/driver.h | 3 ++-
> > > 3 files changed, 8 insertions(+), 7 deletions(-)
> > >
> >
> > LGTM, Leon, let me know if you are happy with this version,
> > this should go to mlx5-next.
>
> Thanks,
> Acked-by: Leon Romanovsky <leonro@mellanox.com>
Applied to mlx5-next.
Thanks !
^ permalink raw reply
* Re: [bpf-next PATCH 2/3] samples/bpf: make xdp_fwd more practically usable via devmap lookup
From: Y Song @ 2019-08-07 18:04 UTC (permalink / raw)
To: Jesper Dangaard Brouer
Cc: netdev, Daniel Borkmann, Alexei Starovoitov, Xdp, a.s.protopopov,
David Ahern
In-Reply-To: <156518138310.5636.13064696265479533742.stgit@firesoul>
On Wed, Aug 7, 2019 at 5:37 AM Jesper Dangaard Brouer <brouer@redhat.com> wrote:
>
> This address the TODO in samples/bpf/xdp_fwd_kern.c, which points out
> that the chosen egress index should be checked for existence in the
> devmap. This can now be done via taking advantage of Toke's work in
> commit 0cdbb4b09a06 ("devmap: Allow map lookups from eBPF").
>
> This change makes xdp_fwd more practically usable, as this allows for
> a mixed environment, where IP-forwarding fallback to network stack, if
> the egress device isn't configured to use XDP.
>
> Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com>
> ---
> samples/bpf/xdp_fwd_kern.c | 20 ++++++++++++++------
> samples/bpf/xdp_fwd_user.c | 36 +++++++++++++++++++++++++-----------
> 2 files changed, 39 insertions(+), 17 deletions(-)
>
> diff --git a/samples/bpf/xdp_fwd_kern.c b/samples/bpf/xdp_fwd_kern.c
> index e6ffc4ea06f4..4a5ad381ed2a 100644
> --- a/samples/bpf/xdp_fwd_kern.c
> +++ b/samples/bpf/xdp_fwd_kern.c
> @@ -104,13 +104,21 @@ static __always_inline int xdp_fwd_flags(struct xdp_md *ctx, u32 flags)
>
> rc = bpf_fib_lookup(ctx, &fib_params, sizeof(fib_params), flags);
>
> - /* verify egress index has xdp support
> - * TO-DO bpf_map_lookup_elem(&tx_port, &key) fails with
> - * cannot pass map_type 14 into func bpf_map_lookup_elem#1:
> - * NOTE: without verification that egress index supports XDP
> - * forwarding packets are dropped.
> - */
> if (rc == 0) {
> + int *val;
> +
> + /* Verify egress index has been configured as TX-port.
> + * (Note: User can still have inserted an egress ifindex that
> + * doesn't support XDP xmit, which will result in packet drops).
> + *
> + * Note: lookup in devmap supported since 0cdbb4b09a0.
> + * If not supported will fail with:
> + * cannot pass map_type 14 into func bpf_map_lookup_elem#1:
> + */
> + val = bpf_map_lookup_elem(&tx_port, &fib_params.ifindex);
It should be "xdp_tx_ports". Otherwise, you will have compilation errors.
> + if (!val)
> + return XDP_PASS;
Also, maybe we can do
if (!bpf_map_lookup_elem(&tx_port, &fib_params.ifindex))
return XDP_PASS;
so we do not need to define val at all.
> +
> if (h_proto == htons(ETH_P_IP))
> ip_decrease_ttl(iph);
> else if (h_proto == htons(ETH_P_IPV6))
> diff --git a/samples/bpf/xdp_fwd_user.c b/samples/bpf/xdp_fwd_user.c
> index ba012d9f93dd..20951bc27477 100644
> --- a/samples/bpf/xdp_fwd_user.c
> +++ b/samples/bpf/xdp_fwd_user.c
> @@ -27,14 +27,20 @@
> #include "libbpf.h"
> #include <bpf/bpf.h>
>
> -
> -static int do_attach(int idx, int fd, const char *name)
> +static int do_attach(int idx, int prog_fd, int map_fd, const char *name)
> {
> int err;
>
> - err = bpf_set_link_xdp_fd(idx, fd, 0);
> - if (err < 0)
> + err = bpf_set_link_xdp_fd(idx, prog_fd, 0);
> + if (err < 0) {
> printf("ERROR: failed to attach program to %s\n", name);
> + return err;
> + }
> +
> + /* Adding ifindex as a possible egress TX port */
> + err = bpf_map_update_elem(map_fd, &idx, &idx, 0);
> + if (err)
> + printf("ERROR: failed using device %s as TX-port\n", name);
>
> return err;
> }
> @@ -47,6 +53,9 @@ static int do_detach(int idx, const char *name)
> if (err < 0)
> printf("ERROR: failed to detach program from %s\n", name);
>
> + /* TODO: Remember to cleanup map, when adding use of shared map
> + * bpf_map_delete_elem((map_fd, &idx);
> + */
> return err;
> }
>
> @@ -67,10 +76,10 @@ int main(int argc, char **argv)
> };
> const char *prog_name = "xdp_fwd";
> struct bpf_program *prog;
> + int prog_fd, map_fd = -1;
> char filename[PATH_MAX];
> struct bpf_object *obj;
> int opt, i, idx, err;
> - int prog_fd, map_fd;
> int attach = 1;
> int ret = 0;
>
> @@ -103,8 +112,17 @@ int main(int argc, char **argv)
> return 1;
> }
>
> - if (bpf_prog_load_xattr(&prog_load_attr, &obj, &prog_fd))
> + err = bpf_prog_load_xattr(&prog_load_attr, &obj, &prog_fd);
> + if (err) {
> + if (err == -22) {
-EINVAL?
For -EINVAL, many things could go wrong. But maybe the blow error
is the most common one so I am fine with that.
> + printf("Does kernel support devmap lookup?\n");
> + /* If not, the error message will be:
> + * "cannot pass map_type 14 into func
> + * bpf_map_lookup_elem#1"
> + */
> + }
> return 1;
> + }
>
> prog = bpf_object__find_program_by_title(obj, prog_name);
> prog_fd = bpf_program__fd(prog);
> @@ -119,10 +137,6 @@ int main(int argc, char **argv)
> return 1;
> }
> }
> - if (attach) {
> - for (i = 1; i < 64; ++i)
> - bpf_map_update_elem(map_fd, &i, &i, 0);
> - }
>
> for (i = optind; i < argc; ++i) {
> idx = if_nametoindex(argv[i]);
> @@ -138,7 +152,7 @@ int main(int argc, char **argv)
> if (err)
> ret = err;
> } else {
> - err = do_attach(idx, prog_fd, argv[i]);
> + err = do_attach(idx, prog_fd, map_fd, argv[i]);
> if (err)
> ret = err;
> }
>
^ permalink raw reply
* Re: [bpf-next PATCH 3/3] samples/bpf: xdp_fwd explain bpf_fib_lookup return codes
From: Y Song @ 2019-08-07 18:05 UTC (permalink / raw)
To: Jesper Dangaard Brouer
Cc: netdev, Daniel Borkmann, Alexei Starovoitov, Xdp, a.s.protopopov,
David Ahern
In-Reply-To: <156518138817.5636.3626699603179533211.stgit@firesoul>
On Wed, Aug 7, 2019 at 5:38 AM Jesper Dangaard Brouer <brouer@redhat.com> wrote:
>
> Make it clear that this XDP program depend on the network
> stack to do the ARP resolution. This is connected with the
> BPF_FIB_LKUP_RET_NO_NEIGH return code from bpf_fib_lookup().
>
> Another common mistake (seen via XDP-tutorial) is that users
> don't realize that sysctl net.ipv{4,6}.conf.all.forwarding
> setting is honored by bpf_fib_lookup.
>
> Reported-by: Anton Protopopov <a.s.protopopov@gmail.com>
> Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com>
Acked-by: Yonghong Song <yhs@fb.com>
^ permalink raw reply
* Re: [PATCH v5 2/4] net: phy: Add support for generic LED configuration through the DT
From: Andrew Lunn @ 2019-08-07 18:15 UTC (permalink / raw)
To: Matthias Kaehlcke
Cc: David S . Miller, Rob Herring, Mark Rutland, Florian Fainelli,
Heiner Kallweit, netdev, devicetree, linux-kernel,
Douglas Anderson
In-Reply-To: <20190807170449.205378-3-mka@chromium.org>
> + for_each_child_of_node(np, child) {
> + u32 led;
> +
> + if (of_property_read_u32(child, "reg", &led))
> + goto skip_config;
> +
> + skip_config:
> + of_node_put(child);
There is no need for this put. So long as you don't break out of
for_each_child_of_node() with a return, it will correctly release
child at the end of each loop. A continue statement is also O.K.
Andrew
^ permalink raw reply
* Re: [PATCH v5 2/4] net: phy: Add support for generic LED configuration through the DT
From: Andrew Lunn @ 2019-08-07 18:18 UTC (permalink / raw)
To: Matthias Kaehlcke
Cc: David S . Miller, Rob Herring, Mark Rutland, Florian Fainelli,
Heiner Kallweit, netdev, devicetree, linux-kernel,
Douglas Anderson
In-Reply-To: <20190807170449.205378-3-mka@chromium.org>
> +static void of_phy_config_leds(struct phy_device *phydev)
> +{
> + struct device_node *np, *child;
> + struct phy_led_config cfg;
> + const char *trigger;
> + int ret;
> +
> + if (!IS_ENABLED(CONFIG_OF_MDIO) || !phydev->drv->config_led)
> + return;
> +
> + np = of_find_node_by_name(phydev->mdio.dev.of_node, "leds");
> + if (!np)
> + return;
> +
> + for_each_child_of_node(np, child) {
> + u32 led;
> +
> + if (of_property_read_u32(child, "reg", &led))
> + goto skip_config;
> +
> + ret = of_property_read_string(child, "linux,default-trigger",
> + &trigger);
> + if (ret)
> + trigger = "none";
> +
> + memset(&cfg, 0, sizeof(cfg));
> +
> + if (!strcmp(trigger, "none")) {
> + cfg.trigger.t = PHY_LED_TRIGGER_NONE;
> + } else if (!strcmp(trigger, "phy-link")) {
> + cfg.trigger.t = PHY_LED_TRIGGER_LINK;
> + } else if (!strcmp(trigger, "phy-link-10m")) {
> + cfg.trigger.t = PHY_LED_TRIGGER_LINK_10M;
> + } else if (!strcmp(trigger, "phy-link-100m")) {
> + cfg.trigger.t = PHY_LED_TRIGGER_LINK_100M;
> + } else if (!strcmp(trigger, "phy-link-1g")) {
> + cfg.trigger.t = PHY_LED_TRIGGER_LINK_1G;
> + } else if (!strcmp(trigger, "phy-link-10g")) {
> + cfg.trigger.t = PHY_LED_TRIGGER_LINK_10G;
> + } else if (!strcmp(trigger, "phy-link-activity")) {
> + cfg.trigger.t = PHY_LED_TRIGGER_LINK;
> + cfg.trigger.activity = true;
> + } else if (!strcmp(trigger, "phy-link-10m-activity")) {
> + cfg.trigger.t = PHY_LED_TRIGGER_LINK_10M;
> + cfg.trigger.activity = true;
> + } else if (!strcmp(trigger, "phy-link-100m-activity")) {
> + cfg.trigger.t = PHY_LED_TRIGGER_LINK_100M;
> + cfg.trigger.activity = true;
> + } else if (!strcmp(trigger, "phy-link-1g-activity")) {
> + cfg.trigger.t = PHY_LED_TRIGGER_LINK_1G;
> + cfg.trigger.activity = true;
> + } else if (!strcmp(trigger, "phy-link-10g-activity")) {
> + cfg.trigger.t = PHY_LED_TRIGGER_LINK_10G;
> + cfg.trigger.activity = true;
> + } else {
> + phydev_warn(phydev, "trigger '%s' for LED%d is invalid\n",
> + trigger, led);
> + goto skip_config;
> + }
> +
> + phydev->drv->config_led(phydev, led, &cfg);
We should not ignore the return value. I expect drivers will return
-EINVAL, or EOPNOTSUPP if asked to configure an LED in a way they
don't support. We need to report this. So i would add a phydev_warn:
> + phydev_warn(phydev, "trigger '%s' for LED%d not supported\n",
> + trigger, led);
Andrew
^ permalink raw reply
* Re: [PATCH net-next 2/5] r8152: replace array with linking list for rx information
From: Jakub Kicinski @ 2019-08-07 18:21 UTC (permalink / raw)
To: Hayes Wang
Cc: netdev@vger.kernel.org, nic_swsd, linux-kernel@vger.kernel.org,
linux-usb@vger.kernel.org
In-Reply-To: <0835B3720019904CB8F7AA43166CEEB2F18D04FA@RTITMBSVM03.realtek.com.tw>
On Wed, 7 Aug 2019 04:34:24 +0000, Hayes Wang wrote:
> Jakub Kicinski [mailto:jakub.kicinski@netronome.com]
> > > static int rtl_stop_rx(struct r8152 *tp)
> > > {
> > > - int i;
> > > + struct list_head *cursor, *next, tmp_list;
> > > + unsigned long flags;
> > > +
> > > + INIT_LIST_HEAD(&tmp_list);
> > >
> > > - for (i = 0; i < RTL8152_MAX_RX; i++)
> > > - usb_kill_urb(tp->rx_info[i].urb);
> > > + /* The usb_kill_urb() couldn't be used in atomic.
> > > + * Therefore, move the list of rx_info to a tmp one.
> > > + * Then, list_for_each_safe could be used without
> > > + * spin lock.
> > > + */
> >
> > Would you mind explaining in a little more detail why taking the
> > entries from the list for a brief period of time is safe?
>
> Usually, it needs the spin lock before accessing the entry
> of the list "tp->rx_info". However, for some reasons,
> if we want to access the entry without spin lock, we
> cloud move all entries to a local list temporally. Then,
> we could make sure no other one could access the entries
> included in the temporal local list.
>
> For this case, when I move all entries to a temporal
> local list, no other one could access them. Therefore,
> I could access the entries included in the temporal local
> list without spin lock.
I see, thanks for the explanation.
^ permalink raw reply
* Re: [PATCH v5 4/4] net: phy: realtek: Add LED configuration support for RTL8211E
From: Andrew Lunn @ 2019-08-07 18:24 UTC (permalink / raw)
To: Matthias Kaehlcke
Cc: David S . Miller, Rob Herring, Mark Rutland, Florian Fainelli,
Heiner Kallweit, netdev, devicetree, linux-kernel,
Douglas Anderson
In-Reply-To: <20190807170449.205378-5-mka@chromium.org>
> +static int rtl8211e_config_led(struct phy_device *phydev, int led,
> + struct phy_led_config *cfg)
> +{
> + u16 lacr_bits = 0, lcr_bits = 0;
> + int oldpage, ret;
> +
> + switch (cfg->trigger.t) {
> + case PHY_LED_TRIGGER_LINK:
> + lcr_bits = 7 << (led * 4);
> + break;
> +
> + case PHY_LED_TRIGGER_LINK_10M:
> + lcr_bits = 1 << (led * 4);
> + break;
> +
> + case PHY_LED_TRIGGER_LINK_100M:
> + lcr_bits = 2 << (led * 4);
> + break;
> +
> + case PHY_LED_TRIGGER_LINK_1G:
> + lcr_bits |= 4 << (led * 4);
> + break;
> +
> + case PHY_LED_TRIGGER_NONE:
> + break;
> +
> + default:
> + phydev_warn(phydev,
> + "unknown trigger for LED%d: %d\n",
> + led, cfg->trigger.t);
Lets do this once in the core, not in every driver.
> + return -EINVAL;
> + }
> +
> + if (cfg->trigger.activity)
> + lacr_bits = BIT(RLT8211E_LACR_LEDACTCTRL_SHIFT + led);
> +
> + rtl8211e_disable_eee_led_mode(phydev);
> +
> + oldpage = rtl8211x_select_ext_page(phydev, 0x2c);
> + if (oldpage < 0) {
> + phydev_err(phydev, "failed to select extended page: %d\n", oldpage);
> + return oldpage;
> + }
> +
> + ret = __phy_modify(phydev, RTL8211E_LACR,
> + LACR_MASK(led), lacr_bits);
> + if (ret) {
> + phydev_err(phydev, "failed to write LACR reg: %d\n",
> + ret);
> + goto err;
> + }
> +
> + ret = __phy_modify(phydev, RTL8211E_LCR,
> + LCR_MASK(led), lcr_bits);
> + if (ret)
> + phydev_err(phydev, "failed to write LCR reg: %d\n",
> + ret);
That is a lot of phydev_err() calls. The rest of the driver does not
use any. Also, mdio operations are very unlikely to fail. So i would
just leave it to the layer above to report there is a problem.
Andrew
^ permalink raw reply
* Re: [PATCH net-next] net/ncsi: allow to customize BMC MAC Address offset
From: Jakub Kicinski @ 2019-08-07 18:25 UTC (permalink / raw)
To: Tao Ren
Cc: Samuel Mendoza-Jonas, David S . Miller, netdev, linux-kernel,
openbmc, William Kennington, Joel Stanley, Andrew Lunn
In-Reply-To: <20190807002118.164360-1-taoren@fb.com>
On Tue, 6 Aug 2019 17:21:18 -0700, Tao Ren wrote:
> Currently BMC's MAC address is calculated by adding 1 to NCSI NIC's base
> MAC address when CONFIG_NCSI_OEM_CMD_GET_MAC option is enabled. The logic
> doesn't work for platforms with different BMC MAC offset: for example,
> Facebook Yamp BMC's MAC address is calculated by adding 2 to NIC's base
> MAC address ("BaseMAC + 1" is reserved for Host use).
>
> This patch adds NET_NCSI_MC_MAC_OFFSET config option to customize offset
> between NIC's Base MAC address and BMC's MAC address. Its default value is
> set to 1 to avoid breaking existing users.
>
> Signed-off-by: Tao Ren <taoren@fb.com>
Maybe someone more knowledgeable like Andrew has an opinion here,
but to me it seems a bit strange to encode what seems to be platfrom
information in the kernel config :(
> diff --git a/net/ncsi/Kconfig b/net/ncsi/Kconfig
> index 2f1e5756c03a..be8efe1ed99e 100644
> --- a/net/ncsi/Kconfig
> +++ b/net/ncsi/Kconfig
> @@ -17,3 +17,11 @@ config NCSI_OEM_CMD_GET_MAC
> ---help---
> This allows to get MAC address from NCSI firmware and set them back to
> controller.
> +config NET_NCSI_MC_MAC_OFFSET
> + int
> + prompt "Offset of Management Controller's MAC Address"
> + depends on NCSI_OEM_CMD_GET_MAC
> + default 1
> + help
> + This defines the offset between Network Controller's (base) MAC
> + address and Management Controller's MAC address.
> diff --git a/net/ncsi/ncsi-rsp.c b/net/ncsi/ncsi-rsp.c
> index 7581bf919885..24a791f9ebf5 100644
> --- a/net/ncsi/ncsi-rsp.c
> +++ b/net/ncsi/ncsi-rsp.c
> @@ -656,6 +656,11 @@ static int ncsi_rsp_handler_oem_bcm_gma(struct ncsi_request *nr)
> struct ncsi_rsp_oem_pkt *rsp;
> struct sockaddr saddr;
> int ret = 0;
> +#ifdef CONFIG_NET_NCSI_MC_MAC_OFFSET
> + int mac_offset = CONFIG_NET_NCSI_MC_MAC_OFFSET;
> +#else
> + int mac_offset = 1;
> +#endif
>
> /* Get the response header */
> rsp = (struct ncsi_rsp_oem_pkt *)skb_network_header(nr->rsp);
> @@ -663,8 +668,14 @@ static int ncsi_rsp_handler_oem_bcm_gma(struct ncsi_request *nr)
> saddr.sa_family = ndev->type;
> ndev->priv_flags |= IFF_LIVE_ADDR_CHANGE;
> memcpy(saddr.sa_data, &rsp->data[BCM_MAC_ADDR_OFFSET], ETH_ALEN);
> - /* Increase mac address by 1 for BMC's address */
> - eth_addr_inc((u8 *)saddr.sa_data);
> +
> + /* Management Controller's MAC address is calculated by adding
> + * the offset to Network Controller's (base) MAC address.
> + * Note: negative offset is "ignored", and BMC will use the Base
> + * MAC address in this case.
> + */
> + while (mac_offset-- > 0)
> + eth_addr_inc((u8 *)saddr.sa_data);
> if (!is_valid_ether_addr((const u8 *)saddr.sa_data))
> return -ENXIO;
>
^ 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