* Re: [PATCH 4/8] tty/bcm63xx_uart: allow naming clock in device tree
From: Jonas Gorski @ 2017-09-06 11:01 UTC (permalink / raw)
To: MIPS Mailing List,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org,
linux-serial-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
Network Development
Cc: Greg Kroah-Hartman, Rob Herring, Mark Rutland, Ralf Baechle,
Florian Fainelli, bcm-kernel-feedback-list, Kevin Cernekee,
Jiri Slaby, David S. Miller, Russell King
In-Reply-To: <20170802093429.12572-5-jonas.gorski-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Hi Greg,
On 2 August 2017 at 11:34, Jonas Gorski <jonas.gorski-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> Codify using a named clock for the refclk of the uart. This makes it
> easier if we might need to add a gating clock (like present on the
> BCM6345).
>
> Signed-off-by: Jonas Gorski <jonas.gorski-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Could I please get a (N)Ack so Ralf can add this patch to his tree?
Regards
Jonas
> ---
> Documentation/devicetree/bindings/serial/brcm,bcm6345-uart.txt | 6 ++++++
> drivers/tty/serial/bcm63xx_uart.c | 6 ++++--
> 2 files changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/serial/brcm,bcm6345-uart.txt b/Documentation/devicetree/bindings/serial/brcm,bcm6345-uart.txt
> index 5c52e5eef16d..8b2b0460259a 100644
> --- a/Documentation/devicetree/bindings/serial/brcm,bcm6345-uart.txt
> +++ b/Documentation/devicetree/bindings/serial/brcm,bcm6345-uart.txt
> @@ -11,6 +11,11 @@ Required properties:
> - clocks: Clock driving the hardware; used to figure out the baud rate
> divisor.
>
> +
> +Optional properties:
> +
> +- clock-names: Should be "refclk".
> +
> Example:
>
> uart0: serial@14e00520 {
> @@ -19,6 +24,7 @@ Example:
> interrupt-parent = <&periph_intc>;
> interrupts = <2>;
> clocks = <&periph_clk>;
> + clock-names = "refclk";
> };
>
> clocks {
> diff --git a/drivers/tty/serial/bcm63xx_uart.c b/drivers/tty/serial/bcm63xx_uart.c
> index a2b9376ec861..f227eff28d3a 100644
> --- a/drivers/tty/serial/bcm63xx_uart.c
> +++ b/drivers/tty/serial/bcm63xx_uart.c
> @@ -841,8 +841,10 @@ static int bcm_uart_probe(struct platform_device *pdev)
> if (!res_irq)
> return -ENODEV;
>
> - clk = pdev->dev.of_node ? of_clk_get(pdev->dev.of_node, 0) :
> - clk_get(&pdev->dev, "refclk");
> + clk = clk_get(&pdev->dev, "refclk");
> + if (IS_ERR(clk) && pdev->dev.of_node)
> + clk = of_clk_get(pdev->dev.of_node, 0);
> +
> if (IS_ERR(clk))
> return -ENODEV;
>
> --
> 2.13.2
>
--
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 net-next v6 3/3] openvswitch: enable NSH support
From: Yang, Yi @ 2017-09-06 11:03 UTC (permalink / raw)
To: Hannes Frederic Sowa
Cc: dev-yBygre7rU0TnMu66kgdUjQ@public.gmane.org,
netdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
jbenc-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org, e@erig.me
In-Reply-To: <87o9qo9ru6.fsf-tFNcAqjVMyqKXQKiL6tip0B+6BGkLq7r@public.gmane.org>
On Wed, Sep 06, 2017 at 04:03:29PM +0800, Hannes Frederic Sowa wrote:
> "Yang, Yi" <yi.y.yang-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org> writes:
> >>
> >> > If you check GENEVE implementation, tun_metadata* can be set or matched
> >> > as any other match field.
> >>
> >> Yes, I wrote that in my previous mail. I wonder why NSH context metadata
> >> is not in tun_metadata as well?
> >
> > tun_metadata is tunnel metadata, GENEVE needs tunnel port, but NSH is
> > not so, NSH can't directly use tun_metadata, for MD type 2, we need to a
> > lot of rework on tun_metadata to make it shared between GENEVE and NSH,
> > I don't think this can happen in near term. So tun_metadata isn't option
> > for this now.
>
> Sorry, I couldn't follow you. Why can't you store the context headers in
> tun_metadata exactly?
tun_metadata can work only if in_port and out_port are GENEVE tunnel
port, it is designed specially for GENEVE, Eth+NSH can work without
any tunnel port involved, context headers for NSH are not tunnel
metadata, they aren't tunnel-specific, you can't treat them as same
thing. In addtition, tun_metadata also can be matched and set, they have
the same issue as you concern.
>
> [...]
>
> Bye,
> Hannes
^ permalink raw reply
* [patch net] net: sched: fix memleak for chain zero
From: Jiri Pirko @ 2017-09-06 11:14 UTC (permalink / raw)
To: netdev; +Cc: davem, jhs, xiyou.wangcong, kubakici, mlxsw
From: Jiri Pirko <jiri@mellanox.com>
There's a memleak happening for chain 0. The thing is, chain 0 needs to
be always present, not created on demand. Therefore tcf_block_get upon
creation of block calls the tcf_chain_create function directly. The
chain is created with refcnt == 1, which is not correct in this case and
causes the memleak. So move the refcnt increment into tcf_chain_get
function even for the case when chain needs to be created.
Reported-by: Jakub Kicinski <kubakici@wp.pl>
Fixes: 5bc1701881e3 ("net: sched: introduce multichain support for filters")
Signed-off-by: Jiri Pirko <jiri@mellanox.com>
---
net/sched/cls_api.c | 18 +++++++++---------
1 file changed, 9 insertions(+), 9 deletions(-)
diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
index 6c5ea84..30ef466 100644
--- a/net/sched/cls_api.c
+++ b/net/sched/cls_api.c
@@ -197,7 +197,7 @@ static struct tcf_chain *tcf_chain_create(struct tcf_block *block,
list_add_tail(&chain->list, &block->chain_list);
chain->block = block;
chain->index = chain_index;
- chain->refcnt = 1;
+ chain->refcnt = 0;
return chain;
}
@@ -232,15 +232,15 @@ struct tcf_chain *tcf_chain_get(struct tcf_block *block, u32 chain_index,
struct tcf_chain *chain;
list_for_each_entry(chain, &block->chain_list, list) {
- if (chain->index == chain_index) {
- chain->refcnt++;
- return chain;
- }
+ if (chain->index == chain_index)
+ goto incref;
}
- if (create)
- return tcf_chain_create(block, chain_index);
- else
- return NULL;
+ chain = create ? tcf_chain_create(block, chain_index) : NULL;
+
+incref:
+ if (chain)
+ chain->refcnt++;
+ return chain;
}
EXPORT_SYMBOL(tcf_chain_get);
--
2.9.3
^ permalink raw reply related
* hung task in mac80211
From: Matteo Croce @ 2017-09-06 11:57 UTC (permalink / raw)
To: linux-wireless, netdev, linux-kernel
Hi,
I have an hung task on vanilla 4.13 kernel which I haven't on 4.12.
The problem is present both on my AP and on my notebook,
so it seems it affects AP and STA mode as well.
The generated messages are:
INFO: task kworker/u16:6:120 blocked for more than 120 seconds.
Not tainted 4.13.0 #57
"echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message.
kworker/u16:6 D 0 120 2 0x00000000
Workqueue: phy0 ieee80211_ba_session_work [mac80211]
Call Trace:
? __schedule+0x174/0x5b0
? schedule+0x31/0x80
? schedule_preempt_disabled+0x9/0x10
? __mutex_lock.isra.2+0x163/0x480
? select_task_rq_fair+0xb9f/0xc60
? __ieee80211_start_rx_ba_session+0x135/0x4d0 [mac80211]
? __ieee80211_start_rx_ba_session+0x135/0x4d0 [mac80211]
? try_to_wake_up+0x1f1/0x340
? update_curr+0x88/0xd0
? ieee80211_ba_session_work+0x148/0x230 [mac80211]
? process_one_work+0x1a5/0x330
? worker_thread+0x42/0x3c0
? create_worker+0x170/0x170
? kthread+0x10d/0x130
? kthread_create_on_node+0x40/0x40
? ret_from_fork+0x22/0x30
I did a bisect and the offending commit is:
commit 699cb58c8a52ff39bf659bff7971893ebe111bf2
Author: Johannes Berg <johannes.berg@intel.com>
Date: Tue May 30 16:34:46 2017 +0200
mac80211: manage RX BA session offload without SKB queue
Instead of using the SKB queue with the fake pkt_type for the
offloaded RX BA session management, also handle this with the
normal aggregation state machine worker. This also makes the
use of this more reliable since it gets rid of the allocation
of the fake skb.
Combined with the previous patch, this finally allows us to
get rid of the pkt_type hack entirely, so do that as well.
Signed-off-by: Johannes Berg <johannes.berg@intel.com>
Regards,
--
Matteo Croce
per aspera ad upstream
^ permalink raw reply
* Re: [PATCH 4/8] tty/bcm63xx_uart: allow naming clock in device tree
From: Greg Kroah-Hartman @ 2017-09-06 12:17 UTC (permalink / raw)
To: Jonas Gorski
Cc: MIPS Mailing List, linux-arm-kernel@lists.infradead.org,
linux-serial@vger.kernel.org, devicetree@vger.kernel.org,
Network Development, Rob Herring, Mark Rutland, Ralf Baechle,
Florian Fainelli, bcm-kernel-feedback-list, Kevin Cernekee,
Jiri Slaby, David S. Miller, Russell King
In-Reply-To: <CAOiHx=mC=GfX0VvuRWR-AmXYfVOEkuruwGHooS08WrL_z-60UA@mail.gmail.com>
On Wed, Sep 06, 2017 at 01:01:32PM +0200, Jonas Gorski wrote:
> Hi Greg,
>
> On 2 August 2017 at 11:34, Jonas Gorski <jonas.gorski@gmail.com> wrote:
> > Codify using a named clock for the refclk of the uart. This makes it
> > easier if we might need to add a gating clock (like present on the
> > BCM6345).
> >
> > Signed-off-by: Jonas Gorski <jonas.gorski@gmail.com>
>
> Could I please get a (N)Ack so Ralf can add this patch to his tree?
>
>
> Regards
> Jonas
>
>
> > ---
> > Documentation/devicetree/bindings/serial/brcm,bcm6345-uart.txt | 6 ++++++
> > drivers/tty/serial/bcm63xx_uart.c | 6 ++++--
> > 2 files changed, 10 insertions(+), 2 deletions(-)
> >
> > diff --git a/Documentation/devicetree/bindings/serial/brcm,bcm6345-uart.txt b/Documentation/devicetree/bindings/serial/brcm,bcm6345-uart.txt
> > index 5c52e5eef16d..8b2b0460259a 100644
> > --- a/Documentation/devicetree/bindings/serial/brcm,bcm6345-uart.txt
> > +++ b/Documentation/devicetree/bindings/serial/brcm,bcm6345-uart.txt
> > @@ -11,6 +11,11 @@ Required properties:
> > - clocks: Clock driving the hardware; used to figure out the baud rate
> > divisor.
> >
> > +
> > +Optional properties:
> > +
> > +- clock-names: Should be "refclk".
> > +
> > Example:
> >
> > uart0: serial@14e00520 {
> > @@ -19,6 +24,7 @@ Example:
> > interrupt-parent = <&periph_intc>;
> > interrupts = <2>;
> > clocks = <&periph_clk>;
> > + clock-names = "refclk";
> > };
> >
> > clocks {
I don't ack devtree changes :)
> > diff --git a/drivers/tty/serial/bcm63xx_uart.c b/drivers/tty/serial/bcm63xx_uart.c
> > index a2b9376ec861..f227eff28d3a 100644
> > --- a/drivers/tty/serial/bcm63xx_uart.c
> > +++ b/drivers/tty/serial/bcm63xx_uart.c
> > @@ -841,8 +841,10 @@ static int bcm_uart_probe(struct platform_device *pdev)
> > if (!res_irq)
> > return -ENODEV;
> >
> > - clk = pdev->dev.of_node ? of_clk_get(pdev->dev.of_node, 0) :
> > - clk_get(&pdev->dev, "refclk");
> > + clk = clk_get(&pdev->dev, "refclk");
> > + if (IS_ERR(clk) && pdev->dev.of_node)
> > + clk = of_clk_get(pdev->dev.of_node, 0);
> > +
> > if (IS_ERR(clk))
> > return -ENODEV;
> >
This part is fine with me:
Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
^ permalink raw reply
* Re: hung task in mac80211
From: Christian Lamparter @ 2017-09-06 12:28 UTC (permalink / raw)
To: Matteo Croce
Cc: linux-wireless-u79uwXL29TY76Z2rM5mHXA,
netdev-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
johannes-cdvu00un1VgdHxzADdlk8Q
In-Reply-To: <CAGnkfhwi9MmtLneeU23iWEGLj9cb1ODb3KzzOqTDvA6nNVsugg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
On Wednesday, September 6, 2017 1:57:47 PM CEST Matteo Croce wrote:
> Hi,
>
> I have an hung task on vanilla 4.13 kernel which I haven't on 4.12.
> The problem is present both on my AP and on my notebook,
> so it seems it affects AP and STA mode as well.
> The generated messages are:
>
> INFO: task kworker/u16:6:120 blocked for more than 120 seconds.
> Not tainted 4.13.0 #57
> "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message.
> kworker/u16:6 D 0 120 2 0x00000000
> Workqueue: phy0 ieee80211_ba_session_work [mac80211]
> Call Trace:
> ? __schedule+0x174/0x5b0
> ? schedule+0x31/0x80
> ? schedule_preempt_disabled+0x9/0x10
> ? __mutex_lock.isra.2+0x163/0x480
> ? select_task_rq_fair+0xb9f/0xc60
> ? __ieee80211_start_rx_ba_session+0x135/0x4d0 [mac80211]
> ? __ieee80211_start_rx_ba_session+0x135/0x4d0 [mac80211]
> ? try_to_wake_up+0x1f1/0x340
> ? update_curr+0x88/0xd0
> ? ieee80211_ba_session_work+0x148/0x230 [mac80211]
>
> I did a bisect and the offending commit is:
>
> commit 699cb58c8a52ff39bf659bff7971893ebe111bf2
> Author: Johannes Berg <johannes.berg-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> Date: Tue May 30 16:34:46 2017 +0200
>
> mac80211: manage RX BA session offload without SKB queue
I looked at this briefly:
ieee80211_ba_session_work acquires:
mutex_lock(&sta->ampdu_mlme.mtx) @
<http://elixir.free-electrons.com/linux/v4.13/source/net/mac80211/ht.c#L321>
But it now also calls
__ieee80211_start_rx_ba_session() @
http://elixir.free-electrons.com/linux/v4.13/source/net/mac80211/ht.c#L336
which also wants to hold mutex_lock(&sta->ampdu_mlme.mtx) in:
http://elixir.free-electrons.com/linux/v4.13/source/net/mac80211/agg-rx.c#L314
I guess this is where it deadlocks?
Regards,
Christian
^ permalink raw reply
* Re: [PATCH 4/8] tty/bcm63xx_uart: allow naming clock in device tree
From: Jonas Gorski @ 2017-09-06 12:37 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: MIPS Mailing List,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org,
linux-serial-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
Network Development, Rob Herring, Mark Rutland, Ralf Baechle,
Florian Fainelli, bcm-kernel-feedback-list, Kevin Cernekee,
Jiri Slaby, David S. Miller, Russell King
In-Reply-To: <20170906121715.GA27869-U8xfFu+wG4EAvxtiuMwx3w@public.gmane.org>
On 6 September 2017 at 14:17, Greg Kroah-Hartman
<gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r@public.gmane.org> wrote:
> On Wed, Sep 06, 2017 at 01:01:32PM +0200, Jonas Gorski wrote:
>> Hi Greg,
>>
>> On 2 August 2017 at 11:34, Jonas Gorski <jonas.gorski-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>> > Codify using a named clock for the refclk of the uart. This makes it
>> > easier if we might need to add a gating clock (like present on the
>> > BCM6345).
>> >
>> > Signed-off-by: Jonas Gorski <jonas.gorski-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>>
>> Could I please get a (N)Ack so Ralf can add this patch to his tree?
>>
>>
>> Regards
>> Jonas
>>
>>
>> > ---
>> > Documentation/devicetree/bindings/serial/brcm,bcm6345-uart.txt | 6 ++++++
>> > drivers/tty/serial/bcm63xx_uart.c | 6 ++++--
>> > 2 files changed, 10 insertions(+), 2 deletions(-)
>> >
>> > diff --git a/Documentation/devicetree/bindings/serial/brcm,bcm6345-uart.txt b/Documentation/devicetree/bindings/serial/brcm,bcm6345-uart.txt
>> > index 5c52e5eef16d..8b2b0460259a 100644
>> > --- a/Documentation/devicetree/bindings/serial/brcm,bcm6345-uart.txt
>> > +++ b/Documentation/devicetree/bindings/serial/brcm,bcm6345-uart.txt
>> > @@ -11,6 +11,11 @@ Required properties:
>> > - clocks: Clock driving the hardware; used to figure out the baud rate
>> > divisor.
>> >
>> > +
>> > +Optional properties:
>> > +
>> > +- clock-names: Should be "refclk".
>> > +
>> > Example:
>> >
>> > uart0: serial@14e00520 {
>> > @@ -19,6 +24,7 @@ Example:
>> > interrupt-parent = <&periph_intc>;
>> > interrupts = <2>;
>> > clocks = <&periph_clk>;
>> > + clock-names = "refclk";
>> > };
>> >
>> > clocks {
>
> I don't ack devtree changes :)
>
>> > diff --git a/drivers/tty/serial/bcm63xx_uart.c b/drivers/tty/serial/bcm63xx_uart.c
>> > index a2b9376ec861..f227eff28d3a 100644
>> > --- a/drivers/tty/serial/bcm63xx_uart.c
>> > +++ b/drivers/tty/serial/bcm63xx_uart.c
>> > @@ -841,8 +841,10 @@ static int bcm_uart_probe(struct platform_device *pdev)
>> > if (!res_irq)
>> > return -ENODEV;
>> >
>> > - clk = pdev->dev.of_node ? of_clk_get(pdev->dev.of_node, 0) :
>> > - clk_get(&pdev->dev, "refclk");
>> > + clk = clk_get(&pdev->dev, "refclk");
>> > + if (IS_ERR(clk) && pdev->dev.of_node)
>> > + clk = of_clk_get(pdev->dev.of_node, 0);
>> > +
>> > if (IS_ERR(clk))
>> > return -ENODEV;
>> >
>
> This part is fine with me:
That's all I wanted :)
>
> Acked-by: Greg Kroah-Hartman <gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r@public.gmane.org>
Thank you!
Jonas
--
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: hung task in mac80211
From: Stefano Brivio @ 2017-09-06 12:40 UTC (permalink / raw)
To: Matteo Croce; +Cc: linux-wireless, netdev, linux-kernel, Johannes Berg
In-Reply-To: <CAGnkfhwi9MmtLneeU23iWEGLj9cb1ODb3KzzOqTDvA6nNVsugg@mail.gmail.com>
On Wed, 6 Sep 2017 13:57:47 +0200
Matteo Croce <mcroce@redhat.com> wrote:
> Hi,
>
> I have an hung task on vanilla 4.13 kernel which I haven't on 4.12.
> The problem is present both on my AP and on my notebook,
> so it seems it affects AP and STA mode as well.
> The generated messages are:
>
> INFO: task kworker/u16:6:120 blocked for more than 120 seconds.
> Not tainted 4.13.0 #57
> "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message.
> kworker/u16:6 D 0 120 2 0x00000000
> Workqueue: phy0 ieee80211_ba_session_work [mac80211]
> Call Trace:
> ? __schedule+0x174/0x5b0
> ? schedule+0x31/0x80
> ? schedule_preempt_disabled+0x9/0x10
> ? __mutex_lock.isra.2+0x163/0x480
> ? select_task_rq_fair+0xb9f/0xc60
> ? __ieee80211_start_rx_ba_session+0x135/0x4d0 [mac80211]
> ? __ieee80211_start_rx_ba_session+0x135/0x4d0 [mac80211]
This is ugly and maybe wrong, but you could check perhaps...:
diff --git a/net/mac80211/ht.c b/net/mac80211/ht.c
index c92df492e898..bd7512a656f2 100644
--- a/net/mac80211/ht.c
+++ b/net/mac80211/ht.c
@@ -320,28 +320,40 @@ void ieee80211_ba_session_work(struct work_struct *work)
mutex_lock(&sta->ampdu_mlme.mtx);
for (tid = 0; tid < IEEE80211_NUM_TIDS; tid++) {
- if (test_and_clear_bit(tid, sta->ampdu_mlme.tid_rx_timer_expired))
+ if (test_and_clear_bit(tid, sta->ampdu_mlme.tid_rx_timer_expired)) {
+ mutex_unlock(&sta->ampdu_mlme.mtx);
___ieee80211_stop_rx_ba_session(
sta, tid, WLAN_BACK_RECIPIENT,
WLAN_REASON_QSTA_TIMEOUT, true);
+ mutex_lock(&sta->ampdu_mlme.mtx);
+ }
if (test_and_clear_bit(tid,
- sta->ampdu_mlme.tid_rx_stop_requested))
+ sta->ampdu_mlme.tid_rx_stop_requested)) {
+ mutex_unlock(&sta->ampdu_mlme.mtx);
___ieee80211_stop_rx_ba_session(
sta, tid, WLAN_BACK_RECIPIENT,
WLAN_REASON_UNSPECIFIED, true);
+ mutex_lock(&sta->ampdu_mlme.mtx);
+ }
if (test_and_clear_bit(tid,
- sta->ampdu_mlme.tid_rx_manage_offl))
+ sta->ampdu_mlme.tid_rx_manage_offl)) {
+ mutex_unlock(&sta->ampdu_mlme.mtx);
__ieee80211_start_rx_ba_session(sta, 0, 0, 0, 1, tid,
IEEE80211_MAX_AMPDU_BUF,
false, true);
+ mutex_lock(&sta->ampdu_mlme.mtx);
+ }
if (test_and_clear_bit(tid + IEEE80211_NUM_TIDS,
- sta->ampdu_mlme.tid_rx_manage_offl))
+ sta->ampdu_mlme.tid_rx_manage_offl)) {
+ mutex_unlock(&sta->ampdu_mlme.mtx);
___ieee80211_stop_rx_ba_session(
sta, tid, WLAN_BACK_RECIPIENT,
0, false);
+ mutex_lock(&sta->ampdu_mlme.mtx);
+ }
spin_lock_bh(&sta->lock);
--
Stefano
^ permalink raw reply related
* [PATCH net] udp: drop head states only when all skb references are gone
From: Paolo Abeni @ 2017-09-06 12:44 UTC (permalink / raw)
To: netdev; +Cc: David S. Miller, Eric Dumazet, Hannes Frederic Sowa
In-Reply-To: <1504631927.15310.47.camel@edumazet-glaptop3.roam.corp.google.com>
After commit 0ddf3fb2c43d ("udp: preserve skb->dst if required
for IP options processing") we clear the skb head state as soon
as the skb carrying them is first processed.
Since the same skb can be processed several times when MSG_PEEK
is used, we can end up lacking the required head states, and
eventually oopsing.
Fix this clearing the skb head state only when processing the
last skb reference.
Reported-by: Eric Dumazet <edumazet@google.com>
Fixes: 0ddf3fb2c43d ("udp: preserve skb->dst if required for IP options processing")
Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
include/linux/skbuff.h | 2 +-
net/core/skbuff.c | 9 +++------
net/ipv4/udp.c | 5 ++++-
3 files changed, 8 insertions(+), 8 deletions(-)
diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index d67a8182e5eb..63df75ae70ee 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -885,7 +885,7 @@ void kfree_skb(struct sk_buff *skb);
void kfree_skb_list(struct sk_buff *segs);
void skb_tx_error(struct sk_buff *skb);
void consume_skb(struct sk_buff *skb);
-void consume_stateless_skb(struct sk_buff *skb);
+void __consume_stateless_skb(struct sk_buff *skb);
void __kfree_skb(struct sk_buff *skb);
extern struct kmem_cache *skbuff_head_cache;
diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index e07556606284..72eb23d2426f 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -753,14 +753,11 @@ EXPORT_SYMBOL(consume_skb);
* consume_stateless_skb - free an skbuff, assuming it is stateless
* @skb: buffer to free
*
- * Works like consume_skb(), but this variant assumes that all the head
- * states have been already dropped.
+ * Alike consume_skb(), but this variant assumes that this is the last
+ * skb reference and all the head states have been already dropped
*/
-void consume_stateless_skb(struct sk_buff *skb)
+void __consume_stateless_skb(struct sk_buff *skb)
{
- if (!skb_unref(skb))
- return;
-
trace_consume_skb(skb);
if (likely(skb->head))
skb_release_data(skb);
diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c
index 62344804baae..979e4d8526ba 100644
--- a/net/ipv4/udp.c
+++ b/net/ipv4/udp.c
@@ -1386,12 +1386,15 @@ void skb_consume_udp(struct sock *sk, struct sk_buff *skb, int len)
unlock_sock_fast(sk, slow);
}
+ if (!skb_unref(skb))
+ return;
+
/* In the more common cases we cleared the head states previously,
* see __udp_queue_rcv_skb().
*/
if (unlikely(udp_skb_has_head_state(skb)))
skb_release_head_state(skb);
- consume_stateless_skb(skb);
+ __consume_stateless_skb(skb);
}
EXPORT_SYMBOL_GPL(skb_consume_udp);
--
2.13.5
^ permalink raw reply related
* Re: hung task in mac80211
From: Johannes Berg @ 2017-09-06 12:48 UTC (permalink / raw)
To: Stefano Brivio, Matteo Croce; +Cc: linux-wireless, netdev, linux-kernel
In-Reply-To: <20170906144019.1c98a636@elisabeth>
I'll look in a bit - but
> + mutex_unlock(&sta->ampdu_mlme.mtx);
> ___ieee80211_stop_rx_ba_session(
> sta, tid, WLAN_BACK_RECIPIENT,
> WLAN_REASON_QSTA_TIMEOUT, true);
This already has three underscores so shouldn't drop.
>
> + mutex_unlock(&sta->ampdu_mlme.mtx);
> ___ieee80211_stop_rx_ba_session(
ditto
> + mutex_unlock(&sta->ampdu_mlme.mtx);
> __ieee80211_start_rx_ba_session(sta, 0, 0,
> 0, 1, tid,
maybe this one needs a ___ version then?
> + mutex_unlock(&sta->ampdu_mlme.mtx);
> ___ieee80211_stop_rx_ba_session(
>
already ___
I'm surprised nobody saw this before - though perhaps Sebastian's
useless report is the same.
johannes
^ permalink raw reply
* Re: [PATCH] tipc: remove unnecessary call to dev_net()
From: Ying Xue @ 2017-09-06 12:50 UTC (permalink / raw)
To: Kleber Sacilotto de Souza, netdev; +Cc: Jon Maloy
In-Reply-To: <20170906090806.20931-1-kleber.souza@canonical.com>
On 09/06/2017 05:08 PM, Kleber Sacilotto de Souza wrote:
> The net device is already stored in the 'net' variable, so no need to call
> dev_net() again.
>
> Signed-off-by: Kleber Sacilotto de Souza <kleber.souza@canonical.com>
Acked-by: Ying Xue <ying.xue@windriver.com>
> ---
> net/tipc/bearer.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/net/tipc/bearer.c b/net/tipc/bearer.c
> index 89cd061c4468..6d6395e0904e 100644
> --- a/net/tipc/bearer.c
> +++ b/net/tipc/bearer.c
> @@ -661,7 +661,7 @@ static int tipc_l2_device_event(struct notifier_block *nb, unsigned long evt,
> break;
> case NETDEV_UNREGISTER:
> case NETDEV_CHANGENAME:
> - bearer_disable(dev_net(dev), b);
> + bearer_disable(net, b);
> break;
> }
> return NOTIFY_OK;
>
^ permalink raw reply
* Re: hung task in mac80211
From: Johannes Berg @ 2017-09-06 12:58 UTC (permalink / raw)
To: Matteo Croce, linux-wireless, netdev, linux-kernel
In-Reply-To: <CAGnkfhwi9MmtLneeU23iWEGLj9cb1ODb3KzzOqTDvA6nNVsugg@mail.gmail.com>
On Wed, 2017-09-06 at 13:57 +0200, Matteo Croce wrote:
> I have an hung task on vanilla 4.13 kernel which I haven't on 4.12.
> The problem is present both on my AP and on my notebook,
> so it seems it affects AP and STA mode as well.
> The generated messages are:
>
> INFO: task kworker/u16:6:120 blocked for more than 120 seconds.
> Not tainted 4.13.0 #57
> "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this
> message.
> kworker/u16:6 D 0 120 2 0x00000000
> Workqueue: phy0 ieee80211_ba_session_work [mac80211]
> Call Trace:
> ? __schedule+0x174/0x5b0
> ? schedule+0x31/0x80
> ? schedule_preempt_disabled+0x9/0x10
> ? __mutex_lock.isra.2+0x163/0x480
> ? select_task_rq_fair+0xb9f/0xc60
> ? __ieee80211_start_rx_ba_session+0x135/0x4d0 [mac80211]
> ? __ieee80211_start_rx_ba_session+0x135/0x4d0 [mac80211]
Yeah - obviously as Stefano found, both take &sta->ampdu_mlme.mtx.
Can you try this?
diff --git a/net/mac80211/agg-rx.c b/net/mac80211/agg-rx.c
index 2b36eff5d97e..d8d32776031e 100644
--- a/net/mac80211/agg-rx.c
+++ b/net/mac80211/agg-rx.c
@@ -245,10 +245,10 @@ static void ieee80211_send_addba_resp(struct ieee80211_sub_if_data *sdata, u8 *d
ieee80211_tx_skb(sdata, skb);
}
-void __ieee80211_start_rx_ba_session(struct sta_info *sta,
- u8 dialog_token, u16 timeout,
- u16 start_seq_num, u16 ba_policy, u16 tid,
- u16 buf_size, bool tx, bool auto_seq)
+void ___ieee80211_start_rx_ba_session(struct sta_info *sta,
+ u8 dialog_token, u16 timeout,
+ u16 start_seq_num, u16 ba_policy, u16 tid,
+ u16 buf_size, bool tx, bool auto_seq)
{
struct ieee80211_local *local = sta->sdata->local;
struct tid_ampdu_rx *tid_agg_rx;
@@ -267,7 +267,7 @@ void __ieee80211_start_rx_ba_session(struct sta_info *sta,
ht_dbg(sta->sdata,
"STA %pM requests BA session on unsupported tid %d\n",
sta->sta.addr, tid);
- goto end_no_lock;
+ goto end;
}
if (!sta->sta.ht_cap.ht_supported) {
@@ -275,14 +275,14 @@ void __ieee80211_start_rx_ba_session(struct sta_info *sta,
"STA %pM erroneously requests BA session on tid %d w/o QoS\n",
sta->sta.addr, tid);
/* send a response anyway, it's an error case if we get here */
- goto end_no_lock;
+ goto end;
}
if (test_sta_flag(sta, WLAN_STA_BLOCK_BA)) {
ht_dbg(sta->sdata,
"Suspend in progress - Denying ADDBA request (%pM tid %d)\n",
sta->sta.addr, tid);
- goto end_no_lock;
+ goto end;
}
/* sanity check for incoming parameters:
@@ -296,7 +296,7 @@ void __ieee80211_start_rx_ba_session(struct sta_info *sta,
ht_dbg_ratelimited(sta->sdata,
"AddBA Req with bad params from %pM on tid %u. policy %d, buffer size %d\n",
sta->sta.addr, tid, ba_policy, buf_size);
- goto end_no_lock;
+ goto end;
}
/* determine default buffer size */
if (buf_size == 0)
@@ -311,7 +311,6 @@ void __ieee80211_start_rx_ba_session(struct sta_info *sta,
buf_size, sta->sta.addr);
/* examine state machine */
- mutex_lock(&sta->ampdu_mlme.mtx);
if (test_bit(tid, sta->ampdu_mlme.agg_session_valid)) {
if (sta->ampdu_mlme.tid_rx_token[tid] == dialog_token) {
@@ -415,15 +414,25 @@ void __ieee80211_start_rx_ba_session(struct sta_info *sta,
__clear_bit(tid, sta->ampdu_mlme.unexpected_agg);
sta->ampdu_mlme.tid_rx_token[tid] = dialog_token;
}
- mutex_unlock(&sta->ampdu_mlme.mtx);
-end_no_lock:
if (tx)
ieee80211_send_addba_resp(sta->sdata, sta->sta.addr, tid,
dialog_token, status, 1, buf_size,
timeout);
}
+void __ieee80211_start_rx_ba_session(struct sta_info *sta,
+ u8 dialog_token, u16 timeout,
+ u16 start_seq_num, u16 ba_policy, u16 tid,
+ u16 buf_size, bool tx, bool auto_seq)
+{
+ mutex_lock(&sta->ampdu_mlme.mtx);
+ ___ieee80211_start_rx_ba_session(sta, dialog_token, timeout,
+ start_seq_num, ba_policy, tid,
+ buf_size, tx, auto_seq);
+ mutex_unlock(&sta->ampdu_mlme.mtx);
+}
+
void ieee80211_process_addba_request(struct ieee80211_local *local,
struct sta_info *sta,
struct ieee80211_mgmt *mgmt,
diff --git a/net/mac80211/ht.c b/net/mac80211/ht.c
index c92df492e898..198b2d3e56fd 100644
--- a/net/mac80211/ht.c
+++ b/net/mac80211/ht.c
@@ -333,9 +333,9 @@ void ieee80211_ba_session_work(struct work_struct *work)
if (test_and_clear_bit(tid,
sta->ampdu_mlme.tid_rx_manage_offl))
- __ieee80211_start_rx_ba_session(sta, 0, 0, 0, 1, tid,
- IEEE80211_MAX_AMPDU_BUF,
- false, true);
+ ___ieee80211_start_rx_ba_session(sta, 0, 0, 0, 1, tid,
+ IEEE80211_MAX_AMPDU_BUF,
+ false, true);
if (test_and_clear_bit(tid + IEEE80211_NUM_TIDS,
sta->ampdu_mlme.tid_rx_manage_offl))
diff --git a/net/mac80211/ieee80211_i.h b/net/mac80211/ieee80211_i.h
index 2197c62a0a6e..9675814f64db 100644
--- a/net/mac80211/ieee80211_i.h
+++ b/net/mac80211/ieee80211_i.h
@@ -1760,6 +1760,10 @@ void __ieee80211_start_rx_ba_session(struct sta_info *sta,
u8 dialog_token, u16 timeout,
u16 start_seq_num, u16 ba_policy, u16 tid,
u16 buf_size, bool tx, bool auto_seq);
+void ___ieee80211_start_rx_ba_session(struct sta_info *sta,
+ u8 dialog_token, u16 timeout,
+ u16 start_seq_num, u16 ba_policy, u16 tid,
+ u16 buf_size, bool tx, bool auto_seq);
void ieee80211_sta_tear_down_BA_sessions(struct sta_info *sta,
enum ieee80211_agg_stop_reason reason);
void ieee80211_process_delba(struct ieee80211_sub_if_data *sdata,
johannes
^ permalink raw reply related
* [PATCH] ath10: mark PM functions as __maybe_unused
From: Arnd Bergmann @ 2017-09-06 12:58 UTC (permalink / raw)
To: Kalle Valo
Cc: Arnd Bergmann, Colin Ian King, Bartosz Markowski, Govind Singh,
Ryan Hsu, Srinivas Kandagatla, Rajkumar Manoharan,
Ashok Raj Nagarajan, Ben Greear, ath10k, linux-wireless, netdev,
linux-kernel
When CONFIG_PM_SLEEP is disabled, we get a compile-time
warning:
drivers/net/wireless/ath/ath10k/pci.c:3417:12: error: 'ath10k_pci_pm_resume' defined but not used [-Werror=unused-function]
static int ath10k_pci_pm_resume(struct device *dev)
^~~~~~~~~~~~~~~~~~~~
drivers/net/wireless/ath/ath10k/pci.c:3401:12: error: 'ath10k_pci_pm_suspend' defined but not used [-Werror=unused-function]
static int ath10k_pci_pm_suspend(struct device *dev)
Rather than fixing the #ifdef, this just marks both functions
as __maybe_unused, which is a more robust way to do this.
Fixes: 32faa3f0ee50 ("ath10k: add the PCI PM core suspend/resume ops")
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
drivers/net/wireless/ath/ath10k/pci.c | 7 ++-----
1 file changed, 2 insertions(+), 5 deletions(-)
diff --git a/drivers/net/wireless/ath/ath10k/pci.c b/drivers/net/wireless/ath/ath10k/pci.c
index bc1633945a56..195dafb98131 100644
--- a/drivers/net/wireless/ath/ath10k/pci.c
+++ b/drivers/net/wireless/ath/ath10k/pci.c
@@ -3396,9 +3396,7 @@ static void ath10k_pci_remove(struct pci_dev *pdev)
MODULE_DEVICE_TABLE(pci, ath10k_pci_id_table);
-#ifdef CONFIG_PM
-
-static int ath10k_pci_pm_suspend(struct device *dev)
+static __maybe_unused int ath10k_pci_pm_suspend(struct device *dev)
{
struct ath10k *ar = dev_get_drvdata(dev);
int ret;
@@ -3414,7 +3412,7 @@ static int ath10k_pci_pm_suspend(struct device *dev)
return ret;
}
-static int ath10k_pci_pm_resume(struct device *dev)
+static __maybe_unused int ath10k_pci_pm_resume(struct device *dev)
{
struct ath10k *ar = dev_get_drvdata(dev);
int ret;
@@ -3433,7 +3431,6 @@ static int ath10k_pci_pm_resume(struct device *dev)
static SIMPLE_DEV_PM_OPS(ath10k_pci_pm_ops,
ath10k_pci_pm_suspend,
ath10k_pci_pm_resume);
-#endif
static struct pci_driver ath10k_pci_driver = {
.name = "ath10k_pci",
--
2.9.0
^ permalink raw reply related
* Re: hung task in mac80211
From: Johannes Berg @ 2017-09-06 13:03 UTC (permalink / raw)
To: Stefano Brivio, Matteo Croce; +Cc: linux-wireless, netdev, linux-kernel
In-Reply-To: <1504702115.13457.16.camel@sipsolutions.net>
On Wed, 2017-09-06 at 14:48 +0200, Johannes Berg wrote:
>
> I'm surprised nobody saw this before - though perhaps Sebastian's
> useless report is the same.
Oh, that's because this is only for the offloaded manager thing, and
that's only ath10k.
johannes
^ permalink raw reply
* Re: hung task in mac80211
From: Matteo Croce @ 2017-09-06 13:07 UTC (permalink / raw)
To: Stefano Brivio
Cc: linux-wireless-u79uwXL29TY76Z2rM5mHXA,
netdev-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA, Johannes Berg
In-Reply-To: <20170906144019.1c98a636@elisabeth>
On Wed, Sep 6, 2017 at 2:40 PM, Stefano Brivio <sbrivio-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> On Wed, 6 Sep 2017 13:57:47 +0200
> Matteo Croce <mcroce-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
>
>> Hi,
>>
>> I have an hung task on vanilla 4.13 kernel which I haven't on 4.12.
>> The problem is present both on my AP and on my notebook,
>> so it seems it affects AP and STA mode as well.
>> The generated messages are:
>>
>> INFO: task kworker/u16:6:120 blocked for more than 120 seconds.
>> Not tainted 4.13.0 #57
>> "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message.
>> kworker/u16:6 D 0 120 2 0x00000000
>> Workqueue: phy0 ieee80211_ba_session_work [mac80211]
>> Call Trace:
>> ? __schedule+0x174/0x5b0
>> ? schedule+0x31/0x80
>> ? schedule_preempt_disabled+0x9/0x10
>> ? __mutex_lock.isra.2+0x163/0x480
>> ? select_task_rq_fair+0xb9f/0xc60
>> ? __ieee80211_start_rx_ba_session+0x135/0x4d0 [mac80211]
>> ? __ieee80211_start_rx_ba_session+0x135/0x4d0 [mac80211]
>
> This is ugly and maybe wrong, but you could check perhaps...:
>
> diff --git a/net/mac80211/ht.c b/net/mac80211/ht.c
> index c92df492e898..bd7512a656f2 100644
> --- a/net/mac80211/ht.c
> +++ b/net/mac80211/ht.c
> @@ -320,28 +320,40 @@ void ieee80211_ba_session_work(struct work_struct *work)
>
> mutex_lock(&sta->ampdu_mlme.mtx);
> for (tid = 0; tid < IEEE80211_NUM_TIDS; tid++) {
> - if (test_and_clear_bit(tid, sta->ampdu_mlme.tid_rx_timer_expired))
> + if (test_and_clear_bit(tid, sta->ampdu_mlme.tid_rx_timer_expired)) {
> + mutex_unlock(&sta->ampdu_mlme.mtx);
> ___ieee80211_stop_rx_ba_session(
> sta, tid, WLAN_BACK_RECIPIENT,
> WLAN_REASON_QSTA_TIMEOUT, true);
> + mutex_lock(&sta->ampdu_mlme.mtx);
> + }
>
> if (test_and_clear_bit(tid,
> - sta->ampdu_mlme.tid_rx_stop_requested))
> + sta->ampdu_mlme.tid_rx_stop_requested)) {
> + mutex_unlock(&sta->ampdu_mlme.mtx);
> ___ieee80211_stop_rx_ba_session(
> sta, tid, WLAN_BACK_RECIPIENT,
> WLAN_REASON_UNSPECIFIED, true);
> + mutex_lock(&sta->ampdu_mlme.mtx);
> + }
>
> if (test_and_clear_bit(tid,
> - sta->ampdu_mlme.tid_rx_manage_offl))
> + sta->ampdu_mlme.tid_rx_manage_offl)) {
> + mutex_unlock(&sta->ampdu_mlme.mtx);
> __ieee80211_start_rx_ba_session(sta, 0, 0, 0, 1, tid,
> IEEE80211_MAX_AMPDU_BUF,
> false, true);
> + mutex_lock(&sta->ampdu_mlme.mtx);
> + }
>
> if (test_and_clear_bit(tid + IEEE80211_NUM_TIDS,
> - sta->ampdu_mlme.tid_rx_manage_offl))
> + sta->ampdu_mlme.tid_rx_manage_offl)) {
> + mutex_unlock(&sta->ampdu_mlme.mtx);
> ___ieee80211_stop_rx_ba_session(
> sta, tid, WLAN_BACK_RECIPIENT,
> 0, false);
> + mutex_lock(&sta->ampdu_mlme.mtx);
> + }
>
> spin_lock_bh(&sta->lock);
>
> --
> Stefano
>
ACK, I have it running since 12 minutes.
The hang usually appears shortly after boot as I set
kernel.hung_task_timeout_secs=10
--
Matteo Croce
per aspera ad upstream
^ permalink raw reply
* Re: hung task in mac80211
From: Sebastian Gottschall @ 2017-09-06 13:08 UTC (permalink / raw)
To: Johannes Berg, Stefano Brivio, Matteo Croce
Cc: linux-wireless-u79uwXL29TY76Z2rM5mHXA,
netdev-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <1504702990.13457.19.camel-cdvu00un1VgdHxzADdlk8Q@public.gmane.org>
Am 06.09.2017 um 15:03 schrieb Johannes Berg:
> On Wed, 2017-09-06 at 14:48 +0200, Johannes Berg wrote:
>> I'm surprised nobody saw this before - though perhaps Sebastian's
>> useless report is the same.
> Oh, that's because this is only for the offloaded manager thing, and
> that's only ath10k.
>
> johannes
that explans the behaviour i have found with latest mac80211 and ath10k.
--
Mit freundlichen Grüssen / Regards
Sebastian Gottschall / CTO
NewMedia-NET GmbH - DD-WRT
Firmensitz: Berliner Ring 101, 64625 Bensheim
Registergericht: Amtsgericht Darmstadt, HRB 25473
Geschäftsführer: Peter Steinhäuser, Christian Scheele
http://www.dd-wrt.com
email: s.gottschall-t37Kgv3TaIPQT0dZR+AlfA@public.gmane.org
Tel.: +496251-582650 / Fax: +496251-5826565
^ permalink raw reply
* Re: hung task in mac80211
From: Stefano Brivio @ 2017-09-06 13:19 UTC (permalink / raw)
To: Johannes Berg; +Cc: Matteo Croce, linux-wireless, netdev, linux-kernel
In-Reply-To: <1504702115.13457.16.camel@sipsolutions.net>
On Wed, 06 Sep 2017 14:48:35 +0200
Johannes Berg <johannes@sipsolutions.net> wrote:
> I'll look in a bit - but
>
> > + mutex_unlock(&sta->ampdu_mlme.mtx);
> > ___ieee80211_stop_rx_ba_session(
> > sta, tid, WLAN_BACK_RECIPIENT,
> > WLAN_REASON_QSTA_TIMEOUT, true);
>
> This already has three underscores so shouldn't drop.
Right, of course.
> [...]
> > + mutex_unlock(&sta->ampdu_mlme.mtx);
> > __ieee80211_start_rx_ba_session(sta, 0, 0,
> > 0, 1, tid,
>
> maybe this one needs a ___ version then?
Either that, or as it's a single call, perhaps just the following?
Matter of taste I guess...
diff --git a/net/mac80211/ht.c b/net/mac80211/ht.c
index c92df492e898..377dd3c233d3 100644
--- a/net/mac80211/ht.c
+++ b/net/mac80211/ht.c
@@ -332,10 +332,13 @@ void ieee80211_ba_session_work(struct work_struct *work)
WLAN_REASON_UNSPECIFIED, true);
if (test_and_clear_bit(tid,
- sta->ampdu_mlme.tid_rx_manage_offl))
+ sta->ampdu_mlme.tid_rx_manage_offl)) {
+ mutex_unlock(&sta->ampdu_mlme.mtx);
__ieee80211_start_rx_ba_session(sta, 0, 0, 0, 1, tid,
IEEE80211_MAX_AMPDU_BUF,
false, true);
+ mutex_lock(&sta->ampdu_mlme.mtx);
+ }
if (test_and_clear_bit(tid + IEEE80211_NUM_TIDS,
sta->ampdu_mlme.tid_rx_manage_offl))
--
Stefano
^ permalink raw reply related
* Re: hung task in mac80211
From: Johannes Berg @ 2017-09-06 13:21 UTC (permalink / raw)
To: Stefano Brivio; +Cc: Matteo Croce, linux-wireless, netdev, linux-kernel
In-Reply-To: <20170906151922.4a320b1d@elisabeth>
On Wed, 2017-09-06 at 15:19 +0200, Stefano Brivio wrote:
> On Wed, 06 Sep 2017 14:48:35 +0200
> Johannes Berg <johannes@sipsolutions.net> wrote:
>
> > I'll look in a bit - but
> >
> > > + mutex_unlock(&sta->ampdu_mlme.mtx);
> > > ___ieee80211_stop_rx_ba_session(
> > > sta, tid, WLAN_BACK_RECIPIENT,
> > > WLAN_REASON_QSTA_TIMEOUT,
> > > true);
> >
> > This already has three underscores so shouldn't drop.
>
> Right, of course.
>
> > [...]
> > > + mutex_unlock(&sta->ampdu_mlme.mtx);
> > > __ieee80211_start_rx_ba_session(sta, 0,
> > > 0,
> > > 0, 1, tid,
> >
> > maybe this one needs a ___ version then?
>
> Either that, or as it's a single call, perhaps just the following?
> Matter of taste I guess...
I don't think it's a matter of taste - for me, in principle, dropping
locks for small sections of code where the larger section holds it is a
bug waiting to happen. It may (may, I don't even know) be OK here, but
in general it's something to avoid.
johannes
^ permalink raw reply
* Re: hung task in mac80211
From: Stefano Brivio @ 2017-09-06 13:27 UTC (permalink / raw)
To: Johannes Berg; +Cc: Matteo Croce, linux-wireless, netdev, linux-kernel
In-Reply-To: <1504704060.13457.20.camel@sipsolutions.net>
On Wed, 06 Sep 2017 15:21:00 +0200
Johannes Berg <johannes@sipsolutions.net> wrote:
> On Wed, 2017-09-06 at 15:19 +0200, Stefano Brivio wrote:
> > On Wed, 06 Sep 2017 14:48:35 +0200
> > Johannes Berg <johannes@sipsolutions.net> wrote:
> >
> > > I'll look in a bit - but
> > >
> > > > + mutex_unlock(&sta->ampdu_mlme.mtx);
> > > > ___ieee80211_stop_rx_ba_session(
> > > > sta, tid, WLAN_BACK_RECIPIENT,
> > > > WLAN_REASON_QSTA_TIMEOUT,
> > > > true);
> > >
> > > This already has three underscores so shouldn't drop.
> >
> > Right, of course.
> >
> > > [...]
> > > > + mutex_unlock(&sta->ampdu_mlme.mtx);
> > > > __ieee80211_start_rx_ba_session(sta, 0,
> > > > 0,
> > > > 0, 1, tid,
> > >
> > > maybe this one needs a ___ version then?
> >
> > Either that, or as it's a single call, perhaps just the following?
> > Matter of taste I guess...
>
> I don't think it's a matter of taste - for me, in principle, dropping
> locks for small sections of code where the larger section holds it is a
> bug waiting to happen. It may (may, I don't even know) be OK here, but
> in general it's something to avoid.
Yes, that was based on the assumption that the initial part of
__ieee80211_start_rx_ba_session() can't really affect the AMPDU
state-machine in any way.
But sure, one small change there in the future and the assumption
doesn't hold anymore.
--
Stefano
^ permalink raw reply
* Re: hung task in mac80211
From: Johannes Berg @ 2017-09-06 13:30 UTC (permalink / raw)
To: Stefano Brivio; +Cc: Matteo Croce, linux-wireless, netdev, linux-kernel
In-Reply-To: <20170906152709.673f230d@elisabeth>
On Wed, 2017-09-06 at 15:27 +0200, Stefano Brivio wrote:
>
> Yes, that was based on the assumption that the initial part of
> __ieee80211_start_rx_ba_session() can't really affect the AMPDU
> state-machine in any way.
That's not really the point, if that changes that function would have
to move the locking around, and nothing else.
The point is more that code in ieee80211_ba_session_work() could assume
the lock is held across the entire loop, since that's the way it's
written and looks like even with your patch.
So for example replacing the loop of tid = 0..NUM_TIDS-1 with a
list_for_each_entry() would already be unsafe with the dropping if the
list were to require the mutex for locking.
johannes
^ permalink raw reply
* [PATCH] isdn: isdnloop: fix logic error in isdnloop_sendbuf
From: Arnd Bergmann @ 2017-09-06 13:38 UTC (permalink / raw)
To: Karsten Keil
Cc: Arnd Bergmann, Joe Perches, David S. Miller, netdev, linux-kernel
gcc-7 found an ancient bug in the loop driver, leading to a condition that
is always false, meaning we ignore the contents of 'card->flags' here:
drivers/isdn/isdnloop/isdnloop.c:412:37: error: ?: using integer constants in boolean context, the expression will always evaluate to 'true' [-Werror=int-in-bool-context]
This changes the braces in the expression to ensure we actually
compare the flag bits, rather than comparing a constant. As Joe Perches
pointed out, an earlier patch of mine incorrectly assumed this was a
false-positive warning.
Cc: Joe Perches <joe@perches.com>
Link: https://patchwork.kernel.org/patch/9840289/
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
drivers/isdn/isdnloop/isdnloop.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/isdn/isdnloop/isdnloop.c b/drivers/isdn/isdnloop/isdnloop.c
index 6ffd13466b8c..e97232646ba1 100644
--- a/drivers/isdn/isdnloop/isdnloop.c
+++ b/drivers/isdn/isdnloop/isdnloop.c
@@ -409,7 +409,7 @@ isdnloop_sendbuf(int channel, struct sk_buff *skb, isdnloop_card *card)
return -EINVAL;
}
if (len) {
- if (!(card->flags & (channel) ? ISDNLOOP_FLAGS_B2ACTIVE : ISDNLOOP_FLAGS_B1ACTIVE))
+ if (!(card->flags & (channel ? ISDNLOOP_FLAGS_B2ACTIVE : ISDNLOOP_FLAGS_B1ACTIVE)))
return 0;
if (card->sndcount[channel] > ISDNLOOP_MAX_SQUEUE)
return 0;
--
2.9.0
^ permalink raw reply related
* Re: hung task in mac80211
From: Stefano Brivio @ 2017-09-06 13:40 UTC (permalink / raw)
To: Johannes Berg; +Cc: Matteo Croce, linux-wireless, netdev, linux-kernel
In-Reply-To: <1504704610.23905.1.camel@sipsolutions.net>
On Wed, 06 Sep 2017 15:30:10 +0200
Johannes Berg <johannes@sipsolutions.net> wrote:
> So for example replacing the loop of tid = 0..NUM_TIDS-1 with a
> list_for_each_entry() would already be unsafe with the dropping if the
> list were to require the mutex for locking.
Sure. Still, it would need another code change to break, but in general
I do agree indeed. :)
--
Stefano
^ permalink raw reply
* Re: [PATCH net 1/4] lan78xx: Fix for crash associated with System suspend
From: Andrew Lunn @ 2017-09-06 13:59 UTC (permalink / raw)
To: Nisar.Sayed; +Cc: davem, UNGLinuxDriver, netdev
In-Reply-To: <CE371C1263339941885964188A0225FA333370@CHN-SV-EXMX03.mchp-main.com>
On Wed, Sep 06, 2017 at 10:51:31AM +0000, Nisar.Sayed@microchip.com wrote:
> From: Nisar Sayed <Nisar.Sayed@microchip.com>
>
> Fix for crash associated with System suspend
>
> Since ndo_stop removes phydev which makes phydev NULL.
> Whenever system suspend is initiated or after "ifconfig <interface> down",
> if set_wol or get_wol is triggered phydev is NULL leads system crash.
> Hence phy_start/phy_stop for ndo_start/ndo_stop fixes the issues
> instead of adding/removing phydevice
Looking at this patch, there apears to be lots of different things
going on. Please can you split it up into multiple patches.
> Signed-off-by: Nisar Sayed <Nisar.Sayed@microchip.com>
> ---
> drivers/net/usb/lan78xx.c | 44 ++++++++++++++++++++++++++++----------------
> 1 file changed, 28 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/net/usb/lan78xx.c b/drivers/net/usb/lan78xx.c
> index b99a7fb..955ab3b 100644
> --- a/drivers/net/usb/lan78xx.c
> +++ b/drivers/net/usb/lan78xx.c
> @@ -2024,6 +2024,8 @@ static int lan78xx_phy_init(struct lan78xx_net *dev)
> lan8835_fixup);
> if (ret < 0) {
> netdev_err(dev->net, "fail to register fixup\n");
> + phy_unregister_fixup_for_uid(PHY_KSZ9031RNX,
> + 0xfffffff0);
goto error; would be better. phy_unregister_fixup_for_uid() does not care if you try to unregister
something which has not been registered.
Also, this should be a separate patch.
> return ret;
> }
> /* add more external PHY fixup here if needed */
> @@ -2031,8 +2033,7 @@ static int lan78xx_phy_init(struct lan78xx_net *dev)
> phydev->is_internal = false;
> } else {
> netdev_err(dev->net, "unknown ID found\n");
> - ret = -EIO;
> - goto error;
> + return -EIO;
> }
>
> /* if phyirq is not set, use polling mode in phylib */
> @@ -2051,7 +2052,10 @@ static int lan78xx_phy_init(struct lan78xx_net *dev)
> if (ret) {
> netdev_err(dev->net, "can't attach PHY to %s\n",
> dev->mdiobus->id);
> - return -EIO;
> + ret = -EIO;
> + if (dev->chipid == ID_REV_CHIP_ID_7801_)
> + goto error;
> + return ret;
Why not add the if (dev->chipid == ID_REV_CHIP_ID_7801_) after the
error: label?
> }
>
> /* MAC doesn't support 1000T Half */
> @@ -2067,8 +2071,6 @@ static int lan78xx_phy_init(struct lan78xx_net *dev)
>
> dev->fc_autoneg = phydev->autoneg;
>
> - phy_start(phydev);
> -
> netif_dbg(dev, ifup, dev->net, "phy initialised successfully");
>
> return 0;
> @@ -2497,9 +2499,9 @@ static int lan78xx_open(struct net_device *net)
> if (ret < 0)
> goto done;
>
> - ret = lan78xx_phy_init(dev);
> - if (ret < 0)
> - goto done;
> + if (dev->domain_data.phyirq > 0)
> + phy_start_interrupts(dev->net->phydev);
This is unusual. I don't see any other MAC driver starting interrupts.
This needs explaining.
Andrew
^ permalink raw reply
* RE: [iproute PATCH 1/6] utils: Implement strlcpy() and strlcat()
From: David Laight @ 2017-09-06 13:59 UTC (permalink / raw)
To: 'Stephen Hemminger', Phil Sutter; +Cc: netdev@vger.kernel.org
In-Reply-To: <20170904112525.04bc66d5@xeon-e3>
From: Stephen Hemminger [mailto:stephen@networkplumber.org]
> Sent: 04 September 2017 19:25
> On Mon, 4 Sep 2017 17:00:15 +0200
> Phil Sutter <phil@nwl.cc> wrote:
>
> > On Mon, Sep 04, 2017 at 02:49:20PM +0000, David Laight wrote:
> > > From: Phil Sutter
> > > > Sent: 01 September 2017 17:53
> > > > By making use of strncpy(), both implementations are really simple so
> > > > there is no need to add libbsd as additional dependency.
> > > >
> > > ...
> > > > +
> > > > +size_t strlcpy(char *dst, const char *src, size_t size)
> > > > +{
> > > > + if (size) {
> > > > + strncpy(dst, src, size - 1);
> > > > + dst[size - 1] = '\0';
> > > > + }
> > > > + return strlen(src);
> > > > +}
> > >
> > > Except that isn't really strlcpy().
> > > Better would be:
> > > len = strlen(src) + 1;
> > > if (len <= size)
> > > memcpy(dst, src, len);
> > > else if (size) {
> > > dst[size - 1] = 0;
> > > memcpy(dst, src, size - 1);
> > > }
> > > return len - 1;
> >
> > Please elaborate: Why isn't my version "really" strlcpy()? Why is your
> > proposed version better?
> >
> > Thanks, Phil
>
> Linux kernel:
> size_t strlcpy(char *dest, const char *src, size_t size)
> {
> size_t ret = strlen(src);
>
> if (size) {
> size_t len = (ret >= size) ? size - 1 : ret;
> memcpy(dest, src, len);
> dest[len] = '\0';
> }
> return ret;
> }
>
> FreeBSD:
> size_t
> strlcpy(char * __restrict dst, const char * __restrict src, size_t dsize)
> {
> const char *osrc = src;
> size_t nleft = dsize;
>
> /* Copy as many bytes as will fit. */
> if (nleft != 0) {
> while (--nleft != 0) {
> if ((*dst++ = *src++) == '\0')
> break;
> }
> }
>
> /* Not enough room in dst, add NUL and traverse rest of src. */
> if (nleft == 0) {
> if (dsize != 0)
> *dst = '\0'; /* NUL-terminate dst */
> while (*src++)
> ;
> }
>
> return(src - osrc - 1); /* count does not include NUL */
> }
>
>
> They all give the same results for some basic tests.
> Test FreeBSD Linux Iproute2
> "",0: 0 "JUNK" 0 "JUNK" 0 "JUNK"
> "",1: 0 "" 0 "" 0 ""
> "",8: 0 "" 0 "" 0 ""
> "foo",0: 3 "JUNK" 3 "JUNK" 3 "JUNK"
> "foo",3: 3 "fo" 3 "fo" 3 "fo"
> "foo",4: 3 "foo" 3 "foo" 3 "foo"
> "foo",8: 3 "foo" 3 "foo" 3 "foo"
> "longstring",0: 10 "JUNK" 10 "JUNK" 10 "JUNK"
> "longstring",8: 10 "longstr" 10 "longstr" 10 "longstr"
You need to look at the contents of the destination buffer after the
first '\0'.
strlcpy() shouldn't change it.
David
^ permalink raw reply
* Re: [PATCH v2 rfc 0/8] IGMP snooping for local traffic
From: Vivien Didelot @ 2017-09-06 13:56 UTC (permalink / raw)
To: Andrew Lunn, netdev, Florian Fainelli, Woojung.Huh, jbe,
sean.wang, john
In-Reply-To: <20170906004703.GB27385@lunn.ch>
Hi Andrew,
Andrew Lunn <andrew@lunn.ch> writes:
> So there is the third issue. It affects just DSA, but it possible
> affects all DSA drivers.
>
> This patchset broken broadcast with the Marvell drivers. It could
> break broadcast on others drivers as well.
>
> What i found is that the Marvell chips don't flood broadcast frames
> between bridged ports. What appears to happen is there is a fdb miss,
> so it gets forwarded to the CPU port for the host to deal with. The
> software bridge when floods it out all ports of the bridge.
Do you have this issue on a single switch?
I do expect FDB (not MDB) miss on a multi-chip fabric, not on a single
chip though.
> But the set offload_fwd_mark patch changes this. The software bridge
> now assumes the hardware has already flooded broadcast out all ports
> of the switch as needed. So it does not do any flooding itself. As a
> result, on Marvell devices, broadcast packets don't get flooded at
> all.
>
> The issue can be fixed. I just need to add an mdb entry for the
> broadcast address to each port of the bridge in the switch, and the
> CPU port. But i don't know at what level to do this.
>
> Should this be done at the DSA level, or at the driver level? Do any
> chips do broadcast flooding in hardware already? Hence they currently
> see broadcast duplication? If i add a broadcast mdb at the DSA level,
> and the chip is already hard wired to flooding broadcast, is it going
> to double flood?
It should be done at the DSA level. DSA core must be the single entry
point to handle all the switch logic, calling into (dumb) DSA drivers.
If it is buried into a specific driver, we'll likely lose track of the
problem and make it harder to maintain.
Thanks!
Vivien
^ 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