* Re: [PATCH net-next 1/3] dt-bindings: net: add DT bindings for Socionext UniPhier AVE
From: Andrew Lunn @ 2017-09-08 14:03 UTC (permalink / raw)
To: Kunihiko Hayashi
Cc: netdev, David S. Miller, Florian Fainelli, Rob Herring,
Mark Rutland, linux-arm-kernel, linux-kernel, devicetree,
Masahiro Yamada, Masami Hiramatsu, Jassi Brar
In-Reply-To: <1504875731-3680-2-git-send-email-hayashi.kunihiko@socionext.com>
> + eth: ethernet@65000000 {
> + compatible = "socionext,uniphier-ave4";
> + reg = <0x65000000 0x8500>;
> + interrupts = <0 66 4>;
> + pinctrl-names = "default";
> + pinctrl-0 = <&pinctrl_ether_rgmii>;
> + phy-mode = "rgmii";
> + socionext,desc-bits = <64>;
> + local-mac-address = [00 00 00 00 00 00];
> +
> + #address-cells = <1>;
> + #size-cells = <0>;
> + ethphy: ethphy@1 {
> + reg = <1>;
> + };
So you normally have an mdio node, and the phy as a children of that
node.
mdio {
ethphy: ethernet-phy@6 {
reg = <6>;
};
};
Andrew
^ permalink raw reply
* Re: [PATCH v5 10/10] net: stmmac: dwmac-sun8i: Handle integrated/external MDIOs
From: Corentin Labbe @ 2017-09-08 14:08 UTC (permalink / raw)
To: Andrew Lunn
Cc: robh+dt-DgEjT+Ai2ygdnm+yROfE0A, mark.rutland-5wv7dgnIgG8,
maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8, wens-jdAy2FN1RRM,
linux-I+IVW8TIWO2tmTQ+vhA3Yw, catalin.marinas-5wv7dgnIgG8,
will.deacon-5wv7dgnIgG8, peppe.cavallaro-qxv4g6HH51o,
alexandre.torgue-qxv4g6HH51o, f.fainelli-Re5JQEeQqe8AvxtiuMwx3w,
netdev-u79uwXL29TY76Z2rM5mHXA, devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-kernel-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <20170908140020.GC25219-g2DYL2Zd6BY@public.gmane.org>
On Fri, Sep 08, 2017 at 04:00:20PM +0200, Andrew Lunn wrote:
> > > > +static int mdio_mux_syscon_switch_fn(int current_child, int desired_child,
> > > > + void *data)
> > > > +{
> > > > + struct stmmac_priv *priv = data;
> > > > + struct sunxi_priv_data *gmac = priv->plat->bsp_priv;
> > > > + u32 reg, val;
> > > > + int ret = 0;
> > > > + bool need_reset = false;
> > > > +
> > > > + if (current_child ^ desired_child) {
> > > > + regmap_read(gmac->regmap, SYSCON_EMAC_REG, ®);
> > > > + switch (desired_child) {
> > > > + case DWMAC_sUN8I_MDIO_MUX_INTERNAL_ID:
> > > > + dev_info(priv->device, "Switch mux to internal PHY");
> > > > + val = (reg & ~H3_EPHY_MUX_MASK) | H3_EPHY_SELECT;
> > > > + if (gmac->use_internal_phy)
> > > > + need_reset = true;
> > > > + break;
> > >
> > > This i don't get. Why do you need use_internal_phy? Isn't that
> > > implicit from DWMAC_sUN8I_MDIO_MUX_INTERNAL_ID? Is it even possible to
> > > use an external PHY on the internal MDIO bus?
> > >
> >
> > On my H3 box with external PHY, the MDIO mux library first select (for scan ?) the internal MDIO.
> > Without use_internal_phy usage, this board will launch a reset to use the internal MDIO... and this reset timeout/fail.
>
> Do you know why the reset times out/fails?
>
Because there are nothing connected to it.
I got also reset timeout on integrated MDIO when the integrated PHY is not powered.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH RFC] Update documentation for KSZ DSA drivers so that new drivers can be added
From: Andrew Lunn @ 2017-09-08 14:12 UTC (permalink / raw)
To: Maxim Uvarov
Cc: Tristram.Ha, Pavel Machek, Nathan Conrad, Vivien Didelot,
Florian Fainelli, netdev, linux-kernel, Woojung.Huh
In-Reply-To: <CAJGZr0+r3UhhRJcGXvBKZGamrSH_XEQY5oo7MTBjTTrDFOiKtg@mail.gmail.com>
On Fri, Sep 08, 2017 at 04:32:35PM +0300, Maxim Uvarov wrote:
> 2017-09-08 0:54 GMT+03:00 Andrew Lunn <andrew@lunn.ch>:
> >> -- compatible: For external switch chips, compatible string must be exactly one
> >> - of: "microchip,ksz9477"
> >> +- compatible: Should be "microchip,ksz9477" for KSZ9477 chip,
> >> + "microchip,ksz8795" for KSZ8795 chip,
> >> + "microchip,ksz8794" for KSZ8794 chip,
> >> + "microchip,ksz8765" for KSZ8765 chip,
> >> + "microchip,ksz8895" for KSZ8895 chip,
> >> + "microchip,ksz8864" for KSZ8864 chip,
> >> + "microchip,ksz8873" for KSZ8873 chip,
> >> + "microchip,ksz8863" for KSZ8863 chip,
> >> + "microchip,ksz8463" for KSZ8463 chip
> >
>
> all that chips have the same spi access to get chip id on probe(). I
> prefer common microship,ksz-spi rather than somebody will always
> maintain that list.
The Marvell DSA driver is similar. The compatibility string tells you
enough to go find the switch ID in the switch itself.
I suppose this comes down to, is there going to be one SPI driver for
all the devices, or lots of drivers? In general, DSA has one driver
for lots of devices. The mv88e6xxx supports around 25 devices. The b53
has around 17, etc.
So i would suggest one driver supporting all the different devices.
Andrew
^ permalink raw reply
* Re: [PATCH v5 10/10] net: stmmac: dwmac-sun8i: Handle integrated/external MDIOs
From: Andrew Lunn @ 2017-09-08 14:17 UTC (permalink / raw)
To: Corentin Labbe
Cc: robh+dt, mark.rutland, maxime.ripard, wens, linux,
catalin.marinas, will.deacon, peppe.cavallaro, alexandre.torgue,
f.fainelli, netdev, devicetree, linux-arm-kernel, linux-kernel
In-Reply-To: <20170908140832.GB3037@Red>
> > Do you know why the reset times out/fails?
> >
>
> Because there are nothing connected to it.
That should not be an issue. A read should just return 0xffff. And it
should return 0xffff fast. The timing of the MDIO protocol is fixed. A
read or a write takes a fixed number of cycles, independent of if
there is a device there or not. The bus data line has a pullup, so if
you try to access a missing device, you automatically read 0xffff.
Andrew
^ permalink raw reply
* Re: [PATCH] net: bonding: Fix transmit load balancing in balance-alb mode if specified by sysfs
From: Kosuke Tatsukawa @ 2017-09-08 14:17 UTC (permalink / raw)
To: Nikolay Aleksandrov
Cc: Jay Vosburgh, Veaceslav Falico, Andy Gospodarek,
netdev@vger.kernel.org, linux-kernel@vger.kernel.org,
Reinis Rozitis
In-Reply-To: <99818f9e-7ee0-4e53-b2be-b61b958f87e7@cumulusnetworks.com>
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.
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.
---
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))
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);
^ permalink raw reply related
* Re: Fwd: DA850-evm MAC Address is random
From: Adam Ford @ 2017-09-08 14:18 UTC (permalink / raw)
To: Sekhar Nori; +Cc: Tony Lindgren, Grygorii Strashko, linux-omap, netdev
In-Reply-To: <37fd2aaa-0802-9fde-61ee-43bdc287ff36@ti.com>
On Thu, Sep 7, 2017 at 3:36 AM, Sekhar Nori <nsekhar@ti.com> wrote:
> On Thursday 07 September 2017 03:11 AM, Adam Ford wrote:
>> On Mon, Sep 4, 2017 at 11:42 PM, Sekhar Nori <nsekhar@ti.com> wrote:
>>> Hi Adam,
>>>
>>> On Wednesday 30 August 2017 11:08 AM, Sekhar Nori wrote:
>>>>> I wonder if U-Boot isn't pushing something to Linux because it doesn't
>>>>> appear to be running some of the da850 specific code even when I run
>>>>> linux-next. Can you tell me what verision of U-Boot you're using?
>>>>> Other than using davinci_all_defconfig, did you change the
>>>>> configuration at all?
>>>
>>>> I am using U-Boot 2017.01. Yes, the kernel was built using
>>>> davinci_all_defconfig and no other config change. Before booting kernel,
>>>> can you confirm that ethaddr is set in U-Boot environment? This is what
>>>> fdt_fixup_ethernet() reads to fixup the FDT before boot.
>>>>
>>>> Here is my complete boot log with environment variable dump.
>>>>
>>>> http://pastebin.ubuntu.com/25430265/
>>>
>>> Were you able to get rid of the random mac address problem?
>>
>> Not yet. I haven't been able to rebuild Arago using TI's instructions
>> on the Wiki. I am not sure if it's a dependency issue or something
>> else. When I run Linux 4.13 using Buildroot as the rootfs, it does
>> not appear to run da850_evm_m25p80_notify_add(). I am going to
>> investigate whether or not da850_evm_init() is getting called. I was
>> wondering if you had some insight as to what calls that function? It
>> looks like it's defined as part of MACHINE_START(DAVINCI_DA850_EVM,
>> "DaVinci DA850/OMAP-L138/AM18x EVM"), but I don't know how it gets
>> called.
>
> These functions are called only when booting using the legacy board file
> method. From your logs before, you are booting using device tree. So
> these functions are irrelevant.
Ok. That makes a lot more sense now. I was really confused why the functions
were not getting called.
> Can you check if the mac address has been populated in the device-tree
> by dumping it from /proc/device-tree/.../local-mac-address? That will
> tell us if U-Boot is updating the mac address or not.
It does not appear to getting called.
# hexdump ./soc@1c00000/ethernet@220000/local-mac-address
0000000 0000 0000 0000
0000006
#
I'll work on something that pulls in the MAC address then inserts it
into the device tree like the recommendation that Tony made.
Thanks for all your help.
adam
>
> Thanks,
> Sekhar
^ permalink raw reply
* Re: [PATCH net-next v2 02/10] net: dsa: debugfs: add tree
From: Vivien Didelot @ 2017-09-08 14:18 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: netdev, linux-kernel, kernel, David S. Miller, Florian Fainelli,
Andrew Lunn, Egil Hjelmeland, John Crispin, Woojung Huh,
Sean Wang, Nikita Yushchenko, Chris Healy
In-Reply-To: <20170828191748.19492-3-vivien.didelot@savoirfairelinux.com>
Hi Greg,
Can I ask for a quick review of this patch as well? It's the one adding
the boilerplate for a single debugfs file, and I'm pretty sure it can be
reduced somehow.
Also more important, you will notice what seems to be a bug to me:
I can read or write a file even if I didn't mask the corresponding mode,
hence the double check in dsa_debugfs_show and dsa_debugfs_write.
Thanks,
Vivien
^ permalink raw reply
* Re: [PATCH v5 10/10] net: stmmac: dwmac-sun8i: Handle integrated/external MDIOs
From: Corentin Labbe @ 2017-09-08 14:28 UTC (permalink / raw)
To: Andrew Lunn
Cc: robh+dt, mark.rutland, maxime.ripard, wens, linux,
catalin.marinas, will.deacon, peppe.cavallaro, alexandre.torgue,
f.fainelli, netdev, devicetree, linux-arm-kernel, linux-kernel
In-Reply-To: <20170908141736.GF25219@lunn.ch>
On Fri, Sep 08, 2017 at 04:17:36PM +0200, Andrew Lunn wrote:
> > > Do you know why the reset times out/fails?
> > >
> >
> > Because there are nothing connected to it.
>
> That should not be an issue. A read should just return 0xffff. And it
> should return 0xffff fast. The timing of the MDIO protocol is fixed. A
> read or a write takes a fixed number of cycles, independent of if
> there is a device there or not. The bus data line has a pullup, so if
> you try to access a missing device, you automatically read 0xffff.
>
Perhaps, but the reality is that with nothing connected to it, the reset of the MAC timeout.
Certainly, the MAC does not support finding no PHY.
So, to prevent an error message, and a "freeze" of the net process, the need_reset trick is necessary.
Regards
Corentin Labbe
^ permalink raw reply
* Re: [PATCH] net: bonding: Fix transmit load balancing in balance-alb mode if specified by sysfs
From: Nikolay Aleksandrov @ 2017-09-08 14:30 UTC (permalink / raw)
To: Kosuke Tatsukawa
Cc: Jay Vosburgh, Veaceslav Falico, Andy Gospodarek,
netdev@vger.kernel.org, linux-kernel@vger.kernel.org,
Reinis Rozitis
In-Reply-To: <17EC94B0A072C34B8DCF0D30AD16044A0298684C@BPXM09GP.gisp.nec.co.jp>
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 ?
> 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);
>
^ permalink raw reply
* [PATCH net] sctp: fix missing wake ups in some situations
From: Marcelo Ricardo Leitner @ 2017-09-08 14:35 UTC (permalink / raw)
To: netdev; +Cc: linux-sctp, Neil Horman, Vlad Yasevich, Harald Welte
Commit fb586f25300f ("sctp: delay calls to sk_data_ready() as much as
possible") minimized the number of wake ups that are triggered in case
the association receives a packet with multiple data chunks on it and/or
when io_events are enabled and then commit 0970f5b36659 ("sctp: signal
sk_data_ready earlier on data chunks reception") moved the wake up to as
soon as possible. It thus relies on the state machine running later to
clean the flag that the event was already generated.
The issue is that there are 2 call paths that calls
sctp_ulpq_tail_event() outside of the state machine, causing the flag to
linger and possibly omitting a needed wake up in the sequence.
One of the call paths is when enabling SCTP_SENDER_DRY_EVENTS via
setsockopt(SCTP_EVENTS), as noticed by Harald Welte. The other is when
partial reliability triggers removal of chunks from the send queue when
the application calls sendmsg().
This commit fixes it by not setting the flag in case the socket is not
owned by the user, as it won't be cleaned later. This works for
user-initiated calls and also for rx path processing.
Fixes: fb586f25300f ("sctp: delay calls to sk_data_ready() as much as possible")
Reported-by: Harald Welte <laforge@gnumonks.org>
Signed-off-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
---
Hi. Please consider this one for -stable. Thanks
net/sctp/ulpqueue.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/net/sctp/ulpqueue.c b/net/sctp/ulpqueue.c
index 0225d62a869f8deff10565c4625df0a10464ce87..a71be33f3afeb0aaaef174ee082c4c547aab1e2d 100644
--- a/net/sctp/ulpqueue.c
+++ b/net/sctp/ulpqueue.c
@@ -265,7 +265,8 @@ int sctp_ulpq_tail_event(struct sctp_ulpq *ulpq, struct sctp_ulpevent *event)
sctp_ulpq_clear_pd(ulpq);
if (queue == &sk->sk_receive_queue && !sp->data_ready_signalled) {
- sp->data_ready_signalled = 1;
+ if (!sock_owned_by_user(sk))
+ sp->data_ready_signalled = 1;
sk->sk_data_ready(sk);
}
return 1;
--
2.13.5
^ permalink raw reply related
* IFA_F_OPTIMISTIC is not supported?
From: soohoon.lee @ 2017-09-08 14:29 UTC (permalink / raw)
To: netdev
rtm_newaddr masks off OPTIMISTIC.
inet6_rtm_newaddr(struct sk_buff *skb, struct nlmsghdr *nlh)
...
/* We ignore other flags so far. */
ifa_flags &= IFA_F_NODAD | IFA_F_HOMEADDRESS | IFA_F_MANAGETEMPADDR |
IFA_F_NOPREFIXROUTE;
Is there any problem or not allowed?
I need to do something like adding a route for the address after adding the address but fails because the address is not useable yet.
I tried NODAD but there's still little delay until IFA_F_TENTATIVE is cleared.
- rtm_newaddr sets IFA_F_TENTATIVE
- set a timer for dad start with delay=0
- some delay
- dad starts but ends immediately because of NODAD flag, and clears TENTATIVE.
And it looks like OPTIMISTIC does what I need but kind of disabled like the code above.
Thanks,
Soohoon.
^ permalink raw reply
* Re: [PATCH net-next v2 02/10] net: dsa: debugfs: add tree
From: Greg Kroah-Hartman @ 2017-09-08 14:40 UTC (permalink / raw)
To: Vivien Didelot
Cc: netdev, linux-kernel, kernel, David S. Miller, Florian Fainelli,
Andrew Lunn, Egil Hjelmeland, John Crispin, Woojung Huh,
Sean Wang, Nikita Yushchenko, Chris Healy
In-Reply-To: <87shfxxoi4.fsf@weeman.i-did-not-set--mail-host-address--so-tickle-me>
On Fri, Sep 08, 2017 at 10:18:27AM -0400, Vivien Didelot wrote:
> Hi Greg,
>
> Can I ask for a quick review of this patch as well? It's the one adding
> the boilerplate for a single debugfs file, and I'm pretty sure it can be
> reduced somehow.
I don't see a patch here :(
> Also more important, you will notice what seems to be a bug to me:
> I can read or write a file even if I didn't mask the corresponding mode,
> hence the double check in dsa_debugfs_show and dsa_debugfs_write.
The mode can be changed by userspace, you shouldn't ever need to check
it in any debugfs calls, right?
thanks,
greg k-h
^ permalink raw reply
* Re: [PATCH net-next 2/3] net: ethernet: socionext: add AVE ethernet driver
From: Masahiro Yamada @ 2017-09-08 14:44 UTC (permalink / raw)
To: Kunihiko Hayashi
Cc: netdev, David S. Miller, Andrew Lunn, Florian Fainelli,
Rob Herring, Mark Rutland, linux-arm-kernel,
Linux Kernel Mailing List, devicetree, Masami Hiramatsu,
Jassi Brar
In-Reply-To: <1504875731-3680-3-git-send-email-hayashi.kunihiko@socionext.com>
2017-09-08 22:02 GMT+09:00 Kunihiko Hayashi <hayashi.kunihiko@socionext.com>:
> diff --git a/drivers/net/ethernet/socionext/Kconfig b/drivers/net/ethernet/socionext/Kconfig
> new file mode 100644
> index 0000000..788f26f
> --- /dev/null
> +++ b/drivers/net/ethernet/socionext/Kconfig
> @@ -0,0 +1,22 @@
> +config NET_VENDOR_SOCIONEXT
> + bool "Socionext ethernet drivers"
> + default y
> + ---help---
> + Option to select ethernet drivers for Socionext platforms.
> +
> + Note that the answer to this question doesn't directly affect the
> + kernel: saying N will just cause the configurator to skip all
> + the questions about Agere devices. If you say Y, you will be asked
> + for your specific card in the following questions.
Agere?
> +
> + dev_info(dev, "Socionext %c%c%c%c Ethernet IP %s (irq=%d, phy=%s)\n",
> + (ave_id >> 24) & 0xff, (ave_id >> 16) & 0xff,
> + (ave_id >> 8) & 0xff, (ave_id >> 0) & 0xff,
> + buf, ndev->irq, phy_modes(phy_mode));
> +
> + return 0;
> +err_netdev_register:
Maybe, a bad label name.
for ex. out_del_napi or whatever.
Documentation/process/coding-style.rst says
"Choose label names which say what the goto does ..."
> + netif_napi_del(&priv->napi_rx);
> + netif_napi_del(&priv->napi_tx);
> + mdiobus_unregister(priv->mdio);
> +err_mdiobus_register:
> +err_mdiobus_alloc:
> +err_req_irq:
These three should be merged, for ex.
out_free_device.
I ran sparse for you.
Please take a look if it is worthwhile.
Seems endianess around mac_addr.
drivers/net/ethernet/socionext/sni_ave.c:423:15: warning: cast to
restricted __le32
drivers/net/ethernet/socionext/sni_ave.c:425:20: warning: cast to
restricted __le16
drivers/net/ethernet/socionext/sni_ave.c:1194:15: warning: cast to
restricted __le32
drivers/net/ethernet/socionext/sni_ave.c:1196:20: warning: cast to
restricted __le16
drivers/net/ethernet/socionext/sni_ave.c:1398:15: warning: cast to
restricted __le32
drivers/net/ethernet/socionext/sni_ave.c:1400:20: warning: cast to
restricted __le16
--
Best Regards
Masahiro Yamada
^ permalink raw reply
* Re: [PATCH iproute2 1/2] lib/libnetlink: re malloc buff if size is not enough
From: Phil Sutter @ 2017-09-08 14:51 UTC (permalink / raw)
To: Hangbin Liu; +Cc: netdev, Stephen Hemminger, Michal Kubecek
In-Reply-To: <20170908140131.GU5465@leo.usersys.redhat.com>
Hi,
On Fri, Sep 08, 2017 at 10:01:31PM +0800, Hangbin Liu wrote:
[...]
> > > diff --git a/lib/libnetlink.c b/lib/libnetlink.c
> > > index be7ac86..37cfb5a 100644
> > > --- a/lib/libnetlink.c
> > > +++ b/lib/libnetlink.c
> > > @@ -402,6 +402,59 @@ static void rtnl_dump_error(const struct rtnl_handle *rth,
> > > }
> > > }
> > >
> > > +static int rtnl_recvmsg(int fd, struct msghdr *msg, char **buf)
> > > +{
> > > + struct iovec *iov;
> > > + int len = -1, buf_len = 32768;
> > > + char *buffer = *buf;
> >
> > Isn't it possible to make 'buffer' static instead of the two 'buf'
> > variables in rtnl_dump_filter_l() and __rtnl_talk()? Then we would have
> > only a single buffer which is shared between both functions instead of
> > two which are independently allocated.
>
> I was also thinking of this before. But in function ipaddrlabel_flush()
>
> if (rtnl_dump_filter(&rth, flush_addrlabel, NULL) < 0)
>
> It will cal rtnl_dump_filter_l() first via
> rtnl_dump_filter() -> rtnl_dump_filter_nc() -> rtnl_dump_filter_l().
>
> Then call rtnl_talk() later via call back
> a->filter(&nladdr, h, a->arg1) -> flush_addrlabel() -> rtnl_talk()
>
> So if we only use one static buffer in rtnl_recvmsg(). Then it will be written
> at lease twice.
Oh yes, in that case we really can't have a single buffer.
[...]
> > > + buf_len = len;
> >
> > For this to work you have to make buf_len static too, otherwise you will
> > unnecessarily reallocate the buffer. Oh, and that also requires the
> > single buffer (as pointed out above) because you will otherwise use a
> > common buf_len for both static buffers passed to this function.
>
> Since we have to use two static bufffers. So how about check like
>
> if (len > strlen(buffer))
I don't think that will work. We are reusing the buffer and it contains
binary data, so a NUL byte may appear anywhere. I fear you will have to
change rtnl_recvmsg() to accept a buflen parameter which callers have to
define statically together with the buffer pointer.
Regarding Michal's concern about reentrancy, maybe we should go into a
different direction and make rtnl_recvmsg() return a newly allocated
buffer which the caller has to free.
[...]
> > When retrying inside rtnl_recvmsg(), it won't return 0 anymore. I
> > believe the whole 'while (1)' loop could go away then.
> >
>
> Like Michal said, there may have multi netlink packets?
Ah yes, I missed that.
Thanks, Phil
^ permalink raw reply
* Re: [PATCH net-next v2 02/10] net: dsa: debugfs: add tree
From: Vivien Didelot @ 2017-09-08 14:57 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: netdev, linux-kernel, kernel, David S. Miller, Florian Fainelli,
Andrew Lunn, Egil Hjelmeland, John Crispin, Woojung Huh,
Sean Wang, Nikita Yushchenko, Chris Healy
In-Reply-To: <20170828191748.19492-3-vivien.didelot@savoirfairelinux.com>
Hi Greg,
You wrote:
> > Can I ask for a quick review of this patch as well? It's the one adding
> > the boilerplate for a single debugfs file, and I'm pretty sure it can be
> > reduced somehow.
>
> I don't see a patch here :(
Oops, you weren't originally in Cc. Please find the patch below.
> > Also more important, you will notice what seems to be a bug to me:
> > I can read or write a file even if I didn't mask the corresponding mode
> > hence the double check in dsa_debugfs_show and dsa_debugfs_write.
>
> The mode can be changed by userspace, you shouldn't ever need to check
> it in any debugfs calls, right?
Correct. But this happens even if the file mode isn't changed by
userspace in the meantime, which seemed weird to me. e.g. echo
redirected to a -r--r--r-- debugfs entry will call dsa_debugfs_write.
Thanks,
Vivien
------ Beginning of the patch ------
This commit adds the boiler plate to create a DSA related debug
filesystem entry as well as a "tree" file, containing the tree index.
# cat switch1/tree
0
Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
---
net/dsa/debugfs.c | 107 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 107 insertions(+)
diff --git a/net/dsa/debugfs.c b/net/dsa/debugfs.c
index b6b5e5c97389..54e97e05a9d7 100644
--- a/net/dsa/debugfs.c
+++ b/net/dsa/debugfs.c
@@ -10,6 +10,7 @@
*/
#include <linux/debugfs.h>
+#include <linux/seq_file.h>
#include "dsa_priv.h"
@@ -19,6 +20,107 @@
/* DSA module debugfs directory */
static struct dentry *dsa_debugfs_dir;
+struct dsa_debugfs_ops {
+ int (*read)(struct dsa_switch *ds, int id, struct seq_file *seq);
+ int (*write)(struct dsa_switch *ds, int id, char *buf);
+};
+
+struct dsa_debugfs_priv {
+ const struct dsa_debugfs_ops *ops;
+ struct dsa_switch *ds;
+ int id;
+};
+
+static int dsa_debugfs_show(struct seq_file *seq, void *p)
+{
+ struct dsa_debugfs_priv *priv = seq->private;
+ struct dsa_switch *ds = priv->ds;
+
+ /* Somehow file mode is bypassed... Double check here */
+ if (!priv->ops->read)
+ return -EOPNOTSUPP;
+
+ return priv->ops->read(ds, priv->id, seq);
+}
+
+static ssize_t dsa_debugfs_write(struct file *file, const char __user *user_buf,
+ size_t count, loff_t *ppos)
+{
+ struct seq_file *seq = file->private_data;
+ struct dsa_debugfs_priv *priv = seq->private;
+ struct dsa_switch *ds = priv->ds;
+ char buf[count + 1];
+ int err;
+
+ /* Somehow file mode is bypassed... Double check here */
+ if (!priv->ops->write)
+ return -EOPNOTSUPP;
+
+ if (copy_from_user(buf, user_buf, count))
+ return -EFAULT;
+
+ buf[count] = '\0';
+
+ err = priv->ops->write(ds, priv->id, buf);
+
+ return err ? err : count;
+}
+
+static int dsa_debugfs_open(struct inode *inode, struct file *file)
+{
+ return single_open(file, dsa_debugfs_show, inode->i_private);
+}
+
+static const struct file_operations dsa_debugfs_fops = {
+ .open = dsa_debugfs_open,
+ .read = seq_read,
+ .write = dsa_debugfs_write,
+ .llseek = no_llseek,
+ .release = single_release,
+ .owner = THIS_MODULE,
+};
+
+static int dsa_debugfs_create_file(struct dsa_switch *ds, struct dentry *dir,
+ char *name, int id,
+ const struct dsa_debugfs_ops *ops)
+{
+ struct dsa_debugfs_priv *priv;
+ struct dentry *entry;
+ umode_t mode;
+
+ priv = devm_kzalloc(ds->dev, sizeof(*priv), GFP_KERNEL);
+ if (!priv)
+ return -ENOMEM;
+
+ priv->ops = ops;
+ priv->ds = ds;
+ priv->id = id;
+
+ mode = 0;
+ if (ops->read)
+ mode |= 0444;
+ if (ops->write)
+ mode |= 0200;
+
+ entry = debugfs_create_file(name, mode, dir, priv, &dsa_debugfs_fops);
+ if (IS_ERR_OR_NULL(entry))
+ return -EFAULT;
+
+ return 0;
+}
+
+static int dsa_debugfs_tree_read(struct dsa_switch *ds, int id,
+ struct seq_file *seq)
+{
+ seq_printf(seq, "%d\n", ds->dst->tree);
+
+ return 0;
+}
+
+static const struct dsa_debugfs_ops dsa_debugfs_tree_ops = {
+ .read = dsa_debugfs_tree_read,
+};
+
static int dsa_debugfs_create_port(struct dsa_switch *ds, int port)
{
struct dentry *dir;
@@ -48,6 +150,11 @@ static int dsa_debugfs_create_switch(struct dsa_switch *ds)
if (IS_ERR_OR_NULL(ds->debugfs_dir))
return -EFAULT;
+ err = dsa_debugfs_create_file(ds, ds->debugfs_dir, "tree", -1,
+ &dsa_debugfs_tree_ops);
+ if (err)
+ return err;
+
for (i = 0; i < ds->num_ports; i++) {
if (ds->enabled_port_mask & BIT(i)) {
err = dsa_debugfs_create_port(ds, i);
--
2.14.1
^ permalink raw reply related
* [PATCH] can: check for null sk before deferencing it via the call to sock_net
From: Colin King @ 2017-09-08 15:02 UTC (permalink / raw)
To: Oliver Hartkopp, Marc Kleine-Budde, David S . Miller, linux-can,
netdev
Cc: kernel-janitors, linux-kernel
From: Colin Ian King <colin.king@canonical.com>
The assignment of net via call sock_net will dereference sk. This
is performed before a sanity null check on sk, so there could be
a potential null dereference on the sock_net call if sk is null.
Fix this by assigning net after the sk null check. Also replace
the sk == NULL with the more usual !sk idiom.
Detected by CoverityScan CID#1431862 ("Dereference before null check")
Fixes: 384317ef4187 ("can: network namespace support for CAN_BCM protocol")
Signed-off-by: Colin Ian King <colin.king@canonical.com>
---
net/can/bcm.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/net/can/bcm.c b/net/can/bcm.c
index 47a8748d953a..a3791674b8ce 100644
--- a/net/can/bcm.c
+++ b/net/can/bcm.c
@@ -1493,13 +1493,14 @@ static int bcm_init(struct sock *sk)
static int bcm_release(struct socket *sock)
{
struct sock *sk = sock->sk;
- struct net *net = sock_net(sk);
+ struct net *net;
struct bcm_sock *bo;
struct bcm_op *op, *next;
- if (sk == NULL)
+ if (!sk)
return 0;
+ net = sock_net(sk);
bo = bcm_sk(sk);
/* remove bcm_ops, timer, rx_unregister(), etc. */
--
2.14.1
^ permalink raw reply related
* RE: [PATCH net-next v2 02/10] net: dsa: debugfs: add tree
From: David Laight @ 2017-09-08 15:03 UTC (permalink / raw)
To: 'Vivien Didelot', Greg Kroah-Hartman
Cc: netdev@vger.kernel.org, linux-kernel@vger.kernel.org,
kernel@savoirfairelinux.com, David S. Miller, Florian Fainelli,
Andrew Lunn, Egil Hjelmeland, John Crispin, Woojung Huh,
Sean Wang, Nikita Yushchenko, Chris Healy
In-Reply-To: <87k21944rq.fsf@weeman.i-did-not-set--mail-host-address--so-tickle-me>
From: Vivien Didelot
> Sent: 08 September 2017 15:57
...
> > > Also more important, you will notice what seems to be a bug to me:
> > > I can read or write a file even if I didn't mask the corresponding mode
> > > hence the double check in dsa_debugfs_show and dsa_debugfs_write.
> >
> > The mode can be changed by userspace, you shouldn't ever need to check
> > it in any debugfs calls, right?
>
> Correct. But this happens even if the file mode isn't changed by
> userspace in the meantime, which seemed weird to me. e.g. echo
> redirected to a -r--r--r-- debugfs entry will call dsa_debugfs_write.
root will be able to write using 'root' permissions, regardless of
the directory entry.
David
^ permalink raw reply
* Re: IFA_F_OPTIMISTIC is not supported?
From: Sabrina Dubroca @ 2017-09-08 15:07 UTC (permalink / raw)
To: soohoon.lee; +Cc: netdev
In-Reply-To: <59b2a92f.G2i6Z3JSoznmqvuW%soohoon.lee@f5.com>
2017-09-08, 10:29:03 -0400, soohoon.lee@f5.com wrote:
> rtm_newaddr masks off OPTIMISTIC.
>
> inet6_rtm_newaddr(struct sk_buff *skb, struct nlmsghdr *nlh)
> ...
> /* We ignore other flags so far. */
> ifa_flags &= IFA_F_NODAD | IFA_F_HOMEADDRESS | IFA_F_MANAGETEMPADDR |
> IFA_F_NOPREFIXROUTE;
>
> Is there any problem or not allowed?
I was planning to submit a patch to allow that soon (when net-next
reopens). While testing, I've found you need a few other small changes
once you allow userspace to set the optimistic flag on addresses.
Thanks,
--
Sabrina
^ permalink raw reply
* [PATCH] rsi: fix a dereference on adapter before it has been null checked
From: Colin King @ 2017-09-08 15:24 UTC (permalink / raw)
To: Kalle Valo, Amitkumar Karwar, Prameela Rani Garnepudi,
Karun Eagalapati, linux-wireless, netdev
Cc: kernel-janitors, linux-kernel
From: Colin Ian King <colin.king@canonical.com>
The assignment of dev is dereferencing adapter before adapter has
been null checked, potentially leading to a null pointer dereference.
Fix this by simply moving the assignment of dev to a later point
after the sanity null check of adapter.
Detected by CoverityScan CID#1398383 ("Dereference before null check")
Fixes: dad0d04fa7ba ("rsi: Add RS9113 wireless driver")
Signed-off-by: Colin Ian King <colin.king@canonical.com>
---
drivers/net/wireless/rsi/rsi_91x_usb.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/net/wireless/rsi/rsi_91x_usb.c b/drivers/net/wireless/rsi/rsi_91x_usb.c
index 81df09dd2636..08730227cd18 100644
--- a/drivers/net/wireless/rsi/rsi_91x_usb.c
+++ b/drivers/net/wireless/rsi/rsi_91x_usb.c
@@ -73,8 +73,7 @@ static int rsi_write_multiple(struct rsi_hw *adapter,
u8 *data,
u32 count)
{
- struct rsi_91x_usbdev *dev =
- (struct rsi_91x_usbdev *)adapter->rsi_dev;
+ struct rsi_91x_usbdev *dev;
if (!adapter)
return -ENODEV;
@@ -82,6 +81,7 @@ static int rsi_write_multiple(struct rsi_hw *adapter,
if (endpoint == 0)
return -EINVAL;
+ dev = (struct rsi_91x_usbdev *)adapter->rsi_dev;
if (dev->write_fail)
return -ENETDOWN;
--
2.14.1
^ permalink raw reply related
* Re: [PATCH net-next v2 02/10] net: dsa: debugfs: add tree
From: Greg Kroah-Hartman @ 2017-09-08 15:29 UTC (permalink / raw)
To: Vivien Didelot
Cc: netdev, linux-kernel, kernel, David S. Miller, Florian Fainelli,
Andrew Lunn, Egil Hjelmeland, John Crispin, Woojung Huh,
Sean Wang, Nikita Yushchenko, Chris Healy
In-Reply-To: <87k21944rq.fsf@weeman.i-did-not-set--mail-host-address--so-tickle-me>
On Fri, Sep 08, 2017 at 10:57:29AM -0400, Vivien Didelot wrote:
> Hi Greg,
>
> You wrote:
>
> > > Can I ask for a quick review of this patch as well? It's the one adding
> > > the boilerplate for a single debugfs file, and I'm pretty sure it can be
> > > reduced somehow.
> >
> > I don't see a patch here :(
>
> Oops, you weren't originally in Cc. Please find the patch below.
>
> > > Also more important, you will notice what seems to be a bug to me:
> > > I can read or write a file even if I didn't mask the corresponding mode
> > > hence the double check in dsa_debugfs_show and dsa_debugfs_write.
> >
> > The mode can be changed by userspace, you shouldn't ever need to check
> > it in any debugfs calls, right?
>
> Correct. But this happens even if the file mode isn't changed by
> userspace in the meantime, which seemed weird to me. e.g. echo
> redirected to a -r--r--r-- debugfs entry will call dsa_debugfs_write.
>
>
> Thanks,
>
> Vivien
>
>
> ------ Beginning of the patch ------
>
> This commit adds the boiler plate to create a DSA related debug
> filesystem entry as well as a "tree" file, containing the tree index.
>
> # cat switch1/tree
> 0
>
> Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
> Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
> Reviewed-by: Andrew Lunn <andrew@lunn.ch>
> ---
> net/dsa/debugfs.c | 107 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 107 insertions(+)
>
> diff --git a/net/dsa/debugfs.c b/net/dsa/debugfs.c
> index b6b5e5c97389..54e97e05a9d7 100644
> --- a/net/dsa/debugfs.c
> +++ b/net/dsa/debugfs.c
> @@ -10,6 +10,7 @@
> */
>
> #include <linux/debugfs.h>
> +#include <linux/seq_file.h>
>
> #include "dsa_priv.h"
>
> @@ -19,6 +20,107 @@
> /* DSA module debugfs directory */
> static struct dentry *dsa_debugfs_dir;
>
> +struct dsa_debugfs_ops {
> + int (*read)(struct dsa_switch *ds, int id, struct seq_file *seq);
> + int (*write)(struct dsa_switch *ds, int id, char *buf);
> +};
> +
> +struct dsa_debugfs_priv {
> + const struct dsa_debugfs_ops *ops;
> + struct dsa_switch *ds;
> + int id;
> +};
> +
> +static int dsa_debugfs_show(struct seq_file *seq, void *p)
> +{
> + struct dsa_debugfs_priv *priv = seq->private;
> + struct dsa_switch *ds = priv->ds;
> +
> + /* Somehow file mode is bypassed... Double check here */
As was said, root can do this, change your comment, just delete it :)
> + if (!priv->ops->read)
> + return -EOPNOTSUPP;
> +
> + return priv->ops->read(ds, priv->id, seq);
> +}
> +
> +static ssize_t dsa_debugfs_write(struct file *file, const char __user *user_buf,
> + size_t count, loff_t *ppos)
> +{
> + struct seq_file *seq = file->private_data;
> + struct dsa_debugfs_priv *priv = seq->private;
> + struct dsa_switch *ds = priv->ds;
> + char buf[count + 1];
Nice, userspace asks to write 100Gb, and boom, you just smashed the
stack!
Repeat after me:
All input is evil.
Say it again.
Always remember it.
> + int err;
> +
> + /* Somehow file mode is bypassed... Double check here */
> + if (!priv->ops->write)
> + return -EOPNOTSUPP;
> +
> + if (copy_from_user(buf, user_buf, count))
> + return -EFAULT;
> +
> + buf[count] = '\0';
Be careful here.
Use the kernel library functions instead of a "raw" copy_from/to_user()
calls, that is what they are there for (simple_read_to_buffer,
simple_write_to_buffer).
> +
> + err = priv->ops->write(ds, priv->id, buf);
> +
> + return err ? err : count;
> +}
> +
> +static int dsa_debugfs_open(struct inode *inode, struct file *file)
> +{
> + return single_open(file, dsa_debugfs_show, inode->i_private);
> +}
> +
> +static const struct file_operations dsa_debugfs_fops = {
> + .open = dsa_debugfs_open,
> + .read = seq_read,
> + .write = dsa_debugfs_write,
> + .llseek = no_llseek,
> + .release = single_release,
> + .owner = THIS_MODULE,
> +};
> +
> +static int dsa_debugfs_create_file(struct dsa_switch *ds, struct dentry *dir,
> + char *name, int id,
> + const struct dsa_debugfs_ops *ops)
> +{
> + struct dsa_debugfs_priv *priv;
> + struct dentry *entry;
> + umode_t mode;
> +
> + priv = devm_kzalloc(ds->dev, sizeof(*priv), GFP_KERNEL);
> + if (!priv)
> + return -ENOMEM;
> +
> + priv->ops = ops;
> + priv->ds = ds;
> + priv->id = id;
> +
> + mode = 0;
> + if (ops->read)
> + mode |= 0444;
> + if (ops->write)
> + mode |= 0200;
> +
> + entry = debugfs_create_file(name, mode, dir, priv, &dsa_debugfs_fops);
> + if (IS_ERR_OR_NULL(entry))
> + return -EFAULT;
Again, you don't care, don't check!
thanks,
greg k-h
^ permalink raw reply
* pull-request: wireless-drivers 2017-09-08
From: Kalle Valo @ 2017-09-08 15:50 UTC (permalink / raw)
To: David Miller; +Cc: linux-wireless, netdev, linux-kernel
Hi Dave,
few fixes to net tree for 4.14. Note that this pull request contains the
iwlwifi fix Linus hopes to have by end of the merge window. Please let
me know if there are any problems.
Kalle
The following changes since commit 8e0deed92406d93ae0365cb8a6134db5721e7aca:
tipc: remove unnecessary call to dev_net() (2017-09-06 21:25:52 -0700)
are available in the git repository at:
git://git.kernel.org/pub/scm/linux/kernel/git/kvalo/wireless-drivers.git tags/wireless-drivers-for-davem-2017-09-08
for you to fetch changes up to f957dd3c8db2781c8a334b166800788feb618625:
brcmfmac: feature check for multi-scheduled scan fails on bcm4345 devices (2017-09-08 12:25:24 +0300)
----------------------------------------------------------------
wireless-drivers fixes for 4.14
Few fixes to regressions introduced in the last one or two releases.
The iwlwifi fix is for a regression reported by Linus.
rtlwifi
* fix two antenna selection related bugs
iwlwifi
* fix regression with older firmwares
brcmfmac
* workaround firmware crash for bcm4345
----------------------------------------------------------------
Ian W MORRISON (1):
brcmfmac: feature check for multi-scheduled scan fails on bcm4345 devices
Larry Finger (2):
rtlwifi: btcoexist: Fix breakage of ant_sel for rtl8723be
rtlwifi: btcoexist: Fix antenna selection code
Luca Coelho (1):
iwlwifi: mvm: only send LEDS_CMD when the FW supports it
.../wireless/broadcom/brcm80211/brcmfmac/feature.c | 3 ++-
drivers/net/wireless/intel/iwlwifi/fw/file.h | 1 +
drivers/net/wireless/intel/iwlwifi/mvm/led.c | 3 ++-
.../realtek/rtlwifi/btcoexist/halbtc8723b2ant.c | 5 ++++-
.../realtek/rtlwifi/btcoexist/halbtcoutsrc.c | 23 +++++++++++++++-------
5 files changed, 25 insertions(+), 10 deletions(-)
^ permalink raw reply
* Re: [PATCH 3/3] net: skb_queue_purge(): lock/unlock the list only once
From: Eric Dumazet @ 2017-09-08 16:01 UTC (permalink / raw)
To: Michael Witten
Cc: David S. Miller, Alexey Kuznetsov, Hideaki YOSHIFUJI, netdev,
linux-kernel
In-Reply-To: <60c8906b751d4915be456009c220516e-mfwitten@gmail.com>
On Fri, 2017-09-08 at 05:06 +0000, Michael Witten wrote:
> Date: Thu, 7 Sep 2017 20:07:40 +0000
> With this commit, the list's lock is locked/unlocked only once
> for the duration of `skb_queue_purge()'.
>
> Hitherto, the list's lock has been locked/unlocked every time
> an item is dequeued; this seems not only inefficient, but also
> incorrect, as the whole point of `skb_queue_purge()' is to clear
> the list, presumably without giving anything else a chance to
> manipulate the list in the interim.
>
> Signed-off-by: Michael Witten <mfwitten@gmail.com>
> ---
> net/core/skbuff.c | 6 +++++-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> index 68065d7d383f..66c0731a2a5f 100644
> --- a/net/core/skbuff.c
> +++ b/net/core/skbuff.c
> @@ -2834,9 +2834,13 @@ EXPORT_SYMBOL(skb_dequeue_tail);
> */
> void skb_queue_purge(struct sk_buff_head *list)
> {
> + unsigned long flags;
> struct sk_buff *skb;
> - while ((skb = skb_dequeue(list)) != NULL)
> +
> + spin_lock_irqsave(&list->lock, flags);
> + while ((skb = __skb_dequeue(list)) != NULL)
> kfree_skb(skb);
> + spin_unlock_irqrestore(&list->lock, flags);
> }
> EXPORT_SYMBOL(skb_queue_purge);
>
No, this is very wrong :
Holding hard IRQ for a potential very long time is going to break
horribly. Some lists can have 10,000+ skbs in them.
Note that net-next tree is currently closed, please read
Documentation/networking/netdev-FAQ.txt
^ permalink raw reply
* Re: Use after free in __dst_destroy_metrics_generic
From: Cong Wang @ 2017-09-08 16:10 UTC (permalink / raw)
To: Subash Abhinov Kasiviswanathan
Cc: Linux Kernel Network Developers, Eric Dumazet, Lorenzo Colitti
In-Reply-To: <1234e09b75197d43ed84bdb1b154b4b6@codeaurora.org>
[-- Attachment #1: Type: text/plain, Size: 555 bytes --]
On Thu, Sep 7, 2017 at 5:52 PM, Subash Abhinov Kasiviswanathan
<subashab@codeaurora.org> wrote:
> We are seeing a possible use after free in ip6_dst_destroy.
>
> It appears as if memory of the __DST_METRICS_PTR(old) was freed in some path
> and allocated
> to ion driver. ion driver has also freed it. Finally the memory is freed by
> the
> fib gc and crashes since it is already deallocated.
Does the attach (compile-only) patch help anything?
>From my _quick_ glance, it seems we miss the refcnt'ing
right in __dst_destroy_metrics_generic().
Thanks!
[-- Attachment #2: dst-metrics-ref.diff --]
[-- Type: text/plain, Size: 661 bytes --]
diff --git a/net/core/dst.c b/net/core/dst.c
index 00aa972ad1a1..b293aeae3018 100644
--- a/net/core/dst.c
+++ b/net/core/dst.c
@@ -241,8 +241,14 @@ void __dst_destroy_metrics_generic(struct dst_entry *dst, unsigned long old)
new = ((unsigned long) &dst_default_metrics) | DST_METRICS_READ_ONLY;
prev = cmpxchg(&dst->_metrics, old, new);
- if (prev == old)
- kfree(__DST_METRICS_PTR(old));
+ if (prev == old) {
+ struct dst_metrics *old_p = (struct dst_metrics *)__DST_METRICS_PTR(old);
+
+ if (prev & DST_METRICS_REFCOUNTED) {
+ if (atomic_dec_and_test(&old_p->refcnt))
+ kfree(old_p);
+ }
+ }
}
EXPORT_SYMBOL(__dst_destroy_metrics_generic);
^ permalink raw reply related
* Re: Use after free in __dst_destroy_metrics_generic
From: Cong Wang @ 2017-09-08 16:12 UTC (permalink / raw)
To: Stefano Brivio
Cc: Subash Abhinov Kasiviswanathan, Linux Kernel Network Developers,
Eric Dumazet, Lorenzo Colitti
In-Reply-To: <20170908025620.47d0ad2e@elisabeth>
On Thu, Sep 7, 2017 at 5:56 PM, Stefano Brivio <sbrivio@redhat.com> wrote:
> On Thu, 07 Sep 2017 18:52:02 -0600
> Subash Abhinov Kasiviswanathan <subashab@codeaurora.org> wrote:
>
>> We are seeing a possible use after free in ip6_dst_destroy.
>>
>> It appears as if memory of the __DST_METRICS_PTR(old) was freed in some
>> path and allocated
>> to ion driver. ion driver has also freed it. Finally the memory is freed
>> by the
>> fib gc and crashes since it is already deallocated.
>>
>> Target is running an ARM64 Android based 4.9 kernel.
>> Issue was seen once on a regression rack (sorry, no reproducer).
>> Any pointers to debug this is highly appreciated.
>>
>> [ 3489.470581] [<ffffff83c0a289c0>] object_err+0x4c/0x5c
>> [ 3489.470586] [<ffffff83c0a2b284>] free_debug_processing+0x2e0/0x398
>> [ 3489.470589] [<ffffff83c0a2b63c>] __slab_free+0x300/0x3e0
>> [ 3489.470593] [<ffffff83c0a2bfc8>] kfree+0x28c/0x290
>> [ 3489.470601] [<ffffff83c16b9580>]
>> __dst_destroy_metrics_generic+0x6c/0x78
>> [ 3489.470609] [<ffffff83c17d3408>] ip6_dst_destroy+0xb0/0xb4
>
> Should be fixed by:
>
> commit ad65a2f05695aced349e308193c6e2a6b1d87112
> Author: Wei Wang <weiwan@google.com>
> Date: Sat Jun 17 10:42:35 2017 -0700
>
> ipv6: call dst_hold_safe() properly
Obviously it should not. One is dst metric, the other is dst.
^ permalink raw reply
* Re: Use after free in __dst_destroy_metrics_generic
From: Stefano Brivio @ 2017-09-08 16:17 UTC (permalink / raw)
To: Cong Wang
Cc: Subash Abhinov Kasiviswanathan, Linux Kernel Network Developers,
Eric Dumazet, Lorenzo Colitti
In-Reply-To: <CAM_iQpV--V9ROSd61CWVbcpCHonMUubLeyEKVSvEUTTW3gh5hA@mail.gmail.com>
On Fri, 8 Sep 2017 09:12:09 -0700
Cong Wang <xiyou.wangcong@gmail.com> wrote:
> On Thu, Sep 7, 2017 at 5:56 PM, Stefano Brivio <sbrivio@redhat.com> wrote:
> > On Thu, 07 Sep 2017 18:52:02 -0600
> > Subash Abhinov Kasiviswanathan <subashab@codeaurora.org> wrote:
> >
> >> We are seeing a possible use after free in ip6_dst_destroy.
> >>
> >> It appears as if memory of the __DST_METRICS_PTR(old) was freed in some
> >> path and allocated
> >> to ion driver. ion driver has also freed it. Finally the memory is freed
> >> by the
> >> fib gc and crashes since it is already deallocated.
> >>
> >> Target is running an ARM64 Android based 4.9 kernel.
> >> Issue was seen once on a regression rack (sorry, no reproducer).
> >> Any pointers to debug this is highly appreciated.
> >>
> >> [ 3489.470581] [<ffffff83c0a289c0>] object_err+0x4c/0x5c
> >> [ 3489.470586] [<ffffff83c0a2b284>] free_debug_processing+0x2e0/0x398
> >> [ 3489.470589] [<ffffff83c0a2b63c>] __slab_free+0x300/0x3e0
> >> [ 3489.470593] [<ffffff83c0a2bfc8>] kfree+0x28c/0x290
> >> [ 3489.470601] [<ffffff83c16b9580>]
> >> __dst_destroy_metrics_generic+0x6c/0x78
> >> [ 3489.470609] [<ffffff83c17d3408>] ip6_dst_destroy+0xb0/0xb4
> >
> > Should be fixed by:
> >
> > commit ad65a2f05695aced349e308193c6e2a6b1d87112
> > Author: Wei Wang <weiwan@google.com>
> > Date: Sat Jun 17 10:42:35 2017 -0700
> >
> > ipv6: call dst_hold_safe() properly
>
> Obviously it should not. One is dst metric, the other is dst.
And obviously you're right. Sorry for the confusion, I blatantly
misread the backtrace.
^ 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