* Re: [net-next.git 3/7] stmmac: review private structure fields
From: Ben Hutchings @ 2013-04-03 16:09 UTC (permalink / raw)
To: Giuseppe CAVALLARO; +Cc: Eric Dumazet, netdev
In-Reply-To: <515BDB30.4080603@st.com>
On Wed, 2013-04-03 at 09:33 +0200, Giuseppe CAVALLARO wrote:
> On 4/3/2013 9:21 AM, Eric Dumazet wrote:
> > On Wed, 2013-04-03 at 07:41 +0200, Giuseppe CAVALLARO wrote:
> >> recently many new supports have been added in the stmmac driver w/o taking care
> >> about where each new field had to be placed inside the private structure for
> >> guaranteeing the best cache usage.
> >> This is what I wanted in the beginning, so this patch reorganizes all the fields
> >> in order to keep adjacent fields for cache effect.
> >> I have also tried to optimize them by using pahole.
> >>
> >> Signed-off-by: Giuseppe Cavallaro <peppe.cavallaro@st.com>
> >> ---
> >> drivers/net/ethernet/stmicro/stmmac/stmmac.h | 70 +++++++++++++-------------
> >> 1 files changed, 35 insertions(+), 35 deletions(-)
> >>
> >> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac.h b/drivers/net/ethernet/stmicro/stmmac/stmmac.h
> >> index 75f997b..8aa28c5 100644
> >> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac.h
> >> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac.h
> >> @@ -35,36 +35,45 @@
> >>
> >> struct stmmac_priv {
> >> /* Frequently used values are kept adjacent for cache effect */
> >> - struct dma_desc *dma_tx ____cacheline_aligned; /* Basic TX desc */
> >> - struct dma_extended_desc *dma_etx; /* Extended TX descriptor */
> >> - dma_addr_t dma_tx_phy;
> >> - struct sk_buff **tx_skbuff;
> >> - dma_addr_t *tx_skbuff_dma;
> >> + struct dma_extended_desc *dma_etx;
> >> + struct dma_desc *dma_tx ____cacheline_aligned_in_smp;
> >> + struct sk_buff **tx_skbuff ____cacheline_aligned_in_smp;
> >
> > dma_tx & tx_skbuff are readonly, why put them in separate cache lines ?
>
> I put tx_skbuff in a separate cache line because, when we use extended
> descriptors, the driver works with dma_etx instead of dma_tx.
> So my idea was to have both dma_etx, dma_tx and tx_skbuff aligned in
> any case.
[...]
It's generally not that important to put fields at the beginning of a
cache line. (If you've measured with typical systems using stmmac and
found an advantage, then I accept that you have a good reason to do
this. But that advantage is unlikely to be specific to SMP systems.)
I would use ____cacheline_aligned_in_smp to separate fields that are
likely to be changed on different CPUs, so that the cache line doesn't
bounce between their caches. Fields that are rarely modified (such as
these pointers), or are used together on the same CPU should be packed
together into a small number of cache lines.
Ben.
--
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.
^ permalink raw reply
* Re: [net-next.git 1/7] stmmac: review napi gro support
From: Ben Hutchings @ 2013-04-03 16:01 UTC (permalink / raw)
To: Giuseppe CAVALLARO; +Cc: netdev
In-Reply-To: <1364967689-11155-1-git-send-email-peppe.cavallaro@st.com>
On Wed, 2013-04-03 at 07:41 +0200, Giuseppe CAVALLARO wrote:
> This patch is to:
> o use napi_gro_flush() before calling __napi_complete()
napi_complete() already does that, and some other important things. Why
do you think it makes sense to call __napi_complete() directly?
Ben.
> o turn on NETIF_F_GRO by default
>
> Signed-off-by: Giuseppe Cavallaro <peppe.cavallaro@st.com>
> ---
> drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 5 +++--
> 1 files changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index 6b26d31..8b69e3b 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> @@ -2046,7 +2046,8 @@ static int stmmac_poll(struct napi_struct *napi, int budget)
>
> work_done = stmmac_rx(priv, budget);
> if (work_done < budget) {
> - napi_complete(napi);
> + napi_gro_flush(napi, false);
> + __napi_complete(napi);
> stmmac_enable_dma_irq(priv);
> }
> return work_done;
> @@ -2586,7 +2587,7 @@ struct stmmac_priv *stmmac_dvr_probe(struct device *device,
>
> ndev->hw_features = NETIF_F_SG | NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM |
> NETIF_F_RXCSUM;
> - ndev->features |= ndev->hw_features | NETIF_F_HIGHDMA;
> + ndev->features |= ndev->hw_features | NETIF_F_HIGHDMA | NETIF_F_GRO;
> ndev->watchdog_timeo = msecs_to_jiffies(watchdog);
> #ifdef STMMAC_VLAN_TAG_USED
> /* Both mac100 and gmac support receive VLAN tag detection */
--
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.
^ permalink raw reply
* Re: [PATCH] MPLS: Add limited GSO support
From: Ben Hutchings @ 2013-04-03 15:59 UTC (permalink / raw)
To: Simon Horman; +Cc: dev, netdev, Jesse Gross, Pravin B Shelar
In-Reply-To: <1364966697-20131-1-git-send-email-horms@verge.net.au>
I don't know anything about MPLS so this is a pretty superficial review.
On Wed, 2013-04-03 at 14:24 +0900, Simon Horman wrote:
[...]
> --- a/include/linux/netdev_features.h
> +++ b/include/linux/netdev_features.h
> @@ -43,6 +43,7 @@ enum {
> NETIF_F_FSO_BIT, /* ... FCoE segmentation */
> NETIF_F_GSO_GRE_BIT, /* ... GRE with TSO */
> NETIF_F_GSO_UDP_TUNNEL_BIT, /* ... UDP TUNNEL with TSO */
> + NETIF_F_GSO_MPLS_BIT, /* ... MPLS segmentation */
> /**/NETIF_F_GSO_LAST = /* last bit, see GSO_MASK */
> NETIF_F_GSO_UDP_TUNNEL_BIT,
You need to change NETIF_F_GSO_LAST as well.
[...]
> --- a/include/linux/skbuff.h
> +++ b/include/linux/skbuff.h
[...]
> @@ -2789,12 +2791,17 @@ static inline struct sec_path *skb_sec_path(struct sk_buff *skb)
> }
> #endif
>
> -/* Keeps track of mac header offset relative to skb->head.
> - * It is useful for TSO of Tunneling protocol. e.g. GRE.
> - * For non-tunnel skb it points to skb_mac_header() and for
> - * tunnel skb it points to outer mac header. */
> struct skb_gso_cb {
> + /* Keeps track of mac header offset relative to skb->head.
> + * It is useful for TSO of Tunneling protocol. e.g. GRE.
> + * For non-tunnel skb it points to skb_mac_header() and for
> + * tunnel skb it points to outer mac header. */
> int mac_offset;
> +
> + /* Keeps track of the ethernet type of an encapsualted
[...]
Typo: 'encapsualted' should be 'encapsulated'.
Ben.
--
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.
^ permalink raw reply
* Re: [PATCH v5 2/6] usb: phy: omap-usb2: use the new generic PHY framework
From: Felipe Balbi @ 2013-04-03 15:48 UTC (permalink / raw)
To: Arnd Bergmann
Cc: balbi, Kishon Vijay Abraham I, gregkh, akpm, swarren,
sylvester.nawrocki, rob, netdev, davem, cesarb, linux-usb,
linux-omap, linux-kernel, tony, grant.likely, rob.herring,
b-cousson, linux, eballetbo, javier, mchehab, santosh.shilimkar,
broonie, swarren, linux-doc, devicetree-discuss, linux-arm-kernel
In-Reply-To: <201304031455.47864.arnd@arndb.de>
[-- Attachment #1: Type: text/plain, Size: 592 bytes --]
Hi,
On Wed, Apr 03, 2013 at 02:55:47PM +0000, Arnd Bergmann wrote:
> On Wednesday 03 April 2013, Felipe Balbi wrote:
> > const ? Maybe provide a:
> >
> > #define DEFINE_PHY_OPS(name) \
> > const struct phy_ops #name_phy_ops = {
> >
> > macro ? This will force people to add the const keyword :-)
>
> Forcing people to use const structures is good, but I think it would be
> better without the macro, just by marking the argument in
> devm_phy_create() as const.
that won't force the definition of the struct to be const, however. But
I get your point.
--
balbi
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* Re: [PATCH v5 1/6] drivers: phy: add generic PHY framework
From: Felipe Balbi @ 2013-04-03 15:47 UTC (permalink / raw)
To: Kishon Vijay Abraham I
Cc: broonie-yzvPICuk2AATkU/dhu1WVueM+bqZidxxQQ4Iyu8u01E,
mchehab-H+wXaHxf7aLQT0dZR+AlfA, linux-doc-u79uwXL29TY76Z2rM5mHXA,
linux-lFZ/pmaqli7XmaaqVzeoHQ, javier-0uQlZySMnqxg9hUCZPvPmw,
cesarb-PWySMVKUnqmsTnJN9+BGXg, eballetbo-Re5JQEeQqe8AvxtiuMwx3w,
devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ,
rob.herring-bsGFqQB8/DxBDgjK7y7TUQ,
swarren-DDmLM1+adcrQT0dZR+AlfA,
sylvester.nawrocki-Re5JQEeQqe8AvxtiuMwx3w,
linux-omap-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r,
linux-usb-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA, balbi-l0cyMroinI0,
santosh.shilimkar-l0cyMroinI0, netdev-u79uwXL29TY76Z2rM5mHXA,
akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b,
davem-fT/PcQaiUtIeIZ0/mPfg9Q
In-Reply-To: <515C3D94.1060000-l0cyMroinI0@public.gmane.org>
[-- Attachment #1.1: Type: text/plain, Size: 794 bytes --]
Hi,
On Wed, Apr 03, 2013 at 08:02:52PM +0530, Kishon Vijay Abraham I wrote:
> >>>>+ ret = -EINVAL;
> >>>>+ goto err0;
> >>>>+ }
> >>>>+
> >>>>+ if (!phy_class)
> >>>>+ phy_core_init();
> >>>
> >>>why don't you setup the class on module_init ? Then this would be a
> >>>terrible error condition here :-)
> >>
> >>This is for the case where the PHY driver gets loaded before the PHY
> >>framework. I could have returned EPROBE_DEFER here instead I thought
> >>will have it this way.
> >
> >looks a bit weird IMO. Is it really possible for PHY to load before ?
>
> yeah. it actually happened when I tried with beagle and had all the
> modules as built-in. Because twl4030 has subsys_initcall(), it loads
> before PHY framework.
that's a bug in twl4030.
--
balbi
[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
[-- Attachment #2: Type: text/plain, Size: 192 bytes --]
_______________________________________________
devicetree-discuss mailing list
devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org
https://lists.ozlabs.org/listinfo/devicetree-discuss
^ permalink raw reply
* [PATCH] bonding: remove sysfs before removing devices
From: Veaceslav Falico @ 2013-04-03 15:46 UTC (permalink / raw)
To: netdev; +Cc: vfalico, fubar, andy, nikolay
We have a race condition if we try to rmmod bonding and simultaneously add
a bond master through sysfs. In bonding_exit() we first remove the devices
(through rtnl_link_unregister() ) and only after that we remove the sysfs.
If we manage to add a device through sysfs after that the devices were
removed - we'll end up with that device/sysfs structure and with the module
unloaded.
Fix this by first removing the sysfs and only after that calling
rtnl_link_unregister().
Signed-off-by: Veaceslav Falico <vfalico@redhat.com>
---
drivers/net/bonding/bond_main.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index 11a8cb3..dadeffc 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -4902,8 +4902,8 @@ static void __exit bonding_exit(void)
bond_destroy_debugfs();
- rtnl_link_unregister(&bond_link_ops);
unregister_pernet_subsys(&bond_net_ops);
+ rtnl_link_unregister(&bond_link_ops);
#ifdef CONFIG_NET_POLL_CONTROLLER
/*
--
1.7.1
^ permalink raw reply related
* Re: [PATCH 5/5] IPoIB: add support for TIPC protocol
From: Patrick McHardy @ 2013-04-03 15:44 UTC (permalink / raw)
To: Or Gerlitz
Cc: jon.maloy, allan.stephens, netdev, roland, sean.hefty,
hal.rosenstock, linux-rdma
In-Reply-To: <515C4B65.4040600@mellanox.com>
On Wed, Apr 03, 2013 at 06:31:49PM +0300, Or Gerlitz wrote:
> On 03/04/2013 15:43, Patrick McHardy wrote:
> >[...] all TIPC needs to know about is ARP and RARP since
> >it wants to always perform "path find", even if a path is already known. [...]
>
> Not sure to follow this part... did you mean "all IPoIB needs to
> know about is ARP or RARP", this makes
> sense indeed, since for arp/rarp we want to call unicast_arp_send
> which does path_find and looks
> also for the case the path isn't valid
What I meant is that it doesn't require any knowledge about IPv4/IPv6 or
other higher layer protocols anymore. At least almost none.
We have protocol knowledge in ipoib_start_xmit(). For broadcast packets,
it drops unknown protocols. For unicast packets, it handles ARP/RARP
seperately because of the path find differences, IP/IPv6 are sent
using the neigh, all others are dropped.
ipoib_cm also has knowledge about IPv4/IPv6 in order to send ICMP errors.
What we could do instead of adding TIPC to the broadcast-don't-drop list
and to the send-using-neigh list in ipoib_start_xmit() is to only treat
ARP/RARP special and send every other protocol using the neigh or
ipoib_mcast_send().
Right now the supported protocols are artificially limited without a
technical reason.
^ permalink raw reply
* Re: [0/3] bridge: Do not send multicast queries by default
From: Linus Lüssing @ 2013-04-03 15:28 UTC (permalink / raw)
To: netdev
In-Reply-To: <20120415111300.GA14147@gondor.apana.org.au>
Herbert Xu <herbert <at> gondor.apana.org.au> writes:
>
> On Fri, Apr 13, 2012 at 10:53:45AM -0400, David Miller wrote:
> > From: Herbert Xu <herbert <at> gondor.hengli.com.au>
> > Date: Fri, 13 Apr 2012 20:36:41 +0800
> >
> > > (incidentally, I noticed that our IPv6 code has been "fixed" to not
> > > use zero source addresses, which is wrong as we may end up being THE
> > > MLD querier in a network).
> >
> > I seem to recall it was explicitly changed to be this way and that
> > there was a good reason for this, see the history.
>
> Right, the reason given is that RFC2710 (for MLD) requires the
> source address to be a link-local address.
>
> However, we're not implementing an RFC2710 node here. What we're
> doing is better described by RFC4541 (IGMP/MLD snooping), which calls
> for the use of a zero source address for both IPv4 and IPv6.
Hm, yes, you're right, RFC4541 mentions a zero-source address in a short
paragraph.
>
> The reason is precisely because it's invalid for normal querier
> nodes and as such they would ignore us (rather than elect us
> and potentially disrupt things).
>
Okay, right, RFC2710, section 6, "query received from a router with a lower IP
address" prevents that. But aren't RFC2710, section 5, "query received" and
RFC3810, section 5.1.14 preventing multicast listeners from processing our
query, too?
I'm not sure, but the _informational_ RFC4541 is not supposed to update the
general MLDv1/2 node (especially multicast listener) behaviour from the
proposed standards RFC2710/RFC3810, is it?
> Now granted we may also end up having other nodes ignoring our
> queries where we'd rather that they answered us with reports.
> However, this isn't as bad because the whole querying mechanism
> in the snooping code is merely an optimisation to speed up
> convergence primarily during start-up. So if we don't see the
> reports straight away it's not a deal-breaker.
I'm not sure about that either whether it's just an optimization. Changing the
default setting to a disabled querier in the snooping code actually broke things
for me in a setup where no multicast router is present:
The thing is, with no querier on the link there are no periodic MLDv1 Reports /
MLDv2 Current State Reports (see RFC2710, section 5. for instance, there's no
timer for the "Idle Listener" state; for RFC3810/MLDv2 I didn't find such a
timer either).
If creating a bridge interface after a multicast listener has sent all its
initial reports, then such a multicast listener will never be seen by our
bridge. Which either results in flooding the according multicast traffic on all
ports (because of another bug in the multicast snooping code, a bug which you've
actually fixed for IPv4 already, but not for IPv6, I think) if no other listener
joins after the bridge creation. Or results in this multicast listener not
receiving its traffic from the according IPv6 multicast address with a transient
flag if another listener joins on another port after creating the bridge.
Unfortunately if we were changing things back to sending multicast queries by
default, then this breaks things when another multicast router is present, one
which performs a proper querier protocol, urgh...
Maybe we'd need to implement a proper querier behaviour in the snooping code,
one as specified in RFC3810 for multicast routers, for instance?
And I'm also wondering what if yes the least harmful default settings would be
for the bridge multicast snooping for now.
>
> Cheers,
Cheers, Linus
^ permalink raw reply
* Re: [PATCH 5/5] IPoIB: add support for TIPC protocol
From: Or Gerlitz @ 2013-04-03 15:31 UTC (permalink / raw)
To: Patrick McHardy
Cc: jon.maloy-IzeFyvvaP7pWk0Htik3J/w,
allan.stephens-CWA4WttNNZF54TAoqtyWWQ,
netdev-u79uwXL29TY76Z2rM5mHXA, roland-DgEjT+Ai2ygdnm+yROfE0A,
sean.hefty-ral2JQCrhuEAvxtiuMwx3w,
hal.rosenstock-Re5JQEeQqe8AvxtiuMwx3w,
linux-rdma-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <1364993010-15515-6-git-send-email-kaber-dcUjhNyLwpNeoWH0uzbU5w@public.gmane.org>
On 03/04/2013 15:43, Patrick McHardy wrote:
> [...] all TIPC needs to know about is ARP and RARP since
> it wants to always perform "path find", even if a path is already known. [...]
Not sure to follow this part... did you mean "all IPoIB needs to know
about is ARP or RARP", this makes
sense indeed, since for arp/rarp we want to call unicast_arp_send which
does path_find and looks
also for the case the path isn't valid
Or.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" 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: [Suggestion] ISDN: isdnloop: C grammar issue, '}' miss match 'if' and 'switch' statement.
From: Linus Torvalds @ 2013-04-03 15:30 UTC (permalink / raw)
To: Eric Dumazet
Cc: Michal Kubecek, Chen Gang, Wu Fengguang, Karsten Keil,
David Miller, netdev, Joe Perches
In-Reply-To: <1364999477.13853.21.camel@edumazet-glaptop>
[-- Attachment #1: Type: text/plain, Size: 1387 bytes --]
On Wed, Apr 3, 2013 at 7:31 AM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
>
> And what is the problem exactly ?
The indentation does look completely broken.
It should still *work*, because case-statements don't actually care
about nesting (you can use a case statement to jump into other control
statements, the traditional example is the so-called "duff's device"),
but I agree with Chen Gang that it looks wrong.
I'm attaching a patch that would appear to fix the nesting, but I
haven't actually tested it. Also, regardless of that patch, the code
looks like complete and utter crap, because it sets the "i" variable
in many of the case statements, and then doesn't actually *use* it.
Finally, almost all of the case statements test for something like
if (c->arg < ISDNLOOP_BCH) {
but if "c->arg" is out of range, it will then just break out of the
switch statement and return 0, even though it looks like it should be
an error.
Of course, nobody sane actually cares about ISDN any more, so I think
this is all pretty academic. I think even Germany (where ISDN *used*
to be very common due to telephone monopolies and odd rules) no longer
uses it. I can't imagine that anybody else does either.
But if somebody does care, and can validate my patch (if not by
actually using it, then by at least looking at it more), feel free to
take it and take my sign-off.
Linus
[-- Attachment #2: patch.diff --]
[-- Type: application/octet-stream, Size: 4860 bytes --]
drivers/isdn/isdnloop/isdnloop.c | 140 +++++++++++++++++++--------------------
1 file changed, 70 insertions(+), 70 deletions(-)
diff --git a/drivers/isdn/isdnloop/isdnloop.c b/drivers/isdn/isdnloop/isdnloop.c
index baf2686aa8eb..992b0b78a091 100644
--- a/drivers/isdn/isdnloop/isdnloop.c
+++ b/drivers/isdn/isdnloop/isdnloop.c
@@ -1262,83 +1262,83 @@ isdnloop_command(isdn_ctrl *c, isdnloop_card *card)
}
printk(KERN_DEBUG "isdnloop writecmd '%s'\n", cbuf);
i = isdnloop_writecmd(cbuf, strlen(cbuf), 0, card);
+ }
+ break;
+ case ISDN_CMD_HANGUP:
+ if (!(card->flags & ISDNLOOP_FLAGS_RUNNING))
+ return -ENODEV;
+ if (c->arg < ISDNLOOP_BCH) {
+ a = c->arg + 1;
+ sprintf(cbuf, "%02d;BDIS_R\n%02d;DDIS_R\n", (int) a, (int) a);
+ i = isdnloop_writecmd(cbuf, strlen(cbuf), 0, card);
+ }
+ break;
+ case ISDN_CMD_SETEAZ:
+ if (!(card->flags & ISDNLOOP_FLAGS_RUNNING))
+ return -ENODEV;
+ if (card->leased)
break;
- case ISDN_CMD_HANGUP:
- if (!(card->flags & ISDNLOOP_FLAGS_RUNNING))
- return -ENODEV;
- if (c->arg < ISDNLOOP_BCH) {
- a = c->arg + 1;
- sprintf(cbuf, "%02d;BDIS_R\n%02d;DDIS_R\n", (int) a, (int) a);
- i = isdnloop_writecmd(cbuf, strlen(cbuf), 0, card);
- }
- break;
- case ISDN_CMD_SETEAZ:
- if (!(card->flags & ISDNLOOP_FLAGS_RUNNING))
- return -ENODEV;
- if (card->leased)
- break;
- if (c->arg < ISDNLOOP_BCH) {
- a = c->arg + 1;
- if (card->ptype == ISDN_PTYPE_EURO) {
- sprintf(cbuf, "%02d;MS%s%s\n", (int) a,
- c->parm.num[0] ? "N" : "ALL", c->parm.num);
- } else
- sprintf(cbuf, "%02d;EAZ%s\n", (int) a,
- c->parm.num[0] ? c->parm.num : (u_char *) "0123456789");
- i = isdnloop_writecmd(cbuf, strlen(cbuf), 0, card);
- }
+ if (c->arg < ISDNLOOP_BCH) {
+ a = c->arg + 1;
+ if (card->ptype == ISDN_PTYPE_EURO) {
+ sprintf(cbuf, "%02d;MS%s%s\n", (int) a,
+ c->parm.num[0] ? "N" : "ALL", c->parm.num);
+ } else
+ sprintf(cbuf, "%02d;EAZ%s\n", (int) a,
+ c->parm.num[0] ? c->parm.num : (u_char *) "0123456789");
+ i = isdnloop_writecmd(cbuf, strlen(cbuf), 0, card);
+ }
+ break;
+ case ISDN_CMD_CLREAZ:
+ if (!(card->flags & ISDNLOOP_FLAGS_RUNNING))
+ return -ENODEV;
+ if (card->leased)
break;
- case ISDN_CMD_CLREAZ:
- if (!(card->flags & ISDNLOOP_FLAGS_RUNNING))
- return -ENODEV;
- if (card->leased)
+ if (c->arg < ISDNLOOP_BCH) {
+ a = c->arg + 1;
+ if (card->ptype == ISDN_PTYPE_EURO)
+ sprintf(cbuf, "%02d;MSNC\n", (int) a);
+ else
+ sprintf(cbuf, "%02d;EAZC\n", (int) a);
+ i = isdnloop_writecmd(cbuf, strlen(cbuf), 0, card);
+ }
+ break;
+ case ISDN_CMD_SETL2:
+ if (!(card->flags & ISDNLOOP_FLAGS_RUNNING))
+ return -ENODEV;
+ if ((c->arg & 255) < ISDNLOOP_BCH) {
+ a = c->arg;
+ switch (a >> 8) {
+ case ISDN_PROTO_L2_X75I:
+ sprintf(cbuf, "%02d;BX75\n", (int) (a & 255) + 1);
break;
- if (c->arg < ISDNLOOP_BCH) {
- a = c->arg + 1;
- if (card->ptype == ISDN_PTYPE_EURO)
- sprintf(cbuf, "%02d;MSNC\n", (int) a);
- else
- sprintf(cbuf, "%02d;EAZC\n", (int) a);
- i = isdnloop_writecmd(cbuf, strlen(cbuf), 0, card);
- }
- break;
- case ISDN_CMD_SETL2:
- if (!(card->flags & ISDNLOOP_FLAGS_RUNNING))
- return -ENODEV;
- if ((c->arg & 255) < ISDNLOOP_BCH) {
- a = c->arg;
- switch (a >> 8) {
- case ISDN_PROTO_L2_X75I:
- sprintf(cbuf, "%02d;BX75\n", (int) (a & 255) + 1);
- break;
#ifdef CONFIG_ISDN_X25
- case ISDN_PROTO_L2_X25DTE:
- sprintf(cbuf, "%02d;BX2T\n", (int) (a & 255) + 1);
- break;
- case ISDN_PROTO_L2_X25DCE:
- sprintf(cbuf, "%02d;BX2C\n", (int) (a & 255) + 1);
- break;
+ case ISDN_PROTO_L2_X25DTE:
+ sprintf(cbuf, "%02d;BX2T\n", (int) (a & 255) + 1);
+ break;
+ case ISDN_PROTO_L2_X25DCE:
+ sprintf(cbuf, "%02d;BX2C\n", (int) (a & 255) + 1);
+ break;
#endif
- case ISDN_PROTO_L2_HDLC:
- sprintf(cbuf, "%02d;BTRA\n", (int) (a & 255) + 1);
- break;
- case ISDN_PROTO_L2_TRANS:
- sprintf(cbuf, "%02d;BTRA\n", (int) (a & 255) + 1);
- break;
- default:
- return -EINVAL;
- }
- i = isdnloop_writecmd(cbuf, strlen(cbuf), 0, card);
- card->l2_proto[a & 255] = (a >> 8);
+ case ISDN_PROTO_L2_HDLC:
+ sprintf(cbuf, "%02d;BTRA\n", (int) (a & 255) + 1);
+ break;
+ case ISDN_PROTO_L2_TRANS:
+ sprintf(cbuf, "%02d;BTRA\n", (int) (a & 255) + 1);
+ break;
+ default:
+ return -EINVAL;
}
- break;
- case ISDN_CMD_SETL3:
- if (!(card->flags & ISDNLOOP_FLAGS_RUNNING))
- return -ENODEV;
- return 0;
- default:
- return -EINVAL;
+ i = isdnloop_writecmd(cbuf, strlen(cbuf), 0, card);
+ card->l2_proto[a & 255] = (a >> 8);
}
+ break;
+ case ISDN_CMD_SETL3:
+ if (!(card->flags & ISDNLOOP_FLAGS_RUNNING))
+ return -ENODEV;
+ return 0;
+ default:
+ return -EINVAL;
}
return 0;
}
^ permalink raw reply related
* Re: [v3.8, v3.9] [Regression] brcmsmac: move PHY functions
From: Joseph Salisbury @ 2013-04-03 15:28 UTC (permalink / raw)
To: Piotr Haber
Cc: John W. Linville, arend, pieterpg, meuleman, LKML, brudley,
frankyl, linux-wireless, brcm80211-dev-list, netdev
In-Reply-To: <515A9EAF.4010005@broadcom.com>
On 04/02/2013 05:02 AM, Piotr Haber wrote:
> On 04/01/13 17:18, Joseph Salisbury wrote:
>> On 04/01/2013 10:42 AM, John W. Linville wrote:
>>> On Fri, Mar 29, 2013 at 03:52:39PM -0400, Joseph Salisbury wrote:
>>>> Hi Piotr,
>>>>
>>>> A bug was opened against the Ubuntu kernel[0]. After a kernel
>>>> bisect, it was found that reverting the following commit resolved
>>>> this bug:
>>>>
>>>> commit b83576341664957978e125f5f5db2f15496980b1
>>>> Author: Piotr Haber <phaber@broadcom.com>
>>>> Date: Wed Nov 28 21:44:09 2012 +0100
>>>>
>>>> brcmsmac: move PHY functions
>>>>
>>>> The regression was introduced as of v3.8-rc1. The regression still
>>>> exists in v3.9-rc4.
>>>>
>>>> I see that you are the author of this patch, so I wanted to run this
>>>> by you. I was thinking of requesting a revert for v3.9, but I
>>>> wanted to get your feedback first.
>>>>
>>>>
>>>> Thanks,
>>>>
>>>> Joe
>>>>
>>>> [0] http://pad.lv/1131914
>>> I recently reverted b6fc28a1, which is the follow-on to that patch.
>>> The revert is _not_ in 3.9-rc5.
>>>
>>> Could you try reverting that patch instead? Does that fix the issue
>>> for you?
>>>
>>> John
>> Hi John,
>>
>> Thanks for the response.
>>
>> Yes, reverting commit b6fc28a1 does resolve this bug. That is the appropriate fix for this issue.
>> Thanks for the assistance.
>>
>> Thanks,
>>
>> Joe
>>
>>
>>
>>
> Hi Joe,
> could you elaborate a little bit on your failure scenario?
> In bug report you say it happens after suspend/resume, are there any other scenarios you see this
> behaviour? (like disassociation/association without suspend)
> Also you mention it comes back after some time - what is the time needed?
> We had reports of problems on 4313 with this patch (that's why the revert was done) but so far i
> assumed it was a total breakdown, in your case this seem like a transient issue.
This only happens after a suspend/resume cycle. I haven't seen the
issue happen with disassociation/association without suspend. After
suspend, the connection will re-establish after about 15 minutes or so.
>
> One more thing, could you provide info about your hardware by sending me contents of:
> <debugfs_mount>/brcmsmac/bcma0:0/hardware
board vendor: 144f
board type: 7179
board revision: 1408
board flags: 8402a01
board flags2: 880
firmware revision: 262032c
>
> Kind regards
> Piotr
>
>
^ permalink raw reply
* Re: [Suggestion] ISDN: isdnloop: C grammar issue, '}' miss match 'if' and 'switch' statement.
From: Michal Kubecek @ 2013-04-03 15:08 UTC (permalink / raw)
To: Eric Dumazet
Cc: Chen Gang, fengguang.wu, isdn, Linus Torvalds, David Miller,
netdev, Joe Perches
In-Reply-To: <1364999477.13853.21.camel@edumazet-glaptop>
On Wed, Apr 03, 2013 at 07:31:17AM -0700, Eric Dumazet wrote:
> On Wed, 2013-04-03 at 16:08 +0200, Michal Kubecek wrote:
>
> > As far as I can see, this rather comes from
> >
> > commit 475be4d85a274d0961593db41cf85689db1d583c
> > Author: Joe Perches <joe@perches.com>
> > Date: Sun Feb 19 19:52:38 2012 -0800
> >
> > isdn: whitespace coding style cleanup
>
> And what is the problem exactly ?
>
> # make drivers/isdn/isdnloop/isdnloop.o
> ...
> CC [M] drivers/isdn/isdnloop/isdnloop.o
>
> No error here.
I just checked that gcc (surprisingly - at least for me) accepts
switch(a) {
case 1:
if(...) {
do_something();
break;
case 2:
...
break;
default:
...
break;
}
}
and it seems to work the same as
switch(a) {
case 1:
if(...) {
do_something();
}
break;
case 2:
...
break;
default:
...
break;
}
However, I can't think of a reason to use the former.
Michal Kubecek
^ permalink raw reply
* Re: [PATCH -next] sctp: fix error return code in __sctp_connect()
From: Neil Horman @ 2013-04-03 14:59 UTC (permalink / raw)
To: Vlad Yasevich; +Cc: Wei Yongjun, davem, yongjun_wei, linux-sctp, netdev
In-Reply-To: <515C4220.6030802@gmail.com>
On Wed, Apr 03, 2013 at 10:52:16AM -0400, Vlad Yasevich wrote:
> On 04/03/2013 09:51 AM, Neil Horman wrote:
> >On Wed, Apr 03, 2013 at 09:02:28PM +0800, Wei Yongjun wrote:
> >>From: Wei Yongjun <yongjun_wei@trendmicro.com.cn>
> >>
> >>Fix to return a negative error code from the error handling
> >>case instead of 0, as returned elsewhere in this function.
> >>
> >>Signed-off-by: Wei Yongjun <yongjun_wei@trendmicro.com.cn>
> >>---
> >> net/sctp/socket.c | 5 +++--
> >> 1 file changed, 3 insertions(+), 2 deletions(-)
> >>
> >>diff --git a/net/sctp/socket.c b/net/sctp/socket.c
> >>index dd21ae3..f631c5f 100644
> >>--- a/net/sctp/socket.c
> >>+++ b/net/sctp/socket.c
> >>@@ -1119,9 +1119,10 @@ static int __sctp_connect(struct sock* sk,
> >> /* Make sure the destination port is correctly set
> >> * in all addresses.
> >> */
> >>- if (asoc && asoc->peer.port && asoc->peer.port != port)
> >>+ if (asoc && asoc->peer.port && asoc->peer.port != port) {
> >>+ err = -EINVAL;
> >> goto out_free;
> >>-
> >>+ }
> >>
> >> /* Check if there already is a matching association on the
> >> * endpoint (other than the one created here).
> >>
> >>--
> >>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
> >>
> >Actually, I think you can remove that entire if statement (as well as some
> >checks further down). Looking at the net-next trees __sctp_connect, it appears
> >that asoc is set to NULL at the top of the function, and not assigned to
> >anything else until the call to sctp_association_new much farther down (line
> >1201). That means the above if statement, as well as this:
> > if (asoc2 && asoc2 != asoc) {
> >and this:
> >if (!asoc) {
> >will always be false, false, and true, respectively.
>
> No, I don't think you can. Consider a case of sctp_connectx() where
> each address specified in connectx has a different destination port.
>
> First time through the loop, we'll create the association and set
> the peer.port. The second time through the loop, we'll compare the
> that port to the port specified in the second address. If the ports
> do not match, we need to stop.
>
> -vlad
Ah, you're right, I missed the while loop, apologies
Acked-by: Neil Horman <nhorman@tuxdriver.com>
> >
> >Regards
> >Neil
> >
>
> --
> 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: [PATCH v5 2/6] usb: phy: omap-usb2: use the new generic PHY framework
From: Arnd Bergmann @ 2013-04-03 14:55 UTC (permalink / raw)
To: balbi
Cc: Kishon Vijay Abraham I, gregkh, akpm, swarren, sylvester.nawrocki,
rob, netdev, davem, cesarb, linux-usb, linux-omap, linux-kernel,
tony, grant.likely, rob.herring, b-cousson, linux, eballetbo,
javier, mchehab, santosh.shilimkar, broonie, swarren, linux-doc,
devicetree-discuss, linux-arm-kernel
In-Reply-To: <20130403134827.GF14680@arwen.pp.htv.fi>
On Wednesday 03 April 2013, Felipe Balbi wrote:
> const ? Maybe provide a:
>
> #define DEFINE_PHY_OPS(name) \
> const struct phy_ops #name_phy_ops = {
>
> macro ? This will force people to add the const keyword :-)
Forcing people to use const structures is good, but I think it would be
better without the macro, just by marking the argument in
devm_phy_create() as const.
Arnd
^ permalink raw reply
* Re: [PATCH -next] sctp: fix error return code in __sctp_connect()
From: Vlad Yasevich @ 2013-04-03 14:52 UTC (permalink / raw)
To: Wei Yongjun; +Cc: sri, nhorman, davem, yongjun_wei, linux-sctp, netdev
In-Reply-To: <CAPgLHd-X5Qw64BnaSMr2ny6==jXGpRAYC7_1MGhP2v_GcVeApQ@mail.gmail.com>
On 04/03/2013 09:02 AM, Wei Yongjun wrote:
> From: Wei Yongjun <yongjun_wei@trendmicro.com.cn>
>
> Fix to return a negative error code from the error handling
> case instead of 0, as returned elsewhere in this function.
>
> Signed-off-by: Wei Yongjun <yongjun_wei@trendmicro.com.cn>
Acked-by: Vlad Yasevich <vyasevich@gmail.com>
-vlad
> ---
> net/sctp/socket.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/net/sctp/socket.c b/net/sctp/socket.c
> index dd21ae3..f631c5f 100644
> --- a/net/sctp/socket.c
> +++ b/net/sctp/socket.c
> @@ -1119,9 +1119,10 @@ static int __sctp_connect(struct sock* sk,
> /* Make sure the destination port is correctly set
> * in all addresses.
> */
> - if (asoc && asoc->peer.port && asoc->peer.port != port)
> + if (asoc && asoc->peer.port && asoc->peer.port != port) {
> + err = -EINVAL;
> goto out_free;
> -
> + }
>
> /* Check if there already is a matching association on the
> * endpoint (other than the one created here).
>
^ permalink raw reply
* Re: [PATCH -next] sctp: fix error return code in __sctp_connect()
From: Vlad Yasevich @ 2013-04-03 14:52 UTC (permalink / raw)
To: Neil Horman; +Cc: Wei Yongjun, davem, yongjun_wei, linux-sctp, netdev
In-Reply-To: <20130403135126.GA32276@hmsreliant.think-freely.org>
On 04/03/2013 09:51 AM, Neil Horman wrote:
> On Wed, Apr 03, 2013 at 09:02:28PM +0800, Wei Yongjun wrote:
>> From: Wei Yongjun <yongjun_wei@trendmicro.com.cn>
>>
>> Fix to return a negative error code from the error handling
>> case instead of 0, as returned elsewhere in this function.
>>
>> Signed-off-by: Wei Yongjun <yongjun_wei@trendmicro.com.cn>
>> ---
>> net/sctp/socket.c | 5 +++--
>> 1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/net/sctp/socket.c b/net/sctp/socket.c
>> index dd21ae3..f631c5f 100644
>> --- a/net/sctp/socket.c
>> +++ b/net/sctp/socket.c
>> @@ -1119,9 +1119,10 @@ static int __sctp_connect(struct sock* sk,
>> /* Make sure the destination port is correctly set
>> * in all addresses.
>> */
>> - if (asoc && asoc->peer.port && asoc->peer.port != port)
>> + if (asoc && asoc->peer.port && asoc->peer.port != port) {
>> + err = -EINVAL;
>> goto out_free;
>> -
>> + }
>>
>> /* Check if there already is a matching association on the
>> * endpoint (other than the one created here).
>>
>> --
>> 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
>>
> Actually, I think you can remove that entire if statement (as well as some
> checks further down). Looking at the net-next trees __sctp_connect, it appears
> that asoc is set to NULL at the top of the function, and not assigned to
> anything else until the call to sctp_association_new much farther down (line
> 1201). That means the above if statement, as well as this:
> if (asoc2 && asoc2 != asoc) {
> and this:
> if (!asoc) {
> will always be false, false, and true, respectively.
No, I don't think you can. Consider a case of sctp_connectx() where
each address specified in connectx has a different destination port.
First time through the loop, we'll create the association and set the
peer.port. The second time through the loop, we'll compare the that
port to the port specified in the second address. If the ports do not
match, we need to stop.
-vlad
>
> Regards
> Neil
>
^ permalink raw reply
* Re: [PATCH 4/5] tipc: add InfiniBand media type
From: Patrick McHardy @ 2013-04-03 14:49 UTC (permalink / raw)
To: Erik Hugne
Cc: jon.maloy, allan.stephens, netdev, roland, sean.hefty,
hal.rosenstock, linux-rdma
In-Reply-To: <20130403144140.GA19219@eerihug-hybrid.ki.sw.ericsson.se>
On Wed, Apr 03, 2013 at 04:41:40PM +0200, Erik Hugne wrote:
> On Wed, Apr 03, 2013 at 02:43:29PM +0200, Patrick McHardy wrote:
> > diff --git a/net/tipc/Kconfig b/net/tipc/Kconfig
> > index 4f99600..900ee66 100644
> > --- a/net/tipc/Kconfig
> > +++ b/net/tipc/Kconfig
> > @@ -31,3 +31,10 @@ config TIPC_PORTS
> >
> > Setting this to a smaller value saves some memory,
> > setting it to higher allows for more ports.
> > +
> > +config TIPC_MEDIA_IB
> > + bool "InfiniBand media type support"
> > + depends on INFINIBAND_IPOIB
> > + help
> > + Saying Y here will enable support for running TIPC on
> > + IP-over-InfiniBand devices.
> > diff --git a/net/tipc/Makefile b/net/tipc/Makefile
> > index 6cd55d6..4df8e02 100644
> > --- a/net/tipc/Makefile
> > +++ b/net/tipc/Makefile
> > @@ -9,3 +9,5 @@ tipc-y += addr.o bcast.o bearer.o config.o \
> > name_distr.o subscr.o name_table.o net.o \
> > netlink.o node.o node_subscr.o port.o ref.o \
> > socket.o log.o eth_media.o
> > +
> > +tipc-$(CONFIG_TIPC_MEDIA_IB) += ib_media.o
>
> The TIPC_MEDIA_IB option shows up directly under networking options,
> instead of under "TIPC". I think "depends on TIPC" is missing?
Oops, I guess I messed that up during forward porting. I'll fix it up for
the next submission, thanks.
^ permalink raw reply
* Re: [PATCH 4/5] tipc: add InfiniBand media type
From: Erik Hugne @ 2013-04-03 14:41 UTC (permalink / raw)
To: Patrick McHardy
Cc: jon.maloy, allan.stephens, netdev, roland, sean.hefty,
hal.rosenstock, linux-rdma
In-Reply-To: <1364993010-15515-5-git-send-email-kaber@trash.net>
On Wed, Apr 03, 2013 at 02:43:29PM +0200, Patrick McHardy wrote:
> diff --git a/net/tipc/Kconfig b/net/tipc/Kconfig
> index 4f99600..900ee66 100644
> --- a/net/tipc/Kconfig
> +++ b/net/tipc/Kconfig
> @@ -31,3 +31,10 @@ config TIPC_PORTS
>
> Setting this to a smaller value saves some memory,
> setting it to higher allows for more ports.
> +
> +config TIPC_MEDIA_IB
> + bool "InfiniBand media type support"
> + depends on INFINIBAND_IPOIB
> + help
> + Saying Y here will enable support for running TIPC on
> + IP-over-InfiniBand devices.
> diff --git a/net/tipc/Makefile b/net/tipc/Makefile
> index 6cd55d6..4df8e02 100644
> --- a/net/tipc/Makefile
> +++ b/net/tipc/Makefile
> @@ -9,3 +9,5 @@ tipc-y += addr.o bcast.o bearer.o config.o \
> name_distr.o subscr.o name_table.o net.o \
> netlink.o node.o node_subscr.o port.o ref.o \
> socket.o log.o eth_media.o
> +
> +tipc-$(CONFIG_TIPC_MEDIA_IB) += ib_media.o
The TIPC_MEDIA_IB option shows up directly under networking options,
instead of under "TIPC". I think "depends on TIPC" is missing?
//E
^ permalink raw reply
* Re: [PATCH v5 1/6] drivers: phy: add generic PHY framework
From: Kishon Vijay Abraham I @ 2013-04-03 14:32 UTC (permalink / raw)
To: balbi
Cc: gregkh, arnd, akpm, swarren, sylvester.nawrocki, rob, netdev,
davem, cesarb, linux-usb, linux-omap, linux-kernel, tony,
grant.likely, rob.herring, b-cousson, linux, eballetbo, javier,
mchehab, santosh.shilimkar, broonie, swarren, linux-doc,
devicetree-discuss, linux-arm-kernel
In-Reply-To: <20130403142747.GK14680@arwen.pp.htv.fi>
Hi,
On Wednesday 03 April 2013 07:57 PM, Felipe Balbi wrote:
> hi,
>
> On Wed, Apr 03, 2013 at 07:48:42PM +0530, Kishon Vijay Abraham I wrote:
>>>> +struct phy *of_phy_xlate(struct phy *phy, struct of_phandle_args *args)
>>>> +{
>>>> + return phy;
>>>> +}
>>>> +EXPORT_SYMBOL_GPL(of_phy_xlate);
>>>
>>> so you get a PHY and just return it ? What gives ?? (maybe I skipped
>>> some of the discussion...)
>>
>> hmm.. this is for the common case where the PHY provider implements
>> only one PHY. And both phy provider and phy_instance is represented
>> by struct phy *.
>>
>> For the case where PHY provider implements multiple PHYs (here it
>> will have a single dt node), the PHY provider will implement it's own
>> version of of_xlate that takes *of_phandle_args* as argument and
>> finds the appropriate PHY.
>
> got it.
>
>>>> +struct phy *of_phy_get(struct device *dev, int index)
>>>> +{
>>>> + int ret;
>>>> + struct phy *phy = NULL;
>>>> + struct phy_bind *phy_map = NULL;
>>>> + struct of_phandle_args args;
>>>> + struct device_node *node;
>>>> +
>>>> + if (!dev->of_node) {
>>>> + dev_dbg(dev, "device does not have a device node entry\n");
>>>> + return ERR_PTR(-EINVAL);
>>>> + }
>>>> +
>>>> + ret = of_parse_phandle_with_args(dev->of_node, "phys", "#phy-cells",
>>>> + index, &args);
>>>> + if (ret) {
>>>> + dev_dbg(dev, "failed to get phy in %s node\n",
>>>> + dev->of_node->full_name);
>>>> + return ERR_PTR(-ENODEV);
>>>> + }
>>>> +
>>>> + phy = of_phy_lookup(args.np);
>>>> + if (IS_ERR(phy) || !try_module_get(phy->ops->owner)) {
>>>> + phy = ERR_PTR(-EPROBE_DEFER);
>>>> + goto err0;
>>>> + }
>>>> +
>>>> + phy = phy->ops->of_xlate(phy, &args);
>>>
>>> alright, so of_xlate() is optional, am I right ? How about not
>>
>> Not really. of_xlate is mandatory (it's even checked in phy_create).
>> Either the PHY provider can implement it's own version or use the
>> implementation above (by filling the function pointer).
>
> alright.
>
>>> implementing the above and have a check for of_xlate() being a valid
>>> pointer here ?
>>
>> Having the way it is actually mandates the PHY providers to always
>> provide of_xlate which IMO is better since some PHY providers wont
>> accidentally be using the default implementation.
>
> ok cool, thanks for clarifying.
>
>>>> + ret = -EINVAL;
>>>> + goto err0;
>>>> + }
>>>> +
>>>> + if (!phy_class)
>>>> + phy_core_init();
>>>
>>> why don't you setup the class on module_init ? Then this would be a
>>> terrible error condition here :-)
>>
>> This is for the case where the PHY driver gets loaded before the PHY
>> framework. I could have returned EPROBE_DEFER here instead I thought
>> will have it this way.
>
> looks a bit weird IMO. Is it really possible for PHY to load before ?
yeah. it actually happened when I tried with beagle and had all the
modules as built-in. Because twl4030 has subsys_initcall(), it loads
before PHY framework.
Thanks
Kishon
^ permalink raw reply
* Re: [Suggestion] ISDN: isdnloop: C grammar issue, '}' miss match 'if' and 'switch' statement.
From: Eric Dumazet @ 2013-04-03 14:31 UTC (permalink / raw)
To: Michal Kubecek
Cc: Chen Gang, fengguang.wu, isdn, Linus Torvalds, David Miller,
netdev, Joe Perches
In-Reply-To: <20130403140823.GB3401@unicorn.suse.cz>
On Wed, 2013-04-03 at 16:08 +0200, Michal Kubecek wrote:
> As far as I can see, this rather comes from
>
> commit 475be4d85a274d0961593db41cf85689db1d583c
> Author: Joe Perches <joe@perches.com>
> Date: Sun Feb 19 19:52:38 2012 -0800
>
> isdn: whitespace coding style cleanup
And what is the problem exactly ?
# make drivers/isdn/isdnloop/isdnloop.o
...
CC [M] drivers/isdn/isdnloop/isdnloop.o
No error here.
^ permalink raw reply
* Re: [PATCH v5 1/6] drivers: phy: add generic PHY framework
From: Felipe Balbi @ 2013-04-03 14:27 UTC (permalink / raw)
To: Kishon Vijay Abraham I
Cc: broonie-yzvPICuk2AATkU/dhu1WVueM+bqZidxxQQ4Iyu8u01E,
mchehab-H+wXaHxf7aLQT0dZR+AlfA, linux-doc-u79uwXL29TY76Z2rM5mHXA,
linux-lFZ/pmaqli7XmaaqVzeoHQ, javier-0uQlZySMnqxg9hUCZPvPmw,
cesarb-PWySMVKUnqmsTnJN9+BGXg, eballetbo-Re5JQEeQqe8AvxtiuMwx3w,
devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ,
rob.herring-bsGFqQB8/DxBDgjK7y7TUQ,
swarren-DDmLM1+adcrQT0dZR+AlfA,
sylvester.nawrocki-Re5JQEeQqe8AvxtiuMwx3w,
linux-omap-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r,
linux-usb-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA, balbi-l0cyMroinI0,
santosh.shilimkar-l0cyMroinI0, netdev-u79uwXL29TY76Z2rM5mHXA,
akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b,
davem-fT/PcQaiUtIeIZ0/mPfg9Q
In-Reply-To: <515C3A42.4020404-l0cyMroinI0@public.gmane.org>
[-- Attachment #1.1: Type: text/plain, Size: 3538 bytes --]
hi,
On Wed, Apr 03, 2013 at 07:48:42PM +0530, Kishon Vijay Abraham I wrote:
> >>+struct phy *of_phy_xlate(struct phy *phy, struct of_phandle_args *args)
> >>+{
> >>+ return phy;
> >>+}
> >>+EXPORT_SYMBOL_GPL(of_phy_xlate);
> >
> >so you get a PHY and just return it ? What gives ?? (maybe I skipped
> >some of the discussion...)
>
> hmm.. this is for the common case where the PHY provider implements
> only one PHY. And both phy provider and phy_instance is represented
> by struct phy *.
>
> For the case where PHY provider implements multiple PHYs (here it
> will have a single dt node), the PHY provider will implement it's own
> version of of_xlate that takes *of_phandle_args* as argument and
> finds the appropriate PHY.
got it.
> >>+struct phy *of_phy_get(struct device *dev, int index)
> >>+{
> >>+ int ret;
> >>+ struct phy *phy = NULL;
> >>+ struct phy_bind *phy_map = NULL;
> >>+ struct of_phandle_args args;
> >>+ struct device_node *node;
> >>+
> >>+ if (!dev->of_node) {
> >>+ dev_dbg(dev, "device does not have a device node entry\n");
> >>+ return ERR_PTR(-EINVAL);
> >>+ }
> >>+
> >>+ ret = of_parse_phandle_with_args(dev->of_node, "phys", "#phy-cells",
> >>+ index, &args);
> >>+ if (ret) {
> >>+ dev_dbg(dev, "failed to get phy in %s node\n",
> >>+ dev->of_node->full_name);
> >>+ return ERR_PTR(-ENODEV);
> >>+ }
> >>+
> >>+ phy = of_phy_lookup(args.np);
> >>+ if (IS_ERR(phy) || !try_module_get(phy->ops->owner)) {
> >>+ phy = ERR_PTR(-EPROBE_DEFER);
> >>+ goto err0;
> >>+ }
> >>+
> >>+ phy = phy->ops->of_xlate(phy, &args);
> >
> >alright, so of_xlate() is optional, am I right ? How about not
>
> Not really. of_xlate is mandatory (it's even checked in phy_create).
> Either the PHY provider can implement it's own version or use the
> implementation above (by filling the function pointer).
alright.
> >implementing the above and have a check for of_xlate() being a valid
> >pointer here ?
>
> Having the way it is actually mandates the PHY providers to always
> provide of_xlate which IMO is better since some PHY providers wont
> accidentally be using the default implementation.
ok cool, thanks for clarifying.
> >>+ ret = -EINVAL;
> >>+ goto err0;
> >>+ }
> >>+
> >>+ if (!phy_class)
> >>+ phy_core_init();
> >
> >why don't you setup the class on module_init ? Then this would be a
> >terrible error condition here :-)
>
> This is for the case where the PHY driver gets loaded before the PHY
> framework. I could have returned EPROBE_DEFER here instead I thought
> will have it this way.
looks a bit weird IMO. Is it really possible for PHY to load before ?
Since PHY driver uses symbols from phy-core, modprobe will make sure to
load drivers based on symbol dependency, right ?
> >>+static struct device_attribute phy_dev_attrs[] = {
> >>+ __ATTR(label, 0444, phy_name_show, NULL),
> >>+ __ATTR(phy_bind, 0444, phy_bind_show, NULL),
> >
> >you could expose a human-readable 'type' string. BTW, how are you using
> >type ? USB2/USB3/etc ? Have you considered our OMAP5 SerDes pair which
>
> Actually not using *type* anywhere. Just used as an additional info
> about the PHY. It's actually not even enum. Maybe I can remove it
> completely.
makes sense.
> >are currently for USB3 and SATA (and could just as easily be used for
> >PCIe)
>
> Yeah. Me and Balaji were planning to work on it for having a single
> driver to be used for all the above.
cool :-)
--
balbi
[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
[-- Attachment #2: Type: text/plain, Size: 192 bytes --]
_______________________________________________
devicetree-discuss mailing list
devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org
https://lists.ozlabs.org/listinfo/devicetree-discuss
^ permalink raw reply
* Re: [PATCH v5 1/6] drivers: phy: add generic PHY framework
From: Kishon Vijay Abraham I @ 2013-04-03 14:18 UTC (permalink / raw)
To: balbi-l0cyMroinI0
Cc: linux-usb-u79uwXL29TY76Z2rM5mHXA, mchehab-H+wXaHxf7aLQT0dZR+AlfA,
linux-doc-u79uwXL29TY76Z2rM5mHXA, linux-lFZ/pmaqli7XmaaqVzeoHQ,
javier-0uQlZySMnqxg9hUCZPvPmw, cesarb-PWySMVKUnqmsTnJN9+BGXg,
eballetbo-Re5JQEeQqe8AvxtiuMwx3w,
devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ,
rob.herring-bsGFqQB8/DxBDgjK7y7TUQ,
swarren-DDmLM1+adcrQT0dZR+AlfA,
sylvester.nawrocki-Re5JQEeQqe8AvxtiuMwx3w,
linux-omap-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r,
broonie-yzvPICuk2AATkU/dhu1WVueM+bqZidxxQQ4Iyu8u01E,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
santosh.shilimkar-l0cyMroinI0, netdev-u79uwXL29TY76Z2rM5mHXA,
akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b,
davem-fT/PcQaiUtIeIZ0/mPfg9Q
In-Reply-To: <20130403134102.GC14680-S8G//mZuvNWo5Im9Ml3/Zg@public.gmane.org>
Hi,
On Wednesday 03 April 2013 07:12 PM, Felipe Balbi wrote:
> On Wed, Apr 03, 2013 at 06:23:49PM +0530, Kishon Vijay Abraham I wrote:
>> The PHY framework provides a set of APIs for the PHY drivers to
>> create/destroy a PHY and APIs for the PHY users to obtain a reference to the
>> PHY with or without using phandle. To obtain a reference to the PHY without
>> using phandle, the platform specfic intialization code (say from board file)
>> should have already called phy_bind with the binding information. The binding
>> information consists of phy's device name, phy user device name and an index.
>> The index is used when the same phy user binds to mulitple phys.
>>
>> PHY drivers should create the PHY by passing phy_descriptor that has
>> describes the PHY (label, type etc..) and ops like init, exit, suspend, resume,
>> power_on, power_off.
>>
>> The documentation for the generic PHY framework is added in
>> Documentation/phy.txt and the documentation for the sysfs entry is added
>> in Documentation/ABI/testing/sysfs-class-phy and the documentation for
>> dt binding is can be found at
>> Documentation/devicetree/bindings/phy/phy-bindings.txt
>>
>> Signed-off-by: Kishon Vijay Abraham I <kishon-l0cyMroinI0@public.gmane.org>
>> ---
>> Documentation/ABI/testing/sysfs-class-phy | 15 +
>> .../devicetree/bindings/phy/phy-bindings.txt | 67 +++
>> Documentation/phy.txt | 113 ++++
>> MAINTAINERS | 7 +
>> drivers/Kconfig | 2 +
>> drivers/Makefile | 2 +
>> drivers/phy/Kconfig | 13 +
>> drivers/phy/Makefile | 5 +
>> drivers/phy/phy-core.c | 616 ++++++++++++++++++++
>> include/linux/phy/phy.h | 228 ++++++++
>> 10 files changed, 1068 insertions(+)
>> create mode 100644 Documentation/ABI/testing/sysfs-class-phy
>> create mode 100644 Documentation/devicetree/bindings/phy/phy-bindings.txt
>> create mode 100644 Documentation/phy.txt
>> create mode 100644 drivers/phy/Kconfig
>> create mode 100644 drivers/phy/Makefile
>> create mode 100644 drivers/phy/phy-core.c
>> create mode 100644 include/linux/phy/phy.h
>>
>> diff --git a/Documentation/ABI/testing/sysfs-class-phy b/Documentation/ABI/testing/sysfs-class-phy
>> new file mode 100644
>> index 0000000..b735467
>> --- /dev/null
>> +++ b/Documentation/ABI/testing/sysfs-class-phy
>> @@ -0,0 +1,15 @@
>> +What: /sys/class/phy/<phy>/label
>> +Date: Apr 2013
>> +KernelVersion: 3.10
>> +Contact: Kishon Vijay Abraham I <kishon-l0cyMroinI0@public.gmane.org>
>> +Description:
>> + This is a read-only file for getting the label of the phy.
>> +
>> +What: /sys/class/phy/<phy>/phy_bind
>> +Date: Apr 2013
>> +KernelVersion: 3.10
>> +Contact: Kishon Vijay Abraham I <kishon-l0cyMroinI0@public.gmane.org>
>> +Description:
>> + This is a read-only file for reading the phy binding
>> + information. It contains the device name of the controller,
>> + the index and the device name of the PHY in that order.
>> diff --git a/Documentation/devicetree/bindings/phy/phy-bindings.txt b/Documentation/devicetree/bindings/phy/phy-bindings.txt
>> new file mode 100644
>> index 0000000..e7b246a
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/phy/phy-bindings.txt
>> @@ -0,0 +1,67 @@
>> +This document explains only the dt data binding. For general information about
>> +PHY subsystem refer Documentation/phy.txt
>> +
>> +PHY device node
>> +===============
>> +
>> +Required Properties:
>> +#phy-cells: Number of cells in a PHY specifier; The meaning of all those
>> + cells is defined by the binding for the phy node. The PHY
>> + provider can use the values in cells to find the appropriate
>> + PHY.
>> +
>> +For example:
>> +
>> +phys: phy {
>> + compatible = "xxx";
>> + reg = <...>;
>> + .
>> + .
>> + #phy-cells = <1>;
>> + .
>> + .
>> +};
>> +
>> +That node describes an IP block that implements 2 different PHYs. In order to
>> +differentiate between these 2 PHYs, an additonal specifier should be given
>> +while trying to get a reference to it.
>> +
>> +PHY user node
>> +=============
>> +
>> +Required Properties:
>> +phys : the phandle for the PHY device (used by the PHY subsystem)
>> +
>> +Optional properties:
>> +phy-names : the names of the PHY corresponding to the PHYs present in the
>> + *phys* phandle
>> +
>> +Example 1:
>> +usb1: usb_otg_ss@xxx {
>> + compatible = "xxx";
>> + reg = <xxx>;
>> + .
>> + .
>> + phys = <&usb2_phy>, <&usb3_phy>;
>> + phy-names = "usb2phy", "usb3phy";
>> + .
>> + .
>> +};
>> +
>> +This node represents a controller that uses two PHYs one for usb2 and one for
>> +usb3.
>> +
>> +Example 2:
>> +usb2: usb_otg_ss@xxx {
>> + compatible = "xxx";
>> + reg = <xxx>;
>> + .
>> + .
>> + phys = <&phys 1>;
>> + .
>> + .
>> +};
>> +
>> +This node represents a controller that uses one of the PHYs which is defined
>> +previously. Note that the phy handle has an additional specifier "1" to
>> +differentiate between the two PHYs.
>> diff --git a/Documentation/phy.txt b/Documentation/phy.txt
>> new file mode 100644
>> index 0000000..7785ec0
>> --- /dev/null
>> +++ b/Documentation/phy.txt
>> @@ -0,0 +1,113 @@
>> + PHY SUBSYSTEM
>> + Kishon Vijay Abraham I <kishon-l0cyMroinI0@public.gmane.org>
>> +
>> +This document explains the Generic PHY Framework along with the APIs provided,
>> +and how-to-use.
>> +
>> +1. Introduction
>> +
>> +*PHY* is the abbreviation for physical layer. It is used to connect a device
>> +to the physical medium e.g., the USB controller has a PHY to provide functions
>> +such as serialization, de-serialization, encoding, decoding and is responsible
>> +for obtaining the required data transmission rate. Note that some USB
>> +controller has PHY functionality embedded into it and others use an external
>> +PHY. Other peripherals that uses a PHY include Wireless LAN, Ethernet,
>> +SATA etc.
>> +
>> +The intention of creating this framework is to bring the phy drivers spread
>> +all over the Linux kernel to drivers/phy to increase code re-use and to
>> +increase code maintainability.
>> +
>> +This framework will be of use only to devices that uses external PHY (PHY
>> +functionality is not embedded within the controller).
>> +
>> +2. Creating the PHY
>> +
>> +The PHY driver should create the PHY in order for other peripheral controllers
>> +to make use of it. The PHY framework provides 2 APIs to create the PHY.
>> +
>> +struct phy *phy_create(struct device *dev, const char *label,
>> + struct device_node *of_node, int type, struct phy_ops *ops,
>> + void *priv);
>> +struct phy *devm_phy_create(struct device *dev, const char *label,
>> + struct device_node *of_node, int type, struct phy_ops *ops,
>> + void *priv);
>> +
>> +The PHY drivers can use one of the above 2 APIs to create the PHY by passing
>> +the device pointer, label, device node, type, phy ops and a driver data.
>> +phy_ops is a set of function pointers for performing PHY operations such as
>> +init, exit, suspend, resume, power_on and power_off.
>> +
>> +3. Binding the PHY to the controller
>> +
>> +The framework provides an API for binding the controller to the PHY in the
>> +case of non dt boot.
>> +
>> +struct phy_bind *phy_bind(const char *dev_name, int index,
>> + const char *phy_dev_name);
>> +
>> +The API fills the phy_bind structure with the dev_name (device name of the
>> +controller), index and phy_dev_name (device name of the PHY). This will
>> +be used when the controller requests this phy. This API should be used by
>> +platform specific initialization code (board file).
>> +
>> +In the case of dt boot, the binding information should be added in the dt
>> +data of the controller.
>> +
>> +4. Getting a reference to the PHY
>> +
>> +Before the controller can make use of the PHY, it has to get a reference to
>> +it. This framework provides 6 APIs to get a reference to the PHY.
>> +
>> +struct phy *phy_get(struct device *dev, int index);
>> +struct phy *devm_phy_get(struct device *dev, int index);
>> +struct phy *of_phy_get(struct device *dev, const char *phandle, int index);
>> +struct phy *devm_of_phy_get(struct device *dev, const char *phandle, int index);
>> +struct phy *of_phy_get_byname(struct device *dev, const char *string);
>> +struct phy *devm_of_phy_get_byname(struct device *dev, const char *string);
>> +
>> +phy_get and devm_phy_get can be used to get the PHY in non-dt boot. This API
>> +uses the binding information added using the phy_bind API to find and return
>> +the appropriate PHY. The only difference between the two APIs is that
>> +devm_phy_get associates the device with the PHY using devres on successful PHY
>> +get. On driver detach, release function is invoked on the the devres data and
>> +devres data is freed.
>> +
>> +of_phy_get and devm_of_phy_get can be used to get the PHY in dt boot. These
>> +APIs take the phandle and index to get a reference to the PHY. The only
>> +difference between the two APIs is that devm_of_phy_get associates the device
>> +with the PHY using devres on successful phy get. On driver detach, release
>> +function is invoked on the devres data and it is freed.
>> +
>> +of_phy_get_byname and devm_of_phy_get_byname can also be used to get the PHY
>> +in dt boot. It is same as the above API except that the user has to pass the
>> +phy name as filled in "phy-names" phandle in dt data and the framework will
>> +find the index and get the PHY.
>> +
>> +5. Releasing a reference to the PHY
>> +
>> +When the controller no longer needs the PHY, it has to release the reference
>> +to the PHY it has obtained using the APIs mentioned in the above section. The
>> +PHY framework provides 2 APIS to release a reference to the PHY.
>> +
>> +void phy_put(struct phy *phy);
>> +void devm_phy_put(struct device *dev, struct phy *phy);
>> +
>> +Both these APIs are used to release a reference to the PHY and devm_phy_put
>> +destroys the devres associated with this PHY.
>> +
>> +6. Destroying the PHY
>> +
>> +When the driver that created the PHY is unloaded, it should destroy the PHY it
>> +created using one of the following 2 APIs.
>> +
>> +void phy_destroy(struct phy *phy);
>> +void devm_phy_destroy(struct device *dev, struct phy *phy);
>> +
>> +Both these APIs destroys the PHY and devm_phy_destroy destroys the devres
>> +associated with this PHY.
>> +
>> +7. DeviceTree Binding
>> +
>> +The documentation for PHY dt binding can be found @
>> +Documentation/devicetree/bindings/phy/phy-bindings.txt
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 72b0843..f2674e7 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -3474,6 +3474,13 @@ S: Maintained
>> F: include/asm-generic
>> F: include/uapi/asm-generic
>>
>> +GENERIC PHY FRAMEWORK
>> +M: Kishon Vijay Abraham I <kishon-l0cyMroinI0@public.gmane.org>
>> +L: linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
>> +S: Supported
>> +F: drivers/phy/
>> +F: include/linux/phy/
>> +
>> GENERIC UIO DRIVER FOR PCI DEVICES
>> M: "Michael S. Tsirkin" <mst-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>> L: kvm-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
>> diff --git a/drivers/Kconfig b/drivers/Kconfig
>> index 202fa6d..ad2c374a 100644
>> --- a/drivers/Kconfig
>> +++ b/drivers/Kconfig
>> @@ -162,4 +162,6 @@ source "drivers/irqchip/Kconfig"
>>
>> source "drivers/ipack/Kconfig"
>>
>> +source "drivers/phy/Kconfig"
>> +
>> endmenu
>> diff --git a/drivers/Makefile b/drivers/Makefile
>> index dce39a9..9da8321 100644
>> --- a/drivers/Makefile
>> +++ b/drivers/Makefile
>> @@ -45,6 +45,8 @@ obj-y += char/
>> # gpu/ comes after char for AGP vs DRM startup
>> obj-y += gpu/
>>
>> +obj-y += phy/
>> +
>> obj-$(CONFIG_CONNECTOR) += connector/
>>
>> # i810fb and intelfb depend on char/agp/
>> diff --git a/drivers/phy/Kconfig b/drivers/phy/Kconfig
>> new file mode 100644
>> index 0000000..5f85909
>> --- /dev/null
>> +++ b/drivers/phy/Kconfig
>> @@ -0,0 +1,13 @@
>> +#
>> +# PHY
>> +#
>> +
>> +menuconfig GENERIC_PHY
>> + tristate "PHY Subsystem"
>> + help
>> + Generic PHY support.
>> +
>> + This framework is designed to provide a generic interface for PHY
>> + devices present in the kernel. This layer will have the generic
>> + API by which phy drivers can create PHY using the phy framework and
>> + phy users can obtain reference to the PHY.
>> diff --git a/drivers/phy/Makefile b/drivers/phy/Makefile
>> new file mode 100644
>> index 0000000..9e9560f
>> --- /dev/null
>> +++ b/drivers/phy/Makefile
>> @@ -0,0 +1,5 @@
>> +#
>> +# Makefile for the phy drivers.
>> +#
>> +
>> +obj-$(CONFIG_GENERIC_PHY) += phy-core.o
>> diff --git a/drivers/phy/phy-core.c b/drivers/phy/phy-core.c
>> new file mode 100644
>> index 0000000..1d753f2
>> --- /dev/null
>> +++ b/drivers/phy/phy-core.c
>> @@ -0,0 +1,616 @@
>> +/*
>> + * phy-core.c -- Generic Phy framework.
>> + *
>> + * Copyright (C) 2013 Texas Instruments
>> + *
>> + * Author: Kishon Vijay Abraham I <kishon-l0cyMroinI0@public.gmane.org>
>> + *
>> + * This program is free software; you can redistribute it and/or modify it
>> + * under the terms of the GNU General Public License as published by the
>> + * Free Software Foundation; either version 2 of the License, or (at your
>> + * option) any later version.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>> + * GNU General Public License for more details.
>> + *
>> + * You should have received a copy of the GNU General Public License
>> + * along with this program. If not, see <http://www.gnu.org/licenses/>.
>> + */
>> +
>> +#include <linux/kernel.h>
>> +#include <linux/export.h>
>> +#include <linux/module.h>
>> +#include <linux/err.h>
>> +#include <linux/device.h>
>> +#include <linux/slab.h>
>> +#include <linux/of.h>
>> +#include <linux/phy/phy.h>
>> +
>> +static struct class *phy_class;
>> +static DEFINE_MUTEX(phy_bind_mutex);
>> +static LIST_HEAD(phy_bind_list);
>> +static int phy_core_init(void);
>> +
>> +static void devm_phy_release(struct device *dev, void *res)
>> +{
>> + struct phy *phy = *(struct phy **)res;
>> +
>> + phy_put(phy);
>> +}
>> +
>> +static void devm_phy_consume(struct device *dev, void *res)
>> +{
>> + struct phy *phy = *(struct phy **)res;
>> +
>> + phy_destroy(phy);
>> +}
>> +
>> +static int devm_phy_match(struct device *dev, void *res, void *match_data)
>> +{
>> + return res == match_data;
>> +}
>> +
>> +static struct phy *phy_lookup(struct device *dev, int index)
>> +{
>> + struct phy_bind *phy_bind = NULL;
>> +
>> + list_for_each_entry(phy_bind, &phy_bind_list, list) {
>> + if (!(strcmp(phy_bind->dev_name, dev_name(dev)) &&
>> + phy_bind->index == index)) {
>> + if (phy_bind->phy)
>> + return phy_bind->phy;
>> + else
>> + return ERR_PTR(-EPROBE_DEFER);
>> + }
>> + }
>> +
>> + return ERR_PTR(-ENODEV);
>> +}
>> +
>> +static struct phy *of_phy_lookup(struct device_node *node)
>> +{
>> + struct phy *phy;
>> + struct device *dev;
>> + struct class_dev_iter iter;
>> +
>> + class_dev_iter_init(&iter, phy_class, NULL, NULL);
>> + while ((dev = class_dev_iter_next(&iter))) {
>> + phy = container_of(dev, struct phy, dev);
>
> it would look a bit better if you provided a to_phy() macro. Specially
> since this container_of() repeats multiple times in this file.
hmm.. ok.
>
>> +/**
>> + * phy_put() - release the PHY
>> + * @phy: the phy returned by phy_get()
>> + *
>> + * Releases a refcount the caller received from phy_get().
>> + */
>> +void phy_put(struct phy *phy)
>> +{
>
> I would rather:
>
> if (WARN(IS_ERR(phy), "invalid parameter\n"))
> return;
>
> module_put(phy->ops->owner);
> put_device(&phy->dev);
>
> that way we can catch users passing bogus pointers here. When PHY layer
> is disabled, you want to make this is no-op with a static inline in a
> header anyway.
yeah. Have that no-op in header file.
>
>> +struct phy *of_phy_xlate(struct phy *phy, struct of_phandle_args *args)
>> +{
>> + return phy;
>> +}
>> +EXPORT_SYMBOL_GPL(of_phy_xlate);
>
> so you get a PHY and just return it ? What gives ?? (maybe I skipped
> some of the discussion...)
hmm.. this is for the common case where the PHY provider implements only
one PHY. And both phy provider and phy_instance is represented by struct
phy *.
For the case where PHY provider implements multiple PHYs (here it will
have a single dt node), the PHY provider will implement it's own version
of of_xlate that takes *of_phandle_args* as argument and finds the
appropriate PHY.
>
>> +struct phy *of_phy_get(struct device *dev, int index)
>> +{
>> + int ret;
>> + struct phy *phy = NULL;
>> + struct phy_bind *phy_map = NULL;
>> + struct of_phandle_args args;
>> + struct device_node *node;
>> +
>> + if (!dev->of_node) {
>> + dev_dbg(dev, "device does not have a device node entry\n");
>> + return ERR_PTR(-EINVAL);
>> + }
>> +
>> + ret = of_parse_phandle_with_args(dev->of_node, "phys", "#phy-cells",
>> + index, &args);
>> + if (ret) {
>> + dev_dbg(dev, "failed to get phy in %s node\n",
>> + dev->of_node->full_name);
>> + return ERR_PTR(-ENODEV);
>> + }
>> +
>> + phy = of_phy_lookup(args.np);
>> + if (IS_ERR(phy) || !try_module_get(phy->ops->owner)) {
>> + phy = ERR_PTR(-EPROBE_DEFER);
>> + goto err0;
>> + }
>> +
>> + phy = phy->ops->of_xlate(phy, &args);
>
> alright, so of_xlate() is optional, am I right ? How about not
Not really. of_xlate is mandatory (it's even checked in phy_create).
Either the PHY provider can implement it's own version or use the
implementation above (by filling the function pointer).
> implementing the above and have a check for of_xlate() being a valid
> pointer here ?
Having the way it is actually mandates the PHY providers to always
provide of_xlate which IMO is better since some PHY providers wont
accidentally be using the default implementation.
>
>> +struct phy *phy_create(struct device *dev, const char *label,
>> + struct device_node *of_node, int type, struct phy_ops *ops,
>> + void *priv)
>> +{
>> + int ret;
>> + struct phy *phy;
>> + struct phy_bind *phy_bind;
>> + const char *devname = NULL;
>> +
>> + if (!dev) {
>> + dev_err(dev, "no device provided for PHY\n");
>
> I'd call this a WARN() or am I too pedantic? :-p
>
>> + if (!ops || !ops->of_xlate || !priv) {
>> + dev_err(dev, "no PHY ops/PHY data provided\n");
>
> likewise here.
hmm.. ok.
>
>> + ret = -EINVAL;
>> + goto err0;
>> + }
>> +
>> + if (!phy_class)
>> + phy_core_init();
>
> why don't you setup the class on module_init ? Then this would be a
> terrible error condition here :-)
This is for the case where the PHY driver gets loaded before the PHY
framework. I could have returned EPROBE_DEFER here instead I thought
will have it this way.
>
>> +static struct device_attribute phy_dev_attrs[] = {
>> + __ATTR(label, 0444, phy_name_show, NULL),
>> + __ATTR(phy_bind, 0444, phy_bind_show, NULL),
>
> you could expose a human-readable 'type' string. BTW, how are you using
> type ? USB2/USB3/etc ? Have you considered our OMAP5 SerDes pair which
Actually not using *type* anywhere. Just used as an additional info
about the PHY. It's actually not even enum. Maybe I can remove it
completely.
> are currently for USB3 and SATA (and could just as easily be used for
> PCIe)
Yeah. Me and Balaji were planning to work on it for having a single
driver to be used for all the above.
>
>> +static void phy_release(struct device *dev)
>> +{
>> + struct phy *phy;
>> +
>> + phy = container_of(dev, struct phy, dev);
>> + dev_dbg(dev, "releasing '%s'\n", dev_name(dev));
>
> how about dev_vdbg() ? I doubt anyone will be waiting for this
> message... Just a thought
>
>> +static int phy_core_init(void)
>> +{
>> + if (phy_class)
>> + return 0;
>
> Weird.. if you initialize the class here, why do you need to initialize
> it during phy_create() ?
>
> What's going on ? Also, module_init() will only be called once, why this
> if (phy_class) check ?
er.. for the case where phy driver is loaded before this PHY framework,
phy_create would have already called phy_core_init to create the
phy_class. So module_init() is not needed at all since we have already
created the phy_class. I think this looks a bit hacky.
Either we can have EPROBE_DEFER in phy_create or have this module as
subsys_initcall() like I had it before. I would actually prefer it to be
subsys_initcall().
Thanks
Kishon
^ permalink raw reply
* Re: [Suggestion] ISDN: isdnloop: C grammar issue, '}' miss match 'if' and 'switch' statement.
From: Michal Kubecek @ 2013-04-03 14:08 UTC (permalink / raw)
To: Chen Gang
Cc: fengguang.wu, isdn, Linus Torvalds, David Miller, netdev,
Joe Perches
In-Reply-To: <515C303B.1040304@asianux.com>
On Wed, Apr 03, 2013 at 09:35:55PM +0800, Chen Gang wrote:
>
> in drivers/isdn/isdnloop/isdnloop.c
>
> issue description:
> it is in function 'isdnloop_command'.
> it seems a C grammar issue for '}' miss match 'if' and 'switch' statement
> please check the line 1243, 1265, 1341.
>
> building:
> make allyesconfig, can not let it built.
> in menuconfig, we (at least for me) can not let ISDN_DRV_LOOP = 'y' or 'm'.
> is this module a waste module which should be deleted ?
>
> the related commit:
> commit 1da177e4c3f41524e886b7f1b8a0c1fc7321cac2
> Author: Linus Torvalds <torvalds@ppc970.osdl.org>
> Date: Sat Apr 16 15:20:36 2005 -0700
As far as I can see, this rather comes from
commit 475be4d85a274d0961593db41cf85689db1d583c
Author: Joe Perches <joe@perches.com>
Date: Sun Feb 19 19:52:38 2012 -0800
isdn: whitespace coding style cleanup
> 1240 case ISDN_CMD_ACCEPTB:
> 1241 if (!(card->flags & ISDNLOOP_FLAGS_RUNNING))
> 1242 return -ENODEV;
> 1243 if (c->arg < ISDNLOOP_BCH) {
> 1244 a = c->arg + 1;
> 1245 switch (card->l2_proto[a - 1]) {
> 1246 case ISDN_PROTO_L2_X75I:
> 1247 sprintf(cbuf, "%02d;BCON_R,BX75\n", (int) a);
> 1248 break;
> 1249 #ifdef CONFIG_ISDN_X25
> 1250 case ISDN_PROTO_L2_X25DTE:
> 1251 sprintf(cbuf, "%02d;BCON_R,BX2T\n", (int) a);
> 1252 break;
> 1253 case ISDN_PROTO_L2_X25DCE:
> 1254 sprintf(cbuf, "%02d;BCON_R,BX2C\n", (int) a);
> 1255 break;
> 1256 #endif
> 1257 case ISDN_PROTO_L2_HDLC:
> 1258 sprintf(cbuf, "%02d;BCON_R,BTRA\n", (int) a);
> 1259 break;
> 1260 default:
> 1261 sprintf(cbuf, "%02d;BCON_R\n", (int) a);
> 1262 }
> 1263 printk(KERN_DEBUG "isdnloop writecmd '%s'\n", cbuf);
> 1264 i = isdnloop_writecmd(cbuf, strlen(cbuf), 0, card);
> 1265 break;
> 1266 case ISDN_CMD_HANGUP:
Michal Kubecek
^ permalink raw reply
* Re: [net-next.git 2/7] stmmac: review barriers
From: Sergei Shtylyov @ 2013-04-03 14:02 UTC (permalink / raw)
To: Giuseppe CAVALLARO; +Cc: netdev, Deepak Sikri, Shiraz Hashim
In-Reply-To: <1364967689-11155-2-git-send-email-peppe.cavallaro@st.com>
Hello.
On 03-04-2013 9:41, Giuseppe CAVALLARO wrote:
> In all my tests performed on SH4 and ARM A9 platforms, I've never met problems
> that can be fixed by using memory barriers. In the past there was some issues
> on SMP ARM but fixed by reviewing xmit spinlock.
> Further barriers have been added in the commits too: 8e83989106562326bf
Please also specify that commit's summary line in parens.
> This patch is to use the smp_wbm instead of wbm because the driver
It's "wmb".
> runs on UP systems. Then, IMO it could make sense to only maintain the barriers
> just in places where we touch the dma owner bits (that is the
> only real critical path as we had seen and fixed in the commit:
> eb0dc4bb2e22c04964d).
This one's too.
> Signed-off-by: Giuseppe Cavallaro <peppe.cavallaro@st.com>
> Cc: Deepak Sikri <deepak.sikri@st.com>
> Cc: Shiraz Hashim <shiraz.hashim@st.com>
WBR, Sergei
^ permalink raw reply
* [PATCH v2 1/4] mac802154: Do not try to resend failed packets
From: Alan Ott @ 2013-04-03 14:00 UTC (permalink / raw)
To: Alexander Smirnov, Dmitry Eremin-Solenikov, David S. Miller
Cc: linux-zigbee-devel, netdev, linux-kernel, Alan Ott
In-Reply-To: <1364997658-16498-1-git-send-email-alan@signal11.us>
When ops->xmit() fails, drop the packet. Devices which support hardware
ack and retry (which include all devices currently supported by mainline),
will automatically retry sending the packet (in the hardware) up to 3
times, per the 802.15.4 spec. There is no need, and it is incorrect to
try to do it in mac802154.
Signed-off-by: Alan Ott <alan@signal11.us>
---
net/mac802154/mac802154.h | 2 --
net/mac802154/tx.c | 12 ++----------
2 files changed, 2 insertions(+), 12 deletions(-)
diff --git a/net/mac802154/mac802154.h b/net/mac802154/mac802154.h
index 21fa386..5c9e021 100644
--- a/net/mac802154/mac802154.h
+++ b/net/mac802154/mac802154.h
@@ -88,8 +88,6 @@ struct mac802154_sub_if_data {
#define mac802154_to_priv(_hw) container_of(_hw, struct mac802154_priv, hw)
-#define MAC802154_MAX_XMIT_ATTEMPTS 3
-
#define MAC802154_CHAN_NONE (~(u8)0) /* No channel is assigned */
extern struct ieee802154_reduced_mlme_ops mac802154_mlme_reduced;
diff --git a/net/mac802154/tx.c b/net/mac802154/tx.c
index 4e09d07..7264874 100644
--- a/net/mac802154/tx.c
+++ b/net/mac802154/tx.c
@@ -39,7 +39,6 @@ struct xmit_work {
struct mac802154_priv *priv;
u8 chan;
u8 page;
- u8 xmit_attempts;
};
static void mac802154_xmit_worker(struct work_struct *work)
@@ -60,18 +59,12 @@ static void mac802154_xmit_worker(struct work_struct *work)
}
res = xw->priv->ops->xmit(&xw->priv->hw, xw->skb);
+ if (res)
+ pr_debug("transmission failed\n");
out:
mutex_unlock(&xw->priv->phy->pib_lock);
- if (res) {
- if (xw->xmit_attempts++ < MAC802154_MAX_XMIT_ATTEMPTS) {
- queue_work(xw->priv->dev_workqueue, &xw->work);
- return;
- } else
- pr_debug("transmission failed for %d times",
- MAC802154_MAX_XMIT_ATTEMPTS);
- }
dev_kfree_skb(xw->skb);
@@ -114,7 +107,6 @@ netdev_tx_t mac802154_tx(struct mac802154_priv *priv, struct sk_buff *skb,
work->priv = priv;
work->page = page;
work->chan = chan;
- work->xmit_attempts = 0;
queue_work(priv->dev_workqueue, &work->work);
--
1.7.11.2
^ 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