* Re: [PATCH v4 26/42] vhost/net: force len for TX to host endian
From: Michael S. Tsirkin @ 2014-11-26 14:44 UTC (permalink / raw)
To: Cornelia Huck
Cc: kvm, rusty, netdev, linux-kernel, virtualization, pbonzini,
David Miller
In-Reply-To: <20141126153102.3f5b8fc9.cornelia.huck@de.ibm.com>
On Wed, Nov 26, 2014 at 03:31:02PM +0100, Cornelia Huck wrote:
> On Tue, 25 Nov 2014 18:43:14 +0200
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
>
> > We use native endian-ness internally but never
> > expose it to guest.
> >
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > ---
> > drivers/vhost/net.c | 10 +++++-----
> > 1 file changed, 5 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
> > index 8dae2f7..dce5c58 100644
> > --- a/drivers/vhost/net.c
> > +++ b/drivers/vhost/net.c
> > @@ -48,15 +48,15 @@ MODULE_PARM_DESC(experimental_zcopytx, "Enable Zero Copy TX;"
> > * status internally; used for zerocopy tx only.
> > */
> > /* Lower device DMA failed */
> > -#define VHOST_DMA_FAILED_LEN 3
> > +#define VHOST_DMA_FAILED_LEN ((__force __virtio32)3)
> > /* Lower device DMA done */
> > -#define VHOST_DMA_DONE_LEN 2
> > +#define VHOST_DMA_DONE_LEN ((__force __virtio32)2)
> > /* Lower device DMA in progress */
> > -#define VHOST_DMA_IN_PROGRESS 1
> > +#define VHOST_DMA_IN_PROGRESS ((__force __virtio32)1)
> > /* Buffer unused */
> > -#define VHOST_DMA_CLEAR_LEN 0
> > +#define VHOST_DMA_CLEAR_LEN ((__force __virtio32)0)
>
> I find these constants a bit confusing: What does __virtio32 mean
> without the context of a vq or device?
>
> >
> > -#define VHOST_DMA_IS_DONE(len) ((len) >= VHOST_DMA_DONE_LEN)
> > +#define VHOST_DMA_IS_DONE(len) ((__force u32)(len) >= (__force u32)VHOST_DMA_DONE_LEN)
>
> And here you cast it to a plain u32 again.
>
> I looked at the final code, and you seem either to use the above
> constants for .len or do a cpu_to_vhost32(). Wouldn't you need to
> convert the constants as well?
I tried to explain it in the commit message.
It's a hack in vhost: it keeps virtio used structure in host
memory, but abuses length field for internal housekeeping.
This works because length in used ring for tx is always 0.
> >
> > enum {
> > VHOST_NET_FEATURES = VHOST_FEATURES |
^ permalink raw reply
* Re: [PATCH v4 26/42] vhost/net: force len for TX to host endian
From: Cornelia Huck @ 2014-11-26 14:54 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: kvm, rusty, netdev, linux-kernel, virtualization, pbonzini,
David Miller
In-Reply-To: <20141126144400.GA8086@redhat.com>
On Wed, 26 Nov 2014 16:44:00 +0200
"Michael S. Tsirkin" <mst@redhat.com> wrote:
> On Wed, Nov 26, 2014 at 03:31:02PM +0100, Cornelia Huck wrote:
> > On Tue, 25 Nov 2014 18:43:14 +0200
> > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> >
> > > We use native endian-ness internally but never
> > > expose it to guest.
> > >
> > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > > ---
> > > drivers/vhost/net.c | 10 +++++-----
> > > 1 file changed, 5 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
> > > index 8dae2f7..dce5c58 100644
> > > --- a/drivers/vhost/net.c
> > > +++ b/drivers/vhost/net.c
> > > @@ -48,15 +48,15 @@ MODULE_PARM_DESC(experimental_zcopytx, "Enable Zero Copy TX;"
> > > * status internally; used for zerocopy tx only.
> > > */
> > > /* Lower device DMA failed */
> > > -#define VHOST_DMA_FAILED_LEN 3
> > > +#define VHOST_DMA_FAILED_LEN ((__force __virtio32)3)
> > > /* Lower device DMA done */
> > > -#define VHOST_DMA_DONE_LEN 2
> > > +#define VHOST_DMA_DONE_LEN ((__force __virtio32)2)
> > > /* Lower device DMA in progress */
> > > -#define VHOST_DMA_IN_PROGRESS 1
> > > +#define VHOST_DMA_IN_PROGRESS ((__force __virtio32)1)
> > > /* Buffer unused */
> > > -#define VHOST_DMA_CLEAR_LEN 0
> > > +#define VHOST_DMA_CLEAR_LEN ((__force __virtio32)0)
> >
> > I find these constants a bit confusing: What does __virtio32 mean
> > without the context of a vq or device?
> >
> > >
> > > -#define VHOST_DMA_IS_DONE(len) ((len) >= VHOST_DMA_DONE_LEN)
> > > +#define VHOST_DMA_IS_DONE(len) ((__force u32)(len) >= (__force u32)VHOST_DMA_DONE_LEN)
> >
> > And here you cast it to a plain u32 again.
> >
> > I looked at the final code, and you seem either to use the above
> > constants for .len or do a cpu_to_vhost32(). Wouldn't you need to
> > convert the constants as well?
>
> I tried to explain it in the commit message.
> It's a hack in vhost: it keeps virtio used structure in host
> memory, but abuses length field for internal housekeeping.
> This works because length in used ring for tx is always 0.
Ah, ok. It might make sense to add this explanation to the patch :)
^ permalink raw reply
* Re: [PATCH v4 26/42] vhost/net: force len for TX to host endian
From: Michael S. Tsirkin @ 2014-11-26 15:01 UTC (permalink / raw)
To: Cornelia Huck
Cc: kvm, rusty, netdev, linux-kernel, virtualization, pbonzini,
David Miller
In-Reply-To: <20141126155440.4f49b70f.cornelia.huck@de.ibm.com>
On Wed, Nov 26, 2014 at 03:54:40PM +0100, Cornelia Huck wrote:
> On Wed, 26 Nov 2014 16:44:00 +0200
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
>
> > On Wed, Nov 26, 2014 at 03:31:02PM +0100, Cornelia Huck wrote:
> > > On Tue, 25 Nov 2014 18:43:14 +0200
> > > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > >
> > > > We use native endian-ness internally but never
> > > > expose it to guest.
> > > >
> > > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > > > ---
> > > > drivers/vhost/net.c | 10 +++++-----
> > > > 1 file changed, 5 insertions(+), 5 deletions(-)
> > > >
> > > > diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
> > > > index 8dae2f7..dce5c58 100644
> > > > --- a/drivers/vhost/net.c
> > > > +++ b/drivers/vhost/net.c
> > > > @@ -48,15 +48,15 @@ MODULE_PARM_DESC(experimental_zcopytx, "Enable Zero Copy TX;"
> > > > * status internally; used for zerocopy tx only.
> > > > */
> > > > /* Lower device DMA failed */
> > > > -#define VHOST_DMA_FAILED_LEN 3
> > > > +#define VHOST_DMA_FAILED_LEN ((__force __virtio32)3)
> > > > /* Lower device DMA done */
> > > > -#define VHOST_DMA_DONE_LEN 2
> > > > +#define VHOST_DMA_DONE_LEN ((__force __virtio32)2)
> > > > /* Lower device DMA in progress */
> > > > -#define VHOST_DMA_IN_PROGRESS 1
> > > > +#define VHOST_DMA_IN_PROGRESS ((__force __virtio32)1)
> > > > /* Buffer unused */
> > > > -#define VHOST_DMA_CLEAR_LEN 0
> > > > +#define VHOST_DMA_CLEAR_LEN ((__force __virtio32)0)
> > >
> > > I find these constants a bit confusing: What does __virtio32 mean
> > > without the context of a vq or device?
> > >
> > > >
> > > > -#define VHOST_DMA_IS_DONE(len) ((len) >= VHOST_DMA_DONE_LEN)
> > > > +#define VHOST_DMA_IS_DONE(len) ((__force u32)(len) >= (__force u32)VHOST_DMA_DONE_LEN)
> > >
> > > And here you cast it to a plain u32 again.
> > >
> > > I looked at the final code, and you seem either to use the above
> > > constants for .len or do a cpu_to_vhost32(). Wouldn't you need to
> > > convert the constants as well?
> >
> > I tried to explain it in the commit message.
> > It's a hack in vhost: it keeps virtio used structure in host
> > memory, but abuses length field for internal housekeeping.
> > This works because length in used ring for tx is always 0.
>
> Ah, ok. It might make sense to add this explanation to the patch :)
Absolutely.
^ permalink raw reply
* [PATCH RFC net-next] net: Add GRO support for GRE tunneling of TEB packets
From: Or Gerlitz @ 2014-11-26 15:08 UTC (permalink / raw)
To: David S. Miller; +Cc: netdev, therbert, edumazet, Or Gerlitz, H.K. Jerry Chu
Add the missing parts in the gre gro handlers when the inner protocol
is ETH_P_TEB which is the case for OVS based GRE tunneling.
Cc: H.K. Jerry Chu <hkchu@google.com>
Signed-off-by: Or Gerlitz <ogerlitz@mellanox.com>
---
net/ipv4/gre_offload.c | 57 +++++++++++++++++++++++++++++++++++++----------
1 files changed, 45 insertions(+), 12 deletions(-)
diff --git a/net/ipv4/gre_offload.c b/net/ipv4/gre_offload.c
index bb5947b..06ae197 100644
--- a/net/ipv4/gre_offload.c
+++ b/net/ipv4/gre_offload.c
@@ -14,6 +14,7 @@
#include <linux/init.h>
#include <net/protocol.h>
#include <net/gre.h>
+#include <linux/etherdevice.h>
static struct sk_buff *gre_gso_segment(struct sk_buff *skb,
netdev_features_t features)
@@ -121,8 +122,9 @@ static struct sk_buff **gre_gro_receive(struct sk_buff **head,
struct sk_buff **pp = NULL;
struct sk_buff *p;
const struct gre_base_hdr *greh;
+ struct ethhdr *eh = NULL;
unsigned int hlen, grehlen;
- unsigned int off;
+ unsigned int off, off_eth = 0;
int flush = 1;
struct packet_offload *ptype;
__be16 type;
@@ -147,11 +149,6 @@ static struct sk_buff **gre_gro_receive(struct sk_buff **head,
type = greh->protocol;
- rcu_read_lock();
- ptype = gro_find_receive_by_type(type);
- if (ptype == NULL)
- goto out_unlock;
-
grehlen = GRE_HEADER_SECTION;
if (greh->flags & GRE_KEY)
@@ -164,22 +161,45 @@ static struct sk_buff **gre_gro_receive(struct sk_buff **head,
if (skb_gro_header_hard(skb, hlen)) {
greh = skb_gro_header_slow(skb, hlen, off);
if (unlikely(!greh))
- goto out_unlock;
+ goto out;
}
/* Don't bother verifying checksum if we're going to flush anyway. */
if ((greh->flags & GRE_CSUM) && !NAPI_GRO_CB(skb)->flush) {
if (skb_gro_checksum_simple_validate(skb))
- goto out_unlock;
+ goto out;
skb_gro_checksum_try_convert(skb, IPPROTO_GRE, 0,
null_compute_pseudo);
}
+ skb_gro_pull(skb, grehlen);
+
+ /* Adjusted NAPI_GRO_CB(skb)->csum after skb_gro_pull()*/
+ skb_gro_postpull_rcsum(skb, greh, grehlen);
+
+ if (type == ntohs(ETH_P_TEB)) {
+ off_eth = skb_gro_offset(skb);
+ hlen = off_eth + sizeof(*eh);
+ eh = skb_gro_header_fast(skb, off_eth);
+ if (skb_gro_header_hard(skb, hlen)) {
+ eh = skb_gro_header_slow(skb, hlen, off_eth);
+ if (unlikely(!eh))
+ goto out;
+ }
+ type = eh->h_proto;
+ }
+
+ rcu_read_lock();
+ ptype = gro_find_receive_by_type(type);
+ if (ptype == NULL)
+ goto out_unlock;
+
flush = 0;
for (p = *head; p; p = p->next) {
const struct gre_base_hdr *greh2;
+ const struct ethhdr *eh2;
if (!NAPI_GRO_CB(p)->same_flow)
continue;
@@ -205,13 +225,19 @@ static struct sk_buff **gre_gro_receive(struct sk_buff **head,
NAPI_GRO_CB(p)->same_flow = 0;
continue;
}
+
+ eh2 = (struct ethhdr *)(p->data + off_eth);
+ if (eh && compare_ether_header(eh, eh2)) {
+ NAPI_GRO_CB(p)->same_flow = 0;
+ continue;
+ }
}
}
- skb_gro_pull(skb, grehlen);
-
- /* Adjusted NAPI_GRO_CB(skb)->csum after skb_gro_pull()*/
- skb_gro_postpull_rcsum(skb, greh, grehlen);
+ if (eh) {
+ skb_gro_pull(skb, sizeof(*eh)); /* pull inner eth header */
+ skb_gro_postpull_rcsum(skb, eh, sizeof(*eh));
+ }
pp = ptype->callbacks.gro_receive(head, skb);
@@ -229,6 +255,7 @@ static int gre_gro_complete(struct sk_buff *skb, int nhoff)
struct packet_offload *ptype;
unsigned int grehlen = sizeof(*greh);
int err = -ENOENT;
+ struct ethhdr *eh;
__be16 type;
skb->encapsulation = 1;
@@ -241,6 +268,12 @@ static int gre_gro_complete(struct sk_buff *skb, int nhoff)
if (greh->flags & GRE_CSUM)
grehlen += GRE_HEADER_SECTION;
+ if (type == ntohs(ETH_P_TEB)) {
+ eh = (struct ethhdr *)(skb->data + nhoff + grehlen);
+ type = eh->h_proto;
+ grehlen += sizeof(*eh);
+ }
+
rcu_read_lock();
ptype = gro_find_complete_by_type(type);
if (ptype != NULL)
--
1.7.1
^ permalink raw reply related
* Re: [PATCH RFC net-next] net: Add GRO support for GRE tunneling of TEB packets
From: Tom Herbert @ 2014-11-26 15:44 UTC (permalink / raw)
To: Or Gerlitz
Cc: David S. Miller, Linux Netdev List, Eric Dumazet, H.K. Jerry Chu
In-Reply-To: <1417014504-5929-1-git-send-email-ogerlitz@mellanox.com>
On Wed, Nov 26, 2014 at 7:08 AM, Or Gerlitz <ogerlitz@mellanox.com> wrote:
> Add the missing parts in the gre gro handlers when the inner protocol
> is ETH_P_TEB which is the case for OVS based GRE tunneling.
>
> Cc: H.K. Jerry Chu <hkchu@google.com>
> Signed-off-by: Or Gerlitz <ogerlitz@mellanox.com>
> ---
> net/ipv4/gre_offload.c | 57 +++++++++++++++++++++++++++++++++++++----------
> 1 files changed, 45 insertions(+), 12 deletions(-)
>
> diff --git a/net/ipv4/gre_offload.c b/net/ipv4/gre_offload.c
> index bb5947b..06ae197 100644
> --- a/net/ipv4/gre_offload.c
> +++ b/net/ipv4/gre_offload.c
> @@ -14,6 +14,7 @@
> #include <linux/init.h>
> #include <net/protocol.h>
> #include <net/gre.h>
> +#include <linux/etherdevice.h>
>
> static struct sk_buff *gre_gso_segment(struct sk_buff *skb,
> netdev_features_t features)
> @@ -121,8 +122,9 @@ static struct sk_buff **gre_gro_receive(struct sk_buff **head,
> struct sk_buff **pp = NULL;
> struct sk_buff *p;
> const struct gre_base_hdr *greh;
> + struct ethhdr *eh = NULL;
> unsigned int hlen, grehlen;
> - unsigned int off;
> + unsigned int off, off_eth = 0;
> int flush = 1;
> struct packet_offload *ptype;
> __be16 type;
> @@ -147,11 +149,6 @@ static struct sk_buff **gre_gro_receive(struct sk_buff **head,
>
> type = greh->protocol;
>
> - rcu_read_lock();
> - ptype = gro_find_receive_by_type(type);
> - if (ptype == NULL)
> - goto out_unlock;
> -
> grehlen = GRE_HEADER_SECTION;
>
> if (greh->flags & GRE_KEY)
> @@ -164,22 +161,45 @@ static struct sk_buff **gre_gro_receive(struct sk_buff **head,
> if (skb_gro_header_hard(skb, hlen)) {
> greh = skb_gro_header_slow(skb, hlen, off);
> if (unlikely(!greh))
> - goto out_unlock;
> + goto out;
> }
>
> /* Don't bother verifying checksum if we're going to flush anyway. */
> if ((greh->flags & GRE_CSUM) && !NAPI_GRO_CB(skb)->flush) {
> if (skb_gro_checksum_simple_validate(skb))
> - goto out_unlock;
> + goto out;
>
> skb_gro_checksum_try_convert(skb, IPPROTO_GRE, 0,
> null_compute_pseudo);
> }
>
> + skb_gro_pull(skb, grehlen);
> +
> + /* Adjusted NAPI_GRO_CB(skb)->csum after skb_gro_pull()*/
> + skb_gro_postpull_rcsum(skb, greh, grehlen);
> +
> + if (type == ntohs(ETH_P_TEB)) {
> + off_eth = skb_gro_offset(skb);
> + hlen = off_eth + sizeof(*eh);
> + eh = skb_gro_header_fast(skb, off_eth);
> + if (skb_gro_header_hard(skb, hlen)) {
> + eh = skb_gro_header_slow(skb, hlen, off_eth);
> + if (unlikely(!eh))
> + goto out;
> + }
> + type = eh->h_proto;
I don't think this is the right approach. It would probably be better
to a add a gro_receive handler for ETH_P_TEB and then you wouldn't
need to modify GRE path with special case code. That would also be
applicable in geneve.
> + }
> +
> + rcu_read_lock();
> + ptype = gro_find_receive_by_type(type);
> + if (ptype == NULL)
> + goto out_unlock;
> +
> flush = 0;
>
> for (p = *head; p; p = p->next) {
> const struct gre_base_hdr *greh2;
> + const struct ethhdr *eh2;
>
> if (!NAPI_GRO_CB(p)->same_flow)
> continue;
> @@ -205,13 +225,19 @@ static struct sk_buff **gre_gro_receive(struct sk_buff **head,
> NAPI_GRO_CB(p)->same_flow = 0;
> continue;
> }
> +
> + eh2 = (struct ethhdr *)(p->data + off_eth);
> + if (eh && compare_ether_header(eh, eh2)) {
> + NAPI_GRO_CB(p)->same_flow = 0;
> + continue;
> + }
> }
> }
>
> - skb_gro_pull(skb, grehlen);
> -
> - /* Adjusted NAPI_GRO_CB(skb)->csum after skb_gro_pull()*/
> - skb_gro_postpull_rcsum(skb, greh, grehlen);
> + if (eh) {
> + skb_gro_pull(skb, sizeof(*eh)); /* pull inner eth header */
> + skb_gro_postpull_rcsum(skb, eh, sizeof(*eh));
> + }
>
> pp = ptype->callbacks.gro_receive(head, skb);
>
> @@ -229,6 +255,7 @@ static int gre_gro_complete(struct sk_buff *skb, int nhoff)
> struct packet_offload *ptype;
> unsigned int grehlen = sizeof(*greh);
> int err = -ENOENT;
> + struct ethhdr *eh;
> __be16 type;
>
> skb->encapsulation = 1;
> @@ -241,6 +268,12 @@ static int gre_gro_complete(struct sk_buff *skb, int nhoff)
> if (greh->flags & GRE_CSUM)
> grehlen += GRE_HEADER_SECTION;
>
> + if (type == ntohs(ETH_P_TEB)) {
> + eh = (struct ethhdr *)(skb->data + nhoff + grehlen);
> + type = eh->h_proto;
> + grehlen += sizeof(*eh);
> + }
> +
> rcu_read_lock();
> ptype = gro_find_complete_by_type(type);
> if (ptype != NULL)
> --
> 1.7.1
>
^ permalink raw reply
* Re: [PATCH v5 2/4] arch: Add lightweight memory barriers dma_rmb() and dma_wmb()
From: Will Deacon @ 2014-11-26 16:04 UTC (permalink / raw)
To: Alexander Duyck
Cc: linux-arch@vger.kernel.org, netdev@vger.kernel.org,
linux-kernel@vger.kernel.org, mathieu.desnoyers@polymtl.ca,
peterz@infradead.org, benh@kernel.crashing.org,
heiko.carstens@de.ibm.com, mingo@kernel.org, mikey@neuling.org,
linux@arm.linux.org.uk, donald.c.skidmore@intel.com,
matthew.vick@intel.com, geert@linux-m68k.org,
jeffrey.t.kirsher@intel.com, romieu@fr.zoreil.com,
paulmck@linux.vnet.ibm.com,
"nic_swsd@realtek.com" <nic_
In-Reply-To: <5474ADB4.5070200@redhat.com>
On Tue, Nov 25, 2014 at 04:26:28PM +0000, Alexander Duyck wrote:
> On 11/25/2014 06:01 AM, Will Deacon wrote:
> > If we ever see platforms using Linux/dma_alloc_coherent with devices
> > mastering from a different outer-shareable domain that the one containing
> > the CPUs, then we'll need to revisit this.
>
> Would we just need a system wide memory barrier in that case instead of
> an outer shareable memory barrier, or would we need to look as something
> like a sync barrier?
I think dmb(sy) would do the trick, but let's cross that bridge if/when we
have to.
Will
^ permalink raw reply
* Re: [patch net-next v3 04/17] net: introduce generic switch devices support
From: Thomas Graf @ 2014-11-26 16:08 UTC (permalink / raw)
To: Jamal Hadi Salim
Cc: Scott Feldman, Jiri Pirko, Netdev, David S. Miller,
nhorman@tuxdriver.com, Andy Gospodarek, dborkman@redhat.com,
ogerlitz@mellanox.com, jesse@nicira.com, pshelar@nicira.com,
azhou@nicira.com, ben@decadent.org.uk, stephen@networkplumber.org,
Kirsher, Jeffrey T, vyasevic@redhat.com, Cong Wang,
Fastabend, John R, Eric Dumazet, Florian Fainelli, Roopa Prabhu,
John Linville
In-Reply-To: <5475BB53.3070200@mojatatu.com>
On 11/26/14 at 06:36am, Jamal Hadi Salim wrote:
> On 11/25/14 23:18, Scott Feldman wrote:
> >On Tue, Nov 25, 2014 at 5:33 PM, Jamal Hadi Salim <jhs@mojatatu.com> wrote:
>
> >
> >You have a pointer to the kernel driver for that HW?
>
> I wasnt sure if that was a passive aggressive move there to
> question what i am claiming?(Only Canadians are allowed to be
> passive aggressive Scott). To answer your question, no
> code currently littered with vendor SDK unfortunately (as you
> would know!).
> But hopefully if we get these changes in correctly it would
> not be hard to show the driver working fully in the kernel.
> There are definetely a few other pieces of hardware that are
> making me come back here and invest time and effort in these
> long discussions.
>
> >Can you show how
> >you're using Linux tc netlink msg in kernel to program HW? I'd like
> >to see the in-kernel API.
> >
>
> Lets do the L2/port thing first. But yes, I am using Linux tc in
> kernel.
Jamal,
What is irriating in this context is that you are pushing back on
Jiri and others while referring to properitary and closed code which
you are unwilling or unable to share. I don't see this as being
passive aggressive, everybody is treated the same way in this regard.
It is exactly the point of this API and related discussions to
decouple the control plane (tc) from any vendor specifics while
allowing them to innovate, compete, and solve different use cases.
I think it's absolutely the right thing to write the API against
code that is public, which in this case is rocker and the existing
in-kernel NIC drivers.
^ permalink raw reply
* Re: [PATCH] x86: bpf_jit_comp: simplify trivial boolean return
From: Alexei Starovoitov @ 2014-11-26 16:42 UTC (permalink / raw)
To: Quentin Lambert
Cc: David S. Miller, Alexey Kuznetsov, James Morris,
Hideaki YOSHIFUJI, Patrick McHardy, Thomas Gleixner, Ingo Molnar,
H. Peter Anvin, x86, netdev@vger.kernel.org,
linux-kernel@vger.kernel.org
On Wed, Nov 26, 2014 at 1:18 AM, Quentin Lambert
<lambert.quentin@gmail.com> wrote:
> Remove if then else statements preceding
> boolean return. Occurences were found using
> Coccinelle.
>
> The semantic patch used was:
>
> @@
> expression expr;
> @@
>
>
> - if ( expr )
> - return true;
> - else
> - return false;
> + return expr;
>
> Signed-off-by: Quentin Lambert <lambert.quentin@gmail.com>
>
> ---
> arch/x86/net/bpf_jit_comp.c | 8 +++-----
> 1 file changed, 3 insertions(+), 5 deletions(-)
>
> diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c
> index 3f62734..1542f39 100644
> --- a/arch/x86/net/bpf_jit_comp.c
> +++ b/arch/x86/net/bpf_jit_comp.c
> @@ -135,11 +135,9 @@ static const int reg2hex[] = {
> */
> static inline bool is_ereg(u32 reg)
> {
> - if (reg == BPF_REG_5 || reg == AUX_REG ||
> - (reg >= BPF_REG_7 && reg <= BPF_REG_9))
> - return true;
> - else
> - return false;
> + return (reg == BPF_REG_5 ||
> + reg == AUX_REG ||
> + (reg >= BPF_REG_7 && reg <= BPF_REG_9));
please remove extra () around the whole expression, and
align in properly, and
don't move reg==AUX_REG check to a different line.
Subject is not warranted. I don't think it's a simplification.
imo existing code is fine and I don't think the time spent
reviewing such changes is worth it when there is no
improvement in readability.
^ permalink raw reply
* Re: [PATCH 2/5] net: Validate IFLA_BRIDGE_MODE attribute length
From: John Fastabend @ 2014-11-26 16:42 UTC (permalink / raw)
To: Thomas Graf; +Cc: davem, stephen, netdev, Ajit Khaparde, John Fastabend
In-Reply-To: <4a88a0350064b5c2ec4e2adcef5afdfcab3e45dd.1417005245.git.tgraf@suug.ch>
On 11/26/2014 04:42 AM, Thomas Graf wrote:
> Payload is currently accessed blindly and may exceed valid message
> boundaries.
>
> Fixes: a77dcb8c8 ("be2net: set and query VEB/VEPA mode of the PF interface")
> Fixes: 815cccbf1 ("ixgbe: add setlink, getlink support to ixgbe and ixgbevf")
> Cc: Ajit Khaparde <ajit.khaparde@emulex.com>
> Cc: John Fastabend <john.r.fastabend@intel.com>
> Signed-off-by: Thomas Graf <tgraf@suug.ch>
> ---
> drivers/net/ethernet/emulex/benet/be_main.c | 3 +++
> drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 3 +++
> 2 files changed, 6 insertions(+)
>
Thanks Thomas.
Acked-by: John Fastabend <john.r.fastabend@intel.com>
--
John Fastabend Intel Corporation
^ permalink raw reply
* Re: [PATCH net] r8152: drop the tx packet with invalid length
From: Eric Dumazet @ 2014-11-26 16:52 UTC (permalink / raw)
To: Hayes Wang; +Cc: netdev, nic_swsd, linux-kernel, linux-usb
In-Reply-To: <1394712342-15778-104-Taiwan-albertk@realtek.com>
On Wed, 2014-11-26 at 17:56 +0800, Hayes Wang wrote:
> Drop the tx packet which is more than the size of agg_buf_sz. When
> creating a bridge with the device, we may get the tx packet with
> TSO and the length is more than the gso_max_size which is set by
> the driver through netif_set_gso_max_size(). Such packets couldn't
> be transmitted and should be dropped directly.
>
> Signed-off-by: Hayes Wang <hayeswang@realtek.com>
> ---
> drivers/net/usb/r8152.c | 9 +++++++++
> 1 file changed, 9 insertions(+)
>
> diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
> index c6554c7..ebdaff7 100644
> --- a/drivers/net/usb/r8152.c
> +++ b/drivers/net/usb/r8152.c
> @@ -1897,6 +1897,15 @@ static netdev_tx_t rtl8152_start_xmit(struct sk_buff *skb,
> {
> struct r8152 *tp = netdev_priv(netdev);
>
> + if ((skb->len + sizeof(struct tx_desc)) > agg_buf_sz) {
> + struct net_device_stats *stats = &netdev->stats;
> +
> + dev_kfree_skb_any(skb);
> + stats->tx_dropped++;
> + WARN_ON_ONCE(1);
> + return NETDEV_TX_OK;
> + }
> +
> skb_tx_timestamp(skb);
>
> skb_queue_tail(&tp->tx_queue, skb);
Looks like a candidate for ndo_gso_check(), so that we do not drop, but
instead segment from netif_needs_gso()/validate_xmit_skb()
^ permalink raw reply
* Re: [PATCH] x86: bpf_jit_comp: simplify trivial boolean return
From: Joe Perches @ 2014-11-26 16:58 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: Quentin Lambert, David S. Miller, Alexey Kuznetsov, James Morris,
Hideaki YOSHIFUJI, Patrick McHardy, Thomas Gleixner, Ingo Molnar,
H. Peter Anvin, x86, netdev@vger.kernel.org,
linux-kernel@vger.kernel.org
In-Reply-To: <CAADnVQK8mn4ZHwbhBArosEw3Bmsc5vJfOzAeRE3R_1S7dbNfkg@mail.gmail.com>
On Wed, 2014-11-26 at 08:42 -0800, Alexei Starovoitov wrote:
> On Wed, Nov 26, 2014 at 1:18 AM, Quentin Lambert
> <lambert.quentin@gmail.com> wrote:
> > Remove if then else statements preceding
> > boolean return.
[]
> > diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c
[]
> > @@ -135,11 +135,9 @@ static const int reg2hex[] = {
> > */
> > static inline bool is_ereg(u32 reg)
> > {
> > - if (reg == BPF_REG_5 || reg == AUX_REG ||
> > - (reg >= BPF_REG_7 && reg <= BPF_REG_9))
> > - return true;
> > - else
> > - return false;
> > + return (reg == BPF_REG_5 ||
> > + reg == AUX_REG ||
> > + (reg >= BPF_REG_7 && reg <= BPF_REG_9));
>
> please remove extra () around the whole expression, and
> align in properly, and
> don't move reg==AUX_REG check to a different line.
> Subject is not warranted. I don't think it's a simplification.
It's not really a simplification,
gcc should emit the same object code.
> imo existing code is fine and I don't think the time spent
> reviewing such changes is worth it when there is no
> improvement in readability.
Is there any value in reordering these tests for frequency
or maybe using | instead of || to avoid multiple jumps?
^ permalink raw reply
* Re: [PATCH 0/5 net] bridge: Fix missing Netlink message validations
From: John Fastabend @ 2014-11-26 16:58 UTC (permalink / raw)
To: Thomas Graf, Jiri Pirko; +Cc: davem, stephen, netdev
In-Reply-To: <cover.1417005245.git.tgraf@suug.ch>
On 11/26/2014 04:42 AM, Thomas Graf wrote:
> Adds various missing length checks in the bridging code for Netlink
> messages and corresponding attributes provided by user space.
>
> Thomas Graf (5):
> bridge: Validate IFLA_BRIDGE_FLAGS attribute length
> net: Validate IFLA_BRIDGE_MODE attribute length
> net: Check for presence of IFLA_AF_SPEC
> bridge: Add missing policy entry for IFLA_BRPORT_FAST_LEAVE
> bridge: Sanitize IFLA_EXT_MASK for AF_BRIDGE:RTM_GETLINK
>
> drivers/net/ethernet/emulex/benet/be_main.c | 5 +++++
> drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 5 +++++
> net/bridge/br_netlink.c | 1 +
> net/core/rtnetlink.c | 23 ++++++++++++++++++-----
> 4 files changed, 29 insertions(+), 5 deletions(-)
>
+Jiri
Looks like a miss in bond_netlink also? Seems like writing
a smatch or cocci check for this would be worthwhile.
>
> diff --git a/drivers/net/bonding/bond_netlink.c b/drivers/net/bonding/bond_netlink.c
> index 3e6eebd..7b11243 100644
> --- a/drivers/net/bonding/bond_netlink.c
> +++ b/drivers/net/bonding/bond_netlink.c
> @@ -225,7 +225,12 @@ static int bond_changelink(struct net_device *bond_dev,
>
> bond_option_arp_ip_targets_clear(bond);
> nla_for_each_nested(attr, data[IFLA_BOND_ARP_IP_TARGET], rem) {
> - __be32 target = nla_get_be32(attr);
> + __be32 target;
> +
> + if (nla_len(attr) < sizeof(target))
> + return -EINVAL;
> +
> + target = nla_get_be32(attr);
>
> bond_opt_initval(&newval, (__force u64)target);
> err = __bond_opt_set(bond, BOND_OPT_ARP_TARGETS,
--
John Fastabend Intel Corporation
^ permalink raw reply
* Re: [PATCH net] net/mlx4_core: Limit count field to 24 bits in qp_alloc_res
From: David Miller @ 2014-11-26 17:05 UTC (permalink / raw)
To: ogerlitz; +Cc: netdev, matanb, amirv, jackm
In-Reply-To: <5475E19D.5000508@mellanox.com>
From: Or Gerlitz <ogerlitz@mellanox.com>
Date: Wed, 26 Nov 2014 16:20:13 +0200
> Currently, the upstream PF driver is wrongly NOT masking out these
> eight bits and as such, some VF drivers which are already setting
> them for optimized allocation are failing to allocate QPs as the
> count seen by the PF becomes way too large. The optimization is
> best effort anyway, and hence we can safely ignore their request.
Ok, thanks for explaining.
Applied and queued up for -stable, thanks.
^ permalink raw reply
* Re: [PATCH net-next v4] ipvlan: Initial check-in of the IPVLAN driver.
From: Mahesh Bandewar @ 2014-11-26 17:05 UTC (permalink / raw)
To: Toshiaki Makita
Cc: netdev, Eric Dumazet, Maciej Zenczykowski, Laurent Chavey,
Tim Hockin, David Miller, Brandon Philips, Pavel Emelianov
In-Reply-To: <54757631.8090604@lab.ntt.co.jp>
On Tue, Nov 25, 2014 at 10:41 PM, Toshiaki Makita
<makita.toshiaki@lab.ntt.co.jp> wrote:
> Hi Mahesh,
>
> I found that deleting the last ipvlan device triggers WARN_ON() in
> rtmsg_ifinfo().
> ipvlan_nl_fillinfo() seems to return -EINVAL in that case.
>
>> +static int ipvlan_nl_fillinfo(struct sk_buff *skb,
>> + const struct net_device *dev)
>> +{
>> + struct ipvl_dev *ipvlan = netdev_priv(dev);
>> + struct ipvl_port *port = ipvlan_port_get_rtnl(ipvlan->phy_dev);
>> + int ret = -EINVAL;
>> +
>> + if (!port)
>> + goto err;
>> +
>> + ret = -EMSGSIZE;
>> + if (nla_put_u16(skb, IFLA_IPVLAN_MODE, port->mode))
>> + goto err;
>> +
>> + return 0;
>> +
>> +err:
>> + return ret;
>> +}
>
> rollback_registered_many() calls rtmsg_ifinfo() after calling ndo_uninit().
> ndo_uninit() (ipvlan_uninit() -> ipvlan_port_destroy() ->
> netdev_rx_handler_unregister()) sets rx_handler_data into NULL.
> So, we cannot dereference "port" in ipvlan_nl_fillinfo().
>
Calling fillinfo() after calling uninit() seems pointless on any
device. But how are you hitting this case? Can you share the command
sequence with me?
Thanks,
--mahesh..
> Maybe "mode" should belong to struct ipvl_dev?
>
> Thanks,
> Toshiaki Makita
>
^ permalink raw reply
* Re: [PATCH 0/5 net] bridge: Fix missing Netlink message validations
From: Thomas Graf @ 2014-11-26 17:06 UTC (permalink / raw)
To: John Fastabend; +Cc: Jiri Pirko, davem, stephen, netdev
In-Reply-To: <547606B3.3060808@gmail.com>
On 11/26/14 at 08:58am, John Fastabend wrote:
> On 11/26/2014 04:42 AM, Thomas Graf wrote:
> >Adds various missing length checks in the bridging code for Netlink
> >messages and corresponding attributes provided by user space.
> >
> >Thomas Graf (5):
> > bridge: Validate IFLA_BRIDGE_FLAGS attribute length
> > net: Validate IFLA_BRIDGE_MODE attribute length
> > net: Check for presence of IFLA_AF_SPEC
> > bridge: Add missing policy entry for IFLA_BRPORT_FAST_LEAVE
> > bridge: Sanitize IFLA_EXT_MASK for AF_BRIDGE:RTM_GETLINK
> >
> > drivers/net/ethernet/emulex/benet/be_main.c | 5 +++++
> > drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 5 +++++
> > net/bridge/br_netlink.c | 1 +
> > net/core/rtnetlink.c | 23 ++++++++++++++++++-----
> > 4 files changed, 29 insertions(+), 5 deletions(-)
> >
>
> +Jiri
>
> Looks like a miss in bond_netlink also? Seems like writing
> a smatch or cocci check for this would be worthwhile.
Thanks, I'll take a look.
The cocci check is somewhat difficult as validation is often
centralized and decoupled from actual access to implement atomic
operations. I'll give it a try though.
^ permalink raw reply
* Re: [PATCH net] r8152: drop the tx packet with invalid length
From: David Miller @ 2014-11-26 17:06 UTC (permalink / raw)
To: eric.dumazet; +Cc: hayeswang, netdev, nic_swsd, linux-kernel, linux-usb
In-Reply-To: <1417020748.29427.59.camel@edumazet-glaptop2.roam.corp.google.com>
From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Wed, 26 Nov 2014 08:52:28 -0800
> On Wed, 2014-11-26 at 17:56 +0800, Hayes Wang wrote:
>> Drop the tx packet which is more than the size of agg_buf_sz. When
>> creating a bridge with the device, we may get the tx packet with
>> TSO and the length is more than the gso_max_size which is set by
>> the driver through netif_set_gso_max_size(). Such packets couldn't
>> be transmitted and should be dropped directly.
>>
>> Signed-off-by: Hayes Wang <hayeswang@realtek.com>
...
> Looks like a candidate for ndo_gso_check(), so that we do not drop, but
> instead segment from netif_needs_gso()/validate_xmit_skb()
You mean have the bridge implement the ndo_gso_check() method right?
^ permalink raw reply
* Re: [PATCH net-next] pkt_sched: fq: increase max delay from 125 ms to one second
From: David Miller @ 2014-11-26 17:08 UTC (permalink / raw)
To: eric.dumazet; +Cc: yangyingliang, netdev, ncardwell
In-Reply-To: <1416934649.29427.34.camel@edumazet-glaptop2.roam.corp.google.com>
From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Tue, 25 Nov 2014 08:57:29 -0800
> From: Eric Dumazet <edumazet@google.com>
>
> FQ/pacing has a clamp of delay of 125 ms, to avoid some possible harm.
>
> It turns out this delay is too small to allow pacing low rates :
> Some ISP setup very aggressive policers as low as 16kbit.
>
> Now TCP stack has spurious rtx prevention, it seems safe to increase
> this fixed parameter, without adding a qdisc attribute.
>
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Cc: Yang Yingliang <yangyingliang@huawei.com>
Applied, thanks Eric.
^ permalink raw reply
* Re: [patch net-next v3 04/17] net: introduce generic switch devices support
From: Jamal Hadi Salim @ 2014-11-26 17:09 UTC (permalink / raw)
To: Thomas Graf
Cc: Scott Feldman, Jiri Pirko, Netdev, David S. Miller,
nhorman@tuxdriver.com, Andy Gospodarek, dborkman@redhat.com,
ogerlitz@mellanox.com, jesse@nicira.com, pshelar@nicira.com,
azhou@nicira.com, ben@decadent.org.uk, stephen@networkplumber.org,
Kirsher, Jeffrey T, vyasevic@redhat.com, Cong Wang,
Fastabend, John R, Eric Dumazet, Florian Fainelli, Roopa Prabhu,
John Linville
In-Reply-To: <20141126160859.GB13786@casper.infradead.org>
On 11/26/14 11:08, Thomas Graf wrote:
> On 11/26/14 at 06:36am, Jamal Hadi Salim wrote:
>
>
> Jamal,
>
> What is irriating in this context is that you are pushing back on
> Jiri and others while referring to properitary and closed code which
> you are unwilling or unable to share. I don't see this as being
> passive aggressive, everybody is treated the same way in this regard.
>
WTF? I said i have hardware that is not a switch because it doesnt
do switching. This all started with the name being "switch" which
I objected to. You ask me to describe hardware and then you come
back and say I am using that to stop progress?
Where the hell did i push back on Jiri? Stop going around
telling people i do. I invest my time and effort reviewing code,
proposing ideas, posting etc calling meetings. Infact i initiated
this whole effort to begin with.
There is no point to responding to any of your other comments.
cheers,
jamal
^ permalink raw reply
* Re: [PATCH net] tcp: fix possible NULL dereference in tcp_vX_send_reset()
From: David Miller @ 2014-11-26 17:09 UTC (permalink / raw)
To: eric.dumazet; +Cc: netdev, dborkman, jasa.bartelj
In-Reply-To: <1416930004.29427.21.camel@edumazet-glaptop2.roam.corp.google.com>
From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Tue, 25 Nov 2014 07:40:04 -0800
> From: Eric Dumazet <edumazet@google.com>
>
> After commit ca777eff51f7 ("tcp: remove dst refcount false sharing for
> prequeue mode") we have to relax check against skb dst in
> tcp_v[46]_send_reset() if prequeue dropped the dst.
>
> If a socket is provided, a full lookup was done to find this socket,
> so the dst test can be skipped.
>
> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=88191
> Reported-by: Jaša Bartelj <jasa.bartelj@gmail.com>
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Reported-by: Daniel Borkmann <dborkman@redhat.com>
> Fixes: ca777eff51f7 ("tcp: remove dst refcount false sharing for prequeue mode")
Applied, thanks Eric.
^ permalink raw reply
* Re: [PATCH] stmmac: platform: fix default values of the filter bins setting
From: Giuseppe CAVALLARO @ 2014-11-26 17:16 UTC (permalink / raw)
To: Huacai Chen; +Cc: Vince Bridgers, David S. Miller, netdev
In-Reply-To: <1416969486-9047-1-git-send-email-chenhc@lemote.com>
On 11/26/2014 3:38 AM, Huacai Chen wrote:
> The commit 3b57de958e2a brought the support for a different amount of
> the filter bins, but didn't update the platform driver that without
> CONFIG_OF.
>
> Fixes: 3b57de958e2a (net: stmmac: Support devicetree configs for mcast
> and ucast filter entries)
>
> Signed-off-by: Huacai Chen <chenhc@lemote.com>
Acked-by: Giuseppe Cavallaro <peppe.cavallaro@st.com>
> ---
> .../net/ethernet/stmicro/stmmac/stmmac_platform.c | 13 +++++++------
> 1 files changed, 7 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
> index db56fa7..5b0da39 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
> @@ -177,12 +177,6 @@ static int stmmac_probe_config_dt(struct platform_device *pdev,
> */
> plat->maxmtu = JUMBO_LEN;
>
> - /* Set default value for multicast hash bins */
> - plat->multicast_filter_bins = HASH_TABLE_SIZE;
> -
> - /* Set default value for unicast filter entries */
> - plat->unicast_filter_entries = 1;
> -
> /*
> * Currently only the properties needed on SPEAr600
> * are provided. All other properties should be added
> @@ -270,6 +264,13 @@ static int stmmac_pltfr_probe(struct platform_device *pdev)
> return PTR_ERR(addr);
>
> plat_dat = dev_get_platdata(&pdev->dev);
> +
> + /* Set default value for multicast hash bins */
> + plat_dat->multicast_filter_bins = HASH_TABLE_SIZE;
> +
> + /* Set default value for unicast filter entries */
> + plat_dat->unicast_filter_entries = 1;
> +
> if (pdev->dev.of_node) {
> if (!plat_dat)
> plat_dat = devm_kzalloc(&pdev->dev,
>
^ permalink raw reply
* Re: [PATCH 1/4] GMAC: add driver for Rockchip RK3288 SoCs integrated GMAC
From: Giuseppe CAVALLARO @ 2014-11-26 17:14 UTC (permalink / raw)
To: Roger, heiko
Cc: netdev, linux-kernel, linux-rockchip, kever.yang, mark.yao,
eddie.cai
In-Reply-To: <54753B17.1050308@rock-chips.com>
On 11/26/2014 3:29 AM, Roger wrote:
> Hi! Giuseppe CAVALLARO
>
> 在 2014/11/25 18:05, Giuseppe CAVALLARO 写道:
>> Hello Roger
>>
>> thx for these patches, my comments inline below
>>
>> On 11/25/2014 10:07 AM, Roger Chen wrote:
>>> This driver is based on stmmac driver.
>>>
>>> Signed-off-by: Roger Chen <roger.chen@rock-chips.com>
>>> ---
>>> drivers/net/ethernet/stmicro/stmmac/Makefile | 2 +-
>>> drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c | 629
>>> ++++++++++++++++++++
>>> .../net/ethernet/stmicro/stmmac/stmmac_platform.c | 3 +
>>> .../net/ethernet/stmicro/stmmac/stmmac_platform.h | 1 +
>>> 4 files changed, 634 insertions(+), 1 deletion(-)
>>> create mode 100644 drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c
>>>
>>> diff --git a/drivers/net/ethernet/stmicro/stmmac/Makefile
>>> b/drivers/net/ethernet/stmicro/stmmac/Makefile
>>> index ac4d562..73c2715 100644
>>> --- a/drivers/net/ethernet/stmicro/stmmac/Makefile
>>> +++ b/drivers/net/ethernet/stmicro/stmmac/Makefile
>>> @@ -6,7 +6,7 @@ stmmac-objs:= stmmac_main.o stmmac_ethtool.o
>>> stmmac_mdio.o ring_mode.o \
>>>
>>> obj-$(CONFIG_STMMAC_PLATFORM) += stmmac-platform.o
>>> stmmac-platform-objs:= stmmac_platform.o dwmac-meson.o
>>> dwmac-sunxi.o \
>>> - dwmac-sti.o dwmac-socfpga.o
>>> + dwmac-sti.o dwmac-socfpga.o dwmac-rk.o
>>>
>>> obj-$(CONFIG_STMMAC_PCI) += stmmac-pci.o
>>> stmmac-pci-objs:= stmmac_pci.o
>>> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c
>>> b/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c
>>> new file mode 100644
>>> index 0000000..9e50b37
>>> --- /dev/null
>>> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c
>>> @@ -0,0 +1,629 @@
>>> +/**
>>> + * dwmac-rk.c - Rockchip RK3288 DWMAC specific glue layer
>>> + *
>>> + * Copyright (C) 2014 Chen-Zhi (Roger Chen)
>>> + *
>>> + * Chen-Zhi (Roger Chen) <roger.chen@rock-chips.com>
>>> + *
>>> + * 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.
>>> + */
>>> +
>>> +#include <linux/stmmac.h>
>>> +#include <linux/clk.h>
>>> +#include <linux/phy.h>
>>> +#include <linux/of_net.h>
>>> +#include <linux/gpio.h>
>>> +#include <linux/of_gpio.h>
>>> +#include <linux/of_device.h>
>>> +#include <linux/regulator/consumer.h>
>>> +#include <linux/delay.h>
>>> +#include <linux/regmap.h>
>>> +#include <linux/mfd/syscon.h>
>>> +
>>> +struct rk_priv_data {
>>> + struct platform_device *pdev;
>>> + int phy_iface;
>>> + bool power_ctrl_by_pmu;
>>> + char pmu_regulator[32];
>>> + int pmu_enable_level;
>>> +
>>> + int power_io;
>>> + int power_io_level;
>>> + int reset_io;
>>> + int reset_io_level;
>>> + int phyirq_io;
>>> + int phyirq_io_level;
>>> +
>>> + bool clk_enabled;
>>> + bool clock_input;
>>> +
>>> + struct clk *clk_mac;
>>> + struct clk *clk_mac_pll;
>>> + struct clk *gmac_clkin;
>>> + struct clk *mac_clk_rx;
>>> + struct clk *mac_clk_tx;
>>> + struct clk *clk_mac_ref;
>>> + struct clk *clk_mac_refout;
>>> + struct clk *aclk_mac;
>>> + struct clk *pclk_mac;
>>> +
>>> + int tx_delay;
>>> + int rx_delay;
>>> +
>>> + struct regmap *grf;
>>> +};
>>> +
>>> +#define RK3288_GRF_SOC_CON1 0x0248
>>> +#define RK3288_GRF_SOC_CON3 0x0250
>>> +#define RK3288_GRF_GPIO3D_E 0x01ec
>>> +#define RK3288_GRF_GPIO4A_E 0x01f0
>>> +#define RK3288_GRF_GPIO4B_E 0x01f4
>>> +
>>> +/*RK3288_GRF_SOC_CON1*/
>>> +#define GMAC_PHY_INTF_SEL_RGMII ((0x01C0 << 16) | (0x0040))
>>> +#define GMAC_PHY_INTF_SEL_RMII ((0x01C0 << 16) | (0x0100))
>>> +#define GMAC_FLOW_CTRL ((0x0200 << 16) | (0x0200))
>>> +#define GMAC_FLOW_CTRL_CLR ((0x0200 << 16) | (0x0000))
>>> +#define GMAC_SPEED_10M ((0x0400 << 16) | (0x0000))
>>> +#define GMAC_SPEED_100M ((0x0400 << 16) | (0x0400))
>>> +#define GMAC_RMII_CLK_25M ((0x0800 << 16) | (0x0800))
>>> +#define GMAC_RMII_CLK_2_5M ((0x0800 << 16) | (0x0000))
>>> +#define GMAC_CLK_125M ((0x3000 << 16) | (0x0000))
>>> +#define GMAC_CLK_25M ((0x3000 << 16) | (0x3000))
>>> +#define GMAC_CLK_2_5M ((0x3000 << 16) | (0x2000))
>>> +#define GMAC_RMII_MODE ((0x4000 << 16) | (0x4000))
>>> +#define GMAC_RMII_MODE_CLR ((0x4000 << 16) | (0x0000))
>>> +
>>> +/*RK3288_GRF_SOC_CON3*/
>>> +#define GMAC_TXCLK_DLY_ENABLE ((0x4000 << 16) | (0x4000))
>>> +#define GMAC_TXCLK_DLY_DISABLE ((0x4000 << 16) | (0x0000))
>>> +#define GMAC_RXCLK_DLY_ENABLE ((0x8000 << 16) | (0x8000))
>>> +#define GMAC_RXCLK_DLY_DISABLE ((0x8000 << 16) | (0x0000))
>>> +#define GMAC_CLK_RX_DL_CFG(val) ((0x3F80 << 16) | (val<<7))
>>> +#define GMAC_CLK_TX_DL_CFG(val) ((0x007F << 16) | (val))
>>
>> why do not use BIT and BIT_MASK where possible?
>>
>>> +static void set_to_rgmii(struct rk_priv_data *bsp_priv,
>>> + int tx_delay, int rx_delay)
>>> +{
>>> + if (IS_ERR(bsp_priv->grf)) {
>>> + pr_err("%s: Missing rockchip,grf property\n", __func__);
>>> + return;
>>> + }
>>> +
>>> + regmap_write(bsp_priv->grf, RK3288_GRF_SOC_CON1,
>>> + GMAC_PHY_INTF_SEL_RGMII);
>>> + regmap_write(bsp_priv->grf, RK3288_GRF_SOC_CON1,
>>> + GMAC_RMII_MODE_CLR);
>>
>> maybe you could perform just one write unless there is some HW
>> constraint.
>>
>>> + regmap_write(bsp_priv->grf, RK3288_GRF_SOC_CON3,
>>> + GMAC_RXCLK_DLY_ENABLE);
>>> + regmap_write(bsp_priv->grf, RK3288_GRF_SOC_CON3,
>>> + GMAC_TXCLK_DLY_ENABLE);
>>> + regmap_write(bsp_priv->grf, RK3288_GRF_SOC_CON3,
>>> + GMAC_CLK_RX_DL_CFG(rx_delay));
>>> + regmap_write(bsp_priv->grf, RK3288_GRF_SOC_CON3,
>>> + GMAC_CLK_TX_DL_CFG(tx_delay));
>>
>> ditto
>>
>>> +
>>> + regmap_write(bsp_priv->grf, RK3288_GRF_GPIO3D_E, 0xFFFFFFFF);
>>> + regmap_write(bsp_priv->grf, RK3288_GRF_GPIO4B_E,
>>> + 0x3<<2<<16 | 0x3<<2);
>>
>> pls use macros, these shift sequence is really help to decode
>>
>>> + regmap_write(bsp_priv->grf, RK3288_GRF_GPIO4A_E, 0xFFFFFFFF);
>>> +
>>> + pr_info("%s: tx delay=0x%x; rx delay=0x%x;\n",
>>> + __func__, tx_delay, rx_delay);
>>
>> may I suggest pr_debug?
>>
>>> +}
>>> +
>>> +static void set_to_rmii(struct rk_priv_data *bsp_priv)
>>> +{
>>> + if (IS_ERR(bsp_priv->grf)) {
>>> + pr_err("%s: Missing rockchip,grf property\n", __func__);
>>> + return;
>>> + }
>>> +
>>> + regmap_write(bsp_priv->grf, RK3288_GRF_SOC_CON1,
>>> + GMAC_PHY_INTF_SEL_RMII);
>>> + regmap_write(bsp_priv->grf, RK3288_GRF_SOC_CON1,
>>> + GMAC_RMII_MODE);
>>> +}
>>> +
>>> +static void set_rgmii_speed(struct rk_priv_data *bsp_priv, int speed)
>>> +{
>>> + if (IS_ERR(bsp_priv->grf)) {
>>> + pr_err("%s: Missing rockchip,grf property\n", __func__);
>>> + return;
>>> + }
>>> +
>>> + if (speed == 10)
>>> + regmap_write(bsp_priv->grf, RK3288_GRF_SOC_CON1,
>>> GMAC_CLK_2_5M);
>>> + else if (speed == 100)
>>> + regmap_write(bsp_priv->grf, RK3288_GRF_SOC_CON1, GMAC_CLK_25M);
>>> + else if (speed == 1000)
>>> + regmap_write(bsp_priv->grf, RK3288_GRF_SOC_CON1,
>>> GMAC_CLK_125M);
>>> + else
>>> + pr_err("unknown speed value for RGMII! speed=%d", speed);
>>> +}
>>> +
>>> +static void set_rmii_speed(struct rk_priv_data *bsp_priv, int speed)
>>> +{
>>> + if (IS_ERR(bsp_priv->grf)) {
>>> + pr_err("%s: Missing rockchip,grf property\n", __func__);
>>> + return;
>>> + }
>>> +
>>> + if (speed == 10) {
>>> + regmap_write(bsp_priv->grf, RK3288_GRF_SOC_CON1,
>>> + GMAC_RMII_CLK_2_5M);
>>> + regmap_write(bsp_priv->grf, RK3288_GRF_SOC_CON1,
>>> + GMAC_SPEED_10M);
>>> + } else if (speed == 100) {
>>> + regmap_write(bsp_priv->grf, RK3288_GRF_SOC_CON1,
>>> + GMAC_RMII_CLK_25M);
>>> + regmap_write(bsp_priv->grf, RK3288_GRF_SOC_CON1,
>>> + GMAC_SPEED_100M);
>>> + } else {
>>> + pr_err("unknown speed value for RMII! speed=%d", speed);
>>> + }
>>> +}
>>> +
>>> +#define MAC_CLK_RX "mac_clk_rx"
>>> +#define MAC_CLK_TX "mac_clk_tx"
>>> +#define CLK_MAC_REF "clk_mac_ref"
>>> +#define CLK_MAC_REF_OUT "clk_mac_refout"
>>> +#define CLK_MAC_PLL "clk_mac_pll"
>>> +#define ACLK_MAC "aclk_mac"
>>> +#define PCLK_MAC "pclk_mac"
>>> +#define MAC_CLKIN "ext_gmac"
>>> +#define CLK_MAC "stmmaceth"
>>
>> Concerning the clocks, the "stmmaceth" is actually used for the main
>> clock so maybe you should use another name.
>>
> in stmmac_main.c, clk "stmmaceth"(STMMAC_RESOURCE_NAME) is used to be
> the source of priv->clk_csr.
> priv->stmmac_clk = devm_clk_get(priv->device, STMMAC_RESOURCE_NAME);
> so I have to define a kind of clock named "stmmaceth".
ok
I was a bit confused because I had seen that you manage it as parent
so I believed it was for phy clock.
>
>> See: Documentation/devicetree/bindings/net/stmmac.txt
>>
>> For St platforms, it has been used: sti-ethclk but we could
>> used a common and better name: stmmac-phyclk
>> And I could also propose this change on STi glue-logic.
>>
>> Then, I think that all the clock macros above should be documented
>> and maybe managed at DT level, what do you think?
>>
>>> +
>>> +int gmac_clk_init(struct rk_priv_data *bsp_priv)
>>> +{
>>> + struct device *dev = &bsp_priv->pdev->dev;
>>> +
>>> + bsp_priv->clk_enabled = false;
>>> +
>>> + bsp_priv->mac_clk_rx = clk_get(dev, MAC_CLK_RX);
>>> + if (IS_ERR(bsp_priv->mac_clk_rx))
>>> + pr_warn("%s: warning: cannot get clock %s\n",
>>> + __func__, MAC_CLK_RX);
>>> +
>>> + bsp_priv->mac_clk_tx = clk_get(dev, MAC_CLK_TX);
>>> + if (IS_ERR(bsp_priv->mac_clk_tx))
>>> + pr_warn("%s: warning: cannot get clock %s\n",
>>> + __func__, MAC_CLK_TX);
>>> +
>>> + bsp_priv->clk_mac_ref = clk_get(dev, CLK_MAC_REF);
>>> + if (IS_ERR(bsp_priv->clk_mac_ref))
>>> + pr_warn("%s: warning: cannot get clock %s\n",
>>> + __func__, CLK_MAC_REF);
>>> +
>>> + bsp_priv->clk_mac_refout = clk_get(dev, CLK_MAC_REF_OUT);
>>> + if (IS_ERR(bsp_priv->clk_mac_refout))
>>> + pr_warn("%s: warning:cannot get clock %s\n",
>>> + __func__, CLK_MAC_REF_OUT);
>>> +
>>> + bsp_priv->aclk_mac = clk_get(dev, ACLK_MAC);
>>> + if (IS_ERR(bsp_priv->aclk_mac))
>>> + pr_warn("%s: warning: cannot get clock %s\n",
>>> + __func__, ACLK_MAC);
>>> +
>>> + bsp_priv->pclk_mac = clk_get(dev, PCLK_MAC);
>>> + if (IS_ERR(bsp_priv->pclk_mac))
>>> + pr_warn("%s: warning: cannot get clock %s\n",
>>> + __func__, PCLK_MAC);
>>> +
>>> + bsp_priv->clk_mac_pll = clk_get(dev, CLK_MAC_PLL);
>>> + if (IS_ERR(bsp_priv->clk_mac_pll))
>>> + pr_warn("%s: warning: cannot get clock %s\n",
>>> + __func__, CLK_MAC_PLL);
>>> +
>>> + bsp_priv->gmac_clkin = clk_get(dev, MAC_CLKIN);
>>> + if (IS_ERR(bsp_priv->gmac_clkin))
>>> + pr_warn("%s: warning: cannot get clock %s\n",
>>> + __func__, MAC_CLKIN);
>>> +
>>> + bsp_priv->clk_mac = clk_get(dev, CLK_MAC);
>>> + if (IS_ERR(bsp_priv->clk_mac))
>>> + pr_warn("%s: warning: cannot get clock %s\n",
>>> + __func__, CLK_MAC);
>>> +
>>> + if (bsp_priv->clock_input) {
>>> + pr_info("%s: clock input from PHY\n", __func__);
>>> + } else {
>>> + if (bsp_priv->phy_iface == PHY_INTERFACE_MODE_RMII)
>>> + clk_set_rate(bsp_priv->clk_mac_pll, 50000000);
>>> +
>>> + clk_set_parent(bsp_priv->clk_mac, bsp_priv->clk_mac_pll);
>>> + }
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static int gmac_clk_enable(struct rk_priv_data *bsp_priv, bool enable)
>>> +{
>>> + int phy_iface = phy_iface = bsp_priv->phy_iface;
>>> +
>>> + if (enable) {
>>> + if (!bsp_priv->clk_enabled) {
>>> + if (phy_iface == PHY_INTERFACE_MODE_RMII) {
>>> + if (!IS_ERR(bsp_priv->mac_clk_rx))
>>> + clk_prepare_enable(
>>> + bsp_priv->mac_clk_rx);
>>> +
>>> + if (!IS_ERR(bsp_priv->clk_mac_ref))
>>> + clk_prepare_enable(
>>> + bsp_priv->clk_mac_ref);
>>> +
>>> + if (!IS_ERR(bsp_priv->clk_mac_refout))
>>> + clk_prepare_enable(
>>> + bsp_priv->clk_mac_refout);
>>> + }
>>> +
>>> + if (!IS_ERR(bsp_priv->aclk_mac))
>>> + clk_prepare_enable(bsp_priv->aclk_mac);
>>> +
>>> + if (!IS_ERR(bsp_priv->pclk_mac))
>>> + clk_prepare_enable(bsp_priv->pclk_mac);
>>> +
>>> + if (!IS_ERR(bsp_priv->mac_clk_tx))
>>> + clk_prepare_enable(bsp_priv->mac_clk_tx);
>>> +
>>> + /**
>>> + * if (!IS_ERR(bsp_priv->clk_mac))
>>> + * clk_prepare_enable(bsp_priv->clk_mac);
>>> + */
>>
>> why do you need to comment this? could it be related to the stmmaceth
>> clk that is used by the main?
>>
> yes. the same explanation as above
>>> + mdelay(5);
>>
>> hmm, do you actually need this mdelay? Is it to stabilize PLL sources?
>>
> yes.
>>> + bsp_priv->clk_enabled = true;
>>> + }
>>> + } else {
>>> + if (bsp_priv->clk_enabled) {
>>> + if (phy_iface == PHY_INTERFACE_MODE_RMII) {
>>> + if (!IS_ERR(bsp_priv->mac_clk_rx))
>>> + clk_disable_unprepare(
>>> + bsp_priv->mac_clk_rx);
>>> +
>>> + if (!IS_ERR(bsp_priv->clk_mac_ref))
>>> + clk_disable_unprepare(
>>> + bsp_priv->clk_mac_ref);
>>> +
>>> + if (!IS_ERR(bsp_priv->clk_mac_refout))
>>> + clk_disable_unprepare(
>>> + bsp_priv->clk_mac_refout);
>>> + }
>>> +
>>> + if (!IS_ERR(bsp_priv->aclk_mac))
>>> + clk_disable_unprepare(bsp_priv->aclk_mac);
>>> +
>>> + if (!IS_ERR(bsp_priv->pclk_mac))
>>> + clk_disable_unprepare(bsp_priv->pclk_mac);
>>> +
>>> + if (!IS_ERR(bsp_priv->mac_clk_tx))
>>> + clk_disable_unprepare(bsp_priv->mac_clk_tx);
>>> + /**
>>> + * if (!IS_ERR(bsp_priv->clk_mac))
>>> + * clk_disable_unprepare(bsp_priv->clk_mac);
>>> + */
>>> + bsp_priv->clk_enabled = false;
>>> + }
>>> + }
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static int power_on_by_pmu(struct rk_priv_data *bsp_priv, bool enable)
>>> +{
>>> + struct regulator *ldo;
>>> + char *ldostr = bsp_priv->pmu_regulator;
>>> + int ret;
>>> +
>>> + if (!ldostr) {
>>> + pr_err("%s: no ldo found\n", __func__);
>>> + return -1;
>>> + }
>>> +
>>> + ldo = regulator_get(NULL, ldostr);
>>> + if (!ldo) {
>>> + pr_err("\n%s get ldo %s failed\n", __func__, ldostr);
>>> + } else {
>>> + if (enable) {
>>> + if (!regulator_is_enabled(ldo)) {
>>> + regulator_set_voltage(ldo, 3300000, 3300000);
>>> + ret = regulator_enable(ldo);
>>> + if (ret != 0)
>>> + pr_err("%s: fail to enable %s\n",
>>> + __func__, ldostr);
>>> + else
>>> + pr_info("turn on ldo done.\n");
>>> + } else {
>>> + pr_warn("%s is enabled before enable", ldostr);
>>> + }
>>> + } else {
>>> + if (regulator_is_enabled(ldo)) {
>>> + ret = regulator_disable(ldo);
>>> + if (ret != 0)
>>> + pr_err("%s: fail to disable %s\n",
>>> + __func__, ldostr);
>>> + else
>>> + pr_info("turn off ldo done.\n");
>>> + } else {
>>> + pr_warn("%s is disabled before disable",
>>> + ldostr);
>>> + }
>>> + }
>>> + regulator_put(ldo);
>>> + }
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static int power_on_by_gpio(struct rk_priv_data *bsp_priv, bool enable)
>>> +{
>>> + if (enable) {
>>> + /*power on*/
>>> + if (gpio_is_valid(bsp_priv->power_io))
>>> + gpio_direction_output(bsp_priv->power_io,
>>> + bsp_priv->power_io_level);
>>> + } else {
>>> + /*power off*/
>>> + if (gpio_is_valid(bsp_priv->power_io))
>>> + gpio_direction_output(bsp_priv->power_io,
>>> + !bsp_priv->power_io_level);
>>> + }
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static int phy_power_on(struct rk_priv_data *bsp_priv, bool enable)
>>> +{
>>> + int ret = -1;
>>> +
>>> + pr_info("Ethernet PHY power %s\n", enable == 1 ? "on" : "off");
>>> +
>>> + if (bsp_priv->power_ctrl_by_pmu)
>>> + ret = power_on_by_pmu(bsp_priv, enable);
>>> + else
>>> + ret = power_on_by_gpio(bsp_priv, enable);
>>> +
>>> + if (enable) {
>>> + /*reset*/
>>> + if (gpio_is_valid(bsp_priv->reset_io)) {
>>> + gpio_direction_output(bsp_priv->reset_io,
>>> + bsp_priv->reset_io_level);
>>> + mdelay(5);
>>> + gpio_direction_output(bsp_priv->reset_io,
>>> + !bsp_priv->reset_io_level);
>>> + }
>>> + mdelay(30);
>>> +
>>> + } else {
>>> + /*pull down reset*/
>>> + if (gpio_is_valid(bsp_priv->reset_io)) {
>>> + gpio_direction_output(bsp_priv->reset_io,
>>> + bsp_priv->reset_io_level);
>>> + }
>>> + }
>>> +
>>> + return ret;
>>> +}
>>> +
>>> +#define GPIO_PHY_POWER "gmac_phy_power"
>>> +#define GPIO_PHY_RESET "gmac_phy_reset"
>>> +#define GPIO_PHY_IRQ "gmac_phy_irq"
>>> +
>>> +static void *rk_gmac_setup(struct platform_device *pdev)
>>> +{
>>> + struct rk_priv_data *bsp_priv;
>>> + struct device *dev = &pdev->dev;
>>> + enum of_gpio_flags flags;
>>> + int ret;
>>> + const char *strings = NULL;
>>> + int value;
>>> + int irq;
>>> +
>>> + bsp_priv = devm_kzalloc(dev, sizeof(*bsp_priv), GFP_KERNEL);
>>> + if (!bsp_priv)
>>> + return ERR_PTR(-ENOMEM);
>>> +
>>> + bsp_priv->phy_iface = of_get_phy_mode(dev->of_node);
>>> +
>>> + ret = of_property_read_string(dev->of_node, "pmu_regulator",
>>> &strings);
>>> + if (ret) {
>>> + pr_err("%s: Can not read property: pmu_regulator.\n",
>>> __func__);
>>> + bsp_priv->power_ctrl_by_pmu = false;
>>> + } else {
>>> + pr_info("%s: ethernet phy power controlled by pmu(%s).\n",
>>> + __func__, strings);
>>> + bsp_priv->power_ctrl_by_pmu = true;
>>> + strcpy(bsp_priv->pmu_regulator, strings);
>>> + }
>>> +
>>> + ret = of_property_read_u32(dev->of_node, "pmu_enable_level",
>>> &value);
>>> + if (ret) {
>>> + pr_err("%s: Can not read property: pmu_enable_level.\n",
>>> + __func__);
>>> + bsp_priv->power_ctrl_by_pmu = false;
>>> + } else {
>>> + pr_info("%s: PHY power controlled by pmu(level = %s).\n",
>>> + __func__, (value == 1) ? "HIGH" : "LOW");
>>> + bsp_priv->power_ctrl_by_pmu = true;
>>> + bsp_priv->pmu_enable_level = value;
>>> + }
>>> +
>>> + ret = of_property_read_string(dev->of_node, "clock_in_out",
>>> &strings);
>>> + if (ret) {
>>> + pr_err("%s: Can not read property: clock_in_out.\n", __func__);
>>> + bsp_priv->clock_input = true;
>>> + } else {
>>> + pr_info("%s: clock input or output? (%s).\n",
>>> + __func__, strings);
>>> + if (!strcmp(strings, "input"))
>>> + bsp_priv->clock_input = true;
>>> + else
>>> + bsp_priv->clock_input = false;
>>> + }
>>> +
>>> + ret = of_property_read_u32(dev->of_node, "tx_delay", &value);
>>> + if (ret) {
>>> + bsp_priv->tx_delay = 0x30;
>>> + pr_err("%s: Can not read property: tx_delay.", __func__);
>>> + pr_err("%s: set tx_delay to 0x%x\n",
>>> + __func__, bsp_priv->tx_delay);
>>> + } else {
>>> + pr_info("%s: TX delay(0x%x).\n", __func__, value);
>>> + bsp_priv->tx_delay = value;
>>> + }
>>> +
>>> + ret = of_property_read_u32(dev->of_node, "rx_delay", &value);
>>> + if (ret) {
>>> + bsp_priv->rx_delay = 0x10;
>>> + pr_err("%s: Can not read property: rx_delay.", __func__);
>>> + pr_err("%s: set rx_delay to 0x%x\n",
>>> + __func__, bsp_priv->rx_delay);
>>> + } else {
>>> + pr_info("%s: RX delay(0x%x).\n", __func__, value);
>>> + bsp_priv->rx_delay = value;
>>> + }
>>> +
>>> + bsp_priv->grf = syscon_regmap_lookup_by_phandle(dev->of_node,
>>> + "rockchip,grf");
>>> + if (IS_ERR(bsp_priv->grf))
>>> + dev_err(&pdev->dev, "Missing rockchip,grf property\n");
>>
>> I wonder if you can fail on here and save all the check in
>> set_rgmii_speed etc.
>> Maybe this can be considered a mandatory property for the glue-logic.
>>
> about fail check, you mean "if(IS_ERR(bsp_priv->grf))" ?
> or all of the return value of of_property_*
I meant about the grf only.
Peppe
>>> +
>>> + bsp_priv->phyirq_io =
>>> + of_get_named_gpio_flags(dev->of_node,
>>> + "phyirq-gpio", 0, &flags);
>>> + bsp_priv->phyirq_io_level = (flags & OF_GPIO_ACTIVE_LOW) ? 0 : 1;
>>
>>
>> Sorry, did you document all in Documentation/devicetree/bindings/net/ ?
>>
>>> +
>>> + bsp_priv->reset_io =
>>> + of_get_named_gpio_flags(dev->of_node,
>>> + "reset-gpio", 0, &flags);
>>> + bsp_priv->reset_io_level = (flags & OF_GPIO_ACTIVE_LOW) ? 0 : 1;
>>> +
>>> + bsp_priv->power_io =
>>> + of_get_named_gpio_flags(dev->of_node, "power-gpio", 0, &flags);
>>> + bsp_priv->power_io_level = (flags & OF_GPIO_ACTIVE_LOW) ? 0 : 1;
>>> +
>>> + /*power*/
>>> + if (!gpio_is_valid(bsp_priv->power_io)) {
>>> + pr_err("%s: Failed to get GPIO %s.\n",
>>> + __func__, "power-gpio");
>>> + } else {
>>> + ret = gpio_request(bsp_priv->power_io, GPIO_PHY_POWER);
>>> + if (ret)
>>> + pr_err("%s: ERROR: Failed to request GPIO %s.\n",
>>> + __func__, GPIO_PHY_POWER);
>>> + }
>>> +
>>> + if (!gpio_is_valid(bsp_priv->reset_io)) {
>>> + pr_err("%s: ERROR: Get reset-gpio failed.\n", __func__);
>>> + } else {
>>> + ret = gpio_request(bsp_priv->reset_io, GPIO_PHY_RESET);
>>> + if (ret)
>>> + pr_err("%s: ERROR: Failed to request GPIO %s.\n",
>>> + __func__, GPIO_PHY_RESET);
>>> + }
>>> +
>>> + if (bsp_priv->phyirq_io > 0) {
>>> + struct plat_stmmacenet_data *plat_dat;
>>> +
>>> + pr_info("PHY irq in use\n");
>>> + ret = gpio_request(bsp_priv->phyirq_io, GPIO_PHY_IRQ);
>>> + if (ret < 0) {
>>> + pr_warn("%s: Failed to request GPIO %s\n",
>>> + __func__, GPIO_PHY_IRQ);
>>> + goto goon;
>>> + }
>>> +
>>> + ret = gpio_direction_input(bsp_priv->phyirq_io);
>>> + if (ret < 0) {
>>> + pr_err("%s, Failed to set input for GPIO %s\n",
>>> + __func__, GPIO_PHY_IRQ);
>>> + gpio_free(bsp_priv->phyirq_io);
>>> + goto goon;
>>> + }
>>> +
>>> + irq = gpio_to_irq(bsp_priv->phyirq_io);
>>> + if (irq < 0) {
>>> + ret = irq;
>>> + pr_err("Failed to set irq for %s\n",
>>> + GPIO_PHY_IRQ);
>>> + gpio_free(bsp_priv->phyirq_io);
>>> + goto goon;
>>> + }
>>> +
>>> + plat_dat = dev_get_platdata(&pdev->dev);
>>> + if (plat_dat)
>>> + plat_dat->mdio_bus_data->probed_phy_irq = irq;
>>> + else
>>> + pr_err("%s: plat_data is NULL\n", __func__);
>>> + }
>>> +
>>> +goon:
>>> + /*rmii or rgmii*/
>>> + if (bsp_priv->phy_iface == PHY_INTERFACE_MODE_RGMII) {
>>> + pr_info("%s: init for RGMII\n", __func__);
>>> + set_to_rgmii(bsp_priv, bsp_priv->tx_delay, bsp_priv->rx_delay);
>>> + } else if (bsp_priv->phy_iface == PHY_INTERFACE_MODE_RMII) {
>>> + pr_info("%s: init for RMII\n", __func__);
>>> + set_to_rmii(bsp_priv);
>>> + } else {
>>> + pr_err("%s: ERROR: NO interface defined!\n", __func__);
>>> + }
>>> +
>>> + bsp_priv->pdev = pdev;
>>> +
>>> + gmac_clk_init(bsp_priv);
>>> +
>>> + return bsp_priv;
>>> +}
>>> +
>>> +static int rk_gmac_init(struct platform_device *pdev, void *priv)
>>> +{
>>> + struct rk_priv_data *bsp_priv = priv;
>>> + int ret;
>>> +
>>> + ret = phy_power_on(bsp_priv, true);
>>> + if (ret)
>>> + return ret;
>>> +
>>> + ret = gmac_clk_enable(bsp_priv, true);
>>> + if (ret)
>>> + return ret;
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static void rk_gmac_exit(struct platform_device *pdev, void *priv)
>>> +{
>>> + struct rk_priv_data *gmac = priv;
>>> +
>>> + phy_power_on(gmac, false);
>>> + gmac_clk_enable(gmac, false);
>>> +}
>>> +
>>> +static void rk_fix_speed(void *priv, unsigned int speed)
>>> +{
>>> + struct rk_priv_data *bsp_priv = priv;
>>> +
>>> + if (bsp_priv->phy_iface == PHY_INTERFACE_MODE_RGMII)
>>> + set_rgmii_speed(bsp_priv, speed);
>>> + else if (bsp_priv->phy_iface == PHY_INTERFACE_MODE_RMII)
>>> + set_rmii_speed(bsp_priv, speed);
>>> + else
>>> + pr_err("unsupported interface %d", bsp_priv->phy_iface);
>>> +}
>>> +
>>> +const struct stmmac_of_data rk_gmac_data = {
>>> + .has_gmac = 1,
>>> + .tx_coe = 1,
>>
>> FYI, on new gmac there is the HW capability register to dinamically
>> provide you if coe is supported.
>>
>> IMO you should add the OF "compatible" string and in case of mac
>> newer than the 3.50a you can remove coe.
>>
>>
>> BR
>> Peppe
>>
>>> + .fix_mac_speed = rk_fix_speed,
>>> + .setup = rk_gmac_setup,
>>> + .init = rk_gmac_init,
>>> + .exit = rk_gmac_exit,
>>> +};
>>> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
>>> b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
>>> index 15814b7..b4dee96 100644
>>> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
>>> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
>>> @@ -33,6 +33,7 @@
>>>
>>> static const struct of_device_id stmmac_dt_ids[] = {
>>> /* SoC specific glue layers should come before generic bindings */
>>> + { .compatible = "rockchip,rk3288-gmac", .data = &rk_gmac_data},
>>> { .compatible = "amlogic,meson6-dwmac", .data =
>>> &meson6_dwmac_data},
>>> { .compatible = "allwinner,sun7i-a20-gmac", .data =
>>> &sun7i_gmac_data},
>>> { .compatible = "st,stih415-dwmac", .data = &stih4xx_dwmac_data},
>>> @@ -291,6 +292,8 @@ static int stmmac_pltfr_probe(struct
>>> platform_device *pdev)
>>> return -ENOMEM;
>>> }
>>>
>>> + pdev->dev.platform_data = plat_dat;
>>> +
>>> ret = stmmac_probe_config_dt(pdev, plat_dat, &mac);
>>> if (ret) {
>>> pr_err("%s: main dt probe failed", __func__);
>>> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.h
>>> b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.h
>>> index 25dd1f7..32a0516 100644
>>> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.h
>>> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.h
>>> @@ -24,5 +24,6 @@ extern const struct stmmac_of_data sun7i_gmac_data;
>>> extern const struct stmmac_of_data stih4xx_dwmac_data;
>>> extern const struct stmmac_of_data stid127_dwmac_data;
>>> extern const struct stmmac_of_data socfpga_gmac_data;
>>> +extern const struct stmmac_of_data rk_gmac_data;
>>>
>>> #endif /* __STMMAC_PLATFORM_H__ */
>>>
>>
>>
>>
>>
>
>
>
^ permalink raw reply
* Re: [PATCH] x86: bpf_jit_comp: simplify trivial boolean return
From: Alexei Starovoitov @ 2014-11-26 17:23 UTC (permalink / raw)
To: Joe Perches
Cc: Quentin Lambert, David S. Miller, Alexey Kuznetsov, James Morris,
Hideaki YOSHIFUJI, Patrick McHardy, Thomas Gleixner, Ingo Molnar,
H. Peter Anvin, x86, netdev@vger.kernel.org,
linux-kernel@vger.kernel.org
On Wed, Nov 26, 2014 at 8:58 AM, Joe Perches <joe@perches.com> wrote:
> On Wed, 2014-11-26 at 08:42 -0800, Alexei Starovoitov wrote:
>> On Wed, Nov 26, 2014 at 1:18 AM, Quentin Lambert
>> <lambert.quentin@gmail.com> wrote:
>> > Remove if then else statements preceding
>> > boolean return.
> []
>> > diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c
> []
>> > @@ -135,11 +135,9 @@ static const int reg2hex[] = {
>> > */
>> > static inline bool is_ereg(u32 reg)
>> > {
>> > - if (reg == BPF_REG_5 || reg == AUX_REG ||
>> > - (reg >= BPF_REG_7 && reg <= BPF_REG_9))
>> > - return true;
>> > - else
>> > - return false;
>> > + return (reg == BPF_REG_5 ||
>> > + reg == AUX_REG ||
>> > + (reg >= BPF_REG_7 && reg <= BPF_REG_9));
>>
>> please remove extra () around the whole expression, and
>> align in properly, and
>> don't move reg==AUX_REG check to a different line.
>> Subject is not warranted. I don't think it's a simplification.
>
> It's not really a simplification,
> gcc should emit the same object code.
exactly.
>> imo existing code is fine and I don't think the time spent
>> reviewing such changes is worth it when there is no
>> improvement in readability.
>
> Is there any value in reordering these tests for frequency
> or maybe using | instead of || to avoid multiple jumps?
probably not. It's not a critical path.
compiler may fuse conditions depending on values anyway.
If it was a critical path, we could have used
(1 << reg) & mask trick.
I picked explicit 'return true' else 'return false' here,
because it felt easier to read. Just a matter of taste.
^ permalink raw reply
* Re: [PATCH 0/5 net] bridge: Fix missing Netlink message validations
From: John Fastabend @ 2014-11-26 17:25 UTC (permalink / raw)
To: Thomas Graf; +Cc: Jiri Pirko, davem, stephen, netdev
In-Reply-To: <20141126170610.GA2399@casper.infradead.org>
On 11/26/2014 09:06 AM, Thomas Graf wrote:
> On 11/26/14 at 08:58am, John Fastabend wrote:
>> On 11/26/2014 04:42 AM, Thomas Graf wrote:
>>> Adds various missing length checks in the bridging code for Netlink
>>> messages and corresponding attributes provided by user space.
>>>
>>> Thomas Graf (5):
>>> bridge: Validate IFLA_BRIDGE_FLAGS attribute length
>>> net: Validate IFLA_BRIDGE_MODE attribute length
>>> net: Check for presence of IFLA_AF_SPEC
>>> bridge: Add missing policy entry for IFLA_BRPORT_FAST_LEAVE
>>> bridge: Sanitize IFLA_EXT_MASK for AF_BRIDGE:RTM_GETLINK
>>>
>>> drivers/net/ethernet/emulex/benet/be_main.c | 5 +++++
>>> drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 5 +++++
>>> net/bridge/br_netlink.c | 1 +
>>> net/core/rtnetlink.c | 23 ++++++++++++++++++-----
>>> 4 files changed, 29 insertions(+), 5 deletions(-)
>>>
>>
>> +Jiri
>>
>> Looks like a miss in bond_netlink also? Seems like writing
>> a smatch or cocci check for this would be worthwhile.
>
> Thanks, I'll take a look.
>
> The cocci check is somewhat difficult as validation is often
> centralized and decoupled from actual access to implement atomic
> operations. I'll give it a try though.
>
Sounds good, if I get some time I could give it a try as well.
Also another missing one here or at least I'm not seeing it if its
there,
> --- a/net/ipv4/devinet.c
> +++ b/net/ipv4/devinet.c
> @@ -1687,8 +1687,11 @@ static int inet_set_link_af(struct net_device *dev, const struct nlattr *nla)
> BUG();
>
> if (tb[IFLA_INET_CONF]) {
> - nla_for_each_nested(a, tb[IFLA_INET_CONF], rem)
> + nla_for_each_nested(a, tb[IFLA_INET_CONF], rem) {
> + if (nla_len(a) < sizeof(u32))
> + return -EINVAL;
> ipv4_devconf_set(in_dev, nla_type(a), nla_get_u32(a));
> + }
> }
--
John Fastabend Intel Corporation
^ permalink raw reply
* Re: [PATCH net-next 0/2] gue: Generalize remote checksum offload
From: David Miller @ 2014-11-26 17:27 UTC (permalink / raw)
To: therbert; +Cc: netdev
In-Reply-To: <1416943280-28473-1-git-send-email-therbert@google.com>
From: Tom Herbert <therbert@google.com>
Date: Tue, 25 Nov 2014 11:21:18 -0800
> The remote checksum offload is generalized by creating a common
> function (remcsum_adjust) that does the work of modifying the
> checksum in remote checksum offload. This function can be called
> from normal or GRO path. GUE was modified to use this function.
>
> Remote checksum offload is described in
> https://tools.ietf.org/html/draft-herbert-remotecsumoffload-01
>
> Tested by running 200 TCP_STREAM connections over GUE, did not see
> any problems with remote checksum offload enabled.
Series applied, thanks Tom.
^ permalink raw reply
* Re: [PATCH v2 01/17] new helper: skb_copy_and_csum_datagram_msg()
From: David Miller @ 2014-11-26 17:27 UTC (permalink / raw)
To: viro; +Cc: netdev, linux-kernel
In-Reply-To: <20141125205911.GA29748@ZenIV.linux.org.uk>
From: Al Viro <viro@ZenIV.linux.org.uk>
Date: Tue, 25 Nov 2014 20:59:11 +0000
> On Tue, Nov 25, 2014 at 02:28:20PM -0500, David Miller wrote:
>>
>> Al, this series looks fine to me, do you have a tree I can pull
>> it from?
>
> Er... Same as the last time?
>
> git://git.kernel.org/pub/scm/linux/kernel/git/viro/vfs.git for-davem
>
> and there's more fun stuff in #iov_iter-net in the same tree, but that's
> the next batch...
Pulled, thanks Al.
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox