* Re: [PATCH] net: bonding: Fix transmit load balancing in balance-alb mode if specified by sysfs
From: Mahesh Bandewar (महेश बंडेवार) @ 2017-09-08 23:54 UTC (permalink / raw)
To: Nikolay Aleksandrov
Cc: Kosuke Tatsukawa, Jay Vosburgh, Veaceslav Falico, Andy Gospodarek,
netdev@vger.kernel.org, linux-kernel@vger.kernel.org,
Reinis Rozitis
In-Reply-To: <03b61877-053b-2f0e-dc35-8fe31cc90c08@cumulusnetworks.com>
On Fri, Sep 8, 2017 at 7:30 AM, Nikolay Aleksandrov
<nikolay@cumulusnetworks.com> wrote:
> On 08/09/17 17:17, Kosuke Tatsukawa wrote:
>> Hi,
>>
>>> On 08/09/17 13:10, Nikolay Aleksandrov wrote:
>>>> On 08/09/17 05:06, Kosuke Tatsukawa wrote:
>>>>> Hi,
>>>>>
>>>>>> On 7.09.2017 01:47, Kosuke Tatsukawa wrote:
>>>>>>> Commit cbf5ecb30560 ("net: bonding: Fix transmit load balancing in
>>>>>>> balance-alb mode") tried to fix transmit dynamic load balancing in
>>>>>>> balance-alb mode, which wasn't working after commit 8b426dc54cf4
>>>>>>> ("bonding: remove hardcoded value").
>>>>>>>
>>>>>>> It turned out that my previous patch only fixed the case when
>>>>>>> balance-alb was specified as bonding module parameter, and not when
>>>>>>> balance-alb mode was set using /sys/class/net/*/bonding/mode (the most
>>>>>>> common usage). In the latter case, tlb_dynamic_lb was set up according
>>>>>>> to the default mode of the bonding interface, which happens to be
>>>>>>> balance-rr.
>>>>>>>
>>>>>>> This additional patch addresses this issue by setting up tlb_dynamic_lb
>>>>>>> to 1 if "mode" is set to balance-alb through the sysfs interface.
>>>>>>>
>>>>>>> I didn't add code to change tlb_balance_lb back to the default value for
>>>>>>> other modes, because "mode" is usually set up only once during
>>>>>>> initialization, and it's not worthwhile to change the static variable
>>>>>>> bonding_defaults in bond_main.c to a global variable just for this
>>>>>>> purpose.
>>>>>>>
>>>>>>> Commit 8b426dc54cf4 also changes the value of tlb_dynamic_lb for
>>>>>>> balance-tlb mode if it is set up using the sysfs interface. I didn't
>>>>>>> change that behavior, because the value of tlb_balance_lb can be changed
>>>>>>> using the sysfs interface for balance-tlb, and I didn't like changing
>>>>>>> the default value back and forth for balance-tlb.
>>>>>>>
>>>>>>> As for balance-alb, /sys/class/net/*/bonding/tlb_balance_lb cannot be
>>>>>>> written to. However, I think balance-alb with tlb_dynamic_lb set to 0
>>>>>>> is not an intended usage, so there is little use making it writable at
>>>>>>> this moment.
>>>>>>>
>>>>>>> Fixes: 8b426dc54cf4 ("bonding: remove hardcoded value")
>>>>>>> Reported-by: Reinis Rozitis <r@roze.lv>
>>>>>>> Signed-off-by: Kosuke Tatsukawa <tatsu@ab.jp.nec.com>
>>>>>>> Cc: stable@vger.kernel.org # v4.12+
>>>>>>> ---
>>>>>>> drivers/net/bonding/bond_options.c | 3 +++
>>>>>>> 1 files changed, 3 insertions(+), 0 deletions(-)
>>>>>>>
>>>>>>
>>>>>> I don't believe this to be the right solution, hardcoding it like this
>>>>>> changes user-visible behaviour. The issue is that if someone configured
>>>>>> it to be 0 in tlb mode, suddenly it will become 1 and will silently
>>>>>> override their config if they switch the mode to alb. Also it robs users
>>>>>> from their choice.
>>>>>>
>>>>>> If you think this should be settable in ALB mode, then IMO you should
>>>>>> edit tlb_dynamic_lb option's unsuppmodes and allow it to be set in ALB.
>>>>>> That would also be consistent with how it's handled in TLB mode.
>>>>>
>>>>> No, I don't think tlb_dynamic_lb should be settable in balance-alb at
>>>>> this point. All the current commits regarding tlb_dynamic_lb are for
>>>>> balance-tlb mode, so I don't think balance-alb with tlb_dynamic_lb set
>>>>> to 0 is an intended usage.
>>>>>
>>>>>
>>>>>> Going back and looking at your previous fix I'd argue that it is also
>>>>>> wrong, you should've removed the mode check altogether to return the
>>>>>> original behaviour where the dynamic_lb is set to 1 (enabled) by
>>>>>> default and then ALB mode would've had it, of course that would've left
>>>>>> the case of setting it to 0 in TLB mode and switching to ALB, but that
>>>>>> is a different issue.
>>>>>
>>>>> Maybe balance-alb shouldn't be dependent on tlb_dynamic_lb.
>>>>> tlb_dynamic_lb is referenced in the following functions.
>>>>>
>>>>> + bond_do_alb_xmit() -- Used by both balance-tlb and balance-alb
>>>>> + bond_tlb_xmit() -- Only used by balance-tlb
>>>>> + bond_open() -- Used by both balance-tlb and balance-alb
>>>>> + bond_check_params() -- Used during module initialization
>>>>> + bond_fill_info() -- Used to get/set value
>>>>> + bond_option_tlb_dynamic_lb_set() -- Used to get/set value
>>>>> + bonding_show_tlb_dynamic_lb() -- Used to get/set value
>>>>> + bond_is_nondyn_tlb() -- Only referenced if balance-tlb mode
>>>>>
>>>>> The following untested patch adds code to make balance-alb work as if
>>>>> tlb_dynamic_lb==1 for the functions which affect balance-alb mode. It
>>>>> also reverts my previous patch.
>>>>>
>>>>> What do you think about this approach?
>>>>> ---
>>>>> Kosuke TATSUKAWA | 1st Platform Software Division
>>>>> | NEC Solution Innovators
>>>>> | tatsu@ab.jp.nec.com
>>>>>
>>>>
>>>> Logically the approach looks good, that being said it adds unnecessary tests in
>>>> the fast path, why not just something like the patch below ? That only leaves the
>>>> problem if it is zeroed in TLB and switched to ALB mode, and that is a one line
>>>> fix to unsuppmodes just allow it to be set for that specific case. The below
>>>> returns the default behaviour before the commit in your Fixes tag.
>>>>
>>>>
>>>
>>> Actually I'm fine with your approach, too. It will fix this regardless of the
>>> value of tlb_dynamic_lb which sounds good to me for the price of a test in
>>> the fast path.
>>
>> If you're concerned about the additional test in the fast path, how
>> about the patch below. I've added an arguemnt to bond_do_alb_xmit()
>> to handle both balance-tlb and balance-alb similary.
>>
>
> Even better, looks great! 1 question below though.
>
>> I'm not sure if this causes any problem if tlb_dynamic_lb is changed
>> while calling bond_do_alb_xmit() in balance-tlb mode.
>
> The option has the ifdown flag, you shouldn't be able to change it while
> the bond dev is up, but even if you could I don't think it will be an issue
> for the xmit.
>
>> ---
>> Kosuke TATSUKAWA | 1st Platform Software Division
>> | NEC Solution Innovators
>> | tatsu@ab.jp.nec.com
>>
>> ------------------------------------------------------------------------
>> drivers/net/bonding/bond_alb.c | 11 ++++++-----
>> drivers/net/bonding/bond_main.c | 5 +++--
>> 2 files changed, 9 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/net/bonding/bond_alb.c b/drivers/net/bonding/bond_alb.c
>> index c02cc81..7710f20 100644
>> --- a/drivers/net/bonding/bond_alb.c
>> +++ b/drivers/net/bonding/bond_alb.c
>> @@ -1317,7 +1317,7 @@ void bond_alb_deinitialize(struct bonding *bond)
>> }
>>
>> static int bond_do_alb_xmit(struct sk_buff *skb, struct bonding *bond,
>> - struct slave *tx_slave)
>> + struct slave *tx_slave, int tlb_dynamic_lb)
>> {
>> struct alb_bond_info *bond_info = &(BOND_ALB_INFO(bond));
>> struct ethhdr *eth_data = eth_hdr(skb);
>> @@ -1325,7 +1325,7 @@ static int bond_do_alb_xmit(struct sk_buff *skb, struct bonding *bond,
>> if (!tx_slave) {
>> /* unbalanced or unassigned, send through primary */
>> tx_slave = rcu_dereference(bond->curr_active_slave);
>> - if (bond->params.tlb_dynamic_lb)
>> + if (tlb_dynamic_lb)
>> bond_info->unbalanced_load += skb->len;
>> }
>>
>> @@ -1339,7 +1339,7 @@ static int bond_do_alb_xmit(struct sk_buff *skb, struct bonding *bond,
>> goto out;
>> }
>>
>> - if (tx_slave && bond->params.tlb_dynamic_lb) {
>> + if (tx_slave && tlb_dynamic_lb) {
>> spin_lock(&bond->mode_lock);
>> __tlb_clear_slave(bond, tx_slave, 0);
>> spin_unlock(&bond->mode_lock);
>> @@ -1386,7 +1386,8 @@ int bond_tlb_xmit(struct sk_buff *skb, struct net_device *bond_dev)
>> break;
>> }
>> }
>> - return bond_do_alb_xmit(skb, bond, tx_slave);
>> + return bond_do_alb_xmit(skb, bond, tx_slave,
>> + bond->params.tlb_dynamic_lb);
>> }
>>
>> int bond_alb_xmit(struct sk_buff *skb, struct net_device *bond_dev)
>> @@ -1483,7 +1484,7 @@ int bond_alb_xmit(struct sk_buff *skb, struct net_device *bond_dev)
>> tx_slave = tlb_choose_channel(bond, hash_index, skb->len);
>> }
>>
>> - return bond_do_alb_xmit(skb, bond, tx_slave);
>> + return bond_do_alb_xmit(skb, bond, tx_slave, 1);
>> }
>>
>> void bond_alb_monitor(struct work_struct *work)
>> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>> index fc63992..bcb71e7 100644
>> --- a/drivers/net/bonding/bond_main.c
>> +++ b/drivers/net/bonding/bond_main.c
>> @@ -3305,7 +3305,8 @@ static int bond_open(struct net_device *bond_dev)
>> */
>> if (bond_alb_initialize(bond, (BOND_MODE(bond) == BOND_MODE_ALB)))
>> return -ENOMEM;
>> - if (bond->params.tlb_dynamic_lb)
>> + if (bond->params.tlb_dynamic_lb ||
>> + (BOND_MODE(bond) == BOND_MODE_TLB))
>
> mode == tlb ? shouldn't this check be for alb ?
>
Actually this is not needed since this is already inside if
(bond_is_lb()) condition.
>> queue_delayed_work(bond->wq, &bond->alb_work, 0);
>> }
>>
>> @@ -4601,7 +4602,7 @@ static int bond_check_params(struct bond_params *params)
>> }
>> ad_user_port_key = valptr->value;
>>
>> - if ((bond_mode == BOND_MODE_TLB) || (bond_mode == BOND_MODE_ALB)) {
>> + if (bond_mode == BOND_MODE_TLB) {
>> bond_opt_initstr(&newval, "default");
>> valptr = bond_opt_parse(bond_opt_get(BOND_OPT_TLB_DYNAMIC_LB),
>> &newval);
>>
>
I think the underlying issue is that tlb_dynamic_lb should be set to 1
for all modes which was not the case when it was getting initialized
only forTLB mode. So from that perspective I prefer Nik's patch with a
small variation that guards the case when mode transitions from TLB to
ALB. The reason why I like that patch is because it's simple and
avoids complications.
Here is what I meant -
diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index fc63992ab0e0..c99dc59d729b 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -4289,7 +4289,7 @@ static int bond_check_params(struct bond_params *params)
int bond_mode = BOND_MODE_ROUNDROBIN;
int xmit_hashtype = BOND_XMIT_POLICY_LAYER2;
int lacp_fast = 0;
- int tlb_dynamic_lb = 0;
+ int tlb_dynamic_lb;
/* Convert string parameters. */
if (mode) {
@@ -4601,16 +4601,13 @@ static int bond_check_params(struct bond_params *params)
}
ad_user_port_key = valptr->value;
- if ((bond_mode == BOND_MODE_TLB) || (bond_mode == BOND_MODE_ALB)) {
- bond_opt_initstr(&newval, "default");
- valptr = bond_opt_parse(bond_opt_get(BOND_OPT_TLB_DYNAMIC_LB),
- &newval);
- if (!valptr) {
- pr_err("Error: No tlb_dynamic_lb default value");
- return -EINVAL;
- }
- tlb_dynamic_lb = valptr->value;
+ bond_opt_initstr(&newval, "default");
+ valptr = bond_opt_parse(bond_opt_get(BOND_OPT_TLB_DYNAMIC_LB), &newval);
+ if (!valptr) {
+ pr_err("Error: No tlb_dynamic_lb default value");
+ return -EINVAL;
}
+ tlb_dynamic_lb = valptr->value;
if (lp_interval == 0) {
pr_warn("Warning: ip_interval must be between 1 and
%d, so it was reset to %d\n",
diff --git a/drivers/net/bonding/bond_options.c
b/drivers/net/bonding/bond_options.c
index a12d603d41c6..7feacd262182 100644
--- a/drivers/net/bonding/bond_options.c
+++ b/drivers/net/bonding/bond_options.c
@@ -754,6 +754,12 @@ static int bond_option_mode_set(struct bonding *bond,
bond->params.miimon);
}
+ /* Guard against transition TLB/tlb_dynamic_lb=0 -> ALB mode.
+ * In ALB mode, tlb_dynamic_lb must be set to 1.
+ */
+ if (newval->value == BOND_MODE_ALB && bond->params.tlb_dynamic_lb != 1)
+ bond->params.tlb_dynamic_lb = 1;
+
/* don't cache arp_validate between modes */
bond->params.arp_validate = BOND_ARP_VALIDATE_NONE;
bond->params.mode = newval->value;
^ permalink raw reply related
* [PATCH net] bpf: make error reporting in bpf_warn_invalid_xdp_action more clear
From: Daniel Borkmann @ 2017-09-08 23:40 UTC (permalink / raw)
To: davem; +Cc: ast, john.fastabend, netdev, Daniel Borkmann
Differ between illegal XDP action code and just driver
unsupported one to provide better feedback when we throw
a one-time warning here. Reason is that with 814abfabef3c
("xdp: add bpf_redirect helper function") not all drivers
support the new XDP return code yet and thus they will
fall into their 'default' case when checking for return
codes after program return, which then triggers a
bpf_warn_invalid_xdp_action() stating that the return
code is illegal, but from XDP perspective it's not.
I decided not to place something like a XDP_ACT_MAX define
into uapi i) given we don't have this either for all other
program types, ii) future action codes could have further
encoding there, which would render such define unsuitable
and we wouldn't be able to rip it out again, and iii) we
rarely add new action codes.
Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
Acked-by: Alexei Starovoitov <ast@kernel.org>
---
include/uapi/linux/bpf.h | 4 ++--
net/core/filter.c | 6 +++++-
2 files changed, 7 insertions(+), 3 deletions(-)
diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index ba848b7..43ab5c4 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -766,8 +766,8 @@ struct bpf_sock {
/* User return codes for XDP prog type.
* A valid XDP program must return one of these defined values. All other
- * return codes are reserved for future use. Unknown return codes will result
- * in packet drop.
+ * return codes are reserved for future use. Unknown return codes will
+ * result in packet drops and a warning via bpf_warn_invalid_xdp_action().
*/
enum xdp_action {
XDP_ABORTED = 0,
diff --git a/net/core/filter.c b/net/core/filter.c
index 0848df2..adac4eb 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -3609,7 +3609,11 @@ static bool xdp_is_valid_access(int off, int size,
void bpf_warn_invalid_xdp_action(u32 act)
{
- WARN_ONCE(1, "Illegal XDP return value %u, expect packet loss\n", act);
+ const u32 act_max = XDP_REDIRECT;
+
+ WARN_ONCE(1, "%s XDP return value %u, expect packet loss!\n",
+ act > act_max ? "Illegal" : "Driver unsupported",
+ act);
}
EXPORT_SYMBOL_GPL(bpf_warn_invalid_xdp_action);
--
1.9.3
^ permalink raw reply related
* Re: [PATCH net] Revert "mdio_bus: Remove unneeded gpiod NULL check"
From: Linus Walleij @ 2017-09-08 23:24 UTC (permalink / raw)
To: Florian Fainelli
Cc: netdev@vger.kernel.org, David S. Miller, Andrew Lunn,
Fabio Estevam, Sergei Shtylyov, Fabio Estevam, Bryan.Whitehead
In-Reply-To: <fe6846be-afc3-1025-d78a-96674e1b2a35@gmail.com>
On Sat, Sep 9, 2017 at 1:18 AM, Florian Fainelli <f.fainelli@gmail.com> wrote:
> On 09/08/2017 04:13 PM, Linus Walleij wrote:
>> On Sat, Sep 9, 2017 at 12:38 AM, Florian Fainelli <f.fainelli@gmail.com> wrote:
>>
>>> This reverts commit 95b80bf3db03c2bf572a357cf74b9a6aefef0a4a ("mdio_bus:
>>> Remove unneeded gpiod NULL check"), this commit assumed that GPIOLIB
>>> checks for NULL descriptors, so it's safe to drop them, but it is not
>>> when CONFIG_GPIOLIB is disabled in the kernel. If we do call
>>> gpiod_set_value_cansleep() on a GPIO descriptor we will issue warnings
>>> coming from the inline stubs declared in include/linux/gpio/consumer.h.
>>>
>>> Fixes: 95b80bf3db03 ("mdio_bus: Remove unneeded gpiod NULL check")
>>> Reported-by: Woojung Huh <Woojung.Huh@microchip.com>
>>> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
>>
>> Yeah I guess you don't wanna have these messages spewing
>> in the console. :/
>>
>> But what about simply doing this:
>
> That would create another dependency which really does not need to be
> there as it really prevents you from testing more configurations,
> including but not limited to having CONFIG_GPIOLIB=n w/
> CONFIG_MDIO_DEVICE=[ym].
>
> The GPIOLIB=n inline stubs have a "contract" with the code calling them
> that is fairly clear, which is what this revert is leveraging.
Ayeah the contract is something like "you can call us if compiled out,
but then you get warnings".
Yeah NULL checks does the trick as well as #ifdef:in in that sense.
It's no big deal so if you prefer this:
Acked-by: Linus Walleij <linus.walleij@linaro.org>
Yours,
Linus Walleij
^ permalink raw reply
* Re: [PATCH net] Revert "mdio_bus: Remove unneeded gpiod NULL check"
From: Florian Fainelli @ 2017-09-08 23:18 UTC (permalink / raw)
To: Linus Walleij
Cc: netdev@vger.kernel.org, David S. Miller, Andrew Lunn,
Fabio Estevam, Sergei Shtylyov, Fabio Estevam, Bryan.Whitehead
In-Reply-To: <CACRpkdZD-nRXXusX36Gaypf5A601CMEO3vVgaOV5tKufvy1M3Q@mail.gmail.com>
On 09/08/2017 04:13 PM, Linus Walleij wrote:
> On Sat, Sep 9, 2017 at 12:38 AM, Florian Fainelli <f.fainelli@gmail.com> wrote:
>
>> This reverts commit 95b80bf3db03c2bf572a357cf74b9a6aefef0a4a ("mdio_bus:
>> Remove unneeded gpiod NULL check"), this commit assumed that GPIOLIB
>> checks for NULL descriptors, so it's safe to drop them, but it is not
>> when CONFIG_GPIOLIB is disabled in the kernel. If we do call
>> gpiod_set_value_cansleep() on a GPIO descriptor we will issue warnings
>> coming from the inline stubs declared in include/linux/gpio/consumer.h.
>>
>> Fixes: 95b80bf3db03 ("mdio_bus: Remove unneeded gpiod NULL check")
>> Reported-by: Woojung Huh <Woojung.Huh@microchip.com>
>> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
>
> Yeah I guess you don't wanna have these messages spewing
> in the console. :/
>
> But what about simply doing this:
That would create another dependency which really does not need to be
there as it really prevents you from testing more configurations,
including but not limited to having CONFIG_GPIOLIB=n w/
CONFIG_MDIO_DEVICE=[ym].
The GPIOLIB=n inline stubs have a "contract" with the code calling them
that is fairly clear, which is what this revert is leveraging.
Instead of doing what you suggest, we could also encapsulate the
offending code within an #IS_ENABLED(CONFIG_GPIOLIB) block, which would
be virtually the same thing in terms of object code generated, but would
make it clear there is a functional dependency, I would personally be
keener on that approach than having a select or depends.
Thanks
>
>
> From 150e4f3c1f227c9a4c31bbb48d44220e25b792ec Mon Sep 17 00:00:00 2001
> From: Linus Walleij <linus.walleij@linaro.org>
> Date: Sat, 9 Sep 2017 01:07:22 +0200
> Subject: [PATCH] net: phy: mdio_bus: mandate gpiolib
>
> The core mdio bus unconditionally uses gpiolib APIs to handle
> reset lines which results in runtime errors when it is compiled
> out. It is not supposed to be possible to stub out gpiolib
> like this, the error messages from the stubs are a sign that
> something is wrong.
>
> So just select it. Fix some unneeded #includes while we're
> at it.>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
> drivers/net/phy/Kconfig | 1 +
> drivers/net/phy/mdio_bus.c | 2 --
> 2 files changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig
> index a9d16a3af514..b6c5bb9941c3 100644
> --- a/drivers/net/phy/Kconfig
> +++ b/drivers/net/phy/Kconfig
> @@ -4,6 +4,7 @@
>
> menuconfig MDIO_DEVICE
> tristate "MDIO bus device drivers"
> + select GPIOLIB
> help
> MDIO devices and driver infrastructure code.
>
> diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c
> index b6f9fa670168..9c60f87b7209 100644
> --- a/drivers/net/phy/mdio_bus.c
> +++ b/drivers/net/phy/mdio_bus.c
> @@ -22,11 +22,9 @@
> #include <linux/init.h>
> #include <linux/delay.h>
> #include <linux/device.h>
> -#include <linux/gpio.h>
> #include <linux/gpio/consumer.h>
> #include <linux/of_device.h>
> #include <linux/of_mdio.h>
> -#include <linux/of_gpio.h>
> #include <linux/netdevice.h>
> #include <linux/etherdevice.h>
> #include <linux/skbuff.h>
>
--
Florian
^ permalink raw reply
* Re: [PATCH net-next] mdio_bus: Remove unneeded gpiod NULL check
From: Linus Walleij @ 2017-09-08 23:17 UTC (permalink / raw)
To: Florian Fainelli
Cc: Woojung.Huh, Andrew Lunn, Fabio Estevam, Sergei Shtylyov,
David S. Miller, netdev@vger.kernel.org, Fabio Estevam
In-Reply-To: <81338659-4f46-0f2f-13c3-135b3ac876d7@gmail.com>
On Thu, Sep 7, 2017 at 11:39 PM, Florian Fainelli <f.fainelli@gmail.com> wrote:
> I think it means CONFIG_GPIOLIB=n in the kernel because it's not needed,
> yet you run code (like drivers/net/phy/mdio_bus.c) that unconditionally
> calls into GPIOLIB and attempts to configure a given GPIO if available.
> This thread is actually what prompted me to write this email:
Yeah I think GPIOLIB should simply be selected in KConfig then.
Sent an inline patch to do that in the other reply.
Maybe I should simply delete the stubs if they confuse
people.
It is possible to select GPIOLIB on any platform these days
and get the right APIs, this used to not be the case but I fixed
it a while back.
Alternatively we can depend on GPIOLIB but it's simpler to just
select it I think, it's like a library this code uses and so that's
it.
Yours,
Linus Walleij
^ permalink raw reply
* Re: [PATCH net] Revert "mdio_bus: Remove unneeded gpiod NULL check"
From: Linus Walleij @ 2017-09-08 23:13 UTC (permalink / raw)
To: Florian Fainelli
Cc: netdev@vger.kernel.org, David S. Miller, Andrew Lunn,
Fabio Estevam, Sergei Shtylyov, Fabio Estevam, Bryan.Whitehead
In-Reply-To: <20170908223807.6015-1-f.fainelli@gmail.com>
On Sat, Sep 9, 2017 at 12:38 AM, Florian Fainelli <f.fainelli@gmail.com> wrote:
> This reverts commit 95b80bf3db03c2bf572a357cf74b9a6aefef0a4a ("mdio_bus:
> Remove unneeded gpiod NULL check"), this commit assumed that GPIOLIB
> checks for NULL descriptors, so it's safe to drop them, but it is not
> when CONFIG_GPIOLIB is disabled in the kernel. If we do call
> gpiod_set_value_cansleep() on a GPIO descriptor we will issue warnings
> coming from the inline stubs declared in include/linux/gpio/consumer.h.
>
> Fixes: 95b80bf3db03 ("mdio_bus: Remove unneeded gpiod NULL check")
> Reported-by: Woojung Huh <Woojung.Huh@microchip.com>
> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
Yeah I guess you don't wanna have these messages spewing
in the console. :/
But what about simply doing this:
>From 150e4f3c1f227c9a4c31bbb48d44220e25b792ec Mon Sep 17 00:00:00 2001
From: Linus Walleij <linus.walleij@linaro.org>
Date: Sat, 9 Sep 2017 01:07:22 +0200
Subject: [PATCH] net: phy: mdio_bus: mandate gpiolib
The core mdio bus unconditionally uses gpiolib APIs to handle
reset lines which results in runtime errors when it is compiled
out. It is not supposed to be possible to stub out gpiolib
like this, the error messages from the stubs are a sign that
something is wrong.
So just select it. Fix some unneeded #includes while we're
at it.
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
drivers/net/phy/Kconfig | 1 +
drivers/net/phy/mdio_bus.c | 2 --
2 files changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig
index a9d16a3af514..b6c5bb9941c3 100644
--- a/drivers/net/phy/Kconfig
+++ b/drivers/net/phy/Kconfig
@@ -4,6 +4,7 @@
menuconfig MDIO_DEVICE
tristate "MDIO bus device drivers"
+ select GPIOLIB
help
MDIO devices and driver infrastructure code.
diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c
index b6f9fa670168..9c60f87b7209 100644
--- a/drivers/net/phy/mdio_bus.c
+++ b/drivers/net/phy/mdio_bus.c
@@ -22,11 +22,9 @@
#include <linux/init.h>
#include <linux/delay.h>
#include <linux/device.h>
-#include <linux/gpio.h>
#include <linux/gpio/consumer.h>
#include <linux/of_device.h>
#include <linux/of_mdio.h>
-#include <linux/of_gpio.h>
#include <linux/netdevice.h>
#include <linux/etherdevice.h>
#include <linux/skbuff.h>
--
2.13.5
Yours,
Linus Walleij
^ permalink raw reply related
* Re: [PATCH net] ipv6: fix typo in fib6_net_exit()
From: Sabrina Dubroca @ 2017-09-08 23:09 UTC (permalink / raw)
To: Eric Dumazet; +Cc: David Miller, netdev
In-Reply-To: <1504910927.15310.105.camel@edumazet-glaptop3.roam.corp.google.com>
2017-09-08, 15:48:47 -0700, Eric Dumazet wrote:
> From: Eric Dumazet <edumazet@google.com>
>
> IPv6 FIB should use FIB6_TABLE_HASHSZ, not FIB_TABLE_HASHSZ.
>
> Fixes: ba1cc08d9488 ("ipv6: fix memory leak with multiple tables during netns destruction")
> Signed-off-by: Eric Dumazet <edumazet@google.com>
Ouch, yes :(
Thanks for the fix.
Acked-by: Sabrina Dubroca <sd@queasysnail.net>
--
Sabrina
^ permalink raw reply
* Re: [PATCH net] ipv6: fix typo in fib6_net_exit()
From: David Miller @ 2017-09-08 23:09 UTC (permalink / raw)
To: eric.dumazet; +Cc: sd, netdev
In-Reply-To: <1504910927.15310.105.camel@edumazet-glaptop3.roam.corp.google.com>
From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Fri, 08 Sep 2017 15:48:47 -0700
> From: Eric Dumazet <edumazet@google.com>
>
> IPv6 FIB should use FIB6_TABLE_HASHSZ, not FIB_TABLE_HASHSZ.
>
> Fixes: ba1cc08d9488 ("ipv6: fix memory leak with multiple tables during netns destruction")
> Signed-off-by: Eric Dumazet <edumazet@google.com>
Applied.
^ permalink raw reply
* Re: [PATCH net] tcp: fix a request socket leak
From: David Miller @ 2017-09-08 23:08 UTC (permalink / raw)
To: alexei.starovoitov; +Cc: eric.dumazet, netdev
In-Reply-To: <20170908225921.bxbgaldzt5fslpda@ast-mbp>
From: Alexei Starovoitov <alexei.starovoitov@gmail.com>
Date: Fri, 8 Sep 2017 15:59:23 -0700
> On Fri, Sep 08, 2017 at 12:44:47PM -0700, Eric Dumazet wrote:
>> From: Eric Dumazet <edumazet@google.com>
>>
>> While the cited commit fixed a possible deadlock, it added a leak
>> of the request socket, since reqsk_put() must be called if the BPF
>> filter decided the ACK packet must be dropped.
>>
>> Fixes: d624d276d1dd ("tcp: fix possible deadlock in TCP stack vs BPF filter")
>> Signed-off-by: Eric Dumazet <edumazet@google.com>
>> ---
>> David, this is a stable candidate (linux-4.13 has this bug)
>
> Thanks for the fix
> Acked-by: Alexei Starovoitov <ast@kernel.org>
> .. since the fix makes sense to me and as a reminder not to forget
> to backport it as well :)
Applied and queued up for -stable.
^ permalink raw reply
* Re: [PATCH net] tcp: fix a request socket leak
From: Alexei Starovoitov @ 2017-09-08 22:59 UTC (permalink / raw)
To: Eric Dumazet; +Cc: David Miller, netdev
In-Reply-To: <1504899887.15310.95.camel@edumazet-glaptop3.roam.corp.google.com>
On Fri, Sep 08, 2017 at 12:44:47PM -0700, Eric Dumazet wrote:
> From: Eric Dumazet <edumazet@google.com>
>
> While the cited commit fixed a possible deadlock, it added a leak
> of the request socket, since reqsk_put() must be called if the BPF
> filter decided the ACK packet must be dropped.
>
> Fixes: d624d276d1dd ("tcp: fix possible deadlock in TCP stack vs BPF filter")
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> ---
> David, this is a stable candidate (linux-4.13 has this bug)
Thanks for the fix
Acked-by: Alexei Starovoitov <ast@kernel.org>
.. since the fix makes sense to me and as a reminder not to forget
to backport it as well :)
^ permalink raw reply
* [PATCH net] ipv6: fix typo in fib6_net_exit()
From: Eric Dumazet @ 2017-09-08 22:48 UTC (permalink / raw)
To: Sabrina Dubroca, David Miller; +Cc: netdev
In-Reply-To: <1504893592.15310.80.camel@edumazet-glaptop3.roam.corp.google.com>
From: Eric Dumazet <edumazet@google.com>
IPv6 FIB should use FIB6_TABLE_HASHSZ, not FIB_TABLE_HASHSZ.
Fixes: ba1cc08d9488 ("ipv6: fix memory leak with multiple tables during netns destruction")
Signed-off-by: Eric Dumazet <edumazet@google.com>
---
net/ipv6/ip6_fib.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/net/ipv6/ip6_fib.c b/net/ipv6/ip6_fib.c
index 8280172c806ca47c5ca4aef723d405a0a8d7df2a..e5308d7cbd75c4fb67861082382122d445cf5b74 100644
--- a/net/ipv6/ip6_fib.c
+++ b/net/ipv6/ip6_fib.c
@@ -2033,7 +2033,7 @@ static void fib6_net_exit(struct net *net)
rt6_ifdown(net, NULL);
del_timer_sync(&net->ipv6.ip6_fib_timer);
- for (i = 0; i < FIB_TABLE_HASHSZ; i++) {
+ for (i = 0; i < FIB6_TABLE_HASHSZ; i++) {
struct hlist_head *head = &net->ipv6.fib_table_hash[i];
struct hlist_node *tmp;
struct fib6_table *tb;
^ permalink raw reply related
* Re: [net PATCH 3/3] bpf: devmap, use cond_resched instead of cpu_relax
From: Alexei Starovoitov @ 2017-09-08 22:40 UTC (permalink / raw)
To: John Fastabend, davem; +Cc: netdev, daniel
In-Reply-To: <150490447017.11590.17727368565733920857.stgit@john-XPS-13-9360>
On 9/8/17 2:01 PM, John Fastabend wrote:
> Be a bit more friendly about waiting for flush bits to complete.
> Replace the cpu_relax() with a cond_resched().
>
> Suggested-by: Daniel Borkmann <daniel@iogearbox.net>
> Acked-by: Daniel Borkmann <daniel@iogearbox.net>
> Signed-off-by: John Fastabend <john.fastabend@gmail.com>
unlike patch 1 and 2, this one could have waited till net-next opens,
but I don't mind now. lgtm
Acked-by: Alexei Starovoitov <ast@kernel.org>
^ permalink raw reply
* Re: [net PATCH 2/3] bpf: add support for sockmap detach programs
From: Alexei Starovoitov @ 2017-09-08 22:38 UTC (permalink / raw)
To: John Fastabend, davem; +Cc: netdev, daniel
In-Reply-To: <150490444956.11590.4733752227675213913.stgit@john-XPS-13-9360>
On 9/8/17 2:00 PM, John Fastabend wrote:
> The bpf map sockmap supports adding programs via attach commands. This
> patch adds the detach command to keep the API symmetric and allow
> users to remove previously added programs. Otherwise the user would
> have to delete the map and re-add it to get in this state.
>
> This also adds a series of additional tests to capture detach operation
> and also attaching/detaching invalid prog types.
>
> API note: socks will run (or not run) programs depending on the state
> of the map at the time the sock is added. We do not for example walk
> the map and remove programs from previously attached socks.
>
> Acked-by: Daniel Borkmann <daniel@iogearbox.net>
> Signed-off-by: John Fastabend <john.fastabend@gmail.com>
Nice clean patch. Thx
Acked-by: Alexei Starovoitov <ast@kernel.org>
^ permalink raw reply
* Re: [net PATCH 1/3] net: rcu lock and preempt disable missing around generic xdp
From: Alexei Starovoitov @ 2017-09-08 22:38 UTC (permalink / raw)
To: John Fastabend, davem; +Cc: netdev, daniel
In-Reply-To: <150490443011.11590.2847947557652786219.stgit@john-XPS-13-9360>
On 9/8/17 2:00 PM, John Fastabend wrote:
> do_xdp_generic must be called inside rcu critical section with preempt
> disabled to ensure BPF programs are valid and per-cpu variables used
> for redirect operations are consistent. This patch ensures this is true
> and fixes the splat below.
>
> The netif_receive_skb_internal() code path is now broken into two rcu
> critical sections. I decided it was better to limit the preempt_enable/disable
> block to just the xdp static key portion and the fallout is more
> rcu_read_lock/unlock calls. Seems like the best option to me.
>
> [ 607.596901] =============================
> [ 607.596906] WARNING: suspicious RCU usage
> [ 607.596912] 4.13.0-rc4+ #570 Not tainted
> [ 607.596917] -----------------------------
> [ 607.596923] net/core/dev.c:3948 suspicious rcu_dereference_check() usage!
> [ 607.596927]
> [ 607.596927] other info that might help us debug this:
> [ 607.596927]
> [ 607.596933]
> [ 607.596933] rcu_scheduler_active = 2, debug_locks = 1
> [ 607.596938] 2 locks held by pool/14624:
> [ 607.596943] #0: (rcu_read_lock_bh){......}, at: [<ffffffff95445ffd>] ip_finish_output2+0x14d/0x890
> [ 607.596973] #1: (rcu_read_lock_bh){......}, at: [<ffffffff953c8e3a>] __dev_queue_xmit+0x14a/0xfd0
> [ 607.597000]
> [ 607.597000] stack backtrace:
> [ 607.597006] CPU: 5 PID: 14624 Comm: pool Not tainted 4.13.0-rc4+ #570
> [ 607.597011] Hardware name: Dell Inc. Precision Tower 5810/0HHV7N, BIOS A17 03/01/2017
> [ 607.597016] Call Trace:
> [ 607.597027] dump_stack+0x67/0x92
> [ 607.597040] lockdep_rcu_suspicious+0xdd/0x110
> [ 607.597054] do_xdp_generic+0x313/0xa50
> [ 607.597068] ? time_hardirqs_on+0x5b/0x150
> [ 607.597076] ? mark_held_locks+0x6b/0xc0
> [ 607.597088] ? netdev_pick_tx+0x150/0x150
> [ 607.597117] netif_rx_internal+0x205/0x3f0
> [ 607.597127] ? do_xdp_generic+0xa50/0xa50
> [ 607.597144] ? lock_downgrade+0x2b0/0x2b0
> [ 607.597158] ? __lock_is_held+0x93/0x100
> [ 607.597187] netif_rx+0x119/0x190
> [ 607.597202] loopback_xmit+0xfd/0x1b0
> [ 607.597214] dev_hard_start_xmit+0x127/0x4e0
>
> Fixes: d445516966dc ("net: xdp: support xdp generic on virtual devices")
> Fixes: b5cdae3291f7 ("net: Generic XDP")
> Acked-by: Daniel Borkmann <daniel@iogearbox.net>
> Signed-off-by: John Fastabend <john.fastabend@gmail.com>
argh, so it's due to virtual devices and loopback.
Not pretty, but have to agree I don't see another way of fixing it.
Acked-by: Alexei Starovoitov <ast@kernel.org>
^ permalink raw reply
* [PATCH net] Revert "mdio_bus: Remove unneeded gpiod NULL check"
From: Florian Fainelli @ 2017-09-08 22:38 UTC (permalink / raw)
To: netdev
Cc: davem, linus.walleij, andrew, festevam, sergei.shtylyov,
fabio.estevam, Bryan.Whitehead, Florian Fainelli
This reverts commit 95b80bf3db03c2bf572a357cf74b9a6aefef0a4a ("mdio_bus:
Remove unneeded gpiod NULL check"), this commit assumed that GPIOLIB
checks for NULL descriptors, so it's safe to drop them, but it is not
when CONFIG_GPIOLIB is disabled in the kernel. If we do call
gpiod_set_value_cansleep() on a GPIO descriptor we will issue warnings
coming from the inline stubs declared in include/linux/gpio/consumer.h.
Fixes: 95b80bf3db03 ("mdio_bus: Remove unneeded gpiod NULL check")
Reported-by: Woojung Huh <Woojung.Huh@microchip.com>
Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
drivers/net/phy/mdio_bus.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c
index b6f9fa670168..2df7b62c1a36 100644
--- a/drivers/net/phy/mdio_bus.c
+++ b/drivers/net/phy/mdio_bus.c
@@ -399,7 +399,8 @@ int __mdiobus_register(struct mii_bus *bus, struct module *owner)
}
/* Put PHYs in RESET to save power */
- gpiod_set_value_cansleep(bus->reset_gpiod, 1);
+ if (bus->reset_gpiod)
+ gpiod_set_value_cansleep(bus->reset_gpiod, 1);
device_del(&bus->dev);
return err;
@@ -424,7 +425,8 @@ void mdiobus_unregister(struct mii_bus *bus)
}
/* Put PHYs in RESET to save power */
- gpiod_set_value_cansleep(bus->reset_gpiod, 1);
+ if (bus->reset_gpiod)
+ gpiod_set_value_cansleep(bus->reset_gpiod, 1);
device_del(&bus->dev);
}
--
2.9.3
^ permalink raw reply related
* Re: [PATCH] ipv4: Namespaceify tcp_max_orphans knob
From: Cong Wang @ 2017-09-08 22:13 UTC (permalink / raw)
To: Haishuang Yan
Cc: David S. Miller, Alexey Kuznetsov, Hideaki YOSHIFUJI,
Eric Dumazet, Linux Kernel Network Developers, LKML
In-Reply-To: <1504753808-13266-1-git-send-email-yanhaishuang@cmss.chinamobile.com>
On Wed, Sep 6, 2017 at 8:10 PM, Haishuang Yan
<yanhaishuang@cmss.chinamobile.com> wrote:
> Different namespace application might require different maximal number
> of TCP sockets independently of the host.
So after your patch we could have N * net->ipv4.sysctl_tcp_max_orphans
in a whole system, right? This just makes OOM easier to trigger.
^ permalink raw reply
* Re: [PATCH RFC 3/5] Add KSZ8795 switch driver
From: Pavel Machek @ 2017-09-08 21:57 UTC (permalink / raw)
To: Tristram.Ha
Cc: andrew, muvarov, nathan.leigh.conrad, vivien.didelot, f.fainelli,
netdev, linux-kernel, Woojung.Huh
In-Reply-To: <93AF473E2DA327428DE3D46B72B1E9FD41121E3D@CHN-SV-EXMX02.mchp-main.com>
[-- Attachment #1: Type: text/plain, Size: 531 bytes --]
Hi!
> > > + default:
> > > + processed = false;
> > > + break;
> > > + }
> > > + if (processed)
> > > + *val = data;
> > > +}
> >
> > Similar code will be needed by other drivers, right?
>
> Although KSZ8795 and KSZ8895 may use the same code, the other
> chips will have different code.
Ok, please make sure code is shared between these two.
Thansk,
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]
^ permalink raw reply
* Re: [PATCH net] netlink: fix an use-after-free issue for nlk groups
From: Cong Wang @ 2017-09-08 21:56 UTC (permalink / raw)
To: Xin Long; +Cc: network dev, David Miller, Florian Westphal, chunwang, syzkaller
In-Reply-To: <1217baf9faf9d034ba8224cb6362b822c51a0eae.1504669632.git.lucien.xin@gmail.com>
On Tue, Sep 5, 2017 at 8:47 PM, Xin Long <lucien.xin@gmail.com> wrote:
> ChunYu found a netlink use-after-free issue by syzkaller:
>
> [28448.842981] BUG: KASAN: use-after-free in __nla_put+0x37/0x40 at addr ffff8807185e2378
> [28448.969918] Call Trace:
> [...]
> [28449.117207] __nla_put+0x37/0x40
> [28449.132027] nla_put+0xf5/0x130
> [28449.146261] sk_diag_fill.isra.4.constprop.5+0x5a0/0x750 [netlink_diag]
> [28449.176608] __netlink_diag_dump+0x25a/0x700 [netlink_diag]
> [28449.202215] netlink_diag_dump+0x176/0x240 [netlink_diag]
> [28449.226834] netlink_dump+0x488/0xbb0
> [28449.298014] __netlink_dump_start+0x4e8/0x760
> [28449.317924] netlink_diag_handler_dump+0x261/0x340 [netlink_diag]
> [28449.413414] sock_diag_rcv_msg+0x207/0x390
> [28449.432409] netlink_rcv_skb+0x149/0x380
> [28449.467647] sock_diag_rcv+0x2d/0x40
> [28449.484362] netlink_unicast+0x562/0x7b0
> [28449.564790] netlink_sendmsg+0xaa8/0xe60
> [28449.661510] sock_sendmsg+0xcf/0x110
> [28449.865631] __sys_sendmsg+0xf3/0x240
> [28450.000964] SyS_sendmsg+0x32/0x50
> [28450.016969] do_syscall_64+0x25c/0x6c0
> [28450.154439] entry_SYSCALL64_slow_path+0x25/0x25
>
> It was caused by no protection between nlk groups' free in netlink_release
> and nlk groups' accessing in sk_diag_dump_groups. The similar issue also
> exists in netlink_seq_show().
>
> This patch is to defer nlk groups' free in deferred_put_nlk_sk.
This looks odd too, at least not complete.
The netlink sock itself is protected by RCU to speed up
the lookup path, but not necessarily nlk->groups, at least
I don't see rcu_dereference() in sk_diag_dump_groups().
And netlink_realloc_groups() needs fix too, right? Otherwise
krealloc() could reallocate a brand new memory and
existing readers will crash too?
I am afraid you need more work to make nlk->groups
RCU friendly. RCU is not just about call_rcu(), both
readers and writers need to use proper RCU API.
^ permalink raw reply
* Re: [PATCH RFC 5/5] Add KSZ8795 SPI driver
From: Pavel Machek @ 2017-09-08 21:55 UTC (permalink / raw)
To: Tristram.Ha
Cc: andrew, muvarov, nathan.leigh.conrad, vivien.didelot, f.fainelli,
netdev, linux-kernel, Woojung.Huh
In-Reply-To: <93AF473E2DA327428DE3D46B72B1E9FD41121E1C@CHN-SV-EXMX02.mchp-main.com>
[-- Attachment #1: Type: text/plain, Size: 931 bytes --]
Hi!
> > Please format according to CodingStyle. (Not only this.)
> >
> > And this will be common for more drivers. Can it go to a header file
> > and be included...?
> >
>
> Sorry about the formatting. It seems my e-mail system needs to be checked
> to make sure it does not auto-format the contents again.
>
> About the SPI access functions they are the same for each driver except the
> low level ksz_spi_read_reg and ksz_spi_write_reg. The dev_io_ops structure
> should contain only those 2 and ksz_spi_get and ksz_spi_set.
>
> But that requires changing ksz_spi.c. The idea was to keep the code of
> KSZ9477 driver with little change as possible while introducing another driver.
So we change ksz_spi.c. Goal is to have clean code.
Thanks,
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]
^ permalink raw reply
* Re: [PATCH RFC] Update documentation for KSZ DSA drivers so that new drivers can be added
From: Pavel Machek @ 2017-09-08 21:53 UTC (permalink / raw)
To: Tristram.Ha
Cc: andrew, muvarov, nathan.leigh.conrad, vivien.didelot, f.fainelli,
netdev, linux-kernel, Woojung.Huh
In-Reply-To: <93AF473E2DA327428DE3D46B72B1E9FD41121E68@CHN-SV-EXMX02.mchp-main.com>
[-- Attachment #1: Type: text/plain, Size: 1314 bytes --]
Hi!
> There will be 5 drivers to support these devices:
>
> ksz9477.c - KSZ9893/KSZ9897/KSZ9567/KSZ9566/KSZ9477
> ksz8795.c - KSZ8795/KSZ8795/KSZ8765
> ksz8895.c - KSZ8895/KSZ8864
Could we see the 8895 driver, please?
> Out of topic I have a question to ask the community regarding the DSA
> port creation:
>
> Port 1 can be specified using the reg parameter specifying 0; port 2, 1;
> and so on. The KSZ8794 is a variant of KSZ8795 with port 4 disabled.
> So
> lan1 = 0, lan2 = 1, lan3 = 2, cpu = 4.
> But our company Marketing does not want to promote that fact but treat
> KSZ8794 as a distinct product.
> So
> lan1 = 0, lan2 = 1, lan3 = 2, cpu = 3.
> Is this okay to hide this information inside the driver? This is manageable
> for KSZ8794 but for KSZ8864 the first port is disabled:
> lan1 = 1, lan2 = 2, lan3 = 3, cpu = 4.
>
> I am not sure whether DSA has or will have a way to display the port
> mapping to regular users.
Kernel is not a place to play marketing games, and people reading the
kernel sources are not targets of your marketing department.
Please let us just see the underlying hardware, as is.
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]
^ permalink raw reply
* [PATCH iproute2 3/3] bridge: request vlans along with link information
From: Roman Mashak @ 2017-09-08 21:52 UTC (permalink / raw)
To: stephen; +Cc: netdev, jhs, Roman Mashak
In-Reply-To: <1504907543-14394-1-git-send-email-mrv@mojatatu.com>
Signed-off-by: Roman Mashak <mrv@mojatatu.com>
---
bridge/link.c | 16 +++++++++++++---
1 file changed, 13 insertions(+), 3 deletions(-)
diff --git a/bridge/link.c b/bridge/link.c
index 60200f1..9e4206f 100644
--- a/bridge/link.c
+++ b/bridge/link.c
@@ -461,9 +461,19 @@ static int brlink_show(int argc, char **argv)
}
}
- if (rtnl_wilddump_request(&rth, PF_BRIDGE, RTM_GETLINK) < 0) {
- perror("Cannon send dump request");
- exit(1);
+ if (show_details) {
+ if (rtnl_wilddump_req_filter(&rth, PF_BRIDGE, RTM_GETLINK,
+ (compress_vlans ?
+ RTEXT_FILTER_BRVLAN_COMPRESSED :
+ RTEXT_FILTER_BRVLAN)) < 0) {
+ perror("Cannon send dump request");
+ exit(1);
+ }
+ } else {
+ if (rtnl_wilddump_request(&rth, PF_BRIDGE, RTM_GETLINK) < 0) {
+ perror("Cannon send dump request");
+ exit(1);
+ }
}
if (rtnl_dump_filter(&rth, print_linkinfo, stdout) < 0) {
--
1.9.1
^ permalink raw reply related
* [PATCH iproute2 2/3] bridge: dump vlan table information for link
From: Roman Mashak @ 2017-09-08 21:52 UTC (permalink / raw)
To: stephen; +Cc: netdev, jhs, Roman Mashak
In-Reply-To: <1504907543-14394-1-git-send-email-mrv@mojatatu.com>
Kernel also reports vlans a port is member of, so print it. Since vlan
table can be quite large, dump it only when detailed information is
requested.
Signed-off-by: Roman Mashak <mrv@mojatatu.com>
---
bridge/link.c | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/bridge/link.c b/bridge/link.c
index 93472ad..60200f1 100644
--- a/bridge/link.c
+++ b/bridge/link.c
@@ -213,6 +213,13 @@ int print_linkinfo(const struct sockaddr_nl *who,
if (aftb[IFLA_BRIDGE_MODE])
print_hwmode(fp, rta_getattr_u16(aftb[IFLA_BRIDGE_MODE]));
+ if (show_details) {
+ if (aftb[IFLA_BRIDGE_VLAN_INFO]) {
+ fprintf(fp, "\n");
+ print_vlan_info(fp, tb[IFLA_AF_SPEC],
+ ifi->ifi_index);
+ }
+ }
}
fprintf(fp, "\n");
--
1.9.1
^ permalink raw reply related
* [PATCH iproute2 1/3] bridge: isolate vlans parsing code in a separate API
From: Roman Mashak @ 2017-09-08 21:52 UTC (permalink / raw)
To: stephen; +Cc: netdev, jhs, Roman Mashak
In-Reply-To: <1504907543-14394-1-git-send-email-mrv@mojatatu.com>
IFLA_BRIDGE_VLAN_INFO parsing logic will be used in link and vlan
processing code, so it makes sense to move it in the separate function.
Signed-off-by: Roman Mashak <mrv@mojatatu.com>
---
bridge/br_common.h | 1 +
bridge/vlan.c | 145 +++++++++++++++++++++++++++--------------------------
2 files changed, 76 insertions(+), 70 deletions(-)
diff --git a/bridge/br_common.h b/bridge/br_common.h
index c649e7d..01447dd 100644
--- a/bridge/br_common.h
+++ b/bridge/br_common.h
@@ -4,6 +4,7 @@
#define MDB_RTR_RTA(r) \
((struct rtattr *)(((char *)(r)) + RTA_ALIGN(sizeof(__u32))))
+extern void print_vlan_info(FILE *fp, struct rtattr *tb, int ifindex);
extern int print_linkinfo(const struct sockaddr_nl *who,
struct nlmsghdr *n,
void *arg);
diff --git a/bridge/vlan.c b/bridge/vlan.c
index ebcdace..fc361ae 100644
--- a/bridge/vlan.c
+++ b/bridge/vlan.c
@@ -189,7 +189,6 @@ static int print_vlan(const struct sockaddr_nl *who,
struct ifinfomsg *ifm = NLMSG_DATA(n);
int len = n->nlmsg_len;
struct rtattr *tb[IFLA_MAX+1];
- bool vlan_flags = false;
if (n->nlmsg_type != RTM_NEWLINK) {
fprintf(stderr, "Not RTM_NEWLINK: %08x %08x %08x\n",
@@ -218,75 +217,7 @@ static int print_vlan(const struct sockaddr_nl *who,
ll_index_to_name(ifm->ifi_index));
return 0;
} else {
- struct rtattr *i, *list = tb[IFLA_AF_SPEC];
- int rem = RTA_PAYLOAD(list);
- __u16 last_vid_start = 0;
-
- if (!filter_vlan)
- print_vlan_port(fp, ifm->ifi_index);
-
- for (i = RTA_DATA(list); RTA_OK(i, rem); i = RTA_NEXT(i, rem)) {
- struct bridge_vlan_info *vinfo;
- int vcheck_ret;
-
- if (i->rta_type != IFLA_BRIDGE_VLAN_INFO)
- continue;
-
- vinfo = RTA_DATA(i);
-
- if (!(vinfo->flags & BRIDGE_VLAN_INFO_RANGE_END))
- last_vid_start = vinfo->vid;
- vcheck_ret = filter_vlan_check(vinfo);
- if (vcheck_ret == -1)
- break;
- else if (vcheck_ret == 0)
- continue;
-
- if (filter_vlan)
- print_vlan_port(fp, ifm->ifi_index);
- if (jw_global) {
- jsonw_start_object(jw_global);
- jsonw_uint_field(jw_global, "vlan",
- last_vid_start);
- if (vinfo->flags & BRIDGE_VLAN_INFO_RANGE_BEGIN)
- continue;
- } else {
- fprintf(fp, "\t %hu", last_vid_start);
- }
- if (last_vid_start != vinfo->vid) {
- if (jw_global)
- jsonw_uint_field(jw_global, "vlanEnd",
- vinfo->vid);
- else
- fprintf(fp, "-%hu", vinfo->vid);
- }
- if (vinfo->flags & BRIDGE_VLAN_INFO_PVID) {
- if (jw_global) {
- start_json_vlan_flags_array(&vlan_flags);
- jsonw_string(jw_global, "PVID");
- } else {
- fprintf(fp, " PVID");
- }
- }
- if (vinfo->flags & BRIDGE_VLAN_INFO_UNTAGGED) {
- if (jw_global) {
- start_json_vlan_flags_array(&vlan_flags);
- jsonw_string(jw_global,
- "Egress Untagged");
- } else {
- fprintf(fp, " Egress Untagged");
- }
- }
- if (jw_global && vlan_flags) {
- jsonw_end_array(jw_global);
- vlan_flags = false;
- }
-
- if (jw_global)
- jsonw_end_object(jw_global);
- else
- fprintf(fp, "\n");
- }
+ print_vlan_info(fp, tb[IFLA_AF_SPEC], ifm->ifi_index);
}
if (!filter_vlan) {
if (jw_global)
@@ -470,6 +401,80 @@ static int vlan_show(int argc, char **argv)
return 0;
}
+void print_vlan_info(FILE *fp, struct rtattr *tb, int ifindex)
+{
+ struct rtattr *i, *list = tb;
+ int rem = RTA_PAYLOAD(list);
+ __u16 last_vid_start = 0;
+ bool vlan_flags = false;
+
+ if (!filter_vlan)
+ print_vlan_port(fp, ifindex);
+
+ for (i = RTA_DATA(list); RTA_OK(i, rem); i = RTA_NEXT(i, rem)) {
+ struct bridge_vlan_info *vinfo;
+ int vcheck_ret;
+
+ if (i->rta_type != IFLA_BRIDGE_VLAN_INFO)
+ continue;
+
+ vinfo = RTA_DATA(i);
+
+ if (!(vinfo->flags & BRIDGE_VLAN_INFO_RANGE_END))
+ last_vid_start = vinfo->vid;
+ vcheck_ret = filter_vlan_check(vinfo);
+ if (vcheck_ret == -1)
+ break;
+ else if (vcheck_ret == 0)
+ continue;
+
+ if (filter_vlan)
+ print_vlan_port(fp, ifindex);
+ if (jw_global) {
+ jsonw_start_object(jw_global);
+ jsonw_uint_field(jw_global, "vlan",
+ last_vid_start);
+ if (vinfo->flags & BRIDGE_VLAN_INFO_RANGE_BEGIN)
+ continue;
+ } else {
+ fprintf(fp, "\t %hu", last_vid_start);
+ }
+ if (last_vid_start != vinfo->vid) {
+ if (jw_global)
+ jsonw_uint_field(jw_global, "vlanEnd",
+ vinfo->vid);
+ else
+ fprintf(fp, "-%hu", vinfo->vid);
+ }
+ if (vinfo->flags & BRIDGE_VLAN_INFO_PVID) {
+ if (jw_global) {
+ start_json_vlan_flags_array(&vlan_flags);
+ jsonw_string(jw_global, "PVID");
+ } else {
+ fprintf(fp, " PVID");
+ }
+ }
+ if (vinfo->flags & BRIDGE_VLAN_INFO_UNTAGGED) {
+ if (jw_global) {
+ start_json_vlan_flags_array(&vlan_flags);
+ jsonw_string(jw_global,
+ "Egress Untagged");
+ } else {
+ fprintf(fp, " Egress Untagged");
+ }
+ }
+ if (jw_global && vlan_flags) {
+ jsonw_end_array(jw_global);
+ vlan_flags = false;
+ }
+
+ if (jw_global)
+ jsonw_end_object(jw_global);
+ else
+ fprintf(fp, "\n");
+ }
+}
+
int do_vlan(int argc, char **argv)
{
ll_init_map(&rth);
--
1.9.1
^ permalink raw reply related
* [PATCH iproute2 0/3] Process IFLA_BRIDGE_VLAN_INFO tlv
From: Roman Mashak @ 2017-09-08 21:52 UTC (permalink / raw)
To: stephen; +Cc: netdev, jhs, Roman Mashak
Process IFLA_BRIDGE_VLAN_INFO attribute nested in IFLA_AF_SPEC, which
contains bridge vlan table per port passed in link events.
Roman Mashak (3):
bridge: isolate vlans parsing code in a separate API
bridge: dump vlan table information for link
bridge: request vlans along with link information
bridge/br_common.h | 1 +
bridge/link.c | 23 +++++++--
bridge/vlan.c | 145 +++++++++++++++++++++++++++--------------------------
3 files changed, 96 insertions(+), 73 deletions(-)
--
1.9.1
^ permalink raw reply
* Re: [PATCH RFC] Update documentation for KSZ DSA drivers so that new drivers can be added
From: Pavel Machek @ 2017-09-08 21:50 UTC (permalink / raw)
To: Andrew Lunn
Cc: Tristram.Ha, muvarov, nathan.leigh.conrad, vivien.didelot,
f.fainelli, netdev, linux-kernel, Woojung.Huh
In-Reply-To: <20170908190122.GM25219@lunn.ch>
[-- Attachment #1: Type: text/plain, Size: 1357 bytes --]
On Fri 2017-09-08 21:01:22, Andrew Lunn wrote:
> > > So i would suggest one driver supporting all the different devices.
> >
> > There will be 5 drivers to support these devices:
> >
> > ksz9477.c - KSZ9893/KSZ9897/KSZ9567/KSZ9566/KSZ9477
> > ksz8795.c - KSZ8795/KSZ8795/KSZ8765
> > ksz8895.c - KSZ8895/KSZ8864
> > ksz8863.c - KSZ8863/KSZ8873
> > ksz8463.c - KSZ8463
> >
> > These chips have different SPI access mechanisms, MIB counter reading,
> > and register set. These can be combined into one single driver using
> > function pointers, at least for ksz8795/ksz8895/ksz8863/ksz8463. My
> > only concern is the memory footprint. The customer may not want a
> > big driver to cover all the switches while only one is used.
>
> If memory footprint is your problem, make it a compile time choice
> which devices are supported within the one driver. In practice, you
> will find most distributions just enable them all.
I have to side with Tristram here. The register layouts are so
different that single driver does not make sense.
What could make sense is single function, compiled 5 times, based on
different includes; same source code but 5 different binaries.
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]
^ 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