* RE: [PATCHv2 net-next 04/12] sctp: implement make_datafrag for sctp_stream_interleave
From: David Laight @ 2017-12-08 16:22 UTC (permalink / raw)
To: 'Xin Long', Marcelo Ricardo Leitner
Cc: Neil Horman, network dev, linux-sctp@vger.kernel.org,
davem@davemloft.net
In-Reply-To: <CADvbK_ckOgH9vzXVzckEtWAkJaYn9wuhJtZ+qpzep2T6C8Wung@mail.gmail.com>
From: Xin Long
> Sent: 08 December 2017 16:18
>
...
> >> Alternatively you could preform the dereference in two steps (i.e. declare an si
> >> pointer on the stack and set it equal to asoc->stream.si, then deref
> >> si->make_datafrag at call time. That will at least give the compiler an
> >> opportunity to preload the first pointer.
You want to save the function pointer itself.
...
> Another small difference:
> as you can see, comparing to (X), (Y) is using 0x28(%rsp) in the loop,
> instead of %r13.
>
> So that's what I can see from the related generated code.
> If 0x848(%r13) is not worse than 0x28(%rsp) for cpu, I think
> asoc->stream.si->make_datafrag() is even better. No ?
That code must have far too many life local variables.
Otherwise there's be a caller saved register available.
David
^ permalink raw reply
* RE: [PATCH net 3/3] hv_netvsc: Fix the default receive buffer size
From: Haiyang Zhang @ 2017-12-08 16:18 UTC (permalink / raw)
To: Dan Carpenter, Stephen Hemminger
Cc: devel@linuxdriverproject.org, Stephen Hemminger,
netdev@vger.kernel.org
In-Reply-To: <20171208104459.wtwexebad4acbod3@mwanda>
> -----Original Message-----
> From: Dan Carpenter [mailto:dan.carpenter@oracle.com]
> Sent: Friday, December 8, 2017 5:45 AM
> To: Stephen Hemminger <stephen@networkplumber.org>
> Cc: KY Srinivasan <kys@microsoft.com>; Haiyang Zhang
> <haiyangz@microsoft.com>; Stephen Hemminger
> <sthemmin@microsoft.com>; devel@linuxdriverproject.org;
> netdev@vger.kernel.org
> Subject: Re: [PATCH net 3/3] hv_netvsc: Fix the default receive buffer size
>
> On Thu, Dec 07, 2017 at 04:10:55PM -0800, Stephen Hemminger wrote:
> > From: Haiyang Zhang <haiyangz@microsoft.com>
> >
> > The intended size is 16 MB, and the default slot size is 1728.
> > So, NETVSC_DEFAULT_RX should be 16*1024*1024 / 1728 = 9709.
> >
> > Fixes: 5023a6db73196 ("netvsc: increase default receive buffer size")
> > Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
> > Signed-off-by: Stephen Hemminger <sthemmin@microsoft.com>
> > ---
> > drivers/net/hyperv/netvsc_drv.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/net/hyperv/netvsc_drv.c
> > b/drivers/net/hyperv/netvsc_drv.c index dc70de674ca9..edfcde5d3621
> > 100644
> > --- a/drivers/net/hyperv/netvsc_drv.c
> > +++ b/drivers/net/hyperv/netvsc_drv.c
> > @@ -50,7 +50,7 @@
> > #define NETVSC_MIN_TX_SECTIONS 10
> > #define NETVSC_DEFAULT_TX 192 /* ~1M */
> > #define NETVSC_MIN_RX_SECTIONS 10 /* ~64K */
> > -#define NETVSC_DEFAULT_RX 10485 /* Max ~16M */
> > +#define NETVSC_DEFAULT_RX 9709 /* ~16M */
>
> How does this bug look like to the user? Memory corruption?
>
> It's weird to me reviewing this code that the default sizes are stored in
> netvsc_drv.c and the max sizes are stored in hyperv_net.h.
> Could we move these to hyperv_net.h? We could write it like:
> #define NETVSC_DEFAULT_RX ((16 * 1024 * 1024) /
> NETVSC_RECV_SECTION_SIZE)
>
> 16MB is sort of a weird default because it's larger than the 15MB allowed for
> legacy versions, but it's smaller than the 32MB you'd want for the current
> versions.
As a tradeoff between traffic burst and latency, we set the default to be 16MB.
And this default is reduced automatically for legacy hosts based on the NVSP
version in patch 2.
I will move the change to hyperv_net.h
Thanks,
- Haiyang
^ permalink raw reply
* Re: [PATCHv2 net-next 04/12] sctp: implement make_datafrag for sctp_stream_interleave
From: Xin Long @ 2017-12-08 16:17 UTC (permalink / raw)
To: Marcelo Ricardo Leitner
Cc: Neil Horman, David Laight, network dev,
linux-sctp@vger.kernel.org, davem@davemloft.net
In-Reply-To: <20171208160001.GF3328@localhost.localdomain>
On Sat, Dec 9, 2017 at 12:00 AM, Marcelo Ricardo Leitner
<marcelo.leitner@gmail.com> wrote:
> On Fri, Dec 08, 2017 at 10:37:34AM -0500, Neil Horman wrote:
>> On Fri, Dec 08, 2017 at 12:56:30PM -0200, Marcelo Ricardo Leitner wrote:
>> > On Fri, Dec 08, 2017 at 02:06:04PM +0000, David Laight wrote:
>> > > From: Xin Long
>> > > > Sent: 08 December 2017 13:04
>> > > ...
>> > > > @@ -264,8 +264,8 @@ struct sctp_datamsg *sctp_datamsg_from_user(struct sctp_association *asoc,
>> > > > frag |= SCTP_DATA_SACK_IMM;
>> > > > }
>> > > >
>> > > > - chunk = sctp_make_datafrag_empty(asoc, sinfo, len, frag,
>> > > > - 0, GFP_KERNEL);
>> > > > + chunk = asoc->stream.si->make_datafrag(asoc, sinfo, len, frag,
>> > > > + GFP_KERNEL);
>> > >
>> > > I know that none of the sctp code is very optimised, but that indirect
>> > > call is going to be horrid.
>> >
>> > Yeah.. but there is no way to avoid the double derreference
>> > considering we only have the asoc pointer in there and we have to
>> > reach the contents of the data chunk operations struct, and the .si
>> > part is the same as 'stream' part as it's a constant offset.
>> >
>> > Due to the for() in there, we could add a variable to store
>> > asoc->stream.si outside the for and then we can do only a single deref
>> > inside it. Xin, can you please try and see if the generated code is
>> > different?
>> >
>> > Other suggestions?
>> >
>> Is it worth replacing the si struct with an index/enum value, and indexing an
>> array of method pointer structs? That would save you at least one dereference.
>
> Hmmm, maybe, yes. It would be like
> sctp_stream_interleave[asoc->stream.si].make_datafrag(...)
>
> Then same goes for pf->af, probably.
>
>>
>> Alternatively you could preform the dereference in two steps (i.e. declare an si
>> pointer on the stack and set it equal to asoc->stream.si, then deref
>> si->make_datafrag at call time. That will at least give the compiler an
>> opportunity to preload the first pointer.
>
> Yep, that was my 2nd paragraph above :-) but it only works for cases
> such as this one.
Now:
for(N) {
...
chunk = asoc->stream.si->make_datafrag(asoc, sinfo, len, frag,
0x000000000000fb58 <+360>: mov 0x848(%r13),%rax <---- [a]
0x000000000000fb5f <+367>: movzbl %cl,%ecx
0x000000000000fb62 <+370>: mov $0x14000c0,%r8d
0x000000000000fb68 <+376>: mov %r12d,%edx
0x000000000000fb6b <+379>: mov (%rsp),%rsi
0x000000000000fb6f <+383>: mov %r13,%rdi <=(X)
0x000000000000fb72 <+386>: callq *0x8(%rax) <---- [b]
0x000000000000fb78 <+392>: mov %rax,%r15
}
ret = N * ([a] + [b])
After using a variable:
struct sctp_stream_interleave *si;
...
si = asoc->stream.si;
0x000000000000fb44 <+340>: mov 0x848(%r14),%rax
0x000000000000fb4e <+350>: mov %rax,0x20(%rsp) <----- [1]
for(N) {
...
chunk = si->make_datafrag(asoc, sinfo, len, frag, GFP_KERNEL);
0x000000000000fb69 <+377>: mov 0x20(%rsp),%rax <----- [2]
0x000000000000fb6e <+382>: movzbl %cl,%ecx
0x000000000000fb71 <+385>: mov $0x14000c0,%r8d
0x000000000000fb77 <+391>: mov %r12d,%edx
0x000000000000fb7a <+394>: mov (%rsp),%rsi
0x000000000000fb7e <+398>: mov 0x28(%rsp),%rdi <=(Y)
0x000000000000fb83 <+403>: callq *0x8(%rax) <----- [3]
0x000000000000fb89 <+409>: mov %rax,%r14
}
ret = [1] + N * ([2] + [3])
Another small difference:
as you can see, comparing to (X), (Y) is using 0x28(%rsp) in the loop,
instead of %r13.
So that's what I can see from the related generated code.
If 0x848(%r13) is not worse than 0x28(%rsp) for cpu, I think
asoc->stream.si->make_datafrag() is even better. No ?
^ permalink raw reply
* Re: [PATCH RFC 0/4] Fixes for Marvell MII paged register access races
From: Andrew Lunn @ 2017-12-08 16:17 UTC (permalink / raw)
To: Russell King - ARM Linux; +Cc: Florian Fainelli, netdev
In-Reply-To: <20171208154756.GF10595@n2100.armlinux.org.uk>
Hi Russell
> There is an open question whether there should be generic helpers for
> this. Generic helpers would mean:
>
> - Additional couple of function pointers in phy_driver to read/write the
> paging register. This has the restriction that there must only be one
> paging register.
I must be missing something. I don't see why there is this
restriction. Don't we just need
int phy_get_page(phydev);
int phy_set_page(phydev, page);
We might even be able to do without phy_get_page(), if we assume page
0 should be selected by default, and all paged reads/write return to
page 0 afterwards.
> - The helpers become more expensive, and because they're in a separate
> compilation unit, the compiler will be unable to optimise them by
> inlining the static functions.
The mdio bus is slow. Often there is a completion function triggered
by an interrupt etc. Worst case it is bit-banging. I suspect the gains
from inlining are just noise in the bigger picture.
> - The helpers would be re-usable, saving replications of that code, and
> making it more likely for phy authors to safely access the PHY.
This is the key point for me. This has likely to of been broken for
years. If it is obviously broken, driver writers are more likely to
get it right. Here it is not obvious, so we should take it out of the
driver writers hands and do it in the core.
> Another potential question is whether using the mdiobus lock (which
> excludes all other MII bus access) is best - while it has the advantage
> of also ensuring atomicity with userspace accesses, it means that no one
> else can access an independent PHY on the same bus while a paged access
> is on-going. It feels like a big hammer, but I'm not convinced that we
> will see a lot of contention on it.
I think you are right about there being little contention on the lock.
I suspect most paged accesses are performed during initial setup and
configuration. I guess the once per second poll does not need to use
paged registers. And the weight of that hammer can be reduced a lot by
using interrupts instead of polling.
Andrew
^ permalink raw reply
* RE: [PATCH net 1/3] hv_netvsc: Correct the max receive buffer size
From: Haiyang Zhang @ 2017-12-08 16:11 UTC (permalink / raw)
To: Dan Carpenter, Stephen Hemminger
Cc: KY Srinivasan, Stephen Hemminger, devel@linuxdriverproject.org,
netdev@vger.kernel.org
In-Reply-To: <20171208103325.xt6nrzvxijewbdkl@mwanda>
> -----Original Message-----
> From: Dan Carpenter [mailto:dan.carpenter@oracle.com]
> Sent: Friday, December 8, 2017 5:33 AM
> To: Stephen Hemminger <stephen@networkplumber.org>
> Cc: KY Srinivasan <kys@microsoft.com>; Haiyang Zhang
> <haiyangz@microsoft.com>; Stephen Hemminger
> <sthemmin@microsoft.com>; devel@linuxdriverproject.org;
> netdev@vger.kernel.org
> Subject: Re: [PATCH net 1/3] hv_netvsc: Correct the max receive buffer size
>
> On Thu, Dec 07, 2017 at 04:10:53PM -0800, Stephen Hemminger wrote:
> > From: Haiyang Zhang <haiyangz@microsoft.com>
> >
> > It should be 31 MB on recent host versions.
> >
> > Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
> > Signed-off-by: Stephen Hemminger <sthemmin@microsoft.com>
>
> This is very vague. What does "recent" mean in this context? There are also
> some unrelated white space changes here which make the patch harder to
> read.
>
> This patch kind of makes the bug fixed by patch 2 even worse because
> before the receive buffer was capped at around 16MB and now we can set
> the receive buffer to 31MB. It might make sense to fold the two patches
> together.
>
> Is patch 2 a memory corruption bug? The changelog doesn't really say what
> the user visible effects of the bug are. Basically if you make the buffer too
> small then it's a performance issue but if you make it too large what happens?
> It's not clear to me.
For NVSP v2, and earlier host, the limit is 15MB. Later hosts, the limit is 31MB.
Setting beyond it will be denied by the host, resulting the vNIC doesn't come up.
I will merge this one together with the patch 2.
Thanks,
- Haiyang
^ permalink raw reply
* Re: [PATCHv2 net-next 04/12] sctp: implement make_datafrag for sctp_stream_interleave
From: Neil Horman @ 2017-12-08 16:08 UTC (permalink / raw)
To: David Laight
Cc: 'Marcelo Ricardo Leitner', 'Xin Long',
network dev, linux-sctp@vger.kernel.org, davem@davemloft.net
In-Reply-To: <e3256db09d434461acb5861b30ef6f65@AcuMS.aculab.com>
On Fri, Dec 08, 2017 at 04:04:58PM +0000, David Laight wrote:
> From: Marcelo Ricardo Leitner
> > Sent: 08 December 2017 16:00
> ...
> > > Is it worth replacing the si struct with an index/enum value, and indexing an
> > > array of method pointer structs? That would save you at least one dereference.
> >
> > Hmmm, maybe, yes. It would be like
> > sctp_stream_interleave[asoc->stream.si].make_datafrag(...)
>
> If you only expect 2 choices then an if () is likely
> to produce better code that the above.
>
> The actual implementation can be hidden inside a #define
> or static inline function.
>
Thats the real question though, will we expect more than two interleaving
strategies? Currently its a boolean operation so the answer seems like yes, but
is there a possiblity of a biased interleaving, or other worthwhile algorithm?
Neil
> David
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-sctp" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
^ permalink raw reply
* Re: pull-request: bpf-next 2017-12-07
From: David Miller @ 2017-12-08 16:06 UTC (permalink / raw)
To: ast; +Cc: daniel, netdev
In-Reply-To: <20171208062211.2545706-1-ast@kernel.org>
From: Alexei Starovoitov <ast@kernel.org>
Date: Thu, 7 Dec 2017 22:22:11 -0800
> The following pull-request contains BPF updates for your net-next tree.
>
> The main changes are:
>
> 1) Detailed documentation of BPF development process from Daniel.
>
> 2) Addition of is_fullsock, snd_cwnd and srtt_us fields to bpf_sock_ops
> from Lawrence.
>
> 3) Minor follow up for bpf_skb_set_tunnel_key() from William.
>
> Please consider pulling these changes from:
>
> git://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git
Pulled, thanks Alexei.
^ permalink raw reply
* [PATCH] net: dsa: allow XAUI phy interface mode
From: Russell King @ 2017-12-08 16:04 UTC (permalink / raw)
To: Andrew Lunn, Vivien Didelot; +Cc: Florian Fainelli, netdev
XGMII is a 32-bit bus plus two clock signals per direction. XAUI is
four serial lanes per direction. The 88e6190 supports XAUI but not
XGMII as it doesn't have enough pins. The same is true of 88e6176.
Match on PHY_INTERFACE_MODE_XAUI for the XAUI port type, but keep
accepting XGMII for backwards compatibility.
Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
---
drivers/net/dsa/mv88e6xxx/port.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/net/dsa/mv88e6xxx/port.c b/drivers/net/dsa/mv88e6xxx/port.c
index a7801f6668a5..6315774d72b3 100644
--- a/drivers/net/dsa/mv88e6xxx/port.c
+++ b/drivers/net/dsa/mv88e6xxx/port.c
@@ -338,6 +338,7 @@ int mv88e6390x_port_set_cmode(struct mv88e6xxx_chip *chip, int port,
cmode = MV88E6XXX_PORT_STS_CMODE_2500BASEX;
break;
case PHY_INTERFACE_MODE_XGMII:
+ case PHY_INTERFACE_MODE_XAUI:
cmode = MV88E6XXX_PORT_STS_CMODE_XAUI;
break;
case PHY_INTERFACE_MODE_RXAUI:
--
2.7.4
^ permalink raw reply related
* RE: [PATCHv2 net-next 04/12] sctp: implement make_datafrag for sctp_stream_interleave
From: David Laight @ 2017-12-08 16:04 UTC (permalink / raw)
To: 'Marcelo Ricardo Leitner', Neil Horman
Cc: 'Xin Long', network dev, linux-sctp@vger.kernel.org,
davem@davemloft.net
In-Reply-To: <20171208160001.GF3328@localhost.localdomain>
From: Marcelo Ricardo Leitner
> Sent: 08 December 2017 16:00
...
> > Is it worth replacing the si struct with an index/enum value, and indexing an
> > array of method pointer structs? That would save you at least one dereference.
>
> Hmmm, maybe, yes. It would be like
> sctp_stream_interleave[asoc->stream.si].make_datafrag(...)
If you only expect 2 choices then an if () is likely
to produce better code that the above.
The actual implementation can be hidden inside a #define
or static inline function.
David
^ permalink raw reply
* Re: Linux 4.14 - regression: broken tun/tap / bridge network with virtio - bisected
From: Willem de Bruijn @ 2017-12-08 16:04 UTC (permalink / raw)
To: Michal Kubecek
Cc: Andreas Hartmann, Jason Wang, David Miller, Network Development
In-Reply-To: <20171208114025.kjcaratqcveq7zu5@unicorn.suse.cz>
On Fri, Dec 8, 2017 at 6:40 AM, Michal Kubecek <mkubecek@suse.cz> wrote:
> On Fri, Dec 08, 2017 at 11:31:50AM +0100, Andreas Hartmann wrote:
>> On 12/08/2017 at 09:47 AM Michal Kubecek wrote:
>> > On Fri, Dec 08, 2017 at 08:21:16AM +0100, Andreas Hartmann wrote:
>> >>
>> >> All my VMs are using virtio_net. BTW: I couldn't see the problems
>> >> (sometimes, the VM couldn't be stopped at all) if all my VMs are using
>> >> e1000 as interface instead.
>> >>
>> >> This finding now matches pretty much the responsible UDP-package which
>> >> caused the stall. I already mentioned it here [2].
>> >>
>> >> To prove it, I reverted from the patch series "[PATCH v2 RFC 0/13]
>> >> Remove UDP Fragmentation Offload support" [3]
>> >>
>> >> 11/13 [v2,RFC,11/13] net: Remove all references to SKB_GSO_UDP. [4]
>> >> 12/13 [v2,RFC,12/13] inet: Remove software UFO fragmenting code. [5]
>> >> 13/13 [v2,RFC,13/13] net: Kill NETIF_F_UFO and SKB_GSO_UDP. [6]
>> >>
>> >> and applied it to Linux 4.14.4. It compiled fine and is running fine.
>> >> The vnet doesn't die anymore. Yet, I can't say if the qemu stop hangs
>> >> are gone, too.
>> >>
>> >> Obviously, there is something broken with the new UDP handling. Could
>> >> you please analyze this problem? I could test some more patches ... .
>> >
>> > Any chance your VMs were live migrated from pre-4.14 host kernel?
>>
>> No - the VMs are not live migrated. They are always running on the same
>> host - either with kernel < 4.14 or with kernel 4.14.x.
>
> This is disturbing... unless I'm mistaken, it shouldn't be possible to
> have UFO enabled on a virtio device in a VM booted on a host with 4.14
> kernel.
Indeed. When working on that revert patch I verified that UFO in
the guest virtio_net was off before the revert patch, on after.
Qemu should check host support with tap_probe_has_ufo
before advertising support to the guest. Indeed, this is exactly
what broke live migration in virtio_net_load_device at
if (qemu_get_byte(f) && !peer_has_ufo(n)) {
error_report("virtio-net: saved image requires TUN_F_UFO support");
return -1;
}
Which follows
peer_has_ufo
qemu_has_ufo
tap_has_ufo
s->has_ufo
where s->has_ufo was set by tap_probe_has_ufo in net_tap_fd_init.
Now, checking my qemu git branch, I ran pretty old 2.7.0-rc3. But this
codepath does not seem to have changed between then and 2.10.1.
I cherry-picked the revert onto 4.14.3. It did not apply cleanly, but the
fix-up wasn't too hard. Compiled and booted, but untested otherwise. At
https://github.com/wdebruij/linux/commits/v4.14.3-aargh-ufo
>
>> > If this is the case, you should try commit 0c19f846d582 ("net:
>> > accept UFO datagrams from tuntap and packet").
>>
>> It doesn't apply to 4.14.4
>>
>> > Or disabling UFO in the guest should
>> > work around the issue.
>>
>> ethtool -K ethX ufo off for each device / bridge in VM.
>>
>> Yes, this seems to work. I'll wait and see if the non stoppable
>> qemu-problem on shutdown will remain.
>>
>> When will there be a fix for 4.14? It is clearly a regression. Is it
>> possible / a good idea to just remove the complete patch series "Remove
>> UDP Fragmentation Offload support"?
>
> I cannot give an exact date but the patch is queued for stable
> (see http://patchwork.ozlabs.org/bundle/davem/stable/?state=* ) so that
> it should land in stable-4.14 in near future (weeks at most).
>
> Michal Kubecek
>
^ permalink raw reply
* Re: [PATCH v5 net-next] net/tcp: trace all TCP/IP state transition with tcp_set_state tracepoint
From: David Miller @ 2017-12-08 16:03 UTC (permalink / raw)
To: marcelo.leitner
Cc: laoar.shao, songliubraving, kuznet, yoshfuji, rostedt, bgregg,
netdev, linux-kernel
In-Reply-To: <20171208110356.GS13341@localhost.localdomain>
From: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
Date: Fri, 8 Dec 2017 09:03:56 -0200
> I'm okay with this v5 then, can't see a better way around it.
I would suggest not making this a TCP specific feature.
Because socket state changes are not a TCP specific behavior.
^ permalink raw reply
* Re: [PATCH net 1/3] hv_netvsc: Correct the max receive buffer size
From: David Miller @ 2017-12-08 16:03 UTC (permalink / raw)
To: dan.carpenter; +Cc: stephen, kys, haiyangz, sthemmin, devel, netdev
In-Reply-To: <20171208103325.xt6nrzvxijewbdkl@mwanda>
From: Dan Carpenter <dan.carpenter@oracle.com>
Date: Fri, 8 Dec 2017 13:33:25 +0300
> On Thu, Dec 07, 2017 at 04:10:53PM -0800, Stephen Hemminger wrote:
>> From: Haiyang Zhang <haiyangz@microsoft.com>
>>
>> It should be 31 MB on recent host versions.
>>
>> Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
>> Signed-off-by: Stephen Hemminger <sthemmin@microsoft.com>
>
> This is very vague. What does "recent" mean in this context? There are
> also some unrelated white space changes here which make the patch harder
> to read.
>
> This patch kind of makes the bug fixed by patch 2 even worse because
> before the receive buffer was capped at around 16MB and now we can set
> the receive buffer to 31MB. It might make sense to fold the two
> patches together.
>
> Is patch 2 a memory corruption bug? The changelog doesn't really say
> what the user visible effects of the bug are. Basically if you make the
> buffer too small then it's a performance issue but if you make it too
> large what happens? It's not clear to me.
Agreed with Dan, we definitely need more verbose and detailed commit
log messages for this series.
^ permalink raw reply
* Re: [PATCHv2 net-next 04/12] sctp: implement make_datafrag for sctp_stream_interleave
From: 'Marcelo Ricardo Leitner' @ 2017-12-08 16:02 UTC (permalink / raw)
To: David Laight
Cc: 'Xin Long', network dev, linux-sctp@vger.kernel.org,
Neil Horman, davem@davemloft.net
In-Reply-To: <60df84ed900b49799782a43c84be0bc4@AcuMS.aculab.com>
On Fri, Dec 08, 2017 at 03:32:54PM +0000, David Laight wrote:
> From: 'Marcelo Ricardo Leitner'
> > Sent: 08 December 2017 15:16
> > On Fri, Dec 08, 2017 at 03:01:31PM +0000, David Laight wrote:
> > > From: Marcelo Ricardo Leitner
> > > > Sent: 08 December 2017 14:57
> > > >
> > > > On Fri, Dec 08, 2017 at 02:06:04PM +0000, David Laight wrote:
> > > > > From: Xin Long
> > > > > > Sent: 08 December 2017 13:04
> > > > > ...
> > > > > > @@ -264,8 +264,8 @@ struct sctp_datamsg *sctp_datamsg_from_user(struct sctp_association *asoc,
> > > > > > frag |= SCTP_DATA_SACK_IMM;
> > > > > > }
> > > > > >
> > > > > > - chunk = sctp_make_datafrag_empty(asoc, sinfo, len, frag,
> > > > > > - 0, GFP_KERNEL);
> > > > > > + chunk = asoc->stream.si->make_datafrag(asoc, sinfo, len, frag,
> > > > > > + GFP_KERNEL);
> > > > >
> > > > > I know that none of the sctp code is very optimised, but that indirect
> > > > > call is going to be horrid.
> > > >
> > > > Yeah.. but there is no way to avoid the double derreference
> > > > considering we only have the asoc pointer in there and we have to
> > > > reach the contents of the data chunk operations struct, and the .si
> > > > part is the same as 'stream' part as it's a constant offset.
> > > ...
> > >
> > > It isn't only the double indirect, the indirect call itself isn't 'fun'.
> >
> > I meant in this context.
> >
> > The indirect call is so we don't have to flood the stack with
> > if (old data chunk fmt) {
> > ...
> > } else {
> > ...
> > }
> >
> > So instead of this, we now have some key operations identified and
> > wrapped up behind this struct, allowing us to abstract whatever data
> > chunk format it is.
>
> Nothing wrong with:
> #define foo(asoc, ...) \
> if (asoc->new_fmt) \
Not all function pointers in sctp_stream_interleave have asoc as
a parameter, so we would have to have something like:
#define foo_asoc(asoc, ...) \
if (asoc->new_fmt) \
...
#define foo_chunk(chunk, ...) \
if (chunk->asoc->new_fmt) \
...
#define foo_ulpq(ulpq, ...) \
if (ulpq->asoc->new_fmt) \
...
and we're pretty much back to double deref.
Maybe some reworking on the parameters could alleviate some of these,
but not all.
> foo_new(asoc, __VA_LIST__); \
> else \
> foo_old(asoc, __VA_LIST__);
>
> > > I think there are other hot paths where you've replaced a sizeof()
> > > with a ?: clause.
> > > Caching the result might be much better.
> >
> > The only new ?: clause I could find this patchset is on patch 12 and
> > has nothing to do with sizeof().
> >
> > The sizeof() results are indeed cached, as you can see in patch 4:
> > +static struct sctp_stream_interleave sctp_stream_interleave_0 = {
> > + .data_chunk_len = sizeof(struct sctp_data_chunk),
> > and the two helpers on it at the begining of the patch.
>
> I was getting two bits mixed up.
> But the code that reads data_chunk_len is following an awful lot of pointers.
>From path 4:
max_data = asoc->pathmtu -
sctp_sk(asoc->base.sk)->pf->af->net_header_len -
- sizeof(struct sctphdr) - sizeof(struct sctp_data_chunk);
+ sizeof(struct sctphdr) - sctp_datachk_len(&asoc->stream);
You're worried with the double deref in sctp_datachk_len() but on the
line right above it we have 4 derefs. There are several other cases
of double deref in sctp stack even on hot path. Not sure why you're
picking on this one, but ok.
Marcelo
^ permalink raw reply
* Re: [PATCHv2 net-next 04/12] sctp: implement make_datafrag for sctp_stream_interleave
From: Marcelo Ricardo Leitner @ 2017-12-08 16:00 UTC (permalink / raw)
To: Neil Horman
Cc: David Laight, 'Xin Long', network dev,
linux-sctp@vger.kernel.org, davem@davemloft.net
In-Reply-To: <20171208153734.GB6955@hmswarspite.think-freely.org>
On Fri, Dec 08, 2017 at 10:37:34AM -0500, Neil Horman wrote:
> On Fri, Dec 08, 2017 at 12:56:30PM -0200, Marcelo Ricardo Leitner wrote:
> > On Fri, Dec 08, 2017 at 02:06:04PM +0000, David Laight wrote:
> > > From: Xin Long
> > > > Sent: 08 December 2017 13:04
> > > ...
> > > > @@ -264,8 +264,8 @@ struct sctp_datamsg *sctp_datamsg_from_user(struct sctp_association *asoc,
> > > > frag |= SCTP_DATA_SACK_IMM;
> > > > }
> > > >
> > > > - chunk = sctp_make_datafrag_empty(asoc, sinfo, len, frag,
> > > > - 0, GFP_KERNEL);
> > > > + chunk = asoc->stream.si->make_datafrag(asoc, sinfo, len, frag,
> > > > + GFP_KERNEL);
> > >
> > > I know that none of the sctp code is very optimised, but that indirect
> > > call is going to be horrid.
> >
> > Yeah.. but there is no way to avoid the double derreference
> > considering we only have the asoc pointer in there and we have to
> > reach the contents of the data chunk operations struct, and the .si
> > part is the same as 'stream' part as it's a constant offset.
> >
> > Due to the for() in there, we could add a variable to store
> > asoc->stream.si outside the for and then we can do only a single deref
> > inside it. Xin, can you please try and see if the generated code is
> > different?
> >
> > Other suggestions?
> >
> Is it worth replacing the si struct with an index/enum value, and indexing an
> array of method pointer structs? That would save you at least one dereference.
Hmmm, maybe, yes. It would be like
sctp_stream_interleave[asoc->stream.si].make_datafrag(...)
Then same goes for pf->af, probably.
>
> Alternatively you could preform the dereference in two steps (i.e. declare an si
> pointer on the stack and set it equal to asoc->stream.si, then deref
> si->make_datafrag at call time. That will at least give the compiler an
> opportunity to preload the first pointer.
Yep, that was my 2nd paragraph above :-) but it only works for cases
such as this one.
Marcelo
>
> Neil
>
> > Marcelo
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-sctp" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at http://vger.kernel.org/majordomo-info.html
> >
> --
> To unsubscribe from this list: send the line "unsubscribe linux-sctp" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
^ permalink raw reply
* [PATCH 6/6] can: peak/pcie_fd: fix potential bug in restarting tx queue
From: Marc Kleine-Budde @ 2017-12-08 15:58 UTC (permalink / raw)
To: netdev
Cc: davem, linux-can, kernel, Stephane Grosjean, linux-stable,
Marc Kleine-Budde
In-Reply-To: <20171208155817.9364-1-mkl@pengutronix.de>
From: Stephane Grosjean <s.grosjean@peak-system.com>
Don't rely on can_get_echo_skb() return value to wake the network tx
queue up: can_get_echo_skb() returns 0 if the echo array slot was not
occupied, but also when the DLC of the released echo frame was 0.
Signed-off-by: Stephane Grosjean <s.grosjean@peak-system.com>
Cc: linux-stable <stable@vger.kernel.org>
Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
drivers/net/can/peak_canfd/peak_canfd.c | 9 +++------
1 file changed, 3 insertions(+), 6 deletions(-)
diff --git a/drivers/net/can/peak_canfd/peak_canfd.c b/drivers/net/can/peak_canfd/peak_canfd.c
index 85268be0c913..55513411a82e 100644
--- a/drivers/net/can/peak_canfd/peak_canfd.c
+++ b/drivers/net/can/peak_canfd/peak_canfd.c
@@ -258,21 +258,18 @@ static int pucan_handle_can_rx(struct peak_canfd_priv *priv,
/* if this frame is an echo, */
if ((rx_msg_flags & PUCAN_MSG_LOOPED_BACK) &&
!(rx_msg_flags & PUCAN_MSG_SELF_RECEIVE)) {
- int n;
unsigned long flags;
spin_lock_irqsave(&priv->echo_lock, flags);
- n = can_get_echo_skb(priv->ndev, msg->client);
+ can_get_echo_skb(priv->ndev, msg->client);
spin_unlock_irqrestore(&priv->echo_lock, flags);
/* count bytes of the echo instead of skb */
stats->tx_bytes += cf_len;
stats->tx_packets++;
- if (n) {
- /* restart tx queue only if a slot is free */
- netif_wake_queue(priv->ndev);
- }
+ /* restart tx queue (a slot is free) */
+ netif_wake_queue(priv->ndev);
return 0;
}
--
2.15.0
^ permalink raw reply related
* [PATCH 5/6] can: usb_8dev: cancel urb on -EPIPE and -EPROTO
From: Marc Kleine-Budde @ 2017-12-08 15:58 UTC (permalink / raw)
To: netdev
Cc: davem, linux-can, kernel, Martin Kelly, linux-stable,
Marc Kleine-Budde
In-Reply-To: <20171208155817.9364-1-mkl@pengutronix.de>
From: Martin Kelly <mkelly@xevo.com>
In mcba_usb, we have observed that when you unplug the device, the driver will
endlessly resubmit failing URBs, which can cause CPU stalls. This issue
is fixed in mcba_usb by catching the codes seen on device disconnect
(-EPIPE and -EPROTO).
This driver also resubmits in the case of -EPIPE and -EPROTO, so fix it
in the same way.
Signed-off-by: Martin Kelly <mkelly@xevo.com>
Cc: linux-stable <stable@vger.kernel.org>
Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
drivers/net/can/usb/usb_8dev.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/net/can/usb/usb_8dev.c b/drivers/net/can/usb/usb_8dev.c
index d000cb62d6ae..27861c417c94 100644
--- a/drivers/net/can/usb/usb_8dev.c
+++ b/drivers/net/can/usb/usb_8dev.c
@@ -524,6 +524,8 @@ static void usb_8dev_read_bulk_callback(struct urb *urb)
break;
case -ENOENT:
+ case -EPIPE:
+ case -EPROTO:
case -ESHUTDOWN:
return;
--
2.15.0
^ permalink raw reply related
* [PATCH 4/6] can: kvaser_usb: cancel urb on -EPIPE and -EPROTO
From: Marc Kleine-Budde @ 2017-12-08 15:58 UTC (permalink / raw)
To: netdev
Cc: davem, linux-can, kernel, Martin Kelly, linux-stable,
Marc Kleine-Budde
In-Reply-To: <20171208155817.9364-1-mkl@pengutronix.de>
From: Martin Kelly <mkelly@xevo.com>
In mcba_usb, we have observed that when you unplug the device, the driver will
endlessly resubmit failing URBs, which can cause CPU stalls. This issue
is fixed in mcba_usb by catching the codes seen on device disconnect
(-EPIPE and -EPROTO).
This driver also resubmits in the case of -EPIPE and -EPROTO, so fix it
in the same way.
Signed-off-by: Martin Kelly <mkelly@xevo.com>
Cc: linux-stable <stable@vger.kernel.org>
Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
drivers/net/can/usb/kvaser_usb.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/net/can/usb/kvaser_usb.c b/drivers/net/can/usb/kvaser_usb.c
index f95945915d20..63587b8e6825 100644
--- a/drivers/net/can/usb/kvaser_usb.c
+++ b/drivers/net/can/usb/kvaser_usb.c
@@ -1326,6 +1326,8 @@ static void kvaser_usb_read_bulk_callback(struct urb *urb)
case 0:
break;
case -ENOENT:
+ case -EPIPE:
+ case -EPROTO:
case -ESHUTDOWN:
return;
default:
--
2.15.0
^ permalink raw reply related
* [PATCH 3/6] can: esd_usb2: cancel urb on -EPIPE and -EPROTO
From: Marc Kleine-Budde @ 2017-12-08 15:58 UTC (permalink / raw)
To: netdev
Cc: davem, linux-can, kernel, Martin Kelly, linux-stable,
Marc Kleine-Budde
In-Reply-To: <20171208155817.9364-1-mkl@pengutronix.de>
From: Martin Kelly <mkelly@xevo.com>
In mcba_usb, we have observed that when you unplug the device, the driver will
endlessly resubmit failing URBs, which can cause CPU stalls. This issue
is fixed in mcba_usb by catching the codes seen on device disconnect
(-EPIPE and -EPROTO).
This driver also resubmits in the case of -EPIPE and -EPROTO, so fix it
in the same way.
Signed-off-by: Martin Kelly <mkelly@xevo.com>
Cc: linux-stable <stable@vger.kernel.org>
Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
drivers/net/can/usb/esd_usb2.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/net/can/usb/esd_usb2.c b/drivers/net/can/usb/esd_usb2.c
index 9fdb0f0bfa06..c6dcf93675c0 100644
--- a/drivers/net/can/usb/esd_usb2.c
+++ b/drivers/net/can/usb/esd_usb2.c
@@ -393,6 +393,8 @@ static void esd_usb2_read_bulk_callback(struct urb *urb)
break;
case -ENOENT:
+ case -EPIPE:
+ case -EPROTO:
case -ESHUTDOWN:
return;
--
2.15.0
^ permalink raw reply related
* [PATCH 2/6] can: ems_usb: cancel urb on -EPIPE and -EPROTO
From: Marc Kleine-Budde @ 2017-12-08 15:58 UTC (permalink / raw)
To: netdev
Cc: davem, linux-can, kernel, Martin Kelly, linux-stable,
Marc Kleine-Budde
In-Reply-To: <20171208155817.9364-1-mkl@pengutronix.de>
From: Martin Kelly <mkelly@xevo.com>
In mcba_usb, we have observed that when you unplug the device, the driver will
endlessly resubmit failing URBs, which can cause CPU stalls. This issue
is fixed in mcba_usb by catching the codes seen on device disconnect
(-EPIPE and -EPROTO).
This driver also resubmits in the case of -EPIPE and -EPROTO, so fix it
in the same way.
Signed-off-by: Martin Kelly <mkelly@xevo.com>
Cc: linux-stable <stable@vger.kernel.org>
Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
drivers/net/can/usb/ems_usb.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/net/can/usb/ems_usb.c b/drivers/net/can/usb/ems_usb.c
index b3d02759c226..b00358297424 100644
--- a/drivers/net/can/usb/ems_usb.c
+++ b/drivers/net/can/usb/ems_usb.c
@@ -288,6 +288,8 @@ static void ems_usb_read_interrupt_callback(struct urb *urb)
case -ECONNRESET: /* unlink */
case -ENOENT:
+ case -EPIPE:
+ case -EPROTO:
case -ESHUTDOWN:
return;
--
2.15.0
^ permalink raw reply related
* [PATCH 1/6] can: mcba_usb: cancel urb on -EPROTO
From: Marc Kleine-Budde @ 2017-12-08 15:58 UTC (permalink / raw)
To: netdev
Cc: davem, linux-can, kernel, Martin Kelly, linux-stable,
Marc Kleine-Budde
In-Reply-To: <20171208155817.9364-1-mkl@pengutronix.de>
From: Martin Kelly <mkelly@xevo.com>
When we unplug the device, we can see both -EPIPE and -EPROTO depending
on exact timing and what system we run on. If we continue to resubmit
URBs, they will immediately fail, and they can cause stalls, especially
on slower CPUs.
Fix this by not resubmitting on -EPROTO, as we already do on -EPIPE.
Signed-off-by: Martin Kelly <mkelly@xevo.com>
Cc: linux-stable <stable@vger.kernel.org>
Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
drivers/net/can/usb/mcba_usb.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/net/can/usb/mcba_usb.c b/drivers/net/can/usb/mcba_usb.c
index ef417dcddbf7..8d8c2086424d 100644
--- a/drivers/net/can/usb/mcba_usb.c
+++ b/drivers/net/can/usb/mcba_usb.c
@@ -593,6 +593,7 @@ static void mcba_usb_read_bulk_callback(struct urb *urb)
case -ENOENT:
case -EPIPE:
+ case -EPROTO:
case -ESHUTDOWN:
return;
--
2.15.0
^ permalink raw reply related
* pull-request: can 2017-12-08
From: Marc Kleine-Budde @ 2017-12-08 15:58 UTC (permalink / raw)
To: netdev; +Cc: davem, linux-can, kernel
Hello David,
this is a pull request of 6 patches for net/master.
Martin Kelly provides 5 patches for various USB based CAN drivers, that
properly cancel the URBs on adapter unplug, so that the driver doesn't
end up in an endless loop. Stephane Grosjean provides a patch to restart
the tx queue if zero length packages are transmitted.
regards,
Marc
---
The following changes since commit 195bd525d5f6e338b948d9a6b25bfaae86291353:
Merge git://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf (2017-12-07 16:22:51 -0500)
are available in the Git repository at:
git://git.kernel.org/pub/scm/linux/kernel/git/mkl/linux-can.git tags/linux-can-fixes-for-4.15-20171208
for you to fetch changes up to 91785de6f94b58c3fb6664609e3682f011bd28d2:
can: peak/pcie_fd: fix potential bug in restarting tx queue (2017-12-08 10:51:53 +0100)
----------------------------------------------------------------
linux-can-fixes-for-4.15-20171208
----------------------------------------------------------------
Martin Kelly (5):
can: mcba_usb: cancel urb on -EPROTO
can: ems_usb: cancel urb on -EPIPE and -EPROTO
can: esd_usb2: cancel urb on -EPIPE and -EPROTO
can: kvaser_usb: cancel urb on -EPIPE and -EPROTO
can: usb_8dev: cancel urb on -EPIPE and -EPROTO
Stephane Grosjean (1):
can: peak/pcie_fd: fix potential bug in restarting tx queue
drivers/net/can/peak_canfd/peak_canfd.c | 9 +++------
drivers/net/can/usb/ems_usb.c | 2 ++
drivers/net/can/usb/esd_usb2.c | 2 ++
drivers/net/can/usb/kvaser_usb.c | 2 ++
drivers/net/can/usb/mcba_usb.c | 1 +
drivers/net/can/usb/usb_8dev.c | 2 ++
6 files changed, 12 insertions(+), 6 deletions(-)
^ permalink raw reply
* Re: [PATCH v5 net-next] net/tcp: trace all TCP/IP state transition with tcp_set_state tracepoint
From: Yafang Shao @ 2017-12-08 15:50 UTC (permalink / raw)
To: David Miller
Cc: Marcelo Ricardo Leitner, Song Liu, Alexey Kuznetsov, yoshfuji,
Steven Rostedt, Brendan Gregg, netdev, LKML
In-Reply-To: <20171208.104256.1026704308758536035.davem@davemloft.net>
2017-12-08 23:42 GMT+08:00 David Miller <davem@davemloft.net>:
> From: Yafang Shao <laoar.shao@gmail.com>
> Date: Fri, 8 Dec 2017 11:40:23 +0800
>
>> It will looks like these,
>>
>> if (sk->sk_protocol == IPPROTO_TCP)
>> __tcp_set_state(newsk, TCP_SYN_RECV);
>> else
>> newsk->sk_state = TCP_SYN_RECV;
>>
>>
>> if (sk->sk_protocol == IPPROTO_TCP)
>> __tcp_set_state(sk, TCP_CLOSE);
>> else
>> sk->sk_state = TCP_CLOSE;
>>
>> if (sk->sk_protocol == IPPROTO_TCP)
>> tcp_state_store(sk, state);
>> else
>> sk_state_store(sk, state);
>>
>>
>> Some redundant code.
>>
>> IMO, put these similar code into a wrapper is more nice.
>
> I think this discussion and how ugly this is getting shows that
> tracing the state transitions of a socket is perhaps not best as a TCP
> specific feature.
Do you mean that tcp_set_state tracepoint should be replaced with
sk_set_state tracepoint and move that tracepoint to
trace/events/sock.h ?
Thanks
Yafang
^ permalink raw reply
* [PATCH RFC 4/4] net: phy: marvell: fix paged access races
From: Russell King @ 2017-12-08 15:48 UTC (permalink / raw)
To: Andrew Lunn, Florian Fainelli; +Cc: netdev
In-Reply-To: <20171208154756.GF10595@n2100.armlinux.org.uk>
For paged accesses to be truely safe, we need to hold the bus lock to
prevent anyone else gaining access to the registers while we modify
them.
The phydev->lock mutex does not do this: userspace via the MII ioctl
can still sneak in and read or write any register while we are on a
different page, and the suspend/resume methods can be called by a
thread different to the thread polling the phy status.
Races have been observed with mvneta on SolidRun Clearfog with phylink,
particularly between the phylib worker reading the PHYs status, and
the thread resuming mvneta, calling phy_start() which then calls
through to m88e1121_config_aneg_rgmii_delays(), which tries to
read-modify-write the MSCR register:
CPU0 CPU1
marvell_read_status_page()
marvell_set_page(phydev, MII_MARVELL_FIBER_PAGE)
...
m88e1121_config_aneg_rgmii_delays()
set_page(MII_MARVELL_MSCR_PAGE)
phy_read(phydev, MII_88E1121_PHY_MSCR_REG)
marvell_set_page(phydev, MII_MARVELL_COPPER_PAGE);
...
phy_write(phydev, MII_88E1121_PHY_MSCR_REG)
The result of this is we end up writing the copper page register 21,
which causes the copper PHY to be disabled, and the link partner sees
the link immediately go down.
Solve this by taking the bus lock instead of the PHY lock, thereby
preventing other accesses to the PHY while we are accessing other PHY
pages.
Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
---
drivers/net/phy/marvell.c | 365 +++++++++++++++++++++-------------------------
1 file changed, 168 insertions(+), 197 deletions(-)
diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c
index 4d02b27df044..87b76db456cf 100644
--- a/drivers/net/phy/marvell.c
+++ b/drivers/net/phy/marvell.c
@@ -177,27 +177,96 @@ struct marvell_priv {
struct device *hwmon_dev;
};
-static int marvell_get_page(struct phy_device *phydev)
+static int marvell_read_page(struct phy_device *phydev)
{
- return phy_read(phydev, MII_MARVELL_PHY_PAGE);
+ return __phy_read(phydev, MII_MARVELL_PHY_PAGE);
}
-static int marvell_set_page(struct phy_device *phydev, int page)
+static int marvell_write_page(struct phy_device *phydev, int page)
{
- return phy_write(phydev, MII_MARVELL_PHY_PAGE, page);
+ return __phy_write(phydev, MII_MARVELL_PHY_PAGE, page);
}
-static int marvell_get_set_page(struct phy_device *phydev, int page)
+static int marvell_save_page(struct phy_device *phydev)
{
- int oldpage = marvell_get_page(phydev);
+ mutex_lock(&phydev->mdio.bus->mdio_lock);
+ return marvell_read_page(phydev);
+}
- if (oldpage < 0)
- return oldpage;
+static int marvell_select_page(struct phy_device *phydev, int page)
+{
+ int ret, oldpage;
- if (page != oldpage)
- return marvell_set_page(phydev, page);
+ oldpage = ret = marvell_save_page(phydev);
+ if (ret < 0)
+ return ret;
- return 0;
+ if (oldpage != page)
+ ret = marvell_write_page(phydev, page);
+
+ return ret < 0 ? ret : oldpage;
+}
+
+static int marvell_restore_page(struct phy_device *phydev, int oldpage, int ret)
+{
+ int r;
+
+ if (oldpage >= 0) {
+ r = marvell_write_page(phydev, oldpage);
+ if (ret == 0 && r < 0)
+ ret = r;
+ } else {
+ ret = oldpage;
+ }
+ mutex_unlock(&phydev->mdio.bus->mdio_lock);
+
+ return ret;
+}
+
+static int marvell_read_paged(struct phy_device *phydev, int page, int regnum)
+{
+ int ret = 0, oldpage;
+
+ oldpage = marvell_select_page(phydev, page);
+ if (oldpage >= 0)
+ ret = __phy_read(phydev, regnum);
+
+ return marvell_restore_page(phydev, oldpage, ret);
+}
+
+static int marvell_write_paged(struct phy_device *phydev, int page, int regnum,
+ int val)
+{
+ int ret = 0, oldpage;
+
+ oldpage = marvell_select_page(phydev, page);
+ if (oldpage >= 0)
+ ret = __phy_write(phydev, regnum, val);
+
+ return marvell_restore_page(phydev, oldpage, ret);
+}
+
+static int marvell_modify_paged(struct phy_device *phydev, int page, int regnum,
+ int mask, int set)
+{
+ int ret = 0, res, oldpage;
+
+ oldpage = marvell_select_page(phydev, page);
+ if (oldpage >= 0) {
+ ret = __phy_read(phydev, regnum);
+ if (ret >= 0) {
+ res = __phy_write(phydev, regnum, (ret & mask) | set);
+ if (res < 0)
+ ret = res;
+ }
+ }
+
+ return marvell_restore_page(phydev, oldpage, ret);
+}
+
+static int marvell_set_page(struct phy_device *phydev, int page)
+{
+ return phy_write(phydev, MII_MARVELL_PHY_PAGE, page);
}
static int marvell_ack_interrupt(struct phy_device *phydev)
@@ -399,7 +468,7 @@ static int m88e1111_config_aneg(struct phy_device *phydev)
static int marvell_of_reg_init(struct phy_device *phydev)
{
const __be32 *paddr;
- int len, i, saved_page, current_page, ret;
+ int len, i, saved_page, current_page, ret = 0;
if (!phydev->mdio.dev.of_node)
return 0;
@@ -409,12 +478,11 @@ static int marvell_of_reg_init(struct phy_device *phydev)
if (!paddr || len < (4 * sizeof(*paddr)))
return 0;
- saved_page = marvell_get_page(phydev);
+ saved_page = marvell_save_page(phydev);
if (saved_page < 0)
- return saved_page;
+ goto err;
current_page = saved_page;
- ret = 0;
len /= sizeof(*paddr);
for (i = 0; i < len - 3; i += 4) {
u16 page = be32_to_cpup(paddr + i);
@@ -425,14 +493,14 @@ static int marvell_of_reg_init(struct phy_device *phydev)
if (page != current_page) {
current_page = page;
- ret = marvell_set_page(phydev, page);
+ ret = marvell_write_page(phydev, page);
if (ret < 0)
goto err;
}
val = 0;
if (mask) {
- val = phy_read(phydev, reg);
+ val = __phy_read(phydev, reg);
if (val < 0) {
ret = val;
goto err;
@@ -441,17 +509,12 @@ static int marvell_of_reg_init(struct phy_device *phydev)
}
val |= val_bits;
- ret = phy_write(phydev, reg, val);
+ ret = __phy_write(phydev, reg, val);
if (ret < 0)
goto err;
}
err:
- if (current_page != saved_page) {
- i = marvell_set_page(phydev, saved_page);
- if (ret == 0)
- ret = i;
- }
- return ret;
+ return marvell_restore_page(phydev, saved_page, ret);
}
#else
static int marvell_of_reg_init(struct phy_device *phydev)
@@ -462,34 +525,21 @@ static int marvell_of_reg_init(struct phy_device *phydev)
static int m88e1121_config_aneg_rgmii_delays(struct phy_device *phydev)
{
- int err, oldpage, mscr;
-
- oldpage = marvell_get_set_page(phydev, MII_MARVELL_MSCR_PAGE);
- if (oldpage < 0)
- return oldpage;
-
- mscr = phy_read(phydev, MII_88E1121_PHY_MSCR_REG);
- if (mscr < 0) {
- err = mscr;
- goto out;
- }
-
- mscr &= MII_88E1121_PHY_MSCR_DELAY_MASK;
+ int mscr;
if (phydev->interface == PHY_INTERFACE_MODE_RGMII_ID)
- mscr |= (MII_88E1121_PHY_MSCR_RX_DELAY |
- MII_88E1121_PHY_MSCR_TX_DELAY);
+ mscr = MII_88E1121_PHY_MSCR_RX_DELAY |
+ MII_88E1121_PHY_MSCR_TX_DELAY;
else if (phydev->interface == PHY_INTERFACE_MODE_RGMII_RXID)
- mscr |= MII_88E1121_PHY_MSCR_RX_DELAY;
+ mscr = MII_88E1121_PHY_MSCR_RX_DELAY;
else if (phydev->interface == PHY_INTERFACE_MODE_RGMII_TXID)
- mscr |= MII_88E1121_PHY_MSCR_TX_DELAY;
-
- err = phy_write(phydev, MII_88E1121_PHY_MSCR_REG, mscr);
-
-out:
- marvell_set_page(phydev, oldpage);
+ mscr = MII_88E1121_PHY_MSCR_TX_DELAY;
+ else
+ mscr = 0;
- return err;
+ return marvell_modify_paged(phydev, MII_MARVELL_MSCR_PAGE,
+ MII_88E1121_PHY_MSCR_REG,
+ MII_88E1121_PHY_MSCR_DELAY_MASK, mscr);
}
static int m88e1121_config_aneg(struct phy_device *phydev)
@@ -515,20 +565,11 @@ static int m88e1121_config_aneg(struct phy_device *phydev)
static int m88e1318_config_aneg(struct phy_device *phydev)
{
- int err, oldpage, mscr;
-
- oldpage = marvell_get_set_page(phydev, MII_MARVELL_MSCR_PAGE);
- if (oldpage < 0)
- return oldpage;
-
- mscr = phy_read(phydev, MII_88E1318S_PHY_MSCR1_REG);
- mscr |= MII_88E1318S_PHY_MSCR1_PAD_ODD;
-
- err = phy_write(phydev, MII_88E1318S_PHY_MSCR1_REG, mscr);
- if (err < 0)
- return err;
+ int err;
- err = marvell_set_page(phydev, oldpage);
+ err = marvell_modify_paged(phydev, MII_MARVELL_MSCR_PAGE,
+ MII_88E1318S_PHY_MSCR1_REG,
+ 0, MII_88E1318S_PHY_MSCR1_PAD_ODD);
if (err < 0)
return err;
@@ -850,20 +891,15 @@ static int m88e1111_config_init(struct phy_device *phydev)
static int m88e1121_config_init(struct phy_device *phydev)
{
- int err, oldpage;
-
- oldpage = marvell_get_set_page(phydev, MII_MARVELL_LED_PAGE);
- if (oldpage < 0)
- return oldpage;
+ int err;
/* Default PHY LED config: LED[0] .. Link, LED[1] .. Activity */
- err = phy_write(phydev, MII_88E1121_PHY_LED_CTRL,
- MII_88E1121_PHY_LED_DEF);
+ err = marvell_write_paged(phydev, MII_MARVELL_LED_PAGE,
+ MII_88E1121_PHY_LED_CTRL,
+ MII_88E1121_PHY_LED_DEF);
if (err < 0)
return err;
- marvell_set_page(phydev, oldpage);
-
/* Set marvell,reg-init configuration from device tree */
return marvell_config_init(phydev);
}
@@ -1382,100 +1418,102 @@ static int m88e1121_did_interrupt(struct phy_device *phydev)
static void m88e1318_get_wol(struct phy_device *phydev,
struct ethtool_wolinfo *wol)
{
+ int oldpage, ret = 0;
+
wol->supported = WAKE_MAGIC;
wol->wolopts = 0;
- if (marvell_set_page(phydev, MII_MARVELL_WOL_PAGE) < 0)
- return;
+ oldpage = marvell_select_page(phydev, MII_MARVELL_WOL_PAGE);
+ if (oldpage < 0)
+ goto error;
- if (phy_read(phydev, MII_88E1318S_PHY_WOL_CTRL) &
- MII_88E1318S_PHY_WOL_CTRL_MAGIC_PACKET_MATCH_ENABLE)
+ ret = __phy_read(phydev, MII_88E1318S_PHY_WOL_CTRL);
+ if (ret & MII_88E1318S_PHY_WOL_CTRL_MAGIC_PACKET_MATCH_ENABLE)
wol->wolopts |= WAKE_MAGIC;
- if (marvell_set_page(phydev, MII_MARVELL_COPPER_PAGE) < 0)
- return;
+error:
+ marvell_restore_page(phydev, oldpage, ret);
}
static int m88e1318_set_wol(struct phy_device *phydev,
struct ethtool_wolinfo *wol)
{
- int err, oldpage, temp;
+ int err = 0, oldpage, temp;
- oldpage = marvell_get_page(phydev);
+ oldpage = marvell_save_page(phydev);
+ if (oldpage < 0)
+ goto error;
if (wol->wolopts & WAKE_MAGIC) {
/* Explicitly switch to page 0x00, just to be sure */
- err = marvell_set_page(phydev, MII_MARVELL_COPPER_PAGE);
+ err = marvell_write_page(phydev, MII_MARVELL_COPPER_PAGE);
if (err < 0)
- return err;
+ goto error;
/* Enable the WOL interrupt */
temp = phy_read(phydev, MII_88E1318S_PHY_CSIER);
temp |= MII_88E1318S_PHY_CSIER_WOL_EIE;
- err = phy_write(phydev, MII_88E1318S_PHY_CSIER, temp);
+ err = __phy_write(phydev, MII_88E1318S_PHY_CSIER, temp);
if (err < 0)
- return err;
+ goto error;
- err = marvell_set_page(phydev, MII_MARVELL_LED_PAGE);
+ err = marvell_write_page(phydev, MII_MARVELL_LED_PAGE);
if (err < 0)
- return err;
+ goto error;
/* Setup LED[2] as interrupt pin (active low) */
temp = phy_read(phydev, MII_88E1318S_PHY_LED_TCR);
temp &= ~MII_88E1318S_PHY_LED_TCR_FORCE_INT;
temp |= MII_88E1318S_PHY_LED_TCR_INTn_ENABLE;
temp |= MII_88E1318S_PHY_LED_TCR_INT_ACTIVE_LOW;
- err = phy_write(phydev, MII_88E1318S_PHY_LED_TCR, temp);
+ err = __phy_write(phydev, MII_88E1318S_PHY_LED_TCR, temp);
if (err < 0)
- return err;
+ goto error;
- err = marvell_set_page(phydev, MII_MARVELL_WOL_PAGE);
+ err = marvell_write_page(phydev, MII_MARVELL_WOL_PAGE);
if (err < 0)
- return err;
+ goto error;
/* Store the device address for the magic packet */
- err = phy_write(phydev, MII_88E1318S_PHY_MAGIC_PACKET_WORD2,
+ err = __phy_write(phydev, MII_88E1318S_PHY_MAGIC_PACKET_WORD2,
((phydev->attached_dev->dev_addr[5] << 8) |
phydev->attached_dev->dev_addr[4]));
if (err < 0)
- return err;
- err = phy_write(phydev, MII_88E1318S_PHY_MAGIC_PACKET_WORD1,
+ goto error;
+ err = __phy_write(phydev, MII_88E1318S_PHY_MAGIC_PACKET_WORD1,
((phydev->attached_dev->dev_addr[3] << 8) |
phydev->attached_dev->dev_addr[2]));
if (err < 0)
- return err;
- err = phy_write(phydev, MII_88E1318S_PHY_MAGIC_PACKET_WORD0,
+ goto error;
+ err = __phy_write(phydev, MII_88E1318S_PHY_MAGIC_PACKET_WORD0,
((phydev->attached_dev->dev_addr[1] << 8) |
phydev->attached_dev->dev_addr[0]));
if (err < 0)
- return err;
+ goto error;
/* Clear WOL status and enable magic packet matching */
- temp = phy_read(phydev, MII_88E1318S_PHY_WOL_CTRL);
+ temp = __phy_read(phydev, MII_88E1318S_PHY_WOL_CTRL);
temp |= MII_88E1318S_PHY_WOL_CTRL_CLEAR_WOL_STATUS;
temp |= MII_88E1318S_PHY_WOL_CTRL_MAGIC_PACKET_MATCH_ENABLE;
- err = phy_write(phydev, MII_88E1318S_PHY_WOL_CTRL, temp);
+ err = __phy_write(phydev, MII_88E1318S_PHY_WOL_CTRL, temp);
if (err < 0)
- return err;
+ goto error;
} else {
- err = marvell_set_page(phydev, MII_MARVELL_WOL_PAGE);
+ err = marvell_write_page(phydev, MII_MARVELL_WOL_PAGE);
if (err < 0)
- return err;
+ goto error;
/* Clear WOL status and disable magic packet matching */
- temp = phy_read(phydev, MII_88E1318S_PHY_WOL_CTRL);
+ temp = __phy_read(phydev, MII_88E1318S_PHY_WOL_CTRL);
temp |= MII_88E1318S_PHY_WOL_CTRL_CLEAR_WOL_STATUS;
temp &= ~MII_88E1318S_PHY_WOL_CTRL_MAGIC_PACKET_MATCH_ENABLE;
- err = phy_write(phydev, MII_88E1318S_PHY_WOL_CTRL, temp);
+ err = __phy_write(phydev, MII_88E1318S_PHY_WOL_CTRL, temp);
if (err < 0)
- return err;
+ goto error;
}
- err = marvell_set_page(phydev, oldpage);
- if (err < 0)
- return err;
-
- return 0;
+error:
+ return marvell_restore_page(phydev, oldpage, err);
}
static int marvell_get_sset_count(struct phy_device *phydev)
@@ -1503,14 +1541,10 @@ static u64 marvell_get_stat(struct phy_device *phydev, int i)
{
struct marvell_hw_stat stat = marvell_hw_stats[i];
struct marvell_priv *priv = phydev->priv;
- int oldpage, val;
+ int val;
u64 ret;
- oldpage = marvell_get_set_page(phydev, stat.page);
- if (oldpage < 0)
- return UINT64_MAX;
-
- val = phy_read(phydev, stat.reg);
+ val = marvell_read_paged(phydev, stat.page, stat.reg);
if (val < 0) {
ret = UINT64_MAX;
} else {
@@ -1519,8 +1553,6 @@ static u64 marvell_get_stat(struct phy_device *phydev, int i)
ret = priv->stats[i];
}
- marvell_set_page(phydev, oldpage);
-
return ret;
}
@@ -1537,18 +1569,14 @@ static void marvell_get_stats(struct phy_device *phydev,
static int m88e1121_get_temp(struct phy_device *phydev, long *temp)
{
int oldpage;
- int ret;
+ int ret = 0;
int val;
*temp = 0;
- mutex_lock(&phydev->lock);
-
- oldpage = marvell_get_set_page(phydev, MII_MARVELL_MISC_TEST_PAGE);
- if (oldpage < 0) {
- mutex_unlock(&phydev->lock);
- return oldpage;
- }
+ oldpage = marvell_select_page(phydev, MII_MARVELL_MISC_TEST_PAGE);
+ if (oldpage < 0)
+ goto error;
/* Enable temperature sensor */
ret = phy_read(phydev, MII_88E1121_MISC_TEST);
@@ -1578,10 +1606,7 @@ static int m88e1121_get_temp(struct phy_device *phydev, long *temp)
*temp = ((val & MII_88E1121_MISC_TEST_TEMP_MASK) - 5) * 5000;
error:
- marvell_set_page(phydev, oldpage);
- mutex_unlock(&phydev->lock);
-
- return ret;
+ return marvell_restore_page(phydev, oldpage, ret);
}
static int m88e1121_hwmon_read(struct device *dev,
@@ -1655,118 +1680,64 @@ static const struct hwmon_chip_info m88e1121_hwmon_chip_info = {
static int m88e1510_get_temp(struct phy_device *phydev, long *temp)
{
- int oldpage;
int ret;
*temp = 0;
- mutex_lock(&phydev->lock);
-
- oldpage = marvell_get_set_page(phydev, MII_MARVELL_MISC_TEST_PAGE);
- if (oldpage < 0) {
- mutex_unlock(&phydev->lock);
- return oldpage;
- }
-
- ret = phy_read(phydev, MII_88E1510_TEMP_SENSOR);
+ ret = marvell_read_paged(phydev, MII_MARVELL_MISC_TEST_PAGE,
+ MII_88E1510_TEMP_SENSOR);
if (ret < 0)
- goto error;
+ return ret;
*temp = ((ret & MII_88E1510_TEMP_SENSOR_MASK) - 25) * 1000;
-error:
- marvell_set_page(phydev, oldpage);
- mutex_unlock(&phydev->lock);
-
- return ret;
+ return 0;
}
static int m88e1510_get_temp_critical(struct phy_device *phydev, long *temp)
{
- int oldpage;
int ret;
*temp = 0;
- mutex_lock(&phydev->lock);
-
- oldpage = marvell_get_set_page(phydev, MII_MARVELL_MISC_TEST_PAGE);
- if (oldpage < 0) {
- mutex_unlock(&phydev->lock);
- return oldpage;
- }
-
- ret = phy_read(phydev, MII_88E1121_MISC_TEST);
+ ret = marvell_read_paged(phydev, MII_MARVELL_MISC_TEST_PAGE,
+ MII_88E1121_MISC_TEST);
if (ret < 0)
- goto error;
+ return ret;
*temp = (((ret & MII_88E1510_MISC_TEST_TEMP_THRESHOLD_MASK) >>
MII_88E1510_MISC_TEST_TEMP_THRESHOLD_SHIFT) * 5) - 25;
/* convert to mC */
*temp *= 1000;
-error:
- marvell_set_page(phydev, oldpage);
- mutex_unlock(&phydev->lock);
-
- return ret;
+ return 0;
}
static int m88e1510_set_temp_critical(struct phy_device *phydev, long temp)
{
- int oldpage;
- int ret;
-
- mutex_lock(&phydev->lock);
-
- oldpage = marvell_get_set_page(phydev, MII_MARVELL_MISC_TEST_PAGE);
- if (oldpage < 0) {
- mutex_unlock(&phydev->lock);
- return oldpage;
- }
-
- ret = phy_read(phydev, MII_88E1121_MISC_TEST);
- if (ret < 0)
- goto error;
-
temp = temp / 1000;
temp = clamp_val(DIV_ROUND_CLOSEST(temp, 5) + 5, 0, 0x1f);
- ret = phy_write(phydev, MII_88E1121_MISC_TEST,
- (ret & ~MII_88E1510_MISC_TEST_TEMP_THRESHOLD_MASK) |
- (temp << MII_88E1510_MISC_TEST_TEMP_THRESHOLD_SHIFT));
-error:
- marvell_set_page(phydev, oldpage);
- mutex_unlock(&phydev->lock);
-
- return ret;
+ return marvell_modify_paged(phydev, MII_MARVELL_MISC_TEST_PAGE,
+ MII_88E1121_MISC_TEST,
+ ~MII_88E1510_MISC_TEST_TEMP_THRESHOLD_MASK,
+ temp << MII_88E1510_MISC_TEST_TEMP_THRESHOLD_SHIFT);
}
static int m88e1510_get_temp_alarm(struct phy_device *phydev, long *alarm)
{
- int oldpage;
int ret;
*alarm = false;
- mutex_lock(&phydev->lock);
-
- oldpage = marvell_get_set_page(phydev, MII_MARVELL_MISC_TEST_PAGE);
- if (oldpage < 0) {
- mutex_unlock(&phydev->lock);
- return oldpage;
- }
-
- ret = phy_read(phydev, MII_88E1121_MISC_TEST);
+ ret = marvell_read_paged(phydev, MII_MARVELL_MISC_TEST_PAGE,
+ MII_88E1121_MISC_TEST);
if (ret < 0)
- goto error;
- *alarm = !!(ret & MII_88E1510_MISC_TEST_TEMP_IRQ);
+ return ret;
-error:
- marvell_set_page(phydev, oldpage);
- mutex_unlock(&phydev->lock);
+ *alarm = !!(ret & MII_88E1510_MISC_TEST_TEMP_IRQ);
- return ret;
+ return 0;
}
static int m88e1510_hwmon_read(struct device *dev,
--
2.7.4
^ permalink raw reply related
* [PATCH RFC 3/4] net: phy: add unlocked accessors
From: Russell King @ 2017-12-08 15:48 UTC (permalink / raw)
To: Andrew Lunn, Florian Fainelli; +Cc: netdev
In-Reply-To: <20171208154756.GF10595@n2100.armlinux.org.uk>
Add unlocked versions of the bus accessors, which allows access to the
bus with all the tracing. These accessors validate that the bus mutex
is held, which is a basic requirement for all mii bus accesses.
Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
---
include/linux/phy.h | 26 ++++++++++++++++++++++++++
1 file changed, 26 insertions(+)
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 71d777fe6c3d..964803bd7324 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -716,6 +716,18 @@ static inline int phy_read(struct phy_device *phydev, u32 regnum)
}
/**
+ * __phy_read - convenience function for reading a given PHY register
+ * @phydev: the phy_device struct
+ * @regnum: register number to read
+ *
+ * The caller must have taken the MDIO bus lock.
+ */
+static inline int __phy_read(struct phy_device *phydev, u32 regnum)
+{
+ return __mdiobus_read(phydev->mdio.bus, phydev->mdio.addr, regnum);
+}
+
+/**
* phy_write - Convenience function for writing a given PHY register
* @phydev: the phy_device struct
* @regnum: register number to write
@@ -731,6 +743,20 @@ static inline int phy_write(struct phy_device *phydev, u32 regnum, u16 val)
}
/**
+ * __phy_write - Convenience function for writing a given PHY register
+ * @phydev: the phy_device struct
+ * @regnum: register number to write
+ * @val: value to write to @regnum
+ *
+ * The caller must have taken the MDIO bus lock.
+ */
+static inline int __phy_write(struct phy_device *phydev, u32 regnum, u16 val)
+{
+ return __mdiobus_write(phydev->mdio.bus, phydev->mdio.addr, regnum,
+ val);
+}
+
+/**
* phy_interrupt_is_valid - Convenience function for testing a given PHY irq
* @phydev: the phy_device struct
*
--
2.7.4
^ permalink raw reply related
* [PATCH RFC 2/4] net: phy: use unlocked accessors for indirect MMD accesses
From: Russell King @ 2017-12-08 15:48 UTC (permalink / raw)
To: Andrew Lunn, Florian Fainelli; +Cc: netdev
In-Reply-To: <20171208154756.GF10595@n2100.armlinux.org.uk>
Use unlocked accessors for indirect MMD accesses to clause 22 PHYs.
This permits tracing of these accesses.
Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
---
drivers/net/phy/phy-core.c | 11 ++++++-----
1 file changed, 6 insertions(+), 5 deletions(-)
diff --git a/drivers/net/phy/phy-core.c b/drivers/net/phy/phy-core.c
index 21f75ae244b3..83d32644cb4d 100644
--- a/drivers/net/phy/phy-core.c
+++ b/drivers/net/phy/phy-core.c
@@ -193,13 +193,14 @@ static void mmd_phy_indirect(struct mii_bus *bus, int phy_addr, int devad,
u16 regnum)
{
/* Write the desired MMD Devad */
- bus->write(bus, phy_addr, MII_MMD_CTRL, devad);
+ __mdiobus_write(bus, phy_addr, MII_MMD_CTRL, devad);
/* Write the desired MMD register address */
- bus->write(bus, phy_addr, MII_MMD_DATA, regnum);
+ __mdiobus_write(bus, phy_addr, MII_MMD_DATA, regnum);
/* Select the Function : DATA with no post increment */
- bus->write(bus, phy_addr, MII_MMD_CTRL, devad | MII_MMD_CTRL_NOINCR);
+ __mdiobus_write(bus, phy_addr, MII_MMD_CTRL,
+ devad | MII_MMD_CTRL_NOINCR);
}
/**
@@ -232,7 +233,7 @@ int phy_read_mmd(struct phy_device *phydev, int devad, u32 regnum)
mmd_phy_indirect(bus, phy_addr, devad, regnum);
/* Read the content of the MMD's selected register */
- val = bus->read(bus, phy_addr, MII_MMD_DATA);
+ val = __mdiobus_read(bus, phy_addr, MII_MMD_DATA);
mutex_unlock(&bus->mdio_lock);
}
return val;
@@ -271,7 +272,7 @@ int phy_write_mmd(struct phy_device *phydev, int devad, u32 regnum, u16 val)
mmd_phy_indirect(bus, phy_addr, devad, regnum);
/* Write the data into MMD's selected register */
- bus->write(bus, phy_addr, MII_MMD_DATA, val);
+ __mdiobus_write(bus, phy_addr, MII_MMD_DATA, val);
mutex_unlock(&bus->mdio_lock);
ret = 0;
--
2.7.4
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox