* Re: [net-next: PATCH v2 5/5] net: mvpp2: enable ACPI support in the driver
From: Florian Fainelli @ 2018-01-02 17:22 UTC (permalink / raw)
To: Marcin Wojtas, Andrew Lunn
Cc: linux-kernel, linux-arm-kernel, netdev, linux-acpi,
Graeme Gregory, David S. Miller, Russell King - ARM Linux,
Rafael J. Wysocki, Antoine Ténart, Thomas Petazzoni,
Gregory Clément, Ezequiel Garcia, nadavh,
Neta Zur Hershkovits, Ard Biesheuvel, Grzegorz Jaszczyk,
Tomasz Nowicki
In-Reply-To: <CAPv3WKdD0HwdFVOBx-yeTCfcMF7DXxxFaUt+Zm06oxuVuty8Rg@mail.gmail.com>
On January 2, 2018 7:05:35 AM PST, Marcin Wojtas <mw@semihalf.com> wrote:
>2018-01-02 15:08 GMT+01:00 Andrew Lunn <andrew@lunn.ch>:
>>> Indeed in of_mdio_bus_register_phy, there is of_irq_get. This is
>more
>>> a discussion for a MDIO bus / ACPI patchset, but we either find a
>way
>>> to use IRQs with ACPI obtained from child nodes or for this world
>the
>>> functionality will be limited (at least for the beginning).
>>
>> Hi Marcin
>>
>> What i want to avoid is adding something which partially works, and
>> then have to throw it all away and start again in order to add full
>> support.
>>
>> If ACPI really limits interrupts to devices, maybe we need a totally
>> different representation of MDIO and PHYs in ACPI to what it used in
>> device tree? The same may be true for the Ethernet ports of the
>mvpp2?
>> They might have to be represented as real devices, not children of a
>> device? Maybe trying to map DT to ACPI on a one-to-one basis is the
>> wrong approach?
>>
>
>In terms of PP2 controller, I'd prefer to keep as much as possible to
>describing how real hardware looks like, i.e. single common controller
>with multiple ports as its children. Those considerations are
>reflected in the DT description shape and how the driver enumerates,
>which was part of the design of the initial support. Bending the
>driver (huge amount of shared initialization and resources) to
>multiple instances just for the sake of possible avoidance of IRQ
>description in ACPI is IMO a huge and unnecessary overkill.
True, although keep in mind that Device Tree, as implemented in Linux allows for a lot of flexibility in how parent/child nodes are represented and backed or not by a corresponding struct device. I suspect ACPI is much less permissive than that and we might want to have a struct device for the whole mvpp2 controller as well as for the child ports within that controller (something you could today with device tree too, just it would be more overhead). This does not necessarily have to influence the representation within the description language but we should not be biased by how the current implementation using Device Tree has shaped both representation and implementation.
--
Florian
^ permalink raw reply
* Re: [net-next PATCH] net: ptr_ring: otherwise safe empty checks can overrun array bounds
From: Michael S. Tsirkin @ 2018-01-02 17:17 UTC (permalink / raw)
To: David Miller; +Cc: john.fastabend, jakub.kicinski, xiyou.wangcong, jiri, netdev
In-Reply-To: <20180102190107-mutt-send-email-mst@kernel.org>
On Tue, Jan 02, 2018 at 07:01:33PM +0200, Michael S. Tsirkin wrote:
> On Tue, Jan 02, 2018 at 11:52:19AM -0500, David Miller wrote:
> > From: John Fastabend <john.fastabend@gmail.com>
> > Date: Wed, 27 Dec 2017 19:50:25 -0800
> >
> > > When running consumer and/or producer operations and empty checks in
> > > parallel its possible to have the empty check run past the end of the
> > > array. The scenario occurs when an empty check is run while
> > > __ptr_ring_discard_one() is in progress. Specifically after the
> > > consumer_head is incremented but before (consumer_head >= ring_size)
> > > check is made and the consumer head is zeroe'd.
> > >
> > > To resolve this, without having to rework how consumer/producer ops
> > > work on the array, simply add an extra dummy slot to the end of the
> > > array. Even if we did a rework to avoid the extra slot it looks
> > > like the normal case checks would suffer some so best to just
> > > allocate an extra pointer.
> > >
> > > Reported-by: Jakub Kicinski <jakub.kicinski@netronome.com>
> > > Fixes: c5ad119fb6c09 ("net: sched: pfifo_fast use skb_array")
> > > Signed-off-by: John Fastabend <john.fastabend@gmail.com>
> >
> > Applied, thanks John.
>
> I think that patch is wrong. I'd rather it got reverted.
And replaced with something like the following - stil untested, but
apparently there's some urgency to fix it so posting for review ASAP.
John, others, could you pls confirm it's not too bad performance-wise?
I'll then split it up properly and re-post.
-->
net: don't misuse ptr_ring_peek
ptr_ring_peek only claims to be safe if the result is never
dereferenced, which isn't the case for its use in sch_generic.
Add locked API variants and use the bh one here.
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
diff --git a/include/linux/ptr_ring.h b/include/linux/ptr_ring.h
index 6866df4..f3d96c7 100644
--- a/include/linux/ptr_ring.h
+++ b/include/linux/ptr_ring.h
@@ -236,6 +236,51 @@ static inline bool ptr_ring_empty_bh(struct ptr_ring *r)
return ret;
}
+static inline void *ptr_ring_peek(struct ptr_ring *r)
+{
+ void *ret;
+
+ spin_lock(&r->consumer_lock);
+ ret = __ptr_ring_peek(r);
+ spin_unlock(&r->consumer_lock);
+
+ return ret;
+}
+
+static inline void *ptr_ring_peek_irq(struct ptr_ring *r)
+{
+ void *ret;
+
+ spin_lock_irq(&r->consumer_lock);
+ ret = __ptr_ring_peek(r);
+ spin_unlock_irq(&r->consumer_lock);
+
+ return ret;
+}
+
+static inline void *ptr_ring_peek_any(struct ptr_ring *r)
+{
+ unsigned long flags;
+ void *ret;
+
+ spin_lock_irqsave(&r->consumer_lock, flags);
+ ret = __ptr_ring_peek(r);
+ spin_unlock_irqrestore(&r->consumer_lock, flags);
+
+ return ret;
+}
+
+static inline void *ptr_ring_peek_bh(struct ptr_ring *r)
+{
+ void *ret;
+
+ spin_lock_bh(&r->consumer_lock);
+ ret = __ptr_ring_peek(r);
+ spin_unlock_bh(&r->consumer_lock);
+
+ return ret;
+}
+
/* Must only be called after __ptr_ring_peek returned !NULL */
static inline void __ptr_ring_discard_one(struct ptr_ring *r)
{
diff --git a/include/linux/skb_array.h b/include/linux/skb_array.h
index c7addf3..ee3625c 100644
--- a/include/linux/skb_array.h
+++ b/include/linux/skb_array.h
@@ -97,6 +97,26 @@ static inline bool skb_array_empty_any(struct skb_array *a)
return ptr_ring_empty_any(&a->ring);
}
+static inline struct sk_buff *skb_array_peek(struct skb_array *a)
+{
+ return ptr_ring_peek(&a->ring);
+}
+
+static inline struct sk_buff *skb_array_peek_bh(struct skb_array *a)
+{
+ return ptr_ring_peek_bh(&a->ring);
+}
+
+static inline struct sk_buff *skb_array_peek_irq(struct skb_array *a)
+{
+ return ptr_ring_peek_irq(&a->ring);
+}
+
+static inline struct sk_buff *skb_array_peek_any(struct skb_array *a)
+{
+ return ptr_ring_peek_any(&a->ring);
+}
+
static inline struct sk_buff *skb_array_consume(struct skb_array *a)
{
return ptr_ring_consume(&a->ring);
diff --git a/net/sched/sch_generic.c b/net/sched/sch_generic.c
index cc069b2..9288e84 100644
--- a/net/sched/sch_generic.c
+++ b/net/sched/sch_generic.c
@@ -659,7 +659,7 @@ static struct sk_buff *pfifo_fast_peek(struct Qdisc *qdisc)
for (band = 0; band < PFIFO_FAST_BANDS && !skb; band++) {
struct skb_array *q = band2list(priv, band);
- skb = __skb_array_peek(q);
+ skb = skb_array_peek_bh(q);
}
return skb;
^ permalink raw reply related
* [net-next] ipv6: sr: export some functions of seg6local
From: Ahmed Abdelsalam @ 2017-12-29 21:09 UTC (permalink / raw)
To: davem, kuznet, yoshfuji; +Cc: david.lebrun, netdev, amsalam20
Some functions of seg6local are very useful to process SRv6
encapsulated packets.
This patch exports some functions of seg6local that are useful and
can be re-used at different parts of the kernel.
The set of exported functions are:
(1) get_srh()
(2) advance_nextseg()
(3) lookup_nexthop
Signed-off-by: Ahmed Abdelsalam <amsalam20@gmail.com>
---
I'm writing some extensions to netfilter framework to support
Segment Routing. These function are useful to process
SR-encapsulated packets
include/net/seg6.h | 4 ++++
net/ipv6/seg6_local.c | 11 +++++++----
2 files changed, 11 insertions(+), 4 deletions(-)
diff --git a/include/net/seg6.h b/include/net/seg6.h
index 099bad5..4058f23 100644
--- a/include/net/seg6.h
+++ b/include/net/seg6.h
@@ -60,6 +60,10 @@ extern int seg6_local_init(void);
extern void seg6_local_exit(void);
extern bool seg6_validate_srh(struct ipv6_sr_hdr *srh, int len);
+extern struct ipv6_sr_hdr *get_srh(struct sk_buff *skb);
+extern void advance_nextseg(struct ipv6_sr_hdr *srh, struct in6_addr *daddr);
+extern void lookup_nexthop(struct sk_buff *skb, struct in6_addr *nhaddr,
+ u32 tbl_id);
extern int seg6_do_srh_encap(struct sk_buff *skb, struct ipv6_sr_hdr *osrh,
int proto);
extern int seg6_do_srh_inline(struct sk_buff *skb, struct ipv6_sr_hdr *osrh);
diff --git a/net/ipv6/seg6_local.c b/net/ipv6/seg6_local.c
index 825b8e0..5661d6c 100644
--- a/net/ipv6/seg6_local.c
+++ b/net/ipv6/seg6_local.c
@@ -59,7 +59,7 @@ static struct seg6_local_lwt *seg6_local_lwtunnel(struct lwtunnel_state *lwt)
return (struct seg6_local_lwt *)lwt->data;
}
-static struct ipv6_sr_hdr *get_srh(struct sk_buff *skb)
+struct ipv6_sr_hdr *get_srh(struct sk_buff *skb)
{
struct ipv6_sr_hdr *srh;
int len, srhoff = 0;
@@ -82,6 +82,7 @@ static struct ipv6_sr_hdr *get_srh(struct sk_buff *skb)
return srh;
}
+EXPORT_SYMBOL_GPL(get_srh);
static struct ipv6_sr_hdr *get_and_validate_srh(struct sk_buff *skb)
{
@@ -131,7 +132,7 @@ static bool decap_and_validate(struct sk_buff *skb, int proto)
return true;
}
-static void advance_nextseg(struct ipv6_sr_hdr *srh, struct in6_addr *daddr)
+void advance_nextseg(struct ipv6_sr_hdr *srh, struct in6_addr *daddr)
{
struct in6_addr *addr;
@@ -139,9 +140,10 @@ static void advance_nextseg(struct ipv6_sr_hdr *srh, struct in6_addr *daddr)
addr = srh->segments + srh->segments_left;
*daddr = *addr;
}
+EXPORT_SYMBOL_GPL(advance_nextseg);
-static void lookup_nexthop(struct sk_buff *skb, struct in6_addr *nhaddr,
- u32 tbl_id)
+void lookup_nexthop(struct sk_buff *skb, struct in6_addr *nhaddr,
+ u32 tbl_id)
{
struct net *net = dev_net(skb->dev);
struct ipv6hdr *hdr = ipv6_hdr(skb);
@@ -188,6 +190,7 @@ static void lookup_nexthop(struct sk_buff *skb, struct in6_addr *nhaddr,
skb_dst_drop(skb);
skb_dst_set(skb, dst);
}
+EXPORT_SYMBOL_GPL(lookup_nexthop);
/* regular endpoint function */
static int input_action_end(struct sk_buff *skb, struct seg6_local_lwt *slwt)
--
2.1.4
^ permalink raw reply related
* Re: [net-next PATCH] net: ptr_ring: otherwise safe empty checks can overrun array bounds
From: Michael S. Tsirkin @ 2018-01-02 17:01 UTC (permalink / raw)
To: John Fastabend; +Cc: jakub.kicinski, davem, xiyou.wangcong, jiri, netdev
In-Reply-To: <20180102183322-mutt-send-email-mst@kernel.org>
On Tue, Jan 02, 2018 at 06:53:08PM +0200, Michael S. Tsirkin wrote:
> On Wed, Dec 27, 2017 at 07:50:25PM -0800, John Fastabend wrote:
> > When running consumer and/or producer operations and empty checks in
> > parallel its possible to have the empty check run past the end of the
> > array. The scenario occurs when an empty check is run while
> > __ptr_ring_discard_one() is in progress. Specifically after the
> > consumer_head is incremented but before (consumer_head >= ring_size)
> > check is made and the consumer head is zeroe'd.
> >
> > To resolve this, without having to rework how consumer/producer ops
> > work on the array, simply add an extra dummy slot to the end of the
> > array. Even if we did a rework to avoid the extra slot it looks
> > like the normal case checks would suffer some so best to just
> > allocate an extra pointer.
> >
> > Reported-by: Jakub Kicinski <jakub.kicinski@netronome.com>
> > Fixes: c5ad119fb6c09 ("net: sched: pfifo_fast use skb_array")
> > Signed-off-by: John Fastabend <john.fastabend@gmail.com>
>
>
>
>
> > ---
> > include/linux/ptr_ring.h | 7 ++++++-
> > 1 file changed, 6 insertions(+), 1 deletion(-)
> >
> > diff --git a/include/linux/ptr_ring.h b/include/linux/ptr_ring.h
> > index 6866df4..13fb06a 100644
> > --- a/include/linux/ptr_ring.h
> > +++ b/include/linux/ptr_ring.h
> > @@ -447,7 +447,12 @@ static inline int ptr_ring_consume_batched_bh(struct ptr_ring *r,
> >
> > static inline void **__ptr_ring_init_queue_alloc(unsigned int size, gfp_t gfp)
> > {
> > - return kcalloc(size, sizeof(void *), gfp);
> > + /* Allocate an extra dummy element at end of ring to avoid consumer head
> > + * or produce head access past the end of the array. Possible when
> > + * producer/consumer operations and __ptr_ring_peek operations run in
> > + * parallel.
> > + */
> > + return kcalloc(size + 1, sizeof(void *), gfp);
> > }
> >
> > static inline void __ptr_ring_set_size(struct ptr_ring *r, int size)
>
>
> Well the peek will return a false negative then, won't it?
>
> So I kind of prefer just fixing the consumer. The first step I think
> would look something like the below untested patch. Pls take a look. I
> suspect we'll need a memory barrier too.
>
> I wonder though: are false positives or negatives ever a problem?
>
> Would it be a big deal to just take a lock there, and
> avoid trying to support a lockless peek?
>
>
> It would definitely be more straight-forward to just
> remove the promise to support a lockless peek.
>
> Thoughts?
In fact, the API says:
* Callers must take consumer_lock
* if they dereference the pointer - see e.g. PTR_RING_PEEK_CALL.
* If ring is never resized, and if the pointer is merely
* tested, there's no need to take the lock - see e.g. __ptr_ring_empty.
So it looks like the API is actually misused here as callers
will dereferences the skb returned.
> -->
>
> ptr_ring: keep consumer_head valid at all times
>
> The comment near __ptr_ring_peek says:
>
> * If ring is never resized, and if the pointer is merely
> * tested, there's no need to take the lock - see e.g. __ptr_ring_empty.
>
> but this was in fact never possible.
>
> Fixes: c5ad119fb6c09 ("net: sched: pfifo_fast use skb_array")
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>
> ---
>
> diff --git a/include/linux/ptr_ring.h b/include/linux/ptr_ring.h
> index 37b4bb2..802375f 100644
> --- a/include/linux/ptr_ring.h
> +++ b/include/linux/ptr_ring.h
> @@ -236,22 +236,28 @@ static inline void __ptr_ring_discard_one(struct ptr_ring *r)
> /* Fundamentally, what we want to do is update consumer
> * index and zero out the entry so producer can reuse it.
> * Doing it naively at each consume would be as simple as:
> - * r->queue[r->consumer++] = NULL;
> - * if (unlikely(r->consumer >= r->size))
> - * r->consumer = 0;
> + * consumer = r->consumer;
> + * r->queue[consumer++] = NULL;
> + * if (unlikely(consumer >= r->size))
> + * consumer = 0;
> + * r->consumer = consumer;
> * but that is suboptimal when the ring is full as producer is writing
> * out new entries in the same cache line. Defer these updates until a
> * batch of entries has been consumed.
> */
> - int head = r->consumer_head++;
> + /* Note: we must keep consumer_head valid at all times for __ptr_ring_peek
> + * to work correctly.
> + */
> + int consumer_head = r->consumer_head;
> + int head = consumer_head++;
>
> /* Once we have processed enough entries invalidate them in
> * the ring all at once so producer can reuse their space in the ring.
> * We also do this when we reach end of the ring - not mandatory
> * but helps keep the implementation simple.
> */
> - if (unlikely(r->consumer_head - r->consumer_tail >= r->batch ||
> - r->consumer_head >= r->size)) {
> + if (unlikely(consumer_head - r->consumer_tail >= r->batch ||
> + consumer_head >= r->size)) {
> /* Zero out entries in the reverse order: this way we touch the
> * cache line that producer might currently be reading the last;
> * producer won't make progress and touch other cache lines
> @@ -259,12 +265,13 @@ static inline void __ptr_ring_discard_one(struct ptr_ring *r)
> */
> while (likely(head >= r->consumer_tail))
> r->queue[head--] = NULL;
> - r->consumer_tail = r->consumer_head;
> + r->consumer_tail = consumer_head;
> }
> - if (unlikely(r->consumer_head >= r->size)) {
> - r->consumer_head = 0;
> + if (unlikely(consumer_head >= r->size)) {
> + consumer_head = 0;
> r->consumer_tail = 0;
> }
> + r->consumer_head = consumer_head;
> }
>
> static inline void *__ptr_ring_consume(struct ptr_ring *r)
^ permalink raw reply
* Re: [net-next PATCH] net: ptr_ring: otherwise safe empty checks can overrun array bounds
From: Michael S. Tsirkin @ 2018-01-02 17:01 UTC (permalink / raw)
To: David Miller; +Cc: john.fastabend, jakub.kicinski, xiyou.wangcong, jiri, netdev
In-Reply-To: <20180102.115219.1101472320429215260.davem@davemloft.net>
On Tue, Jan 02, 2018 at 11:52:19AM -0500, David Miller wrote:
> From: John Fastabend <john.fastabend@gmail.com>
> Date: Wed, 27 Dec 2017 19:50:25 -0800
>
> > When running consumer and/or producer operations and empty checks in
> > parallel its possible to have the empty check run past the end of the
> > array. The scenario occurs when an empty check is run while
> > __ptr_ring_discard_one() is in progress. Specifically after the
> > consumer_head is incremented but before (consumer_head >= ring_size)
> > check is made and the consumer head is zeroe'd.
> >
> > To resolve this, without having to rework how consumer/producer ops
> > work on the array, simply add an extra dummy slot to the end of the
> > array. Even if we did a rework to avoid the extra slot it looks
> > like the normal case checks would suffer some so best to just
> > allocate an extra pointer.
> >
> > Reported-by: Jakub Kicinski <jakub.kicinski@netronome.com>
> > Fixes: c5ad119fb6c09 ("net: sched: pfifo_fast use skb_array")
> > Signed-off-by: John Fastabend <john.fastabend@gmail.com>
>
> Applied, thanks John.
I think that patch is wrong. I'd rather it got reverted.
^ permalink raw reply
* Re: [net-next PATCH] net: ptr_ring: otherwise safe empty checks can overrun array bounds
From: Michael S. Tsirkin @ 2018-01-02 16:53 UTC (permalink / raw)
To: John Fastabend; +Cc: jakub.kicinski, davem, xiyou.wangcong, jiri, netdev
In-Reply-To: <20171228035024.14699.69453.stgit@john-Precision-Tower-5810>
On Wed, Dec 27, 2017 at 07:50:25PM -0800, John Fastabend wrote:
> When running consumer and/or producer operations and empty checks in
> parallel its possible to have the empty check run past the end of the
> array. The scenario occurs when an empty check is run while
> __ptr_ring_discard_one() is in progress. Specifically after the
> consumer_head is incremented but before (consumer_head >= ring_size)
> check is made and the consumer head is zeroe'd.
>
> To resolve this, without having to rework how consumer/producer ops
> work on the array, simply add an extra dummy slot to the end of the
> array. Even if we did a rework to avoid the extra slot it looks
> like the normal case checks would suffer some so best to just
> allocate an extra pointer.
>
> Reported-by: Jakub Kicinski <jakub.kicinski@netronome.com>
> Fixes: c5ad119fb6c09 ("net: sched: pfifo_fast use skb_array")
> Signed-off-by: John Fastabend <john.fastabend@gmail.com>
> ---
> include/linux/ptr_ring.h | 7 ++++++-
> 1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/include/linux/ptr_ring.h b/include/linux/ptr_ring.h
> index 6866df4..13fb06a 100644
> --- a/include/linux/ptr_ring.h
> +++ b/include/linux/ptr_ring.h
> @@ -447,7 +447,12 @@ static inline int ptr_ring_consume_batched_bh(struct ptr_ring *r,
>
> static inline void **__ptr_ring_init_queue_alloc(unsigned int size, gfp_t gfp)
> {
> - return kcalloc(size, sizeof(void *), gfp);
> + /* Allocate an extra dummy element at end of ring to avoid consumer head
> + * or produce head access past the end of the array. Possible when
> + * producer/consumer operations and __ptr_ring_peek operations run in
> + * parallel.
> + */
> + return kcalloc(size + 1, sizeof(void *), gfp);
> }
>
> static inline void __ptr_ring_set_size(struct ptr_ring *r, int size)
Well the peek will return a false negative then, won't it?
So I kind of prefer just fixing the consumer. The first step I think
would look something like the below untested patch. Pls take a look. I
suspect we'll need a memory barrier too.
I wonder though: are false positives or negatives ever a problem?
Would it be a big deal to just take a lock there, and
avoid trying to support a lockless peek?
It would definitely be more straight-forward to just
remove the promise to support a lockless peek.
Thoughts?
-->
ptr_ring: keep consumer_head valid at all times
The comment near __ptr_ring_peek says:
* If ring is never resized, and if the pointer is merely
* tested, there's no need to take the lock - see e.g. __ptr_ring_empty.
but this was in fact never possible.
Fixes: c5ad119fb6c09 ("net: sched: pfifo_fast use skb_array")
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
diff --git a/include/linux/ptr_ring.h b/include/linux/ptr_ring.h
index 37b4bb2..802375f 100644
--- a/include/linux/ptr_ring.h
+++ b/include/linux/ptr_ring.h
@@ -236,22 +236,28 @@ static inline void __ptr_ring_discard_one(struct ptr_ring *r)
/* Fundamentally, what we want to do is update consumer
* index and zero out the entry so producer can reuse it.
* Doing it naively at each consume would be as simple as:
- * r->queue[r->consumer++] = NULL;
- * if (unlikely(r->consumer >= r->size))
- * r->consumer = 0;
+ * consumer = r->consumer;
+ * r->queue[consumer++] = NULL;
+ * if (unlikely(consumer >= r->size))
+ * consumer = 0;
+ * r->consumer = consumer;
* but that is suboptimal when the ring is full as producer is writing
* out new entries in the same cache line. Defer these updates until a
* batch of entries has been consumed.
*/
- int head = r->consumer_head++;
+ /* Note: we must keep consumer_head valid at all times for __ptr_ring_peek
+ * to work correctly.
+ */
+ int consumer_head = r->consumer_head;
+ int head = consumer_head++;
/* Once we have processed enough entries invalidate them in
* the ring all at once so producer can reuse their space in the ring.
* We also do this when we reach end of the ring - not mandatory
* but helps keep the implementation simple.
*/
- if (unlikely(r->consumer_head - r->consumer_tail >= r->batch ||
- r->consumer_head >= r->size)) {
+ if (unlikely(consumer_head - r->consumer_tail >= r->batch ||
+ consumer_head >= r->size)) {
/* Zero out entries in the reverse order: this way we touch the
* cache line that producer might currently be reading the last;
* producer won't make progress and touch other cache lines
@@ -259,12 +265,13 @@ static inline void __ptr_ring_discard_one(struct ptr_ring *r)
*/
while (likely(head >= r->consumer_tail))
r->queue[head--] = NULL;
- r->consumer_tail = r->consumer_head;
+ r->consumer_tail = consumer_head;
}
- if (unlikely(r->consumer_head >= r->size)) {
- r->consumer_head = 0;
+ if (unlikely(consumer_head >= r->size)) {
+ consumer_head = 0;
r->consumer_tail = 0;
}
+ r->consumer_head = consumer_head;
}
static inline void *__ptr_ring_consume(struct ptr_ring *r)
^ permalink raw reply related
* Re: [net-next PATCH] net: ptr_ring: otherwise safe empty checks can overrun array bounds
From: David Miller @ 2018-01-02 16:52 UTC (permalink / raw)
To: john.fastabend; +Cc: jakub.kicinski, mst, xiyou.wangcong, jiri, netdev
In-Reply-To: <20171228035024.14699.69453.stgit@john-Precision-Tower-5810>
From: John Fastabend <john.fastabend@gmail.com>
Date: Wed, 27 Dec 2017 19:50:25 -0800
> When running consumer and/or producer operations and empty checks in
> parallel its possible to have the empty check run past the end of the
> array. The scenario occurs when an empty check is run while
> __ptr_ring_discard_one() is in progress. Specifically after the
> consumer_head is incremented but before (consumer_head >= ring_size)
> check is made and the consumer head is zeroe'd.
>
> To resolve this, without having to rework how consumer/producer ops
> work on the array, simply add an extra dummy slot to the end of the
> array. Even if we did a rework to avoid the extra slot it looks
> like the normal case checks would suffer some so best to just
> allocate an extra pointer.
>
> Reported-by: Jakub Kicinski <jakub.kicinski@netronome.com>
> Fixes: c5ad119fb6c09 ("net: sched: pfifo_fast use skb_array")
> Signed-off-by: John Fastabend <john.fastabend@gmail.com>
Applied, thanks John.
^ permalink raw reply
* Re: [net] Revert "net: core: maybe return -EEXIST in __dev_alloc_name"
From: Johannes Berg @ 2018-01-02 16:52 UTC (permalink / raw)
To: David Miller, mpe; +Cc: linux, michael, j, netdev, linuxppc-dev
In-Reply-To: <20180102.115008.2038929402603091054.davem@davemloft.net>
On Tue, 2018-01-02 at 11:50 -0500, David Miller wrote:
> From: Michael Ellerman <mpe@ellerman.id.au>
> Date: Fri, 22 Dec 2017 15:22:22 +1100
>
> >> On Tue, Dec 19 2017, Michael Ellerman <michael@concordia.ellerman.id.au> wrote:
> >>> This revert seems to have broken networking on one of my powerpc
> >>> machines, according to git bisect.
> >>>
> >>> The symptom is DHCP fails and I don't get a link, I didn't dig any
> >>> further than that. I can if it's helpful.
> >>>
> >>> I think the problem is that 87c320e51519 ("net: core: dev_get_valid_name
> >>> is now the same as dev_alloc_name_ns") only makes sense while
> >>> d6f295e9def0 remains in the tree.
> >>
> >> I'm sorry about all of this, I really didn't think there would be such
> >> consequences of changing an errno return. Indeed, d6f29 was preparation
> >> for unifying the two functions that do the exact same thing (and how we
> >> ever got into that situation is somewhat unclear), except for
> >> their behaviour in the case the requested name already exists. So one of
> >> the two interfaces had to change its return value, and as I wrote, I
> >> thought EEXIST was the saner choice when an explicit name (no %d) had
> >> been requested.
> >
> > No worries.
> >
> >>> ie. before the entire series, dev_get_valid_name() would return EEXIST,
> >>> and that was retained when 87c320e51519 was merged, but now that
> >>> d6f295e9def0 has been reverted dev_get_valid_name() is returning ENFILE.
> >>>
> >>> I can get the network up again if I also revert 87c320e51519 ("net:
> >>> core: dev_get_valid_name is now the same as dev_alloc_name_ns"), or with
> >>> the gross patch below.
> >>
> >> I don't think changing -ENFILE to -EEXIST would be right either, since
> >> dev_get_valid_name() used to be able to return both (-EEXIST in the case
> >> where there's no %d, -ENFILE in the case where we end up calling
> >> dev_alloc_name_ns()). If anything, we could do the check for the old
> >> -EEXIST condition first, and then call dev_alloc_name_ns(). But I'm also
> >> fine with reverting.
> >
> > Yeah I think a revert would be best, given it's nearly rc5.
> >
> > My userspace is not exotic AFAIK, just debian something, so presumably
> > this will affect other people too.
>
> I've just queued up the following revert, thanks!
>
> ====================
> From 5047543928139184f060c8f3bccb788b3df4c1ea Mon Sep 17 00:00:00 2001
> From: "David S. Miller" <davem@davemloft.net>
> Date: Tue, 2 Jan 2018 11:45:07 -0500
> Subject: [PATCH] Revert "net: core: dev_get_valid_name is now the same as
> dev_alloc_name_ns"
>
> This reverts commit 87c320e51519a83c496ab7bfb4e96c8f9c001e89.
>
> Changing the error return code in some situations turns out to
> be harmful in practice. In particular Michael Ellerman reports
> that DHCP fails on his powerpc machines, and this revert gets
> things working again.
>
> Johannes Berg agrees that this revert is the best course of
> action for now.
I'm not sure my voice matters much, I merely did the first revert of
these two patches ... :)
But I agree with Michael that you can't really salvage this without the
other patch, and that one caused problems in wifi ...
Thanks :)
johannes
^ permalink raw reply
* Re: [net] Revert "net: core: maybe return -EEXIST in __dev_alloc_name"
From: David Miller @ 2018-01-02 16:50 UTC (permalink / raw)
To: mpe; +Cc: linux, michael, j, netdev, johannes, linuxppc-dev, johannes.berg
In-Reply-To: <87bmirpf29.fsf@concordia.ellerman.id.au>
From: Michael Ellerman <mpe@ellerman.id.au>
Date: Fri, 22 Dec 2017 15:22:22 +1100
>> On Tue, Dec 19 2017, Michael Ellerman <michael@concordia.ellerman.id.au> wrote:
>>> This revert seems to have broken networking on one of my powerpc
>>> machines, according to git bisect.
>>>
>>> The symptom is DHCP fails and I don't get a link, I didn't dig any
>>> further than that. I can if it's helpful.
>>>
>>> I think the problem is that 87c320e51519 ("net: core: dev_get_valid_name
>>> is now the same as dev_alloc_name_ns") only makes sense while
>>> d6f295e9def0 remains in the tree.
>>
>> I'm sorry about all of this, I really didn't think there would be such
>> consequences of changing an errno return. Indeed, d6f29 was preparation
>> for unifying the two functions that do the exact same thing (and how we
>> ever got into that situation is somewhat unclear), except for
>> their behaviour in the case the requested name already exists. So one of
>> the two interfaces had to change its return value, and as I wrote, I
>> thought EEXIST was the saner choice when an explicit name (no %d) had
>> been requested.
>
> No worries.
>
>>> ie. before the entire series, dev_get_valid_name() would return EEXIST,
>>> and that was retained when 87c320e51519 was merged, but now that
>>> d6f295e9def0 has been reverted dev_get_valid_name() is returning ENFILE.
>>>
>>> I can get the network up again if I also revert 87c320e51519 ("net:
>>> core: dev_get_valid_name is now the same as dev_alloc_name_ns"), or with
>>> the gross patch below.
>>
>> I don't think changing -ENFILE to -EEXIST would be right either, since
>> dev_get_valid_name() used to be able to return both (-EEXIST in the case
>> where there's no %d, -ENFILE in the case where we end up calling
>> dev_alloc_name_ns()). If anything, we could do the check for the old
>> -EEXIST condition first, and then call dev_alloc_name_ns(). But I'm also
>> fine with reverting.
>
> Yeah I think a revert would be best, given it's nearly rc5.
>
> My userspace is not exotic AFAIK, just debian something, so presumably
> this will affect other people too.
I've just queued up the following revert, thanks!
====================
>From 5047543928139184f060c8f3bccb788b3df4c1ea Mon Sep 17 00:00:00 2001
From: "David S. Miller" <davem@davemloft.net>
Date: Tue, 2 Jan 2018 11:45:07 -0500
Subject: [PATCH] Revert "net: core: dev_get_valid_name is now the same as
dev_alloc_name_ns"
This reverts commit 87c320e51519a83c496ab7bfb4e96c8f9c001e89.
Changing the error return code in some situations turns out to
be harmful in practice. In particular Michael Ellerman reports
that DHCP fails on his powerpc machines, and this revert gets
things working again.
Johannes Berg agrees that this revert is the best course of
action for now.
Fixes: 029b6d140550 ("Revert "net: core: maybe return -EEXIST in __dev_alloc_name"")
Reported-by: Michael Ellerman <mpe@ellerman.id.au>
Signed-off-by: David S. Miller <davem@davemloft.net>
---
net/core/dev.c | 14 +++++++++++++-
1 file changed, 13 insertions(+), 1 deletion(-)
diff --git a/net/core/dev.c b/net/core/dev.c
index 01ee854454a8..0e0ba36eeac9 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -1146,7 +1146,19 @@ EXPORT_SYMBOL(dev_alloc_name);
int dev_get_valid_name(struct net *net, struct net_device *dev,
const char *name)
{
- return dev_alloc_name_ns(net, dev, name);
+ BUG_ON(!net);
+
+ if (!dev_valid_name(name))
+ return -EINVAL;
+
+ if (strchr(name, '%'))
+ return dev_alloc_name_ns(net, dev, name);
+ else if (__dev_get_by_name(net, name))
+ return -EEXIST;
+ else if (dev->name != name)
+ strlcpy(dev->name, name, IFNAMSIZ);
+
+ return 0;
}
EXPORT_SYMBOL(dev_get_valid_name);
--
2.14.3
^ permalink raw reply related
* [PATCH net-next] net: mdio: Only perform gpio reset for PHYs
From: Andrew Lunn @ 2018-01-02 16:40 UTC (permalink / raw)
To: David Miller
Cc: Florian Fainelli, sean.wang, sergei.shtylyov, geert+renesas,
netdev, Andrew Lunn
Ethernet switch on the MDIO bus have historically performed their own
handling of the GPIO reset line. The resent patch to have the MDIO
core handle the reset has broken the switch drivers, in that they
cannot claim the GPIO. Some switch drivers need more control over the
GPIO line than what the MDIO core provides. So restore the historical
behaviour by only performing a reset of PHYs, not switches.
Fixes: bafbdd527d56 ("phylib: Add device reset GPIO support")
Reported-by: Sean Wang <sean.wang@mediatek.com>
Signed-off-by: Andrew Lunn <andrew@lunn.ch>
---
drivers/net/phy/mdio_bus.c | 21 +++++++++++++++++----
1 file changed, 17 insertions(+), 4 deletions(-)
diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c
index a0f34c385cad..6afb1b578aec 100644
--- a/drivers/net/phy/mdio_bus.c
+++ b/drivers/net/phy/mdio_bus.c
@@ -47,13 +47,10 @@
#include "mdio-boardinfo.h"
-int mdiobus_register_device(struct mdio_device *mdiodev)
+static int mdiobus_register_gpiod(struct mdio_device *mdiodev)
{
struct gpio_desc *gpiod = NULL;
- if (mdiodev->bus->mdio_map[mdiodev->addr])
- return -EBUSY;
-
/* Deassert the optional reset signal */
if (mdiodev->dev.of_node)
gpiod = fwnode_get_named_gpiod(&mdiodev->dev.of_node->fwnode,
@@ -69,6 +66,22 @@ int mdiobus_register_device(struct mdio_device *mdiodev)
/* Assert the reset signal again */
mdio_device_reset(mdiodev, 1);
+ return 0;
+}
+
+int mdiobus_register_device(struct mdio_device *mdiodev)
+{
+ int err;
+
+ if (mdiodev->bus->mdio_map[mdiodev->addr])
+ return -EBUSY;
+
+ if (mdiodev->flags & MDIO_DEVICE_FLAG_PHY) {
+ err = mdiobus_register_gpiod(mdiodev);
+ if (err)
+ return err;
+ }
+
mdiodev->bus->mdio_map[mdiodev->addr] = mdiodev;
return 0;
--
2.15.1
^ permalink raw reply related
* Re: [Patch net-next] net_sched: call qdisc_reset() with qdisc lock
From: David Miller @ 2018-01-02 16:39 UTC (permalink / raw)
To: xiyou.wangcong; +Cc: netdev, jakub.kicinski, john.fastabend
In-Reply-To: <20171222000330.29009-1-xiyou.wangcong@gmail.com>
From: Cong Wang <xiyou.wangcong@gmail.com>
Date: Thu, 21 Dec 2017 16:03:29 -0800
> qdisc_reset() should always be called with qdisc spinlock
> and with BH disabled, otherwise qdisc ->reset() could race
> with TX BH.
>
> Fixes: 7bbde83b1860 ("net: sched: drop qdisc_reset from dev_graft_qdisc")
> Reported-by: Jakub Kicinski <jakub.kicinski@netronome.com>
> Cc: John Fastabend <john.fastabend@gmail.com>
> Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
There doesn't seem to be agreement over whether qdisc_reset() really
needs to run with the lock held. In fact, the general consensus is
that this really shouldn't run until a grace period had occurred and
therefore parallel TX paths cannot be running any longer.
In any event, this was supposed to work towards a bug fix which
ultimately was fixed instead with the ptr_ring change from John.
So this seems unnecessary now.
If you disagree, please repost with an updated commit message which
explains things further.
Thank you.
^ permalink raw reply
* Re: [Patch net-next] net_sched: remove the unsafe __skb_array_empty()
From: David Miller @ 2018-01-02 16:37 UTC (permalink / raw)
To: xiyou.wangcong; +Cc: netdev, jakub.kicinski, john.fastabend
In-Reply-To: <20171222000330.29009-2-xiyou.wangcong@gmail.com>
From: Cong Wang <xiyou.wangcong@gmail.com>
Date: Thu, 21 Dec 2017 16:03:30 -0800
> __skb_array_empty() is only safe if array is never resized.
> pfifo_fast_dequeue() is called in TX BH context and without
> qdisc lock, so even after we disable BH on ->reset() path
> we can still race with other CPU's.
>
> Fixes: c5ad119fb6c0 ("net: sched: pfifo_fast use skb_array")
> Reported-by: Jakub Kicinski <jakub.kicinski@netronome.com>
> Cc: John Fastabend <john.fastabend@gmail.com>
> Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
Based upon the discussion over this patch, this bug is ultimately fixed
by John's patch which adds a dummy element at the end of allocated
ptr_ring queues.
And I've just applied that.
^ permalink raw reply
* [PATCH] net: ipv4: emulate READ_ONCE() on ->hdrincl bit-field in raw_sendmsg()
From: Nicolai Stange @ 2018-01-02 16:30 UTC (permalink / raw)
To: David S. Miller, Alexey Kuznetsov, Hideaki YOSHIFUJI
Cc: Mohamed Ghannam, Michal Kubecek, Miroslav Benes, netdev,
linux-kernel, Nicolai Stange
Commit 8f659a03a0ba ("net: ipv4: fix for a race condition in
raw_sendmsg") fixed the issue of possibly inconsistent ->hdrincl handling
due to concurrent updates by reading this bit-field member into a local
variable and using the thus stabilized value in subsequent tests.
However, aforementioned commit also adds the (correct) comment that
/* hdrincl should be READ_ONCE(inet->hdrincl)
* but READ_ONCE() doesn't work with bit fields
*/
because as it stands, the compiler is free to shortcut or even eliminate
the local variable at its will.
Note that I have not seen anything like this happening in reality and thus,
the concern is a theoretical one.
However, in order to be on the safe side, emulate a READ_ONCE() on the
bit-field by introducing an intermediate local variable and doing a
READ_ONCE() from it:
int __hdrincl = inet->hdrincl;
int hdrincl = READ_ONCE(__hdrincl);
This breaks the chain in the sense that the compiler is not allowed
to replace subsequent reads from hdrincl with reloads from inet->hdrincl.
Fixes: 8f659a03a0ba ("net: ipv4: fix for a race condition in raw_sendmsg")
Signed-off-by: Nicolai Stange <nstange@suse.de>
---
Compile-tested only (with inspection of compiler output on x86_64).
Applicable to linux-next-20180102.
net/ipv4/raw.c | 8 +++++---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/net/ipv4/raw.c b/net/ipv4/raw.c
index 5b9bd5c33d9d..e84290c28c0c 100644
--- a/net/ipv4/raw.c
+++ b/net/ipv4/raw.c
@@ -513,16 +513,18 @@ static int raw_sendmsg(struct sock *sk, struct msghdr *msg, size_t len)
int err;
struct ip_options_data opt_copy;
struct raw_frag_vec rfv;
- int hdrincl;
+ int hdrincl, __hdrincl;
err = -EMSGSIZE;
if (len > 0xFFFF)
goto out;
/* hdrincl should be READ_ONCE(inet->hdrincl)
- * but READ_ONCE() doesn't work with bit fields
+ * but READ_ONCE() doesn't work with bit fields.
+ * Emulate it by doing the READ_ONCE() from an intermediate int.
*/
- hdrincl = inet->hdrincl;
+ __hdrincl = inet->hdrincl;
+ hdrincl = READ_ONCE(__hdrincl);
/*
* Check the flags.
*/
--
2.13.6
^ permalink raw reply related
* Re: [RFC PATCH net-next 04/19] ipv6: Prepare to handle multiple netdev events
From: David Ahern @ 2018-01-02 16:29 UTC (permalink / raw)
To: Ido Schimmel, netdev; +Cc: davem, roopa, nicolas.dichtel, mlxsw
In-Reply-To: <20171231161513.25785-5-idosch@mellanox.com>
On 12/31/17 9:14 AM, Ido Schimmel wrote:
> To make IPv6 more in line with IPv4 we need to be able to respond
> differently to different netdev events. For example, when a netdev is
> unregistered all the routes using it as their nexthop device should be
> flushed, whereas when the netdev's carrier changes only the 'linkdown'
> flag should be toggled.
>
> Currently, this is not possible, as the function that traverses the
> routing tables is not aware of the triggering event.
>
> Propagate the triggering event down, so that it could be used in later
> patches.
>
> Signed-off-by: Ido Schimmel <idosch@mellanox.com>
> ---
> include/net/ip6_route.h | 1 +
> net/ipv6/addrconf.c | 4 ++--
> net/ipv6/route.c | 35 ++++++++++++++++++++---------------
> 3 files changed, 23 insertions(+), 17 deletions(-)
>
> diff --git a/include/net/ip6_route.h b/include/net/ip6_route.h
> index caad39198c2a..8205402ff3dc 100644
> --- a/include/net/ip6_route.h
> +++ b/include/net/ip6_route.h
rt6_ifdown is done with this change, so the declaration of rt6_ifdown
can be removed from this file as well.
> @@ -170,6 +170,7 @@ void rt6_mtu_change(struct net_device *dev, unsigned int mtu);
> void rt6_remove_prefsrc(struct inet6_ifaddr *ifp);
> void rt6_clean_tohost(struct net *net, struct in6_addr *gateway);
> void rt6_sync_up(struct net_device *dev, unsigned int nh_flags);
> +void rt6_disable_ip(struct net_device *dev, unsigned long event);
>
> static inline const struct rt6_info *skb_rt6_info(const struct sk_buff *skb)
> {
^ permalink raw reply
* Re: Linux ECN Handling
From: Neal Cardwell @ 2018-01-02 16:27 UTC (permalink / raw)
To: Steve Ibanez
Cc: Eric Dumazet, Yuchung Cheng, Daniel Borkmann, Netdev,
Florian Westphal, Mohammad Alizadeh, Lawrence Brakmo
In-Reply-To: <CACJspmLiYAG6Z7Zzn+J_DQ=QVs=zXp8ttBihvO8LHDsassapXg@mail.gmail.com>
On Tue, Jan 2, 2018 at 2:43 AM, Steve Ibanez <sibanez@stanford.edu> wrote:
> Hi Neal,
>
> Apologies for the delay, and happy new year!
>
> To answer your question, data is only transferred in one direction
> (from the client to the server). The SeqNo in the pkts from the server
> to the client is not incremented. So I don't think that a data pkt is
> attempted to be sent in the tcp_data_snd_check() call clearing the
> ICSK_ACK_SCHED bit. Although I think it would be helpful to include
> your new debugging field in the tcp_sock (tp->processing_cwr) so that
> I can check this field in the tcp_transmit_skb() and tcp_send_ack()
> functions. I added the new field and tried to set it at the top of the
> tcp_rcv_established(), but then when I try to check the field in the
> tcp_send_ack() function it never appears to be set. Below I'm showing
> how I set the tp->processing_cwr field in the tcp_rcv_established
> function and how I check it in the tcp_send_ack function. Is this how
> you were imagining the processing_cwr field to be used?
Happy new year to you as well, and thank you, Steve, for running this
experiment! Yes, this is basically the kind of thing I had in mind.
The connection will run the "fast path" tcp_rcv_established() code if
the connection is in the ESTABLISHED state. From the symptoms it
sounds like what's happening is that in this test the connection is
not in the ESTABLISHED state when the CWR arrives, so it's probably
running the more general tcp_rcv_state_process() function. I would
suggest adding your tp->processing_cwr instrumentation at the top of
tcp_rcv_state_process() as well, and then re-running the test. (In
tcp_v4_do_rcv() and tcp_v6_do_rcv(), for each incoming skb one of
those two functions is called).
It is interesting that the connection does not seem to be in the
ESTABLISHED state. Maybe that is an ingredient of the unexpected
behavior in this case...
Thanks!
neal
^ permalink raw reply
* Re: [RFC PATCH net-next 03/19] ipv6: Clear nexthop flags upon netdev up
From: David Ahern @ 2018-01-02 16:20 UTC (permalink / raw)
To: Ido Schimmel, netdev; +Cc: davem, roopa, nicolas.dichtel, mlxsw
In-Reply-To: <20171231161513.25785-4-idosch@mellanox.com>
On 12/31/17 9:14 AM, Ido Schimmel wrote:
> Previous patch marked nexthops with the 'dead' and 'linkdown' flags.
> Clear these flags when the netdev comes back up.
>
> Signed-off-by: Ido Schimmel <idosch@mellanox.com>
> ---
> include/net/ip6_route.h | 1 +
> net/ipv6/addrconf.c | 3 +++
> net/ipv6/route.c | 29 +++++++++++++++++++++++++++++
> 3 files changed, 33 insertions(+)
>
> diff --git a/include/net/ip6_route.h b/include/net/ip6_route.h
> index 18e442ea93d8..caad39198c2a 100644
> --- a/include/net/ip6_route.h
> +++ b/include/net/ip6_route.h
> @@ -169,6 +169,7 @@ void rt6_ifdown(struct net *net, struct net_device *dev);
> void rt6_mtu_change(struct net_device *dev, unsigned int mtu);
> void rt6_remove_prefsrc(struct inet6_ifaddr *ifp);
> void rt6_clean_tohost(struct net *net, struct in6_addr *gateway);
> +void rt6_sync_up(struct net_device *dev, unsigned int nh_flags);
>
> static inline const struct rt6_info *skb_rt6_info(const struct sk_buff *skb)
> {
> diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
> index ed06b1190f05..b6405568ed7b 100644
> --- a/net/ipv6/addrconf.c
> +++ b/net/ipv6/addrconf.c
> @@ -3484,6 +3484,9 @@ static int addrconf_notify(struct notifier_block *this, unsigned long event,
> if (run_pending)
> addrconf_dad_run(idev);
>
> + /* Device has an address by now */
> + rt6_sync_up(dev, RTNH_F_DEAD);
> +
Seems like this should be in the NETDEV_UP section, say after
addrconf_permanent_addr.
^ permalink raw reply
* Re: [PATCH v6 1/6] can: dev: Add support for limiting configured bitrate
From: Marc Kleine-Budde @ 2018-01-02 16:15 UTC (permalink / raw)
To: Faiz Abbas, wg, robh+dt, mark.rutland
Cc: linux-can, netdev, devicetree, linux-kernel, nsekhar, fcooper,
robh, Wenyou.Yang, sergei.shtylyov
In-Reply-To: <1513949488-13026-2-git-send-email-faiz_abbas@ti.com>
[-- Attachment #1.1: Type: text/plain, Size: 1179 bytes --]
On 12/22/2017 02:31 PM, Faiz Abbas wrote:
> From: Franklin S Cooper Jr <fcooper@ti.com>
>
> Various CAN or CAN-FD IP may be able to run at a faster rate than
> what the transceiver the CAN node is connected to. This can lead to
> unexpected errors. However, CAN transceivers typically have fixed
> limitations and provide no means to discover these limitations at
> runtime. Therefore, add support for a can-transceiver node that
> can be reused by other CAN peripheral drivers to determine for both
> CAN and CAN-FD what the max bitrate that can be used. If the user
> tries to configure CAN to pass these maximum bitrates it will throw
> an error.
Please add support to read the maximum bitrate via netlink. Have a look
at 12a6075cabc0 ("can: dev: add CAN interface termination API"). I think
you need to extend the following functions: can_get_size() and
can_fill_info().
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Industrial Linux Solutions | Phone: +49-231-2826-924 |
Vertretung West/Dortmund | Fax: +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de |
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply
* Re: [PATCH net-next 5/5] net: phy: marvell10g: add support for half duplex 100M and 10M
From: Andrew Lunn @ 2018-01-02 16:13 UTC (permalink / raw)
To: Russell King; +Cc: Florian Fainelli, netdev
In-Reply-To: <E1eUu3f-0002Y8-5d@rmk-PC.armlinux.org.uk>
On Fri, Dec 29, 2017 at 12:46:43PM +0000, Russell King wrote:
> Add support for half-duplex 100M and 10M copper connections by parsing
> the advertisment results rather than trying to decode the negotiated
> speed from one of the PHYs "vendor" registers. This allows us to
> decode the duplex as well, which means we can support half-duplex mode
> for the slower speeds.
>
> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Andrew
^ permalink raw reply
* Re: [PATCH net-next 4/5] net: phy: add helper to convert negotiation result to phy settings
From: Andrew Lunn @ 2018-01-02 16:11 UTC (permalink / raw)
To: Russell King; +Cc: Florian Fainelli, netdev
In-Reply-To: <E1eUu3a-0002Xy-2H@rmk-PC.armlinux.org.uk>
On Fri, Dec 29, 2017 at 12:46:38PM +0000, Russell King wrote:
> Add a helper to convert the result of the autonegotiation advertisment
> into the PHYs speed and duplex settings. If the result is full duplex,
> also extract the pause mode settings from the link partner advertisment.
>
> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Andrew
^ permalink raw reply
* Re: [PATCH net-next 3/5] net: phy: marvell10g: clean up interface mode switching
From: Andrew Lunn @ 2018-01-02 16:09 UTC (permalink / raw)
To: Russell King; +Cc: Florian Fainelli, netdev
In-Reply-To: <E1eUu3U-0002Xr-UX@rmk-PC.armlinux.org.uk>
On Fri, Dec 29, 2017 at 12:46:32PM +0000, Russell King wrote:
> Centralise the PHY interface mode switching, rather than having it in
> two places.
>
> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Andrew
^ permalink raw reply
* Re: [PATCH net-next 2/5] net: phy: marvell10g: add MDI swap reporting
From: Andrew Lunn @ 2018-01-02 16:08 UTC (permalink / raw)
To: Russell King; +Cc: Florian Fainelli, netdev
In-Reply-To: <E1eUu3P-0002Xj-QW@rmk-PC.armlinux.org.uk>
On Fri, Dec 29, 2017 at 12:46:27PM +0000, Russell King wrote:
> Add reporting of the MDI swap to the Marvell 10G PHY driver by providing
> a generic implementation for the standard 10GBASE-T pair swap register
> and polarity register. We also support reading the MDI swap status for
> 1G and below from a PCS register.
>
> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Andrew
^ permalink raw reply
* Re: [PATCH v6 3/6] can: m_can: Add PM Runtime
From: Marc Kleine-Budde @ 2018-01-02 16:07 UTC (permalink / raw)
To: Faiz Abbas, wg, robh+dt, mark.rutland
Cc: linux-can, netdev, devicetree, linux-kernel, nsekhar, fcooper,
robh, Wenyou.Yang, sergei.shtylyov
In-Reply-To: <1513949488-13026-4-git-send-email-faiz_abbas@ti.com>
[-- Attachment #1.1: Type: text/plain, Size: 4391 bytes --]
On 12/22/2017 02:31 PM, Faiz Abbas wrote:
> From: Franklin S Cooper Jr <fcooper@ti.com>
>
> Add support for PM Runtime which is the new way to handle managing clocks.
> However, to avoid breaking SoCs not using PM_RUNTIME leave the old clk
> management approach in place.
There is no PM_RUNTIME anymore since 464ed18ebdb6 ("PM: Eliminate
CONFIG_PM_RUNTIME")
Have a look at the discussion: https://patchwork.kernel.org/patch/9436507/ :
>> Well, I admit it would be nicer if drivers didn't have to worry about
>> whether or not CONFIG_PM was enabled. A slightly cleaner approach
>> from the one outlined above would have the probe routine do this:
>>
>> my_power_up(dev);
>> pm_runtime_set_active(dev);
>> pm_runtime_get_noresume(dev);
>> pm_runtime_enable(dev);
> PM_RUNTIME is required by OMAP based devices to handle clock management.
> Therefore, this allows future Texas Instruments SoCs that have the MCAN IP
> to work with this driver.
Who will set the SET_RUNTIME_PM_OPS in this case?
> Signed-off-by: Franklin S Cooper Jr <fcooper@ti.com>
> [nsekhar@ti.com: handle pm_runtime_get_sync() failure, fix some bugs]
> Signed-off-by: Sekhar Nori <nsekhar@ti.com>
> Signed-off-by: Faiz Abbas <faiz_abbas@ti.com>
> ---
> drivers/net/can/m_can/m_can.c | 38 ++++++++++++++++++++++++++++++++++----
> 1 file changed, 34 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c
> index f72116e..53e764f 100644
> --- a/drivers/net/can/m_can/m_can.c
> +++ b/drivers/net/can/m_can/m_can.c
> @@ -23,6 +23,7 @@
> #include <linux/of.h>
> #include <linux/of_device.h>
> #include <linux/platform_device.h>
> +#include <linux/pm_runtime.h>
> #include <linux/iopoll.h>
> #include <linux/can/dev.h>
>
> @@ -625,19 +626,33 @@ static int m_can_clk_start(struct m_can_priv *priv)
> {
> int err;
>
> + err = pm_runtime_get_sync(priv->device);
> + if (err) {
> + pm_runtime_put_noidle(priv->device);
Why do you call this in case of an error?
> + return err;
> + }
> +
> err = clk_prepare_enable(priv->hclk);
> if (err)
> - return err;
> + goto pm_runtime_put;
>
> err = clk_prepare_enable(priv->cclk);
> if (err)
> - clk_disable_unprepare(priv->hclk);
> + goto disable_hclk;
>
> return err;
> +
> +disable_hclk:
> + clk_disable_unprepare(priv->hclk);
> +pm_runtime_put:
> + pm_runtime_put_sync(priv->device);
> + return err;
> }
>
> static void m_can_clk_stop(struct m_can_priv *priv)
> {
> + pm_runtime_put_sync(priv->device);
> +
> clk_disable_unprepare(priv->cclk);
> clk_disable_unprepare(priv->hclk);
> }
> @@ -1577,13 +1592,20 @@ static int m_can_plat_probe(struct platform_device *pdev)
> /* Enable clocks. Necessary to read Core Release in order to determine
> * M_CAN version
> */
> + pm_runtime_enable(&pdev->dev);
> + ret = pm_runtime_get_sync(&pdev->dev);
> + if (ret) {
> + pm_runtime_put_noidle(&pdev->dev);
Why do you call this in case of error?
> + goto pm_runtime_fail;
> + }
> +
> ret = clk_prepare_enable(hclk);
> if (ret)
> - goto disable_hclk_ret;
> + goto pm_runtime_put;
>
> ret = clk_prepare_enable(cclk);
> if (ret)
> - goto disable_cclk_ret;
> + goto disable_hclk_ret;
>
> res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "m_can");
> addr = devm_ioremap_resource(&pdev->dev, res);
> @@ -1666,6 +1688,11 @@ static int m_can_plat_probe(struct platform_device *pdev)
> clk_disable_unprepare(cclk);
> disable_hclk_ret:
> clk_disable_unprepare(hclk);
> +pm_runtime_put:
> + pm_runtime_put_sync(&pdev->dev);
> +pm_runtime_fail:
> + if (ret)
> + pm_runtime_disable(&pdev->dev);
> failed_ret:
> return ret;
> }
> @@ -1723,6 +1750,9 @@ static int m_can_plat_remove(struct platform_device *pdev)
> struct net_device *dev = platform_get_drvdata(pdev);
>
> unregister_m_can_dev(dev);
> +
> + pm_runtime_disable(&pdev->dev);
> +
> platform_set_drvdata(pdev, NULL);
>
> free_m_can_dev(dev);
>
regards,
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Industrial Linux Solutions | Phone: +49-231-2826-924 |
Vertretung West/Dortmund | Fax: +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de |
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply
* Re: [PATCH net-next 1/5] net: phy: marvell10g: update header comments
From: Andrew Lunn @ 2018-01-02 16:06 UTC (permalink / raw)
To: Russell King; +Cc: Florian Fainelli, netdev
In-Reply-To: <E1eUu3K-0002Xc-MY@rmk-PC.armlinux.org.uk>
On Fri, Dec 29, 2017 at 12:46:22PM +0000, Russell King wrote:
> Update header comments to indicate the newly found behaviour with XAUI
> interfaces.
>
> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Andrew
^ permalink raw reply
* Re: [PATCH net-next v2 7/7] net: phy: convert read-modify-write to phy_modify()
From: Andrew Lunn @ 2018-01-02 16:05 UTC (permalink / raw)
To: Russell King; +Cc: Florian Fainelli, netdev
In-Reply-To: <E1eWKHa-0003j2-Kd@rmk-PC.armlinux.org.uk>
On Tue, Jan 02, 2018 at 10:58:58AM +0000, Russell King wrote:
> Convert read-modify-write sequences in at803x, Marvell and core phylib
> to use phy_modify() to ensure safety.
>
> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Andrew
^ permalink raw reply
* Re: [PATCH net-next v2 6/7] net: phy: add phy_modify() accessor
From: Andrew Lunn @ 2018-01-02 16:01 UTC (permalink / raw)
To: Russell King; +Cc: Florian Fainelli, netdev
In-Reply-To: <E1eWKHV-0003iv-BH@rmk-PC.armlinux.org.uk>
On Tue, Jan 02, 2018 at 10:58:53AM +0000, Russell King wrote:
> Add phy_modify() convenience accessor to complement the mdiobus
> counterpart.
>
> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Andrew
^ 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