* Re: [patch net-next 6/8] ipv4: fib: Add an API to request a FIB dump
From: Ido Schimmel @ 2016-11-17 13:10 UTC (permalink / raw)
To: Hannes Frederic Sowa
Cc: Jiri Pirko, netdev, davem, idosch, eladr, yotamg, nogahf, arkadis,
ogerlitz, roopa, dsa, nikolay, andy, vivien.didelot, andrew,
f.fainelli, alexander.h.duyck, kuznet, jmorris, yoshfuji, kaber
In-Reply-To: <56d2179d-00ff-bcc5-e365-845179cbe672@stressinduktion.org>
Hi Hannes,
On Wed, Nov 16, 2016 at 08:43:25PM +0100, Hannes Frederic Sowa wrote:
> On 16.11.2016 19:51, Ido Schimmel wrote:
> > On Wed, Nov 16, 2016 at 06:35:45PM +0100, Hannes Frederic Sowa wrote:
> >> On 16.11.2016 16:18, Ido Schimmel wrote:
> >>> On Wed, Nov 16, 2016 at 03:51:01PM +0100, Hannes Frederic Sowa wrote:
> >>>> On 16.11.2016 15:09, Jiri Pirko wrote:
> >>>>> From: Ido Schimmel <idosch@mellanox.com>
> >>>>>
> >>>>> Commit b90eb7549499 ("fib: introduce FIB notification infrastructure")
> >>>>> introduced a new notification chain to notify listeners (f.e., switchdev
> >>>>> drivers) about addition and deletion of routes.
> >>>>>
> >>>>> However, upon registration to the chain the FIB tables can already be
> >>>>> populated, which means potential listeners will have an incomplete view
> >>>>> of the tables.
> >>>>>
> >>>>> Solve that by adding an API to request a FIB dump. The dump itself it
> >>>>> done using RCU in order not to starve consumers that need RTNL to make
> >>>>> progress.
> >>>>>
> >>>>> Signed-off-by: Ido Schimmel <idosch@mellanox.com>
> >>>>> Signed-off-by: Jiri Pirko <jiri@mellanox.com>
> >>>>
> >>>> Have you looked at potential inconsistencies resulting of RCU walking
> >>>> the table and having concurrent inserts?
> >>>
> >>> Yes. I did try to think about situations in which this approach will
> >>> fail, but I could only find problems with concurrent removals, which I
> >>> addressed in 5/8. In case of concurrent insertions, even if you missed
> >>> the node, you would still get the ENTRY_ADD event to your listener.
> >>
> >> Theoretically a node could still be installed while the deletion event
> >> fired before registering the notifier. E.g. a synchronize_net before
> >> dumping could help here?
> >
> > If the deletion event fired for some fib alias, then by 5/8 we are
> > guaranteed that it was already unlinked from the fib alias list in the
> > leaf in which it was contained. So, while it's possible we didn't
> > register our listener in time for the deletion event, we won't traverse
> > this fib alias while dumping the trie anyway. Did I understand you
> > correctly?
> >
>
> Theoretically we can have the same problem for insertion:
>
> You receive a delete event from the notifier that is queued up first but
> the dump will still see the entry in the fib due to being managed by RCU
> (the notifier running on another CPU).
>
> The problem is that the fib_remove_alias->hlist_del_rcu->WRITE_ONCE is
> still not strongly ordered against the local fib dump trie walk.
OK, so I believe my analysis in 5/8 was wrong. Despite CPU A invoking
fib_remove_alias() in t0 for fa1, it's possible for CPU B doing the fib
dump to see fa1 in t1, which will lead to fa1 being permanently present
in the listener's table.
Given the above, I think Dave's suggestion is the only applicable
solution. Do you agree? Any other suggestions?
Thanks
^ permalink raw reply
* Re: [PATCH net 0/3] net: phy: fix of_node and device leaks
From: David Miller @ 2016-11-17 17:05 UTC (permalink / raw)
To: johan; +Cc: f.fainelli, robh+dt, frowand.list, netdev, linux-kernel
In-Reply-To: <1479306038-27211-1-git-send-email-johan@kernel.org>
From: Johan Hovold <johan@kernel.org>
Date: Wed, 16 Nov 2016 15:20:35 +0100
> These patches fix a couple of of_node leaks in the fixed-link code and a
> device reference leak in a phy helper.
Series applied, thanks.
^ permalink raw reply
* Re: [PATCH net v2 7/7] net: ethernet: ti: cpsw: fix fixed-link phy probe deferral
From: David Miller @ 2016-11-17 17:04 UTC (permalink / raw)
To: johan; +Cc: mugunthanvnm, grygorii.strashko, linux-omap, netdev, linux-kernel
In-Reply-To: <1479400804-9847-8-git-send-email-johan@kernel.org>
From: Johan Hovold <johan@kernel.org>
Date: Thu, 17 Nov 2016 17:40:04 +0100
> Make sure to propagate errors from of_phy_register_fixed_link() which
> can fail with -EPROBE_DEFER.
>
> Fixes: 1f71e8c96fc6 ("drivers: net: cpsw: Add support for fixed-link
> PHY")
> Signed-off-by: Johan Hovold <johan@kernel.org>
Johan, when you update a patch within a series you must post the
entire series freshly to the lists, cover posting and all.
Thank you.
^ permalink raw reply
* Re: [PATCH net-next v2 3/3] net: marvell: Allow drivers to be built with COMPILE_TEST
From: Florian Fainelli @ 2016-11-17 16:52 UTC (permalink / raw)
To: Arnd Bergmann, kbuild test robot
Cc: kbuild-all, netdev, davem, mw, gregory.clement, Shaohui.Xie
In-Reply-To: <19888267.TNEXR1Cft3@wuerfel>
On 11/17/2016 01:12 AM, Arnd Bergmann wrote:
> On Thursday, November 17, 2016 3:10:10 PM CET kbuild test robot wrote:
>> url: https://github.com/0day-ci/linux/commits/Florian-Fainelli/net-fsl-Allow-most-drivers-to-be-built-with-COMPILE_TEST/20161116-094841
>> config: mips-allyesconfig (attached as .config)
>> compiler: mips-linux-gnu-gcc (Debian 6.1.1-9) 6.1.1 20160705
>> reproduce:
>> wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
>> chmod +x ~/bin/make.cross
>> # save the attached .config to linux build tree
>> make.cross ARCH=mips
>>
>> All errors (new ones prefixed by >>):
>>
>> drivers/built-in.o: In function `mvneta_bm_pool_create':
>>>> mvneta_bm.c:(.text+0x772f0c): undefined reference to `mvebu_mbus_get_dram_win_info'
>
> We should fix this, I'd suggest either adding an inline wrapper for
> mvebu_mbus_get_dram_win_info() when CONFIG_MVEBU_MBUS is not set,
> or allowing CONFIG_MVEBU_MBUS to be enabled for COMPILE_TEST as well.
Yes, I have a patch prepared for that, thanks!
--
Florian
^ permalink raw reply
* Re: [patch net-next 6/8] ipv4: fib: Add an API to request a FIB dump
From: David Miller @ 2016-11-17 16:45 UTC (permalink / raw)
To: hannes
Cc: idosch, jiri, netdev, idosch, eladr, yotamg, nogahf, arkadis,
ogerlitz, roopa, dsa, nikolay, andy, vivien.didelot, andrew,
f.fainelli, alexander.h.duyck, kuznet, jmorris, yoshfuji, kaber
In-Reply-To: <e2e0eaaf-30f0-e825-29c7-920115742301@stressinduktion.org>
From: Hannes Frederic Sowa <hannes@stressinduktion.org>
Date: Thu, 17 Nov 2016 15:36:48 +0100
> The other way is the journal idea I had, which uses an rb-tree with
> timestamps as keys (can be lamport timestamps). You insert into the tree
> until the dump is finished and use it as queue later to shuffle stuff
> into the hardware.
If you have this "place" where pending inserts are stored, you have
a policy decision to make.
First of all what do other lookups see when there are pending entires?
If, once inserted into the pending queue, you return success to the
inserting entity, then you must make those pending entries visible
to lookups.
If you block the inserting entity, well that doesn't make a lot of
sense. If blocking is a workable solution, then we can just block the
entire insert while this FIB dump to the device driver is happening.
But I am pretty sure the idea of blocking modifications for so long
was considered undesirable.
^ permalink raw reply
* Re: [PATCH] net: dsa: fix fixed-link-phy device leaks
From: Johan Hovold @ 2016-11-17 16:47 UTC (permalink / raw)
To: Florian Fainelli
Cc: Johan Hovold, Andrew Lunn, Vivien Didelot, David S. Miller,
netdev, linux-kernel
In-Reply-To: <20161117095225.GC21237@localhost>
On Thu, Nov 17, 2016 at 10:52:25AM +0100, Johan Hovold wrote:
> On Wed, Nov 16, 2016 at 10:14:10AM -0800, Florian Fainelli wrote:
> > On 11/16/2016 09:11 AM, Johan Hovold wrote:
> > > On Wed, Nov 16, 2016 at 09:06:26AM -0800, Florian Fainelli wrote:
> > >> On 11/16/2016 06:47 AM, Johan Hovold wrote:
> > >>> Make sure to drop the reference taken by of_phy_find_device() when
> > >>> registering and deregistering the fixed-link PHY-device.
> > >>>
> > >>> Note that we need to put both references held at deregistration.
> > >>>
> > >>> Fixes: 39b0c705195e ("net: dsa: Allow configuration of CPU & DSA port
> > >>> speeds/duplex")
> > >>> Signed-off-by: Johan Hovold <johan@kernel.org>
> > >>> ---
> > >>>
> > >>> Hi,
> > >>>
> > >>> This is one has been compile tested only, but fixes a couple of leaks
> > >>> similar to one that was found in the cpsw driver for which I just posted
> > >>> a patch.
> > >>>
> > >>> It turns out all drivers but DSA fail to deregister the fixed-link PHYs
> > >>> registered by of_phy_register_fixed_link(). Due to the way this
> > >>> interface was designed, deregistering such a PHY is a bit cumbersome and
> > >>> looks like it would benefit from a common helper.
> > >>>
> > >>> However, perhaps the interface should instead be changed so that the PHY
> > >>> device is returned so that drivers do not need to use
> > >>> of_phy_find_device() when they need to access properties of the fixed
> > >>> link (e.g. as in dsu_cpu_dsa_setup below).
> > >>>
> > >>> Thoughts?
> > >>>
> > >>> Thanks,
> > >>> Johan
> > >>>
> > >>>
> > >>> net/dsa/dsa.c | 8 +++++++-
> > >>> 1 file changed, 7 insertions(+), 1 deletion(-)
> > >>>
> > >>> diff --git a/net/dsa/dsa.c b/net/dsa/dsa.c
> > >>> index a6902c1e2f28..798a6a776a5f 100644
> > >>> --- a/net/dsa/dsa.c
> > >>> +++ b/net/dsa/dsa.c
> > >>> @@ -233,6 +233,8 @@ int dsa_cpu_dsa_setup(struct dsa_switch *ds, struct device *dev,
> > >>> genphy_read_status(phydev);
> > >>> if (ds->ops->adjust_link)
> > >>> ds->ops->adjust_link(ds, port, phydev);
> > >>> +
> > >>> + phy_device_free(phydev); /* of_phy_find_device */
> > >>> }
> > >>>
> > >>> return 0;
> > >>> @@ -509,8 +511,12 @@ void dsa_cpu_dsa_destroy(struct device_node *port_dn)
> > >>> if (of_phy_is_fixed_link(port_dn)) {
> > >>> phydev = of_phy_find_device(port_dn);
> > >>> if (phydev) {
> > >>> - phy_device_free(phydev);
> > >>> fixed_phy_unregister(phydev);
> > >>> + /* Put references taken by of_phy_find_device() and
> > >>> + * of_phy_register_fixed_link().
> > >>> + */
> > >>> + phy_device_free(phydev);
> > >>> + phy_device_free(phydev);
> > >>
> > >> Double free, this looks bogus here. Actually would not this mean a
> > >> triple free since you already free in dsa_cpu_dsa_setup() which is
> > >> paired with dsa_cpu_dsa_destroy()?
> > >
> > > The naming of phy_device_free() is unfortunate when it's really a put():
> > >
> > > void phy_device_free(struct phy_device *phydev)
> > > {
> > > put_device(&phydev->mdio.dev);
> > > }
> >
> > Indeed, should have looked a little harder.
> >
> > >
> > > which may need to be called multiple times, specifically after a call to
> > > of_phy_find_device() which takes another reference.
> > >
> > > With this patch the refcounts are properly balanced.
> >
> > The intent of your patch is good, but it still feels like having to
> > double imbalance the refcount is symptomatic of a larger issue here, it
> > does not seem like having several refcounts are necessary, so we may
> > really want to rework the API.
>
> I'll cook something up for -next, but how about using
>
> put_device(&phydev->mdio.dev)
>
> for the reference taken by of_phy_find_device() (as some driver
> already do) to fix the immediate leaks?
>
> Then deregistration would look like:
>
> phydev = of_phy_find_device(port_dn);
> if (phydev) {
> - phy_device_free(phydev);
> fixed_phy_unregister(phydev);
> + put_device(&phydev->mdio.dev)
> + phy_device_free(phydev);
On second thought, I suggest just renaming the phy_device_free() as
phy_device_put() to reflect what it really does these days. There can
never be a guarantee that the phy is freed on a call to
phy_device_free() anyway.
The OF interface can still be modified to return a pointer later.
Johan
^ permalink raw reply
* Re: [PATCH net-next] net/mlx4_en: remove napi_hash_del() call
From: David Miller @ 2016-11-17 16:17 UTC (permalink / raw)
To: eric.dumazet; +Cc: netdev, tariqt
In-Reply-To: <1479304162.8455.163.camel@edumazet-glaptop3.roam.corp.google.com>
From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Wed, 16 Nov 2016 05:49:22 -0800
> From: Eric Dumazet <edumazet@google.com>
>
> There is no need calling napi_hash_del()+synchronize_rcu() before
> calling netif_napi_del()
>
> netif_napi_del() does this already.
>
> Using napi_hash_del() in a driver is useful only when dealing with
> a batch of NAPI structures, so that a single synchronize_rcu() can
> be used. mlx4_en_deactivate_cq() is deactivating a single NAPI.
>
> Signed-off-by: Eric Dumazet <edumazet@google.com>
Applied, thanks Eric.
^ permalink raw reply
* Re: [Patch net] net: check dead netns for peernet2id_alloc()
From: David Miller @ 2016-11-17 16:20 UTC (permalink / raw)
To: xiyou.wangcong; +Cc: netdev, avagin, nicolas.dichtel
In-Reply-To: <1479320822-17483-1-git-send-email-xiyou.wangcong@gmail.com>
From: Cong Wang <xiyou.wangcong@gmail.com>
Date: Wed, 16 Nov 2016 10:27:02 -0800
> Andrei reports we still allocate netns ID from idr after we destroy
> it in cleanup_net().
>
> cleanup_net():
> ...
> idr_destroy(&net->netns_ids);
> ...
> list_for_each_entry_reverse(ops, &pernet_list, list)
> ops_exit_list(ops, &net_exit_list);
> -> rollback_registered_many()
> -> rtmsg_ifinfo_build_skb()
> -> rtnl_fill_ifinfo()
> -> peernet2id_alloc()
>
> After that point we should not even access net->netns_ids, we
> should check the death of the current netns as early as we can in
> peernet2id_alloc().
>
> For net-next we can consider to avoid sending rtmsg totally,
> it is a good optimization for netns teardown path.
>
> Fixes: 0c7aecd4bde4 ("netns: add rtnl cmd to add and get peer netns ids")
> Reported-by: Andrei Vagin <avagin@gmail.com>
> Cc: Nicolas Dichtel <nicolas.dichtel@6wind.com>
> Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
Applied and queued up for -stable, thanks.
^ permalink raw reply
* [PATCH net v2 7/7] net: ethernet: ti: cpsw: fix fixed-link phy probe deferral
From: Johan Hovold @ 2016-11-17 16:40 UTC (permalink / raw)
To: Mugunthan V N
Cc: Grygorii Strashko, linux-omap, netdev, linux-kernel, Johan Hovold
In-Reply-To: <1479400804-9847-1-git-send-email-johan@kernel.org>
Make sure to propagate errors from of_phy_register_fixed_link() which
can fail with -EPROBE_DEFER.
Fixes: 1f71e8c96fc6 ("drivers: net: cpsw: Add support for fixed-link
PHY")
Signed-off-by: Johan Hovold <johan@kernel.org>
---
drivers/net/ethernet/ti/cpsw.c | 12 +++++++-----
1 file changed, 7 insertions(+), 5 deletions(-)
diff --git a/drivers/net/ethernet/ti/cpsw.c b/drivers/net/ethernet/ti/cpsw.c
index 1387299030e4..58947aae31c7 100644
--- a/drivers/net/ethernet/ti/cpsw.c
+++ b/drivers/net/ethernet/ti/cpsw.c
@@ -2375,8 +2375,11 @@ static int cpsw_probe_dt(struct cpsw_platform_data *data,
* to the PHY is the Ethernet MAC DT node.
*/
ret = of_phy_register_fixed_link(slave_node);
- if (ret)
+ if (ret) {
+ if (ret != -EPROBE_DEFER)
+ dev_err(&pdev->dev, "failed to register fixed-link phy: %d\n", ret);
return ret;
+ }
slave_data->phy_node = of_node_get(slave_node);
} else if (parp) {
u32 phyid;
@@ -2637,11 +2640,10 @@ static int cpsw_probe(struct platform_device *pdev)
goto clean_runtime_disable_ret;
}
- if (cpsw_probe_dt(&cpsw->data, pdev)) {
- dev_err(&pdev->dev, "cpsw: platform data missing\n");
- ret = -ENODEV;
+ ret = cpsw_probe_dt(&cpsw->data, pdev);
+ if (ret)
goto clean_dt_ret;
- }
+
data = &cpsw->data;
cpsw->rx_ch_num = 1;
cpsw->tx_ch_num = 1;
--
2.7.3
^ permalink raw reply related
* [PATCH net v2 6/7] net: ethernet: ti: cpsw: add missing sanity check
From: Johan Hovold @ 2016-11-17 16:40 UTC (permalink / raw)
To: Mugunthan V N
Cc: Grygorii Strashko, linux-omap, netdev, linux-kernel, Johan Hovold
In-Reply-To: <1479400804-9847-1-git-send-email-johan@kernel.org>
Make sure to check for allocation failures before dereferencing a
NULL-pointer during probe.
Fixes: 649a1688c960 ("net: ethernet: ti: cpsw: create common struct to
hold shared driver data")
Signed-off-by: Johan Hovold <johan@kernel.org>
---
drivers/net/ethernet/ti/cpsw.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/net/ethernet/ti/cpsw.c b/drivers/net/ethernet/ti/cpsw.c
index 11b2daef3158..1387299030e4 100644
--- a/drivers/net/ethernet/ti/cpsw.c
+++ b/drivers/net/ethernet/ti/cpsw.c
@@ -2588,6 +2588,9 @@ static int cpsw_probe(struct platform_device *pdev)
int irq;
cpsw = devm_kzalloc(&pdev->dev, sizeof(struct cpsw_common), GFP_KERNEL);
+ if (!cpsw)
+ return -ENOMEM;
+
cpsw->dev = &pdev->dev;
ndev = alloc_etherdev_mq(sizeof(struct cpsw_priv), CPSW_MAX_QUEUES);
--
2.7.3
^ permalink raw reply related
* [PATCH net v2 5/7] net: ethernet: ti: cpsw: fix secondary-emac probe error path
From: Johan Hovold @ 2016-11-17 16:40 UTC (permalink / raw)
To: Mugunthan V N
Cc: Grygorii Strashko, linux-omap, netdev, linux-kernel, Johan Hovold
In-Reply-To: <1479400804-9847-1-git-send-email-johan@kernel.org>
Make sure to deregister the primary device in case the secondary emac
fails to probe.
kernel BUG at /home/johan/work/omicron/src/linux/net/core/dev.c:7743!
...
[<c05b3dec>] (free_netdev) from [<c04fe6c0>] (cpsw_probe+0x9cc/0xe50)
[<c04fe6c0>] (cpsw_probe) from [<c047b28c>] (platform_drv_probe+0x5c/0xc0)
Fixes: d9ba8f9e6298 ("driver: net: ethernet: cpsw: dual emac interface
implementation")
Signed-off-by: Johan Hovold <johan@kernel.org>
---
drivers/net/ethernet/ti/cpsw.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/ti/cpsw.c b/drivers/net/ethernet/ti/cpsw.c
index c3b78bc4fe58..11b2daef3158 100644
--- a/drivers/net/ethernet/ti/cpsw.c
+++ b/drivers/net/ethernet/ti/cpsw.c
@@ -2852,7 +2852,7 @@ static int cpsw_probe(struct platform_device *pdev)
ret = cpsw_probe_dual_emac(priv);
if (ret) {
cpsw_err(priv, probe, "error probe slave 2 emac interface\n");
- goto clean_ale_ret;
+ goto clean_unregister_netdev_ret;
}
}
@@ -2860,6 +2860,8 @@ static int cpsw_probe(struct platform_device *pdev)
return 0;
+clean_unregister_netdev_ret:
+ unregister_netdev(ndev);
clean_ale_ret:
cpsw_ale_destroy(cpsw->ale);
clean_dma_ret:
--
2.7.3
^ permalink raw reply related
* [PATCH net v2 4/7] net: ethernet: ti: cpsw: fix of_node and phydev leaks
From: Johan Hovold @ 2016-11-17 16:40 UTC (permalink / raw)
To: Mugunthan V N
Cc: Grygorii Strashko, linux-omap, netdev, linux-kernel, Johan Hovold
In-Reply-To: <1479400804-9847-1-git-send-email-johan@kernel.org>
Make sure to drop references taken and deregister devices registered
during probe on probe errors (including deferred probe) and driver
unbind.
Specifically, PHY of-node references were never released and fixed-link
PHY devices were never deregistered.
Fixes: 9e42f715264f ("drivers: net: cpsw: add phy-handle parsing")
Fixes: 1f71e8c96fc6 ("drivers: net: cpsw: Add support for fixed-link
PHY")
Signed-off-by: Johan Hovold <johan@kernel.org>
---
drivers/net/ethernet/ti/cpsw.c | 35 +++++++++++++++++++++++++++++++++++
1 file changed, 35 insertions(+)
diff --git a/drivers/net/ethernet/ti/cpsw.c b/drivers/net/ethernet/ti/cpsw.c
index 5d14abb06486..c3b78bc4fe58 100644
--- a/drivers/net/ethernet/ti/cpsw.c
+++ b/drivers/net/ethernet/ti/cpsw.c
@@ -2443,6 +2443,41 @@ static int cpsw_probe_dt(struct cpsw_platform_data *data,
static void cpsw_remove_dt(struct platform_device *pdev)
{
+ struct net_device *ndev = platform_get_drvdata(pdev);
+ struct cpsw_common *cpsw = ndev_to_cpsw(ndev);
+ struct cpsw_platform_data *data = &cpsw->data;
+ struct device_node *node = pdev->dev.of_node;
+ struct device_node *slave_node;
+ int i = 0;
+
+ for_each_available_child_of_node(node, slave_node) {
+ struct cpsw_slave_data *slave_data = &data->slave_data[i];
+
+ if (strcmp(slave_node->name, "slave"))
+ continue;
+
+ if (of_phy_is_fixed_link(slave_node)) {
+ struct phy_device *phydev;
+
+ phydev = of_phy_find_device(slave_node);
+ if (phydev) {
+ fixed_phy_unregister(phydev);
+ /* Put references taken by
+ * of_phy_find_device() and
+ * of_phy_register_fixed_link().
+ */
+ phy_device_free(phydev);
+ phy_device_free(phydev);
+ }
+ }
+
+ of_node_put(slave_data->phy_node);
+
+ i++;
+ if (i == data->slaves)
+ break;
+ }
+
of_platform_depopulate(&pdev->dev);
}
--
2.7.3
^ permalink raw reply related
* [PATCH net v2 3/7] net: ethernet: ti: cpsw: fix deferred probe
From: Johan Hovold @ 2016-11-17 16:40 UTC (permalink / raw)
To: Mugunthan V N
Cc: Grygorii Strashko, linux-omap, netdev, linux-kernel, Johan Hovold
In-Reply-To: <1479400804-9847-1-git-send-email-johan@kernel.org>
Make sure to deregister all child devices also on probe errors to avoid
leaks and to fix probe deferral:
cpsw 4a100000.ethernet: omap_device: omap_device_enable() called from invalid state 1
cpsw 4a100000.ethernet: use pm_runtime_put_sync_suspend() in driver?
cpsw: probe of 4a100000.ethernet failed with error -22
Add generic helper to undo the effects of cpsw_probe_dt(), which will
also be used in a follow-on patch to fix further leaks that have been
introduced more recently.
Note that the platform device is now runtime-resumed before registering
any child devices in order to make sure that it is synchronously
suspended after having deregistered the children in the error path.
Fixes: 1fb19aa730e4 ("net: cpsw: Add parent<->child relation support
between cpsw and mdio")
Signed-off-by: Johan Hovold <johan@kernel.org>
---
drivers/net/ethernet/ti/cpsw.c | 41 ++++++++++++++++++++++++-----------------
1 file changed, 24 insertions(+), 17 deletions(-)
diff --git a/drivers/net/ethernet/ti/cpsw.c b/drivers/net/ethernet/ti/cpsw.c
index 84c5d214557e..5d14abb06486 100644
--- a/drivers/net/ethernet/ti/cpsw.c
+++ b/drivers/net/ethernet/ti/cpsw.c
@@ -2441,6 +2441,11 @@ static int cpsw_probe_dt(struct cpsw_platform_data *data,
return 0;
}
+static void cpsw_remove_dt(struct platform_device *pdev)
+{
+ of_platform_depopulate(&pdev->dev);
+}
+
static int cpsw_probe_dual_emac(struct cpsw_priv *priv)
{
struct cpsw_common *cpsw = priv->cpsw;
@@ -2585,10 +2590,19 @@ static int cpsw_probe(struct platform_device *pdev)
/* Select default pin state */
pinctrl_pm_select_default_state(&pdev->dev);
+ /* Need to enable clocks with runtime PM api to access module
+ * registers
+ */
+ ret = pm_runtime_get_sync(&pdev->dev);
+ if (ret < 0) {
+ pm_runtime_put_noidle(&pdev->dev);
+ goto clean_runtime_disable_ret;
+ }
+
if (cpsw_probe_dt(&cpsw->data, pdev)) {
dev_err(&pdev->dev, "cpsw: platform data missing\n");
ret = -ENODEV;
- goto clean_runtime_disable_ret;
+ goto clean_dt_ret;
}
data = &cpsw->data;
cpsw->rx_ch_num = 1;
@@ -2609,7 +2623,7 @@ static int cpsw_probe(struct platform_device *pdev)
GFP_KERNEL);
if (!cpsw->slaves) {
ret = -ENOMEM;
- goto clean_runtime_disable_ret;
+ goto clean_dt_ret;
}
for (i = 0; i < data->slaves; i++)
cpsw->slaves[i].slave_num = i;
@@ -2621,7 +2635,7 @@ static int cpsw_probe(struct platform_device *pdev)
if (IS_ERR(clk)) {
dev_err(priv->dev, "fck is not found\n");
ret = -ENODEV;
- goto clean_runtime_disable_ret;
+ goto clean_dt_ret;
}
cpsw->bus_freq_mhz = clk_get_rate(clk) / 1000000;
@@ -2629,25 +2643,17 @@ static int cpsw_probe(struct platform_device *pdev)
ss_regs = devm_ioremap_resource(&pdev->dev, ss_res);
if (IS_ERR(ss_regs)) {
ret = PTR_ERR(ss_regs);
- goto clean_runtime_disable_ret;
+ goto clean_dt_ret;
}
cpsw->regs = ss_regs;
- /* Need to enable clocks with runtime PM api to access module
- * registers
- */
- ret = pm_runtime_get_sync(&pdev->dev);
- if (ret < 0) {
- pm_runtime_put_noidle(&pdev->dev);
- goto clean_runtime_disable_ret;
- }
cpsw->version = readl(&cpsw->regs->id_ver);
res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
cpsw->wr_regs = devm_ioremap_resource(&pdev->dev, res);
if (IS_ERR(cpsw->wr_regs)) {
ret = PTR_ERR(cpsw->wr_regs);
- goto clean_pm_runtime_put_ret;
+ goto clean_dt_ret;
}
memset(&dma_params, 0, sizeof(dma_params));
@@ -2684,7 +2690,7 @@ static int cpsw_probe(struct platform_device *pdev)
default:
dev_err(priv->dev, "unknown version 0x%08x\n", cpsw->version);
ret = -ENODEV;
- goto clean_pm_runtime_put_ret;
+ goto clean_dt_ret;
}
for (i = 0; i < cpsw->data.slaves; i++) {
struct cpsw_slave *slave = &cpsw->slaves[i];
@@ -2713,7 +2719,7 @@ static int cpsw_probe(struct platform_device *pdev)
if (!cpsw->dma) {
dev_err(priv->dev, "error initializing dma\n");
ret = -ENOMEM;
- goto clean_pm_runtime_put_ret;
+ goto clean_dt_ret;
}
cpsw->txch[0] = cpdma_chan_create(cpsw->dma, 0, cpsw_tx_handler, 0);
@@ -2823,7 +2829,8 @@ static int cpsw_probe(struct platform_device *pdev)
cpsw_ale_destroy(cpsw->ale);
clean_dma_ret:
cpdma_ctlr_destroy(cpsw->dma);
-clean_pm_runtime_put_ret:
+clean_dt_ret:
+ cpsw_remove_dt(pdev);
pm_runtime_put_sync(&pdev->dev);
clean_runtime_disable_ret:
pm_runtime_disable(&pdev->dev);
@@ -2850,7 +2857,7 @@ static int cpsw_remove(struct platform_device *pdev)
cpsw_ale_destroy(cpsw->ale);
cpdma_ctlr_destroy(cpsw->dma);
- of_platform_depopulate(&pdev->dev);
+ cpsw_remove_dt(pdev);
pm_runtime_put_sync(&pdev->dev);
pm_runtime_disable(&pdev->dev);
if (cpsw->data.dual_emac)
--
2.7.3
^ permalink raw reply related
* [PATCH net v2 2/7] net: ethernet: ti: cpsw: fix mdio device reference leak
From: Johan Hovold @ 2016-11-17 16:39 UTC (permalink / raw)
To: Mugunthan V N
Cc: Grygorii Strashko, linux-omap, netdev, linux-kernel, Johan Hovold
In-Reply-To: <1479400804-9847-1-git-send-email-johan@kernel.org>
Make sure to drop the reference taken by of_find_device_by_node() when
looking up an mdio device from a phy_id property during probe.
Fixes: 549985ee9c72 ("cpsw: simplify the setup of the register
pointers")
Signed-off-by: Johan Hovold <johan@kernel.org>
---
drivers/net/ethernet/ti/cpsw.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/net/ethernet/ti/cpsw.c b/drivers/net/ethernet/ti/cpsw.c
index f60f8ab7c1e3..84c5d214557e 100644
--- a/drivers/net/ethernet/ti/cpsw.c
+++ b/drivers/net/ethernet/ti/cpsw.c
@@ -2397,6 +2397,7 @@ static int cpsw_probe_dt(struct cpsw_platform_data *data,
}
snprintf(slave_data->phy_id, sizeof(slave_data->phy_id),
PHY_ID_FMT, mdio->name, phyid);
+ put_device(&mdio->dev);
} else {
dev_err(&pdev->dev,
"No slave[%d] phy_id, phy-handle, or fixed-link property\n",
--
2.7.3
^ permalink raw reply related
* [PATCH net v2 1/7] net: ethernet: ti: cpsw: fix bad register access in probe error path
From: Johan Hovold @ 2016-11-17 16:39 UTC (permalink / raw)
To: Mugunthan V N
Cc: Grygorii Strashko, linux-omap, netdev, linux-kernel, Johan Hovold
In-Reply-To: <1479400804-9847-1-git-send-email-johan@kernel.org>
Make sure to keep the platform device runtime-resumed throughout probe
to avoid accessing the CPSW registers in the error path (e.g. for
deferred probe) with clocks disabled:
Unhandled fault: external abort on non-linefetch (0x1008) at 0xd0872d08
...
[<c04fabcc>] (cpsw_ale_control_set) from [<c04fb8b4>] (cpsw_ale_destroy+0x2c/0x44)
[<c04fb8b4>] (cpsw_ale_destroy) from [<c04fea58>] (cpsw_probe+0xbd0/0x10c4)
[<c04fea58>] (cpsw_probe) from [<c047b2a0>] (platform_drv_probe+0x5c/0xc0)
Fixes: df828598a755 ("netdev: driver: ethernet: Add TI CPSW driver")
Signed-off-by: Johan Hovold <johan@kernel.org>
---
drivers/net/ethernet/ti/cpsw.c | 11 +++++++----
1 file changed, 7 insertions(+), 4 deletions(-)
diff --git a/drivers/net/ethernet/ti/cpsw.c b/drivers/net/ethernet/ti/cpsw.c
index c6cff3d2ff05..f60f8ab7c1e3 100644
--- a/drivers/net/ethernet/ti/cpsw.c
+++ b/drivers/net/ethernet/ti/cpsw.c
@@ -2641,13 +2641,12 @@ static int cpsw_probe(struct platform_device *pdev)
goto clean_runtime_disable_ret;
}
cpsw->version = readl(&cpsw->regs->id_ver);
- pm_runtime_put_sync(&pdev->dev);
res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
cpsw->wr_regs = devm_ioremap_resource(&pdev->dev, res);
if (IS_ERR(cpsw->wr_regs)) {
ret = PTR_ERR(cpsw->wr_regs);
- goto clean_runtime_disable_ret;
+ goto clean_pm_runtime_put_ret;
}
memset(&dma_params, 0, sizeof(dma_params));
@@ -2684,7 +2683,7 @@ static int cpsw_probe(struct platform_device *pdev)
default:
dev_err(priv->dev, "unknown version 0x%08x\n", cpsw->version);
ret = -ENODEV;
- goto clean_runtime_disable_ret;
+ goto clean_pm_runtime_put_ret;
}
for (i = 0; i < cpsw->data.slaves; i++) {
struct cpsw_slave *slave = &cpsw->slaves[i];
@@ -2713,7 +2712,7 @@ static int cpsw_probe(struct platform_device *pdev)
if (!cpsw->dma) {
dev_err(priv->dev, "error initializing dma\n");
ret = -ENOMEM;
- goto clean_runtime_disable_ret;
+ goto clean_pm_runtime_put_ret;
}
cpsw->txch[0] = cpdma_chan_create(cpsw->dma, 0, cpsw_tx_handler, 0);
@@ -2815,12 +2814,16 @@ static int cpsw_probe(struct platform_device *pdev)
}
}
+ pm_runtime_put(&pdev->dev);
+
return 0;
clean_ale_ret:
cpsw_ale_destroy(cpsw->ale);
clean_dma_ret:
cpdma_ctlr_destroy(cpsw->dma);
+clean_pm_runtime_put_ret:
+ pm_runtime_put_sync(&pdev->dev);
clean_runtime_disable_ret:
pm_runtime_disable(&pdev->dev);
clean_ndev_ret:
--
2.7.3
^ permalink raw reply related
* [PATCH net v2 0/7] net: cpsw: fix leaks and probe deferral
From: Johan Hovold @ 2016-11-17 16:39 UTC (permalink / raw)
To: Mugunthan V N
Cc: Grygorii Strashko, linux-omap, netdev, linux-kernel, Johan Hovold
This series fixes as number of leaks and issues in the cpsw probe-error
and driver-unbind paths, some which specifically prevented deferred
probing.
Johan
v2
- Keep platform device runtime-resumed throughout probe instead of
resuming in the probe error path as suggested by Grygorii (patch
1/7).
- Runtime-resume platform device before registering any children in
order to make sure it is synchronously suspended after deregistering
children in the error path (patch 3/7).
Johan Hovold (7):
net: ethernet: ti: cpsw: fix bad register access in probe error path
net: ethernet: ti: cpsw: fix mdio device reference leak
net: ethernet: ti: cpsw: fix deferred probe
net: ethernet: ti: cpsw: fix of_node and phydev leaks
net: ethernet: ti: cpsw: fix secondary-emac probe error path
net: ethernet: ti: cpsw: add missing sanity check
net: ethernet: ti: cpsw: fix fixed-link phy probe deferral
drivers/net/ethernet/ti/cpsw.c | 95 ++++++++++++++++++++++++++++++++----------
1 file changed, 74 insertions(+), 21 deletions(-)
--
2.7.3
^ permalink raw reply
* Re: [PATCH 1/3] net: stmmac: replace all pr_xxx by their netdev_xxx counterpart
From: Giuseppe CAVALLARO @ 2016-11-17 16:39 UTC (permalink / raw)
To: Corentin Labbe, alexandre.torgue; +Cc: netdev, linux-kernel
In-Reply-To: <1479323381-26639-1-git-send-email-clabbe.montjoie@gmail.com>
On 11/16/2016 8:09 PM, Corentin Labbe wrote:
> From: LABBE Corentin <clabbe.montjoie@gmail.com>
>
> The stmmac driver use lots of pr_xxx functions to print information.
> This is bad since we cannot know which device logs the information.
> (moreover if two stmmac device are present)
>
> Furthermore, it seems that it assumes wrongly that all logs will always
> be subsequent by using a dev_xxx then some indented pr_xxx like this:
> kernel: sun7i-dwmac 1c50000.ethernet: no reset control found
> kernel: Ring mode enabled
> kernel: No HW DMA feature register supported
> kernel: Normal descriptors
> kernel: TX Checksum insertion supported
>
> So this patch replace all pr_xxx by their netdev_xxx counterpart.
> Excepts for some printing where netdev "cause" unpretty output like:
> sun7i-dwmac 1c50000.ethernet (unnamed net_device) (uninitialized): no reset control found
> In those case, I keep dev_xxx.
>
> In the same time I remove some "stmmac:" print since
> this will be a duplicate with that dev_xxx displays.
>
> Signed-off-by: Corentin Labbe <clabbe.montjoie@gmail.com>
Thanks for these changes.
Acked-by: Giuseppe Cavallaro <peppe.cavallaro@st.com>
> ---
> drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 204 ++++++++++++----------
> drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c | 14 +-
> 2 files changed, 123 insertions(+), 95 deletions(-)
>
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index 8eb12353..791daf4 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> @@ -305,7 +305,7 @@ bool stmmac_eee_init(struct stmmac_priv *priv)
> */
> spin_lock_irqsave(&priv->lock, flags);
> if (priv->eee_active) {
> - pr_debug("stmmac: disable EEE\n");
> + netdev_dbg(priv->dev, "disable EEE\n");
> del_timer_sync(&priv->eee_ctrl_timer);
> priv->hw->mac->set_eee_timer(priv->hw, 0,
> tx_lpi_timer);
> @@ -334,7 +334,7 @@ bool stmmac_eee_init(struct stmmac_priv *priv)
> ret = true;
> spin_unlock_irqrestore(&priv->lock, flags);
>
> - pr_debug("stmmac: Energy-Efficient Ethernet initialized\n");
> + netdev_dbg(priv->dev, "Energy-Efficient Ethernet initialized\n");
> }
> out:
> return ret;
> @@ -456,8 +456,8 @@ static int stmmac_hwtstamp_ioctl(struct net_device *dev, struct ifreq *ifr)
> sizeof(struct hwtstamp_config)))
> return -EFAULT;
>
> - pr_debug("%s config flags:0x%x, tx_type:0x%x, rx_filter:0x%x\n",
> - __func__, config.flags, config.tx_type, config.rx_filter);
> + netdev_dbg(priv->dev, "%s config flags:0x%x, tx_type:0x%x, rx_filter:0x%x\n",
> + __func__, config.flags, config.tx_type, config.rx_filter);
>
> /* reserved for future extensions */
> if (config.flags)
> @@ -756,8 +756,9 @@ static void stmmac_adjust_link(struct net_device *dev)
> break;
> default:
> if (netif_msg_link(priv))
> - pr_warn("%s: Speed (%d) not 10/100\n",
> - dev->name, phydev->speed);
> + netdev_warn(priv->dev,
> + "Speed (%d) not 10/100\n",
> + phydev->speed);
> break;
> }
>
> @@ -810,10 +811,10 @@ static void stmmac_check_pcs_mode(struct stmmac_priv *priv)
> (interface == PHY_INTERFACE_MODE_RGMII_ID) ||
> (interface == PHY_INTERFACE_MODE_RGMII_RXID) ||
> (interface == PHY_INTERFACE_MODE_RGMII_TXID)) {
> - pr_debug("STMMAC: PCS RGMII support enable\n");
> + netdev_dbg(priv->dev, "PCS RGMII support enabled\n");
> priv->hw->pcs = STMMAC_PCS_RGMII;
> } else if (interface == PHY_INTERFACE_MODE_SGMII) {
> - pr_debug("STMMAC: PCS SGMII support enable\n");
> + netdev_dbg(priv->dev, "PCS SGMII support enabled\n");
> priv->hw->pcs = STMMAC_PCS_SGMII;
> }
> }
> @@ -848,15 +849,15 @@ static int stmmac_init_phy(struct net_device *dev)
>
> snprintf(phy_id_fmt, MII_BUS_ID_SIZE + 3, PHY_ID_FMT, bus_id,
> priv->plat->phy_addr);
> - pr_debug("stmmac_init_phy: trying to attach to %s\n",
> - phy_id_fmt);
> + netdev_dbg(priv->dev, "stmmac_init_phy: trying to attach to %s\n",
> + phy_id_fmt);
>
> phydev = phy_connect(dev, phy_id_fmt, &stmmac_adjust_link,
> interface);
> }
>
> if (IS_ERR_OR_NULL(phydev)) {
> - pr_err("%s: Could not attach to PHY\n", dev->name);
> + netdev_err(priv->dev, "Could not attach to PHY\n");
> if (!phydev)
> return -ENODEV;
>
> @@ -889,8 +890,9 @@ static int stmmac_init_phy(struct net_device *dev)
> if (phydev->is_pseudo_fixed_link)
> phydev->irq = PHY_POLL;
>
> - pr_debug("stmmac_init_phy: %s: attached to PHY (UID 0x%x)"
> - " Link = %d\n", dev->name, phydev->phy_id, phydev->link);
> + netdev_dbg(priv->dev,
> + "stmmac_init_phy: attached to PHY (UID 0x%x) Link = %d\n",
> + phydev->phy_id, phydev->link);
>
> return 0;
> }
> @@ -976,7 +978,8 @@ static int stmmac_init_rx_buffers(struct stmmac_priv *priv, struct dma_desc *p,
>
> skb = __netdev_alloc_skb_ip_align(priv->dev, priv->dma_buf_sz, flags);
> if (!skb) {
> - pr_err("%s: Rx init fails; skb is NULL\n", __func__);
> + netdev_err(priv->dev,
> + "%s: Rx init fails; skb is NULL\n", __func__);
> return -ENOMEM;
> }
> priv->rx_skbuff[i] = skb;
> @@ -984,7 +987,7 @@ static int stmmac_init_rx_buffers(struct stmmac_priv *priv, struct dma_desc *p,
> priv->dma_buf_sz,
> DMA_FROM_DEVICE);
> if (dma_mapping_error(priv->device, priv->rx_skbuff_dma[i])) {
> - pr_err("%s: DMA mapping error\n", __func__);
> + netdev_err(priv->dev, "%s: DMA mapping error\n", __func__);
> dev_kfree_skb_any(skb);
> return -EINVAL;
> }
> @@ -1035,11 +1038,12 @@ static int init_dma_desc_rings(struct net_device *dev, gfp_t flags)
> priv->dma_buf_sz = bfsize;
>
> if (netif_msg_probe(priv)) {
> - pr_debug("(%s) dma_rx_phy=0x%08x dma_tx_phy=0x%08x\n", __func__,
> - (u32) priv->dma_rx_phy, (u32) priv->dma_tx_phy);
> + netdev_dbg(priv->dev, "(%s) dma_rx_phy=0x%08x dma_tx_phy=0x%08x\n",
> + __func__, (u32)priv->dma_rx_phy,
> + (u32)priv->dma_tx_phy);
>
> /* RX INITIALIZATION */
> - pr_debug("\tSKB addresses:\nskb\t\tskb data\tdma data\n");
> + netdev_dbg(priv->dev, "SKB addresses:\nskb\t\tskb data\tdma data\n");
> }
> for (i = 0; i < DMA_RX_SIZE; i++) {
> struct dma_desc *p;
> @@ -1053,7 +1057,8 @@ static int init_dma_desc_rings(struct net_device *dev, gfp_t flags)
> goto err_init_rx_buffers;
>
> if (netif_msg_probe(priv))
> - pr_debug("[%p]\t[%p]\t[%x]\n", priv->rx_skbuff[i],
> + netdev_dbg(priv->dev, "[%p]\t[%p]\t[%x]\n",
> + priv->rx_skbuff[i],
> priv->rx_skbuff[i]->data,
> (unsigned int)priv->rx_skbuff_dma[i]);
> }
> @@ -1386,7 +1391,8 @@ static void stmmac_tx_clean(struct stmmac_priv *priv)
> if (netif_queue_stopped(priv->dev) &&
> stmmac_tx_avail(priv) > STMMAC_TX_THRESH) {
> if (netif_msg_tx_done(priv))
> - pr_debug("%s: restart transmit\n", __func__);
> + netdev_dbg(priv->dev, "%s: restart transmit\n",
> + __func__);
> netif_wake_queue(priv->dev);
> }
> netif_tx_unlock(priv->dev);
> @@ -1497,7 +1503,7 @@ static void stmmac_mmc_setup(struct stmmac_priv *priv)
> dwmac_mmc_ctrl(priv->mmcaddr, mode);
> memset(&priv->mmc, 0, sizeof(struct stmmac_counters));
> } else
> - pr_info(" No MAC Management Counters available\n");
> + netdev_info(priv->dev, "No MAC Management Counters available\n");
> }
>
> /**
> @@ -1510,18 +1516,18 @@ static void stmmac_mmc_setup(struct stmmac_priv *priv)
> static void stmmac_selec_desc_mode(struct stmmac_priv *priv)
> {
> if (priv->plat->enh_desc) {
> - pr_info(" Enhanced/Alternate descriptors\n");
> + dev_info(priv->device, "Enhanced/Alternate descriptors\n");
>
> /* GMAC older than 3.50 has no extended descriptors */
> if (priv->synopsys_id >= DWMAC_CORE_3_50) {
> - pr_info("\tEnabled extended descriptors\n");
> + dev_info(priv->device, "Enabled extended descriptors\n");
> priv->extend_desc = 1;
> } else
> - pr_warn("Extended descriptors not supported\n");
> + dev_warn(priv->device, "Extended descriptors not supported\n");
>
> priv->hw->desc = &enh_desc_ops;
> } else {
> - pr_info(" Normal descriptors\n");
> + dev_info(priv->device, "Normal descriptors\n");
> priv->hw->desc = &ndesc_ops;
> }
> }
> @@ -1562,8 +1568,8 @@ static void stmmac_check_ether_addr(struct stmmac_priv *priv)
> priv->dev->dev_addr, 0);
> if (!is_valid_ether_addr(priv->dev->dev_addr))
> eth_hw_addr_random(priv->dev);
> - pr_info("%s: device MAC address %pM\n", priv->dev->name,
> - priv->dev->dev_addr);
> + netdev_info(priv->dev, "device MAC address %pM\n",
> + priv->dev->dev_addr);
> }
> }
>
> @@ -1671,7 +1677,8 @@ static int stmmac_hw_setup(struct net_device *dev, bool init_ptp)
> /* DMA initialization and SW reset */
> ret = stmmac_init_dma_engine(priv);
> if (ret < 0) {
> - pr_err("%s: DMA engine initialization failed\n", __func__);
> + netdev_err(priv->dev, "%s: DMA engine initialization failed\n",
> + __func__);
> return ret;
> }
>
> @@ -1700,7 +1707,7 @@ static int stmmac_hw_setup(struct net_device *dev, bool init_ptp)
>
> ret = priv->hw->mac->rx_ipc(priv->hw);
> if (!ret) {
> - pr_warn(" RX IPC Checksum Offload disabled\n");
> + netdev_warn(priv->dev, "RX IPC Checksum Offload disabled\n");
> priv->plat->rx_coe = STMMAC_RX_COE_NONE;
> priv->hw->rx_csum = 0;
> }
> @@ -1725,10 +1732,11 @@ static int stmmac_hw_setup(struct net_device *dev, bool init_ptp)
> #ifdef CONFIG_DEBUG_FS
> ret = stmmac_init_fs(dev);
> if (ret < 0)
> - pr_warn("%s: failed debugFS registration\n", __func__);
> + netdev_warn(priv->dev, "%s: failed debugFS registration\n",
> + __func__);
> #endif
> /* Start the ball rolling... */
> - pr_debug("%s: DMA RX/TX processes started...\n", dev->name);
> + netdev_dbg(priv->dev, "DMA RX/TX processes started...\n");
> priv->hw->dma->start_tx(priv->ioaddr);
> priv->hw->dma->start_rx(priv->ioaddr);
>
> @@ -1783,8 +1791,9 @@ static int stmmac_open(struct net_device *dev)
> priv->hw->pcs != STMMAC_PCS_RTBI) {
> ret = stmmac_init_phy(dev);
> if (ret) {
> - pr_err("%s: Cannot attach to PHY (error: %d)\n",
> - __func__, ret);
> + netdev_err(priv->dev,
> + "%s: Cannot attach to PHY (error: %d)\n",
> + __func__, ret);
> return ret;
> }
> }
> @@ -1798,19 +1807,21 @@ static int stmmac_open(struct net_device *dev)
>
> ret = alloc_dma_desc_resources(priv);
> if (ret < 0) {
> - pr_err("%s: DMA descriptors allocation failed\n", __func__);
> + netdev_err(priv->dev, "%s: DMA descriptors allocation failed\n",
> + __func__);
> goto dma_desc_error;
> }
>
> ret = init_dma_desc_rings(dev, GFP_KERNEL);
> if (ret < 0) {
> - pr_err("%s: DMA descriptors initialization failed\n", __func__);
> + netdev_err(priv->dev, "%s: DMA descriptors initialization failed\n",
> + __func__);
> goto init_error;
> }
>
> ret = stmmac_hw_setup(dev, true);
> if (ret < 0) {
> - pr_err("%s: Hw setup failed\n", __func__);
> + netdev_err(priv->dev, "%s: Hw setup failed\n", __func__);
> goto init_error;
> }
>
> @@ -1823,8 +1834,9 @@ static int stmmac_open(struct net_device *dev)
> ret = request_irq(dev->irq, stmmac_interrupt,
> IRQF_SHARED, dev->name, dev);
> if (unlikely(ret < 0)) {
> - pr_err("%s: ERROR: allocating the IRQ %d (error: %d)\n",
> - __func__, dev->irq, ret);
> + netdev_err(priv->dev,
> + "%s: ERROR: allocating the IRQ %d (error: %d)\n",
> + __func__, dev->irq, ret);
> goto init_error;
> }
>
> @@ -1833,8 +1845,9 @@ static int stmmac_open(struct net_device *dev)
> ret = request_irq(priv->wol_irq, stmmac_interrupt,
> IRQF_SHARED, dev->name, dev);
> if (unlikely(ret < 0)) {
> - pr_err("%s: ERROR: allocating the WoL IRQ %d (%d)\n",
> - __func__, priv->wol_irq, ret);
> + netdev_err(priv->dev,
> + "%s: ERROR: allocating the WoL IRQ %d (%d)\n",
> + __func__, priv->wol_irq, ret);
> goto wolirq_error;
> }
> }
> @@ -1844,8 +1857,9 @@ static int stmmac_open(struct net_device *dev)
> ret = request_irq(priv->lpi_irq, stmmac_interrupt, IRQF_SHARED,
> dev->name, dev);
> if (unlikely(ret < 0)) {
> - pr_err("%s: ERROR: allocating the LPI IRQ %d (%d)\n",
> - __func__, priv->lpi_irq, ret);
> + netdev_err(priv->dev,
> + "%s: ERROR: allocating the LPI IRQ %d (%d)\n",
> + __func__, priv->lpi_irq, ret);
> goto lpiirq_error;
> }
> }
> @@ -2008,7 +2022,9 @@ static netdev_tx_t stmmac_tso_xmit(struct sk_buff *skb, struct net_device *dev)
> if (!netif_queue_stopped(dev)) {
> netif_stop_queue(dev);
> /* This is a hard error, log it. */
> - pr_err("%s: Tx Ring full when queue awake\n", __func__);
> + netdev_err(priv->dev,
> + "%s: Tx Ring full when queue awake\n",
> + __func__);
> }
> spin_unlock(&priv->tx_lock);
> return NETDEV_TX_BUSY;
> @@ -2082,7 +2098,8 @@ static netdev_tx_t stmmac_tso_xmit(struct sk_buff *skb, struct net_device *dev)
>
> if (unlikely(stmmac_tx_avail(priv) <= (MAX_SKB_FRAGS + 1))) {
> if (netif_msg_hw(priv))
> - pr_debug("%s: stop transmitted packets\n", __func__);
> + netdev_dbg(priv->dev, "%s: stop transmitted packets\n",
> + __func__);
> netif_stop_queue(dev);
> }
>
> @@ -2188,7 +2205,9 @@ static netdev_tx_t stmmac_xmit(struct sk_buff *skb, struct net_device *dev)
> if (!netif_queue_stopped(dev)) {
> netif_stop_queue(dev);
> /* This is a hard error, log it. */
> - pr_err("%s: Tx Ring full when queue awake\n", __func__);
> + netdev_err(priv->dev,
> + "%s: Tx Ring full when queue awake\n",
> + __func__);
> }
> return NETDEV_TX_BUSY;
> }
> @@ -2263,9 +2282,10 @@ static netdev_tx_t stmmac_xmit(struct sk_buff *skb, struct net_device *dev)
> if (netif_msg_pktdata(priv)) {
> void *tx_head;
>
> - pr_debug("%s: curr=%d dirty=%d f=%d, e=%d, first=%p, nfrags=%d",
> - __func__, priv->cur_tx, priv->dirty_tx, first_entry,
> - entry, first, nfrags);
> + netdev_dbg(priv->dev,
> + "%s: curr=%d dirty=%d f=%d, e=%d, first=%p, nfrags=%d",
> + __func__, priv->cur_tx, priv->dirty_tx, first_entry,
> + entry, first, nfrags);
>
> if (priv->extend_desc)
> tx_head = (void *)priv->dma_etx;
> @@ -2274,13 +2294,14 @@ static netdev_tx_t stmmac_xmit(struct sk_buff *skb, struct net_device *dev)
>
> priv->hw->desc->display_ring(tx_head, DMA_TX_SIZE, false);
>
> - pr_debug(">>> frame to be transmitted: ");
> + netdev_dbg(priv->dev, ">>> frame to be transmitted: ");
> print_pkt(skb->data, skb->len);
> }
>
> if (unlikely(stmmac_tx_avail(priv) <= (MAX_SKB_FRAGS + 1))) {
> if (netif_msg_hw(priv))
> - pr_debug("%s: stop transmitted packets\n", __func__);
> + netdev_dbg(priv->dev,
> + "%s: stop transmitted packets\n", __func__);
> netif_stop_queue(dev);
> }
>
> @@ -2357,7 +2378,7 @@ static netdev_tx_t stmmac_xmit(struct sk_buff *skb, struct net_device *dev)
>
> dma_map_err:
> spin_unlock(&priv->tx_lock);
> - dev_err(priv->device, "Tx dma map failed\n");
> + netdev_err(priv->dev, "Tx DMA map failed\n");
> dev_kfree_skb(skb);
> priv->dev->stats.tx_dropped++;
> return NETDEV_TX_OK;
> @@ -2428,7 +2449,7 @@ static inline void stmmac_rx_refill(struct stmmac_priv *priv)
> DMA_FROM_DEVICE);
> if (dma_mapping_error(priv->device,
> priv->rx_skbuff_dma[entry])) {
> - dev_err(priv->device, "Rx dma map failed\n");
> + netdev_err(priv->dev, "Rx DMA map failed\n");
> dev_kfree_skb(skb);
> break;
> }
> @@ -2446,7 +2467,8 @@ static inline void stmmac_rx_refill(struct stmmac_priv *priv)
> priv->rx_zeroc_thresh--;
>
> if (netif_msg_rx_status(priv))
> - pr_debug("\trefill entry #%d\n", entry);
> + netdev_dbg(priv->dev,
> + "refill entry #%d\n", entry);
> }
> wmb();
>
> @@ -2479,7 +2501,7 @@ static int stmmac_rx(struct stmmac_priv *priv, int limit)
> if (netif_msg_rx_status(priv)) {
> void *rx_head;
>
> - pr_debug("%s: descriptor ring:\n", __func__);
> + netdev_dbg(priv->dev, "%s: descriptor ring:\n", __func__);
> if (priv->extend_desc)
> rx_head = (void *)priv->dma_erx;
> else
> @@ -2549,9 +2571,9 @@ static int stmmac_rx(struct stmmac_priv *priv, int limit)
> * ignored
> */
> if (frame_len > priv->dma_buf_sz) {
> - pr_err("%s: len %d larger than size (%d)\n",
> - priv->dev->name, frame_len,
> - priv->dma_buf_sz);
> + netdev_err(priv->dev,
> + "len %d larger than size (%d)\n",
> + frame_len, priv->dma_buf_sz);
> priv->dev->stats.rx_length_errors++;
> break;
> }
> @@ -2563,11 +2585,11 @@ static int stmmac_rx(struct stmmac_priv *priv, int limit)
> frame_len -= ETH_FCS_LEN;
>
> if (netif_msg_rx_status(priv)) {
> - pr_debug("\tdesc: %p [entry %d] buff=0x%x\n",
> - p, entry, des);
> + netdev_dbg(priv->dev, "\tdesc: %p [entry %d] buff=0x%x\n",
> + p, entry, des);
> if (frame_len > ETH_FRAME_LEN)
> - pr_debug("\tframe size %d, COE: %d\n",
> - frame_len, status);
> + netdev_dbg(priv->dev, "frame size %d, COE: %d\n",
> + frame_len, status);
> }
>
> /* The zero-copy is always used for all the sizes
> @@ -2604,8 +2626,9 @@ static int stmmac_rx(struct stmmac_priv *priv, int limit)
> } else {
> skb = priv->rx_skbuff[entry];
> if (unlikely(!skb)) {
> - pr_err("%s: Inconsistent Rx chain\n",
> - priv->dev->name);
> + netdev_err(priv->dev,
> + "%s: Inconsistent Rx chain\n",
> + priv->dev->name);
> priv->dev->stats.rx_dropped++;
> break;
> }
> @@ -2623,7 +2646,8 @@ static int stmmac_rx(struct stmmac_priv *priv, int limit)
> stmmac_get_rx_hwtstamp(priv, entry, skb);
>
> if (netif_msg_pktdata(priv)) {
> - pr_debug("frame received (%dbytes)", frame_len);
> + netdev_dbg(priv->dev, "frame received (%dbytes)",
> + frame_len);
> print_pkt(skb->data, frame_len);
> }
>
> @@ -2720,8 +2744,10 @@ static void stmmac_set_rx_mode(struct net_device *dev)
> */
> static int stmmac_change_mtu(struct net_device *dev, int new_mtu)
> {
> + struct stmmac_priv *priv = netdev_priv(dev);
> +
> if (netif_running(dev)) {
> - pr_err("%s: must be stopped to change its MTU\n", dev->name);
> + netdev_err(priv->dev, "must be stopped to change its MTU\n");
> return -EBUSY;
> }
>
> @@ -2800,7 +2826,7 @@ static irqreturn_t stmmac_interrupt(int irq, void *dev_id)
> pm_wakeup_event(priv->device, 0);
>
> if (unlikely(!dev)) {
> - pr_err("%s: invalid dev pointer\n", __func__);
> + netdev_err(priv->dev, "%s: invalid dev pointer\n", __func__);
> return IRQ_NONE;
> }
>
> @@ -3032,8 +3058,7 @@ static int stmmac_init_fs(struct net_device *dev)
> priv->dbgfs_dir = debugfs_create_dir(dev->name, stmmac_fs_dir);
>
> if (!priv->dbgfs_dir || IS_ERR(priv->dbgfs_dir)) {
> - pr_err("ERROR %s/%s, debugfs create directory failed\n",
> - STMMAC_RESOURCE_NAME, dev->name);
> + netdev_err(priv->dev, "ERROR failed to create debugfs directory\n");
>
> return -ENOMEM;
> }
> @@ -3045,7 +3070,7 @@ static int stmmac_init_fs(struct net_device *dev)
> &stmmac_rings_status_fops);
>
> if (!priv->dbgfs_rings_status || IS_ERR(priv->dbgfs_rings_status)) {
> - pr_info("ERROR creating stmmac ring debugfs file\n");
> + netdev_err(priv->dev, "ERROR creating stmmac ring debugfs file\n");
> debugfs_remove_recursive(priv->dbgfs_dir);
>
> return -ENOMEM;
> @@ -3057,7 +3082,7 @@ static int stmmac_init_fs(struct net_device *dev)
> dev, &stmmac_dma_cap_fops);
>
> if (!priv->dbgfs_dma_cap || IS_ERR(priv->dbgfs_dma_cap)) {
> - pr_info("ERROR creating stmmac MMC debugfs file\n");
> + netdev_err(priv->dev, "ERROR creating stmmac MMC debugfs file\n");
> debugfs_remove_recursive(priv->dbgfs_dir);
>
> return -ENOMEM;
> @@ -3129,11 +3154,11 @@ static int stmmac_hw_init(struct stmmac_priv *priv)
> } else {
> if (chain_mode) {
> priv->hw->mode = &chain_mode_ops;
> - pr_info(" Chain mode enabled\n");
> + dev_info(priv->device, "Chain mode enabled\n");
> priv->mode = STMMAC_CHAIN_MODE;
> } else {
> priv->hw->mode = &ring_mode_ops;
> - pr_info(" Ring mode enabled\n");
> + dev_info(priv->device, "Ring mode enabled\n");
> priv->mode = STMMAC_RING_MODE;
> }
> }
> @@ -3141,7 +3166,7 @@ static int stmmac_hw_init(struct stmmac_priv *priv)
> /* Get the HW capability (new GMAC newer than 3.50a) */
> priv->hw_cap_support = stmmac_get_hw_features(priv);
> if (priv->hw_cap_support) {
> - pr_info(" DMA HW capability register supported");
> + dev_info(priv->device, "DMA HW capability register supported\n");
>
> /* We can override some gmac/dma configuration fields: e.g.
> * enh_desc, tx_coe (e.g. that are passed through the
> @@ -3166,8 +3191,9 @@ static int stmmac_hw_init(struct stmmac_priv *priv)
> else if (priv->dma_cap.rx_coe_type1)
> priv->plat->rx_coe = STMMAC_RX_COE_TYPE1;
>
> - } else
> - pr_info(" No HW DMA feature register supported");
> + } else {
> + dev_info(priv->device, "No HW DMA feature register supported\n");
> + }
>
> /* To use alternate (extended), normal or GMAC4 descriptor structures */
> if (priv->synopsys_id >= DWMAC_CORE_4_00)
> @@ -3177,20 +3203,20 @@ static int stmmac_hw_init(struct stmmac_priv *priv)
>
> if (priv->plat->rx_coe) {
> priv->hw->rx_csum = priv->plat->rx_coe;
> - pr_info(" RX Checksum Offload Engine supported\n");
> + dev_info(priv->device, "RX Checksum Offload Engine supported\n");
> if (priv->synopsys_id < DWMAC_CORE_4_00)
> - pr_info("\tCOE Type %d\n", priv->hw->rx_csum);
> + dev_info(priv->device, "COE Type %d\n", priv->hw->rx_csum);
> }
> if (priv->plat->tx_coe)
> - pr_info(" TX Checksum insertion supported\n");
> + dev_info(priv->device, "TX Checksum insertion supported\n");
>
> if (priv->plat->pmt) {
> - pr_info(" Wake-Up On Lan supported\n");
> + dev_info(priv->device, "Wake-Up On Lan supported\n");
> device_set_wakeup_capable(priv->device, 1);
> }
>
> if (priv->dma_cap.tsoen)
> - pr_info(" TSO supported\n");
> + dev_info(priv->device, "TSO supported\n");
>
> return 0;
> }
> @@ -3249,8 +3275,8 @@ int stmmac_dvr_probe(struct device *device,
>
> priv->stmmac_clk = devm_clk_get(priv->device, STMMAC_RESOURCE_NAME);
> if (IS_ERR(priv->stmmac_clk)) {
> - dev_warn(priv->device, "%s: warning: cannot get CSR clock\n",
> - __func__);
> + netdev_warn(priv->dev, "%s: warning: cannot get CSR clock\n",
> + __func__);
> /* If failed to obtain stmmac_clk and specific clk_csr value
> * is NOT passed from the platform, probe fail.
> */
> @@ -3299,7 +3325,7 @@ int stmmac_dvr_probe(struct device *device,
> if ((priv->plat->tso_en) && (priv->dma_cap.tsoen)) {
> ndev->hw_features |= NETIF_F_TSO;
> priv->tso = true;
> - pr_info(" TSO feature enabled\n");
> + dev_info(priv->device, "TSO feature enabled\n");
> }
> ndev->features |= ndev->hw_features | NETIF_F_HIGHDMA;
> ndev->watchdog_timeo = msecs_to_jiffies(watchdog);
> @@ -3328,7 +3354,7 @@ int stmmac_dvr_probe(struct device *device,
> */
> if ((priv->synopsys_id >= DWMAC_CORE_3_50) && (!priv->plat->riwt_off)) {
> priv->use_riwt = 1;
> - pr_info(" Enable RX Mitigation via HW Watchdog Timer\n");
> + netdev_info(priv->dev, "Enable RX Mitigation via HW Watchdog Timer\n");
> }
>
> netif_napi_add(ndev, &priv->napi, stmmac_poll, 64);
> @@ -3338,7 +3364,8 @@ int stmmac_dvr_probe(struct device *device,
>
> ret = register_netdev(ndev);
> if (ret) {
> - pr_err("%s: ERROR %i registering the device\n", __func__, ret);
> + netdev_err(priv->dev, "%s: ERROR %i registering the device\n",
> + __func__, ret);
> goto error_netdev_register;
> }
>
> @@ -3361,8 +3388,9 @@ int stmmac_dvr_probe(struct device *device,
> /* MDIO bus Registration */
> ret = stmmac_mdio_register(ndev);
> if (ret < 0) {
> - pr_debug("%s: MDIO bus (id: %d) registration failed",
> - __func__, priv->plat->bus_id);
> + netdev_err(priv->dev,
> + "%s: MDIO bus (id: %d) registration failed",
> + __func__, priv->plat->bus_id);
> goto error_mdio_register;
> }
> }
> @@ -3395,7 +3423,7 @@ int stmmac_dvr_remove(struct device *dev)
> struct net_device *ndev = dev_get_drvdata(dev);
> struct stmmac_priv *priv = netdev_priv(ndev);
>
> - pr_info("%s:\n\tremoving driver", __func__);
> + netdev_info(priv->dev, "%s: removing driver", __func__);
>
> priv->hw->dma->stop_rx(priv->ioaddr);
> priv->hw->dma->stop_tx(priv->ioaddr);
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c
> index ec29585..e3216e5 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c
> @@ -260,7 +260,7 @@ int stmmac_mdio_reset(struct mii_bus *bus)
> #endif
>
> if (data->phy_reset) {
> - pr_debug("stmmac_mdio_reset: calling phy_reset\n");
> + netdev_dbg(ndev, "stmmac_mdio_reset: calling phy_reset\n");
> data->phy_reset(priv->plat->bsp_priv);
> }
>
> @@ -325,7 +325,7 @@ int stmmac_mdio_register(struct net_device *ndev)
> else
> err = mdiobus_register(new_bus);
> if (err != 0) {
> - pr_err("%s: Cannot register as MDIO bus\n", new_bus->name);
> + netdev_err(ndev, "Cannot register the MDIO bus\n");
> goto bus_register_fail;
> }
>
> @@ -372,16 +372,16 @@ int stmmac_mdio_register(struct net_device *ndev)
> irq_str = irq_num;
> break;
> }
> - pr_info("%s: PHY ID %08x at %d IRQ %s (%s)%s\n",
> - ndev->name, phydev->phy_id, addr,
> - irq_str, phydev_name(phydev),
> - act ? " active" : "");
> + netdev_info(ndev, "PHY ID %08x at %d IRQ %s (%s)%s\n",
> + phydev->phy_id, addr,
> + irq_str, phydev_name(phydev),
> + act ? " active" : "");
> found = 1;
> }
> }
>
> if (!found && !mdio_node) {
> - pr_warn("%s: No PHY found\n", ndev->name);
> + netdev_warn(ndev, "No PHY found\n");
> mdiobus_unregister(new_bus);
> mdiobus_free(new_bus);
> return -ENODEV;
>
^ permalink raw reply
* Re: [PATCH net-next 1/5] net: stmmac: Implement ethtool::nway_reset
From: Giuseppe CAVALLARO @ 2016-11-17 16:38 UTC (permalink / raw)
To: Florian Fainelli, netdev
Cc: davem, andrew, tremyfr, Alexandre Torgue, open list
In-Reply-To: <20161115191949.15361-2-f.fainelli@gmail.com>
On 11/15/2016 8:19 PM, Florian Fainelli wrote:
> Utilize the generic phy_ethtool_nway_reset() helper function to
> implement an autonegotiation restart.
>
> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
Acked-by: Giuseppe Cavallaro <peppe.cavallaro@st.com>
> ---
> drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c
> index 3fe9340b748f..0290d52330da 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c
> @@ -870,6 +870,7 @@ static const struct ethtool_ops stmmac_ethtool_ops = {
> .get_regs = stmmac_ethtool_gregs,
> .get_regs_len = stmmac_ethtool_get_regs_len,
> .get_link = ethtool_op_get_link,
> + .nway_reset = phy_ethtool_nway_reset,
> .get_pauseparam = stmmac_get_pauseparam,
> .set_pauseparam = stmmac_set_pauseparam,
> .get_ethtool_stats = stmmac_get_ethtool_stats,
>
^ permalink raw reply
* Re: [PATCH net 1/2] r8152: fix the sw rx checksum is unavailable
From: Mark Lord @ 2016-11-17 14:14 UTC (permalink / raw)
To: Hayes Wang, netdev@vger.kernel.org
Cc: nic_swsd, linux-kernel@vger.kernel.org, linux-usb@vger.kernel.org
In-Reply-To: <0835B3720019904CB8F7AA43166CEEB20105013E@RTITMBSV03.realtek.com.tw>
(resending.. not sure if the original had mailer errors)
On 16-11-16 10:36 PM, Hayes Wang wrote:
> [...]
>> Fix the hw rx checksum is always enabled, and the user couldn't switch
>> it to sw rx checksum.
>>
>> Note that the RTL_VER_01 only supports sw rx checksum only. Besides,
>> the hw rx checksum for RTL_VER_02 is disabled after
>> commit b9a321b48af4 ("r8152: Fix broken RX checksums."). Re-enable it.
>
> Excuse me. If I want to re-send this one patch, should I let
> RTL_VER_02 use rx hw checksum?
Definitely NOT.
I am still doing low-level tracing through the driver as time permits,
and just now found some really interesting evidence.
Using coherent buffers (non-cacheable, allocated with usb_alloc_coherent),
I can get it to fail extremely regularly by simply reducing the buffer size
(agg_buf_sz) from 16KB down to 4KB. This makes reproducing the issue
much much easier -- the same problems do happen with the larger 16KB size,
but much less often than with smaller sizes.
So.. with a 4KB URB transfer_buffer size, along with a ton of added error-checking,
I see this behaviour every 10 (rx) URBs or so:
First URB (number 593):
[ 34.260667] r8152_rx_bottom: 593 corrupted urb: head=bf014000 urb_offset=2856/4096 pkt_len(1518) exceeds remainder(1216)
[ 34.271931] r8152_dump_rx_desc: 044805ee 40080000 006005dc 06020000 00000000 00000000 rx_len=1518
Next URB (number 594):
[ 34.281172] r8152_check_rx_desc: rx_desc looks bad.
[ 34.286228] r8152_rx_bottom: 594 corrupted urb. head=bf018000 urb_offset=0/304 len_used=24
[ 34.294774] r8152_dump_rx_desc: 00008300 00008400 00008500 00008600 00008700 00008800 rx_len=768
What the above sample shows, is the URB transfer buffer ran out of space in the middle
of a packet, and the hardware then tried to just continue that same packet in the next URB,
without an rx_desc header inserted. The r8152.c driver always assumes the URB buffer begins
with an rx_desc, so of course this behaviour produces really weird effects, and system crashes, etc..
So until that driver bug is addressed, I would advise disabling hardware RX checksums
for all chip versions, not only for version 02.
It is not clear to me how the chip decides when to forward an rx URB to the host.
If you could describe how that part works for us, then it would help in further
understanding why fast systems (eg. a PC) don't generally notice the issue,
while much slower embedded systems do see the issue regularly.
Thanks
Mark
^ permalink raw reply
* Re: [RFC PATCH 2/2] net: macb: Add 64 bit addressing support for GEM
From: Nicolas Ferre @ 2016-11-17 13:28 UTC (permalink / raw)
To: Harini Katakam, Rafal Ozieblo
Cc: harini.katakam@xilinx.com, netdev@vger.kernel.org,
linux-kernel@vger.kernel.org
In-Reply-To: <CAFcVECJ-ZYjGg-3CbWD2Ka8yKzOokUxS8ZHHOJXa2Z6anuTr7Q@mail.gmail.com>
Le 17/11/2016 à 13:21, Harini Katakam a écrit :
> Hi Rafal,
>
> On Thu, Nov 17, 2016 at 5:20 PM, Rafal Ozieblo <rafalo@cadence.com> wrote:
>> Hello,
>> I think, there could a bug in your patch.
>>
>>> +
>>> +#ifdef CONFIG_ARCH_DMA_ADDR_T_64BIT
>>> + dmacfg |= GEM_BIT(ADDR64);
>>> +#endif
>>
>> You enable 64 bit addressing (64b dma bus width) always when appropriate architecture config option is enabled.
>> But there are some legacy controllers which do not support that feature. According Cadence hardware team:
>> "64 bit addressing was added in July 2013. Earlier version do not have it.
>> This feature was enhanced in release August 2014 to have separate upper address values for transmit and receive."
>>
>>> /* Bitfields in NSR */
>>> @@ -474,6 +479,10 @@
>>> struct macb_dma_desc {
>> > u32 addr;
>>> u32 ctrl;
>>> +#ifdef CONFIG_ARCH_DMA_ADDR_T_64BIT
>>> + u32 addrh;
>>> + u32 resvd;
>>> +#endif
>>> };
>>
>> It will not work for legacy hardware. Old descriptor is 2 words wide, the new one is 4 words wide.
>> If you enable CONFIG_ARCH_DMA_ADDR_T_64BIT but hardware doesn't support it at all,
>> you will miss every second descriptor.
>>
>
> True, this feature is not available in all of Cadence IP versions.
> In fact, the IP version Zynq does not support this. But the one in ZynqMP does.
> So, we enable kernel config for 64 bit DMA addressing for this SoC and hence
> the driver picks it up. My assumption was that if the legacy IP does not support
> 64 bit addressing, then this DMA option wouldn't be enabled.
>
> There is a design config register in Cadence IP which is being read to
> check for 64 bit address support - DMA mask is set based on that.
> But the addition of two descriptor words cannot be based on this runtime check.
> For this reason, all the static changes were placed under this check.
We have quite a bunch of options in this driver to determinate what is
the real capacity of the underlying hardware.
If HW configuration registers are not appropriate, and it seems they are
not, I would advice to simply use the DT compatibility string.
Best regards,
--
Nicolas Ferre
^ permalink raw reply
* Re: [PATCH net 1/2] r8152: fix the sw rx checksum is unavailable
From: Mark Lord @ 2016-11-17 14:25 UTC (permalink / raw)
To: Hayes Wang, netdev@vger.kernel.org
Cc: nic_swsd, linux-kernel@vger.kernel.org, linux-usb@vger.kernel.org,
David Miller
In-Reply-To: <a4a43da6-54e0-a9c8-8802-acb4e6c4a2c8@pobox.com>
On 16-11-17 09:14 AM, Mark Lord wrote:
..
> Using coherent buffers (non-cacheable, allocated with usb_alloc_coherent),
Note that the same behaviour also happens with the original kmalloc'd buffers.
> I can get it to fail extremely regularly by simply reducing the buffer size
> (agg_buf_sz) from 16KB down to 4KB. This makes reproducing the issue
> much much easier -- the same problems do happen with the larger 16KB size,
> but much less often than with smaller sizes.
Increasing the buffer size to 64KB makes the problem much less frequent,
as one might expect. Thus far I haven't seen it happen at all, but a longer
run (1-3 days) is needed to make sure. This however is NOT a "fix".
> So.. with a 4KB URB transfer_buffer size, along with a ton of added error-checking,
> I see this behaviour every 10 (rx) URBs or so:
>
> First URB (number 593):
> [ 34.260667] r8152_rx_bottom: 593 corrupted urb: head=bf014000 urb_offset=2856/4096 pkt_len(1518) exceeds remainder(1216)
> [ 34.271931] r8152_dump_rx_desc: 044805ee 40080000 006005dc 06020000 00000000 00000000 rx_len=1518
>
> Next URB (number 594):
> [ 34.281172] r8152_check_rx_desc: rx_desc looks bad.
> [ 34.286228] r8152_rx_bottom: 594 corrupted urb. head=bf018000 urb_offset=0/304 len_used=24
> [ 34.294774] r8152_dump_rx_desc: 00008300 00008400 00008500 00008600 00008700 00008800 rx_len=768
>
> What the above sample shows, is the URB transfer buffer ran out of space in the middle
> of a packet, and the hardware then tried to just continue that same packet in the next URB,
> without an rx_desc header inserted. The r8152.c driver always assumes the URB buffer begins
> with an rx_desc, so of course this behaviour produces really weird effects, and system crashes, etc..
>
> So until that driver bug is addressed, I would advise disabling hardware RX checksums
> for all chip versions, not only for version 02.
>
> It is not clear to me how the chip decides when to forward an rx URB to the host.
> If you could describe how that part works for us, then it would help in further
> understanding why fast systems (eg. a PC) don't generally notice the issue,
> while much slower embedded systems do see the issue regularly.
That last part is critical to understanding things:
How does the chip decide that a URB is "full enough" before sending it to the host?
Why does a really fast host see fewer packets jammed together into a single URB than a slower host?
The answers will help understand if there are more bugs to be found/fixed,
or if everything is explained by what has been observed thus far.
To recap: the hardware sometimes fills a URB to the very end, and then continues the
current packet at the first byte of the following URB. The r8152.c driver does NOT
handle this situation; instead it always interprets the first 24 bytes of every URB
as an "rx_desc" structure, without any kind of sanity/validation. This results in
buffer overruns (it trusts the packet length field, even though the URB is too small
to hold such a packet), and other semi-random behaviour.
Using software rx checksums prevents Bad Things(tm) happening from most of this,
but even that is not perfect given the severity of the bug.
Cheers
^ permalink raw reply
* Re: [Intel-wired-lan] [PATCH v2] e1000e: free IRQ regardless of __E1000_DOWN
From: Neftin, Sasha @ 2016-11-17 13:31 UTC (permalink / raw)
To: Baicar, Tyler, jeffrey.t.kirsher, intel-wired-lan, netdev,
linux-kernel, okaya, timur
In-Reply-To: <3914adae-91b2-7b74-02bc-579b3e32d143@intel.com>
On 11/13/2016 10:34 AM, Neftin, Sasha wrote:
> On 11/11/2016 12:35 AM, Baicar, Tyler wrote:
>> Hello Sasha,
>>
>> On 11/9/2016 11:19 PM, Neftin, Sasha wrote:
>>> On 11/9/2016 11:41 PM, Tyler Baicar wrote:
>>>> Move IRQ free code so that it will happen regardless of the
>>>> __E1000_DOWN bit. Currently the e1000e driver only releases its IRQ
>>>> if the __E1000_DOWN bit is cleared. This is not sufficient because
>>>> it is possible for __E1000_DOWN to be set without releasing the IRQ.
>>>> In such a situation, we will hit a kernel bug later in e1000_remove
>>>> because the IRQ still has action since it was never freed. A
>>>> secondary bus reset can cause this case to happen.
>>>>
>>>> Signed-off-by: Tyler Baicar <tbaicar@codeaurora.org>
>>>> ---
>>>> drivers/net/ethernet/intel/e1000e/netdev.c | 3 ++-
>>>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/net/ethernet/intel/e1000e/netdev.c
>>>> b/drivers/net/ethernet/intel/e1000e/netdev.c
>>>> index 7017281..36cfcb0 100644
>>>> --- a/drivers/net/ethernet/intel/e1000e/netdev.c
>>>> +++ b/drivers/net/ethernet/intel/e1000e/netdev.c
>>>> @@ -4679,12 +4679,13 @@ int e1000e_close(struct net_device *netdev)
>>>> if (!test_bit(__E1000_DOWN, &adapter->state)) {
>>>> e1000e_down(adapter, true);
>>>> - e1000_free_irq(adapter);
>>>> /* Link status message must follow this format */
>>>> pr_info("%s NIC Link is Down\n", adapter->netdev->name);
>>>> }
>>>> + e1000_free_irq(adapter);
>>>> +
>>>> napi_disable(&adapter->napi);
>>>> e1000e_free_tx_resources(adapter->tx_ring);
>>>>
>>> I would like not recommend insert this change. This change related
>>> driver state machine, we afraid from lot of synchronization problem and
>>> issues.
>>> We need keep e1000_free_irq in loop and check for 'test_bit' ready.
>>
>> What do you mean here? There is no loop. If __E1000_DOWN is set then we
>> will never free the IRQ.
>>
>>> Another point, does before execute secondary bus reset your SW back up
>>> pcie configuration space as properly?
>>
>> After a secondary bus reset, the link needs to recover and go back to a
>> working state after 1 second.
>>
>> From the callstack, the issue is happening while removing the endpoint
>> from the system, before applying the secondary bus reset.
>>
>> The order of events is
>> 1. remove the drivers
>> 2. cause a secondary bus reset
>> 3. wait 1 second
> Actually, this is too much, usually link up in less than 100ms.You can
> check Data Link Layer indication.
>> 4. recover the link
>>
>> callstack:
>> free_msi_irqs+0x6c/0x1a8
>> pci_disable_msi+0xb0/0x148
>> e1000e_reset_interrupt_capability+0x60/0x78
>> e1000_remove+0xc8/0x180
>> pci_device_remove+0x48/0x118
>> __device_release_driver+0x80/0x108
>> device_release_driver+0x2c/0x40
>> pci_stop_bus_device+0xa0/0xb0
>> pci_stop_bus_device+0x3c/0xb0
>> pci_stop_root_bus+0x54/0x80
>> acpi_pci_root_remove+0x28/0x64
>> acpi_bus_trim+0x6c/0xa4
>> acpi_device_hotplug+0x19c/0x3f4
>> acpi_hotplug_work_fn+0x28/0x3c
>> process_one_work+0x150/0x460
>> worker_thread+0x50/0x4b8
>> kthread+0xd4/0xe8
>> ret_from_fork+0x10/0x50
>>
>> Thanks,
>> Tyler
>>
> Hello Tyler,
> Okay, we need consult more about this suggestion.
> May I ask what is setup you run? Is there NIC or on board LAN? I would
> like try reproduce this issue in our lab's too.
> Also, is same issue observed with same scenario and others NIC's too?
> Sasha
> _______________________________________________
> Intel-wired-lan mailing list
> Intel-wired-lan@lists.osuosl.org
> http://lists.osuosl.org/mailman/listinfo/intel-wired-lan
>
Hello Tyler,
I see some in consistent implementation of __*_close methods in our
drivers. Do you have any igb NIC to check if same problem persist there?
Thanks,
Sasha
^ permalink raw reply
* RE: [RFC PATCH 2/2] net: macb: Add 64 bit addressing support for GEM
From: Rafal Ozieblo @ 2016-11-17 12:57 UTC (permalink / raw)
To: Harini Katakam
Cc: harini.katakam@xilinx.com, nicolas.ferre@atmel.com,
netdev@vger.kernel.org, linux-kernel@vger.kernel.org
In-Reply-To: <CAFcVECJ-ZYjGg-3CbWD2Ka8yKzOokUxS8ZHHOJXa2Z6anuTr7Q@mail.gmail.com>
-----Original Message-----
From: Harini Katakam [mailto:harinikatakamlinux@gmail.com]
Sent: 17 listopada 2016 13:22
To: Rafal Ozieblo
Cc: harini.katakam@xilinx.com; nicolas.ferre@atmel.com; netdev@vger.kernel.org; linux-kernel@vger.kernel.org
Subject: Re: [RFC PATCH 2/2] net: macb: Add 64 bit addressing support for GEM
> Hi Rafal,
>
> On Thu, Nov 17, 2016 at 5:20 PM, Rafal Ozieblo <rafalo@cadence.com> wrote:
> > Hello,
> > I think, there could a bug in your patch.
> >
> >> +
> >> +#ifdef CONFIG_ARCH_DMA_ADDR_T_64BIT
> >> + dmacfg |= GEM_BIT(ADDR64); #endif
> >
> > You enable 64 bit addressing (64b dma bus width) always when appropriate architecture config option is enabled.
> > But there are some legacy controllers which do not support that feature. According Cadence hardware team:
> > "64 bit addressing was added in July 2013. Earlier version do not have it.
> > This feature was enhanced in release August 2014 to have separate upper address values for transmit and receive."
> >
> >> /* Bitfields in NSR */
> >> @@ -474,6 +479,10 @@
> >> struct macb_dma_desc {
> > > u32 addr;
> >> u32 ctrl;
> >> +#ifdef CONFIG_ARCH_DMA_ADDR_T_64BIT
> >> + u32 addrh;
> >> + u32 resvd;
> >> +#endif
> >> };
> >
> > It will not work for legacy hardware. Old descriptor is 2 words wide, the new one is 4 words wide.
> > If you enable CONFIG_ARCH_DMA_ADDR_T_64BIT but hardware doesn't
> > support it at all, you will miss every second descriptor.
> >
>
> True, this feature is not available in all of Cadence IP versions.
> In fact, the IP version Zynq does not support this. But the one in ZynqMP does.
> So, we enable kernel config for 64 bit DMA addressing for this SoC and hence the driver picks it up. My assumption was that if the legacy IP does not support
> 64 bit addressing, then this DMA option wouldn't be enabled.
What for example with arm64 (which enables CONFIG_ARCH_DMA_ADDR_T_64BIT by default) and legacy IP controller? Or systems with multiple IP, both with and without 64b dma capable?
It might result in unpredictable behavio. (explained below)
> There is a design config register in Cadence IP which is being read to check for 64 bit address support - DMA mask is set based on that.
> But the addition of two descriptor words cannot be based on this runtime check.
> For this reason, all the static changes were placed under this check.
Are you talking about this?
+#ifdef CONFIG_ARCH_DMA_ADDR_T_64BIT
+ if (GEM_BFEXT(DBWDEF, gem_readl(bp, DCFG1)) > GEM_DBW32)
+ dma_set_mask(&pdev->dev, DMA_BIT_MASK(44));
+#endif
+
It only sets the maximum address which can be seen on address bus. (its mask exactly)
+static inline void macb_set_addr(struct macb_dma_desc *desc, dma_addr_t addr)
+{
+ desc->addr = (u32)addr;
+#ifdef CONFIG_ARCH_DMA_ADDR_T_64BIT
+ desc->addrh = (u32)(addr >> 32);
+#endif
+}
+
Because addr is 32b wide, (u32)(addr >> 32) equals 0. IP controller uses 2 words for dma descriptor, so desc->addr from second hardware descriptor will be overwritten by desc->addrh from the first software descriptor.
(same desc->ctrl from second hardware descriptor will be overwritten by desc->resvd).
Regards,
Harini
^ permalink raw reply
* Re: [RFC PATCH 2/2] net: macb: Add 64 bit addressing support for GEM
From: Harini Katakam @ 2016-11-17 12:21 UTC (permalink / raw)
To: Rafal Ozieblo
Cc: harini.katakam@xilinx.com, nicolas.ferre@atmel.com,
netdev@vger.kernel.org, linux-kernel@vger.kernel.org
In-Reply-To: <BN3PR07MB2516B2EBCF0AE7079971DBF6C9B10@BN3PR07MB2516.namprd07.prod.outlook.com>
Hi Rafal,
On Thu, Nov 17, 2016 at 5:20 PM, Rafal Ozieblo <rafalo@cadence.com> wrote:
> Hello,
> I think, there could a bug in your patch.
>
>> +
>> +#ifdef CONFIG_ARCH_DMA_ADDR_T_64BIT
>> + dmacfg |= GEM_BIT(ADDR64);
>> +#endif
>
> You enable 64 bit addressing (64b dma bus width) always when appropriate architecture config option is enabled.
> But there are some legacy controllers which do not support that feature. According Cadence hardware team:
> "64 bit addressing was added in July 2013. Earlier version do not have it.
> This feature was enhanced in release August 2014 to have separate upper address values for transmit and receive."
>
>> /* Bitfields in NSR */
>> @@ -474,6 +479,10 @@
>> struct macb_dma_desc {
> > u32 addr;
>> u32 ctrl;
>> +#ifdef CONFIG_ARCH_DMA_ADDR_T_64BIT
>> + u32 addrh;
>> + u32 resvd;
>> +#endif
>> };
>
> It will not work for legacy hardware. Old descriptor is 2 words wide, the new one is 4 words wide.
> If you enable CONFIG_ARCH_DMA_ADDR_T_64BIT but hardware doesn't support it at all,
> you will miss every second descriptor.
>
True, this feature is not available in all of Cadence IP versions.
In fact, the IP version Zynq does not support this. But the one in ZynqMP does.
So, we enable kernel config for 64 bit DMA addressing for this SoC and hence
the driver picks it up. My assumption was that if the legacy IP does not support
64 bit addressing, then this DMA option wouldn't be enabled.
There is a design config register in Cadence IP which is being read to
check for 64 bit address support - DMA mask is set based on that.
But the addition of two descriptor words cannot be based on this runtime check.
For this reason, all the static changes were placed under this check.
Regards,
Harini
^ permalink raw reply
* Re: [RFC PATCH 2/2] net: macb: Add 64 bit addressing support for GEM
From: Rafal Ozieblo @ 2016-11-17 11:50 UTC (permalink / raw)
To: harini.katakam@xilinx.com
Cc: nicolas.ferre@atmel.com, netdev@vger.kernel.org,
linux-kernel@vger.kernel.org
Hello,
I think, there could a bug in your patch.
> +
> +#ifdef CONFIG_ARCH_DMA_ADDR_T_64BIT
> + dmacfg |= GEM_BIT(ADDR64);
> +#endif
You enable 64 bit addressing (64b dma bus width) always when appropriate architecture config option is enabled.
But there are some legacy controllers which do not support that feature. According Cadence hardware team:
"64 bit addressing was added in July 2013. Earlier version do not have it.
This feature was enhanced in release August 2014 to have separate upper address values for transmit and receive."
> /* Bitfields in NSR */
> @@ -474,6 +479,10 @@
> struct macb_dma_desc {
> u32 addr;
> u32 ctrl;
> +#ifdef CONFIG_ARCH_DMA_ADDR_T_64BIT
> + u32 addrh;
> + u32 resvd;
> +#endif
> };
It will not work for legacy hardware. Old descriptor is 2 words wide, the new one is 4 words wide.
If you enable CONFIG_ARCH_DMA_ADDR_T_64BIT but hardware doesn't support it at all,
you will miss every second descriptor.
Regards,
Rafal
^ 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