* Re: [PATCH] net: sched: Fix memory exposure from short TCA_U32_SEL
From: Roman Mashak @ 2018-08-27 14:26 UTC (permalink / raw)
To: Kees Cook
Cc: Jamal Hadi Salim, Al Viro, LKML, Cong Wang, Jiri Pirko,
David S. Miller, Network Development
In-Reply-To: <CAGXu5jJD++hegH4j0ajZNejg8mgcHjqVyjveWj9BFaXwRCV_dQ@mail.gmail.com>
Kees Cook <keescook@chromium.org> writes:
> On Mon, Aug 27, 2018 at 4:46 AM, Jamal Hadi Salim <jhs@mojatatu.com> wrote:
>> On 2018-08-26 5:56 p.m., Kees Cook wrote:
>>>
>>> On Sun, Aug 26, 2018 at 10:30 AM, Jamal Hadi Salim <jhs@mojatatu.com>
>>> wrote:
>>>>
>>>> We should add an nla_policy later.
>>>
>>>
>>> What's the right way to do that for cases like this?
>>
>>
>> Meant something like attached which you alluded-to in your comments
>> would give an upper bound (Max allowed keys is 128).
>
> The problem is that policy doesn't parse the contents: "nkeys"
> determines the size, so we have to both validate minimum size (to be
> sure the location of "nkeys" is valid) and check that the size is at
> least nkeys * struct long. I don't think there is a way to do this
> with the existing policy language.
While at these changes, could you also add and export in UAPI max
allowed keys count, which is currently 128? For example,
TCA_U32_NKEYS_MAX in pkt_cls.h
^ permalink raw reply
* [PATCH 0/4] clk-pmc-atom + r8169: Add ether_clk handling to fix suspend issues
From: Hans de Goede @ 2018-08-27 14:31 UTC (permalink / raw)
To: David S . Miller, Heiner Kallweit, Michael Turquette,
Stephen Boyd, Andy Shevchenko, Irina Tirdea
Cc: Hans de Goede, netdev, Johannes Stezenbach, Carlo Caione,
linux-clk
Hi All,
This series has as goal to revert commit d31fd43c0f9a ("clk: x86: Do not gate
clocks enabled by the firmware"), because that commit causes almost all
Cherry Trail devices to not use the S0i3 powerstate when suspending, leading
to them quickly draining their battery when suspended.
This commit was added to fix issues with the r8169 NIC on some Bay Trail
devices, where the pmc_plt_clk_4 was used as clk for the r8169 chip.
This series fixes this properly, so that we can undo the trouble some
commit.
The first patch adds an "ether_clk" alias to the clk-pmc-atom driver so
that the r8169 can pass "ether_clk" as generic id to clk_get instead of
having to add x86 specific code to the r8169 driver.
The second patch makes the r8169 driver do a clk_get for "ether_clk"
(ignoring -ENOENT errors so this is optional) and if that succeeds then
it enables the clock.
The third patch effectively revert the troublesome commit.
This series has been tested on a HP Stream x360 - 11-p000nd, which uses
a Bay Trail SoC with a r8169 ethernet chip and I can confirm that the
pmc_plt_clk_4 gets properly enabled, so this series should not cause any
regressions wrt devices needing pmc_plt_clk_4 enabled (which is not the
case on the Stream x360).
To avoid regressions we need to have patches 1 and 2 merged before 3,
so it is probably best if this entire series gets merged through a single
tree. Given that clk-pmc-atom.c has not seen any changes for over a
year I suggest that all 3 patches are merged through the netdev tree,
with an ack from the clk maintainers. Assuming that is ok with the clk
maintainers of course.
Note there is a fourth patch in this series, this patch is necessary to
reach S0i3 state on Bay Trail devices wich have a r8169 ethernet chip,
since I do not have access to hardware where the r8169 actually needs
the pmc_plt_clk_4 I have not been able to test this, hence it is marked
as RFC for now.
Carlos can you test the 4th patch (when you have time) and let us know if
it works?
Regards,
Hans
^ permalink raw reply
* [PATCH 1/4] clk: x86: add "ether_clk" alias for Bay Trail / Cherry Trail
From: Hans de Goede @ 2018-08-27 14:31 UTC (permalink / raw)
To: David S . Miller, Heiner Kallweit, Michael Turquette,
Stephen Boyd, Andy Shevchenko, Irina Tirdea
Cc: Hans de Goede, netdev, Johannes Stezenbach, Carlo Caione,
linux-clk
In-Reply-To: <20180827143200.8597-1-hdegoede@redhat.com>
Commit d31fd43c0f9a ("clk: x86: Do not gate clocks enabled by the
firmware") causes all unclaimed PMC clocks on Cherry Trail devices to be on
all the time, resulting on the device not being able to reach S0i2 or S0i3
when suspended.
The reason for this commit is that on some Bay Trail / Cherry Trail devices
the ethernet controller uses pmc_plt_clk_4. This commit adds an "ether_clk"
alias, so that the relevant ethernet drivers can try to (optionally) use
this, without needing X86 specific code / hacks, thus fixing ethernet on
these devices without breaking S0i3 support.
This commit uses clkdev_hw_create() to create the alias, mirroring the code
for the already existing "mclk" alias for pmc_plt_clk_3.
Buglink: https://bugzilla.kernel.org/show_bug.cgi?id=193891#c102
Cc: Johannes Stezenbach <js@sig21.net>
Cc: Carlo Caione <carlo@endlessm.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
drivers/clk/x86/clk-pmc-atom.c | 11 +++++++++++
1 file changed, 11 insertions(+)
diff --git a/drivers/clk/x86/clk-pmc-atom.c b/drivers/clk/x86/clk-pmc-atom.c
index 08ef69945ffb..75151901ff7d 100644
--- a/drivers/clk/x86/clk-pmc-atom.c
+++ b/drivers/clk/x86/clk-pmc-atom.c
@@ -55,6 +55,7 @@ struct clk_plt_data {
u8 nparents;
struct clk_plt *clks[PMC_CLK_NUM];
struct clk_lookup *mclk_lookup;
+ struct clk_lookup *ether_clk_lookup;
};
/* Return an index in parent table */
@@ -351,11 +352,20 @@ static int plt_clk_probe(struct platform_device *pdev)
goto err_unreg_clk_plt;
}
+ data->ether_clk_lookup = clkdev_hw_create(&data->clks[4]->hw,
+ "ether_clk", NULL);
+ if (!data->ether_clk_lookup) {
+ err = -ENOMEM;
+ goto err_drop_mclk;
+ }
+
plt_clk_free_parent_names_loop(parent_names, data->nparents);
platform_set_drvdata(pdev, data);
return 0;
+err_drop_mclk:
+ clkdev_drop(data->mclk_lookup);
err_unreg_clk_plt:
plt_clk_unregister_loop(data, i);
plt_clk_unregister_parents(data);
@@ -369,6 +379,7 @@ static int plt_clk_remove(struct platform_device *pdev)
data = platform_get_drvdata(pdev);
+ clkdev_drop(data->ether_clk_lookup);
clkdev_drop(data->mclk_lookup);
plt_clk_unregister_loop(data, PMC_CLK_NUM);
plt_clk_unregister_parents(data);
--
2.18.0
^ permalink raw reply related
* [PATCH 2/4] r8169: Get and enable optional ether_clk clock
From: Hans de Goede @ 2018-08-27 14:31 UTC (permalink / raw)
To: David S . Miller, Heiner Kallweit, Michael Turquette,
Stephen Boyd, Andy Shevchenko, Irina Tirdea
Cc: Hans de Goede, netdev, Johannes Stezenbach, Carlo Caione,
linux-clk
In-Reply-To: <20180827143200.8597-1-hdegoede@redhat.com>
On some boards a platform clock is used as clock for the r8169 chip,
this commit adds support for getting and enabling this clock (assuming
it has an "ether_clk" alias set on it).
This is related to commit d31fd43c0f9a ("clk: x86: Do not gate clocks
enabled by the firmware") which is a previous attempt to fix this for some
x86 boards, but this causes all Cherry Trail SoC using boards to not reach
there lowest power states when suspending.
This commit (together with an atom-pmc-clk driver commit adding the alias)
fixes things properly by making the r8169 get the clock and enable it when
it needs it.
Buglink: https://bugzilla.kernel.org/show_bug.cgi?id=193891#c102
Cc: Johannes Stezenbach <js@sig21.net>
Cc: Carlo Caione <carlo@endlessm.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
drivers/net/ethernet/realtek/r8169.c | 33 ++++++++++++++++++++++++++++
1 file changed, 33 insertions(+)
diff --git a/drivers/net/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek/r8169.c
index eaedc11ed686..779b02979493 100644
--- a/drivers/net/ethernet/realtek/r8169.c
+++ b/drivers/net/ethernet/realtek/r8169.c
@@ -13,6 +13,7 @@
#include <linux/pci.h>
#include <linux/netdevice.h>
#include <linux/etherdevice.h>
+#include <linux/clk.h>
#include <linux/delay.h>
#include <linux/ethtool.h>
#include <linux/mii.h>
@@ -765,6 +766,7 @@ struct rtl8169_private {
u16 event_slow;
const struct rtl_coalesce_info *coalesce_info;
+ struct clk *clk;
struct mdio_ops {
void (*write)(struct rtl8169_private *, int, int);
@@ -7614,6 +7616,11 @@ static void rtl_hw_initialize(struct rtl8169_private *tp)
}
}
+static void rtl_disable_clk(void *data)
+{
+ clk_disable_unprepare(data);
+}
+
static int rtl_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
{
const struct rtl_cfg_info *cfg = rtl_cfg_infos + ent->driver_data;
@@ -7647,6 +7654,32 @@ static int rtl_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
mii->reg_num_mask = 0x1f;
mii->supports_gmii = cfg->has_gmii;
+ /* Get the *optional* external "ether_clk" used on some boards */
+ tp->clk = devm_clk_get(&pdev->dev, "ether_clk");
+ if (IS_ERR(tp->clk)) {
+ rc = PTR_ERR(tp->clk);
+ if (rc == -ENOENT) {
+ /* clk-core allows NULL (for suspend / resume) */
+ tp->clk = NULL;
+ } else if (rc == -EPROBE_DEFER) {
+ return rc;
+ } else {
+ dev_err(&pdev->dev, "failed to get clk: %d\n", rc);
+ return rc;
+ }
+ } else {
+ rc = clk_prepare_enable(tp->clk);
+ if (rc) {
+ dev_err(&pdev->dev, "failed to enable clk: %d\n", rc);
+ return rc;
+ }
+
+ rc = devm_add_action_or_reset(&pdev->dev, rtl_disable_clk,
+ tp->clk);
+ if (rc)
+ return rc;
+ }
+
/* disable ASPM completely as that cause random device stop working
* problems as well as full system hangs for some PCIe devices users */
pci_disable_link_state(pdev, PCIE_LINK_STATE_L0S | PCIE_LINK_STATE_L1 |
--
2.18.0
^ permalink raw reply related
* [PATCH 3/4] clk: x86: Stop marking clocks as CLK_IS_CRITICAL
From: Hans de Goede @ 2018-08-27 14:31 UTC (permalink / raw)
To: David S . Miller, Heiner Kallweit, Michael Turquette,
Stephen Boyd, Andy Shevchenko, Irina Tirdea
Cc: Hans de Goede, netdev, Johannes Stezenbach, Carlo Caione,
linux-clk
In-Reply-To: <20180827143200.8597-1-hdegoede@redhat.com>
Commit d31fd43c0f9a ("clk: x86: Do not gate clocks enabled by the
firmware"), which added the code to mark clocks as CLK_IS_CRITICAL, causes
all unclaimed PMC clocks on Cherry Trail devices to be on all the time,
resulting on the device not being able to reach S0i3 when suspended.
The reason for this commit is that on some Bay Trail / Cherry Trail devices
the r8169 ethernet controller uses pmc_plt_clk_4. Now that the clk-pmc-atom
driver exports an "ether_clk" alias for pmc_plt_clk_4 and the r8169 driver
has been modified to get and enable this clock (if present) the marking of
the clocks as CLK_IS_CRITICAL is no longer necessary.
This commit removes the CLK_IS_CRITICAL marking, fixing Cherry Trail
devices not being able to reach S0i3 greatly decreasing their battery
drain when suspended.
Buglink: https://bugzilla.kernel.org/show_bug.cgi?id=193891#c102
Cc: Johannes Stezenbach <js@sig21.net>
Cc: Carlo Caione <carlo@endlessm.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
drivers/clk/x86/clk-pmc-atom.c | 7 -------
1 file changed, 7 deletions(-)
diff --git a/drivers/clk/x86/clk-pmc-atom.c b/drivers/clk/x86/clk-pmc-atom.c
index 75151901ff7d..d977193842df 100644
--- a/drivers/clk/x86/clk-pmc-atom.c
+++ b/drivers/clk/x86/clk-pmc-atom.c
@@ -187,13 +187,6 @@ static struct clk_plt *plt_clk_register(struct platform_device *pdev, int id,
pclk->reg = base + PMC_CLK_CTL_OFFSET + id * PMC_CLK_CTL_SIZE;
spin_lock_init(&pclk->lock);
- /*
- * If the clock was already enabled by the firmware mark it as critical
- * to avoid it being gated by the clock framework if no driver owns it.
- */
- if (plt_clk_is_enabled(&pclk->hw))
- init.flags |= CLK_IS_CRITICAL;
-
ret = devm_clk_hw_register(&pdev->dev, &pclk->hw);
if (ret) {
pclk = ERR_PTR(ret);
--
2.18.0
^ permalink raw reply related
* [PATCH 4/4] RFC: r8169: Disable clk during suspend / resume
From: Hans de Goede @ 2018-08-27 14:32 UTC (permalink / raw)
To: David S . Miller, Heiner Kallweit, Michael Turquette,
Stephen Boyd, Andy Shevchenko, Irina Tirdea
Cc: Hans de Goede, netdev, Johannes Stezenbach, Carlo Caione,
linux-clk
In-Reply-To: <20180827143200.8597-1-hdegoede@redhat.com>
Disable the clk during suspend to save power. Note that tp->clk may be
NULL, the clk core functions handle this without problems.
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
drivers/net/ethernet/realtek/r8169.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/net/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek/r8169.c
index 779b02979493..aebc90158bd9 100644
--- a/drivers/net/ethernet/realtek/r8169.c
+++ b/drivers/net/ethernet/realtek/r8169.c
@@ -7312,8 +7312,10 @@ static int rtl8169_suspend(struct device *device)
{
struct pci_dev *pdev = to_pci_dev(device);
struct net_device *dev = pci_get_drvdata(pdev);
+ struct rtl8169_private *tp = netdev_priv(dev);
rtl8169_net_suspend(dev);
+ clk_disable_unprepare(tp->clk);
return 0;
}
@@ -7340,6 +7342,7 @@ static int rtl8169_resume(struct device *device)
struct net_device *dev = pci_get_drvdata(pdev);
struct rtl8169_private *tp = netdev_priv(dev);
+ clk_prepare_enable(tp->clk);
rtl8169_init_phy(dev, tp);
if (netif_running(dev))
--
2.18.0
^ permalink raw reply related
* GPL compliance issue with liquidio/lio_23xx_vsw.bin firmware
From: Florian Weimer @ 2018-08-27 15:01 UTC (permalink / raw)
To: linux-firmware
Cc: netdev@vger.kernel.org, Derek Chickles, Satanand Burla,
Felix Manlunas, Raghu Vatsavayi, Manish Awasthi
liquidio/lio_23xx_vsw.bin contains a compiled MIPS Linux kernel:
$ tail --bytes=+1313 liquidio/lio_23xx_vsw.bin > elf
$ readelf -aW elf
[…]
[ 6] __ksymtab PROGBITS ffffffff80e495f8 64a5f8 00d130
00 A 0 0 8
[ 7] __ksymtab_gpl PROGBITS ffffffff80e56728 657728 008400
00 A 0 0 8
[ 8] __ksymtab_strings PROGBITS ffffffff80e5eb28 65fb28 018868
00 A 0 0 1
[…]
Symbol table '.symtab' contains 1349 entries:
Num: Value Size Type Bind Vis Ndx Name
0: 0000000000000000 0 NOTYPE LOCAL DEFAULT UND
1: 0000000000000000 0 FILE LOCAL DEFAULT ABS
arch/mips/kernel/head.o
2: 0000000000000000 0 FILE LOCAL DEFAULT ABS init/main.c
3: 0000000000000000 0 FILE LOCAL DEFAULT ABS
include/linux/types.h
[…]
Yet there is no corresponding source provided, and LICENCE.cavium lacks
the required notices.
Thanks,
Florian
^ permalink raw reply
* Re: [PATCH RFC net-next] net: Poptrie based FIB lookup
From: Jesper Dangaard Brouer @ 2018-08-27 15:33 UTC (permalink / raw)
To: Md. Islam
Cc: Netdev, David Miller, David Ahern, Eric Dumazet, Alexey Kuznetsov,
Stephen Hemminger, makita.toshiaki, panda, yasuhiro.ohara, brouer,
John Fastabend
In-Reply-To: <CAFgPn1CHgSKRMy1RgutEh9sragUK7WtqO_COzzcqZEaugP8+7Q@mail.gmail.com>
On Sun, 26 Aug 2018 22:13:53 -0400 "Md. Islam" <mislam4@kent.edu> wrote:
> This patch implements Poptrie [1] based FIB lookup. It exhibits pretty
> impressive lookup performance compared to LC-trie. This poptrie
> implementation however somewhat deviates from the original
> implementation [2]. I tested this patch very rigorously with several
> FIB tables containing half a million routes. I got same result as
> LC-trie based fib_lookup().
It sounds really promising performance wise. The article [1] claim
lookup speeds up-to 240 Million lookups per second. That is a crazy
speed. This is 4.166 nanosec per lookup (1/240*1000), and their test
CPU is 3.9GHz, which gives them 16.25 CPU cycles for a lookup, with 3
insn per cycle, that gives them max 48 perfectly pipelined instructions
per lookup.
> Poptrie is intended to work in conjunction with LC-trie (not replace
> it). It is primarily designed to overcome many issues of TCAM based
> router [1]. [1] shows that the Poptrie can achieve very impressive
> lookup performance on CPU. This patch will mainly be used by XDP
> forwarding.
>
> 1. Asai, Hirochika, and Yasuhiro Ohara. "Poptrie: A compressed trie
> with population count for fast and scalable software IP routing table
> lookup." ACM SIGCOMM Computer Communication Review. 2015.
>
> 2. https://github.com/pixos/poptrie
>
> From c5e05ea66b06eb9313749bc8969b4c2798fcf96a Mon Sep 17 00:00:00 2001
> From: tamimcse <tamim@csebuet.org>
> Date: Sun, 26 Aug 2018 21:12:38 -0400
> Subject: [PATCH] Implented Poptrie
This above "commit-info" should not be part of the patch description.
> Signed-off-by: tamimcse <tamim@csebuet.org>
Use you real/full name here.
> ---
First of order of business: You need to conform to the kernels coding
standards!
https://www.kernel.org/doc/html/v4.18/process/coding-style.html
There is a script avail to check this called: scripts/checkpatch.pl
It summary says:
total: 139 errors, 238 warnings, 6 checks, 372 lines checked
(Not good, more error+warnings than lines...)
Please fix up those...
> include/net/ip_fib.h | 40 +++++++
> net/ipv4/Makefile | 2 +-
> net/ipv4/fib_poptrie.c | 295 +++++++++++++++++++++++++++++++++++++++++++++++++
> net/ipv4/fib_trie.c | 3 +
> 4 files changed, 339 insertions(+), 1 deletion(-)
> create mode 100644 net/ipv4/fib_poptrie.c
[...]
> diff --git a/net/ipv4/fib_poptrie.c b/net/ipv4/fib_poptrie.c
> new file mode 100644
> index 0000000..b3a88ab
> --- /dev/null
> +++ b/net/ipv4/fib_poptrie.c
> @@ -0,0 +1,295 @@
[....]
> +/*Insert a new node at index*/
> +static void insert_chield_node(struct poptrie_node *node,
> + char index)
> +{
You are misspelling "child" as "chield"
> + int i, j;
> + struct poptrie_node *arr;
> + int arr_size = (int)hweight64(node->nodevec);
> +
> + arr = kcalloc(arr_size + 1, sizeof(*arr), GFP_ATOMIC);
> + for (i = 0, j = 0; i < (arr_size + 1); i++) {
> + if (i != index && j < arr_size)
> + arr[i] = node->chield_nodes[j++];
> + }
> +
> + kfree(node->chield_nodes);
> + node->chield_nodes = arr;
> +}
[...]
> +/*We assume that pt->root is not NULL*/
> +void poptrie_lookup(struct poptrie *pt, __be32 dest, struct net_device **dev)
> +{
> + register u32 index;
> + register u64 bitmap, bitmask;
> + register unsigned long leaf_index;
> + register unsigned long node_index;
> + register struct poptrie_node *node = pt->root;
> + register u8 fib_index = pt->def_nh;
> + register u8 carry = 0;
> + register u8 carry_bit = 2;
Do you have performance data, that tell you that "register" is needed here?
> + while (1) {
> + /*Extract 6 bytes from dest */
> + if (likely(carry_bit != 8)) {
> + index = ((dest & 252) >> carry_bit) | carry;
> + carry = (dest & ((1 << carry_bit) - 1)) << (6 - carry_bit);
> + carry_bit = carry_bit + 2;
> + dest = dest >> 8;
> + } else {
> + index = carry;
> + carry = 0;
> + carry_bit = 2;
> + }
> +
> + /*Create a bitmap based on the the extracted value*/
> + bitmap = 1ULL << index;
> + bitmask = bitmap - 1;
> +
> + /*Find corresponding leaf*/
> + if (likely(node->vector & bitmap)) {
> + leaf_index = hweight64(node->leafvec & bitmask);
Just as help for reviewers, the popcnt instruction is here.
hweight64 == popcnt
> + if (!(node->leafvec & bitmap))
> + leaf_index--;
> + fib_index = node->leaves[leaf_index];
> + }
> +
> + /*Find corresponding node*/
> + if (likely(node->nodevec & bitmap)) {
> + node_index = hweight64(node->nodevec & bitmask);
And here.
> + node = &node->chield_nodes[node_index];
> + continue;
> + }
> +
> + *dev = get_fib(&pt->nhs, fib_index);
> + return;
> + }
> +}
--
Best regards,
Jesper Dangaard Brouer
MSc.CS, Principal Kernel Engineer at Red Hat
LinkedIn: http://www.linkedin.com/in/brouer
^ permalink raw reply
* [PATCH] bpf: fix build error with clang
From: Stefan Agner @ 2018-08-27 19:30 UTC (permalink / raw)
To: daniel; +Cc: kafai, ast, mka, netdev, linux-kernel, Stefan Agner
Building the newly introduced BPF_PROG_TYPE_SK_REUSEPORT leads to
a compile time error when building with clang:
net/core/filter.o: In function `sk_reuseport_convert_ctx_access':
../net/core/filter.c:7284: undefined reference to `__compiletime_assert_7284'
It seems that clang has issues resolving hweight_long at compile
time. Since SK_FL_PROTO_MASK is a constant, we can use the interface
for known constant arguments which works fine with clang.
Fixes: 2dbb9b9e6df6 ("bpf: Introduce BPF_PROG_TYPE_SK_REUSEPORT")
Signed-off-by: Stefan Agner <stefan@agner.ch>
---
net/core/filter.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/net/core/filter.c b/net/core/filter.c
index c25eb36f1320..7a2430945c71 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -7281,7 +7281,7 @@ static u32 sk_reuseport_convert_ctx_access(enum bpf_access_type type,
break;
case offsetof(struct sk_reuseport_md, ip_protocol):
- BUILD_BUG_ON(hweight_long(SK_FL_PROTO_MASK) != BITS_PER_BYTE);
+ BUILD_BUG_ON(HWEIGHT32(SK_FL_PROTO_MASK) != BITS_PER_BYTE);
SK_REUSEPORT_LOAD_SK_FIELD_SIZE_OFF(__sk_flags_offset,
BPF_W, 0);
*insn++ = BPF_ALU32_IMM(BPF_AND, si->dst_reg, SK_FL_PROTO_MASK);
--
2.18.0
^ permalink raw reply related
* Re: [PATCH] net: mvpp2: avoid bouncing buffers
From: Christoph Hellwig @ 2018-08-27 15:48 UTC (permalink / raw)
To: Brian Brooks
Cc: Christoph Hellwig, David Miller, antoine.tenart,
maxime.chevallier, ymarkman, stefanc, netdev, linux-kernel,
bjorn.topel, brian.brooks
In-Reply-To: <20180827135524.fv4mxkwjn5bv7p5e@di3>
WE should basically never have dev->dma_mask = &dev->coherent_dma_mask,
so until that is the case you are doctoring around the symptoms and
not the problem.
Does the patch below help your case?
----
>From 6294e0e330851ee06e66ab85b348f1d92d375d7a Mon Sep 17 00:00:00 2001
From: Christoph Hellwig <hch@lst.de>
Date: Mon, 27 Aug 2018 17:23:24 +0200
Subject: driver core: initialize a default DMA mask for platform device
We still treat devices without a DMA mask as defaulting to 32-bits for
both mask, but a few releases ago we've started warning about such
cases, as they require special cases to work around this sloppyness.
Add a dma_mask field to struct platform_object so that we can initialize
the dma_mask pointer in struct device and initialize both masks to
32-bits by default. Architectures can still override this in
arch_setup_pdev_archdata if needed.
Note that the code looks a little odd with the various conditionals
because we have to support platform_device structures that are
statically allocated.
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
drivers/base/platform.c | 15 +++++++++++++--
include/linux/platform_device.h | 1 +
2 files changed, 14 insertions(+), 2 deletions(-)
diff --git a/drivers/base/platform.c b/drivers/base/platform.c
index dff82a3c2caa..baf4b06cf2d9 100644
--- a/drivers/base/platform.c
+++ b/drivers/base/platform.c
@@ -225,6 +225,17 @@ struct platform_object {
char name[];
};
+static void setup_pdev_archdata(struct platform_device *pdev)
+{
+ if (!pdev->dev.coherent_dma_mask)
+ pdev->dev.coherent_dma_mask = DMA_BIT_MASK(32);
+ if (!pdev->dma_mask)
+ pdev->dma_mask = DMA_BIT_MASK(32);
+ if (!pdev->dev.dma_mask)
+ pdev->dev.dma_mask = &pdev->dma_mask;
+ arch_setup_pdev_archdata(pdev);
+};
+
/**
* platform_device_put - destroy a platform device
* @pdev: platform device to free
@@ -271,7 +282,7 @@ struct platform_device *platform_device_alloc(const char *name, int id)
pa->pdev.id = id;
device_initialize(&pa->pdev.dev);
pa->pdev.dev.release = platform_device_release;
- arch_setup_pdev_archdata(&pa->pdev);
+ setup_pdev_archdata(&pa->pdev);
}
return pa ? &pa->pdev : NULL;
@@ -472,7 +483,7 @@ EXPORT_SYMBOL_GPL(platform_device_del);
int platform_device_register(struct platform_device *pdev)
{
device_initialize(&pdev->dev);
- arch_setup_pdev_archdata(pdev);
+ setup_pdev_archdata(pdev);
return platform_device_add(pdev);
}
EXPORT_SYMBOL_GPL(platform_device_register);
diff --git a/include/linux/platform_device.h b/include/linux/platform_device.h
index 1a9f38f27f65..d84ec1de6022 100644
--- a/include/linux/platform_device.h
+++ b/include/linux/platform_device.h
@@ -25,6 +25,7 @@ struct platform_device {
int id;
bool id_auto;
struct device dev;
+ dma_addr_t dma_mask;
u32 num_resources;
struct resource *resource;
--
2.18.0
^ permalink raw reply related
* Re: [PATCH v2 2/2] can: rcar: use SPDX identifier for Renesas drivers
From: Wolfram Sang @ 2018-08-27 19:45 UTC (permalink / raw)
To: Marc Kleine-Budde
Cc: Wolfram Sang, linux-renesas-soc, Kuninori Morimoto,
Wolfgang Grandegger, David S. Miller, linux-can, netdev,
linux-kernel
In-Reply-To: <89e744d5-8739-5215-1b37-fbd2f4be7aee@pengutronix.de>
[-- Attachment #1: Type: text/plain, Size: 229 bytes --]
> Applied to linux-can-next. Please add a patch description to the patch.
> My $UPSTREAM doesn't like empty patch descriptions :) I've shamelessly
> used Fabio Estevam patch description from his flexcan SPDX patch.
Thx, Marc!
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH v2 01/17] asm: simd context helper API
From: Palmer Dabbelt @ 2018-08-27 19:50 UTC (permalink / raw)
To: Jason; +Cc: linux-kernel, netdev, davem, Jason, luto, Greg KH, sneves,
linux-arch
In-Reply-To: <20180824213849.23647-2-Jason@zx2c4.com>
On Fri, 24 Aug 2018 14:38:33 PDT (-0700), Jason@zx2c4.com wrote:
> Sometimes it's useful to amortize calls to XSAVE/XRSTOR and the related
> FPU/SIMD functions over a number of calls, because FPU restoration is
> quite expensive. This adds a simple header for carrying out this pattern:
>
> simd_context_t simd_context = simd_get();
> while ((item = get_item_from_queue()) != NULL) {
> encrypt_item(item, simd_context);
> simd_context = simd_relax(simd_context);
> }
> simd_put(simd_context);
>
> The relaxation step ensures that we don't trample over preemption, and
> the get/put API should be a familiar paradigm in the kernel.
>
> Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
> Cc: Andy Lutomirski <luto@kernel.org>
> Cc: Greg KH <gregkh@linuxfoundation.org>
> Cc: Samuel Neves <sneves@dei.uc.pt>
> Cc: linux-arch@vger.kernel.org
> ---
> arch/alpha/include/asm/Kbuild | 5 ++--
> arch/arc/include/asm/Kbuild | 1 +
> arch/arm/include/asm/simd.h | 42 ++++++++++++++++++++++++++++++
> arch/arm64/include/asm/simd.h | 37 +++++++++++++++++++++-----
> arch/c6x/include/asm/Kbuild | 3 ++-
> arch/h8300/include/asm/Kbuild | 3 ++-
> arch/hexagon/include/asm/Kbuild | 1 +
> arch/ia64/include/asm/Kbuild | 1 +
> arch/m68k/include/asm/Kbuild | 1 +
> arch/microblaze/include/asm/Kbuild | 1 +
> arch/mips/include/asm/Kbuild | 1 +
> arch/nds32/include/asm/Kbuild | 7 ++---
> arch/nios2/include/asm/Kbuild | 1 +
> arch/openrisc/include/asm/Kbuild | 7 ++---
> arch/parisc/include/asm/Kbuild | 1 +
> arch/powerpc/include/asm/Kbuild | 3 ++-
> arch/riscv/include/asm/Kbuild | 3 ++-
> arch/s390/include/asm/Kbuild | 3 ++-
> arch/sh/include/asm/Kbuild | 1 +
> arch/sparc/include/asm/Kbuild | 1 +
> arch/um/include/asm/Kbuild | 3 ++-
> arch/unicore32/include/asm/Kbuild | 1 +
> arch/x86/include/asm/simd.h | 30 ++++++++++++++++++++-
> arch/xtensa/include/asm/Kbuild | 1 +
> include/asm-generic/simd.h | 15 +++++++++++
> include/linux/simd.h | 28 ++++++++++++++++++++
> 26 files changed, 180 insertions(+), 21 deletions(-)
> create mode 100644 arch/arm/include/asm/simd.h
> create mode 100644 include/linux/simd.h
...
> diff --git a/arch/riscv/include/asm/Kbuild b/arch/riscv/include/asm/Kbuild
> index 576ffdca06ba..8d3e7aef3234 100644
> --- a/arch/riscv/include/asm/Kbuild
> +++ b/arch/riscv/include/asm/Kbuild
> @@ -4,9 +4,9 @@ generic-y += checksum.h
> generic-y += cputime.h
> generic-y += device.h
> generic-y += div64.h
> -generic-y += dma.h
> generic-y += dma-contiguous.h
> generic-y += dma-mapping.h
> +generic-y += dma.h
> generic-y += emergency-restart.h
> generic-y += errno.h
> generic-y += exec.h
If this is the canonical ordering and doing so makes your life easier then I'm
OK taking this as a separate patch into the RISC-V tree, but if not then feel
free to roll something like this up into your next patch set.
> @@ -45,6 +45,7 @@ generic-y += setup.h
> generic-y += shmbuf.h
> generic-y += shmparam.h
> generic-y += signal.h
> +generic-y += simd.h
> generic-y += socket.h
> generic-y += sockios.h
> generic-y += stat.h
Either way, this looks fine for as far as the RISC-V stuff goes as it's pretty
much a NOP. As long as it stays a NOP then feel free to add a
Reviewed-by: Palmer Dabbelt <palmer@sifive.com>
as far as the RISC-V parts are conceded. It looks like there's a lot of other
issues, though, so it's not much of a review :)
^ permalink raw reply
* Re: any reason for "!!netif_carrier_ok" and "!!netif_dormant" in net-sysfs.c?
From: David Miller @ 2018-08-27 16:04 UTC (permalink / raw)
To: rpjday; +Cc: netdev
In-Reply-To: <alpine.LFD.2.21.1808270448030.28019@localhost.localdomain>
From: "Robert P. J. Day" <rpjday@crashcourse.ca>
Date: Mon, 27 Aug 2018 04:55:29 -0400 (EDT)
> another pedantic oddity -- is there a reason for these two double
> negations in net/core/net-sysfs.c?
It turns an arbitrary integer into a boolean, this is a common
construct across the kernel tree so I'm surprised you've never seen
it before.
Although, I don't know how much more hand holding we're willing to
tolerate continuing to give to you at this point.
Thanks.
^ permalink raw reply
* Re: [PATCH v2 iproute2-next 1/3] tc: support conversions to or from 64 bit nanosecond-based time
From: Stephen Hemminger @ 2018-08-27 16:11 UTC (permalink / raw)
To: Yousuk Seung
Cc: netdev, David Ahern, Michael McLennan, Priyaranjan Jha, Dave Taht,
Neal Cardwell
In-Reply-To: <20180827024230.246445-2-ysseung@google.com>
On Sun, 26 Aug 2018 19:42:28 -0700
Yousuk Seung <ysseung@google.com> wrote:
> +int get_time(unsigned int *time, const char *str)
> +{
> + double t;
> + char *p;
> +
> + t = strtod(str, &p);
> + if (p == str)
> + return -1;
> +
> + if (*p) {
> + if (strcasecmp(p, "s") == 0 || strcasecmp(p, "sec") == 0 ||
> + strcasecmp(p, "secs") == 0)
> + t *= TIME_UNITS_PER_SEC;
> + else if (strcasecmp(p, "ms") == 0 || strcasecmp(p, "msec") == 0 ||
> + strcasecmp(p, "msecs") == 0)
> + t *= TIME_UNITS_PER_SEC/1000;
> + else if (strcasecmp(p, "us") == 0 || strcasecmp(p, "usec") == 0 ||
> + strcasecmp(p, "usecs") == 0)
> + t *= TIME_UNITS_PER_SEC/1000000;
> + else
> + return -1;
Do we need to really support UPPER case.
Isn't existing matches semantics good enough?
^ permalink raw reply
* Re: any reason for "!!netif_carrier_ok" and "!!netif_dormant" in net-sysfs.c?
From: rpjday @ 2018-08-27 16:16 UTC (permalink / raw)
To: David Miller; +Cc: netdev
In-Reply-To: <20180827.090424.977658416500279863.davem@davemloft.net>
Quoting David Miller <davem@davemloft.net>:
> From: "Robert P. J. Day" <rpjday@crashcourse.ca>
> Date: Mon, 27 Aug 2018 04:55:29 -0400 (EDT)
>
>> another pedantic oddity -- is there a reason for these two double
>> negations in net/core/net-sysfs.c?
>
> It turns an arbitrary integer into a boolean, this is a common
> construct across the kernel tree so I'm surprised you've never seen
> it before.
>
> Although, I don't know how much more hand holding we're willing to
> tolerate continuing to give to you at this point.
>
> Thanks.
I mentioned in my earlier email that I know what that construct is
*typically* used for; I also pointed out that, AFAICT, it was totally
unnecessary in the context of the two routines I mentioned, which
would appear to ever return only one of two boolean values, 0 or 1.
My experience with kernel code is that one should not introduce
unnecessary complexities, which suggests (as I stated) that there
seems to be no value in the "!!" construct *in those particular
cases*, hence my curiosity.
If you want to criticize my question, at least have the courtesy
to represent it accurately.
rday
^ permalink raw reply
* Re: [PATCH RFC net-next] net/fib: Poptrie based FIB lookup
From: Stephen Hemminger @ 2018-08-27 16:24 UTC (permalink / raw)
To: Md. Islam
Cc: Netdev, David Miller, David Ahern, Eric Dumazet, Alexey Kuznetsov,
makita.toshiaki, panda, yasuhiro.ohara, john.fastabend,
alexei.starovoitov
In-Reply-To: <CAFgPn1Dz3cuNMULfRVKjaN0vndUbXe7V8yUoSURus8UbvyMkqA@mail.gmail.com>
On Sun, 26 Aug 2018 22:28:48 -0400
"Md. Islam" <mislam4@kent.edu> wrote:
> This patch implements Poptrie [1] based FIB lookup. It exhibits pretty
> impressive lookup performance compared to LC-trie. This poptrie
> implementation however somewhat deviates from the original
> implementation [2]. I tested this patch very rigorously with several
> FIB tables containing half a million routes. I got same result as
> LC-trie based fib_lookup().
>
> Poptrie is intended to work in conjunction with LC-trie (not replace
> it). It is primarily designed to overcome many issues of TCAM based
> router [1]. It [1] shows that the Poptrie can achieve very impressive
> lookup performance on CPU. This patch will mainly be used by XDP
> forwarding.
>
> 1. Asai, Hirochika, and Yasuhiro Ohara. "Poptrie: A compressed trie
> with population count for fast and scalable software IP routing table
> lookup." ACM SIGCOMM Computer Communication Review. 2015.
>
> 2. https://github.com/pixos/poptrie
I am glad to see more research in to lookup speed. Here are some non-technical
feedback. Looking deeper takes longer.
The license in github version is not compatiable with GPL. If you based your
code off that, you need to get approval from original copyright holder.
The code is not formatted according to current kernel coding style.
Please use checkpatch to see what the issues are.
It is preferred that a function return a value, rather than being void
and returing result by reference. Example:
> +
> +/*We assume that pt->root is not NULL*/
> +void poptrie_lookup(struct poptrie *pt, __be32 dest, struct net_device **dev)
> +{
...
> + *dev = get_fib(&pt->nhs, fib_index);
> + return;
> + }
Why not?
static struct net_device *poptrie_lookup(struct poptrie *pt, __be32 dest)
Also, as Dave mentioned any implementation needs to handle multiple namespaces
and routing tables.
Could this alternative lookup be enabled via sysctl at runtime rather than kernel config?
^ permalink raw reply
* Re: bpfilter causes a leftover kernel process
From: Olivier Brunel @ 2018-08-27 16:31 UTC (permalink / raw)
To: netdev; +Cc: Olivier Brunel
In-Reply-To: <20180826180816.04ef7d16@jjacky.com>
Small addentum: Of course I failed to realize this bpfilter helper is
also mentioned in the kernel log:
kern.info: [ 8.997711] bpfilter: Loaded bpfilter_umh pid 920
It also seems to be absolutely required when CONFIG_BPFILTER is
enabled, that is I tried blacklisting the module bpfilter, but then
other things (e.g. iptables-restore) just fail to work.
So the process is required, never ends and prevents umouting the
rootfs on shutdown. Unless I'm missing something, there's definitely a
bug there?
Thanks,
^ permalink raw reply
* Re: [PATCH v2 iproute2-next 1/3] tc: support conversions to or from 64 bit nanosecond-based time
From: Dave Taht @ 2018-08-27 16:39 UTC (permalink / raw)
To: Stephen Hemminger
Cc: Yousuk Seung, Linux Kernel Network Developers, David Ahern,
Michael McLennan, Priyaranjan Jha, Neal Cardwell
In-Reply-To: <20180827091156.46691f4f@shemminger-XPS-13-9360>
On Mon, Aug 27, 2018 at 9:11 AM Stephen Hemminger
<stephen@networkplumber.org> wrote:
>
> On Sun, 26 Aug 2018 19:42:28 -0700
> Yousuk Seung <ysseung@google.com> wrote:
>
> > +int get_time(unsigned int *time, const char *str)
> > +{
> > + double t;
> > + char *p;
> > +
> > + t = strtod(str, &p);
> > + if (p == str)
> > + return -1;
> > +
> > + if (*p) {
> > + if (strcasecmp(p, "s") == 0 || strcasecmp(p, "sec") == 0 ||
> > + strcasecmp(p, "secs") == 0)
> > + t *= TIME_UNITS_PER_SEC;
> > + else if (strcasecmp(p, "ms") == 0 || strcasecmp(p, "msec") == 0 ||
> > + strcasecmp(p, "msecs") == 0)
> > + t *= TIME_UNITS_PER_SEC/1000;
> > + else if (strcasecmp(p, "us") == 0 || strcasecmp(p, "usec") == 0 ||
> > + strcasecmp(p, "usecs") == 0)
> > + t *= TIME_UNITS_PER_SEC/1000000;
> > + else
> > + return -1;
>
> Do we need to really support UPPER case.
But that's ALWAYS been the case in the 32 bit version of code above.
Imagine how many former VMS and MVS hackers you'd upset if they had to
turn caps-lock off!
> Isn't existing matches semantics good enough?
But that's the existing case for the 32 bit api, now replicated in the
64 bit api. ? I think the case-insensitive ship has sailed here. Can't
break userspace.
Well.. adding UTF-8 would be cool. We could start using the actual
greek symbols for delta (δ) and beta (β) in particular. It would
replace a lot of typing, with a whole bunch more shift keys on a
single letter, fit better into
80 column lines, and so on, and tc inputs and outputs are already
pretty greek to many.
^ permalink raw reply
* [PATCH 2/2] net/ibm/emac: deletion of unneeded macros definitions
From: Ivan Mikhaylov @ 2018-08-27 16:43 UTC (permalink / raw)
To: netdev; +Cc: David S . Miller, Christian Lamparter, linux-kernel
Signed-off-by: Ivan Mikhaylov <ivan@de.ibm.com>
---
drivers/net/ethernet/ibm/emac/emac.h | 7 -------
1 files changed, 0 insertions(+), 7 deletions(-)
diff --git a/drivers/net/ethernet/ibm/emac/emac.h b/drivers/net/ethernet/ibm/emac/emac.h
index e2f80cc..f541660 100644
--- a/drivers/net/ethernet/ibm/emac/emac.h
+++ b/drivers/net/ethernet/ibm/emac/emac.h
@@ -229,7 +229,6 @@ struct emac_regs {
#define EMAC_STACR_PHYD_SHIFT 16
#define EMAC_STACR_OC 0x00008000
#define EMAC_STACR_PHYE 0x00004000
-#define EMAC_STACR_STAC_MASK 0x00003000
#define EMAC_STACR_STAC_READ 0x00001000
#define EMAC_STACR_STAC_WRITE 0x00000800
#define EMAC_STACR_OPBC_MASK 0x00000C00
@@ -245,14 +244,8 @@ struct emac_regs {
#define EMAC_STACR_PCDA_MASK 0x1f
#define EMAC_STACR_PCDA_SHIFT 5
#define EMAC_STACR_PRA_MASK 0x1f
-#define EMACX_STACR_STAC_MASK 0x00003800
#define EMACX_STACR_STAC_READ 0x00001000
#define EMACX_STACR_STAC_WRITE 0x00000800
-#define EMACX_STACR_STAC_IND_ADDR 0x00002000
-#define EMACX_STACR_STAC_IND_READ 0x00003800
-#define EMACX_STACR_STAC_IND_READINC 0x00003000
-#define EMACX_STACR_STAC_IND_WRITE 0x00002800
-
/* EMACx_TRTR */
#define EMAC_TRTR_SHIFT_EMAC4 24
^ permalink raw reply related
* Re: any reason for "!!netif_carrier_ok" and "!!netif_dormant" in net-sysfs.c?
From: Eric Dumazet @ 2018-08-27 16:45 UTC (permalink / raw)
To: rpjday, David Miller; +Cc: netdev
In-Reply-To: <20180827121623.Horde.Jfv_6y4hNg2t7I9Trd8TLaD@crashcourse.ca>
On 08/27/2018 09:16 AM, rpjday@crashcourse.ca wrote:
>
> Quoting David Miller <davem@davemloft.net>:
>
>> From: "Robert P. J. Day" <rpjday@crashcourse.ca>
>> Date: Mon, 27 Aug 2018 04:55:29 -0400 (EDT)
>>
>>> another pedantic oddity -- is there a reason for these two double
>>> negations in net/core/net-sysfs.c?
>>
>> It turns an arbitrary integer into a boolean, this is a common
>> construct across the kernel tree so I'm surprised you've never seen
>> it before.
>>
>> Although, I don't know how much more hand holding we're willing to
>> tolerate continuing to give to you at this point.
>>
>> Thanks.
>
> I mentioned in my earlier email that I know what that construct is
> *typically* used for; I also pointed out that, AFAICT, it was totally
> unnecessary in the context of the two routines I mentioned, which
> would appear to ever return only one of two boolean values, 0 or 1.
>
> My experience with kernel code is that one should not introduce
> unnecessary complexities, which suggests (as I stated) that there
> seems to be no value in the "!!" construct *in those particular
> cases*, hence my curiosity.
Have you checked git history ?
My guess is that netif_carrier_ok() used to return an int, not a bool.
!!netif_carrier_ok() was not complexity, it was probably shorter than
the form used in
u32 ethtool_op_get_link(struct net_device *dev)
{
return netif_carrier_ok(dev) ? 1 : 0;
}
But really, this is really trivial stuff, we have more interesting stuff
to take care of these days.
^ permalink raw reply
* Re: any reason for "!!netif_carrier_ok" and "!!netif_dormant" in net-sysfs.c?
From: rpjday @ 2018-08-27 16:50 UTC (permalink / raw)
To: Eric Dumazet; +Cc: David Miller, netdev
In-Reply-To: <3b4857e3-f2a6-104a-0370-7c41e2f755e8@gmail.com>
Quoting Eric Dumazet <eric.dumazet@gmail.com>:
> On 08/27/2018 09:16 AM, rpjday@crashcourse.ca wrote:
>>
>> Quoting David Miller <davem@davemloft.net>:
>>
>>> From: "Robert P. J. Day" <rpjday@crashcourse.ca>
>>> Date: Mon, 27 Aug 2018 04:55:29 -0400 (EDT)
>>>
>>>> another pedantic oddity -- is there a reason for these two double
>>>> negations in net/core/net-sysfs.c?
>>>
>>> It turns an arbitrary integer into a boolean, this is a common
>>> construct across the kernel tree so I'm surprised you've never seen
>>> it before.
>>>
>>> Although, I don't know how much more hand holding we're willing to
>>> tolerate continuing to give to you at this point.
>>>
>>> Thanks.
>>
>> I mentioned in my earlier email that I know what that construct is
>> *typically* used for; I also pointed out that, AFAICT, it was totally
>> unnecessary in the context of the two routines I mentioned, which
>> would appear to ever return only one of two boolean values, 0 or 1.
>>
>> My experience with kernel code is that one should not introduce
>> unnecessary complexities, which suggests (as I stated) that there
>> seems to be no value in the "!!" construct *in those particular
>> cases*, hence my curiosity.
>
> Have you checked git history ?
>
> My guess is that netif_carrier_ok() used to return an int, not a bool.
>
> !!netif_carrier_ok() was not complexity, it was probably shorter than
> the form used in
>
> u32 ethtool_op_get_link(struct net_device *dev)
> {
> return netif_carrier_ok(dev) ? 1 : 0;
> }
>
> But really, this is really trivial stuff, we have more interesting stuff
> to take care of these days.
Point taken -- for trivialities like this, I'll check Git history, and
apologies for the noise on that one.
rday
^ permalink raw reply
* Re: any reason for "!!netif_carrier_ok" and "!!netif_dormant" in net-sysfs.c?
From: Joe Perches @ 2018-08-27 16:54 UTC (permalink / raw)
To: rpjday, David Miller; +Cc: netdev
In-Reply-To: <20180827121623.Horde.Jfv_6y4hNg2t7I9Trd8TLaD@crashcourse.ca>
On Mon, 2018-08-27 at 12:16 -0400, rpjday@crashcourse.ca wrote:
> Quoting David Miller <davem@davemloft.net>:
>
> > From: "Robert P. J. Day" <rpjday@crashcourse.ca>
> > Date: Mon, 27 Aug 2018 04:55:29 -0400 (EDT)
> >
> > > another pedantic oddity -- is there a reason for these two double
> > > negations in net/core/net-sysfs.c?
> >
> > It turns an arbitrary integer into a boolean, this is a common
> > construct across the kernel tree so I'm surprised you've never seen
> > it before.
> >
> > Although, I don't know how much more hand holding we're willing to
> > tolerate continuing to give to you at this point.
> >
> > Thanks.
>
> I mentioned in my earlier email that I know what that construct is
> *typically* used for; I also pointed out that, AFAICT, it was totally
> unnecessary in the context of the two routines I mentioned, which
> would appear to ever return only one of two boolean values, 0 or 1.
Both are bool functions, so it's guaranteed and the
!! uses are redundant.
include/linux/netdevice.h:static inline bool netif_carrier_ok(const struct net_device *dev)
include/linux/netdevice.h:static inline bool netif_dormant(const struct net_device *dev)
And there are 4 uses with !!
$ git grep -P '\!\s*\!\s*netif_(?:dormant|carrier_ok)\s*\('
drivers/net/team/team.c: __team_port_change_port_added(port, !!netif_carrier_ok(port_dev));
drivers/net/usb/r8152.c: bool sw_linking = !!netif_carrier_ok(tp->netdev);
net/core/net-sysfs.c: return sprintf(buf, fmt_dec, !!netif_carrier_ok(netdev));
net/core/net-sysfs.c: return sprintf(buf, fmt_dec, !!netif_dormant(netdev));
out of the ~400 or so treewide.
Only redeeming use of the !! is the fmt_dec expects
an int and this forces the type to int, though the
compiler would do that automatically anyway.
I'd suggest removing the fmt_<foo> uses for clarity.
---
net/core/net-sysfs.c | 49 ++++++++++++++++++++++---------------------------
1 file changed, 22 insertions(+), 27 deletions(-)
diff --git a/net/core/net-sysfs.c b/net/core/net-sysfs.c
index bd67c4d0fcfd..ecaa684f4b92 100644
--- a/net/core/net-sysfs.c
+++ b/net/core/net-sysfs.c
@@ -31,11 +31,6 @@
#include "net-sysfs.h"
#ifdef CONFIG_SYSFS
-static const char fmt_hex[] = "%#x\n";
-static const char fmt_dec[] = "%d\n";
-static const char fmt_ulong[] = "%lu\n";
-static const char fmt_u64[] = "%llu\n";
-
static inline int dev_isalive(const struct net_device *dev)
{
return dev->reg_state <= NETREG_REGISTERED;
@@ -107,26 +102,26 @@ static ssize_t netdev_store(struct device *dev, struct device_attribute *attr,
return ret;
}
-NETDEVICE_SHOW_RO(dev_id, fmt_hex);
-NETDEVICE_SHOW_RO(dev_port, fmt_dec);
-NETDEVICE_SHOW_RO(addr_assign_type, fmt_dec);
-NETDEVICE_SHOW_RO(addr_len, fmt_dec);
-NETDEVICE_SHOW_RO(ifindex, fmt_dec);
-NETDEVICE_SHOW_RO(type, fmt_dec);
-NETDEVICE_SHOW_RO(link_mode, fmt_dec);
+NETDEVICE_SHOW_RO(dev_id, "%#x\n");
+NETDEVICE_SHOW_RO(dev_port, "%d\n");
+NETDEVICE_SHOW_RO(addr_assign_type, "%d\n");
+NETDEVICE_SHOW_RO(addr_len, "%d\n");
+NETDEVICE_SHOW_RO(ifindex, "%d\n");
+NETDEVICE_SHOW_RO(type, "%d\n");
+NETDEVICE_SHOW_RO(link_mode, "%d\n");
static ssize_t iflink_show(struct device *dev, struct device_attribute *attr,
char *buf)
{
struct net_device *ndev = to_net_dev(dev);
- return sprintf(buf, fmt_dec, dev_get_iflink(ndev));
+ return sprintf(buf, "%d\n", dev_get_iflink(ndev));
}
static DEVICE_ATTR_RO(iflink);
static ssize_t format_name_assign_type(const struct net_device *dev, char *buf)
{
- return sprintf(buf, fmt_dec, dev->name_assign_type);
+ return sprintf(buf, "%d\n", dev->name_assign_type);
}
static ssize_t name_assign_type_show(struct device *dev,
@@ -188,7 +183,7 @@ static ssize_t carrier_show(struct device *dev,
struct net_device *netdev = to_net_dev(dev);
if (netif_running(netdev))
- return sprintf(buf, fmt_dec, !!netif_carrier_ok(netdev));
+ return sprintf(buf, "%d\n", !!netif_carrier_ok(netdev));
return -EINVAL;
}
@@ -207,7 +202,7 @@ static ssize_t speed_show(struct device *dev,
struct ethtool_link_ksettings cmd;
if (!__ethtool_get_link_ksettings(netdev, &cmd))
- ret = sprintf(buf, fmt_dec, cmd.base.speed);
+ ret = sprintf(buf, "%d\n", cmd.base.speed);
}
rtnl_unlock();
return ret;
@@ -254,7 +249,7 @@ static ssize_t dormant_show(struct device *dev,
struct net_device *netdev = to_net_dev(dev);
if (netif_running(netdev))
- return sprintf(buf, fmt_dec, !!netif_dormant(netdev));
+ return sprintf(buf, "%d\n", !!netif_dormant(netdev));
return -EINVAL;
}
@@ -295,7 +290,7 @@ static ssize_t carrier_changes_show(struct device *dev,
{
struct net_device *netdev = to_net_dev(dev);
- return sprintf(buf, fmt_dec,
+ return sprintf(buf, "%d\n",
atomic_read(&netdev->carrier_up_count) +
atomic_read(&netdev->carrier_down_count));
}
@@ -307,7 +302,7 @@ static ssize_t carrier_up_count_show(struct device *dev,
{
struct net_device *netdev = to_net_dev(dev);
- return sprintf(buf, fmt_dec, atomic_read(&netdev->carrier_up_count));
+ return sprintf(buf, "%d\n", atomic_read(&netdev->carrier_up_count));
}
static DEVICE_ATTR_RO(carrier_up_count);
@@ -317,7 +312,7 @@ static ssize_t carrier_down_count_show(struct device *dev,
{
struct net_device *netdev = to_net_dev(dev);
- return sprintf(buf, fmt_dec, atomic_read(&netdev->carrier_down_count));
+ return sprintf(buf, "%d\n", atomic_read(&netdev->carrier_down_count));
}
static DEVICE_ATTR_RO(carrier_down_count);
@@ -333,7 +328,7 @@ static ssize_t mtu_store(struct device *dev, struct device_attribute *attr,
{
return netdev_store(dev, attr, buf, len, change_mtu);
}
-NETDEVICE_SHOW_RW(mtu, fmt_dec);
+NETDEVICE_SHOW_RW(mtu, "%d\n");
static int change_flags(struct net_device *dev, unsigned long new_flags)
{
@@ -345,7 +340,7 @@ static ssize_t flags_store(struct device *dev, struct device_attribute *attr,
{
return netdev_store(dev, attr, buf, len, change_flags);
}
-NETDEVICE_SHOW_RW(flags, fmt_hex);
+NETDEVICE_SHOW_RW(flags, "%#x\n");
static ssize_t tx_queue_len_store(struct device *dev,
struct device_attribute *attr,
@@ -356,7 +351,7 @@ static ssize_t tx_queue_len_store(struct device *dev,
return netdev_store(dev, attr, buf, len, dev_change_tx_queue_len);
}
-NETDEVICE_SHOW_RW(tx_queue_len, fmt_dec);
+NETDEVICE_SHOW_RW(tx_queue_len, "%d\n");
static int change_gro_flush_timeout(struct net_device *dev, unsigned long val)
{
@@ -373,7 +368,7 @@ static ssize_t gro_flush_timeout_store(struct device *dev,
return netdev_store(dev, attr, buf, len, change_gro_flush_timeout);
}
-NETDEVICE_SHOW_RW(gro_flush_timeout, fmt_ulong);
+NETDEVICE_SHOW_RW(gro_flush_timeout, "%lu\n");
static ssize_t ifalias_store(struct device *dev, struct device_attribute *attr,
const char *buf, size_t len)
@@ -431,7 +426,7 @@ static ssize_t group_store(struct device *dev, struct device_attribute *attr,
{
return netdev_store(dev, attr, buf, len, change_group);
}
-NETDEVICE_SHOW(group, fmt_dec);
+NETDEVICE_SHOW(group, "%d\n");
static DEVICE_ATTR(netdev_group, 0644, group_show, group_store);
static int change_proto_down(struct net_device *dev, unsigned long proto_down)
@@ -445,7 +440,7 @@ static ssize_t proto_down_store(struct device *dev,
{
return netdev_store(dev, attr, buf, len, change_proto_down);
}
-NETDEVICE_SHOW_RW(proto_down, fmt_dec);
+NETDEVICE_SHOW_RW(proto_down, "%d\n");
static ssize_t phys_port_id_show(struct device *dev,
struct device_attribute *attr, char *buf)
@@ -568,7 +563,7 @@ static ssize_t netstat_show(const struct device *d,
struct rtnl_link_stats64 temp;
const struct rtnl_link_stats64 *stats = dev_get_stats(dev, &temp);
- ret = sprintf(buf, fmt_u64, *(u64 *)(((u8 *)stats) + offset));
+ ret = sprintf(buf, "%llu\n", *(u64 *)(((u8 *)stats) + offset));
}
read_unlock(&dev_base_lock);
return ret;
^ permalink raw reply related
* Re: [PATCH RFC net-next] net/fib: Poptrie based FIB lookup
From: David Ahern @ 2018-08-27 16:56 UTC (permalink / raw)
To: Stephen Hemminger, Md. Islam
Cc: Netdev, David Miller, Eric Dumazet, Alexey Kuznetsov,
makita.toshiaki, panda, yasuhiro.ohara, john.fastabend,
alexei.starovoitov
In-Reply-To: <20180827092420.791bb1ad@shemminger-XPS-13-9360>
On 8/27/18 10:24 AM, Stephen Hemminger wrote:
>
> Also, as Dave mentioned any implementation needs to handle multiple namespaces
> and routing tables.
>
> Could this alternative lookup be enabled via sysctl at runtime rather than kernel config?
>
I spent time a couple of years ago refactoring IPv4 fib lookups with the
intent of allowing different algorithms - for use cases like this:
https://github.com/dsahern/linux/commits/net/ipv4-fib-ops
(it is also another way to solve the API nightmare that ipv6 has become).
But the poptrie patches that have been sent so far have much bigger
problems that need to be addressed before anyone worries about how to
select poptrie vs lc-trie.
The patch does not handle errors (e.g., if attributes such as tos,
metric/priority and multipath are not allowed you need to fail the route
insert; further, what happens if someone creates > 255 netdevices?),
last patch has both fib tables populated (a no-go), does not handle
delete or dumps. In the current form, the poptrie algorithm can not be
taken for a test drive. My suggestion to make it a compile time
selection is just so people can actually try it out using current admin
tools.
^ permalink raw reply
* Re: [PATCH net] net: sungem: fix rx checksum support
From: Eric Dumazet @ 2018-08-27 16:57 UTC (permalink / raw)
To: Meelis Roos
Cc: Eric Dumazet, David Miller, netdev, Mathieu Malaterre,
Andreas Schwab
In-Reply-To: <alpine.LRH.2.21.1808261612040.22219@math.ut.ee>
On Sun, Aug 26, 2018 at 6:14 AM <mroos@linux.ee> wrote:
>
> > BTW, removing the FCS also means GRO is going to work, finally on this NIC ;)
> >
> > GRO does not like packets with padding.
>
> As a follow-up, I am seeing hw csum failures on Sun V440 that has
> onboard Sun Cassini with sungem driver. First tested version was 4.18
> (it happened there once) and now that I tried 4.18+git, it still
> happens:
>
> [ 21.563282] libphy: Fixed MDIO Bus: probed
> [ 21.617116] cassini: cassini.c:v1.6 (21 May 2008)
> [ 21.678962] cassini 0000:00:02.0: enabling device (0144 -> 0146)
> [ 21.761931] cassini 0000:00:02.0 eth0: Sun Cassini+ (64bit/66MHz PCI/Cu) Ethernet[6] 00:03:ba:6f:14:39
> [ 21.884952] cassini 0003:00:01.0: enabling device (0144 -> 0146)
> [ 21.967868] cassini 0003:00:01.0 eth1: Sun Cassini+ (64bit/66MHz PCI/Cu) Ethernet[29] 00:03:ba:6f:14:3a
> [...]
> [ 54.341212] eth0: hw csum failure
> [ 54.384725] CPU: 2 PID: 0 Comm: swapper/2 Not tainted 4.18.0-12952-g2923b27 #1397
> [ 54.483167] Call Trace:
> [ 54.515209] [000000000077838c] __skb_checksum_complete+0xcc/0xe0
> [ 54.595272] [000000000080fc84] igmp_rcv+0x224/0x920
> [ 54.660475] [00000000007ca3d0] ip_local_deliver+0xb0/0x240
> [ 54.733675] [00000000007ca5c0] ip_rcv+0x60/0xa0
> [ 54.794304] [0000000000781a30] __netif_receive_skb_one_core+0x30/0x60
> [ 54.880094] [0000000000782914] process_backlog+0x94/0x140
> [ 54.952161] [0000000000788f6c] net_rx_action+0x1ec/0x320
> [ 55.023083] [0000000000870de8] __do_softirq+0xc8/0x200
> [ 55.091719] [000000000042c4cc] do_softirq_own_stack+0x2c/0x40
> [ 55.168362] [00000000004662d8] irq_exit+0xb8/0xe0
> [ 55.231266] [0000000000870ac0] handler_irq+0xc0/0x100
> [ 55.298756] [00000000004208b4] tl0_irq5+0x14/0x20
> [ 55.361670] [000000000042cafc] arch_cpu_idle+0x9c/0xa0
> [ 55.447055] [000000000048a254] cpu_startup_entry+0x14/0x40
> [ 55.536998] [000000000095f4b4] 0x95f4b4
> [ 55.588471] [0000000040000000] 0x40000000
> [ 179.780371] eth0: hw csum failure
> [ 179.823878] CPU: 3 PID: 0 Comm: swapper/3 Not tainted 4.18.0-12952-g2923b27 #1397
> [ 179.922230] Call Trace:
> [ 179.954267] [000000000077838c] __skb_checksum_complete+0xcc/0xe0
> [ 180.034335] [000000000080fc84] igmp_rcv+0x224/0x920
> [ 180.099536] [00000000007ca3d0] ip_local_deliver+0xb0/0x240
> [ 180.172740] [00000000007ca5c0] ip_rcv+0x60/0xa0
> [ 180.233368] [0000000000781a30] __netif_receive_skb_one_core+0x30/0x60
> [ 180.319159] [0000000000782914] process_backlog+0x94/0x140
> [ 180.391225] [0000000000788f6c] net_rx_action+0x1ec/0x320
> [ 180.462148] [0000000000870de8] __do_softirq+0xc8/0x200
> [ 180.530782] [000000000042c4cc] do_softirq_own_stack+0x2c/0x40
> [ 180.607422] [00000000004662d8] irq_exit+0xb8/0xe0
> [ 180.670331] [0000000000870ac0] handler_irq+0xc0/0x100
> [ 180.737822] [00000000004208b4] tl0_irq5+0x14/0x20
> [ 180.800735] [000000000042caf8] arch_cpu_idle+0x98/0xa0
> [ 180.869373] [0000000000489f60] do_idle+0xe0/0x1c0
> [ 180.932281] [000000000048a25c] cpu_startup_entry+0x1c/0x40
> [ 181.005491] [000000000098e9b4] start_kernel+0x3b8/0x3c8
Note that these traces are for non TCP packets.
I suspect the driver should not provide CHECKSUM_COMPLETE for non TCP
packets, maybe the NIC is mishandling this case.
^ permalink raw reply
* Re: [PATCH net-next 02/10] dt-bindings: net: ocelot: remove hsio from the list of register address spaces
From: Alexandre Belloni @ 2018-08-27 20:57 UTC (permalink / raw)
To: Quentin Schulz
Cc: Rob Herring, ralf, paul.burton, jhogan, mark.rutland, davem,
kishon, andrew, f.fainelli, linux-mips, devicetree, linux-kernel,
netdev, allan.nielsen, thomas.petazzoni
In-Reply-To: <20180816142514.pf4eiqeditoy4uei@qschulz>
On 16/08/2018 16:25:14+0200, Quentin Schulz wrote:
> Hi Alexandre,
>
> On Tue, Aug 14, 2018 at 02:41:35PM +0200, Alexandre Belloni wrote:
> > On 14/08/2018 08:49:53+0200, Quentin Schulz wrote:
> > > Understood but it's an intermediate patch. Later (patch 8), the SerDes
> > > muxing "controller" is added as a child to this node. There most likely
> > > will be some others in the future (temperature sensor for example).
> > >
> > > Furthermore, there's already a simple-mfd without children in this file:
> > > https://elixir.bootlin.com/linux/latest/source/Documentation/devicetree/bindings/mips/mscc.txt#L19
> > >
> > > How should we handle this case?
> > >
> >
> > There were child nodes in previous version of the binding. You can
> > remove simple-mfd now. The useful registers that are not used by any
> > drivers are gpr and chipid.
> >
>
> And what about the use case I'm facing? I've got child nodes defined in
> it but with a later patch (but they actually haven't made it to the
> DT binding documentation, so that's for the next version of the patch
> series).
>
I guess you should keep simple-mfd for hsio because it will have child
nodes.
--
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox