* Re: [PATCH net] netvsc: increase default receive buffer size
From: Stephen Hemminger @ 2017-09-14 17:49 UTC (permalink / raw)
To: David Miller; +Cc: netdev, haiyangz, sthemmin, devel
In-Reply-To: <20170914.100203.1656028222884493229.davem@davemloft.net>
On Thu, 14 Sep 2017 10:02:03 -0700 (PDT)
David Miller <davem@davemloft.net> wrote:
> From: Stephen Hemminger <stephen@networkplumber.org>
> Date: Thu, 14 Sep 2017 09:31:07 -0700
>
> > The default receive buffer size was reduced by recent change
> > to a value which was appropriate for 10G and Windows Server 2016.
> > But the value is too small for full performance with 40G on Azure.
> > Increase the default back to maximum supported by host.
> >
> > Fixes: 8b5327975ae1 ("netvsc: allow controlling send/recv buffer size")
> > Signed-off-by: Stephen Hemminger <sthemmin@microsoft.com>
>
> What other side effects are there to making this buffer so large?
>
> Just curious...
It increase latency and exercises bufferbloat avoidance on TCP.
The problem was the smaller buffer caused regressions in UDP
benchmarks on 40G Azure. One could argue that this is not a reasonable
benchmark but people run it. Apparently, Windows already went
the same thing and uses an even bigger buffer.
Longer term there will be more internal discussion with different
teams about what the receive latency and buffering needs to be.
Also, the issue goes away when doing accelerated networking (SR-IOV)
is more widely used.
^ permalink raw reply
* Bug with BPF_ALU64 | BPF_END?
From: Edward Cree @ 2017-09-14 17:53 UTC (permalink / raw)
To: Daniel Borkmann, Alexei Starovoitov; +Cc: netdev
Is BPF_END supposed to only be used with BPF_ALU, never with BPF_ALU64?
In kernel/bpf/core.c:___bpf_prog_run(), there are only jump table targets
for the BPF_ALU case, not for the BPF_ALU64 case (opcodes 0xd7 and 0xdf).
But the verifier doesn't enforce this; by crafting a program that uses
these opcodes I can get a WARN when they're run (without JIT; it looks
like the x86 JIT, at least, won't like it either).
Proposed patch below the cut; build-tested only.
-Ed
---
[PATCH net] bpf/verifier: reject BPF_ALU64|BPF_END
Neither ___bpf_prog_run nor the JITs accept it.
Fixes: 17a5267067f3 ("bpf: verifier (add verifier core)")
Signed-off-by: Edward Cree <ecree@solarflare.com>
---
kernel/bpf/verifier.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 477b693..799b245 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -2292,7 +2292,8 @@ static int check_alu_op(struct bpf_verifier_env *env, struct bpf_insn *insn)
}
} else {
if (insn->src_reg != BPF_REG_0 || insn->off != 0 ||
- (insn->imm != 16 && insn->imm != 32 && insn->imm != 64)) {
+ (insn->imm != 16 && insn->imm != 32 && insn->imm != 64) ||
+ BPF_CLASS(insn->code) == BPF_ALU64) {
verbose("BPF_END uses reserved fields\n");
return -EINVAL;
}
^ permalink raw reply related
* Re: RTL8192EE PCIe Wireless Network Adapter crashed with linux-4.13
From: Larry Finger @ 2017-09-14 18:05 UTC (permalink / raw)
To: Zwindl, linux-wireless@vger.kernel.org
Cc: chaoming_li@realsil.com.cn, kvalo@codeaurora.org,
pkshih@realtek.com, johannes.berg@intel.com,
gregkh@linuxfoundation.org, netdev@vger.kernel.org,
linux-kernel@vger.kernel.org
In-Reply-To: <2WhCEttl4IJtrTB2AkJYmhbsu-kiR_5fU0A_Z7eWW05rbVE5tB86A_ZS8ek_FQFdgaZw1bfx0wFbxOa0Ydjv1T6BzBkIHtaWTbSptcpB_kg=@protonmail.com>
On 09/14/2017 08:30 AM, Zwindl wrote:
> Dear developers:
> I'm using Arch Linux with testing enabled, the current kernel version and
> details are
> `Linux zwindl 4.13.2-1-ARCH #1 SMP PREEMPT Thu Sep 14 02:57:34 UTC 2017 x86_64
> GNU/Linux`.
> The wireless card can't work properly from the kernel 4.13. Here's the log(in
> attachment) when NetworkManager trying to connect my wifi which is named as
> 'TP', my mac addr hided as xx:xx:xx:xx:xx.
> What should I provide to help to debug?
> ZWindL.
The BUG-ON arises in __intel_map_single() due to dir (for direction of DMA)
equal to DMA_NONE (3). When rtl8192ee calls pci_map_single(), it uses
PCI_DMA_TODEVICE (1). I followed the calling sequence through the entire chain,
and none of the routines made any changes to 'dir', other that changing the type
from int to enum dma_data_direction. That would not have changed a 1 to a 3.
I built a 4.13.2 system. The problem does not happen here. At this point, the
system has been up for about two hours. I did discover a small memory leak
associated with firmware loading, but that should not have caused the problem.
Nonetheless, I will be sending a patch to fix that problem.
I will continue testing, although I doubt that the problem will happen here.
How long had your system been up when the problem occurred? Your dmesg fragment
did not show any times. What kernels have you tried besides 4.13.2?
Larry
^ permalink raw reply
* Re: RFC: Audit Kernel Container IDs
From: Richard Guy Briggs @ 2017-09-14 18:07 UTC (permalink / raw)
To: Eric W. Biederman
Cc: cgroups-u79uwXL29TY76Z2rM5mHXA, Linux Containers, Linux API,
Linux Audit, Linux FS Devel, Linux Kernel,
Linux Network Development, Aristeu Rozanski, David Howells,
Eric Paris, jlayton-H+wXaHxf7aLQT0dZR+AlfA, Andy Lutomirski,
mszeredi-H+wXaHxf7aLQT0dZR+AlfA, Paul Moore, Serge E. Hallyn,
Steve Grubb, trondmy-7I+n7zu2hftEKMMhf/gKZA, Al Viro
In-Reply-To: <87d16tb2y5.fsf-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org>
On 2017-09-14 12:33, Eric W. Biederman wrote:
> Richard Guy Briggs <rgb-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> writes:
>
> > The trigger is a pseudo filesystem (proc, since PID tree already exists)
> > write of a u64 representing the container ID to a file representing a
> > process that will become the first process in a new container.
> > This might place restrictions on mount namespaces required to define a
> > container, or at least careful checking of namespaces in the kernel to
> > verify permissions of the orchestrator so it can't change its own
> > container ID.
>
> Why a u64?
u32 will roll too quickly. UUID is large enough that it adds
significantly to audit record bandwidth. I'd prefer u64, but can look
at the difference of accommodating a UUID...
> Why a proc filesystem write and not a magic audit message?
A magic audit message requires CAP_AUDIT_WRITE, which we'd like to use
sparingly. Given that orchestrators will already require it to send
the mandatory AUDIT_VIRT_*, this doesn't seem like an unreasonable burden.
I was originally leaning towards an audit message trigger or a syscall.
> I don't like the fact that the proc filesystem entry is likely going to
> be readable and abusable by non-audit contexts?
This proposal wasn't going to start with that link being readable, but
its filesystem structure and link names would be, perhaps giving away
too much already.
I think we will need to find a way for the orchestrator or one of its
authorized agents to read this information while blocking reads from
unauthorized agents, otherwise this would be of very limited use.
> Why the ability to change the containerid? What is the use case you are
> thinking of there?
This was covered in the end of the conversation with Paul Moore (that
maybe you got tired reading?) I'd originally proposed having it write
once, but Paul figured there was no good reason to restrict it and leave
that decision up to the orchestrator. The use case would be adding
other processes to a container, but it could be argued all additional
processes should be spawned by the first process in a container.
> Eric
- RGB
--
Richard Guy Briggs <rgb-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
Sr. S/W Engineer, Kernel Security, Base Operating Systems
Remote, Ottawa, Red Hat Canada
IRC: rgb, SunRaycer
Voice: +1.647.777.2635, Internal: (81) 32635
^ permalink raw reply
* Re: Bug with BPF_ALU64 | BPF_END?
From: David Miller @ 2017-09-14 18:14 UTC (permalink / raw)
To: ecree; +Cc: daniel, ast, netdev
In-Reply-To: <52c7df55-84d9-f6d2-ed84-51ac90eb6bcc@solarflare.com>
From: Edward Cree <ecree@solarflare.com>
Date: Thu, 14 Sep 2017 18:53:17 +0100
> Is BPF_END supposed to only be used with BPF_ALU, never with BPF_ALU64?
> In kernel/bpf/core.c:___bpf_prog_run(), there are only jump table targets
> for the BPF_ALU case, not for the BPF_ALU64 case (opcodes 0xd7 and 0xdf).
> But the verifier doesn't enforce this; by crafting a program that uses
> these opcodes I can get a WARN when they're run (without JIT; it looks
> like the x86 JIT, at least, won't like it either).
> Proposed patch below the cut; build-tested only.
Good catch.
A really neat test would be a program that uploads random BPF programs
into the kernel, in a syzkaller'ish way. It might have triggered this
(eventually).
^ permalink raw reply
* Re: [PATCH v5 02/10] dt-bindings: net: Restore sun8i dwmac binding
From: Corentin Labbe @ 2017-09-14 18:31 UTC (permalink / raw)
To: Rob Herring
Cc: mark.rutland, maxime.ripard, wens, linux, catalin.marinas,
will.deacon, peppe.cavallaro, alexandre.torgue, andrew,
f.fainelli, netdev, devicetree, linux-arm-kernel, linux-kernel
In-Reply-To: <20170913180734.xvazvf7thpqtssgz@rob-hp-laptop>
On Wed, Sep 13, 2017 at 01:07:34PM -0500, Rob Herring wrote:
> On Fri, Sep 08, 2017 at 09:11:48AM +0200, Corentin Labbe wrote:
> > This patch restore dt-bindings documentation about dwmac-sun8i
> > This reverts commit 8aa33ec2f481 ("dt-bindings: net: Revert sun8i dwmac binding")
>
> Why?
>
I have put the reason in cover-letter and forget to put it in here also.
I fix that in next series.
Thanks
^ permalink raw reply
* Re: [PATCH] net/mlx5: fpga: avoid uninitialized return codes
From: Saeed Mahameed @ 2017-09-14 18:39 UTC (permalink / raw)
To: Leon Romanovsky
Cc: Arnd Bergmann, Ilan Tayari, Saeed Mahameed, Matan Barak,
Boris Pismenny, Linux Netdev List, linux-rdma, linux-kernel
In-Reply-To: <20170914125429.GZ3405@mtr-leonro.local>
On Thu, Sep 14, 2017 at 5:54 AM, Leon Romanovsky <leonro@mellanox.com> wrote:
> On Thu, Sep 14, 2017 at 01:06:18PM +0200, Arnd Bergmann wrote:
>> calling mlx5_fpga_mem_{read,write}_i2c() with a zero length on
>> older compiler version such as gcc-4.6 results in a warning that
>> the return code is not initialized:
>>
>> drivers/net/ethernet/mellanox/mlx5/core/fpga/sdk.c:147:6: error: ‘err’ may be used uninitialized in this function [-Werror=uninitialized]
>> drivers/net/ethernet/mellanox/mlx5/core/fpga/sdk.c:126:6: error: ‘err’ may be used uninitialized in this function [-Werror=uninitialized]
>>
>> On newer compilers, the 'err' variable is optimized away in this
>> code path and assumed to be zero when the loop completes, so we
>> don't get this warning.
>>
>> I'm changing the function here to instead return -EINVAL for the
>> case, under the assumption that it was never meant to be called
>> with a zero length argument.
Thanks Arnd, Looks good.
>
> I agree with you that size can't be zero and this patch will fix the
> warning, but if it is possible, I will prefer to have this check is
> written explicitly and not implicitly.
>
> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/fpga/sdk.c b/drivers/net/ethernet/mellanox/mlx5/core/fpga/sdk.c
> index 3c11d6e2160a..0e85bebe1dad 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/fpga/sdk.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/fpga/sdk.c
> @@ -69,6 +69,9 @@ static int mlx5_fpga_mem_read_i2c(struct mlx5_fpga_device *fdev, size_t size,
> if (!fdev->mdev)
> return -ENOTCONN;
>
> + if (!size)
> + return -EINVAL;
> +
Or simply trust the caller :), and avoid redundant code.
> while (bytes_done < size) {
> actual_size = min(max_size, (size - bytes_done));
>
> Thanks
>
>>
>> Link: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=82203
>> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
>> ---
>> drivers/net/ethernet/mellanox/mlx5/core/fpga/sdk.c | 4 ++--
>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/fpga/sdk.c b/drivers/net/ethernet/mellanox/mlx5/core/fpga/sdk.c
>> index 3c11d6e2160a..914fb9d77a1a 100644
>> --- a/drivers/net/ethernet/mellanox/mlx5/core/fpga/sdk.c
>> +++ b/drivers/net/ethernet/mellanox/mlx5/core/fpga/sdk.c
>> @@ -64,7 +64,7 @@ static int mlx5_fpga_mem_read_i2c(struct mlx5_fpga_device *fdev, size_t size,
>> size_t max_size = MLX5_FPGA_ACCESS_REG_SIZE_MAX;
>> size_t bytes_done = 0;
>> u8 actual_size;
>> - int err;
>> + int err = -EINVAL;
>>
>> if (!fdev->mdev)
>> return -ENOTCONN;
>> @@ -93,7 +93,7 @@ static int mlx5_fpga_mem_write_i2c(struct mlx5_fpga_device *fdev, size_t size,
>> size_t max_size = MLX5_FPGA_ACCESS_REG_SIZE_MAX;
>> size_t bytes_done = 0;
>> u8 actual_size;
>> - int err;
>> + int err = -EINVAL;
>>
>> if (!fdev->mdev)
>> return -ENOTCONN;
>> --
>> 2.9.0
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v5 05/10] dt-bindings: net: dwmac-sun8i: update documentation about integrated PHY
From: Corentin Labbe @ 2017-09-14 18:53 UTC (permalink / raw)
To: Rob Herring
Cc: Maxime Ripard, mark.rutland-5wv7dgnIgG8, wens-jdAy2FN1RRM,
linux-I+IVW8TIWO2tmTQ+vhA3Yw, catalin.marinas-5wv7dgnIgG8,
will.deacon-5wv7dgnIgG8, peppe.cavallaro-qxv4g6HH51o,
alexandre.torgue-qxv4g6HH51o, andrew-g2DYL2Zd6BY,
f.fainelli-Re5JQEeQqe8AvxtiuMwx3w, netdev-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-kernel-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <20170913182004.uniyo5opeilcfk7r@rob-hp-laptop>
On Wed, Sep 13, 2017 at 01:20:04PM -0500, Rob Herring wrote:
> On Fri, Sep 08, 2017 at 09:43:25AM +0200, Corentin Labbe wrote:
> > On Fri, Sep 08, 2017 at 09:25:38AM +0200, Maxime Ripard wrote:
> > > On Fri, Sep 08, 2017 at 09:11:51AM +0200, Corentin Labbe wrote:
> > > > This patch add documentation about the MDIO switch used on sun8i-h3-emac
> > > > for integrated PHY.
> > > >
> > > > Signed-off-by: Corentin Labbe <clabbe.montjoie-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> > > > ---
> > > > .../devicetree/bindings/net/dwmac-sun8i.txt | 127 +++++++++++++++++++--
> > > > 1 file changed, 120 insertions(+), 7 deletions(-)
> > > >
> > > > diff --git a/Documentation/devicetree/bindings/net/dwmac-sun8i.txt b/Documentation/devicetree/bindings/net/dwmac-sun8i.txt
> > > > index 725f3b187886..3fa0e54825ea 100644
> > > > --- a/Documentation/devicetree/bindings/net/dwmac-sun8i.txt
> > > > +++ b/Documentation/devicetree/bindings/net/dwmac-sun8i.txt
> > > > @@ -39,7 +39,7 @@ Optional properties for the following compatibles:
> > > > - allwinner,leds-active-low: EPHY LEDs are active low
> > > >
> > > > Required child node of emac:
> > > > -- mdio bus node: should be named mdio
> > > > +- mdio bus node: should be labelled mdio
> > >
> > > labels do not end up in the final DT (while the names do) so why are
> > > you making this change?
> > >
> >
> > I misunderstood label/name.
> > Anyway, this contrainst should leave due to "snps,dwmac-mdio MDIOs are automatically registered"
> >
> > > >
> > > > Required properties of the mdio node:
> > > > - #address-cells: shall be 1
> > > > @@ -48,14 +48,28 @@ Required properties of the mdio node:
> > > > The device node referenced by "phy" or "phy-handle" should be a child node
> > > > of the mdio node. See phy.txt for the generic PHY bindings.
> > > >
> > > > -Required properties of the phy node with the following compatibles:
> > > > +The following compatibles require an mdio-mux node called "mdio-mux":
> > > > + - "allwinner,sun8i-h3-emac"
> > > > + - "allwinner,sun8i-v3s-emac":
> > > > +Required properties for the mdio-mux node:
> > > > + - compatible = "mdio-mux"
> > > > + - one child mdio for the integrated mdio
> > > > + - one child mdio for the external mdio if present (V3s have none)
> > > > +Required properties for the mdio-mux children node:
> > > > + - reg: 0 for internal MDIO bus, 1 for external MDIO bus
> > > > +
> > > > +The following compatibles require a PHY node representing the integrated
> > > > +PHY, under the integrated MDIO bus node if an mdio-mux node is used:
> > > > - "allwinner,sun8i-h3-emac",
> > > > - "allwinner,sun8i-v3s-emac":
> > > > +
> > > > +Required properties of the integrated phy node:
> > > > - clocks: a phandle to the reference clock for the EPHY
> > > > - resets: a phandle to the reset control for the EPHY
> > > > +- phy-is-integrated
> > > > +- Should be a child of the integrated mdio
> > >
> > > I'm not sure what you mean by that, you ask that it should (so not
> > > required?) be a child of the integrated mdio...
> > >
> >
> > I will change words to "must"
> >
> > > >
> > > > -Example:
> > > > -
> > > > +Example with integrated PHY:
> > > > emac: ethernet@1c0b000 {
> > > > compatible = "allwinner,sun8i-h3-emac";
> > > > syscon = <&syscon>;
> > > > @@ -72,13 +86,112 @@ emac: ethernet@1c0b000 {
> > > > phy-handle = <&int_mii_phy>;
> > > > phy-mode = "mii";
> > > > allwinner,leds-active-low;
> > > > +
> > > > + mdio0: mdio {
> > >
> > > (You don't label it mdio here, unlike what was asked before)
> > >
> > > > + #address-cells = <1>;
> > > > + #size-cells = <0>;
> > > > + compatible = "snps,dwmac-mdio";
> > > > + };
> > >
> > > I think Rob wanted that node gone?
> > >
> >
> > MDIO mux does not work without a parent MDIO, either gived by "parent-bus" or directly via mdio_mux_init() (like it is the case in dwmac-sun8i)
>
> Is the MDIO controller "allwinner,sun8i-h3-emac" or "snps,dwmac-mdio"?
> If the latter, then I think the node is fine, but then the mux should be
> a child node of it. IOW, the child of an MDIO controller should either
> be a mux node or slave devices.
>
It will be snps,dwmac-mdio but putting mdio-mux as a child of it (the mdio node) give me:
[ 18.175338] libphy: stmmac: probed
[ 18.175379] mdio_bus stmmac-0: /soc/ethernet@1c30000/mdio/mdio-mux has invalid PHY address
[ 18.175408] mdio_bus stmmac-0: scan phy mdio-mux at address 0
[ 18.175450] mdio_bus stmmac-0: scan phy mdio-mux at address 1
[ 18.175482] mdio_bus stmmac-0: scan phy mdio-mux at address 2
[ 18.175513] mdio_bus stmmac-0: scan phy mdio-mux at address 3
[ 18.175544] mdio_bus stmmac-0: scan phy mdio-mux at address 4
[ 18.175575] mdio_bus stmmac-0: scan phy mdio-mux at address 5
[ 18.175607] mdio_bus stmmac-0: scan phy mdio-mux at address 6
[ 18.175638] mdio_bus stmmac-0: scan phy mdio-mux at address 7
[ 18.175669] mdio_bus stmmac-0: scan phy mdio-mux at address 8
[ 18.175700] mdio_bus stmmac-0: scan phy mdio-mux at address 9
[ 18.175731] mdio_bus stmmac-0: scan phy mdio-mux at address 10
[ 18.175762] mdio_bus stmmac-0: scan phy mdio-mux at address 11
[ 18.175795] mdio_bus stmmac-0: scan phy mdio-mux at address 12
[ 18.175827] mdio_bus stmmac-0: scan phy mdio-mux at address 13
[ 18.175858] mdio_bus stmmac-0: scan phy mdio-mux at address 14
[ 18.175889] mdio_bus stmmac-0: scan phy mdio-mux at address 15
[ 18.175919] mdio_bus stmmac-0: scan phy mdio-mux at address 16
[ 18.175951] mdio_bus stmmac-0: scan phy mdio-mux at address 17
[ 18.175982] mdio_bus stmmac-0: scan phy mdio-mux at address 18
[ 18.176014] mdio_bus stmmac-0: scan phy mdio-mux at address 19
[ 18.176045] mdio_bus stmmac-0: scan phy mdio-mux at address 20
[ 18.176076] mdio_bus stmmac-0: scan phy mdio-mux at address 21
[ 18.176107] mdio_bus stmmac-0: scan phy mdio-mux at address 22
[ 18.176139] mdio_bus stmmac-0: scan phy mdio-mux at address 23
[ 18.176170] mdio_bus stmmac-0: scan phy mdio-mux at address 24
[ 18.176202] mdio_bus stmmac-0: scan phy mdio-mux at address 25
[ 18.176233] mdio_bus stmmac-0: scan phy mdio-mux at address 26
[ 18.176271] mdio_bus stmmac-0: scan phy mdio-mux at address 27
[ 18.176320] mdio_bus stmmac-0: scan phy mdio-mux at address 28
[ 18.176371] mdio_bus stmmac-0: scan phy mdio-mux at address 29
[ 18.176420] mdio_bus stmmac-0: scan phy mdio-mux at address 30
[ 18.176452] mdio_bus stmmac-0: scan phy mdio-mux at address 31
Adding a fake <reg> to mdio-mux remove it.
Does it is acceptable ? or perhaps patching of_mdiobus_register() to not scan node with compatible "mdio-mux".
> >
> > > > + mdio-mux {
> > > > + compatible = "mdio-mux";
> > > > + #address-cells = <1>;
> > > > + #size-cells = <0>;
> > > > +
> > > > + int_mdio: mdio@1 {
> > > > + reg = <0>;
>
> unit address of 1 and reg prop of 0 don't match. Build your dtb with
> W=2.
>
reg are arbitrary value (like mdio-mux-mmioreg), but in our case it is easy to fix this warning.
Thanks
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v5 05/10] dt-bindings: net: dwmac-sun8i: update documentation about integrated PHY
From: Andrew Lunn @ 2017-09-14 19:19 UTC (permalink / raw)
To: Corentin Labbe
Cc: Rob Herring, Maxime Ripard, mark.rutland, wens, linux,
catalin.marinas, will.deacon, peppe.cavallaro, alexandre.torgue,
f.fainelli, netdev, devicetree, linux-arm-kernel, linux-kernel
In-Reply-To: <20170914185301.GB4021@Red>
> > Is the MDIO controller "allwinner,sun8i-h3-emac" or "snps,dwmac-mdio"?
> > If the latter, then I think the node is fine, but then the mux should be
> > a child node of it. IOW, the child of an MDIO controller should either
> > be a mux node or slave devices.
Hi Rob
Up until now, children of an MDIO bus have been MDIO devices. Those
MDIO devices are either Ethernet PHYs, Ethernet Switches, or the
oddball devices that Broadcom iProc has, like generic PHYs.
We have never had MDIO-muxes as MDIO children. A Mux is not an MDIO
device, and does not have the properties of an MDIO device. It is not
addressable on the MDIO bus. The current MUXes are addressed via GPIOs
or MMIO.
There other similar cases. i2c-mux-gpio is not a child of an i2c bus,
nor i2c-mux-reg or gpio-mux. nxp,pca9548 is however a child of the i2c
bus, because it is an i2c device itself...
If the MDIO mux was an MDIO device, i would agree with you. Bit it is
not, so lets not make it a child.
Andrew
^ permalink raw reply
* feature request for kernel module 8021q
From: Pierre Colombier @ 2017-09-14 19:14 UTC (permalink / raw)
To: netdev
Hello
One big problem with vlans on linux is that the native interface always
get all vlans.
This can cause problem with bridges.
One interesting feature would be to have a special vlan number
(let's say 0 or -1 or 4097 ) designed so that it gets all the untagged
trafic and only the untagged trafic.
what do you think about this ?
^ permalink raw reply
* Re: [PATCH net-next v2 01/10] net: dsa: add debugfs interface
From: Maxim Uvarov @ 2017-09-14 19:59 UTC (permalink / raw)
To: Vivien Didelot
Cc: Greg KH, netdev, linux-kernel, kernel, David S. Miller,
Florian Fainelli, Andrew Lunn, Egil Hjelmeland, John Crispin,
Woojung Huh, Sean Wang, Nikita Yushchenko, Chris Healy
In-Reply-To: <87h8wdb8bj.fsf@weeman.i-did-not-set--mail-host-address--so-tickle-me>
debugfs here is very very useful to read registers directly and
compare what use space tools see. Cool feature to get regs by port and
use standard tools to diff and print them. Even might be better to
allow drivers to decode register names and bits values. Once that is
done driver mainaince will be much easy. I.e. you need only match regs
with spec from one side and regs with user space tools from other
side. Of course it's needed only for debuging, not for production. But
even for production regs dump on something wrong might tell a lot.
Maxim.
2017-09-08 16:58 GMT+03:00 Vivien Didelot <vivien.didelot@savoirfairelinux.com>:
> Hi Greg,
>
> Greg KH <gregkh@linuxfoundation.org> writes:
>
>> I agree you shouldn't be using debugfs for this, but in the future, if
>> you do write debugfs code, please take the following review into
>> account:
>
> Humm sorry I may not have given enough details. This was really meant
> for debug and dev only, because DSA makes it hard to query directly the
> hardware (some switch ports are not exposed to userspace as well.)
>
> This is not meant to be used for anything real at all, or even be
> compiled-in in a production kernel. That's why I found it appropriate.
>
> So I am still wondering why it doesn't fit here, can you tell me why?
>
>> You should _never_ care about the return value of a debugfs call, and
>> you should not need to ever propagate the error upward. The api was
>> written to not need this.
>>
>> Just call the function, and return, that's it. If you need to save the
>> return value (i.e. it's a dentry), you also don't care, just save it and
>> pass it to some other debugfs call, and all will still be fine. Your
>> code should never do anything different if a debugfs call succeeds or
>> fails.
>
> Thank for your interesting review! I'll cleanup my out-of-tree patches.
>
>
> Vivien
--
Best regards,
Maxim Uvarov
^ permalink raw reply
* Re: [PATCH net-next v2 01/10] net: dsa: add debugfs interface
From: Alexander Duyck @ 2017-09-14 20:12 UTC (permalink / raw)
To: Maxim Uvarov
Cc: Vivien Didelot, Greg KH, netdev, linux-kernel@vger.kernel.org,
kernel, David S. Miller, Florian Fainelli, Andrew Lunn,
Egil Hjelmeland, John Crispin, Woojung Huh, Sean Wang,
Nikita Yushchenko, Chris Healy
In-Reply-To: <CAJGZr0KdOPW5_V3kvWutwTY=BJhfd-=yRDPnvbDBOCuMYWj9OQ@mail.gmail.com>
On Thu, Sep 14, 2017 at 12:59 PM, Maxim Uvarov <muvarov@gmail.com> wrote:
> debugfs here is very very useful to read registers directly and
> compare what use space tools see. Cool feature to get regs by port and
> use standard tools to diff and print them. Even might be better to
> allow drivers to decode register names and bits values. Once that is
> done driver mainaince will be much easy. I.e. you need only match regs
> with spec from one side and regs with user space tools from other
> side. Of course it's needed only for debuging, not for production. But
> even for production regs dump on something wrong might tell a lot.
>
> Maxim.
Can you clarify what type of registers it is you are wanting to read?
We already have ethtool which is meant to allow reading the device
registers for a given netdev. As long as the port has a netdev
associated it then there is no need to be getting into debugfs since
we should probably just be using ethtool.
Also as Jiri pointed out there is already devlink which would probably
be a better way to get the associated information for those pieces
that don't have a netdev associated with them.
- Alex
^ permalink raw reply
* Re: Page allocator bottleneck
From: Andi Kleen @ 2017-09-14 20:19 UTC (permalink / raw)
To: Tariq Toukan
Cc: David Miller, Jesper Dangaard Brouer, Mel Gorman, Eric Dumazet,
Alexei Starovoitov, Saeed Mahameed, Eran Ben Elisha,
Linux Kernel Network Developers, Andrew Morton, Michal Hocko,
linux-mm
In-Reply-To: <cef85936-10b2-5d76-9f97-cb03b418fd94@mellanox.com>
Tariq Toukan <tariqt@mellanox.com> writes:
>
> Congestion in this case is very clear.
> When monitored in perf top:
> 85.58% [kernel] [k] queued_spin_lock_slowpath
Please look at the callers. Spinlock profiles without callers
are usually useless because it's just blaming the messenger.
Most likely the PCP lists are too small for your extreme allocation
rate, so it goes back too often to the shared pool.
You can play with the vm.percpu_pagelist_fraction setting.
-Andi
^ permalink raw reply
* Re: [PATCH net-next v2 01/10] net: dsa: add debugfs interface
From: Andrew Lunn @ 2017-09-14 21:01 UTC (permalink / raw)
To: Alexander Duyck
Cc: Maxim Uvarov, Vivien Didelot, Greg KH, netdev,
linux-kernel@vger.kernel.org, kernel, David S. Miller,
Florian Fainelli, Egil Hjelmeland, John Crispin, Woojung Huh,
Sean Wang, Nikita Yushchenko, Chris Healy
In-Reply-To: <CAKgT0Uf8--Yh9tfkTrh9_dfEYMX5jR6j4unxwxM+WD6E4jk9pA@mail.gmail.com>
> Can you clarify what type of registers it is you are wanting to read?
> We already have ethtool which is meant to allow reading the device
> registers for a given netdev. As long as the port has a netdev
> associated it then there is no need to be getting into debugfs since
> we should probably just be using ethtool.
Not all ports of a DSA switch have a netdev. This is by design. The
presentation we gave to Netdev 2.1 gives some of the background.
Plus a switch has a lot of registers not associated to port. Often a
switch has more global registers than port registers.
> Also as Jiri pointed out there is already devlink which would probably
> be a better way to get the associated information for those pieces
> that don't have a netdev associated with them.
We have looked at the devlink a few times. The current dpipe code is
not generic enough. It makes assumptions about the architecture of the
switch, that it is all match/action based. The niche of top of rack
switches might be like that, but average switches are not.
If dpipe was to support simple generic two dimensional tables, we
probably would use it.
David suggested making a class device for DSA. It is not ideal, but we
are probably going to go that way.
Andrew
^ permalink raw reply
* [PATCH net] packet: hold bind lock when rebinding to fanout hook
From: Willem de Bruijn @ 2017-09-14 21:14 UTC (permalink / raw)
To: netdev; +Cc: davem, nixiaoming, Willem de Bruijn
Packet socket bind operations must hold the po->bind_lock. This keeps
po->running consistent with whether the socket is actually on a ptype
list to receive packets.
fanout_add unbinds a socket and its packet_rcv/tpacket_rcv call, then
binds the fanout object to receive through packet_rcv_fanout.
Make it hold the po->bind_lock when testing po->running and rebinding.
Else, it can race with other rebind operations, such as that in
packet_set_ring from packet_rcv to tpacket_rcv. Concurrent updates
can result in a socket being added to a fanout group twice, causing
use-after-free KASAN bug reports, among others.
Reported independently by both trinity and syzkaller.
Verified that the syzkaller reproducer passes after this patch.
Reported-by: nixioaming <nixiaoming@huawei.com>
Signed-off-by: Willem de Bruijn <willemb@google.com>
---
net/packet/af_packet.c | 16 +++++++++++-----
1 file changed, 11 insertions(+), 5 deletions(-)
diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
index c26172995511..d288f52c53f7 100644
--- a/net/packet/af_packet.c
+++ b/net/packet/af_packet.c
@@ -1684,10 +1684,6 @@ static int fanout_add(struct sock *sk, u16 id, u16 type_flags)
mutex_lock(&fanout_mutex);
- err = -EINVAL;
- if (!po->running)
- goto out;
-
err = -EALREADY;
if (po->fanout)
goto out;
@@ -1749,7 +1745,10 @@ static int fanout_add(struct sock *sk, u16 id, u16 type_flags)
list_add(&match->list, &fanout_list);
}
err = -EINVAL;
- if (match->type == type &&
+
+ spin_lock(&po->bind_lock);
+ if (po->running &&
+ match->type == type &&
match->prot_hook.type == po->prot_hook.type &&
match->prot_hook.dev == po->prot_hook.dev) {
err = -ENOSPC;
@@ -1761,6 +1760,13 @@ static int fanout_add(struct sock *sk, u16 id, u16 type_flags)
err = 0;
}
}
+ spin_unlock(&po->bind_lock);
+
+ if (err && !refcount_read(&match->sk_ref)) {
+ list_del(&match->list);
+ kfree(match);
+ }
+
out:
if (err && rollover) {
kfree(rollover);
--
2.14.1.690.gbb1197296e-goog
^ permalink raw reply related
* Re: Bug with BPF_ALU64 | BPF_END?
From: Y Song @ 2017-09-14 21:22 UTC (permalink / raw)
To: David Miller; +Cc: Edward Cree, Daniel Borkmann, Alexei Starovoitov, netdev
In-Reply-To: <20170914.111424.934672262035736390.davem@davemloft.net>
On Thu, Sep 14, 2017 at 11:14 AM, David Miller <davem@davemloft.net> wrote:
> From: Edward Cree <ecree@solarflare.com>
> Date: Thu, 14 Sep 2017 18:53:17 +0100
>
>> Is BPF_END supposed to only be used with BPF_ALU, never with BPF_ALU64?
Yes, only BPF_ALU. The below is LLVM bpf swap insn encoding:
...
// bswap16, bswap32, bswap64
class BSWAP ...
...
let op = 0xd; // BPF_END
let BPFSrc = 1; // BPF_TO_BE (TODO: use BPF_TO_LE for big-endian target)
let BPFClass = 4; // BPF_ALU
...
>> In kernel/bpf/core.c:___bpf_prog_run(), there are only jump table targets
>> for the BPF_ALU case, not for the BPF_ALU64 case (opcodes 0xd7 and 0xdf).
>> But the verifier doesn't enforce this; by crafting a program that uses
>> these opcodes I can get a WARN when they're run (without JIT; it looks
>> like the x86 JIT, at least, won't like it either).
>> Proposed patch below the cut; build-tested only.
>
> Good catch.
>
> A really neat test would be a program that uploads random BPF programs
> into the kernel, in a syzkaller'ish way. It might have triggered this
> (eventually).
>
^ permalink raw reply
* Re: feature request for kernel module 8021q
From: Florian Fainelli @ 2017-09-14 22:16 UTC (permalink / raw)
To: Pierre Colombier, netdev
In-Reply-To: <c010d65d-3050-8d88-22aa-cac092dd3904@abeille.com>
On September 14, 2017 12:14:04 PM PDT, Pierre Colombier <pierre@abeille.com> wrote:
>Hello
>
>
>One big problem with vlans on linux is that the native interface always
>
>get all vlans.
>
>This can cause problem with bridges.
What specific problem are you thinking about?
>
>
>One interesting feature would be to have a special vlan number
>
>(let's say 0 or -1 or 4097 ) designed so that it gets all the untagged
>trafic and only the untagged trafic.
4097 is not a valid number nor is -1 since that closely follows what the VLAN ID extracted from the tag would be. VLAN 0 is more or less the untagged VLAN.
>
>what do you think about this ?
Provided the HW supports it, with two netdevice operations: ndo_rx_vlan_add_vid and ndo_rx_vlan_kill_vid you can have your HW filter out the VLAN tags it wishes to receive, respectively not receive anymore. If your device does not support that, then yes if you send frames with a VLAN tag for which you are not interested in processing, you are still receiving it unless there is a switch or any kind of VLAN filtering device in between.
Bridge has also gained a VLAN filtering that can also be used to only the reception/transmission of desired VLAN tags. And it also allows you to define the default VLAN tag (VID1)
--
Florian
^ permalink raw reply
* Re: [PATCH net] packet: hold bind lock when rebinding to fanout hook
From: Eric Dumazet @ 2017-09-14 22:53 UTC (permalink / raw)
To: Willem de Bruijn; +Cc: netdev, davem, nixiaoming
In-Reply-To: <20170914211441.67326-1-willemb@google.com>
On Thu, 2017-09-14 at 17:14 -0400, Willem de Bruijn wrote:
> Packet socket bind operations must hold the po->bind_lock. This keeps
> po->running consistent with whether the socket is actually on a ptype
> list to receive packets.
>
> fanout_add unbinds a socket and its packet_rcv/tpacket_rcv call, then
> binds the fanout object to receive through packet_rcv_fanout.
>
> Make it hold the po->bind_lock when testing po->running and rebinding.
> Else, it can race with other rebind operations, such as that in
> packet_set_ring from packet_rcv to tpacket_rcv. Concurrent updates
> can result in a socket being added to a fanout group twice, causing
> use-after-free KASAN bug reports, among others.
>
> Reported independently by both trinity and syzkaller.
> Verified that the syzkaller reproducer passes after this patch.
>
> Reported-by: nixioaming <nixiaoming@huawei.com>
> Signed-off-by: Willem de Bruijn <willemb@google.com>
> ---
> net/packet/af_packet.c | 16 +++++++++++-----
> 1 file changed, 11 insertions(+), 5 deletions(-)
>
> diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
> index c26172995511..d288f52c53f7 100644
> --- a/net/packet/af_packet.c
> +++ b/net/packet/af_packet.c
> @@ -1684,10 +1684,6 @@ static int fanout_add(struct sock *sk, u16 id, u16 type_flags)
>
> mutex_lock(&fanout_mutex);
>
> - err = -EINVAL;
> - if (!po->running)
> - goto out;
> -
> err = -EALREADY;
> if (po->fanout)
> goto out;
> @@ -1749,7 +1745,10 @@ static int fanout_add(struct sock *sk, u16 id, u16 type_flags)
> list_add(&match->list, &fanout_list);
> }
> err = -EINVAL;
> - if (match->type == type &&
> +
> + spin_lock(&po->bind_lock);
> + if (po->running &&
> + match->type == type &&
> match->prot_hook.type == po->prot_hook.type &&
> match->prot_hook.dev == po->prot_hook.dev) {
> err = -ENOSPC;
> @@ -1761,6 +1760,13 @@ static int fanout_add(struct sock *sk, u16 id, u16 type_flags)
> err = 0;
> }
> }
> + spin_unlock(&po->bind_lock);
> +
> + if (err && !refcount_read(&match->sk_ref)) {
It seems sk_ref is always read/changed under
mutex_lock(&fanout_mutex) protection.
Not sure why we use a refcount_t (or an atomic_t in older kernels)
All these atomic/spinlock/mutexes are a maze.
> + list_del(&match->list);
> + kfree(match);
> + }
> +
> out:
> if (err && rollover) {
> kfree(rollover);
^ permalink raw reply
* Re: Bug with BPF_ALU64 | BPF_END?
From: Daniel Borkmann @ 2017-09-14 23:04 UTC (permalink / raw)
To: Edward Cree, Alexei Starovoitov; +Cc: netdev
In-Reply-To: <52c7df55-84d9-f6d2-ed84-51ac90eb6bcc@solarflare.com>
On 09/14/2017 07:53 PM, Edward Cree wrote:
> Is BPF_END supposed to only be used with BPF_ALU, never with BPF_ALU64?
> In kernel/bpf/core.c:___bpf_prog_run(), there are only jump table targets
> for the BPF_ALU case, not for the BPF_ALU64 case (opcodes 0xd7 and 0xdf).
> But the verifier doesn't enforce this; by crafting a program that uses
> these opcodes I can get a WARN when they're run (without JIT; it looks
> like the x86 JIT, at least, won't like it either).
> Proposed patch below the cut; build-tested only.
>
> -Ed
> ---
>
> [PATCH net] bpf/verifier: reject BPF_ALU64|BPF_END
>
> Neither ___bpf_prog_run nor the JITs accept it.
>
> Fixes: 17a5267067f3 ("bpf: verifier (add verifier core)")
> Signed-off-by: Edward Cree <ecree@solarflare.com>
Good catch! Can you submit this as an official patch for -net together
with a test case for tools/testing/selftests/bpf/test_verifier.c?
Thanks!
Acked-by: Daniel Borkmann <daniel@iogearbox.net>
> ---
> kernel/bpf/verifier.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 477b693..799b245 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -2292,7 +2292,8 @@ static int check_alu_op(struct bpf_verifier_env *env, struct bpf_insn *insn)
> }
> } else {
> if (insn->src_reg != BPF_REG_0 || insn->off != 0 ||
> - (insn->imm != 16 && insn->imm != 32 && insn->imm != 64)) {
> + (insn->imm != 16 && insn->imm != 32 && insn->imm != 64) ||
> + BPF_CLASS(insn->code) == BPF_ALU64) {
> verbose("BPF_END uses reserved fields\n");
> return -EINVAL;
> }
>
^ permalink raw reply
* RE: [PATCH] e1000e: apply burst mode settings only on default
From: Brown, Aaron F @ 2017-09-14 23:48 UTC (permalink / raw)
To: Willem de Bruijn, Kirsher, Jeffrey T
Cc: intel-wired-lan@lists.osuosl.org, netdev@vger.kernel.org,
Brandeburg, Jesse, Willem de Bruijn
In-Reply-To: <20170825150626.2843-1-willemdebruijn.kernel@gmail.com>
> From: netdev-owner@vger.kernel.org [mailto:netdev-
> owner@vger.kernel.org] On Behalf Of Willem de Bruijn
> Sent: Friday, August 25, 2017 8:06 AM
> To: Kirsher, Jeffrey T <jeffrey.t.kirsher@intel.com>
> Cc: intel-wired-lan@lists.osuosl.org; netdev@vger.kernel.org; Brandeburg,
> Jesse <jesse.brandeburg@intel.com>; Willem de Bruijn
> <willemb@google.com>
> Subject: [PATCH] e1000e: apply burst mode settings only on default
>
> From: Willem de Bruijn <willemb@google.com>
>
> Devices that support FLAG2_DMA_BURST have different default values
> for RDTR and RADV. Apply burst mode default settings only when no
> explicit value was passed at module load.
>
> The RDTR default is zero. If the module is loaded for low latency
> operation with RxIntDelay=0, do not override this value with a burst
> default of 32.
>
> Move the decision to apply burst values earlier, where explicitly
> initialized module variables can be distinguished from defaults.
>
> Signed-off-by: Willem de Bruijn <willemb@google.com>
> ---
> drivers/net/ethernet/intel/e1000e/e1000.h | 4 ----
> drivers/net/ethernet/intel/e1000e/netdev.c | 8 --------
> drivers/net/ethernet/intel/e1000e/param.c | 16 +++++++++++++++-
> 3 files changed, 15 insertions(+), 13 deletions(-)
Tested-by: Aaron Brown <aaron.f.brown@intel.com>
^ permalink raw reply
* RE: [Intel-wired-lan] [PATCH net-next v3] e1000e: Be drop monitor friendly
From: Brown, Aaron F @ 2017-09-14 23:54 UTC (permalink / raw)
To: Florian Fainelli, netdev@vger.kernel.org
Cc: edumazet@gmail.com, open list,
moderated list:INTEL ETHERNET DRIVERS, davem@davemloft.net
In-Reply-To: <20170826011424.27251-1-f.fainelli@gmail.com>
> From: Intel-wired-lan [mailto:intel-wired-lan-bounces@osuosl.org] On Behalf
> Of Florian Fainelli
> Sent: Friday, August 25, 2017 6:14 PM
> To: netdev@vger.kernel.org
> Cc: edumazet@gmail.com; Florian Fainelli <f.fainelli@gmail.com>; open list
> <linux-kernel@vger.kernel.org>; moderated list:INTEL ETHERNET DRIVERS
> <intel-wired-lan@lists.osuosl.org>; davem@davemloft.net
> Subject: [Intel-wired-lan] [PATCH net-next v3] e1000e: Be drop monitor
> friendly
>
> e1000e_put_txbuf() can be called from normal reclamation path as well as
> when a DMA mapping failure, so we need to differentiate these two cases
> when freeing SKBs to be drop monitor friendly. e1000e_tx_hwtstamp_work()
> and e1000_remove() are processing TX timestamped SKBs and those should
> not be accounted as drops either.
>
> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
> ---
> Changes in v3:
>
> - differentiate normal reclamation from TX DMA fragment mapping errors
> - removed a few invalid dev_kfree_skb() replacements (those are already
> drop monitor friendly)
>
> Changes in v2:
>
> - make it compile
>
> drivers/net/ethernet/intel/e1000e/netdev.c | 18 +++++++++++-------
> 1 file changed, 11 insertions(+), 7 deletions(-)
Tested-by: Aaron Brown <aaron.f.brown@intel.com>
^ permalink raw reply
* RE: [Intel-wired-lan] [PATCH 1/5] e1000e: Fix error path in link detection
From: Brown, Aaron F @ 2017-09-15 0:18 UTC (permalink / raw)
To: Benjamin Poirier, Kirsher, Jeffrey T
Cc: netdev@vger.kernel.org, intel-wired-lan@lists.osuosl.org,
linux-kernel@vger.kernel.org, Lennart Sorensen
In-Reply-To: <20170721183627.13373-1-bpoirier@suse.com>
> From: Intel-wired-lan [mailto:intel-wired-lan-bounces@osuosl.org] On Behalf
> Of Benjamin Poirier
> Sent: Friday, July 21, 2017 11:36 AM
> To: Kirsher, Jeffrey T <jeffrey.t.kirsher@intel.com>
> Cc: netdev@vger.kernel.org; intel-wired-lan@lists.osuosl.org; linux-
> kernel@vger.kernel.org; Lennart Sorensen <lsorense@csclub.uwaterloo.ca>
> Subject: [Intel-wired-lan] [PATCH 1/5] e1000e: Fix error path in link detection
>
> In case of error from e1e_rphy(), the loop will exit early and "success"
> will be set to true erroneously.
>
> Signed-off-by: Benjamin Poirier <bpoirier@suse.com>
> ---
> drivers/net/ethernet/intel/e1000e/phy.c | 7 ++++---
> 1 file changed, 4 insertions(+), 3 deletions(-)
>
Tested-by: Aaron Brown <aaron.f.brown@intel.com>
^ permalink raw reply
* RE: [Intel-wired-lan] [PATCH 3/5] e1000e: Fix return value test
From: Brown, Aaron F @ 2017-09-15 0:20 UTC (permalink / raw)
To: Benjamin Poirier, Kirsher, Jeffrey T
Cc: netdev@vger.kernel.org, intel-wired-lan@lists.osuosl.org,
linux-kernel@vger.kernel.org, Lennart Sorensen
In-Reply-To: <20170721183627.13373-3-bpoirier@suse.com>
> From: Intel-wired-lan [mailto:intel-wired-lan-bounces@osuosl.org] On Behalf
> Of Benjamin Poirier
> Sent: Friday, July 21, 2017 11:36 AM
> To: Kirsher, Jeffrey T <jeffrey.t.kirsher@intel.com>
> Cc: netdev@vger.kernel.org; intel-wired-lan@lists.osuosl.org; linux-
> kernel@vger.kernel.org; Lennart Sorensen <lsorense@csclub.uwaterloo.ca>
> Subject: [Intel-wired-lan] [PATCH 3/5] e1000e: Fix return value test
>
> All the helpers return -E1000_ERR_PHY.
>
> Signed-off-by: Benjamin Poirier <bpoirier@suse.com>
> ---
> drivers/net/ethernet/intel/e1000e/netdev.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
Tested-by: Aaron Brown <aaron.f.brown@intel.com>
^ permalink raw reply
* RE: [Intel-wired-lan] [PATCH 4/5] e1000e: Separate signaling for link check/link up
From: Brown, Aaron F @ 2017-09-15 0:27 UTC (permalink / raw)
To: Benjamin Poirier
Cc: netdev@vger.kernel.org, intel-wired-lan@lists.osuosl.org,
linux-kernel@vger.kernel.org, Lennart Sorensen
In-Reply-To: <20170802144922.txmee23d35o4r7mh@f1.synalogic.ca>
On 7/21/2017 21:36, Benjamin Poirier wrote:
> Lennart reported the following race condition:
>
> \ e1000_watchdog_task
> \ e1000e_has_link
> \ hw->mac.ops.check_for_link() === e1000e_check_for_copper_link
> /* link is up */
> mac->get_link_status = false;
>
> /* interrupt */
> \ e1000_msix_other
> hw->mac.get_link_status = true;
>
> link_active = !hw->mac.get_link_status
> /* link_active is false, wrongly */
>
> This problem arises because the single flag get_link_status is used to
> signal two different states: link status needs checking and link status is
> down.
>
> Avoid the problem by using the return value of .check_for_link to signal
> the link status to e1000e_has_link().
>
> Reported-by: Lennart Sorensen <lsorense@csclub.uwaterloo.ca>
> Signed-off-by: Benjamin Poirier <bpoirier@suse.com>
> ---
> drivers/net/ethernet/intel/e1000e/mac.c | 11 ++++++++---
> drivers/net/ethernet/intel/e1000e/netdev.c | 2 +-
> 2 files changed, 9 insertions(+), 4 deletions(-)
Tested-by: Aaron Brown <aaron.f.brown@intel.com>
^ permalink raw reply
* RE: [Intel-wired-lan] [PATCH 5/5] e1000e: Avoid receiver overrun interrupt bursts
From: Brown, Aaron F @ 2017-09-15 0:38 UTC (permalink / raw)
To: Benjamin Poirier, Kirsher, Jeffrey T
Cc: netdev@vger.kernel.org, intel-wired-lan@lists.osuosl.org,
linux-kernel@vger.kernel.org, Lennart Sorensen
In-Reply-To: <20170721183627.13373-5-bpoirier@suse.com>
> From: Intel-wired-lan [mailto:intel-wired-lan-bounces@osuosl.org] On Behalf
> Of Benjamin Poirier
> Sent: Friday, July 21, 2017 11:36 AM
> To: Kirsher, Jeffrey T <jeffrey.t.kirsher@intel.com>
> Cc: netdev@vger.kernel.org; intel-wired-lan@lists.osuosl.org; linux-
> kernel@vger.kernel.org; Lennart Sorensen <lsorense@csclub.uwaterloo.ca>
> Subject: [Intel-wired-lan] [PATCH 5/5] e1000e: Avoid receiver overrun
> interrupt bursts
>
> When e1000e_poll() is not fast enough to keep up with incoming traffic, the
> adapter (when operating in msix mode) raises the Other interrupt to signal
> Receiver Overrun.
>
> This is a double problem because 1) at the moment e1000_msix_other()
> assumes that it is only called in case of Link Status Change and 2) if the
> condition persists, the interrupt is repeatedly raised again in quick
> succession.
>
> Ideally we would configure the Other interrupt to not be raised in case of
> receiver overrun but this doesn't seem possible on this adapter. Instead,
> we handle the first part of the problem by reverting to the practice of
> reading ICR in the other interrupt handler, like before commit 16ecba59bc33
> ("e1000e: Do not read ICR in Other interrupt"). Thanks to commit
> 0a8047ac68e5 ("e1000e: Fix msi-x interrupt automask") which cleared IAME
> from CTRL_EXT, reading ICR doesn't interfere with RxQ0, TxQ0 interrupts
> anymore. We handle the second part of the problem by not re-enabling the
> Other interrupt right away when there is overrun. Instead, we wait until
> traffic subsides, napi polling mode is exited and interrupts are
> re-enabled.
>
> Reported-by: Lennart Sorensen <lsorense@csclub.uwaterloo.ca>
> Fixes: 16ecba59bc33 ("e1000e: Do not read ICR in Other interrupt")
> Signed-off-by: Benjamin Poirier <bpoirier@suse.com>
> ---
> drivers/net/ethernet/intel/e1000e/defines.h | 1 +
> drivers/net/ethernet/intel/e1000e/netdev.c | 33
> +++++++++++++++++++++++------
> 2 files changed, 27 insertions(+), 7 deletions(-)
>
I get an error and a few warnings out of checkpatch from this, but I think the error is false (thinking the reference to a commit in the description is this commit, a fixes commit or something like that) and I'm more concerned with the fix than the warnings, so...
Tested-by: Aaron Brown <aaron.f.brown@intel.com>
Here is the checkpatch output in case anyone has a different opinion on the severity:
-------------
u1484:[0]/usr/src/kernels/next-queue> git format-patch d81d1e6 -1 --stdout|./scripts/checkpatch.pl -
ERROR: Please use git commit description style 'commit <12+ chars of sha1> ("<title line>")' - ie: 'commit 0a8047ac68e5 ("e1000e: Fix msi-x interrupt automask")'
#20:
0a8047ac68e5 ("e1000e: Fix msi-x interrupt automask") which cleared IAME
WARNING: braces {} are not necessary for single statement blocks
#73: FILE: drivers/net/ethernet/intel/e1000e/netdev.c:1931:
+ if (!test_bit(__E1000_DOWN, &adapter->state)) {
+ mod_timer(&adapter->watchdog_timer, jiffies + 1);
+ }
WARNING: braces {} are not necessary for single statement blocks
#83: FILE: drivers/net/ethernet/intel/e1000e/netdev.c:1936:
+ if (enable && !test_bit(__E1000_DOWN, &adapter->state)) {
ew32(IMS, E1000_IMS_OTHER);
}
total: 1 errors, 2 warnings, 0 checks, 59 lines checked
NOTE: For some of the reported defects, checkpatch may be able to
mechanically convert to the typical style using --fix or --fix-inplace.
Your patch has style problems, please review.
NOTE: If any of the errors are false positives, please report
them to the maintainer, see CHECKPATCH in MAINTAINERS.
u1484:[0]/usr/src/kernels/next-queue>
^ 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