* [PATCH] net: dsa: Fix off-by-one number of calls to devlink_port_unregister
From: Vladimir Oltean @ 2019-08-31 12:46 UTC (permalink / raw)
To: f.fainelli, vivien.didelot, andrew, davem; +Cc: netdev, Vladimir Oltean
When a function such as dsa_slave_create fails, currently the following
stack trace can be seen:
[ 2.038342] sja1105 spi0.1: Probed switch chip: SJA1105T
[ 2.054556] sja1105 spi0.1: Reset switch and programmed static config
[ 2.063837] sja1105 spi0.1: Enabled switch tagging
[ 2.068706] fsl-gianfar soc:ethernet@2d90000 eth2: error -19 setting up slave phy
[ 2.076371] ------------[ cut here ]------------
[ 2.080973] WARNING: CPU: 1 PID: 21 at net/core/devlink.c:6184 devlink_free+0x1b4/0x1c0
[ 2.088954] Modules linked in:
[ 2.092005] CPU: 1 PID: 21 Comm: kworker/1:1 Not tainted 5.3.0-rc6-01360-g41b52e38d2b6-dirty #1746
[ 2.100912] Hardware name: Freescale LS1021A
[ 2.105162] Workqueue: events deferred_probe_work_func
[ 2.110287] [<c03133a4>] (unwind_backtrace) from [<c030d8cc>] (show_stack+0x10/0x14)
[ 2.117992] [<c030d8cc>] (show_stack) from [<c10b08d8>] (dump_stack+0xb4/0xc8)
[ 2.125180] [<c10b08d8>] (dump_stack) from [<c0349d04>] (__warn+0xe0/0xf8)
[ 2.132018] [<c0349d04>] (__warn) from [<c0349e34>] (warn_slowpath_null+0x40/0x48)
[ 2.139549] [<c0349e34>] (warn_slowpath_null) from [<c0f19d74>] (devlink_free+0x1b4/0x1c0)
[ 2.147772] [<c0f19d74>] (devlink_free) from [<c1064fc0>] (dsa_switch_teardown+0x60/0x6c)
[ 2.155907] [<c1064fc0>] (dsa_switch_teardown) from [<c1065950>] (dsa_register_switch+0x8e4/0xaa8)
[ 2.164821] [<c1065950>] (dsa_register_switch) from [<c0ba7fe4>] (sja1105_probe+0x21c/0x2ec)
[ 2.173216] [<c0ba7fe4>] (sja1105_probe) from [<c0b35948>] (spi_drv_probe+0x80/0xa4)
[ 2.180920] [<c0b35948>] (spi_drv_probe) from [<c0a4c1cc>] (really_probe+0x108/0x400)
[ 2.188711] [<c0a4c1cc>] (really_probe) from [<c0a4c694>] (driver_probe_device+0x78/0x1bc)
[ 2.196933] [<c0a4c694>] (driver_probe_device) from [<c0a4a3dc>] (bus_for_each_drv+0x58/0xb8)
[ 2.205414] [<c0a4a3dc>] (bus_for_each_drv) from [<c0a4c024>] (__device_attach+0xd0/0x168)
[ 2.213637] [<c0a4c024>] (__device_attach) from [<c0a4b1d0>] (bus_probe_device+0x84/0x8c)
[ 2.221772] [<c0a4b1d0>] (bus_probe_device) from [<c0a4b72c>] (deferred_probe_work_func+0x84/0xc4)
[ 2.230686] [<c0a4b72c>] (deferred_probe_work_func) from [<c03650a4>] (process_one_work+0x218/0x510)
[ 2.239772] [<c03650a4>] (process_one_work) from [<c03660d8>] (worker_thread+0x2a8/0x5c0)
[ 2.247908] [<c03660d8>] (worker_thread) from [<c036b348>] (kthread+0x148/0x150)
[ 2.255265] [<c036b348>] (kthread) from [<c03010e8>] (ret_from_fork+0x14/0x2c)
[ 2.262444] Exception stack(0xea965fb0 to 0xea965ff8)
[ 2.267466] 5fa0: 00000000 00000000 00000000 00000000
[ 2.275598] 5fc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
[ 2.283729] 5fe0: 00000000 00000000 00000000 00000000 00000013 00000000
[ 2.290333] ---[ end trace ca5d506728a0581a ]---
devlink_free is complaining right here:
WARN_ON(!list_empty(&devlink->port_list));
This happens because devlink_port_unregister is no longer done right
away in dsa_port_setup when a DSA_PORT_TYPE_USER has failed.
Vivien said about this change that:
Also no need to call devlink_port_unregister from within dsa_port_setup
as this step is inconditionally handled by dsa_port_teardown on error.
which is not really true. The devlink_port_unregister function _is_
being called unconditionally from within dsa_port_setup, but not for
this port that just failed, just for the previous ones which were set
up.
ports_teardown:
for (i = 0; i < port; i++)
dsa_port_teardown(&ds->ports[i]);
Initially I was tempted to fix this by extending the "for" loop to also
cover the port that failed during setup. But this could have potentially
unforeseen consequences unrelated to devlink_port or even other types of
ports than user ports, which I can't really test for. For example, if
for some reason devlink_port_register itself would fail, then
unconditionally unregistering it in dsa_port_teardown would not be a
smart idea. The list might go on.
So just make dsa_port_setup undo the setup it had done upon failure, and
let the for loop undo the work of setting up the previous ports, which
are guaranteed to be brought up to a consistent state.
Fixes: 955222ca5281 ("net: dsa: use a single switch statement for port setup")
Signed-off-by: Vladimir Oltean <olteanv@gmail.com>
---
net/dsa/dsa2.c | 39 +++++++++++++++++++++++++++++----------
1 file changed, 29 insertions(+), 10 deletions(-)
diff --git a/net/dsa/dsa2.c b/net/dsa/dsa2.c
index f8445fa73448..b501c90aabe4 100644
--- a/net/dsa/dsa2.c
+++ b/net/dsa/dsa2.c
@@ -259,8 +259,11 @@ static int dsa_port_setup(struct dsa_port *dp)
const unsigned char *id = (const unsigned char *)&dst->index;
const unsigned char len = sizeof(dst->index);
struct devlink_port *dlp = &dp->devlink_port;
+ bool dsa_port_link_registered = false;
+ bool devlink_port_registered = false;
struct devlink *dl = ds->devlink;
- int err;
+ bool dsa_port_enabled = false;
+ int err = 0;
switch (dp->type) {
case DSA_PORT_TYPE_UNUSED:
@@ -272,15 +275,19 @@ static int dsa_port_setup(struct dsa_port *dp)
dp->index, false, 0, id, len);
err = devlink_port_register(dl, dlp, dp->index);
if (err)
- return err;
+ break;
+ devlink_port_registered = true;
err = dsa_port_link_register_of(dp);
if (err)
- return err;
+ break;
+ dsa_port_link_registered = true;
err = dsa_port_enable(dp, NULL);
if (err)
- return err;
+ break;
+ dsa_port_enabled = true;
+
break;
case DSA_PORT_TYPE_DSA:
memset(dlp, 0, sizeof(*dlp));
@@ -288,15 +295,19 @@ static int dsa_port_setup(struct dsa_port *dp)
dp->index, false, 0, id, len);
err = devlink_port_register(dl, dlp, dp->index);
if (err)
- return err;
+ break;
+ devlink_port_registered = true;
err = dsa_port_link_register_of(dp);
if (err)
- return err;
+ break;
+ dsa_port_link_registered = true;
err = dsa_port_enable(dp, NULL);
if (err)
- return err;
+ break;
+ dsa_port_enabled = true;
+
break;
case DSA_PORT_TYPE_USER:
memset(dlp, 0, sizeof(*dlp));
@@ -304,18 +315,26 @@ static int dsa_port_setup(struct dsa_port *dp)
dp->index, false, 0, id, len);
err = devlink_port_register(dl, dlp, dp->index);
if (err)
- return err;
+ break;
+ devlink_port_registered = true;
dp->mac = of_get_mac_address(dp->dn);
err = dsa_slave_create(dp);
if (err)
- return err;
+ break;
devlink_port_type_eth_set(dlp, dp->slave);
break;
}
- return 0;
+ if (err && dsa_port_enabled)
+ dsa_port_disable(dp);
+ if (err && dsa_port_link_registered)
+ dsa_port_link_unregister_of(dp);
+ if (err && devlink_port_registered)
+ devlink_port_unregister(dlp);
+
+ return err;
}
static void dsa_port_teardown(struct dsa_port *dp)
--
2.17.1
^ permalink raw reply related
* iproute2: tc: potential buffer overflow
From: tomaspaukrt @ 2019-08-31 13:13 UTC (permalink / raw)
To: netdev
[-- Attachment #1: Type: text/plain, Size: 149 bytes --]
Hi,
there are two potentially dangerous calls of strcpy function in the program "tc". In the attachment is a patch that fixes this issue.
Tomas
[-- Attachment #2: iproute2-overflow-fix.patch --]
[-- Type: text/x-diff, Size: 945 bytes --]
diff --git a/tc/m_ipt.c b/tc/m_ipt.c
index cc95eab7..cb64380b 100644
--- a/tc/m_ipt.c
+++ b/tc/m_ipt.c
@@ -269,7 +269,8 @@ static int build_st(struct xtables_target *target, struct ipt_entry_target *t)
} else {
target->t = t;
}
- strcpy(target->t->u.user.name, target->name);
+ strncpy(target->t->u.user.name, target->name,
+ sizeof(target->t->u.user.name) - 1);
return 0;
}
diff --git a/tc/m_xt_old.c b/tc/m_xt_old.c
index 6a4509a9..974ac496 100644
--- a/tc/m_xt_old.c
+++ b/tc/m_xt_old.c
@@ -177,7 +177,8 @@ build_st(struct xtables_target *target, struct xt_entry_target *t)
if (t == NULL) {
target->t = fw_calloc(1, size);
target->t->u.target_size = size;
- strcpy(target->t->u.user.name, target->name);
+ strncpy(target->t->u.user.name, target->name,
+ sizeof(target->t->u.user.name) - 1);
set_revision(target->t->u.user.name, target->revision);
if (target->init != NULL)
^ permalink raw reply related
* Re: [PATCH 4.14] tcp: fix tcp_rtx_queue_tail in case of empty retransmit queue
From: Matthieu Baerts @ 2019-08-31 13:14 UTC (permalink / raw)
To: Sasha Levin, Tim Froidcoeur
Cc: aprout, cpaasch, davem, edumazet, gregkh, jonathan.lemon, jtl,
linux-kernel, mkubecek, ncardwell, stable, ycheng, netdev
In-Reply-To: <20190831122036.GY5281@sasha-vm>
Hi Sasha,
Thank you for your reply!
On 31/08/2019 14:20, Sasha Levin wrote:
> On Sat, Aug 24, 2019 at 08:03:51AM +0200, Tim Froidcoeur wrote:
>> Commit 8c3088f895a0 ("tcp: be more careful in tcp_fragment()")
>> triggers following stack trace:
>>
>> [25244.848046] kernel BUG at ./include/linux/skbuff.h:1406!
>> [25244.859335] RIP: 0010:skb_queue_prev+0x9/0xc
>> [25244.888167] Call Trace:
>> [25244.889182] <IRQ>
>> [25244.890001] tcp_fragment+0x9c/0x2cf
>> [25244.891295] tcp_write_xmit+0x68f/0x988
>> [25244.892732] __tcp_push_pending_frames+0x3b/0xa0
>> [25244.894347] tcp_data_snd_check+0x2a/0xc8
>> [25244.895775] tcp_rcv_established+0x2a8/0x30d
>> [25244.897282] tcp_v4_do_rcv+0xb2/0x158
>> [25244.898666] tcp_v4_rcv+0x692/0x956
>> [25244.899959] ip_local_deliver_finish+0xeb/0x169
>> [25244.901547] __netif_receive_skb_core+0x51c/0x582
>> [25244.903193] ? inet_gro_receive+0x239/0x247
>> [25244.904756] netif_receive_skb_internal+0xab/0xc6
>> [25244.906395] napi_gro_receive+0x8a/0xc0
>> [25244.907760] receive_buf+0x9a1/0x9cd
>> [25244.909160] ? load_balance+0x17a/0x7b7
>> [25244.910536] ? vring_unmap_one+0x18/0x61
>> [25244.911932] ? detach_buf+0x60/0xfa
>> [25244.913234] virtnet_poll+0x128/0x1e1
>> [25244.914607] net_rx_action+0x12a/0x2b1
>> [25244.915953] __do_softirq+0x11c/0x26b
>> [25244.917269] ? handle_irq_event+0x44/0x56
>> [25244.918695] irq_exit+0x61/0xa0
>> [25244.919947] do_IRQ+0x9d/0xbb
>> [25244.921065] common_interrupt+0x85/0x85
>> [25244.922479] </IRQ>
>>
>> tcp_rtx_queue_tail() (called by tcp_fragment()) can call
>> tcp_write_queue_prev() on the first packet in the queue, which will
>> trigger
>> the BUG in tcp_write_queue_prev(), because there is no previous packet.
>>
>> This happens when the retransmit queue is empty, for example in case of a
>> zero window.
>>
>> Patch is needed for 4.4, 4.9 and 4.14 stable branches.
>
> There needs to be a better explanation of why it's not needed
> upstream...
Commit 8c3088f895a0 ("tcp: be more careful in tcp_fragment()") was not a
simple cherry-pick of the original one from master (b617158dc096)
because there is a specific TCP rtx queue only since v4.15. For more
details, please see the commit message of b617158dc096 ("tcp: be more
careful in tcp_fragment()").
The BUG() is hit due to the specific code added to versions older than
v4.15. The comment in skb_queue_prev() (include/linux/skbuff.h:1406),
just before the BUG_ON() somehow suggests to add a check before using
it, what Tim did.
In master, this code path causing the issue will not be taken because
the implementation of tcp_rtx_queue_tail() is different:
tcp_fragment() → tcp_rtx_queue_tail() → tcp_write_queue_prev() →
skb_queue_prev() → BUG_ON()
Because this patch is specific to versions older than the two last
stable ones but still linked to the network architecture, who can review
and approve it? :)
Cheers,
Matt
--
Matthieu Baerts | R&D Engineer
matthieu.baerts@tessares.net
Tessares SA | Hybrid Access Solutions
www.tessares.net
1 Avenue Jean Monnet, 1348 Louvain-la-Neuve, Belgium
^ permalink raw reply
* Re: [PATCH 0/4 net-next] flow_offload: update mangle action representation
From: Pablo Neira Ayuso @ 2019-08-31 14:22 UTC (permalink / raw)
To: Jakub Kicinski; +Cc: netfilter-devel, davem, netdev, vishal, saeedm, jiri
In-Reply-To: <20190830153351.5d5330fa@cakuba.netronome.com>
On Fri, Aug 30, 2019 at 03:33:51PM -0700, Jakub Kicinski wrote:
> On Fri, 30 Aug 2019 11:07:10 +0200, Pablo Neira Ayuso wrote:
> > > > * The front-end coalesces consecutive pedit actions into one single
> > > > word, so drivers can mangle IPv6 and ethernet address fields in one
> > > > single go.
> > >
> > > You still only coalesce up to 16 bytes, no?
> >
> > You only have to rise FLOW_ACTION_MANGLE_MAXLEN coming in this patch
> > if you need more. I don't know of any packet field larger than 16
> > bytes. If there is a use-case for this, it should be easy to rise that
> > definition.
>
> Please see the definitions of:
>
> struct nfp_fl_set_eth
> struct nfp_fl_set_ip4_addrs
> struct nfp_fl_set_ip4_ttl_tos
> struct nfp_fl_set_ipv6_tc_hl_fl
> struct nfp_fl_set_ipv6_addr
> struct nfp_fl_set_tport
>
> These are the programming primitives for header rewrites in the NFP.
> Since each of those contains more than just one field, we'll have to
> keep all the field coalescing logic in the driver, even if you coalesce
> while fields (i.e. IPv6 addresses).
nfp has been updated in this patch series to deal with the new mangle
representation.
> Perhaps it's not a serious blocker for the series, but it'd be nice if
> rewrite action grouping was handled in the core. Since you're already
> poking at that code..
Rewrite action grouping is already handled from the core front-end in
this patch series.
Thanks.
^ permalink raw reply
* Re: [PATCH v2 2/2] PTP: add support for one-shot output
From: Richard Cochran @ 2019-08-31 14:47 UTC (permalink / raw)
To: Felipe Balbi; +Cc: Christopher S Hall, netdev, linux-kernel, davem
In-Reply-To: <87r253ulpn.fsf@gmail.com>
On Fri, Aug 30, 2019 at 11:00:20AM +0300, Felipe Balbi wrote:
> >> @@ -177,9 +177,8 @@ long ptp_ioctl(struct posix_clock *pc, unsigned int cmd, unsigned long arg)
> >> err = -EFAULT;
> >> break;
> >> }
> >> - if ((req.perout.flags || req.perout.rsv[0] || req.perout.rsv[1]
> >> - || req.perout.rsv[2] || req.perout.rsv[3])
> >> - && cmd == PTP_PEROUT_REQUEST2) {
> >> + if ((req.perout.rsv[0] || req.perout.rsv[1] || req.perout.rsv[2]
> >> + || req.perout.rsv[3]) && cmd == PTP_PEROUT_REQUEST2) {
> >
> > Please check that the reserved bits of req.perout.flags, namely
> > ~PTP_PEROUT_ONE_SHOT, are clear.
>
> Actually, we should check more. PEROUT_FEATURE_ENABLE is still valid
> here, right? So are RISING and FALLING edges, no?
No. The ptp_extts_request.flags are indeed defined:
struct ptp_extts_request {
...
unsigned int flags; /* Bit field for PTP_xxx flags. */
...
};
But the ptp_perout_request.flags are reserved:
struct ptp_perout_request {
...
unsigned int flags; /* Reserved for future use. */
...
};
For this ioctl, the test for enable/disable is
ptp_perout_request.period is zero:
enable = req.perout.period.sec || req.perout.period.nsec;
err = ops->enable(ops, &req, enable);
The usage pattern here is taken from timer_settime(2).
Thanks,
Richard
^ permalink raw reply
* Re: [PATCH v2 2/2] PTP: add support for one-shot output
From: Richard Cochran @ 2019-08-31 15:01 UTC (permalink / raw)
To: Felipe Balbi; +Cc: Christopher S Hall, netdev, linux-kernel, davem
In-Reply-To: <87r253ulpn.fsf@gmail.com>
On Fri, Aug 30, 2019 at 11:00:20AM +0300, Felipe Balbi wrote:
> seems like this should be defined together with the other flags? If
> that's the case, it seems like we would EXTTS and PEROUT masks.
Yes, let's make the meanings of the bit fields clear...
--- ptp_clock.h ---
/*
* Bits of the ptp_extts_request.flags field:
*/
#define PTP_ENABLE_FEATURE BIT(0)
#define PTP_RISING_EDGE BIT(1)
#define PTP_FALLING_EDGE BIT(2)
#define PTP_EXTTS_VALID_FLAGS (PTP_ENABLE_FEATURE | \
PTP_RISING_EDGE | \
PTP_FALLING_EDGE)
/*
* Bits of the ptp_perout_request.flags field:
*/
#define PTP_PEROUT_ONE_SHOT BIT(0)
#define PTP_PEROUT_VALID_FLAGS (PTP_PEROUT_ONE_SHOT)
struct ptp_extts_request {
unsigned int flags; /* Bit field of PTP_EXTTS_VALID_FLAGS. */
};
struct ptp_perout_request {
unsigned int flags; /* Bit field of PTP_PEROUT_VALID_FLAGS. */
};
Thanks,
Richard
^ permalink raw reply
* Re: iproute2: tc: potential buffer overflow
From: Stephen Hemminger @ 2019-08-31 15:37 UTC (permalink / raw)
To: tomaspaukrt; +Cc: netdev
In-Reply-To: <8fo.ZWfD.3kvedbSyU2M.1TQd9t@seznam.cz>
On Sat, 31 Aug 2019 15:13:27 +0200 (CEST)
<tomaspaukrt@email.cz> wrote:
> Hi,
>
> there are two potentially dangerous calls of strcpy function in the program "tc". In the attachment is a patch that fixes this issue.
>
> Tomas
This looks correct.
Please fix with strlcpy() instead; that is clearer.
Plus you can use XT_EXTENSION_MAX_NAMELEN here (optional).
^ permalink raw reply
* Re: [PATCH v3 01/11] checkpatch: check for nested (un)?likely() calls
From: Denis Efremov @ 2019-08-31 15:54 UTC (permalink / raw)
To: Markus Elfring, Joe Perches
Cc: Andrew Morton, Anton Altaparmakov, Andy Whitcroft,
Boris Ostrovsky, Boris Pismenny, Darrick J. Wong, David S. Miller,
Dennis Dalessandro, Dmitry Torokhov, dri-devel,
Inaky Perez-Gonzalez, Jürgen Groß, Leon Romanovsky,
linux-arm-msm, linux-fsdevel, linux-input, linux-kernel,
linux-ntfs-dev, linux-rdma, linux-wimax, linux-xfs,
Mike Marciniszyn, netdev, Pali Rohár, Rob Clark,
Saeed Mahameed, Sean Paul, Alexander Viro, xen-devel,
Enrico Weigelt
In-Reply-To: <0d9345ed-f16a-de0b-6125-1f663765eb46@web.de>
On 31.08.2019 12:15, Markus Elfring wrote:
>> +# nested likely/unlikely calls
>> + if ($line =~ /\b(?:(?:un)?likely)\s*\(\s*!?\s*(IS_ERR(?:_OR_NULL|_VALUE)?|WARN)/) {
>> + WARN("LIKELY_MISUSE",
>
> How do you think about to use the specification “(?:IS_ERR(?:_(?:OR_NULL|VALUE))?|WARN)”
> in this regular expression?
Hmm,
(?: <- Catch group is required here, since it is used in diagnostic message,
see $1
IS_ERR
(?:_ <- Another atomic group just to show that '_' is a common prefix?
I'm not sure about this. Usually, Perl interpreter is very good at optimizing such things.
You could see this optimization if you run perl with -Mre=debug.
(?:OR_NULL|VALUE))?|WARN)
Regards,
Denis
^ permalink raw reply
* Re: [PATCH] net: dsa: Fix off-by-one number of calls to devlink_port_unregister
From: Vivien Didelot @ 2019-08-31 16:19 UTC (permalink / raw)
To: Vladimir Oltean; +Cc: f.fainelli, andrew, davem, netdev, Vladimir Oltean
In-Reply-To: <20190831124619.460-1-olteanv@gmail.com>
Hi Vladimir,
On Sat, 31 Aug 2019 15:46:19 +0300, Vladimir Oltean <olteanv@gmail.com> wrote:
> When a function such as dsa_slave_create fails, currently the following
> stack trace can be seen:
>
> [ 2.038342] sja1105 spi0.1: Probed switch chip: SJA1105T
> [ 2.054556] sja1105 spi0.1: Reset switch and programmed static config
> [ 2.063837] sja1105 spi0.1: Enabled switch tagging
> [ 2.068706] fsl-gianfar soc:ethernet@2d90000 eth2: error -19 setting up slave phy
> [ 2.076371] ------------[ cut here ]------------
> [ 2.080973] WARNING: CPU: 1 PID: 21 at net/core/devlink.c:6184 devlink_free+0x1b4/0x1c0
> [ 2.088954] Modules linked in:
> [ 2.092005] CPU: 1 PID: 21 Comm: kworker/1:1 Not tainted 5.3.0-rc6-01360-g41b52e38d2b6-dirty #1746
> [ 2.100912] Hardware name: Freescale LS1021A
> [ 2.105162] Workqueue: events deferred_probe_work_func
> [ 2.110287] [<c03133a4>] (unwind_backtrace) from [<c030d8cc>] (show_stack+0x10/0x14)
> [ 2.117992] [<c030d8cc>] (show_stack) from [<c10b08d8>] (dump_stack+0xb4/0xc8)
> [ 2.125180] [<c10b08d8>] (dump_stack) from [<c0349d04>] (__warn+0xe0/0xf8)
> [ 2.132018] [<c0349d04>] (__warn) from [<c0349e34>] (warn_slowpath_null+0x40/0x48)
> [ 2.139549] [<c0349e34>] (warn_slowpath_null) from [<c0f19d74>] (devlink_free+0x1b4/0x1c0)
> [ 2.147772] [<c0f19d74>] (devlink_free) from [<c1064fc0>] (dsa_switch_teardown+0x60/0x6c)
> [ 2.155907] [<c1064fc0>] (dsa_switch_teardown) from [<c1065950>] (dsa_register_switch+0x8e4/0xaa8)
> [ 2.164821] [<c1065950>] (dsa_register_switch) from [<c0ba7fe4>] (sja1105_probe+0x21c/0x2ec)
> [ 2.173216] [<c0ba7fe4>] (sja1105_probe) from [<c0b35948>] (spi_drv_probe+0x80/0xa4)
> [ 2.180920] [<c0b35948>] (spi_drv_probe) from [<c0a4c1cc>] (really_probe+0x108/0x400)
> [ 2.188711] [<c0a4c1cc>] (really_probe) from [<c0a4c694>] (driver_probe_device+0x78/0x1bc)
> [ 2.196933] [<c0a4c694>] (driver_probe_device) from [<c0a4a3dc>] (bus_for_each_drv+0x58/0xb8)
> [ 2.205414] [<c0a4a3dc>] (bus_for_each_drv) from [<c0a4c024>] (__device_attach+0xd0/0x168)
> [ 2.213637] [<c0a4c024>] (__device_attach) from [<c0a4b1d0>] (bus_probe_device+0x84/0x8c)
> [ 2.221772] [<c0a4b1d0>] (bus_probe_device) from [<c0a4b72c>] (deferred_probe_work_func+0x84/0xc4)
> [ 2.230686] [<c0a4b72c>] (deferred_probe_work_func) from [<c03650a4>] (process_one_work+0x218/0x510)
> [ 2.239772] [<c03650a4>] (process_one_work) from [<c03660d8>] (worker_thread+0x2a8/0x5c0)
> [ 2.247908] [<c03660d8>] (worker_thread) from [<c036b348>] (kthread+0x148/0x150)
> [ 2.255265] [<c036b348>] (kthread) from [<c03010e8>] (ret_from_fork+0x14/0x2c)
> [ 2.262444] Exception stack(0xea965fb0 to 0xea965ff8)
> [ 2.267466] 5fa0: 00000000 00000000 00000000 00000000
> [ 2.275598] 5fc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
> [ 2.283729] 5fe0: 00000000 00000000 00000000 00000000 00000013 00000000
> [ 2.290333] ---[ end trace ca5d506728a0581a ]---
>
> devlink_free is complaining right here:
>
> WARN_ON(!list_empty(&devlink->port_list));
>
> This happens because devlink_port_unregister is no longer done right
> away in dsa_port_setup when a DSA_PORT_TYPE_USER has failed.
> Vivien said about this change that:
>
> Also no need to call devlink_port_unregister from within dsa_port_setup
> as this step is inconditionally handled by dsa_port_teardown on error.
>
> which is not really true. The devlink_port_unregister function _is_
> being called unconditionally from within dsa_port_setup, but not for
Not from within dsa_port_setup, but from its caller dsa_tree_setup_switches.
> this port that just failed, just for the previous ones which were set
> up.
>
> ports_teardown:
> for (i = 0; i < port; i++)
> dsa_port_teardown(&ds->ports[i]);
>
> Initially I was tempted to fix this by extending the "for" loop to also
> cover the port that failed during setup. But this could have potentially
> unforeseen consequences unrelated to devlink_port or even other types of
> ports than user ports, which I can't really test for. For example, if
> for some reason devlink_port_register itself would fail, then
> unconditionally unregistering it in dsa_port_teardown would not be a
> smart idea. The list might go on.
>
> So just make dsa_port_setup undo the setup it had done upon failure, and
> let the for loop undo the work of setting up the previous ports, which
> are guaranteed to be brought up to a consistent state.
>
> Fixes: 955222ca5281 ("net: dsa: use a single switch statement for port setup")
> Signed-off-by: Vladimir Oltean <olteanv@gmail.com>
> ---
> net/dsa/dsa2.c | 39 +++++++++++++++++++++++++++++----------
> 1 file changed, 29 insertions(+), 10 deletions(-)
>
> diff --git a/net/dsa/dsa2.c b/net/dsa/dsa2.c
> index f8445fa73448..b501c90aabe4 100644
> --- a/net/dsa/dsa2.c
> +++ b/net/dsa/dsa2.c
> @@ -259,8 +259,11 @@ static int dsa_port_setup(struct dsa_port *dp)
> const unsigned char *id = (const unsigned char *)&dst->index;
> const unsigned char len = sizeof(dst->index);
> struct devlink_port *dlp = &dp->devlink_port;
> + bool dsa_port_link_registered = false;
> + bool devlink_port_registered = false;
> struct devlink *dl = ds->devlink;
> - int err;
> + bool dsa_port_enabled = false;
> + int err = 0;
>
> switch (dp->type) {
> case DSA_PORT_TYPE_UNUSED:
> @@ -272,15 +275,19 @@ static int dsa_port_setup(struct dsa_port *dp)
> dp->index, false, 0, id, len);
> err = devlink_port_register(dl, dlp, dp->index);
> if (err)
> - return err;
> + break;
> + devlink_port_registered = true;
>
> err = dsa_port_link_register_of(dp);
> if (err)
> - return err;
> + break;
> + dsa_port_link_registered = true;
>
> err = dsa_port_enable(dp, NULL);
> if (err)
> - return err;
> + break;
> + dsa_port_enabled = true;
> +
> break;
> case DSA_PORT_TYPE_DSA:
> memset(dlp, 0, sizeof(*dlp));
> @@ -288,15 +295,19 @@ static int dsa_port_setup(struct dsa_port *dp)
> dp->index, false, 0, id, len);
> err = devlink_port_register(dl, dlp, dp->index);
> if (err)
> - return err;
> + break;
> + devlink_port_registered = true;
>
> err = dsa_port_link_register_of(dp);
> if (err)
> - return err;
> + break;
> + dsa_port_link_registered = true;
>
> err = dsa_port_enable(dp, NULL);
> if (err)
> - return err;
> + break;
> + dsa_port_enabled = true;
> +
> break;
> case DSA_PORT_TYPE_USER:
> memset(dlp, 0, sizeof(*dlp));
> @@ -304,18 +315,26 @@ static int dsa_port_setup(struct dsa_port *dp)
> dp->index, false, 0, id, len);
> err = devlink_port_register(dl, dlp, dp->index);
> if (err)
> - return err;
> + break;
> + devlink_port_registered = true;
>
> dp->mac = of_get_mac_address(dp->dn);
> err = dsa_slave_create(dp);
> if (err)
> - return err;
> + break;
>
> devlink_port_type_eth_set(dlp, dp->slave);
> break;
> }
>
> - return 0;
> + if (err && dsa_port_enabled)
> + dsa_port_disable(dp);
> + if (err && dsa_port_link_registered)
> + dsa_port_link_unregister_of(dp);
> + if (err && devlink_port_registered)
> + devlink_port_unregister(dlp);
> +
> + return err;
> }
No no, I'm pretty sure you can tell this is going to be a nightmare to
maintain these boolean states for all port types ;-)
And this is not a proper fix for the problem you've spotted. The problem
you've spotted is that devlink_port_unregister isn't called for the current
port if its setup failed, because dsa_port_teardown -- which is supposed to
be called unconditionally on error -- isn't called for the current port. Your
first attempt was correct, simply fix the loop in dsa_tree_setup_switches
to include the current port:
ports_teardown:
- for (i = 0; i < port; i++)
+ for (i = 0; i <= port; i++)
As for devlink_port_unregister, most kernel APIs unregistering objects are
self protected, so I'm tempted to propose the following patch for devlink:
diff --git a/net/core/devlink.c b/net/core/devlink.c
index 650f36379203..ab95607800d6 100644
--- a/net/core/devlink.c
+++ b/net/core/devlink.c
@@ -6264,6 +6264,8 @@ void devlink_port_unregister(struct devlink_port *devlink_port)
{
struct devlink *devlink = devlink_port->devlink;
+ if (!devlink_port->registered)
+ return;
devlink_port_type_warn_cancel(devlink_port);
devlink_port_notify(devlink_port, DEVLINK_CMD_PORT_DEL);
mutex_lock(&devlink->lock);
Otherwise we can protect the devlink port unregistering ourselves with:
if (dlp->registered)
devlink_port_unregister(dlp);
BTW that is the subtlety between "unregister" which considers that the object
_may_ have been registered, and "deregister" which assumes the object _was_
registered. Would you like to go ahead and propose the devlink patch?
Thanks for pointing this out,
Vivien
^ permalink raw reply
* Re: [PATCH net] rxrpc: Fix lack of conn cleanup when local endpoint is cleaned up [ver #2]
From: David Howells @ 2019-08-31 16:45 UTC (permalink / raw)
To: Hillf Danton; +Cc: dhowells, netdev, marc.dionne, linux-afs, linux-kernel
In-Reply-To: <20190831135906.6028-1-hdanton@sina.com>
Hillf Danton <hdanton@sina.com> wrote:
> > - if (rxnet->live) {
> > + if (rxnet->live && !conn->params.local->dead) {
> > idle_timestamp = READ_ONCE(conn->idle_timestamp);
> > expire_at = idle_timestamp + rxrpc_connection_expiry * HZ;
> > if (conn->params.local->service_closed)
>
> Is there any chance out there that this reaper starts running one minute
> after the dead local went into graveyard?
It's certainly possible that that can happen. The reaper is per
network-namespace.
conn->params.local holds a ref on the local endpoint.
It may be worth wrapping the "local->dead = true;" in rxrpc_local_destroyer()
in rxnet->conn_lock.
David
^ permalink raw reply
* Re: [PATCH v3 01/11] checkpatch: check for nested (un)?likely() calls
From: Markus Elfring @ 2019-08-31 16:45 UTC (permalink / raw)
To: Denis Efremov, Joe Perches
Cc: Andrew Morton, Anton Altaparmakov, Andy Whitcroft,
Boris Ostrovsky, Boris Pismenny, Darrick J. Wong, David S. Miller,
Dennis Dalessandro, Dmitry Torokhov, dri-devel,
Inaky Perez-Gonzalez, Jürgen Groß, Leon Romanovsky,
linux-arm-msm, linux-fsdevel, linux-input, linux-kernel,
linux-ntfs-dev, linux-rdma, linux-wimax, linux-xfs,
Mike Marciniszyn, netdev, Pali Rohár, Rob Clark,
Saeed Mahameed, Sean Paul, Alexander Viro, xen-devel,
Enrico Weigelt
In-Reply-To: <689c8baf-2298-f086-3461-5cd1cdd191c6@linux.com>
>>> +# nested likely/unlikely calls
>>> + if ($line =~ /\b(?:(?:un)?likely)\s*\(\s*!?\s*(IS_ERR(?:_OR_NULL|_VALUE)?|WARN)/) {
>>> + WARN("LIKELY_MISUSE",
>>
>> How do you think about to use the specification “(?:IS_ERR(?:_(?:OR_NULL|VALUE))?|WARN)”
>> in this regular expression?
…
> IS_ERR
> (?:_ <- Another atomic group just to show that '_' is a common prefix?
Yes. - I hope that this specification detail can help a bit.
> Usually, Perl interpreter is very good at optimizing such things.
Would you like to help this software component by omitting a pair of
non-capturing parentheses at the beginning?
\b(?:un)?likely\s*
Regards,
Markus
^ permalink raw reply
* Re: [PATCH] net: dsa: Fix off-by-one number of calls to devlink_port_unregister
From: Vladimir Oltean @ 2019-08-31 16:54 UTC (permalink / raw)
To: Vivien Didelot; +Cc: Florian Fainelli, Andrew Lunn, David S. Miller, netdev
In-Reply-To: <20190831121958.GC12031@t480s.localdomain>
Hi Vivien,
On Sat, 31 Aug 2019 at 19:20, Vivien Didelot <vivien.didelot@gmail.com> wrote:
>
> Hi Vladimir,
>
> On Sat, 31 Aug 2019 15:46:19 +0300, Vladimir Oltean <olteanv@gmail.com> wrote:
> > When a function such as dsa_slave_create fails, currently the following
> > stack trace can be seen:
> >
> > [ 2.038342] sja1105 spi0.1: Probed switch chip: SJA1105T
> > [ 2.054556] sja1105 spi0.1: Reset switch and programmed static config
> > [ 2.063837] sja1105 spi0.1: Enabled switch tagging
> > [ 2.068706] fsl-gianfar soc:ethernet@2d90000 eth2: error -19 setting up slave phy
> > [ 2.076371] ------------[ cut here ]------------
> > [ 2.080973] WARNING: CPU: 1 PID: 21 at net/core/devlink.c:6184 devlink_free+0x1b4/0x1c0
> > [ 2.088954] Modules linked in:
> > [ 2.092005] CPU: 1 PID: 21 Comm: kworker/1:1 Not tainted 5.3.0-rc6-01360-g41b52e38d2b6-dirty #1746
> > [ 2.100912] Hardware name: Freescale LS1021A
> > [ 2.105162] Workqueue: events deferred_probe_work_func
> > [ 2.110287] [<c03133a4>] (unwind_backtrace) from [<c030d8cc>] (show_stack+0x10/0x14)
> > [ 2.117992] [<c030d8cc>] (show_stack) from [<c10b08d8>] (dump_stack+0xb4/0xc8)
> > [ 2.125180] [<c10b08d8>] (dump_stack) from [<c0349d04>] (__warn+0xe0/0xf8)
> > [ 2.132018] [<c0349d04>] (__warn) from [<c0349e34>] (warn_slowpath_null+0x40/0x48)
> > [ 2.139549] [<c0349e34>] (warn_slowpath_null) from [<c0f19d74>] (devlink_free+0x1b4/0x1c0)
> > [ 2.147772] [<c0f19d74>] (devlink_free) from [<c1064fc0>] (dsa_switch_teardown+0x60/0x6c)
> > [ 2.155907] [<c1064fc0>] (dsa_switch_teardown) from [<c1065950>] (dsa_register_switch+0x8e4/0xaa8)
> > [ 2.164821] [<c1065950>] (dsa_register_switch) from [<c0ba7fe4>] (sja1105_probe+0x21c/0x2ec)
> > [ 2.173216] [<c0ba7fe4>] (sja1105_probe) from [<c0b35948>] (spi_drv_probe+0x80/0xa4)
> > [ 2.180920] [<c0b35948>] (spi_drv_probe) from [<c0a4c1cc>] (really_probe+0x108/0x400)
> > [ 2.188711] [<c0a4c1cc>] (really_probe) from [<c0a4c694>] (driver_probe_device+0x78/0x1bc)
> > [ 2.196933] [<c0a4c694>] (driver_probe_device) from [<c0a4a3dc>] (bus_for_each_drv+0x58/0xb8)
> > [ 2.205414] [<c0a4a3dc>] (bus_for_each_drv) from [<c0a4c024>] (__device_attach+0xd0/0x168)
> > [ 2.213637] [<c0a4c024>] (__device_attach) from [<c0a4b1d0>] (bus_probe_device+0x84/0x8c)
> > [ 2.221772] [<c0a4b1d0>] (bus_probe_device) from [<c0a4b72c>] (deferred_probe_work_func+0x84/0xc4)
> > [ 2.230686] [<c0a4b72c>] (deferred_probe_work_func) from [<c03650a4>] (process_one_work+0x218/0x510)
> > [ 2.239772] [<c03650a4>] (process_one_work) from [<c03660d8>] (worker_thread+0x2a8/0x5c0)
> > [ 2.247908] [<c03660d8>] (worker_thread) from [<c036b348>] (kthread+0x148/0x150)
> > [ 2.255265] [<c036b348>] (kthread) from [<c03010e8>] (ret_from_fork+0x14/0x2c)
> > [ 2.262444] Exception stack(0xea965fb0 to 0xea965ff8)
> > [ 2.267466] 5fa0: 00000000 00000000 00000000 00000000
> > [ 2.275598] 5fc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
> > [ 2.283729] 5fe0: 00000000 00000000 00000000 00000000 00000013 00000000
> > [ 2.290333] ---[ end trace ca5d506728a0581a ]---
> >
> > devlink_free is complaining right here:
> >
> > WARN_ON(!list_empty(&devlink->port_list));
> >
> > This happens because devlink_port_unregister is no longer done right
> > away in dsa_port_setup when a DSA_PORT_TYPE_USER has failed.
> > Vivien said about this change that:
> >
> > Also no need to call devlink_port_unregister from within dsa_port_setup
> > as this step is inconditionally handled by dsa_port_teardown on error.
> >
> > which is not really true. The devlink_port_unregister function _is_
> > being called unconditionally from within dsa_port_setup, but not for
>
> Not from within dsa_port_setup, but from its caller dsa_tree_setup_switches.
>
> > this port that just failed, just for the previous ones which were set
> > up.
> >
> > ports_teardown:
> > for (i = 0; i < port; i++)
> > dsa_port_teardown(&ds->ports[i]);
> >
> > Initially I was tempted to fix this by extending the "for" loop to also
> > cover the port that failed during setup. But this could have potentially
> > unforeseen consequences unrelated to devlink_port or even other types of
> > ports than user ports, which I can't really test for. For example, if
> > for some reason devlink_port_register itself would fail, then
> > unconditionally unregistering it in dsa_port_teardown would not be a
> > smart idea. The list might go on.
> >
> > So just make dsa_port_setup undo the setup it had done upon failure, and
> > let the for loop undo the work of setting up the previous ports, which
> > are guaranteed to be brought up to a consistent state.
> >
> > Fixes: 955222ca5281 ("net: dsa: use a single switch statement for port setup")
> > Signed-off-by: Vladimir Oltean <olteanv@gmail.com>
> > ---
> > net/dsa/dsa2.c | 39 +++++++++++++++++++++++++++++----------
> > 1 file changed, 29 insertions(+), 10 deletions(-)
> >
> > diff --git a/net/dsa/dsa2.c b/net/dsa/dsa2.c
> > index f8445fa73448..b501c90aabe4 100644
> > --- a/net/dsa/dsa2.c
> > +++ b/net/dsa/dsa2.c
> > @@ -259,8 +259,11 @@ static int dsa_port_setup(struct dsa_port *dp)
> > const unsigned char *id = (const unsigned char *)&dst->index;
> > const unsigned char len = sizeof(dst->index);
> > struct devlink_port *dlp = &dp->devlink_port;
> > + bool dsa_port_link_registered = false;
> > + bool devlink_port_registered = false;
> > struct devlink *dl = ds->devlink;
> > - int err;
> > + bool dsa_port_enabled = false;
> > + int err = 0;
> >
> > switch (dp->type) {
> > case DSA_PORT_TYPE_UNUSED:
> > @@ -272,15 +275,19 @@ static int dsa_port_setup(struct dsa_port *dp)
> > dp->index, false, 0, id, len);
> > err = devlink_port_register(dl, dlp, dp->index);
> > if (err)
> > - return err;
> > + break;
> > + devlink_port_registered = true;
> >
> > err = dsa_port_link_register_of(dp);
> > if (err)
> > - return err;
> > + break;
> > + dsa_port_link_registered = true;
> >
> > err = dsa_port_enable(dp, NULL);
> > if (err)
> > - return err;
> > + break;
> > + dsa_port_enabled = true;
> > +
> > break;
> > case DSA_PORT_TYPE_DSA:
> > memset(dlp, 0, sizeof(*dlp));
> > @@ -288,15 +295,19 @@ static int dsa_port_setup(struct dsa_port *dp)
> > dp->index, false, 0, id, len);
> > err = devlink_port_register(dl, dlp, dp->index);
> > if (err)
> > - return err;
> > + break;
> > + devlink_port_registered = true;
> >
> > err = dsa_port_link_register_of(dp);
> > if (err)
> > - return err;
> > + break;
> > + dsa_port_link_registered = true;
> >
> > err = dsa_port_enable(dp, NULL);
> > if (err)
> > - return err;
> > + break;
> > + dsa_port_enabled = true;
> > +
> > break;
> > case DSA_PORT_TYPE_USER:
> > memset(dlp, 0, sizeof(*dlp));
> > @@ -304,18 +315,26 @@ static int dsa_port_setup(struct dsa_port *dp)
> > dp->index, false, 0, id, len);
> > err = devlink_port_register(dl, dlp, dp->index);
> > if (err)
> > - return err;
> > + break;
> > + devlink_port_registered = true;
> >
> > dp->mac = of_get_mac_address(dp->dn);
> > err = dsa_slave_create(dp);
> > if (err)
> > - return err;
> > + break;
> >
> > devlink_port_type_eth_set(dlp, dp->slave);
> > break;
> > }
> >
> > - return 0;
> > + if (err && dsa_port_enabled)
> > + dsa_port_disable(dp);
> > + if (err && dsa_port_link_registered)
> > + dsa_port_link_unregister_of(dp);
> > + if (err && devlink_port_registered)
> > + devlink_port_unregister(dlp);
> > +
> > + return err;
> > }
>
> No no, I'm pretty sure you can tell this is going to be a nightmare to
> maintain these boolean states for all port types ;-)
>
> And this is not a proper fix for the problem you've spotted. The problem
> you've spotted is that devlink_port_unregister isn't called for the current
> port if its setup failed, because dsa_port_teardown -- which is supposed to
> be called unconditionally on error -- isn't called for the current port. Your
> first attempt was correct, simply fix the loop in dsa_tree_setup_switches
> to include the current port:
>
Fine, I had not noticed the "registered" field from devlink_port.
But I fail to see how dsa_port_teardown can be entered in the generic
case from whatever failure state dsa_port_setup left it in. What if
it's a DSA_PORT_TYPE_CPU whose devlink_port_register failed. What will
happen to the PHYLINK instance behind dsa_port_link_register_of (not
to mention about data that the driver might be allocating in
dsa_port_enable and expecting a matching disable so it won't leak)?
And that doesn't mean the fix isn't "proper". It may be "supposed" to
be called unconditionally on error, but right now it isn't, so I doubt
anybody has tested that, and that there aren't corner cases. Just
playing the safe side.
>
> ports_teardown:
> - for (i = 0; i < port; i++)
> + for (i = 0; i <= port; i++)
>
>
> As for devlink_port_unregister, most kernel APIs unregistering objects are
> self protected, so I'm tempted to propose the following patch for devlink:
>
>
> diff --git a/net/core/devlink.c b/net/core/devlink.c
> index 650f36379203..ab95607800d6 100644
> --- a/net/core/devlink.c
> +++ b/net/core/devlink.c
> @@ -6264,6 +6264,8 @@ void devlink_port_unregister(struct devlink_port *devlink_port)
> {
> struct devlink *devlink = devlink_port->devlink;
>
> + if (!devlink_port->registered)
> + return;
> devlink_port_type_warn_cancel(devlink_port);
> devlink_port_notify(devlink_port, DEVLINK_CMD_PORT_DEL);
> mutex_lock(&devlink->lock);
>
>
> Otherwise we can protect the devlink port unregistering ourselves with:
>
>
> if (dlp->registered)
> devlink_port_unregister(dlp);
>
>
> BTW that is the subtlety between "unregister" which considers that the object
> _may_ have been registered, and "deregister" which assumes the object _was_
That concept is not familiar to me. Actually I grepped the DSA API for
"unregister" and found:
static void dsa_tag_driver_unregister(struct dsa_tag_driver *dsa_tag_driver)
{
mutex_lock(&dsa_tag_drivers_lock);
list_del(&dsa_tag_driver->list);
mutex_unlock(&dsa_tag_drivers_lock);
}
which looks pretty unconditional to me?
> registered. Would you like to go ahead and propose the devlink patch?
>
Nope, I don't really know what I'm getting myself into :) If you want
to send it, I will consider it during v2.
>
> Thanks for pointing this out,
>
> Vivien
Regards,
-Vladimir
^ permalink raw reply
* Re: [RFC PATCH v2 net-next 00/15] tc-taprio offload for SJA1105 DSA
From: Vladimir Oltean @ 2019-08-31 17:03 UTC (permalink / raw)
To: Jakub Kicinski
Cc: Florian Fainelli, Vivien Didelot, Andrew Lunn, David S. Miller,
Vinicius Costa Gomes, vedang.patel, Richard Cochran, weifeng.voon,
jiri, m-karicheri2, Jose.Abreu, Ilias Apalodimas,
Jamal Hadi Salim, xiyou.wangcong, netdev
In-Reply-To: <20190830152839.0fe34d25@cakuba.netronome.com>
On Sat, 31 Aug 2019 at 01:29, Jakub Kicinski
<jakub.kicinski@netronome.com> wrote:
>
> On Fri, 30 Aug 2019 13:11:11 +0300, Vladimir Oltean wrote:
> > On Fri, 30 Aug 2019 at 04:21, Jakub Kicinski wrote:
> > > On Fri, 30 Aug 2019 03:46:20 +0300, Vladimir Oltean wrote:
> > > > - Configuring the switch over SPI cannot apparently be done from this
> > > > ndo_setup_tc callback because it runs in atomic context. I also have
> > > > some downstream patches to offload tc clsact matchall with mirred
> > > > action, but in that case it looks like the atomic context restriction
> > > > does not apply.
> > >
> > > This sounds really surprising ndo_setup_tc should always be allowed to
> > > sleep. Can the taprio limitation be lifted somehow?
> >
> > I need to get more familiar with the taprio internal data structures.
> > I think you're suggesting to get those updated to a consistent state
> > while under spin_lock_bh(qdisc_lock(sch)), then call ndo_setup_tc from
> > outside that critical section?
>
> I'm not 100% sure how taprio handles locking TBH, it just seems naive
> that HW callback will not need to sleep, so the kernel should make sure
> that callback can sleep. Otherwise we'll end up with 3/4 of drivers
> implementing some async work routine...
>
> Sorry, I know that's quite general and not that helpful.
>
> > Also, I just noticed that I introduced a bug in taprio_disable_offload
> > with my reference counting addition. The qdisc can't just pass stack
> > memory to the driver, now that it's allowing it to keep it. So
> > definitely the patch needs more refactoring.
>
> Ah, a slight deja vu, I think someone else has done it in the past :)
Ok, I think I managed to move the ndo_setup_tc callback outside of
atomic context and nothing broke so far... Any other comments or
should I go ahead and send a first proper patchset?
Thanks,
-Vladimir
^ permalink raw reply
* Re: [PATCH v3 01/11] checkpatch: check for nested (un)?likely() calls
From: Denis Efremov @ 2019-08-31 17:07 UTC (permalink / raw)
To: Markus Elfring, Joe Perches
Cc: Andrew Morton, Anton Altaparmakov, Andy Whitcroft,
Boris Ostrovsky, Boris Pismenny, Darrick J. Wong, David S. Miller,
Dennis Dalessandro, Dmitry Torokhov, dri-devel,
Inaky Perez-Gonzalez, Jürgen Groß, Leon Romanovsky,
linux-arm-msm, linux-fsdevel, linux-input, linux-kernel,
linux-ntfs-dev, linux-rdma, linux-wimax, linux-xfs,
Mike Marciniszyn, netdev, Pali Rohár, Rob Clark,
Saeed Mahameed, Sean Paul, Alexander Viro, xen-devel,
Enrico Weigelt
In-Reply-To: <493a7377-2de9-1d44-cd8f-c658793d15db@web.de>
On 31.08.2019 19:45, Markus Elfring wrote:
>>>> +# nested likely/unlikely calls
>>>> + if ($line =~ /\b(?:(?:un)?likely)\s*\(\s*!?\s*(IS_ERR(?:_OR_NULL|_VALUE)?|WARN)/) {
>>>> + WARN("LIKELY_MISUSE",
>>>
>>> How do you think about to use the specification “(?:IS_ERR(?:_(?:OR_NULL|VALUE))?|WARN)”
>>> in this regular expression?
> …
>> IS_ERR
>> (?:_ <- Another atomic group just to show that '_' is a common prefix?
>
> Yes. - I hope that this specification detail can help a bit.
I'm not sure that another pair of brackets for a single char worth it.
>> Usually, Perl interpreter is very good at optimizing such things.
The interpreter optimizes it internally:
echo 'IS_ERR_OR_NULL' | perl -Mre=debug -ne '/IS_ERR(?:_OR_NULL|_VALUE)?/ && print'
Compiling REx "IS_ERR(?:_OR_NULL|_VALUE)?"
Final program:
1: EXACT <IS_ERR> (4)
4: CURLYX[0]{0,1} (16)
6: EXACT <_> (8) <-- common prefix
8: TRIE-EXACT[OV] (15)
<OR_NULL>
<VALUE>
...
>
> Would you like to help this software component by omitting a pair of
> non-capturing parentheses at the beginning?
>
> \b(?:un)?likely\s*
This pair of brackets is required to match "unlikely" and it's
optional in order to match "likely".
Regards,
Denis
^ permalink raw reply
* Re: [PATCH] net: dsa: Fix off-by-one number of calls to devlink_port_unregister
From: Vivien Didelot @ 2019-08-31 17:14 UTC (permalink / raw)
To: Vladimir Oltean; +Cc: Florian Fainelli, Andrew Lunn, David S. Miller, netdev
In-Reply-To: <CA+h21hoKcg3UUNkYRyEw8FS0q_vxdmoQL90BaOuKoW074DYYow@mail.gmail.com>
Hi Vladimir,
On Sat, 31 Aug 2019 19:54:40 +0300, Vladimir Oltean <olteanv@gmail.com> wrote:
> Fine, I had not noticed the "registered" field from devlink_port.
> But I fail to see how dsa_port_teardown can be entered in the generic
> case from whatever failure state dsa_port_setup left it in. What if
> it's a DSA_PORT_TYPE_CPU whose devlink_port_register failed. What will
> happen to the PHYLINK instance behind dsa_port_link_register_of (not
> to mention about data that the driver might be allocating in
> dsa_port_enable and expecting a matching disable so it won't leak)?
> And that doesn't mean the fix isn't "proper". It may be "supposed" to
> be called unconditionally on error, but right now it isn't, so I doubt
> anybody has tested that, and that there aren't corner cases. Just
> playing the safe side.
You are correct, while I think PHYLINK handles disconnecting properly,
I'm not sure dsa_port_disable is ready yet. Your proposed fix is a
safer solution in the meantime.
> > BTW that is the subtlety between "unregister" which considers that the object
> > _may_ have been registered, and "deregister" which assumes the object _was_
>
> That concept is not familiar to me. Actually I grepped the DSA API for
> "unregister" and found:
That is not a kernel concept, I was simply pointing out the definitions
from the English language, as I found this interesting. Unfortunately
this isn't honored everywhere in the code, as you've noticed ;-)
> > registered. Would you like to go ahead and propose the devlink patch?
>
> Nope, I don't really know what I'm getting myself into :) If you want
> to send it, I will consider it during v2.
Sure, I'll propose it myself.
Thanks,
Vivien
^ permalink raw reply
* Re: [PATCH] net: dsa: Fix off-by-one number of calls to devlink_port_unregister
From: Vivien Didelot @ 2019-08-31 17:17 UTC (permalink / raw)
To: Vladimir Oltean; +Cc: f.fainelli, andrew, davem, netdev, Vladimir Oltean
In-Reply-To: <20190831124619.460-1-olteanv@gmail.com>
On Sat, 31 Aug 2019 15:46:19 +0300, Vladimir Oltean <olteanv@gmail.com> wrote:
> When a function such as dsa_slave_create fails, currently the following
> stack trace can be seen:
>
> [ 2.038342] sja1105 spi0.1: Probed switch chip: SJA1105T
> [ 2.054556] sja1105 spi0.1: Reset switch and programmed static config
> [ 2.063837] sja1105 spi0.1: Enabled switch tagging
> [ 2.068706] fsl-gianfar soc:ethernet@2d90000 eth2: error -19 setting up slave phy
> [ 2.076371] ------------[ cut here ]------------
> [ 2.080973] WARNING: CPU: 1 PID: 21 at net/core/devlink.c:6184 devlink_free+0x1b4/0x1c0
> [ 2.088954] Modules linked in:
> [ 2.092005] CPU: 1 PID: 21 Comm: kworker/1:1 Not tainted 5.3.0-rc6-01360-g41b52e38d2b6-dirty #1746
> [ 2.100912] Hardware name: Freescale LS1021A
> [ 2.105162] Workqueue: events deferred_probe_work_func
> [ 2.110287] [<c03133a4>] (unwind_backtrace) from [<c030d8cc>] (show_stack+0x10/0x14)
> [ 2.117992] [<c030d8cc>] (show_stack) from [<c10b08d8>] (dump_stack+0xb4/0xc8)
> [ 2.125180] [<c10b08d8>] (dump_stack) from [<c0349d04>] (__warn+0xe0/0xf8)
> [ 2.132018] [<c0349d04>] (__warn) from [<c0349e34>] (warn_slowpath_null+0x40/0x48)
> [ 2.139549] [<c0349e34>] (warn_slowpath_null) from [<c0f19d74>] (devlink_free+0x1b4/0x1c0)
> [ 2.147772] [<c0f19d74>] (devlink_free) from [<c1064fc0>] (dsa_switch_teardown+0x60/0x6c)
> [ 2.155907] [<c1064fc0>] (dsa_switch_teardown) from [<c1065950>] (dsa_register_switch+0x8e4/0xaa8)
> [ 2.164821] [<c1065950>] (dsa_register_switch) from [<c0ba7fe4>] (sja1105_probe+0x21c/0x2ec)
> [ 2.173216] [<c0ba7fe4>] (sja1105_probe) from [<c0b35948>] (spi_drv_probe+0x80/0xa4)
> [ 2.180920] [<c0b35948>] (spi_drv_probe) from [<c0a4c1cc>] (really_probe+0x108/0x400)
> [ 2.188711] [<c0a4c1cc>] (really_probe) from [<c0a4c694>] (driver_probe_device+0x78/0x1bc)
> [ 2.196933] [<c0a4c694>] (driver_probe_device) from [<c0a4a3dc>] (bus_for_each_drv+0x58/0xb8)
> [ 2.205414] [<c0a4a3dc>] (bus_for_each_drv) from [<c0a4c024>] (__device_attach+0xd0/0x168)
> [ 2.213637] [<c0a4c024>] (__device_attach) from [<c0a4b1d0>] (bus_probe_device+0x84/0x8c)
> [ 2.221772] [<c0a4b1d0>] (bus_probe_device) from [<c0a4b72c>] (deferred_probe_work_func+0x84/0xc4)
> [ 2.230686] [<c0a4b72c>] (deferred_probe_work_func) from [<c03650a4>] (process_one_work+0x218/0x510)
> [ 2.239772] [<c03650a4>] (process_one_work) from [<c03660d8>] (worker_thread+0x2a8/0x5c0)
> [ 2.247908] [<c03660d8>] (worker_thread) from [<c036b348>] (kthread+0x148/0x150)
> [ 2.255265] [<c036b348>] (kthread) from [<c03010e8>] (ret_from_fork+0x14/0x2c)
> [ 2.262444] Exception stack(0xea965fb0 to 0xea965ff8)
> [ 2.267466] 5fa0: 00000000 00000000 00000000 00000000
> [ 2.275598] 5fc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
> [ 2.283729] 5fe0: 00000000 00000000 00000000 00000000 00000013 00000000
> [ 2.290333] ---[ end trace ca5d506728a0581a ]---
>
> devlink_free is complaining right here:
>
> WARN_ON(!list_empty(&devlink->port_list));
>
> This happens because devlink_port_unregister is no longer done right
> away in dsa_port_setup when a DSA_PORT_TYPE_USER has failed.
> Vivien said about this change that:
>
> Also no need to call devlink_port_unregister from within dsa_port_setup
> as this step is inconditionally handled by dsa_port_teardown on error.
>
> which is not really true. The devlink_port_unregister function _is_
> being called unconditionally from within dsa_port_setup, but not for
> this port that just failed, just for the previous ones which were set
> up.
>
> ports_teardown:
> for (i = 0; i < port; i++)
> dsa_port_teardown(&ds->ports[i]);
>
> Initially I was tempted to fix this by extending the "for" loop to also
> cover the port that failed during setup. But this could have potentially
> unforeseen consequences unrelated to devlink_port or even other types of
> ports than user ports, which I can't really test for. For example, if
> for some reason devlink_port_register itself would fail, then
> unconditionally unregistering it in dsa_port_teardown would not be a
> smart idea. The list might go on.
>
> So just make dsa_port_setup undo the setup it had done upon failure, and
> let the for loop undo the work of setting up the previous ports, which
> are guaranteed to be brought up to a consistent state.
>
> Fixes: 955222ca5281 ("net: dsa: use a single switch statement for port setup")
> Signed-off-by: Vladimir Oltean <olteanv@gmail.com>
Reviewed-by: Vivien Didelot <vivien.didelot@gmail.com>
This belongs to net-next. Thanks,
Vivien
^ permalink raw reply
* Re: [PATCH v3 01/11] checkpatch: check for nested (un)?likely() calls
From: Markus Elfring @ 2019-08-31 17:26 UTC (permalink / raw)
To: Denis Efremov, Joe Perches
Cc: Andrew Morton, Anton Altaparmakov, Andy Whitcroft,
Boris Ostrovsky, Boris Pismenny, Darrick J. Wong, David S. Miller,
Dennis Dalessandro, Dmitry Torokhov, dri-devel,
Inaky Perez-Gonzalez, Jürgen Groß, Leon Romanovsky,
linux-arm-msm, linux-fsdevel, linux-input, linux-kernel,
linux-ntfs-dev, linux-rdma, linux-wimax, linux-xfs,
Mike Marciniszyn, netdev, Pali Rohár, Rob Clark,
Saeed Mahameed, Sean Paul, Alexander Viro, xen-devel,
Enrico Weigelt
In-Reply-To: <c5e4479d-2fb3-b5a5-00c3-b06e5177d869@linux.com>
>>>>> +# nested likely/unlikely calls
>>>>> + if ($line =~ /\b(?:(?:un)?likely)\s*\(\s*!?\s*(IS_ERR(?:_OR_NULL|_VALUE)?|WARN)/) {
>>>>> + WARN("LIKELY_MISUSE",
…
>> \b(?:un)?likely\s*
>
> This pair of brackets is required to match "unlikely"
> and it's optional in order to match "likely".
I agree also to this view if you refer to the shortened regular expression here.
But I got an other development opinion for an extra pair of non-capturing parentheses
at the front (from the version which you suggested).
Regards,
Markus
^ permalink raw reply
* Re: [PATCH 1/2] Fix a NULL-ptr-deref bug in ath6kl_usb_alloc_urb_from_pipe
From: Guenter Roeck @ 2019-08-31 18:02 UTC (permalink / raw)
To: Hui Peng
Cc: kvalo, davem, Mathias Payer, linux-wireless, netdev, linux-kernel
In-Reply-To: <20190804002905.11292-1-benquike@gmail.com>
On Sat, Aug 03, 2019 at 08:29:04PM -0400, Hui Peng wrote:
> The `ar_usb` field of `ath6kl_usb_pipe_usb_pipe` objects
> are initialized to point to the containing `ath6kl_usb` object
> according to endpoint descriptors read from the device side, as shown
> below in `ath6kl_usb_setup_pipe_resources`:
>
> for (i = 0; i < iface_desc->desc.bNumEndpoints; ++i) {
> endpoint = &iface_desc->endpoint[i].desc;
>
> // get the address from endpoint descriptor
> pipe_num = ath6kl_usb_get_logical_pipe_num(ar_usb,
> endpoint->bEndpointAddress,
> &urbcount);
> ......
> // select the pipe object
> pipe = &ar_usb->pipes[pipe_num];
>
> // initialize the ar_usb field
> pipe->ar_usb = ar_usb;
> }
>
> The driver assumes that the addresses reported in endpoint
> descriptors from device side to be complete. If a device is
> malicious and does not report complete addresses, it may trigger
> NULL-ptr-deref `ath6kl_usb_alloc_urb_from_pipe` and
> `ath6kl_usb_free_urb_to_pipe`.
>
> This patch fixes the bug by preventing potential NULL-ptr-deref.
>
> Signed-off-by: Hui Peng <benquike@gmail.com>
> Reported-by: Hui Peng <benquike@gmail.com>
> Reported-by: Mathias Payer <mathias.payer@nebelwelt.net>
I don't see this patch in the upstream kernel or in -next.
At the same time, it is supposed to fix CVE-2019-15098, which has
a CVSS v2.0 score of 7.8 (high).
Is this patch going to be applied to the upstream kernel ? If not,
are there reasons to believe that the vulnerability is not as severe
as its CVSS score indicates ?
Thanks,
Guenter
> ---
> drivers/net/wireless/ath/ath6kl/usb.c | 8 ++++++++
> 1 file changed, 8 insertions(+)
>
> diff --git a/drivers/net/wireless/ath/ath6kl/usb.c b/drivers/net/wireless/ath/ath6kl/usb.c
> index 4defb7a0330f..53b66e9434c9 100644
> --- a/drivers/net/wireless/ath/ath6kl/usb.c
> +++ b/drivers/net/wireless/ath/ath6kl/usb.c
> @@ -132,6 +132,10 @@ ath6kl_usb_alloc_urb_from_pipe(struct ath6kl_usb_pipe *pipe)
> struct ath6kl_urb_context *urb_context = NULL;
> unsigned long flags;
>
> + /* bail if this pipe is not initialized */
> + if (!pipe->ar_usb)
> + return NULL;
> +
> spin_lock_irqsave(&pipe->ar_usb->cs_lock, flags);
> if (!list_empty(&pipe->urb_list_head)) {
> urb_context =
> @@ -150,6 +154,10 @@ static void ath6kl_usb_free_urb_to_pipe(struct ath6kl_usb_pipe *pipe,
> {
> unsigned long flags;
>
> + /* bail if this pipe is not initialized */
> + if (!pipe->ar_usb)
> + return;
> +
> spin_lock_irqsave(&pipe->ar_usb->cs_lock, flags);
> pipe->urb_cnt++;
>
> --
> 2.22.0
>
^ permalink raw reply
* Re: [PATCH] Fix a double free bug in rsi_91x_deinit
From: Guenter Roeck @ 2019-08-31 18:18 UTC (permalink / raw)
To: Hui Peng
Cc: security, Mathias Payer, Kalle Valo, David S. Miller,
linux-wireless, netdev, linux-kernel
In-Reply-To: <20190819220230.10597-1-benquike@gmail.com>
On Mon, Aug 19, 2019 at 06:02:29PM -0400, Hui Peng wrote:
> `dev` (struct rsi_91x_usbdev *) field of adapter
> (struct rsi_91x_usbdev *) is allocated and initialized in
> `rsi_init_usb_interface`. If any error is detected in information
> read from the device side, `rsi_init_usb_interface` will be
> freed. However, in the higher level error handling code in
> `rsi_probe`, if error is detected, `rsi_91x_deinit` is called
> again, in which `dev` will be freed again, resulting double free.
>
> This patch fixes the double free by removing the free operation on
> `dev` in `rsi_init_usb_interface`, because `rsi_91x_deinit` is also
> used in `rsi_disconnect`, in that code path, the `dev` field is not
> (and thus needs to be) freed.
>
> This bug was found in v4.19, but is also present in the latest version
> of kernel.
>
> Reported-by: Hui Peng <benquike@gmail.com>
> Reported-by: Mathias Payer <mathias.payer@nebelwelt.net>
> Signed-off-by: Hui Peng <benquike@gmail.com>
FWIW:
Reviewed-by: Guenter Roeck <linux@roeck-us.net>
This patch is listed as fix for CVE-2019-15504, which has a CVSS 2.0 score
of 10.0 (high) and CVSS 3.0 score of 9.8 (critical).
Are there any plans to apply this patch to the upstream kernel anytime
soon ? If not, are there reasons to believe that the problem is not as
severe as its CVSS score may indicate ?
Thanks,
Guenter
> ---
> drivers/net/wireless/rsi/rsi_91x_usb.c | 1 -
> 1 file changed, 1 deletion(-)
>
> diff --git a/drivers/net/wireless/rsi/rsi_91x_usb.c b/drivers/net/wireless/rsi/rsi_91x_usb.c
> index c0a163e40402..ac917227f708 100644
> --- a/drivers/net/wireless/rsi/rsi_91x_usb.c
> +++ b/drivers/net/wireless/rsi/rsi_91x_usb.c
> @@ -640,7 +640,6 @@ static int rsi_init_usb_interface(struct rsi_hw *adapter,
> kfree(rsi_dev->tx_buffer);
>
> fail_eps:
> - kfree(rsi_dev);
>
> return status;
> }
> --
> 2.22.1
>
^ permalink raw reply
* Re: [PATCH] Fix a double free bug in rsi_91x_deinit
From: Hui Peng @ 2019-08-31 18:32 UTC (permalink / raw)
To: Guenter Roeck
Cc: security, Mathias Payer, Kalle Valo, David S. Miller,
linux-wireless, netdev, linux-kernel
In-Reply-To: <20190831181852.GA22160@roeck-us.net>
On 8/31/19 2:18 PM, Guenter Roeck wrote:
> On Mon, Aug 19, 2019 at 06:02:29PM -0400, Hui Peng wrote:
>> `dev` (struct rsi_91x_usbdev *) field of adapter
>> (struct rsi_91x_usbdev *) is allocated and initialized in
>> `rsi_init_usb_interface`. If any error is detected in information
>> read from the device side, `rsi_init_usb_interface` will be
>> freed. However, in the higher level error handling code in
>> `rsi_probe`, if error is detected, `rsi_91x_deinit` is called
>> again, in which `dev` will be freed again, resulting double free.
>>
>> This patch fixes the double free by removing the free operation on
>> `dev` in `rsi_init_usb_interface`, because `rsi_91x_deinit` is also
>> used in `rsi_disconnect`, in that code path, the `dev` field is not
>> (and thus needs to be) freed.
>>
>> This bug was found in v4.19, but is also present in the latest version
>> of kernel.
>>
>> Reported-by: Hui Peng <benquike@gmail.com>
>> Reported-by: Mathias Payer <mathias.payer@nebelwelt.net>
>> Signed-off-by: Hui Peng <benquike@gmail.com>
> FWIW:
>
> Reviewed-by: Guenter Roeck <linux@roeck-us.net>
>
> This patch is listed as fix for CVE-2019-15504, which has a CVSS 2.0 score
> of 10.0 (high) and CVSS 3.0 score of 9.8 (critical).
>
> Are there any plans to apply this patch to the upstream kernel anytime
> soon ? If not, are there reasons to believe that the problem is not as
> severe as its CVSS score may indicate ?
This patch is also still under review.
> Thanks,
> Guenter
>
>> ---
>> drivers/net/wireless/rsi/rsi_91x_usb.c | 1 -
>> 1 file changed, 1 deletion(-)
>>
>> diff --git a/drivers/net/wireless/rsi/rsi_91x_usb.c b/drivers/net/wireless/rsi/rsi_91x_usb.c
>> index c0a163e40402..ac917227f708 100644
>> --- a/drivers/net/wireless/rsi/rsi_91x_usb.c
>> +++ b/drivers/net/wireless/rsi/rsi_91x_usb.c
>> @@ -640,7 +640,6 @@ static int rsi_init_usb_interface(struct rsi_hw *adapter,
>> kfree(rsi_dev->tx_buffer);
>>
>> fail_eps:
>> - kfree(rsi_dev);
>>
>> return status;
>> }
>> --
>> 2.22.1
>>
^ permalink raw reply
* Re: [PATCH v3 1/2] net: core: Notify on changes to dev->promiscuity.
From: Andrew Lunn @ 2019-08-31 19:35 UTC (permalink / raw)
To: Ido Schimmel
Cc: David Miller, jiri, horatiu.vultur, alexandre.belloni,
UNGLinuxDriver, allan.nielsen, ivecera, f.fainelli, netdev,
linux-kernel
In-Reply-To: <20190830094319.GA31789@splinter>
> Also, what happens when I'm running these application without putting
> the interface in promisc mode? On an offloaded interface I would not be
> able to even capture packets addressed to my interface's MAC address.
Sorry for rejoining the discussion late. I've been travelling and i'm
now 3/4 of the way to Lisbon.
That statement i don't get. If the frame has the MAC address of the
interface, it has to be delivered to the CPU. And so pcap will see it
when running on the interface. I can pretty much guarantee every DSA
driver does that.
But to address the bigger picture. My understanding is that we want to
model offloading as a mechanism to accelerate what Linux can already
do. The user should not have to care about these accelerators. The
interface should work like a normal Linux interface. I can put an IP
address on it and ping a peer. I can run a dhcp client and get an IP
address from a dhcp server. I can add the interface to a bridge, and
packets will get bridged. I as a user should not need to care if this
is done in software, or accelerated by offloading it. I can add a
route, and if the accelerate knows about L3, it can accelerate that as
well. If not, the kernel will route it.
So if i run wireshark on an interface, i expect the interface will be
put into promisc mode and i see all packets ingressing the interface.
What the accelerator needs to do to achieve this, i as a user don't
care.
I can follow the argument that i won't necessarily see every
packet. But that is always true. For many embedded systems, the CPU is
too slow to receive at line rate, even when we are talking about 1G
links. Packets do get dropped. And i hope tcpdump users understand
that.
For me, having tcpdump use tc trap is just wrong. It breaks the model
that the user should not care about the accelerator. If anything, i
think the driver needs to translate cBPF which pcap passes to the
kernel to whatever internal format the accelerator can process. That
is just another example of using hardware acceleration.
Andrew
^ permalink raw reply
* Re: [RFC PATCH v2 net-next 00/15] tc-taprio offload for SJA1105 DSA
From: Andrew Lunn @ 2019-08-31 19:41 UTC (permalink / raw)
To: Jakub Kicinski
Cc: Vladimir Oltean, Florian Fainelli, Vivien Didelot,
David S. Miller, Vinicius Costa Gomes, vedang.patel,
Richard Cochran, weifeng.voon, jiri, m-karicheri2, Jose.Abreu,
Ilias Apalodimas, Jamal Hadi Salim, xiyou.wangcong, netdev
In-Reply-To: <20190830152839.0fe34d25@cakuba.netronome.com>
> I'm not 100% sure how taprio handles locking TBH, it just seems naive
> that HW callback will not need to sleep, so the kernel should make sure
> that callback can sleep. Otherwise we'll end up with 3/4 of drivers
> implementing some async work routine...
Hi Jakub
I suspect this is because until recently, all such devices were on a
PCI bus, for some other form of memory mapped device. It is only
recently with DSA becoming popular, that we need to handle devices on
the end of other sorts of bus, be is MDIO, SPI or i2c.
Andrew
^ permalink raw reply
* Re: [PATCH 0/1] pull request for net-next: batman-adv 2019-08-30
From: David Miller @ 2019-08-31 20:15 UTC (permalink / raw)
To: sw; +Cc: netdev, b.a.t.m.a.n
In-Reply-To: <20190830072736.18535-1-sw@simonwunderlich.de>
From: Simon Wunderlich <sw@simonwunderlich.de>
Date: Fri, 30 Aug 2019 09:27:35 +0200
> here is a small maintenance pull request of batman-adv to go into net-next.
>
> Please pull or let me know of any problem!
Pulled, but generally speaking MAINTAINERS updates are always legitimate
for 'net'.
^ permalink raw reply
* Re: [PATCH 0/2] pull request for net: batman-adv 2019-08-30
From: David Miller @ 2019-08-31 20:18 UTC (permalink / raw)
To: sw; +Cc: netdev, b.a.t.m.a.n
In-Reply-To: <20190830072502.16929-1-sw@simonwunderlich.de>
From: Simon Wunderlich <sw@simonwunderlich.de>
Date: Fri, 30 Aug 2019 09:25:00 +0200
> here is another small batman-adv pull request for net.
>
> Please pull or let me know of any problem!
Pulled, thanks Simon.
^ permalink raw reply
* [PATCH net-next 00/10] net: dsa: mv88e6xxx: centralize SERDES IRQ handling
From: Vivien Didelot @ 2019-08-31 20:18 UTC (permalink / raw)
To: netdev; +Cc: davem, Marek Behún, f.fainelli, andrew, Vivien Didelot
Following Marek's work on the abstraction of the SERDES lanes mapping, this
series trades the .serdes_irq_setup and .serdes_irq_free callbacks for new
.serdes_irq_mapping, .serdes_irq_enable and .serdes_irq_status operations.
This has the benefit to limit the various SERDES implementations to simple
register accesses only; centralize the IRQ handling and mutex locking logic;
as well as reducing boilerplate in the driver.
Vivien Didelot (10):
net: dsa: mv88e6xxx: check errors in mv88e6352_serdes_irq_link
net: dsa: mv88e6xxx: fix SERDES IRQ mapping
net: dsa: mv88e6xxx: introduce .serdes_irq_mapping
net: dsa: mv88e6xxx: simplify .serdes_get_lane
net: dsa: mv88e6xxx: implement mv88e6352_serdes_get_lane
net: dsa: mv88e6xxx: merge mv88e6352_serdes_power_set
net: dsa: mv88e6xxx: pass lane to .serdes_power
net: dsa: mv88e6xxx: introduce .serdes_irq_enable
net: dsa: mv88e6xxx: introduce .serdes_irq_status
net: dsa: mv88e6xxx: centralize SERDES IRQ handling
drivers/net/dsa/mv88e6xxx/chip.c | 141 ++++++++---
drivers/net/dsa/mv88e6xxx/chip.h | 15 +-
drivers/net/dsa/mv88e6xxx/port.c | 21 +-
drivers/net/dsa/mv88e6xxx/serdes.c | 382 ++++++++---------------------
drivers/net/dsa/mv88e6xxx/serdes.h | 107 ++++++--
5 files changed, 315 insertions(+), 351 deletions(-)
--
2.23.0
^ 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