* 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
* [PATCH net-next 01/10] net: dsa: mv88e6xxx: check errors in mv88e6352_serdes_irq_link
From: Vivien Didelot @ 2019-08-31 20:18 UTC (permalink / raw)
To: netdev; +Cc: davem, Marek Behún, f.fainelli, andrew, Vivien Didelot
In-Reply-To: <20190831201836.19957-1-vivien.didelot@gmail.com>
The mv88e6352_serdes_irq_link helper is not checking for any error that
may occur during hardware accesses. Worst, the "up" boolean is set from
the potentially unused "status" variable, if read operations failed.
As done in mv88e6390_serdes_irq_link_sgmii, return right away and do
not call dsa_port_phylink_mac_change if an error occurred.
Signed-off-by: Vivien Didelot <vivien.didelot@gmail.com>
---
drivers/net/dsa/mv88e6xxx/serdes.c | 9 +++++++--
1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/drivers/net/dsa/mv88e6xxx/serdes.c b/drivers/net/dsa/mv88e6xxx/serdes.c
index 38c0da2492c0..7eb7ed68c91d 100644
--- a/drivers/net/dsa/mv88e6xxx/serdes.c
+++ b/drivers/net/dsa/mv88e6xxx/serdes.c
@@ -186,14 +186,19 @@ static void mv88e6352_serdes_irq_link(struct mv88e6xxx_chip *chip, int port)
struct dsa_switch *ds = chip->ds;
u16 status;
bool up;
+ int err;
- mv88e6352_serdes_read(chip, MII_BMSR, &status);
+ err = mv88e6352_serdes_read(chip, MII_BMSR, &status);
+ if (err)
+ return;
/* Status must be read twice in order to give the current link
* status. Otherwise the change in link status since the last
* read of the register is returned.
*/
- mv88e6352_serdes_read(chip, MII_BMSR, &status);
+ err = mv88e6352_serdes_read(chip, MII_BMSR, &status);
+ if (err)
+ return;
up = status & BMSR_LSTATUS;
--
2.23.0
^ permalink raw reply related
* [PATCH net-next 02/10] net: dsa: mv88e6xxx: fix SERDES IRQ mapping
From: Vivien Didelot @ 2019-08-31 20:18 UTC (permalink / raw)
To: netdev; +Cc: davem, Marek Behún, f.fainelli, andrew, Vivien Didelot
In-Reply-To: <20190831201836.19957-1-vivien.didelot@gmail.com>
The current mv88e6xxx SERDES code checks for negative error code from
irq_find_mapping, while this function returns an unsigned integer. This
patch removes this dead code and simply returns 0 is no IRQ is found.
Signed-off-by: Vivien Didelot <vivien.didelot@gmail.com>
---
drivers/net/dsa/mv88e6xxx/chip.h | 2 +-
drivers/net/dsa/mv88e6xxx/serdes.c | 14 ++++----------
2 files changed, 5 insertions(+), 11 deletions(-)
diff --git a/drivers/net/dsa/mv88e6xxx/chip.h b/drivers/net/dsa/mv88e6xxx/chip.h
index 421e8b84bec3..2016f51c868b 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.h
+++ b/drivers/net/dsa/mv88e6xxx/chip.h
@@ -199,7 +199,7 @@ struct mv88e6xxx_port {
u64 vtu_member_violation;
u64 vtu_miss_violation;
u8 cmode;
- int serdes_irq;
+ unsigned int serdes_irq;
};
struct mv88e6xxx_chip {
diff --git a/drivers/net/dsa/mv88e6xxx/serdes.c b/drivers/net/dsa/mv88e6xxx/serdes.c
index 7eb7ed68c91d..f65652e6edec 100644
--- a/drivers/net/dsa/mv88e6xxx/serdes.c
+++ b/drivers/net/dsa/mv88e6xxx/serdes.c
@@ -249,11 +249,8 @@ int mv88e6352_serdes_irq_setup(struct mv88e6xxx_chip *chip, int port)
chip->ports[port].serdes_irq = irq_find_mapping(chip->g2_irq.domain,
MV88E6352_SERDES_IRQ);
- if (chip->ports[port].serdes_irq < 0) {
- dev_err(chip->dev, "Unable to map SERDES irq: %d\n",
- chip->ports[port].serdes_irq);
- return chip->ports[port].serdes_irq;
- }
+ if (!chip->ports[port].serdes_irq)
+ return 0;
/* Requesting the IRQ will trigger irq callbacks. So we cannot
* hold the reg_lock.
@@ -690,11 +687,8 @@ int mv88e6390_serdes_irq_setup(struct mv88e6xxx_chip *chip, int port)
chip->ports[port].serdes_irq = irq_find_mapping(chip->g2_irq.domain,
port);
- if (chip->ports[port].serdes_irq < 0) {
- dev_err(chip->dev, "Unable to map SERDES irq: %d\n",
- chip->ports[port].serdes_irq);
- return chip->ports[port].serdes_irq;
- }
+ if (!chip->ports[port].serdes_irq)
+ return 0;
/* Requesting the IRQ will trigger irq callbacks. So we cannot
* hold the reg_lock.
--
2.23.0
^ permalink raw reply related
* [PATCH net-next 03/10] net: dsa: mv88e6xxx: introduce .serdes_irq_mapping
From: Vivien Didelot @ 2019-08-31 20:18 UTC (permalink / raw)
To: netdev; +Cc: davem, Marek Behún, f.fainelli, andrew, Vivien Didelot
In-Reply-To: <20190831201836.19957-1-vivien.didelot@gmail.com>
Introduce a new .serdes_irq_mapping operation to prepare the
abstraction of IRQ mapping from the SERDES IRQ setup code.
Signed-off-by: Vivien Didelot <vivien.didelot@gmail.com>
---
drivers/net/dsa/mv88e6xxx/chip.c | 11 +++++++++++
drivers/net/dsa/mv88e6xxx/chip.h | 2 ++
drivers/net/dsa/mv88e6xxx/serdes.c | 26 ++++++++++++++++++++------
drivers/net/dsa/mv88e6xxx/serdes.h | 12 ++++++++++++
4 files changed, 45 insertions(+), 6 deletions(-)
diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index c648f9fbfa59..0ab4ce86eda7 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -2931,6 +2931,7 @@ static const struct mv88e6xxx_ops mv88e6141_ops = {
.vtu_loadpurge = mv88e6352_g1_vtu_loadpurge,
.serdes_power = mv88e6390_serdes_power,
.serdes_get_lane = mv88e6341_serdes_get_lane,
+ .serdes_irq_mapping = mv88e6390_serdes_irq_mapping,
.serdes_irq_setup = mv88e6390_serdes_irq_setup,
.serdes_irq_free = mv88e6390_serdes_irq_free,
.gpio_ops = &mv88e6352_gpio_ops,
@@ -3178,6 +3179,7 @@ static const struct mv88e6xxx_ops mv88e6176_ops = {
.vtu_getnext = mv88e6352_g1_vtu_getnext,
.vtu_loadpurge = mv88e6352_g1_vtu_loadpurge,
.serdes_power = mv88e6352_serdes_power,
+ .serdes_irq_mapping = mv88e6352_serdes_irq_mapping,
.serdes_irq_setup = mv88e6352_serdes_irq_setup,
.serdes_irq_free = mv88e6352_serdes_irq_free,
.gpio_ops = &mv88e6352_gpio_ops,
@@ -3261,6 +3263,7 @@ static const struct mv88e6xxx_ops mv88e6190_ops = {
.vtu_loadpurge = mv88e6390_g1_vtu_loadpurge,
.serdes_power = mv88e6390_serdes_power,
.serdes_get_lane = mv88e6390_serdes_get_lane,
+ .serdes_irq_mapping = mv88e6390_serdes_irq_mapping,
.serdes_irq_setup = mv88e6390_serdes_irq_setup,
.serdes_irq_free = mv88e6390_serdes_irq_free,
.gpio_ops = &mv88e6352_gpio_ops,
@@ -3308,6 +3311,7 @@ static const struct mv88e6xxx_ops mv88e6190x_ops = {
.vtu_loadpurge = mv88e6390_g1_vtu_loadpurge,
.serdes_power = mv88e6390_serdes_power,
.serdes_get_lane = mv88e6390x_serdes_get_lane,
+ .serdes_irq_mapping = mv88e6390_serdes_irq_mapping,
.serdes_irq_setup = mv88e6390_serdes_irq_setup,
.serdes_irq_free = mv88e6390_serdes_irq_free,
.gpio_ops = &mv88e6352_gpio_ops,
@@ -3355,6 +3359,7 @@ static const struct mv88e6xxx_ops mv88e6191_ops = {
.vtu_loadpurge = mv88e6390_g1_vtu_loadpurge,
.serdes_power = mv88e6390_serdes_power,
.serdes_get_lane = mv88e6390_serdes_get_lane,
+ .serdes_irq_mapping = mv88e6390_serdes_irq_mapping,
.serdes_irq_setup = mv88e6390_serdes_irq_setup,
.serdes_irq_free = mv88e6390_serdes_irq_free,
.avb_ops = &mv88e6390_avb_ops,
@@ -3403,6 +3408,7 @@ static const struct mv88e6xxx_ops mv88e6240_ops = {
.vtu_getnext = mv88e6352_g1_vtu_getnext,
.vtu_loadpurge = mv88e6352_g1_vtu_loadpurge,
.serdes_power = mv88e6352_serdes_power,
+ .serdes_irq_mapping = mv88e6352_serdes_irq_mapping,
.serdes_irq_setup = mv88e6352_serdes_irq_setup,
.serdes_irq_free = mv88e6352_serdes_irq_free,
.gpio_ops = &mv88e6352_gpio_ops,
@@ -3492,6 +3498,7 @@ static const struct mv88e6xxx_ops mv88e6290_ops = {
.vtu_loadpurge = mv88e6390_g1_vtu_loadpurge,
.serdes_power = mv88e6390_serdes_power,
.serdes_get_lane = mv88e6390_serdes_get_lane,
+ .serdes_irq_mapping = mv88e6390_serdes_irq_mapping,
.serdes_irq_setup = mv88e6390_serdes_irq_setup,
.serdes_irq_free = mv88e6390_serdes_irq_free,
.gpio_ops = &mv88e6352_gpio_ops,
@@ -3629,6 +3636,7 @@ static const struct mv88e6xxx_ops mv88e6341_ops = {
.vtu_loadpurge = mv88e6352_g1_vtu_loadpurge,
.serdes_power = mv88e6390_serdes_power,
.serdes_get_lane = mv88e6341_serdes_get_lane,
+ .serdes_irq_mapping = mv88e6390_serdes_irq_mapping,
.serdes_irq_setup = mv88e6390_serdes_irq_setup,
.serdes_irq_free = mv88e6390_serdes_irq_free,
.gpio_ops = &mv88e6352_gpio_ops,
@@ -3760,6 +3768,7 @@ static const struct mv88e6xxx_ops mv88e6352_ops = {
.vtu_getnext = mv88e6352_g1_vtu_getnext,
.vtu_loadpurge = mv88e6352_g1_vtu_loadpurge,
.serdes_power = mv88e6352_serdes_power,
+ .serdes_irq_mapping = mv88e6352_serdes_irq_mapping,
.serdes_irq_setup = mv88e6352_serdes_irq_setup,
.serdes_irq_free = mv88e6352_serdes_irq_free,
.gpio_ops = &mv88e6352_gpio_ops,
@@ -3814,6 +3823,7 @@ static const struct mv88e6xxx_ops mv88e6390_ops = {
.vtu_loadpurge = mv88e6390_g1_vtu_loadpurge,
.serdes_power = mv88e6390_serdes_power,
.serdes_get_lane = mv88e6390_serdes_get_lane,
+ .serdes_irq_mapping = mv88e6390_serdes_irq_mapping,
.serdes_irq_setup = mv88e6390_serdes_irq_setup,
.serdes_irq_free = mv88e6390_serdes_irq_free,
.gpio_ops = &mv88e6352_gpio_ops,
@@ -3865,6 +3875,7 @@ static const struct mv88e6xxx_ops mv88e6390x_ops = {
.vtu_loadpurge = mv88e6390_g1_vtu_loadpurge,
.serdes_power = mv88e6390_serdes_power,
.serdes_get_lane = mv88e6390x_serdes_get_lane,
+ .serdes_irq_mapping = mv88e6390_serdes_irq_mapping,
.serdes_irq_setup = mv88e6390_serdes_irq_setup,
.serdes_irq_free = mv88e6390_serdes_irq_free,
.gpio_ops = &mv88e6352_gpio_ops,
diff --git a/drivers/net/dsa/mv88e6xxx/chip.h b/drivers/net/dsa/mv88e6xxx/chip.h
index 2016f51c868b..b116bd7f6109 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.h
+++ b/drivers/net/dsa/mv88e6xxx/chip.h
@@ -447,6 +447,8 @@ struct mv88e6xxx_ops {
int (*serdes_get_lane)(struct mv88e6xxx_chip *chip, int port, u8 *lane);
/* SERDES interrupt handling */
+ unsigned int (*serdes_irq_mapping)(struct mv88e6xxx_chip *chip,
+ int port);
int (*serdes_irq_setup)(struct mv88e6xxx_chip *chip, int port);
void (*serdes_irq_free)(struct mv88e6xxx_chip *chip, int port);
diff --git a/drivers/net/dsa/mv88e6xxx/serdes.c b/drivers/net/dsa/mv88e6xxx/serdes.c
index f65652e6edec..4fb1dca64ef1 100644
--- a/drivers/net/dsa/mv88e6xxx/serdes.c
+++ b/drivers/net/dsa/mv88e6xxx/serdes.c
@@ -240,18 +240,25 @@ static int mv88e6352_serdes_irq_disable(struct mv88e6xxx_chip *chip)
return mv88e6352_serdes_write(chip, MV88E6352_SERDES_INT_ENABLE, 0);
}
+unsigned int mv88e6352_serdes_irq_mapping(struct mv88e6xxx_chip *chip, int port)
+{
+ return irq_find_mapping(chip->g2_irq.domain, MV88E6352_SERDES_IRQ);
+}
+
int mv88e6352_serdes_irq_setup(struct mv88e6xxx_chip *chip, int port)
{
+ unsigned int irq;
int err;
if (!mv88e6352_port_has_serdes(chip, port))
return 0;
- chip->ports[port].serdes_irq = irq_find_mapping(chip->g2_irq.domain,
- MV88E6352_SERDES_IRQ);
- if (!chip->ports[port].serdes_irq)
+ irq = mv88e6xxx_serdes_irq_mapping(chip, port);
+ if (!irq)
return 0;
+ chip->ports[port].serdes_irq = irq;
+
/* Requesting the IRQ will trigger irq callbacks. So we cannot
* hold the reg_lock.
*/
@@ -673,8 +680,14 @@ static irqreturn_t mv88e6390_serdes_thread_fn(int irq, void *dev_id)
return ret;
}
+unsigned int mv88e6390_serdes_irq_mapping(struct mv88e6xxx_chip *chip, int port)
+{
+ return irq_find_mapping(chip->g2_irq.domain, port);
+}
+
int mv88e6390_serdes_irq_setup(struct mv88e6xxx_chip *chip, int port)
{
+ unsigned int irq;
int err;
u8 lane;
@@ -685,11 +698,12 @@ int mv88e6390_serdes_irq_setup(struct mv88e6xxx_chip *chip, int port)
return err;
}
- chip->ports[port].serdes_irq = irq_find_mapping(chip->g2_irq.domain,
- port);
- if (!chip->ports[port].serdes_irq)
+ irq = mv88e6xxx_serdes_irq_mapping(chip, port);
+ if (!irq)
return 0;
+ chip->ports[port].serdes_irq = irq;
+
/* Requesting the IRQ will trigger irq callbacks. So we cannot
* hold the reg_lock.
*/
diff --git a/drivers/net/dsa/mv88e6xxx/serdes.h b/drivers/net/dsa/mv88e6xxx/serdes.h
index c2e6fc3ddf8b..cb8d287c9388 100644
--- a/drivers/net/dsa/mv88e6xxx/serdes.h
+++ b/drivers/net/dsa/mv88e6xxx/serdes.h
@@ -90,6 +90,10 @@ static inline int mv88e6xxx_serdes_get_lane(struct mv88e6xxx_chip *chip,
int mv88e6341_serdes_get_lane(struct mv88e6xxx_chip *chip, int port, u8 *lane);
int mv88e6390_serdes_get_lane(struct mv88e6xxx_chip *chip, int port, u8 *lane);
int mv88e6390x_serdes_get_lane(struct mv88e6xxx_chip *chip, int port, u8 *lane);
+unsigned int mv88e6352_serdes_irq_mapping(struct mv88e6xxx_chip *chip,
+ int port);
+unsigned int mv88e6390_serdes_irq_mapping(struct mv88e6xxx_chip *chip,
+ int port);
int mv88e6352_serdes_power(struct mv88e6xxx_chip *chip, int port, bool on);
int mv88e6390_serdes_power(struct mv88e6xxx_chip *chip, int port, bool on);
int mv88e6390_serdes_irq_setup(struct mv88e6xxx_chip *chip, int port);
@@ -106,5 +110,13 @@ int mv88e6390_serdes_irq_disable(struct mv88e6xxx_chip *chip, int port,
int mv88e6352_serdes_irq_setup(struct mv88e6xxx_chip *chip, int port);
void mv88e6352_serdes_irq_free(struct mv88e6xxx_chip *chip, int port);
+static inline unsigned int
+mv88e6xxx_serdes_irq_mapping(struct mv88e6xxx_chip *chip, int port)
+{
+ if (!chip->info->ops->serdes_irq_mapping)
+ return 0;
+
+ return chip->info->ops->serdes_irq_mapping(chip, port);
+}
#endif
--
2.23.0
^ permalink raw reply related
* [PATCH net-next 04/10] net: dsa: mv88e6xxx: simplify .serdes_get_lane
From: Vivien Didelot @ 2019-08-31 20:18 UTC (permalink / raw)
To: netdev; +Cc: davem, Marek Behún, f.fainelli, andrew, Vivien Didelot
In-Reply-To: <20190831201836.19957-1-vivien.didelot@gmail.com>
Because the mapping between a SERDES interface and its lane is static,
we don't need to stick with negative error codes actually and we can
simply return 0 if there is no lane, just like the IRQ mapping.
This way we can keep a simple and intuitive API using unsigned lane
numbers while simplifying the implementations with single return
statements. Last but not least, fix the reverse chrismas tree in
mv88e6390x_serdes_get_lane.
Signed-off-by: Vivien Didelot <vivien.didelot@gmail.com>
---
drivers/net/dsa/mv88e6xxx/chip.h | 2 +-
drivers/net/dsa/mv88e6xxx/port.c | 13 +--
drivers/net/dsa/mv88e6xxx/serdes.c | 152 +++++++++++------------------
drivers/net/dsa/mv88e6xxx/serdes.h | 29 +++---
4 files changed, 74 insertions(+), 122 deletions(-)
diff --git a/drivers/net/dsa/mv88e6xxx/chip.h b/drivers/net/dsa/mv88e6xxx/chip.h
index b116bd7f6109..add0ec5188ec 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.h
+++ b/drivers/net/dsa/mv88e6xxx/chip.h
@@ -444,7 +444,7 @@ struct mv88e6xxx_ops {
int (*serdes_power)(struct mv88e6xxx_chip *chip, int port, bool on);
/* SERDES lane mapping */
- int (*serdes_get_lane)(struct mv88e6xxx_chip *chip, int port, u8 *lane);
+ u8 (*serdes_get_lane)(struct mv88e6xxx_chip *chip, int port);
/* SERDES interrupt handling */
unsigned int (*serdes_irq_mapping)(struct mv88e6xxx_chip *chip,
diff --git a/drivers/net/dsa/mv88e6xxx/port.c b/drivers/net/dsa/mv88e6xxx/port.c
index 4f841335ea32..06e2bdf6fa82 100644
--- a/drivers/net/dsa/mv88e6xxx/port.c
+++ b/drivers/net/dsa/mv88e6xxx/port.c
@@ -431,11 +431,8 @@ static int mv88e6xxx_port_set_cmode(struct mv88e6xxx_chip *chip, int port,
if (cmode == chip->ports[port].cmode)
return 0;
- err = mv88e6xxx_serdes_get_lane(chip, port, &lane);
- if (err && err != -ENODEV)
- return err;
-
- if (err != -ENODEV) {
+ lane = mv88e6xxx_serdes_get_lane(chip, port);
+ if (lane) {
if (chip->ports[port].serdes_irq) {
err = mv88e6390_serdes_irq_disable(chip, port, lane);
if (err)
@@ -463,9 +460,9 @@ static int mv88e6xxx_port_set_cmode(struct mv88e6xxx_chip *chip, int port,
chip->ports[port].cmode = cmode;
- err = mv88e6xxx_serdes_get_lane(chip, port, &lane);
- if (err)
- return err;
+ lane = mv88e6xxx_serdes_get_lane(chip, port);
+ if (!lane)
+ return -ENODEV;
err = mv88e6390_serdes_power(chip, port, true);
if (err)
diff --git a/drivers/net/dsa/mv88e6xxx/serdes.c b/drivers/net/dsa/mv88e6xxx/serdes.c
index 4fb1dca64ef1..ce6d97e5caf8 100644
--- a/drivers/net/dsa/mv88e6xxx/serdes.c
+++ b/drivers/net/dsa/mv88e6xxx/serdes.c
@@ -295,149 +295,119 @@ void mv88e6352_serdes_irq_free(struct mv88e6xxx_chip *chip, int port)
chip->ports[port].serdes_irq = 0;
}
-int mv88e6341_serdes_get_lane(struct mv88e6xxx_chip *chip, int port, u8 *lane)
+u8 mv88e6341_serdes_get_lane(struct mv88e6xxx_chip *chip, int port)
{
u8 cmode = chip->ports[port].cmode;
+ u8 lane = 0;
- if (port != 5)
- return -ENODEV;
-
- if (cmode == MV88E6XXX_PORT_STS_CMODE_1000BASEX ||
- cmode == MV88E6XXX_PORT_STS_CMODE_SGMII ||
- cmode == MV88E6XXX_PORT_STS_CMODE_2500BASEX) {
- *lane = MV88E6341_PORT5_LANE;
- return 0;
+ switch (port) {
+ case 5:
+ if (cmode == MV88E6XXX_PORT_STS_CMODE_1000BASEX ||
+ cmode == MV88E6XXX_PORT_STS_CMODE_SGMII ||
+ cmode == MV88E6XXX_PORT_STS_CMODE_2500BASEX)
+ lane = MV88E6341_PORT5_LANE;
+ break;
}
- return -ENODEV;
+ return lane;
}
-int mv88e6390_serdes_get_lane(struct mv88e6xxx_chip *chip, int port, u8 *lane)
+u8 mv88e6390_serdes_get_lane(struct mv88e6xxx_chip *chip, int port)
{
u8 cmode = chip->ports[port].cmode;
+ u8 lane = 0;
switch (port) {
case 9:
if (cmode == MV88E6XXX_PORT_STS_CMODE_1000BASEX ||
cmode == MV88E6XXX_PORT_STS_CMODE_SGMII ||
- cmode == MV88E6XXX_PORT_STS_CMODE_2500BASEX) {
- *lane = MV88E6390_PORT9_LANE0;
- return 0;
- }
+ cmode == MV88E6XXX_PORT_STS_CMODE_2500BASEX)
+ lane = MV88E6390_PORT9_LANE0;
break;
case 10:
if (cmode == MV88E6XXX_PORT_STS_CMODE_1000BASEX ||
cmode == MV88E6XXX_PORT_STS_CMODE_SGMII ||
- cmode == MV88E6XXX_PORT_STS_CMODE_2500BASEX) {
- *lane = MV88E6390_PORT10_LANE0;
- return 0;
- }
- break;
- default:
+ cmode == MV88E6XXX_PORT_STS_CMODE_2500BASEX)
+ lane = MV88E6390_PORT10_LANE0;
break;
}
- return -ENODEV;
+ return lane;
}
-int mv88e6390x_serdes_get_lane(struct mv88e6xxx_chip *chip, int port, u8 *lane)
+u8 mv88e6390x_serdes_get_lane(struct mv88e6xxx_chip *chip, int port)
{
- u8 cmode_port9, cmode_port10, cmode_port;
-
- cmode_port9 = chip->ports[9].cmode;
- cmode_port10 = chip->ports[10].cmode;
- cmode_port = chip->ports[port].cmode;
+ u8 cmode_port = chip->ports[port].cmode;
+ u8 cmode_port10 = chip->ports[10].cmode;
+ u8 cmode_port9 = chip->ports[9].cmode;
+ u8 lane = 0;
switch (port) {
case 2:
if (cmode_port9 == MV88E6XXX_PORT_STS_CMODE_1000BASEX ||
cmode_port9 == MV88E6XXX_PORT_STS_CMODE_SGMII ||
- cmode_port9 == MV88E6XXX_PORT_STS_CMODE_2500BASEX) {
- if (cmode_port == MV88E6XXX_PORT_STS_CMODE_1000BASEX) {
- *lane = MV88E6390_PORT9_LANE1;
- return 0;
- }
- }
+ cmode_port9 == MV88E6XXX_PORT_STS_CMODE_2500BASEX)
+ if (cmode_port == MV88E6XXX_PORT_STS_CMODE_1000BASEX)
+ lane = MV88E6390_PORT9_LANE1;
break;
case 3:
if (cmode_port9 == MV88E6XXX_PORT_STS_CMODE_1000BASEX ||
cmode_port9 == MV88E6XXX_PORT_STS_CMODE_SGMII ||
cmode_port9 == MV88E6XXX_PORT_STS_CMODE_2500BASEX ||
- cmode_port9 == MV88E6XXX_PORT_STS_CMODE_RXAUI) {
- if (cmode_port == MV88E6XXX_PORT_STS_CMODE_1000BASEX) {
- *lane = MV88E6390_PORT9_LANE2;
- return 0;
- }
- }
+ cmode_port9 == MV88E6XXX_PORT_STS_CMODE_RXAUI)
+ if (cmode_port == MV88E6XXX_PORT_STS_CMODE_1000BASEX)
+ lane = MV88E6390_PORT9_LANE2;
break;
case 4:
if (cmode_port9 == MV88E6XXX_PORT_STS_CMODE_1000BASEX ||
cmode_port9 == MV88E6XXX_PORT_STS_CMODE_SGMII ||
cmode_port9 == MV88E6XXX_PORT_STS_CMODE_2500BASEX ||
- cmode_port9 == MV88E6XXX_PORT_STS_CMODE_RXAUI) {
- if (cmode_port == MV88E6XXX_PORT_STS_CMODE_1000BASEX) {
- *lane = MV88E6390_PORT9_LANE3;
- return 0;
- }
- }
+ cmode_port9 == MV88E6XXX_PORT_STS_CMODE_RXAUI)
+ if (cmode_port == MV88E6XXX_PORT_STS_CMODE_1000BASEX)
+ lane = MV88E6390_PORT9_LANE3;
break;
case 5:
if (cmode_port10 == MV88E6XXX_PORT_STS_CMODE_1000BASEX ||
cmode_port10 == MV88E6XXX_PORT_STS_CMODE_SGMII ||
- cmode_port10 == MV88E6XXX_PORT_STS_CMODE_2500BASEX) {
- if (cmode_port == MV88E6XXX_PORT_STS_CMODE_1000BASEX) {
- *lane = MV88E6390_PORT10_LANE1;
- return 0;
- }
- }
+ cmode_port10 == MV88E6XXX_PORT_STS_CMODE_2500BASEX)
+ if (cmode_port == MV88E6XXX_PORT_STS_CMODE_1000BASEX)
+ lane = MV88E6390_PORT10_LANE1;
break;
case 6:
if (cmode_port10 == MV88E6XXX_PORT_STS_CMODE_1000BASEX ||
cmode_port10 == MV88E6XXX_PORT_STS_CMODE_SGMII ||
cmode_port10 == MV88E6XXX_PORT_STS_CMODE_2500BASEX ||
- cmode_port10 == MV88E6XXX_PORT_STS_CMODE_RXAUI) {
- if (cmode_port == MV88E6XXX_PORT_STS_CMODE_1000BASEX) {
- *lane = MV88E6390_PORT10_LANE2;
- return 0;
- }
- }
+ cmode_port10 == MV88E6XXX_PORT_STS_CMODE_RXAUI)
+ if (cmode_port == MV88E6XXX_PORT_STS_CMODE_1000BASEX)
+ lane = MV88E6390_PORT10_LANE2;
break;
case 7:
if (cmode_port10 == MV88E6XXX_PORT_STS_CMODE_1000BASEX ||
cmode_port10 == MV88E6XXX_PORT_STS_CMODE_SGMII ||
cmode_port10 == MV88E6XXX_PORT_STS_CMODE_2500BASEX ||
- cmode_port10 == MV88E6XXX_PORT_STS_CMODE_RXAUI) {
- if (cmode_port == MV88E6XXX_PORT_STS_CMODE_1000BASEX) {
- *lane = MV88E6390_PORT10_LANE3;
- return 0;
- }
- }
+ cmode_port10 == MV88E6XXX_PORT_STS_CMODE_RXAUI)
+ if (cmode_port == MV88E6XXX_PORT_STS_CMODE_1000BASEX)
+ lane = MV88E6390_PORT10_LANE3;
break;
case 9:
if (cmode_port9 == MV88E6XXX_PORT_STS_CMODE_1000BASEX ||
cmode_port9 == MV88E6XXX_PORT_STS_CMODE_SGMII ||
cmode_port9 == MV88E6XXX_PORT_STS_CMODE_2500BASEX ||
cmode_port9 == MV88E6XXX_PORT_STS_CMODE_XAUI ||
- cmode_port9 == MV88E6XXX_PORT_STS_CMODE_RXAUI) {
- *lane = MV88E6390_PORT9_LANE0;
- return 0;
- }
+ cmode_port9 == MV88E6XXX_PORT_STS_CMODE_RXAUI)
+ lane = MV88E6390_PORT9_LANE0;
break;
case 10:
if (cmode_port10 == MV88E6XXX_PORT_STS_CMODE_1000BASEX ||
cmode_port10 == MV88E6XXX_PORT_STS_CMODE_SGMII ||
cmode_port10 == MV88E6XXX_PORT_STS_CMODE_2500BASEX ||
cmode_port10 == MV88E6XXX_PORT_STS_CMODE_XAUI ||
- cmode_port10 == MV88E6XXX_PORT_STS_CMODE_RXAUI) {
- *lane = MV88E6390_PORT10_LANE0;
- return 0;
- }
- break;
- default:
+ cmode_port10 == MV88E6XXX_PORT_STS_CMODE_RXAUI)
+ lane = MV88E6390_PORT10_LANE0;
break;
}
- return -ENODEV;
+ return lane;
}
/* Set the power on/off for 10GBASE-R and 10GBASE-X4/X2 */
@@ -497,14 +467,10 @@ int mv88e6390_serdes_power(struct mv88e6xxx_chip *chip, int port, bool on)
{
u8 cmode = chip->ports[port].cmode;
u8 lane;
- int err;
- err = mv88e6xxx_serdes_get_lane(chip, port, &lane);
- if (err) {
- if (err == -ENODEV)
- err = 0;
- return err;
- }
+ lane = mv88e6xxx_serdes_get_lane(chip, port);
+ if (!lane)
+ return 0;
switch (cmode) {
case MV88E6XXX_PORT_STS_CMODE_SGMII:
@@ -657,8 +623,8 @@ static irqreturn_t mv88e6390_serdes_thread_fn(int irq, void *dev_id)
mv88e6xxx_reg_lock(chip);
- err = mv88e6xxx_serdes_get_lane(chip, port->port, &lane);
- if (err)
+ lane = mv88e6xxx_serdes_get_lane(chip, port->port);
+ if (!lane)
goto out;
switch (cmode) {
@@ -691,12 +657,9 @@ int mv88e6390_serdes_irq_setup(struct mv88e6xxx_chip *chip, int port)
int err;
u8 lane;
- err = mv88e6xxx_serdes_get_lane(chip, port, &lane);
- if (err) {
- if (err == -ENODEV)
- err = 0;
- return err;
- }
+ lane = mv88e6xxx_serdes_get_lane(chip, port);
+ if (!lane)
+ return 0;
irq = mv88e6xxx_serdes_irq_mapping(chip, port);
if (!irq)
@@ -725,16 +688,11 @@ int mv88e6390_serdes_irq_setup(struct mv88e6xxx_chip *chip, int port)
void mv88e6390_serdes_irq_free(struct mv88e6xxx_chip *chip, int port)
{
- int err;
u8 lane;
- err = mv88e6xxx_serdes_get_lane(chip, port, &lane);
- if (err) {
- if (err != -ENODEV)
- dev_err(chip->dev, "Unable to free SERDES irq: %d\n",
- err);
+ lane = mv88e6xxx_serdes_get_lane(chip, port);
+ if (!lane)
return;
- }
mv88e6390_serdes_irq_disable(chip, port, lane);
diff --git a/drivers/net/dsa/mv88e6xxx/serdes.h b/drivers/net/dsa/mv88e6xxx/serdes.h
index cb8d287c9388..4718dcca6b3c 100644
--- a/drivers/net/dsa/mv88e6xxx/serdes.h
+++ b/drivers/net/dsa/mv88e6xxx/serdes.h
@@ -74,22 +74,9 @@
#define MV88E6390_SGMII_PHY_STATUS_SPD_DPL_VALID BIT(11)
#define MV88E6390_SGMII_PHY_STATUS_LINK BIT(10)
-/* Put the SERDES lane address a port is using into *lane. If a port has
- * multiple lanes, should put the first lane the port is using. If a port does
- * not have a lane, return -ENODEV.
- */
-static inline int mv88e6xxx_serdes_get_lane(struct mv88e6xxx_chip *chip,
- int port, u8 *lane)
-{
- if (!chip->info->ops->serdes_get_lane)
- return -EOPNOTSUPP;
-
- return chip->info->ops->serdes_get_lane(chip, port, lane);
-}
-
-int mv88e6341_serdes_get_lane(struct mv88e6xxx_chip *chip, int port, u8 *lane);
-int mv88e6390_serdes_get_lane(struct mv88e6xxx_chip *chip, int port, u8 *lane);
-int mv88e6390x_serdes_get_lane(struct mv88e6xxx_chip *chip, int port, u8 *lane);
+u8 mv88e6341_serdes_get_lane(struct mv88e6xxx_chip *chip, int port);
+u8 mv88e6390_serdes_get_lane(struct mv88e6xxx_chip *chip, int port);
+u8 mv88e6390x_serdes_get_lane(struct mv88e6xxx_chip *chip, int port);
unsigned int mv88e6352_serdes_irq_mapping(struct mv88e6xxx_chip *chip,
int port);
unsigned int mv88e6390_serdes_irq_mapping(struct mv88e6xxx_chip *chip,
@@ -110,6 +97,16 @@ int mv88e6390_serdes_irq_disable(struct mv88e6xxx_chip *chip, int port,
int mv88e6352_serdes_irq_setup(struct mv88e6xxx_chip *chip, int port);
void mv88e6352_serdes_irq_free(struct mv88e6xxx_chip *chip, int port);
+/* Return the (first) SERDES lane address a port is using, 0 otherwise. */
+static inline u8 mv88e6xxx_serdes_get_lane(struct mv88e6xxx_chip *chip,
+ int port)
+{
+ if (!chip->info->ops->serdes_get_lane)
+ return 0;
+
+ return chip->info->ops->serdes_get_lane(chip, port);
+}
+
static inline unsigned int
mv88e6xxx_serdes_irq_mapping(struct mv88e6xxx_chip *chip, int port)
{
--
2.23.0
^ permalink raw reply related
* [PATCH net-next 06/10] net: dsa: mv88e6xxx: merge mv88e6352_serdes_power_set
From: Vivien Didelot @ 2019-08-31 20:18 UTC (permalink / raw)
To: netdev; +Cc: davem, Marek Behún, f.fainelli, andrew, Vivien Didelot
In-Reply-To: <20190831201836.19957-1-vivien.didelot@gmail.com>
The mv88e6352_serdes_power_set helper is only used at one place, in
mv88e6352_serdes_power. Keep it simple and merge the two functions
together.
Use mv88e6xxx_serdes_get_lane instead of mv88e6352_port_has_serdes
to avoid moving code. No functional changes.
Signed-off-by: Vivien Didelot <vivien.didelot@gmail.com>
---
drivers/net/dsa/mv88e6xxx/serdes.c | 18 ++++--------------
1 file changed, 4 insertions(+), 14 deletions(-)
diff --git a/drivers/net/dsa/mv88e6xxx/serdes.c b/drivers/net/dsa/mv88e6xxx/serdes.c
index 9fb2773a3eb5..e8ad66987be9 100644
--- a/drivers/net/dsa/mv88e6xxx/serdes.c
+++ b/drivers/net/dsa/mv88e6xxx/serdes.c
@@ -49,11 +49,14 @@ static int mv88e6390_serdes_write(struct mv88e6xxx_chip *chip,
return mv88e6xxx_phy_write(chip, lane, reg_c45, val);
}
-static int mv88e6352_serdes_power_set(struct mv88e6xxx_chip *chip, bool on)
+int mv88e6352_serdes_power(struct mv88e6xxx_chip *chip, int port, bool on)
{
u16 val, new_val;
int err;
+ if (!mv88e6xxx_serdes_get_lane(chip, port))
+ return 0;
+
err = mv88e6352_serdes_read(chip, MII_BMCR, &val);
if (err)
return err;
@@ -90,19 +93,6 @@ static bool mv88e6352_port_has_serdes(struct mv88e6xxx_chip *chip, int port)
return false;
}
-int mv88e6352_serdes_power(struct mv88e6xxx_chip *chip, int port, bool on)
-{
- int err;
-
- if (mv88e6352_port_has_serdes(chip, port)) {
- err = mv88e6352_serdes_power_set(chip, on);
- if (err < 0)
- return err;
- }
-
- return 0;
-}
-
struct mv88e6352_serdes_hw_stat {
char string[ETH_GSTRING_LEN];
int sizeof_stat;
--
2.23.0
^ permalink raw reply related
* [PATCH net-next 07/10] net: dsa: mv88e6xxx: pass lane to .serdes_power
From: Vivien Didelot @ 2019-08-31 20:18 UTC (permalink / raw)
To: netdev; +Cc: davem, Marek Behún, f.fainelli, andrew, Vivien Didelot
In-Reply-To: <20190831201836.19957-1-vivien.didelot@gmail.com>
Now the first step of all .serdes_power implementations is getting
the lane mapping. Since we have an operation for that, call it in
the wrapper and pass the lane down to the .serdes_power operation.
This also allows to avoid querying the SERDES lane twice in
mv88e6xxx_port_set_cmode.
At the same time provide mv88e6xxx_serdes_power_{up,down} helpers
and prefer up/down instead of on/off as in the documentation.
Signed-off-by: Vivien Didelot <vivien.didelot@gmail.com>
---
drivers/net/dsa/mv88e6xxx/chip.c | 8 +++++---
drivers/net/dsa/mv88e6xxx/chip.h | 3 ++-
drivers/net/dsa/mv88e6xxx/port.c | 4 ++--
drivers/net/dsa/mv88e6xxx/serdes.c | 32 ++++++++++++------------------
drivers/net/dsa/mv88e6xxx/serdes.h | 24 ++++++++++++++++++++--
5 files changed, 44 insertions(+), 27 deletions(-)
diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index 3962e7368ae5..3522b11d5566 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -2057,13 +2057,15 @@ static int mv88e6xxx_setup_egress_floods(struct mv88e6xxx_chip *chip, int port)
static int mv88e6xxx_serdes_power(struct mv88e6xxx_chip *chip, int port,
bool on)
{
+ u8 lane;
int err;
- if (!chip->info->ops->serdes_power)
+ lane = mv88e6xxx_serdes_get_lane(chip, port);
+ if (!lane)
return 0;
if (on) {
- err = chip->info->ops->serdes_power(chip, port, true);
+ err = mv88e6xxx_serdes_power_up(chip, port, lane);
if (err)
return err;
@@ -2074,7 +2076,7 @@ static int mv88e6xxx_serdes_power(struct mv88e6xxx_chip *chip, int port,
chip->ports[port].serdes_irq)
chip->info->ops->serdes_irq_free(chip, port);
- err = chip->info->ops->serdes_power(chip, port, false);
+ err = mv88e6xxx_serdes_power_down(chip, port, lane);
}
return err;
diff --git a/drivers/net/dsa/mv88e6xxx/chip.h b/drivers/net/dsa/mv88e6xxx/chip.h
index add0ec5188ec..724ce2bf8258 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.h
+++ b/drivers/net/dsa/mv88e6xxx/chip.h
@@ -441,7 +441,8 @@ struct mv88e6xxx_ops {
int (*mgmt_rsvd2cpu)(struct mv88e6xxx_chip *chip);
/* Power on/off a SERDES interface */
- int (*serdes_power)(struct mv88e6xxx_chip *chip, int port, bool on);
+ int (*serdes_power)(struct mv88e6xxx_chip *chip, int port, u8 lane,
+ bool up);
/* SERDES lane mapping */
u8 (*serdes_get_lane)(struct mv88e6xxx_chip *chip, int port);
diff --git a/drivers/net/dsa/mv88e6xxx/port.c b/drivers/net/dsa/mv88e6xxx/port.c
index 06e2bdf6fa82..df71c08eda35 100644
--- a/drivers/net/dsa/mv88e6xxx/port.c
+++ b/drivers/net/dsa/mv88e6xxx/port.c
@@ -439,7 +439,7 @@ static int mv88e6xxx_port_set_cmode(struct mv88e6xxx_chip *chip, int port,
return err;
}
- err = mv88e6390_serdes_power(chip, port, false);
+ err = mv88e6xxx_serdes_power_down(chip, port, lane);
if (err)
return err;
}
@@ -464,7 +464,7 @@ static int mv88e6xxx_port_set_cmode(struct mv88e6xxx_chip *chip, int port,
if (!lane)
return -ENODEV;
- err = mv88e6390_serdes_power(chip, port, true);
+ err = mv88e6xxx_serdes_power_up(chip, port, lane);
if (err)
return err;
diff --git a/drivers/net/dsa/mv88e6xxx/serdes.c b/drivers/net/dsa/mv88e6xxx/serdes.c
index e8ad66987be9..e3ea8cca85b0 100644
--- a/drivers/net/dsa/mv88e6xxx/serdes.c
+++ b/drivers/net/dsa/mv88e6xxx/serdes.c
@@ -49,19 +49,17 @@ static int mv88e6390_serdes_write(struct mv88e6xxx_chip *chip,
return mv88e6xxx_phy_write(chip, lane, reg_c45, val);
}
-int mv88e6352_serdes_power(struct mv88e6xxx_chip *chip, int port, bool on)
+int mv88e6352_serdes_power(struct mv88e6xxx_chip *chip, int port, u8 lane,
+ bool up)
{
u16 val, new_val;
int err;
- if (!mv88e6xxx_serdes_get_lane(chip, port))
- return 0;
-
err = mv88e6352_serdes_read(chip, MII_BMCR, &val);
if (err)
return err;
- if (on)
+ if (up)
new_val = val & ~BMCR_PDOWN;
else
new_val = val | BMCR_PDOWN;
@@ -409,9 +407,9 @@ u8 mv88e6390x_serdes_get_lane(struct mv88e6xxx_chip *chip, int port)
return lane;
}
-/* Set the power on/off for 10GBASE-R and 10GBASE-X4/X2 */
+/* Set power up/down for 10GBASE-R and 10GBASE-X4/X2 */
static int mv88e6390_serdes_power_10g(struct mv88e6xxx_chip *chip, u8 lane,
- bool on)
+ bool up)
{
u16 val, new_val;
int err;
@@ -422,7 +420,7 @@ static int mv88e6390_serdes_power_10g(struct mv88e6xxx_chip *chip, u8 lane,
if (err)
return err;
- if (on)
+ if (up)
new_val = val & ~(MV88E6390_PCS_CONTROL_1_RESET |
MV88E6390_PCS_CONTROL_1_LOOPBACK |
MV88E6390_PCS_CONTROL_1_PDOWN);
@@ -436,9 +434,9 @@ static int mv88e6390_serdes_power_10g(struct mv88e6xxx_chip *chip, u8 lane,
return err;
}
-/* Set the power on/off for SGMII and 1000Base-X */
+/* Set power up/down for SGMII and 1000Base-X */
static int mv88e6390_serdes_power_sgmii(struct mv88e6xxx_chip *chip, u8 lane,
- bool on)
+ bool up)
{
u16 val, new_val;
int err;
@@ -448,7 +446,7 @@ static int mv88e6390_serdes_power_sgmii(struct mv88e6xxx_chip *chip, u8 lane,
if (err)
return err;
- if (on)
+ if (up)
new_val = val & ~(MV88E6390_SGMII_CONTROL_RESET |
MV88E6390_SGMII_CONTROL_LOOPBACK |
MV88E6390_SGMII_CONTROL_PDOWN);
@@ -462,23 +460,19 @@ static int mv88e6390_serdes_power_sgmii(struct mv88e6xxx_chip *chip, u8 lane,
return err;
}
-int mv88e6390_serdes_power(struct mv88e6xxx_chip *chip, int port, bool on)
+int mv88e6390_serdes_power(struct mv88e6xxx_chip *chip, int port, u8 lane,
+ bool up)
{
u8 cmode = chip->ports[port].cmode;
- u8 lane;
-
- lane = mv88e6xxx_serdes_get_lane(chip, port);
- if (!lane)
- return 0;
switch (cmode) {
case MV88E6XXX_PORT_STS_CMODE_SGMII:
case MV88E6XXX_PORT_STS_CMODE_1000BASEX:
case MV88E6XXX_PORT_STS_CMODE_2500BASEX:
- return mv88e6390_serdes_power_sgmii(chip, lane, on);
+ return mv88e6390_serdes_power_sgmii(chip, lane, up);
case MV88E6XXX_PORT_STS_CMODE_XAUI:
case MV88E6XXX_PORT_STS_CMODE_RXAUI:
- return mv88e6390_serdes_power_10g(chip, lane, on);
+ return mv88e6390_serdes_power_10g(chip, lane, up);
}
return 0;
diff --git a/drivers/net/dsa/mv88e6xxx/serdes.h b/drivers/net/dsa/mv88e6xxx/serdes.h
index 7df27a0de9aa..c70023f57090 100644
--- a/drivers/net/dsa/mv88e6xxx/serdes.h
+++ b/drivers/net/dsa/mv88e6xxx/serdes.h
@@ -82,8 +82,10 @@ unsigned int mv88e6352_serdes_irq_mapping(struct mv88e6xxx_chip *chip,
int port);
unsigned int mv88e6390_serdes_irq_mapping(struct mv88e6xxx_chip *chip,
int port);
-int mv88e6352_serdes_power(struct mv88e6xxx_chip *chip, int port, bool on);
-int mv88e6390_serdes_power(struct mv88e6xxx_chip *chip, int port, bool on);
+int mv88e6352_serdes_power(struct mv88e6xxx_chip *chip, int port, u8 lane,
+ bool on);
+int mv88e6390_serdes_power(struct mv88e6xxx_chip *chip, int port, u8 lane,
+ bool on);
int mv88e6390_serdes_irq_setup(struct mv88e6xxx_chip *chip, int port);
void mv88e6390_serdes_irq_free(struct mv88e6xxx_chip *chip, int port);
int mv88e6352_serdes_get_sset_count(struct mv88e6xxx_chip *chip, int port);
@@ -108,6 +110,24 @@ static inline u8 mv88e6xxx_serdes_get_lane(struct mv88e6xxx_chip *chip,
return chip->info->ops->serdes_get_lane(chip, port);
}
+static inline int mv88e6xxx_serdes_power_up(struct mv88e6xxx_chip *chip,
+ int port, u8 lane)
+{
+ if (!chip->info->ops->serdes_power)
+ return -EOPNOTSUPP;
+
+ return chip->info->ops->serdes_power(chip, port, lane, true);
+}
+
+static inline int mv88e6xxx_serdes_power_down(struct mv88e6xxx_chip *chip,
+ int port, u8 lane)
+{
+ if (!chip->info->ops->serdes_power)
+ return -EOPNOTSUPP;
+
+ return chip->info->ops->serdes_power(chip, port, lane, false);
+}
+
static inline unsigned int
mv88e6xxx_serdes_irq_mapping(struct mv88e6xxx_chip *chip, int port)
{
--
2.23.0
^ permalink raw reply related
* [PATCH net-next 05/10] net: dsa: mv88e6xxx: implement mv88e6352_serdes_get_lane
From: Vivien Didelot @ 2019-08-31 20:18 UTC (permalink / raw)
To: netdev; +Cc: davem, Marek Behún, f.fainelli, andrew, Vivien Didelot
In-Reply-To: <20190831201836.19957-1-vivien.didelot@gmail.com>
Even though 88E6352 has no dedicated lane for SERDES interfaces, it
uses a similar code as the other .serdes_get_lane implementations to
check the port's CMODE and ensure that SERDES operations are doable.
For consistency, implement mv88e6352_serdes_get_lane for the 88E6352
and similar switches which simply returns an unused 0xff lane address.
Signed-off-by: Vivien Didelot <vivien.didelot@gmail.com>
---
drivers/net/dsa/mv88e6xxx/chip.c | 4 ++++
drivers/net/dsa/mv88e6xxx/serdes.c | 11 ++++++++++-
drivers/net/dsa/mv88e6xxx/serdes.h | 1 +
3 files changed, 15 insertions(+), 1 deletion(-)
diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index 0ab4ce86eda7..3962e7368ae5 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -3093,6 +3093,7 @@ static const struct mv88e6xxx_ops mv88e6172_ops = {
.rmu_disable = mv88e6352_g1_rmu_disable,
.vtu_getnext = mv88e6352_g1_vtu_getnext,
.vtu_loadpurge = mv88e6352_g1_vtu_loadpurge,
+ .serdes_get_lane = mv88e6352_serdes_get_lane,
.serdes_power = mv88e6352_serdes_power,
.gpio_ops = &mv88e6352_gpio_ops,
.phylink_validate = mv88e6352_phylink_validate,
@@ -3178,6 +3179,7 @@ static const struct mv88e6xxx_ops mv88e6176_ops = {
.rmu_disable = mv88e6352_g1_rmu_disable,
.vtu_getnext = mv88e6352_g1_vtu_getnext,
.vtu_loadpurge = mv88e6352_g1_vtu_loadpurge,
+ .serdes_get_lane = mv88e6352_serdes_get_lane,
.serdes_power = mv88e6352_serdes_power,
.serdes_irq_mapping = mv88e6352_serdes_irq_mapping,
.serdes_irq_setup = mv88e6352_serdes_irq_setup,
@@ -3407,6 +3409,7 @@ static const struct mv88e6xxx_ops mv88e6240_ops = {
.rmu_disable = mv88e6352_g1_rmu_disable,
.vtu_getnext = mv88e6352_g1_vtu_getnext,
.vtu_loadpurge = mv88e6352_g1_vtu_loadpurge,
+ .serdes_get_lane = mv88e6352_serdes_get_lane,
.serdes_power = mv88e6352_serdes_power,
.serdes_irq_mapping = mv88e6352_serdes_irq_mapping,
.serdes_irq_setup = mv88e6352_serdes_irq_setup,
@@ -3767,6 +3770,7 @@ static const struct mv88e6xxx_ops mv88e6352_ops = {
.rmu_disable = mv88e6352_g1_rmu_disable,
.vtu_getnext = mv88e6352_g1_vtu_getnext,
.vtu_loadpurge = mv88e6352_g1_vtu_loadpurge,
+ .serdes_get_lane = mv88e6352_serdes_get_lane,
.serdes_power = mv88e6352_serdes_power,
.serdes_irq_mapping = mv88e6352_serdes_irq_mapping,
.serdes_irq_setup = mv88e6352_serdes_irq_setup,
diff --git a/drivers/net/dsa/mv88e6xxx/serdes.c b/drivers/net/dsa/mv88e6xxx/serdes.c
index ce6d97e5caf8..9fb2773a3eb5 100644
--- a/drivers/net/dsa/mv88e6xxx/serdes.c
+++ b/drivers/net/dsa/mv88e6xxx/serdes.c
@@ -69,13 +69,22 @@ static int mv88e6352_serdes_power_set(struct mv88e6xxx_chip *chip, bool on)
return err;
}
-static bool mv88e6352_port_has_serdes(struct mv88e6xxx_chip *chip, int port)
+u8 mv88e6352_serdes_get_lane(struct mv88e6xxx_chip *chip, int port)
{
u8 cmode = chip->ports[port].cmode;
+ u8 lane = 0;
if ((cmode == MV88E6XXX_PORT_STS_CMODE_100BASEX) ||
(cmode == MV88E6XXX_PORT_STS_CMODE_1000BASEX) ||
(cmode == MV88E6XXX_PORT_STS_CMODE_SGMII))
+ lane = 0xff; /* Unused */
+
+ return lane;
+}
+
+static bool mv88e6352_port_has_serdes(struct mv88e6xxx_chip *chip, int port)
+{
+ if (mv88e6xxx_serdes_get_lane(chip, port))
return true;
return false;
diff --git a/drivers/net/dsa/mv88e6xxx/serdes.h b/drivers/net/dsa/mv88e6xxx/serdes.h
index 4718dcca6b3c..7df27a0de9aa 100644
--- a/drivers/net/dsa/mv88e6xxx/serdes.h
+++ b/drivers/net/dsa/mv88e6xxx/serdes.h
@@ -75,6 +75,7 @@
#define MV88E6390_SGMII_PHY_STATUS_LINK BIT(10)
u8 mv88e6341_serdes_get_lane(struct mv88e6xxx_chip *chip, int port);
+u8 mv88e6352_serdes_get_lane(struct mv88e6xxx_chip *chip, int port);
u8 mv88e6390_serdes_get_lane(struct mv88e6xxx_chip *chip, int port);
u8 mv88e6390x_serdes_get_lane(struct mv88e6xxx_chip *chip, int port);
unsigned int mv88e6352_serdes_irq_mapping(struct mv88e6xxx_chip *chip,
--
2.23.0
^ permalink raw reply related
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