Netdev List
 help / color / mirror / Atom feed
* Re: [net-next PATCH v1 1/2] net: dt-bindings: add RGMII TX delay configuration to meson8b-dwmac
From: Martin Blumenstingl @ 2016-11-24 16:52 UTC (permalink / raw)
  To: Andrew Lunn
  Cc: linux-amlogic-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
	devicetree-u79uwXL29TY76Z2rM5mHXA, netdev-u79uwXL29TY76Z2rM5mHXA,
	davem-fT/PcQaiUtIeIZ0/mPfg9Q, khilman-rdvid1DuHRBWk0Htik3J/w,
	mark.rutland-5wv7dgnIgG8, robh+dt-DgEjT+Ai2ygdnm+yROfE0A,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
	alexandre.torgue-qxv4g6HH51o, peppe.cavallaro-qxv4g6HH51o,
	carlo-KA+7E9HrN00dnm+yROfE0A, jbrunet-rdvid1DuHRBWk0Htik3J/w
In-Reply-To: <20161124154858.GB20455-g2DYL2Zd6BY@public.gmane.org>

Hi Andrew,

On Thu, Nov 24, 2016 at 4:48 PM, Andrew Lunn <andrew-g2DYL2Zd6BY@public.gmane.org> wrote:
>> The configuration values are provided as preprocessor macros to make the
>> devicetree files easier to read.
>
> Hi Martin
>
> If i'm reading the code/comments correctly, you can set the delay to
> 0, 2, 4 or 6ns? So calling this property amlogic,tx-delay-ns would be
> even easier to read.
indeed, this sounds like a very nice idea (as it moves the calculation
from the programmer's brain to dwmac-meson8b.c)!

I'll send an updated version once I received enough feedback (in case
something else is wrong with the patches)
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply

* Re: [PATCH v2] VSOCK: add loopback to virtio_transport
From: David Miller @ 2016-11-24 16:54 UTC (permalink / raw)
  To: stefanha; +Cc: netdev, cavery, imbrenda, jhansen
In-Reply-To: <1479736591-25235-1-git-send-email-stefanha@redhat.com>

From: Stefan Hajnoczi <stefanha@redhat.com>
Date: Mon, 21 Nov 2016 13:56:31 +0000

> The VMware VMCI transport supports loopback inside virtual machines.
> This patch implements loopback for virtio-vsock.
> 
> Flow control is handled by the virtio-vsock protocol as usual.  The
> sending process stops transmitting on a connection when the peer's
> receive buffer space is exhausted.
> 
> Cathy Avery <cavery@redhat.com> noticed this difference between VMCI and
> virtio-vsock when a test case using loopback failed.  Although loopback
> isn't the main point of AF_VSOCK, it is useful for testing and
> virtio-vsock must match VMCI semantics so that userspace programs run
> regardless of the underlying transport.
> 
> My understanding is that loopback is not supported on the host side with
> VMCI.  Follow that by implementing it only in the guest driver, not the
> vhost host driver.
> 
> Cc: Jorgen Hansen <jhansen@vmware.com>
> Reported-by: Cathy Avery <cavery@redhat.com>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
> v2:
>  * Fixed checkpatch.pl warnings [DaveM]

Applied to net-next, thanks.

^ permalink raw reply

* Re: [PATCH v3] net/phy: add trace events for mdio accesses
From: David Miller @ 2016-11-24 16:56 UTC (permalink / raw)
  To: uwe; +Cc: f.fainelli, rostedt, mingo, netdev
In-Reply-To: <20161122154711.26622-1-uwe@kleine-koenig.org>

From: Uwe Kleine-König <uwe@kleine-koenig.org>
Date: Tue, 22 Nov 2016 16:47:11 +0100

> Make it possible to generate trace events for mdio read and write accesses.
> 
> Signed-off-by: Uwe Kleine-König <uwe@kleine-koenig.org>

Applied to net-next, thanks.

^ permalink raw reply

* Re: [PATCH net 1/2] r8152: fix the sw rx checksum is unavailable
From: Mark Lord @ 2016-11-24 17:00 UTC (permalink / raw)
  To: David Miller, hayeswang; +Cc: netdev, nic_swsd, linux-kernel, linux-usb
In-Reply-To: <23e0c132-8844-0a34-3e0b-e412f76493ba@pobox.com>

On 16-11-24 11:43 AM, Mark Lord wrote:
..
> But how does this ASCII data end up at offset zero of the rx buffer??
> Not possible -- this isn't even stale data, because only an rx_desc could
> be at that offset in that buffer.

Answering my own question here, I suspect it ends up there as a result
of overrunning the previous URB.  So I have updated the test copy of the driver
here now to check for that exact situation.  It's running now, but could take
hours or a day for the bug to occur again.

It seems I am being overly helpful here.

Perhaps I should have just stopped with the original regression report
(driver works in 3.12.xx, fails on all newer kernels, as a result of enabling
hardware checksums).

Had I left it there, one might reasonably expect the onus to be on the driver
developer to sort it out, with me providing retests of supplied patches as need be.

But I've gone WAY BEYOND that, even questioning the sanity of the platform on
which it is being used, just to avoid blaming a buggy USB dongle for some other issue.
And this is leading people to suspect that I really think the platform is buggy.

It isn't.   It's been running for years, with a variety of USB hardware attached,
and nary a problem.  Except with this r8152 dongle on kernels > 3.12.

So, yeah, the driver is fixed in our local tree, and has been for some time now.
I just was hoping that perhaps others might be interested in it too,
since the bug (whatever it is) corrupts data on the NFS server.

Cheers

^ permalink raw reply

* Re: [PATCH net] ipv6: bump genid when the IFA_F_TENTATIVE flag is clear
From: David Miller @ 2016-11-24 17:04 UTC (permalink / raw)
  To: pabeni; +Cc: netdev, hideaki.yoshifuji, hannes
In-Reply-To: <f91712a692016619312fdb480ff4ef290bd961db.1479830136.git.pabeni@redhat.com>

From: Paolo Abeni <pabeni@redhat.com>
Date: Tue, 22 Nov 2016 16:57:40 +0100

> When an ipv6 address has the tentative flag set, it can't be
> used as source for egress traffic, while the associated route,
> if any, can be looked up and even stored into some dst_cache.
> 
> In the latter scenario, the source ipv6 address selected and
> stored in the cache is most probably wrong (e.g. with
> link-local scope) and the entity using the dst_cache will
> experience lack of ipv6 connectivity until said cache is
> cleared or invalidated.
> 
> Overall this may cause lack of connectivity over most IPv6 tunnels
> (comprising geneve and vxlan), if the first egress packet reaches
> the tunnel before the DaD is completed for the used ipv6
> address.
> 
> This patch bumps a new genid after that the IFA_F_TENTATIVE flag
> is cleared, so that dst_cache will be invalidated on
> next lookup and ipv6 connectivity restored.
> 
> Fixes: 0c1d70af924b ("net: use dst_cache for vxlan device")
> Fixes: 468dfffcd762 ("geneve: add dst caching support")
> Acked-by: Hannes Frederic Sowa <hannes@stressinduktion.org>
> Signed-off-by: Paolo Abeni <pabeni@redhat.com>

Applied and queued up for -stable, thanks.

^ permalink raw reply

* Re: [net-next PATCH v1 0/2] stmmac: dwmac-meson8b: configurable RGMII TX delay
From: Martin Blumenstingl @ 2016-11-24 17:05 UTC (permalink / raw)
  To: Jerome Brunet, f.fainelli-Re5JQEeQqe8AvxtiuMwx3w
  Cc: linux-amlogic-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
	devicetree-u79uwXL29TY76Z2rM5mHXA, netdev-u79uwXL29TY76Z2rM5mHXA,
	davem-fT/PcQaiUtIeIZ0/mPfg9Q, khilman-rdvid1DuHRBWk0Htik3J/w,
	mark.rutland-5wv7dgnIgG8, robh+dt-DgEjT+Ai2ygdnm+yROfE0A,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
	alexandre.torgue-qxv4g6HH51o, peppe.cavallaro-qxv4g6HH51o,
	carlo-KA+7E9HrN00dnm+yROfE0A
In-Reply-To: <1480002964.17538.131.camel-rdvid1DuHRBWk0Htik3J/w@public.gmane.org>

On Thu, Nov 24, 2016 at 4:56 PM, Jerome Brunet <jbrunet-rdvid1DuHRBWk0Htik3J/w@public.gmane.org> wrote:
> On Thu, 2016-11-24 at 15:34 +0100, Martin Blumenstingl wrote:
>> Currently the dwmac-meson8b stmmac glue driver uses a hardcoded 1/4
>> cycle TX clock delay. This seems to work fine for many boards (for
>> example Odroid-C2 or Amlogic's reference boards) but there are some
>> others where TX traffic is simply broken.
>> There are probably multiple reasons why it's working on some boards
>> while it's broken on others:
>> - some of Amlogic's reference boards are using a Micrel PHY
>> - hardware circuit design
>> - maybe more...
>>
>> This raises a question though:
>> Which device is supposed to enable the TX delay when both MAC and PHY
>> support it? And should we implement it for each PHY / MAC separately
>> or should we think about a more generic solution (currently it's not
>> possible to disable the TX delay generated by the RTL8211F PHY via
>> devicetree when using phy-mode "rgmii")?
>
> Actually you can skip the part which activate the Tx-delay on the phy
> by setting "phy-mode = "rgmii-id" instead of "rgmii"
>
> phy->interface will no longer be PHY_INTERFACE_MODE_RGMII
> but PHY_INTERFACE_MODE_RGMII_ID.
unfortunately this is not true for RTL8211F (I did my previous tests
with the same expectation in mind)!
the code seems to suggest that TX-delay is disabled whenever mode !=
PHY_INTERFACE_MODE_RGMII.
BUT: on my device RTL8211F_TX_DELAY is set even before
"phy_write(phydev, 0x11, reg);"!

Based on what I found it seems that rgmii-id, rgmii-txid and
rgmii-rxid are supposed to be handled by the PHY.
That would mean that we have two problems here:
1) drivers/net/phy/realtek.c:rtl8211f_config_init should check for
PHY_INTERFACE_MODE_RGMII_ID or PHY_INTERFACE_MODE_RGMII_TXID and
enable the TX-delay in that case - otherwise explicitly disable it
2) dwmac-meson8b.c should only use the configured TX-delay for
PHY_INTERFACE_MODE_RGMII
@Florian: could you please share your thoughts on this (who handles
the TX delay in which case)?


Regards,
Martin
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply

* Re: [RFC PATCH net v2 0/3] Fix OdroidC2 Gigabit Tx link issue
From: Martin Blumenstingl @ 2016-11-24 17:10 UTC (permalink / raw)
  To: Jerome Brunet
  Cc: netdev-u79uwXL29TY76Z2rM5mHXA, devicetree-u79uwXL29TY76Z2rM5mHXA,
	Florian Fainelli, Carlo Caione, Kevin Hilman, Giuseppe Cavallaro,
	Alexandre TORGUE, Andre Roth, Neil Armstrong,
	linux-amlogic-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <1480003306.17538.137.camel-rdvid1DuHRBWk0Htik3J/w@public.gmane.org>

On Thu, Nov 24, 2016 at 5:01 PM, Jerome Brunet <jbrunet-rdvid1DuHRBWk0Htik3J/w@public.gmane.org> wrote:
> On Thu, 2016-11-24 at 15:40 +0100, Martin Blumenstingl wrote:
>> Hi Jerome,
>>
>> On Mon, Nov 21, 2016 at 4:35 PM, Jerome Brunet <jbrunet-rdvid1DuHRBWk0Htik3J/w@public.gmane.org>
>> wrote:
>> >
>> > This patchset fixes an issue with the OdroidC2 board (DWMAC +
>> > RTL8211F).
>> > Initially reported as a low Tx throughput issue at gigabit speed,
>> > the
>> > platform enters LPI too often. This eventually break the link (both
>> > Tx
>> > and Rx), and require to bring the interface down and up again to
>> > get the
>> > Rx path working again.
>> >
>> > The root cause of this issue is not fully understood yet but
>> > disabling EEE
>> > advertisement on the PHY prevent this feature to be negotiated.
>> > With this change, the link is stable and reliable, with the
>> > expected
>> > throughput performance.
>> I have just sent a series which allows configuring the TX delay on
>> the
>> MAC (dwmac-meson8b glue) side: [0]
>> Disabling the TX delay generated by the MAC fixes TX throughput for
>> me, even when leaving EEE enabled in the RTL8211F PHY driver!
>>
>> Unfortunately the RTL8211F PHY is a black-box for the community
>> because there is no public datasheeet available.
>> *maybe* (pure speculation!) they're enabling the TX delay based on
>> some internal magic only when EEE is enabled.
>
> Hi already tried acting on the register setting the TX_delay. I also
> tried on the PHY. I never been able to improve situation on the
> Odroic2. Only disabling EEE improved the situation.
OK, thanks for clarifying this!

> To make sure, i tried again with your patch but the result remains
> unchanged. With Tx_delay disabled (either the mac or the phy), the
> situation is even worse, it seems that nothing gets through
This is interesting, because in your case you should have a 4ns TX
delay (2ns from the MAC and presumably 2ns from the PHY).
Maybe that is also the reason why the TX delay is configurable in 2ns
steps in PRG_ETHERNET0 on Amlogic SoCs.

out of curiosity: have you tried setting a 4ns (half clock-cycle) TX
delay for the MAC and disabling it in the PHY?


Regards,
Martin
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply

* Re: [PATCH net 1/2] r8152: fix the sw rx checksum is unavailable
From: David Miller @ 2016-11-24 17:11 UTC (permalink / raw)
  To: mlord; +Cc: hayeswang, netdev, nic_swsd, linux-kernel, linux-usb
In-Reply-To: <23e0c132-8844-0a34-3e0b-e412f76493ba@pobox.com>

From: Mark Lord <mlord@pobox.com>
Date: Thu, 24 Nov 2016 11:43:53 -0500

> So even if this were a platform memory coherency issue, one should
> still never see ASCII data at the beginning of an rx buffer.

I'm not so convinced, since this is the kind of random corruption one
would expect to see when dealing with virtual caches that have
aliasing or similar issues.

Writes to address X that show up at address Y or not at all are
precisely the signature of virtual cache aliasing problems.

Is it a case of the chip writing to X but the cpu is still seeing
stale data from a previous CPU store?

For NFS the cpu is writing into the page cache, so we know that
cpu side stores are where the ASCII text is coming from.

Now is the r8152 buffer one that the USB host controller is DMA'ing
into directly, or is it one that SWIOMMU or similar bounce buffering
is copying into?  In the latter case we are doing cpu stores into
the area and the writes aren't coming from the device.

^ permalink raw reply

* Re: [PATCH net 1/2] r8152: fix the sw rx checksum is unavailable
From: David Miller @ 2016-11-24 17:13 UTC (permalink / raw)
  To: mlord; +Cc: hayeswang, netdev, nic_swsd, linux-kernel, linux-usb
In-Reply-To: <e599b008-07c5-3de1-3349-a088bc3b6b1e@pobox.com>

From: Mark Lord <mlord@pobox.com>
Date: Thu, 24 Nov 2016 12:00:15 -0500

> It seems I am being overly helpful here.

Either you want to cry or you want to keep helping us track down
this problem.  It is your choice, and your choice alone.

Please do not pretend otherwise, everyone else in this thread is
operating with the best intentions and wants to see this through
to a full analysis and a proper solution for the corruptions.

Thank you.

^ permalink raw reply

* Re: [Patch net-next] net_sched: move the empty tp check from ->destroy() to ->delete()
From: Daniel Borkmann @ 2016-11-24 17:18 UTC (permalink / raw)
  To: Roi Dayan, Cong Wang, netdev; +Cc: jiri, John Fastabend
In-Reply-To: <58370558.9070004@iogearbox.net>

On 11/24/2016 04:20 PM, Daniel Borkmann wrote:
> On 11/24/2016 12:01 PM, Roi Dayan wrote:
>> On 24/11/2016 12:14, Daniel Borkmann wrote:
>>> On 11/24/2016 09:29 AM, Roi Dayan wrote:
>>>> Hi,
>>>>
>>>> I'm testing this patch with KASAN enabled and got into a new kernel crash I didn't hit before.
>>>>
>>>> [ 1860.725065] ==================================================================
>>>> [ 1860.733893] BUG: KASAN: use-after-free in __netif_receive_skb_core+0x1ebe/0x29a0 at addr ffff880a68b04028
>>>> [ 1860.745415] Read of size 8 by task CPU 0/KVM/5334
>>>> [ 1860.751368] CPU: 8 PID: 5334 Comm: CPU 0/KVM Tainted: G O 4.9.0-rc3+ #18
>
> (Btw, your kernel is tainted with o-o-tree module? Anything relevant?)
>
>>>> [ 1860.760547] Hardware name: HP ProLiant DL380p Gen8, BIOS P70 07/01/2015
>>>> [ 1860.768036] Call Trace:
>>>> [ 1860.771307]  [<ffffffffa9b6dc42>] dump_stack+0x63/0x81
>>>> [ 1860.777167]  [<ffffffffa95fb751>] kasan_object_err+0x21/0x70
>>>> [ 1860.783826]  [<ffffffffa95fb9dd>] kasan_report_error+0x1ed/0x4e0
>>>> [ 1860.790640]  [<ffffffffa9b9b841>] ? csum_partial+0x11/0x20
>>>> [ 1860.796871]  [<ffffffffaa44a6b9>] ? csum_partial_ext+0x9/0x10
>>>> [ 1860.803571]  [<ffffffffaa453155>] ? __skb_checksum+0x115/0x8d0
>>>> [ 1860.810370]  [<ffffffffa95fbe81>] __asan_report_load8_noabort+0x61/0x70
>>>> [ 1860.818263]  [<ffffffffaa49c3fe>] ? __netif_receive_skb_core+0x1ebe/0x29a0
>>>> [ 1860.826215]  [<ffffffffaa49c3fe>] __netif_receive_skb_core+0x1ebe/0x29a0
>>>> [ 1860.833991]  [<ffffffffaa49a540>] ? netdev_info+0x100/0x100
>>>> [ 1860.840529]  [<ffffffffaa671792>] ? udp4_gro_receive+0x802/0x1090
>>>> [ 1860.847783]  [<ffffffffa9bb9a08>] ? find_next_bit+0x18/0x20
>>>> [ 1860.854126]  [<ffffffffaa49cf04>] __netif_receive_skb+0x24/0x150
>>>> [ 1860.861695]  [<ffffffffaa49d0d1>] netif_receive_skb_internal+0xa1/0x1d0
>>>> [ 1860.869366]  [<ffffffffaa49d030>] ? __netif_receive_skb+0x150/0x150
>>>> [ 1860.876464]  [<ffffffffaa49f7e9>] ? dev_gro_receive+0x969/0x1660
>>>> [ 1860.883924]  [<ffffffffaa4a0e1f>] napi_gro_receive+0x1df/0x300
>>>> [ 1860.890744]  [<ffffffffc02e885d>] mlx5e_handle_rx_cqe_rep+0x83d/0xd30 [mlx5_core]
>>>>
>>>> checking with gdb
>>>>
>>>> (gdb) l *(__netif_receive_skb_core+0x1ebe)
>>>> 0xffffffff8249c3fe is in __netif_receive_skb_core (net/core/dev.c:3937).
>>>> 3932                    *pt_prev = NULL;
>>>> 3933            }
>>>> 3934
>>>> 3935            qdisc_skb_cb(skb)->pkt_len = skb->len;
>>>> 3936            skb->tc_verd = SET_TC_AT(skb->tc_verd, AT_INGRESS);
>>>> 3937            qdisc_bstats_cpu_update(cl->q, skb);
>>>> 3938
>>>> 3939            switch (tc_classify(skb, cl, &cl_res, false)) {
>>>> 3940            case TC_ACT_OK:
>>>> 3941            case TC_ACT_RECLASSIFY:
>>>
>>> Can you elaborate some more on your test-case? Adding/dropping ingress qdisc with
>>> some classifier on it in a loop while traffic goes through?
>>
>> I first delete the qdisc ingress from the relevant interface
>> I start traffic on it then I add the qdisc ingress to the relevant interface and start adding tc flower rules to match the traffic.
>
> Ok, strange, qdisc_destroy() calls into ops->destroy(), where ingress
> drops its entire chain via tcf_destroy_chain(), so that will be NULL
> eventually. The tps are freed by call_rcu() as well as qdisc itself
> later on via qdisc_rcu_free(), where it frees per-cpu bstats as well.
> Outstanding readers should either bail out due to if (!cl) or can still
> process the chain until read section ends, but during that time, cl->q
> resp. bstats should be good. Do you happen to know what's at address
> ffff880a68b04028? I was wondering wrt call_rcu() vs call_rcu_bh(), but
> at least on ingress (netif_receive_skb_internal()) we hold rcu_read_lock()
> here. The KASAN report is reliably happening at this location, right?

Tried to reproduce this on my phys machine on top of Cong's patch and no
luck hitting above so far. I have a KASAN compiled kernel with pktgen
hitting ingress and ingress qdisc + flower filter rules added/destroyed
in a loop. Hmm, do you have a kernel config (particular RCU settings)?

^ permalink raw reply

* Re: 答复: [scr265482] ip_tunnel.c
From: Cong Wang @ 2016-11-24 17:38 UTC (permalink / raw)
  To: Liyang Yu (于立洋1)
  Cc: security@kernel.org, netdev@vger.kernel.org,
	cve-request@mitre.org
In-Reply-To: <F41877A268BCDE49BAEE2286FA8C269F6FECFBBF@LETV-MBX-IDC09.letv.local>

On Wed, Nov 23, 2016 at 11:45 PM, Liyang Yu (于立洋1) <yuliyang1@le.com> wrote:
> Yeah,I means that recreate the tunnel again,
> But I don’t think the patch can fix the bug. It only can make the first packet received successed. And the follow packet will droped also.
> In function __gre_xmit  line 366
>   tunnel->o_seqno++;
>
> If you restart from UINT_MAX, the 'o_seqno' of second packet will return to 0 again.

The first packet after restart: o_seqno == UINT_MAX, the other end: i_seqno = 0
The second packet after restart: o_seqno == 0, the other end: i_seqno = 1

So traffic should be back to normal.

UINT_MAX is also what RFC suggests.

^ permalink raw reply

* Re: [PATCH 1/4] bindings: net: stmmac: correct note about TSO
From: Alexandre Torgue @ 2016-11-24 17:42 UTC (permalink / raw)
  To: Niklas Cassel, Rob Herring, Mark Rutland, David S. Miller,
	Giuseppe CAVALLARO, Phil Reid, Niklas Cassel, Eric Engestrom
  Cc: netdev-u79uwXL29TY76Z2rM5mHXA, devicetree-u79uwXL29TY76Z2rM5mHXA,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <1479911066-19752-1-git-send-email-niklass-VrBV9hrLPhE@public.gmane.org>

Hi Niklas,

On 11/23/2016 03:24 PM, Niklas Cassel wrote:
> From: Niklas Cassel <niklas.cassel-VrBV9hrLPhE@public.gmane.org>
>
> snps,tso was previously placed under AXI BUS Mode parameters,
> suggesting that the property should be in the stmmac-axi-config node.
>
> TSO (TCP Segmentation Offloading) has nothing to do with AXI BUS Mode
> parameters, and the parser actually expects it to be in the root node,
> not in the stmmac-axi-config.
>
> Also added a note about snps,tso only being available on GMAC4 and newer.
>
> Signed-off-by: Niklas Cassel <niklas.cassel-VrBV9hrLPhE@public.gmane.org>
> ---
>  Documentation/devicetree/bindings/net/stmmac.txt | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/net/stmmac.txt b/Documentation/devicetree/bindings/net/stmmac.txt
> index 41b49e6075f5..b95ff998ba73 100644
> --- a/Documentation/devicetree/bindings/net/stmmac.txt
> +++ b/Documentation/devicetree/bindings/net/stmmac.txt
> @@ -1,7 +1,7 @@
>  * STMicroelectronics 10/100/1000 Ethernet driver (GMAC)
>
>  Required properties:
> -- compatible: Should be "snps,dwmac-<ip_version>" "snps,dwmac"
> +- compatible: Should be "snps,dwmac-<ip_version>", "snps,dwmac"
>  	For backwards compatibility: "st,spear600-gmac" is also supported.
>  - reg: Address and length of the register set for the device
>  - interrupt-parent: Should be the phandle for the interrupt controller
> @@ -50,6 +50,8 @@ Optional properties:
>  - snps,ps-speed: port selection speed that can be passed to the core when
>  		 PCS is supported. For example, this is used in case of SGMII
>  		 and MAC2MAC connection.
> +- snps,tso: this enables the TSO feature otherwise it will be managed by
> +		 MAC HW capability register. Only for GMAC4 and newer.
>  - AXI BUS Mode parameters: below the list of all the parameters to program the
>  			   AXI register inside the DMA module:
>  	- snps,lpi_en: enable Low Power Interface
> @@ -62,8 +64,6 @@ Optional properties:
>  	- snps,fb: fixed-burst
>  	- snps,mb: mixed-burst
>  	- snps,rb: rebuild INCRx Burst
> -	- snps,tso: this enables the TSO feature otherwise it will be managed by
> -	    MAC HW capability register.
>  - mdio: with compatible = "snps,dwmac-mdio", create and register mdio bus.
>
>  Examples:
>
Acked-by: <alexandre.torgue-qxv4g6HH51o@public.gmane.org>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply

* Re: [PATCH 4/4] net: stmmac: stmmac_platform: use correct setup function for gmac4
From: Alexandre Torgue @ 2016-11-24 17:58 UTC (permalink / raw)
  To: Niklas Cassel, Giuseppe Cavallaro; +Cc: Niklas Cassel, netdev, linux-kernel
In-Reply-To: <1479911117-19917-1-git-send-email-niklass@axis.com>

Hi Niklas,

On 11/23/2016 03:25 PM, Niklas Cassel wrote:
> From: Niklas Cassel <niklas.cassel@axis.com>
>
> devicetree binding for stmmac states:
> - compatible: Should be "snps,dwmac-<ip_version>", "snps,dwmac"
> 	For backwards compatibility: "st,spear600-gmac" is also supported.
>
> Previously, when specifying "snps,dwmac-4.10a", "snps,dwmac" as your
> compatible string, plat_stmmacenet_data would have both has_gmac and
> has_gmac4 set.
>
> This would lead to stmmac_hw_init calling dwmac1000_setup rather than
> dwmac4_setup, resulting in a non-functional driver.
> This happened since the check for has_gmac is done before the check for
> has_gmac4. However, the order should not matter, so it does not make sense
> to have both set.

Well spot.

>
> If something is valid for both, you should do as the stmmac_interrupt does:
> if (priv->plat->has_gmac || priv->plat->has_gmac4) ...
>
> The places where it was obvious that the author actually meant
> if (has_gmac || has_gmac4) rather than if (has_gmac) has been updated.
>
> Signed-off-by: Niklas Cassel <niklas.cassel@axis.com>
> ---
>  drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c  | 4 ++--
>  drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c | 1 +
>  2 files changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c
> index d5a8122b6033..dd5b38e4cd1f 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c
> @@ -263,7 +263,7 @@ static void stmmac_ethtool_getdrvinfo(struct net_device *dev,
>  {
>  	struct stmmac_priv *priv = netdev_priv(dev);
>
> -	if (priv->plat->has_gmac)
> +	if (priv->plat->has_gmac || priv->plat->has_gmac4)
>  		strlcpy(info->driver, GMAC_ETHTOOL_NAME, sizeof(info->driver));
>  	else
>  		strlcpy(info->driver, MAC100_ETHTOOL_NAME,
> @@ -448,7 +448,7 @@ static void stmmac_ethtool_gregs(struct net_device *dev,
>
>  	memset(reg_space, 0x0, REG_SPACE_SIZE);
>
> -	if (!priv->plat->has_gmac) {
> +	if (!(priv->plat->has_gmac || priv->plat->has_gmac4)) {
>  		/* MAC registers */
>  		for (i = 0; i < 12; i++)
>  			reg_space[i] = readl(priv->ioaddr + (i * 4));
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
> index 4d544c34c1f2..c8a59f396c6e 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
> @@ -291,6 +291,7 @@ stmmac_probe_config_dt(struct platform_device *pdev, const char **mac)
>  	if (of_device_is_compatible(np, "snps,dwmac-4.00") ||
>  	    of_device_is_compatible(np, "snps,dwmac-4.10a")) {
>  		plat->has_gmac4 = 1;
> +		plat->has_gmac = 0;
>  		plat->pmt = 1;
>  		plat->tso_en = of_property_read_bool(np, "snps,tso");
>  	}
>
Thanks Niklas, Acked-by: <alexandre.torgue@st.com>

Regards
Alex

^ permalink raw reply

* Re: wl1251 & mac address & calibration data
From: Sebastian Reichel @ 2016-11-24 18:11 UTC (permalink / raw)
  To: Pali Rohár
  Cc: Pavel Machek, Michal Kazior, Kalle Valo, Ivaylo Dimitrov,
	Aaro Koskinen, Tony Lindgren, linux-wireless, Network Development,
	linux-kernel
In-Reply-To: <201611241749.33681@pali>

[-- Attachment #1: Type: text/plain, Size: 6231 bytes --]

Hi,

On Thu, Nov 24, 2016 at 05:49:33PM +0100, Pali Rohár wrote:
> On Thursday 24 November 2016 17:08:30 Sebastian Reichel wrote:
> > On Thu, Nov 24, 2016 at 04:20:45PM +0100, Pali Rohár wrote:
> > > On Thursday 24 November 2016 16:13:17 Sebastian Reichel wrote:
> > > > On Thu, Nov 24, 2016 at 09:33:29AM +0100, Pali Rohár wrote:
> > > > > On Thursday 24 November 2016 08:51:04 Pavel Machek wrote:
> > > > > > > > "ifconfig hw ether XX" normally sets the address. I guess
> > > > > > > > that's ioctl?
> > > > > > > 
> > > > > > > This sets temporary address and it is ioctl. IIRC same as
> > > > > > > what ethtool uses. (ifconfig is already deprecated).
> > > > > > > 
> > > > > > > > And I guess we should use similar mechanism for permanent
> > > > > > > > address.
> > > > > > > 
> > > > > > > I'm not sure here... Above ioctl ↑↑↑ is for changing
> > > > > > > temporary mac address. But here we do not want to change
> > > > > > > permanent mac address. We want to tell kernel driver
> > > > > > > current permanent mac address which is stored
> > > > > > 
> > > > > > Well... I'd still use similar mechanism :-).
> > > > > 
> > > > > Thats problematic, because in time when wlan0 interface is
> > > > > registered into system and visible in ifconfig output it
> > > > > already needs to have permanent mac address assigned.
> > > > > 
> > > > > We should assign permanent mac address before wlan0 of wl1251
> > > > > is registered into system.
> > > > 
> > > > You can just add the MAC address to the NVS data, which is also
> > > > required for the device initialization.
> > > 
> > > NVS data file has fixed size, there is IIRC no place for it.
> > > 
> > > But one of my suggestion was to use another request_firmware for
> > > MAC address. So this is similar to what you are proposing, as NVS
> > > data are loaded by request_firmware too...
> > 
> > Just append it to NVS data and modify the size check accordingly?
> 
> First that would mean to have request_firmware() function which will not 
> use direct VFS access, but instead use userspace helper.

Permanent MAC is device specific init data, NVS is device specific
init data => load together.

The userspace helper stuff is only needed to get the data from
the NAND calibration area on the fly.

> NVS data file with some default values (not suitable for usage) is 
> already present in linux-firmware and available in /lib/firmware/...

You mentioned NVS data is fixed size, so this should be enough?

switch(loaded_size)
    case IMAGE_SIZE + MAC_SIZE:
        /* extract mac */
        /* fallthrough */
    case IMAGE_SIZE:
        /* load NVS */
        break;
    default:
        /* fail */
}

> Also I'm not sure if such thing is allowed by license:
> https://git.kernel.org/cgit/linux/kernel/git/firmware/linux-firmware.git/tree/LICENCE.ti-connectivity

concating data is not a modification, otherwise you can't
put the file in a filesystem.

> > > > I wonder if those information could be put into DT. Iirc some
> > > > network devices get their MAC address from DT. Maybe we can add
> > > > all NVS info to DT? How much data is it?
> > > 
> > > Proprietary, signed and closed bootloader NOLO does not support DT.
> > > So for booting you need to append DTS file to kernel image.
> > 
> > Yeah, so NOLO without U-Boot will depend on userspace to fixup DT.
> > 
> > > U-Boot is optional and can be used as intermediate bootloader
> > > between NOLO and kernel. But still it has problems with reading
> > > from nand, so cannot read NVS data nor MAC address.
> > 
> > It may in the future?
> 
> I already tried that, but I failed. I was not able to access N900's nand 
> from u-boot. No idea where was problem...
>
> And if somebody fix onenand in u-boot, then needs to reimplement Nokia's 
> proprietary parser of that partition where is stored NVS and mac address 
> && make this support in upstream u-boot.

Are we talking about this?

https://github.com/community-ssu/libcal/blob/master/cal.c

That's ~1k loc for read-only. Getting NAND working looks harder than
porting this to me.

> So... I doubt it will be in any future.
> 
> + no men power

yeah :(

But with that reasoning you should just extract the NVS data
from NAND and put it in your rootfs.

> > > > Userspace application can add all those information to the DT
> > > > using a DT overlay. Also the u-boot could parse and add it at
> > > > some point in the future.
> > > 
> > > In case when wl1251 is statically linked into kernel image, it is
> > > loaded and initialized before even userspace applications starts.
> > > 
> > > So no... adding NVS data or MAC address into DT or DT overlay is
> > > not a solution.
> > 
> > Actually with data loaded from DT you *can* load data quite early in
> > the boot process, while your suggestions always require userspace.
> > So you argument against yourself?
> 
> You cannot add DTS in uboot (no support).

AFAIK that's supported:

http://www.denx.de/wiki/DULG/LinuxFDTBlob

"Note that U-Boot makes some automatic modifications to the blob
before passing it to the kernel - mainly adding and modifying
information that is learnt at run-time."

> And if you modify DTS on running kernel from userspace, then it is
> too late when wl1251 is statically linked into kernel image.
> 
> wl1251 will not wait until some userspace modify DTS and add data...

if (nvs info missing)
    return -EPROBE_DEFER;

> But request_firmware() in fallback mode *can* wait for userspace so 
> wl1251 can postpone its operation until mdev/udev/whatever starts 
> listening for events and push needed firmware data to kernel.

As you said one workaround is waiting. That's not a solution, that
only works with request_firmware().

> So no, there is no argument against... request_firmware() in fallback 
> mode with userspace helper is by design blocking and waiting for 
> userspace. But waiting for some change in DTS in kernel is just 
> nonsense.

I would just mark the wlan device with status = "disabled" and
enable it in the overlay together with adding the NVS & MAC info.

-- Sebastian

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply

* Re: [PATCH] net: stmmac: enable tx queue 0 for gmac4 IPs synthesized with multiple TX queues
From: Alexandre Torgue @ 2016-11-24 18:11 UTC (permalink / raw)
  To: Niklas Cassel, Giuseppe Cavallaro; +Cc: Niklas Cassel, netdev, linux-kernel
In-Reply-To: <1479998194-7113-1-git-send-email-niklass@axis.com>

Hi Niklas,

On 11/24/2016 03:36 PM, Niklas Cassel wrote:
> From: Niklas Cassel <niklas.cassel@axis.com>
>
> The dwmac4 IP can synthesized with 1-8 number of tx queues.
> On an IP synthesized with DWC_EQOS_NUM_TXQ > 1, all txqueues are disabled
> by default. For these IPs, the bitfield TXQEN is R/W.
>
> Always enable tx queue 0. The write will have no effect on IPs synthesized
> with DWC_EQOS_NUM_TXQ == 1.
>
> The driver does still not utilize more than one tx queue in the IP.
>
> Signed-off-by: Niklas Cassel <niklas.cassel@axis.com>
> ---
>  drivers/net/ethernet/stmicro/stmmac/dwmac4.h     |  3 +++
>  drivers/net/ethernet/stmicro/stmmac/dwmac4_dma.c | 12 +++++++++++-
>  2 files changed, 14 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac4.h b/drivers/net/ethernet/stmicro/stmmac/dwmac4.h
> index 6f4f5ce25114..3e8d4fefa5e0 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac4.h
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac4.h
> @@ -155,8 +155,11 @@ enum power_event {
>  #define MTL_CHAN_RX_DEBUG(x)		(MTL_CHANX_BASE_ADDR(x) + 0x38)
>
>  #define MTL_OP_MODE_RSF			BIT(5)
> +#define MTL_OP_MODE_TXQEN		BIT(3)
>  #define MTL_OP_MODE_TSF			BIT(1)
>
> +#define MTL_OP_MODE_TQS_MASK		GENMASK(24, 16)
> +
>  #define MTL_OP_MODE_TTC_MASK		0x70
>  #define MTL_OP_MODE_TTC_SHIFT		4
>
> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac4_dma.c b/drivers/net/ethernet/stmicro/stmmac/dwmac4_dma.c
> index 116151cd6a95..577316de6ba8 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac4_dma.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac4_dma.c
> @@ -213,7 +213,17 @@ static void dwmac4_dma_chan_op_mode(void __iomem *ioaddr, int txmode,
>  		else
>  			mtl_tx_op |= MTL_OP_MODE_TTC_512;
>  	}
> -
> +	/* For an IP with DWC_EQOS_NUM_TXQ == 1, the fields TXQEN and TQS are RO
> +	 * with reset values: TXQEN on, TQS == DWC_EQOS_TXFIFO_SIZE.
> +	 * For an IP with DWC_EQOS_NUM_TXQ > 1, the fields TXQEN and TQS are R/W
> +	 * with reset values: TXQEN off, TQS 256 bytes.
> +	 *
> +	 * Write the bits in both cases, since it will have no effect when RO.
> +	 * For DWC_EQOS_NUM_TXQ > 1, the top bits in MTL_OP_MODE_TQS_MASK might
> +	 * be RO, however, writing the whole TQS field will result in a value
> +	 * equal to DWC_EQOS_TXFIFO_SIZE, just like for DWC_EQOS_NUM_TXQ == 1.
> +	 */
> +	mtl_tx_op |= MTL_OP_MODE_TXQEN | MTL_OP_MODE_TQS_MASK;

Your patch sounds good. Just one question:

In synopsys databook I use, I see that MTL_OP_MODE_TXQEN for channel 2 
can take several values "disabled / enabled / Enabled in AV mode":

Transmit Queue Enable
This field is used to enable/disable the transmit queue 1. 00 R/W
■ 2'b00 - Not enabled
■ 2'b01 - Enable in AV mode (Reserved when Enable Audio Video
Bridging is not selected while configuring the core)
■ 2'b10 - Enabled
■ 2'b11 - Reserved

Do you plan to manage av mode in a future patch ?

Regards
Alex

>  	writel(mtl_tx_op, ioaddr +  MTL_CHAN_TX_OP_MODE(channel));
>
>  	mtl_rx_op = readl(ioaddr + MTL_CHAN_RX_OP_MODE(channel));
>

^ permalink raw reply

* [PATCH net v2 0/5] net: fix phydev reference leaks
From: Johan Hovold @ 2016-11-24 18:21 UTC (permalink / raw)
  To: David S. Miller
  Cc: Florian Fainelli, Madalin Bucur, Timur Tabi, Andrew Lunn,
	Vivien Didelot, netdev, linux-kernel, Johan Hovold

This series fixes a number of phydev reference leaks (and one of_node
leak) due to failure to put the reference taken by of_phy_find_device().

Note that I did not try to fix drivers/net/phy/xilinx_gmii2rgmii.c which
still leaks a reference.

Against net but should apply just as fine to net-next.

Thanks,
Johan

v2: 
 - use put_device() instead of phy_dev_free() to put the references
   taken in net/dsa (patch 1/4).
 - add four new patches fixing similar leaks


Johan Hovold (5):
  net: dsa: fix fixed-link-phy device leaks
  net: bcmgenet: fix phydev reference leak
  net: fsl/fman: fix phydev reference leak
  net: fsl/fman: fix fixed-link-phydev reference leak
  net: qcom/emac: fix of_node and phydev leaks

 drivers/net/ethernet/broadcom/genet/bcmmii.c     | 4 +++-
 drivers/net/ethernet/freescale/fman/fman_memac.c | 3 +++
 drivers/net/ethernet/freescale/fman/mac.c        | 2 ++
 drivers/net/ethernet/qualcomm/emac/emac-phy.c    | 1 +
 drivers/net/ethernet/qualcomm/emac/emac.c        | 4 ++++
 net/dsa/dsa.c                                    | 5 ++++-
 6 files changed, 17 insertions(+), 2 deletions(-)

-- 
2.7.3

^ permalink raw reply

* [PATCH net v2 1/5] net: dsa: fix fixed-link-phy device leaks
From: Johan Hovold @ 2016-11-24 18:21 UTC (permalink / raw)
  To: David S. Miller
  Cc: Florian Fainelli, Madalin Bucur, Timur Tabi, Andrew Lunn,
	Vivien Didelot, netdev, linux-kernel, Johan Hovold
In-Reply-To: <1480011691-13278-1-git-send-email-johan@kernel.org>

Make sure to drop the reference taken by of_phy_find_device() when
registering and deregistering the fixed-link PHY-device.

Fixes: 39b0c705195e ("net: dsa: Allow configuration of CPU & DSA port
speeds/duplex")
Signed-off-by: Johan Hovold <johan@kernel.org>
---
 net/dsa/dsa.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/net/dsa/dsa.c b/net/dsa/dsa.c
index a6902c1e2f28..cb0091b99592 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);
+
+		put_device(&phydev->mdio.dev);
 	}
 
 	return 0;
@@ -509,8 +511,9 @@ 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_device(&phydev->mdio.dev);
+			phy_device_free(phydev);
 		}
 	}
 }
-- 
2.7.3

^ permalink raw reply related

* [PATCH net v2 2/5] net: bcmgenet: fix phydev reference leak
From: Johan Hovold @ 2016-11-24 18:21 UTC (permalink / raw)
  To: David S. Miller
  Cc: Florian Fainelli, Madalin Bucur, Timur Tabi, Andrew Lunn,
	Vivien Didelot, netdev, linux-kernel, Johan Hovold
In-Reply-To: <1480011691-13278-1-git-send-email-johan@kernel.org>

Make sure to drop the reference taken by of_phy_find_device() when
initialising MOCA PHYs.

Fixes: 6ac9de5f6563 ("net: bcmgenet: Register link_update callback for
all MoCA PHYs")
Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/net/ethernet/broadcom/genet/bcmmii.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/broadcom/genet/bcmmii.c b/drivers/net/ethernet/broadcom/genet/bcmmii.c
index 457c3bc8cfff..2e745bd51df4 100644
--- a/drivers/net/ethernet/broadcom/genet/bcmmii.c
+++ b/drivers/net/ethernet/broadcom/genet/bcmmii.c
@@ -542,8 +542,10 @@ static int bcmgenet_mii_of_init(struct bcmgenet_priv *priv)
 	/* Make sure we initialize MoCA PHYs with a link down */
 	if (phy_mode == PHY_INTERFACE_MODE_MOCA) {
 		phydev = of_phy_find_device(dn);
-		if (phydev)
+		if (phydev) {
 			phydev->link = 0;
+			put_device(&phydev->mdio.dev);
+		}
 	}
 
 	return 0;
-- 
2.7.3

^ permalink raw reply related

* [PATCH net v2 3/5] net: fsl/fman: fix phydev reference leak
From: Johan Hovold @ 2016-11-24 18:21 UTC (permalink / raw)
  To: David S. Miller
  Cc: Florian Fainelli, Madalin Bucur, Timur Tabi, Andrew Lunn,
	Vivien Didelot, netdev, linux-kernel, Johan Hovold
In-Reply-To: <1480011691-13278-1-git-send-email-johan@kernel.org>

Make sure to drop the reference taken by of_phy_find_device() during
initialisation when later freeing the struct fman_mac.

Fixes: 57ba4c9b56d8 ("fsl/fman: Add FMan MAC support")
Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/net/ethernet/freescale/fman/fman_memac.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/net/ethernet/freescale/fman/fman_memac.c b/drivers/net/ethernet/freescale/fman/fman_memac.c
index 53ef51e3bd9e..71a5ded9d1de 100644
--- a/drivers/net/ethernet/freescale/fman/fman_memac.c
+++ b/drivers/net/ethernet/freescale/fman/fman_memac.c
@@ -1107,6 +1107,9 @@ int memac_free(struct fman_mac *memac)
 {
 	free_init_resources(memac);
 
+	if (memac->pcsphy)
+		put_device(&memac->pcsphy->mdio.dev);
+
 	kfree(memac->memac_drv_param);
 	kfree(memac);
 
-- 
2.7.3

^ permalink raw reply related

* [PATCH net v2 5/5] net: qcom/emac: fix of_node and phydev leaks
From: Johan Hovold @ 2016-11-24 18:21 UTC (permalink / raw)
  To: David S. Miller
  Cc: Florian Fainelli, Madalin Bucur, Timur Tabi, Andrew Lunn,
	Vivien Didelot, netdev, linux-kernel, Johan Hovold
In-Reply-To: <1480011691-13278-1-git-send-email-johan@kernel.org>

Make sure to drop the reference taken by of_phy_find_device() during
probe on probe errors and on driver unbind.

Also drop the of_node reference taken by of_parse_phandle() in the same
path.

Fixes: b9b17debc69d ("net: emac: emac gigabit ethernet controller driver")
Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/net/ethernet/qualcomm/emac/emac-phy.c | 1 +
 drivers/net/ethernet/qualcomm/emac/emac.c     | 4 ++++
 2 files changed, 5 insertions(+)

diff --git a/drivers/net/ethernet/qualcomm/emac/emac-phy.c b/drivers/net/ethernet/qualcomm/emac/emac-phy.c
index da4e90db4d98..99a14df28b96 100644
--- a/drivers/net/ethernet/qualcomm/emac/emac-phy.c
+++ b/drivers/net/ethernet/qualcomm/emac/emac-phy.c
@@ -212,6 +212,7 @@ int emac_phy_config(struct platform_device *pdev, struct emac_adapter *adpt)
 
 		phy_np = of_parse_phandle(np, "phy-handle", 0);
 		adpt->phydev = of_phy_find_device(phy_np);
+		of_node_put(phy_np);
 	}
 
 	if (!adpt->phydev) {
diff --git a/drivers/net/ethernet/qualcomm/emac/emac.c b/drivers/net/ethernet/qualcomm/emac/emac.c
index 4fede4b86538..57b35aeac51a 100644
--- a/drivers/net/ethernet/qualcomm/emac/emac.c
+++ b/drivers/net/ethernet/qualcomm/emac/emac.c
@@ -711,6 +711,8 @@ static int emac_probe(struct platform_device *pdev)
 err_undo_napi:
 	netif_napi_del(&adpt->rx_q.napi);
 err_undo_mdiobus:
+	if (!has_acpi_companion(&pdev->dev))
+		put_device(&adpt->phydev->mdio.dev);
 	mdiobus_unregister(adpt->mii_bus);
 err_undo_clocks:
 	emac_clks_teardown(adpt);
@@ -730,6 +732,8 @@ static int emac_remove(struct platform_device *pdev)
 
 	emac_clks_teardown(adpt);
 
+	if (!has_acpi_companion(&pdev->dev))
+		put_device(&adpt->phydev->mdio.dev);
 	mdiobus_unregister(adpt->mii_bus);
 	free_netdev(netdev);
 
-- 
2.7.3

^ permalink raw reply related

* [PATCH net v2 4/5] net: fsl/fman: fix fixed-link-phydev reference leak
From: Johan Hovold @ 2016-11-24 18:21 UTC (permalink / raw)
  To: David S. Miller
  Cc: Florian Fainelli, Madalin Bucur, Timur Tabi, Andrew Lunn,
	Vivien Didelot, netdev, linux-kernel, Johan Hovold
In-Reply-To: <1480011691-13278-1-git-send-email-johan@kernel.org>

Make sure to drop the reference taken by of_phy_find_device() when
looking up a fixed-link phydev during probe.

Fixes: 57ba4c9b56d8 ("fsl/fman: Add FMan MAC support")
Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/net/ethernet/freescale/fman/mac.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/net/ethernet/freescale/fman/mac.c b/drivers/net/ethernet/freescale/fman/mac.c
index 8fe6b3e253fa..736db9d9b0ad 100644
--- a/drivers/net/ethernet/freescale/fman/mac.c
+++ b/drivers/net/ethernet/freescale/fman/mac.c
@@ -892,6 +892,8 @@ static int mac_probe(struct platform_device *_of_dev)
 		priv->fixed_link->duplex = phy->duplex;
 		priv->fixed_link->pause = phy->pause;
 		priv->fixed_link->asym_pause = phy->asym_pause;
+
+		put_device(&phy->mdio.dev);
 	}
 
 	err = mac_dev->init(mac_dev);
-- 
2.7.3

^ permalink raw reply related

* Re: [PATCH v2] cpsw: ethtool: add support for getting/setting EEE registers
From: Florian Fainelli @ 2016-11-24 18:23 UTC (permalink / raw)
  To: Andrew Lunn, Yegor Yefremov, Giuseppe Cavallaro
  Cc: netdev, linux-omap@vger.kernel.org, Grygorii Strashko,
	N, Mugunthan V, Rami Rosen
In-Reply-To: <20161124153809.GA20455@lunn.ch>

+Peppe,

Le 24/11/2016 à 07:38, Andrew Lunn a écrit :
>> As for enabling advertising and correct working of cpsw do you mean it
>> would be better to disable EEE in any PHY on cpsw initialization as
>> long as cpsw doesn't provide support for EEE?
>>
>> We observe some strange behavior with our gigabit PHYs and a link
>> partner in a EEE-capable unmanaged NetGear switch. Disabling
>> advertising seems to help. Though we're still investigating the issue.
> 
> Hi Florian
> 
> Am i right in saying, a PHY should not advertise EEE until the MAC
> driver calls phy_init_eee(), indicating the MAC supports EEE?

You would think so, but I don't see how this could possibly work if that
was not the case already, see below.

> 
> If so, it looks like we need to change a few of the PHY drivers, in
> particular, the bcm-*.c.

The first part that bcm-phy-lib.c does is make sure that EEE is enabled
such that this gets reflected in MDIO_PCS_EEE_ABLE, without this, we
won't be able to pass the first test in phy_init_eee(). The second part
is to advertise EEE such that this gets reflected in MDIO_AN_EEE_ADV,
also to make sure that we can pass the second check in phy_init_eee().

Now, looking at phy_init_eee(), and what stmmac does (and bcmgenet,
copied after stmmac), we need to somehow, have EEE advertised for
phy_init_eee() to succeed, prepare the MAC to support EEE, and finally
conclude with a call to phy_ethtool_set_eee(), which writes to the
MDIO_AN_EEE_ADV register, and concludes the EEE auto-negotiated process.
Since we already have EEE advertised, we are essentially just checking
that the EEE advertised settings and the LP advertised settings actually
do match, so it sounds like the final call to phy_ethtool_set_eee() is
potentially useless if the resolved advertised and link partner
advertised settings already match...

So it sounds like at least, the first time you try to initialize EEE, we
should start with EEE not advertised, and then, if we have EEE enabled
at some point, and we re-negotiate the link parameters, somehow
phy_init_eee() does a right job for that.

Peppe, any thoughts on this?
-- 
Florian

^ permalink raw reply

* Re: [PATCH net 1/2] r8152: fix the sw rx checksum is unavailable
From: Mark Lord @ 2016-11-24 18:34 UTC (permalink / raw)
  To: David Miller; +Cc: hayeswang, netdev, nic_swsd, linux-kernel, linux-usb
In-Reply-To: <20161124.121140.2054576632424977475.davem@davemloft.net>

On 16-11-24 12:11 PM, David Miller wrote:
> From: Mark Lord <mlord@pobox.com>
> Date: Thu, 24 Nov 2016 11:43:53 -0500
> 
>> So even if this were a platform memory coherency issue, one should
>> still never see ASCII data at the beginning of an rx buffer.
> 
> I'm not so convinced, since this is the kind of random corruption one
> would expect to see when dealing with virtual caches that have
> aliasing or similar issues.
> 
> Writes to address X that show up at address Y or not at all are
> precisely the signature of virtual cache aliasing problems.
> 
> Is it a case of the chip writing to X but the cpu is still seeing
> stale data from a previous CPU store?
> 
> For NFS the cpu is writing into the page cache, so we know that
> cpu side stores are where the ASCII text is coming from.
> 
> Now is the r8152 buffer one that the USB host controller is DMA'ing
> into directly, or is it one that SWIOMMU or similar bounce buffering
> is copying into?  In the latter case we are doing cpu stores into
> the area and the writes aren't coming from the device.

>From tracing through the powerpc arch code, this is the buffer that
is being directly DMA'd into.  And the USB layer does an invalidate_dcache
on that entire buffer before initiating the DMA (confirmed via printk).

The driver itself NEVER writes anything to that buffer,
and nobody else has a pointer to it other than the USB host controller,
so there's nothing else that can write to it either.

According to the driver writer, the chip should only ever write a fresh
rx_desc struct at the beginning of a buffer, never ASCII data.

So how does that buffer end up containing ASCII data from the NFS transfers?

The only explanation I can see, is if the URB itself contains
the data that we see in the URB buffer.  Which is what one would expect.
So for that to happen, the ethernet chip must be transferring that data.

The thing that is special about the situation here, is that the processor
is very slow (800Mhz 32-bit powerpc), and very busy elsewhere.
So it can easily fall way behind in servicing the ethernet dongle,
something that never happens with most modern faster machines.
So perhaps this results in a FIFO overflow somewhere in the chip.

We can boot/run this same machine from a USB memory stick, and nary a problem.
Ditto for other types of ethernet dongles.
But boot/run from that specific ethernet dongle, and we get regular
random segfaults from corrupted page fetches over NFS.

The only end-to-end data integrity available here is the rx checksum,
when verified by software rather than trusting it to the chip/driver.

One thought:  bulk data streams are byte streams, not packets.
Scheduling on the USB bus can break up larger transfers across
multiple in-kernel buffers.  A "real" URB buffer on USB2 is max 512 bytes.
The driver is providing 16384-byte buffers, and assumes that data will
never spill over from one such buffer to the next.
Yet the observations here consistently show otherwise.

Cheers
-- 
Mark Lord

^ permalink raw reply

* Re: wl1251 & mac address & calibration data
From: Pali Rohár @ 2016-11-24 18:35 UTC (permalink / raw)
  To: Sebastian Reichel
  Cc: Pavel Machek, Michal Kazior, Kalle Valo, Ivaylo Dimitrov,
	Aaro Koskinen, Tony Lindgren, linux-wireless, Network Development,
	linux-kernel
In-Reply-To: <20161124181138.4i6ehkpohjxfgpbn@earth>

[-- Attachment #1: Type: Text/Plain, Size: 8119 bytes --]

On Thursday 24 November 2016 19:11:39 Sebastian Reichel wrote:
> Hi,
> 
> On Thu, Nov 24, 2016 at 05:49:33PM +0100, Pali Rohár wrote:
> > On Thursday 24 November 2016 17:08:30 Sebastian Reichel wrote:
> > > On Thu, Nov 24, 2016 at 04:20:45PM +0100, Pali Rohár wrote:
> > > > On Thursday 24 November 2016 16:13:17 Sebastian Reichel wrote:
> > > > > On Thu, Nov 24, 2016 at 09:33:29AM +0100, Pali Rohár wrote:
> > > > > > On Thursday 24 November 2016 08:51:04 Pavel Machek wrote:
> > > > > > > > > "ifconfig hw ether XX" normally sets the address. I
> > > > > > > > > guess that's ioctl?
> > > > > > > > 
> > > > > > > > This sets temporary address and it is ioctl. IIRC same
> > > > > > > > as what ethtool uses. (ifconfig is already
> > > > > > > > deprecated).
> > > > > > > > 
> > > > > > > > > And I guess we should use similar mechanism for
> > > > > > > > > permanent address.
> > > > > > > > 
> > > > > > > > I'm not sure here... Above ioctl ↑↑↑ is for changing
> > > > > > > > temporary mac address. But here we do not want to
> > > > > > > > change permanent mac address. We want to tell kernel
> > > > > > > > driver current permanent mac address which is stored
> > > > > > > 
> > > > > > > Well... I'd still use similar mechanism :-).
> > > > > > 
> > > > > > Thats problematic, because in time when wlan0 interface is
> > > > > > registered into system and visible in ifconfig output it
> > > > > > already needs to have permanent mac address assigned.
> > > > > > 
> > > > > > We should assign permanent mac address before wlan0 of
> > > > > > wl1251 is registered into system.
> > > > > 
> > > > > You can just add the MAC address to the NVS data, which is
> > > > > also required for the device initialization.
> > > > 
> > > > NVS data file has fixed size, there is IIRC no place for it.
> > > > 
> > > > But one of my suggestion was to use another request_firmware
> > > > for MAC address. So this is similar to what you are proposing,
> > > > as NVS data are loaded by request_firmware too...
> > > 
> > > Just append it to NVS data and modify the size check accordingly?
> > 
> > First that would mean to have request_firmware() function which
> > will not use direct VFS access, but instead use userspace helper.
> 
> Permanent MAC is device specific init data, NVS is device specific
> init data => load together.
> 
> The userspace helper stuff is only needed to get the data from
> the NAND calibration area on the fly.

But it is needed... and currently request_firmware() prefer direct VFS 
access...

> > NVS data file with some default values (not suitable for usage) is
> > already present in linux-firmware and available in
> > /lib/firmware/...
> 
> You mentioned NVS data is fixed size, so this should be enough?
> 
> switch(loaded_size)
>     case IMAGE_SIZE + MAC_SIZE:
>         /* extract mac */
>         /* fallthrough */
>     case IMAGE_SIZE:
>         /* load NVS */
>         break;
>     default:
>         /* fail */
> }
> 
> > Also I'm not sure if such thing is allowed by license:
> > https://git.kernel.org/cgit/linux/kernel/git/firmware/linux-firmwar
> > e.git/tree/LICENCE.ti-connectivity
> 
> concating data is not a modification, otherwise you can't
> put the file in a filesystem.

Ok, if net maintainers agree.

> > > > > I wonder if those information could be put into DT. Iirc some
> > > > > network devices get their MAC address from DT. Maybe we can
> > > > > add all NVS info to DT? How much data is it?
> > > > 
> > > > Proprietary, signed and closed bootloader NOLO does not support
> > > > DT. So for booting you need to append DTS file to kernel
> > > > image.
> > > 
> > > Yeah, so NOLO without U-Boot will depend on userspace to fixup
> > > DT.
> > > 
> > > > U-Boot is optional and can be used as intermediate bootloader
> > > > between NOLO and kernel. But still it has problems with reading
> > > > from nand, so cannot read NVS data nor MAC address.
> > > 
> > > It may in the future?
> > 
> > I already tried that, but I failed. I was not able to access N900's
> > nand from u-boot. No idea where was problem...
> > 
> > And if somebody fix onenand in u-boot, then needs to reimplement
> > Nokia's proprietary parser of that partition where is stored NVS
> > and mac address && make this support in upstream u-boot.
> 
> Are we talking about this?
> 
> https://github.com/community-ssu/libcal/blob/master/cal.c
> 
> That's ~1k loc for read-only.

Yes. There is also read-only alternative library:
https://github.com/pali/0xFFFF/blob/master/src/cal.c

But still, this is proprietary format useful only for one device (or 
two) and I do not know if such thing could be accepted by u-boot 
developers...

> Getting NAND working looks harder than porting this to me.

Right.

> > So... I doubt it will be in any future.
> > 
> > + no men power
> 
> yeah :(
> 
> But with that reasoning you should just extract the NVS data
> from NAND and put it in your rootfs.

Not a clean solution. Some rootfs parts can used as readonly. And 
normally rootfs is flashed into nand, so I still will say that roofs 
(image) should not contain any device specific data.

> > > > > Userspace application can add all those information to the DT
> > > > > using a DT overlay. Also the u-boot could parse and add it at
> > > > > some point in the future.
> > > > 
> > > > In case when wl1251 is statically linked into kernel image, it
> > > > is loaded and initialized before even userspace applications
> > > > starts.
> > > > 
> > > > So no... adding NVS data or MAC address into DT or DT overlay
> > > > is not a solution.
> > > 
> > > Actually with data loaded from DT you *can* load data quite early
> > > in the boot process, while your suggestions always require
> > > userspace. So you argument against yourself?
> > 
> > You cannot add DTS in uboot (no support).
> 
> AFAIK that's supported:
> 
> http://www.denx.de/wiki/DULG/LinuxFDTBlob
> 
> "Note that U-Boot makes some automatic modifications to the blob
> before passing it to the kernel - mainly adding and modifying
> information that is learnt at run-time."

I mean we do not have support for due to n900 nand.

> > And if you modify DTS on running kernel from userspace, then it is
> > too late when wl1251 is statically linked into kernel image.
> > 
> > wl1251 will not wait until some userspace modify DTS and add
> > data...
> 
> if (nvs info missing)
>     return -EPROBE_DEFER;

Forever? Only N times? How long? Only N second?

Here we do not know if userspace is going to send data or not.

My guess is that such code will not be accepted into net/wireless 
subsystem or drivers by maintainers.

> > But request_firmware() in fallback mode *can* wait for userspace so
> > wl1251 can postpone its operation until mdev/udev/whatever starts
> > listening for events and push needed firmware data to kernel.
> 
> As you said one workaround is waiting. That's not a solution, that
> only works with request_firmware().

Yes, but request_firmware() uses interaction with userspace. Your 
proposed solution does not do any interaction with userspace that some 
process needs to fill missing data into DT.

And request_firmware() is already used for loading NVS data.

> > So no, there is no argument against... request_firmware() in
> > fallback mode with userspace helper is by design blocking and
> > waiting for userspace. But waiting for some change in DTS in
> > kernel is just nonsense.
> 
> I would just mark the wlan device with status = "disabled" and
> enable it in the overlay together with adding the NVS & MAC info.

So if you think that this solution make sense, we can wait what net 
wireless maintainers say about it...

For me it looks like that solution can be:

extending request_firmware() to use only userspace helper

and load mac address also via request_firmware() either by appending it 
into NVS data or via separate call

-- 
Pali Rohár
pali.rohar@gmail.com

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

^ permalink raw reply

* Re: [PATCH net 1/2] r8152: fix the sw rx checksum is unavailable
From: Greg KH @ 2016-11-24 18:42 UTC (permalink / raw)
  To: Mark Lord
  Cc: David Miller, hayeswang, netdev, nic_swsd, linux-kernel,
	linux-usb
In-Reply-To: <23e0c132-8844-0a34-3e0b-e412f76493ba@pobox.com>

On Thu, Nov 24, 2016 at 11:43:53AM -0500, Mark Lord wrote:
> On 16-11-24 11:21 AM, David Miller wrote:
> > From: Hayes Wang <hayeswang@realtek.com>
> > Date: Thu, 24 Nov 2016 13:26:55 +0000
> > 
> > > I don't think the garbage results from our driver or device.
> > This is my impression with what has been presented so far as well.
> 
> It's not garbage.
> 
> The latest run with the debug code I posted here earlier just spat out this below.
> Using coherent (guarded, non-cacheable) RX buffers, with mb() calls:
> 
> [   15.199157] r8152_check_rx_desc: rx_desc looks bad.
> [   15.204270] r8152_rx_bottom: offset=0/3376 bad rx_desc
> [   15.209584] r8152_dump_rx_desc: 3d435253 3034336d 202f3a30 47524154 2f3d5445 3034336d rx_len=21075
> 
> The bad data in this case is ASCII:
> 
>         "SRC=m3400:/ TARGET=/m340"

Have you tried using usbmon?  Details for how to use it is in
Documentation/usbmon.txt and it might help you rule out the driver vs.
the USB host controller issues as it sees the raw data the USB host
controller sees before it sends it to the driver.

thanks,

greg k-h

^ permalink raw reply


This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox