* [PATCH net-next 3/7] s390/qeth: collect accurate TX statistics
From: Julian Wiedmann @ 2019-08-23 9:48 UTC (permalink / raw)
To: David Miller
Cc: netdev, linux-s390, Heiko Carstens, Stefan Raspl, Ursula Braun,
Julian Wiedmann
In-Reply-To: <20190823094853.63814-1-jwi@linux.ibm.com>
This consolidates the SW statistics code, and improves it to
(1) account for the header overhead of each segment on a TSO skb,
(2) count dangling packets as in-error (during eg. shutdown), and
(3) only count offloads when the skb was successfully transmitted.
We also count each segment of an TSO skb as one packet - except for
tx_dropped, to be consistent with dev->tx_dropped.
Signed-off-by: Julian Wiedmann <jwi@linux.ibm.com>
---
drivers/s390/net/qeth_core.h | 1 +
drivers/s390/net/qeth_core_main.c | 66 +++++++++++++++++++------------
drivers/s390/net/qeth_l2_main.c | 12 ++----
drivers/s390/net/qeth_l3_main.c | 9 ++---
4 files changed, 49 insertions(+), 39 deletions(-)
diff --git a/drivers/s390/net/qeth_core.h b/drivers/s390/net/qeth_core.h
index 72755a025b4d..a0911ce55db3 100644
--- a/drivers/s390/net/qeth_core.h
+++ b/drivers/s390/net/qeth_core.h
@@ -30,6 +30,7 @@
#include <net/ipv6.h>
#include <net/if_inet6.h>
#include <net/addrconf.h>
+#include <net/sch_generic.h>
#include <net/tcp.h>
#include <asm/debug.h>
diff --git a/drivers/s390/net/qeth_core_main.c b/drivers/s390/net/qeth_core_main.c
index 44fbaa4f7264..d7a15a88bdba 100644
--- a/drivers/s390/net/qeth_core_main.c
+++ b/drivers/s390/net/qeth_core_main.c
@@ -71,7 +71,7 @@ static void qeth_free_qdio_queues(struct qeth_card *card);
static void qeth_notify_skbs(struct qeth_qdio_out_q *queue,
struct qeth_qdio_out_buffer *buf,
enum iucv_tx_notify notification);
-static void qeth_release_skbs(struct qeth_qdio_out_buffer *buf);
+static void qeth_tx_complete_buf(struct qeth_qdio_out_buffer *buf, bool error);
static int qeth_init_qdio_out_buf(struct qeth_qdio_out_q *, int);
static void qeth_close_dev_handler(struct work_struct *work)
@@ -411,7 +411,7 @@ static void qeth_cleanup_handled_pending(struct qeth_qdio_out_q *q, int bidx,
/* release here to avoid interleaving between
outbound tasklet and inbound tasklet
regarding notifications and lifecycle */
- qeth_release_skbs(c);
+ qeth_tx_complete_buf(c, forced_cleanup);
c = f->next_pending;
WARN_ON_ONCE(head->next_pending != f);
@@ -1077,22 +1077,51 @@ static void qeth_notify_skbs(struct qeth_qdio_out_q *q,
}
}
-static void qeth_release_skbs(struct qeth_qdio_out_buffer *buf)
+static void qeth_tx_complete_buf(struct qeth_qdio_out_buffer *buf, bool error)
{
+ struct qeth_qdio_out_q *queue = buf->q;
struct sk_buff *skb;
/* release may never happen from within CQ tasklet scope */
WARN_ON_ONCE(atomic_read(&buf->state) == QETH_QDIO_BUF_IN_CQ);
if (atomic_read(&buf->state) == QETH_QDIO_BUF_PENDING)
- qeth_notify_skbs(buf->q, buf, TX_NOTIFY_GENERALERROR);
+ qeth_notify_skbs(queue, buf, TX_NOTIFY_GENERALERROR);
+
+ /* Empty buffer? */
+ if (buf->next_element_to_fill == 0)
+ return;
+
+ QETH_TXQ_STAT_INC(queue, bufs);
+ QETH_TXQ_STAT_ADD(queue, buf_elements, buf->next_element_to_fill);
+ while ((skb = __skb_dequeue(&buf->skb_list)) != NULL) {
+ unsigned int bytes = qdisc_pkt_len(skb);
+ bool is_tso = skb_is_gso(skb);
+ unsigned int packets;
+
+ packets = is_tso ? skb_shinfo(skb)->gso_segs : 1;
+ if (error) {
+ QETH_TXQ_STAT_ADD(queue, tx_errors, packets);
+ } else {
+ QETH_TXQ_STAT_ADD(queue, tx_packets, packets);
+ QETH_TXQ_STAT_ADD(queue, tx_bytes, bytes);
+ if (skb->ip_summed == CHECKSUM_PARTIAL)
+ QETH_TXQ_STAT_ADD(queue, skbs_csum, packets);
+ if (skb_is_nonlinear(skb))
+ QETH_TXQ_STAT_INC(queue, skbs_sg);
+ if (is_tso) {
+ QETH_TXQ_STAT_INC(queue, skbs_tso);
+ QETH_TXQ_STAT_ADD(queue, tso_bytes, bytes);
+ }
+ }
- while ((skb = __skb_dequeue(&buf->skb_list)) != NULL)
consume_skb(skb);
+ }
}
static void qeth_clear_output_buffer(struct qeth_qdio_out_q *queue,
- struct qeth_qdio_out_buffer *buf)
+ struct qeth_qdio_out_buffer *buf,
+ bool error)
{
int i;
@@ -1100,7 +1129,7 @@ static void qeth_clear_output_buffer(struct qeth_qdio_out_q *queue,
if (buf->buffer->element[0].sflags & SBAL_SFLAGS0_PCI_REQ)
atomic_dec(&queue->set_pci_flags_count);
- qeth_release_skbs(buf);
+ qeth_tx_complete_buf(buf, error);
for (i = 0; i < queue->max_elements; ++i) {
if (buf->buffer->element[i].addr && buf->is_header[i])
@@ -1122,7 +1151,7 @@ static void qeth_drain_output_queue(struct qeth_qdio_out_q *q, bool free)
if (!q->bufs[j])
continue;
qeth_cleanup_handled_pending(q, j, 1);
- qeth_clear_output_buffer(q, q->bufs[j]);
+ qeth_clear_output_buffer(q, q->bufs[j], true);
if (free) {
kmem_cache_free(qeth_qdio_outbuf_cache, q->bufs[j]);
q->bufs[j] = NULL;
@@ -3240,14 +3269,12 @@ static void qeth_flush_buffers(struct qeth_qdio_out_q *queue, int index,
}
}
- QETH_TXQ_STAT_ADD(queue, bufs, count);
qdio_flags = QDIO_FLAG_SYNC_OUTPUT;
if (atomic_read(&queue->set_pci_flags_count))
qdio_flags |= QDIO_FLAG_PCI_OUT;
rc = do_QDIO(CARD_DDEV(queue->card), qdio_flags,
queue->queue_no, index, count);
if (rc) {
- QETH_TXQ_STAT_ADD(queue, tx_errors, count);
/* ignore temporary SIGA errors without busy condition */
if (rc == -ENOBUFS)
return;
@@ -3456,7 +3483,7 @@ static void qeth_qdio_output_handler(struct ccw_device *ccwdev,
qeth_notify_skbs(queue, buffer, n);
}
- qeth_clear_output_buffer(queue, buffer);
+ qeth_clear_output_buffer(queue, buffer, qdio_error);
}
qeth_cleanup_handled_pending(queue, bidx, 0);
}
@@ -3942,7 +3969,6 @@ int qeth_xmit(struct qeth_card *card, struct sk_buff *skb,
unsigned int hd_len = 0;
unsigned int elements;
int push_len, rc;
- bool is_sg;
if (is_tso) {
hw_hdr_len = sizeof(struct qeth_hdr_tso);
@@ -3971,7 +3997,6 @@ int qeth_xmit(struct qeth_card *card, struct sk_buff *skb,
qeth_fill_tso_ext((struct qeth_hdr_tso *) hdr,
frame_len - proto_len, skb, proto_len);
- is_sg = skb_is_nonlinear(skb);
if (IS_IQD(card)) {
rc = qeth_do_send_packet_fast(queue, skb, hdr, data_offset,
hd_len);
@@ -3982,18 +4007,9 @@ int qeth_xmit(struct qeth_card *card, struct sk_buff *skb,
hd_len, elements);
}
- if (!rc) {
- QETH_TXQ_STAT_ADD(queue, buf_elements, elements);
- if (is_sg)
- QETH_TXQ_STAT_INC(queue, skbs_sg);
- if (is_tso) {
- QETH_TXQ_STAT_INC(queue, skbs_tso);
- QETH_TXQ_STAT_ADD(queue, tso_bytes, frame_len);
- }
- } else {
- if (!push_len)
- kmem_cache_free(qeth_core_header_cache, hdr);
- }
+ if (rc && !push_len)
+ kmem_cache_free(qeth_core_header_cache, hdr);
+
return rc;
}
EXPORT_SYMBOL_GPL(qeth_xmit);
diff --git a/drivers/s390/net/qeth_l2_main.c b/drivers/s390/net/qeth_l2_main.c
index 662bd51f922f..b8799cd3e7aa 100644
--- a/drivers/s390/net/qeth_l2_main.c
+++ b/drivers/s390/net/qeth_l2_main.c
@@ -175,10 +175,8 @@ static void qeth_l2_fill_header(struct qeth_qdio_out_q *queue,
hdr->hdr.l2.id = QETH_HEADER_TYPE_L2_TSO;
} else {
hdr->hdr.l2.id = QETH_HEADER_TYPE_LAYER2;
- if (skb->ip_summed == CHECKSUM_PARTIAL) {
+ if (skb->ip_summed == CHECKSUM_PARTIAL)
qeth_tx_csum(skb, &hdr->hdr.l2.flags[1], ipv);
- QETH_TXQ_STAT_INC(queue, skbs_csum);
- }
}
/* set byte byte 3 to casting flags */
@@ -588,9 +586,10 @@ static netdev_tx_t qeth_l2_hard_start_xmit(struct sk_buff *skb,
struct qeth_card *card = dev->ml_priv;
u16 txq = skb_get_queue_mapping(skb);
struct qeth_qdio_out_q *queue;
- int tx_bytes = skb->len;
int rc;
+ if (!skb_is_gso(skb))
+ qdisc_skb_cb(skb)->pkt_len = skb->len;
if (IS_IQD(card))
txq = qeth_iqd_translate_txq(dev, txq);
queue = card->qdio.out_qs[txq];
@@ -601,11 +600,8 @@ static netdev_tx_t qeth_l2_hard_start_xmit(struct sk_buff *skb,
rc = qeth_xmit(card, skb, queue, qeth_get_ip_version(skb),
qeth_l2_fill_header);
- if (!rc) {
- QETH_TXQ_STAT_INC(queue, tx_packets);
- QETH_TXQ_STAT_ADD(queue, tx_bytes, tx_bytes);
+ if (!rc)
return NETDEV_TX_OK;
- }
QETH_TXQ_STAT_INC(queue, tx_dropped);
kfree_skb(skb);
diff --git a/drivers/s390/net/qeth_l3_main.c b/drivers/s390/net/qeth_l3_main.c
index 54799fe6a700..d7bfc7a0e4c0 100644
--- a/drivers/s390/net/qeth_l3_main.c
+++ b/drivers/s390/net/qeth_l3_main.c
@@ -1957,7 +1957,6 @@ static void qeth_l3_fill_header(struct qeth_qdio_out_q *queue,
/* some HW requires combined L3+L4 csum offload: */
if (ipv == 4)
hdr->hdr.l3.ext_flags |= QETH_HDR_EXT_CSUM_HDR_REQ;
- QETH_TXQ_STAT_INC(queue, skbs_csum);
}
}
@@ -2044,9 +2043,10 @@ static netdev_tx_t qeth_l3_hard_start_xmit(struct sk_buff *skb,
u16 txq = skb_get_queue_mapping(skb);
int ipv = qeth_get_ip_version(skb);
struct qeth_qdio_out_q *queue;
- int tx_bytes = skb->len;
int rc;
+ if (!skb_is_gso(skb))
+ qdisc_skb_cb(skb)->pkt_len = skb->len;
if (IS_IQD(card)) {
queue = card->qdio.out_qs[qeth_iqd_translate_txq(dev, txq)];
@@ -2069,11 +2069,8 @@ static netdev_tx_t qeth_l3_hard_start_xmit(struct sk_buff *skb,
else
rc = qeth_xmit(card, skb, queue, ipv, qeth_l3_fill_header);
- if (!rc) {
- QETH_TXQ_STAT_INC(queue, tx_packets);
- QETH_TXQ_STAT_ADD(queue, tx_bytes, tx_bytes);
+ if (!rc)
return NETDEV_TX_OK;
- }
tx_drop:
QETH_TXQ_STAT_INC(queue, tx_dropped);
--
2.17.1
^ permalink raw reply related
* [PATCH net-next 5/7] s390/qeth: when in TX NAPI mode, use napi_consume_skb()
From: Julian Wiedmann @ 2019-08-23 9:48 UTC (permalink / raw)
To: David Miller
Cc: netdev, linux-s390, Heiko Carstens, Stefan Raspl, Ursula Braun,
Julian Wiedmann
In-Reply-To: <20190823094853.63814-1-jwi@linux.ibm.com>
This allows the stack to bulk-free our TX-completed skbs.
Signed-off-by: Julian Wiedmann <jwi@linux.ibm.com>
---
drivers/s390/net/qeth_core_main.c | 24 +++++++++++++-----------
1 file changed, 13 insertions(+), 11 deletions(-)
diff --git a/drivers/s390/net/qeth_core_main.c b/drivers/s390/net/qeth_core_main.c
index 3223ad80998c..70c7e675431e 100644
--- a/drivers/s390/net/qeth_core_main.c
+++ b/drivers/s390/net/qeth_core_main.c
@@ -71,7 +71,8 @@ static void qeth_free_qdio_queues(struct qeth_card *card);
static void qeth_notify_skbs(struct qeth_qdio_out_q *queue,
struct qeth_qdio_out_buffer *buf,
enum iucv_tx_notify notification);
-static void qeth_tx_complete_buf(struct qeth_qdio_out_buffer *buf, bool error);
+static void qeth_tx_complete_buf(struct qeth_qdio_out_buffer *buf, bool error,
+ int budget);
static int qeth_init_qdio_out_buf(struct qeth_qdio_out_q *, int);
static void qeth_close_dev_handler(struct work_struct *work)
@@ -411,7 +412,7 @@ static void qeth_cleanup_handled_pending(struct qeth_qdio_out_q *q, int bidx,
/* release here to avoid interleaving between
outbound tasklet and inbound tasklet
regarding notifications and lifecycle */
- qeth_tx_complete_buf(c, forced_cleanup);
+ qeth_tx_complete_buf(c, forced_cleanup, 0);
c = f->next_pending;
WARN_ON_ONCE(head->next_pending != f);
@@ -1077,7 +1078,8 @@ static void qeth_notify_skbs(struct qeth_qdio_out_q *q,
}
}
-static void qeth_tx_complete_buf(struct qeth_qdio_out_buffer *buf, bool error)
+static void qeth_tx_complete_buf(struct qeth_qdio_out_buffer *buf, bool error,
+ int budget)
{
struct qeth_qdio_out_q *queue = buf->q;
struct sk_buff *skb;
@@ -1115,13 +1117,13 @@ static void qeth_tx_complete_buf(struct qeth_qdio_out_buffer *buf, bool error)
}
}
- consume_skb(skb);
+ napi_consume_skb(skb, budget);
}
}
static void qeth_clear_output_buffer(struct qeth_qdio_out_q *queue,
struct qeth_qdio_out_buffer *buf,
- bool error)
+ bool error, int budget)
{
int i;
@@ -1129,7 +1131,7 @@ static void qeth_clear_output_buffer(struct qeth_qdio_out_q *queue,
if (buf->buffer->element[0].sflags & SBAL_SFLAGS0_PCI_REQ)
atomic_dec(&queue->set_pci_flags_count);
- qeth_tx_complete_buf(buf, error);
+ qeth_tx_complete_buf(buf, error, budget);
for (i = 0; i < queue->max_elements; ++i) {
if (buf->buffer->element[i].addr && buf->is_header[i])
@@ -1151,7 +1153,7 @@ static void qeth_drain_output_queue(struct qeth_qdio_out_q *q, bool free)
if (!q->bufs[j])
continue;
qeth_cleanup_handled_pending(q, j, 1);
- qeth_clear_output_buffer(q, q->bufs[j], true);
+ qeth_clear_output_buffer(q, q->bufs[j], true, 0);
if (free) {
kmem_cache_free(qeth_qdio_outbuf_cache, q->bufs[j]);
q->bufs[j] = NULL;
@@ -3471,7 +3473,7 @@ static void qeth_qdio_output_handler(struct ccw_device *ccwdev,
int bidx = i % QDIO_MAX_BUFFERS_PER_Q;
buffer = queue->bufs[bidx];
qeth_handle_send_error(card, buffer, qdio_error);
- qeth_clear_output_buffer(queue, buffer, qdio_error);
+ qeth_clear_output_buffer(queue, buffer, qdio_error, 0);
}
atomic_sub(count, &queue->used_buffers);
@@ -5138,7 +5140,7 @@ int qeth_poll(struct napi_struct *napi, int budget)
EXPORT_SYMBOL_GPL(qeth_poll);
static void qeth_iqd_tx_complete(struct qeth_qdio_out_q *queue,
- unsigned int bidx, bool error)
+ unsigned int bidx, bool error, int budget)
{
struct qeth_qdio_out_buffer *buffer = queue->bufs[bidx];
u8 sflags = buffer->buffer->element[15].sflags;
@@ -5168,7 +5170,7 @@ static void qeth_iqd_tx_complete(struct qeth_qdio_out_q *queue,
if (card->options.cq == QETH_CQ_ENABLED)
qeth_notify_skbs(queue, buffer,
qeth_compute_cq_notification(sflags, 0));
- qeth_clear_output_buffer(queue, buffer, error);
+ qeth_clear_output_buffer(queue, buffer, error, budget);
}
static int qeth_tx_poll(struct napi_struct *napi, int budget)
@@ -5212,7 +5214,7 @@ static int qeth_tx_poll(struct napi_struct *napi, int budget)
unsigned int bidx = QDIO_BUFNR(i);
qeth_handle_send_error(card, queue->bufs[bidx], error);
- qeth_iqd_tx_complete(queue, bidx, error);
+ qeth_iqd_tx_complete(queue, bidx, error, budget);
qeth_cleanup_handled_pending(queue, bidx, false);
}
--
2.17.1
^ permalink raw reply related
* [PATCH net-next 1/7] s390/qdio: enable drivers to poll for Output completions
From: Julian Wiedmann @ 2019-08-23 9:48 UTC (permalink / raw)
To: David Miller
Cc: netdev, linux-s390, Heiko Carstens, Stefan Raspl, Ursula Braun,
Julian Wiedmann
In-Reply-To: <20190823094853.63814-1-jwi@linux.ibm.com>
While commit d36deae75011 ("qdio: extend API to allow polling") enhanced
the qdio layer so that drivers can poll their Input Queues, we don't
have the corresponding infrastructure for Output Queues yet.
Factor out a helper that scans a single QDIO Queue, so that qeth can
implement TX NAPI on top of it.
While doing so, remove the duplicated tracking of the next-to-scan index
(q->first_to_check vs q->first_to_kick) in this code path.
qdio_handle_aobs() needs to move slightly upwards in the code hierarchy,
so that it's still called from the polling path.
Signed-off-by: Julian Wiedmann <jwi@linux.ibm.com>
Acked-by: Vasily Gorbik <gor@linux.ibm.com>
---
arch/s390/include/asm/qdio.h | 3 ++
drivers/s390/cio/qdio_main.c | 64 ++++++++++++++++++++++++------------
2 files changed, 46 insertions(+), 21 deletions(-)
diff --git a/arch/s390/include/asm/qdio.h b/arch/s390/include/asm/qdio.h
index f647d565bd6d..79b4a3e9dc5d 100644
--- a/arch/s390/include/asm/qdio.h
+++ b/arch/s390/include/asm/qdio.h
@@ -416,6 +416,9 @@ extern int do_QDIO(struct ccw_device *, unsigned int, int, unsigned int,
extern int qdio_start_irq(struct ccw_device *, int);
extern int qdio_stop_irq(struct ccw_device *, int);
extern int qdio_get_next_buffers(struct ccw_device *, int, int *, int *);
+extern int qdio_inspect_queue(struct ccw_device *cdev, unsigned int nr,
+ bool is_input, unsigned int *bufnr,
+ unsigned int *error);
extern int qdio_shutdown(struct ccw_device *, int);
extern int qdio_free(struct ccw_device *);
extern int qdio_get_ssqd_desc(struct ccw_device *, struct qdio_ssqd_desc *);
diff --git a/drivers/s390/cio/qdio_main.c b/drivers/s390/cio/qdio_main.c
index 4142c85e77d8..5efba0d29190 100644
--- a/drivers/s390/cio/qdio_main.c
+++ b/drivers/s390/cio/qdio_main.c
@@ -647,8 +647,6 @@ static void qdio_kick_handler(struct qdio_q *q, unsigned int count)
qperf_inc(q, outbound_handler);
DBF_DEV_EVENT(DBF_INFO, q->irq_ptr, "koh: s:%02x c:%02x",
start, count);
- if (q->u.out.use_cq)
- qdio_handle_aobs(q, start, count);
}
q->handler(q->irq_ptr->cdev, q->qdio_error, q->nr, start, count,
@@ -774,8 +772,11 @@ static inline int qdio_outbound_q_moved(struct qdio_q *q, unsigned int start)
count = get_outbound_buffer_frontier(q, start);
- if (count)
+ if (count) {
DBF_DEV_EVENT(DBF_INFO, q->irq_ptr, "out moved:%1d", q->nr);
+ if (q->u.out.use_cq)
+ qdio_handle_aobs(q, start, count);
+ }
return count;
}
@@ -1655,6 +1656,44 @@ int qdio_start_irq(struct ccw_device *cdev, int nr)
}
EXPORT_SYMBOL(qdio_start_irq);
+static int __qdio_inspect_queue(struct qdio_q *q, unsigned int *bufnr,
+ unsigned int *error)
+{
+ unsigned int start = q->first_to_check;
+ int count;
+
+ count = q->is_input_q ? qdio_inbound_q_moved(q, start) :
+ qdio_outbound_q_moved(q, start);
+ if (count == 0)
+ return 0;
+
+ *bufnr = start;
+ *error = q->qdio_error;
+
+ /* for the next time */
+ q->first_to_check = add_buf(start, count);
+ q->qdio_error = 0;
+
+ return count;
+}
+
+int qdio_inspect_queue(struct ccw_device *cdev, unsigned int nr, bool is_input,
+ unsigned int *bufnr, unsigned int *error)
+{
+ struct qdio_irq *irq_ptr = cdev->private->qdio_data;
+ struct qdio_q *q;
+
+ if (!irq_ptr)
+ return -ENODEV;
+ q = is_input ? irq_ptr->input_qs[nr] : irq_ptr->output_qs[nr];
+
+ if (need_siga_sync(q))
+ qdio_siga_sync_q(q);
+
+ return __qdio_inspect_queue(q, bufnr, error);
+}
+EXPORT_SYMBOL_GPL(qdio_inspect_queue);
+
/**
* qdio_get_next_buffers - process input buffers
* @cdev: associated ccw_device for the qdio subchannel
@@ -1672,13 +1711,10 @@ int qdio_get_next_buffers(struct ccw_device *cdev, int nr, int *bufnr,
{
struct qdio_q *q;
struct qdio_irq *irq_ptr = cdev->private->qdio_data;
- unsigned int start;
- int count;
if (!irq_ptr)
return -ENODEV;
q = irq_ptr->input_qs[nr];
- start = q->first_to_check;
/*
* Cannot rely on automatic sync after interrupt since queues may
@@ -1689,25 +1725,11 @@ int qdio_get_next_buffers(struct ccw_device *cdev, int nr, int *bufnr,
qdio_check_outbound_pci_queues(irq_ptr);
- count = qdio_inbound_q_moved(q, start);
- if (count == 0)
- return 0;
-
- start = add_buf(start, count);
- q->first_to_check = start;
-
/* Note: upper-layer MUST stop processing immediately here ... */
if (unlikely(q->irq_ptr->state != QDIO_IRQ_STATE_ACTIVE))
return -EIO;
- *bufnr = q->first_to_kick;
- *error = q->qdio_error;
-
- /* for the next time */
- q->first_to_kick = add_buf(q->first_to_kick, count);
- q->qdio_error = 0;
-
- return count;
+ return __qdio_inspect_queue(q, bufnr, error);
}
EXPORT_SYMBOL(qdio_get_next_buffers);
--
2.17.1
^ permalink raw reply related
* [PATCH net-next 4/7] s390/qeth: add TX NAPI support for IQD devices
From: Julian Wiedmann @ 2019-08-23 9:48 UTC (permalink / raw)
To: David Miller
Cc: netdev, linux-s390, Heiko Carstens, Stefan Raspl, Ursula Braun,
Julian Wiedmann
In-Reply-To: <20190823094853.63814-1-jwi@linux.ibm.com>
Due to their large MTU and potentially low utilization of TX buffers,
IQD devices in particular require fast TX recycling. This makes them
a prime candidate for a TX NAPI path in qeth.
qeth_tx_poll() uses the recently introduced qdio_inspect_queue() helper
to poll the TX queue for completed buffers. To avoid hogging the CPU for
too long, we yield to the stack after completing an entire queue's worth
of buffers.
While IQD is expected to transfer its buffers synchronously (and thus
doesn't support TX interrupts), a timer covers for the odd case where a
TX buffer doesn't complete synchronously. Currently this timer should
only ever fire for
(1) the mcast queue,
(2) the occasional race, where the NAPI poll code observes an update to
queue->used_buffers while the TX doorbell hasn't been issued yet.
Signed-off-by: Julian Wiedmann <jwi@linux.ibm.com>
---
arch/s390/include/asm/qdio.h | 1 +
drivers/s390/net/qeth_core.h | 26 ++++
drivers/s390/net/qeth_core_main.c | 202 +++++++++++++++++++++++-------
drivers/s390/net/qeth_ethtool.c | 2 +
4 files changed, 183 insertions(+), 48 deletions(-)
diff --git a/arch/s390/include/asm/qdio.h b/arch/s390/include/asm/qdio.h
index 556d3e703fae..78e8a888306d 100644
--- a/arch/s390/include/asm/qdio.h
+++ b/arch/s390/include/asm/qdio.h
@@ -16,6 +16,7 @@
#define QDIO_MAX_QUEUES_PER_IRQ 4
#define QDIO_MAX_BUFFERS_PER_Q 128
#define QDIO_MAX_BUFFERS_MASK (QDIO_MAX_BUFFERS_PER_Q - 1)
+#define QDIO_BUFNR(num) ((num) & QDIO_MAX_BUFFERS_MASK)
#define QDIO_MAX_ELEMENTS_PER_BUFFER 16
#define QDIO_SBAL_SIZE 256
diff --git a/drivers/s390/net/qeth_core.h b/drivers/s390/net/qeth_core.h
index a0911ce55db3..ae2ae17e3e76 100644
--- a/drivers/s390/net/qeth_core.h
+++ b/drivers/s390/net/qeth_core.h
@@ -22,6 +22,7 @@
#include <linux/hashtable.h>
#include <linux/ip.h>
#include <linux/refcount.h>
+#include <linux/timer.h>
#include <linux/wait.h>
#include <linux/workqueue.h>
@@ -474,6 +475,8 @@ struct qeth_out_q_stats {
u64 tso_bytes;
u64 packing_mode_switch;
u64 stopped;
+ u64 completion_yield;
+ u64 completion_timer;
/* rtnl_link_stats64 */
u64 tx_packets;
@@ -482,6 +485,8 @@ struct qeth_out_q_stats {
u64 tx_dropped;
};
+#define QETH_TX_TIMER_USECS 500
+
struct qeth_qdio_out_q {
struct qdio_buffer *qdio_bufs[QDIO_MAX_BUFFERS_PER_Q];
struct qeth_qdio_out_buffer *bufs[QDIO_MAX_BUFFERS_PER_Q];
@@ -500,13 +505,34 @@ struct qeth_qdio_out_q {
atomic_t used_buffers;
/* indicates whether PCI flag must be set (or if one is outstanding) */
atomic_t set_pci_flags_count;
+ struct napi_struct napi;
+ struct timer_list timer;
};
+#define qeth_for_each_output_queue(card, q, i) \
+ for (i = 0; i < card->qdio.no_out_queues && \
+ (q = card->qdio.out_qs[i]); i++)
+
+#define qeth_napi_to_out_queue(n) container_of(n, struct qeth_qdio_out_q, napi)
+
+static inline void qeth_tx_arm_timer(struct qeth_qdio_out_q *queue)
+{
+ if (timer_pending(&queue->timer))
+ return;
+ mod_timer(&queue->timer, usecs_to_jiffies(QETH_TX_TIMER_USECS) +
+ jiffies);
+}
+
static inline bool qeth_out_queue_is_full(struct qeth_qdio_out_q *queue)
{
return atomic_read(&queue->used_buffers) >= QDIO_MAX_BUFFERS_PER_Q;
}
+static inline bool qeth_out_queue_is_empty(struct qeth_qdio_out_q *queue)
+{
+ return atomic_read(&queue->used_buffers) == 0;
+}
+
struct qeth_qdio_info {
atomic_t state;
/* input */
diff --git a/drivers/s390/net/qeth_core_main.c b/drivers/s390/net/qeth_core_main.c
index d7a15a88bdba..3223ad80998c 100644
--- a/drivers/s390/net/qeth_core_main.c
+++ b/drivers/s390/net/qeth_core_main.c
@@ -2284,6 +2284,14 @@ static struct qeth_qdio_out_q *qeth_alloc_output_queue(void)
return q;
}
+static void qeth_tx_completion_timer(struct timer_list *timer)
+{
+ struct qeth_qdio_out_q *queue = from_timer(queue, timer, timer);
+
+ napi_schedule(&queue->napi);
+ QETH_TXQ_STAT_INC(queue, completion_timer);
+}
+
static int qeth_alloc_qdio_queues(struct qeth_card *card)
{
int i, j;
@@ -2305,17 +2313,22 @@ static int qeth_alloc_qdio_queues(struct qeth_card *card)
/* outbound */
for (i = 0; i < card->qdio.no_out_queues; ++i) {
- card->qdio.out_qs[i] = qeth_alloc_output_queue();
- if (!card->qdio.out_qs[i])
+ struct qeth_qdio_out_q *queue;
+
+ queue = qeth_alloc_output_queue();
+ if (!queue)
goto out_freeoutq;
QETH_CARD_TEXT_(card, 2, "outq %i", i);
- QETH_CARD_HEX(card, 2, &card->qdio.out_qs[i], sizeof(void *));
- card->qdio.out_qs[i]->card = card;
- card->qdio.out_qs[i]->queue_no = i;
+ QETH_CARD_HEX(card, 2, &queue, sizeof(void *));
+ card->qdio.out_qs[i] = queue;
+ queue->card = card;
+ queue->queue_no = i;
+ timer_setup(&queue->timer, qeth_tx_completion_timer, 0);
+
/* give outbound qeth_qdio_buffers their qdio_buffers */
for (j = 0; j < QDIO_MAX_BUFFERS_PER_Q; ++j) {
- WARN_ON(card->qdio.out_qs[i]->bufs[j] != NULL);
- if (qeth_init_qdio_out_buf(card->qdio.out_qs[i], j))
+ WARN_ON(queue->bufs[j]);
+ if (qeth_init_qdio_out_buf(queue, j))
goto out_freeoutqbufs;
}
}
@@ -3226,6 +3239,7 @@ static int qeth_switch_to_nonpacking_if_needed(struct qeth_qdio_out_q *queue)
static void qeth_flush_buffers(struct qeth_qdio_out_q *queue, int index,
int count)
{
+ struct qeth_card *card = queue->card;
struct qeth_qdio_out_buffer *buf;
int rc;
int i;
@@ -3274,6 +3288,11 @@ static void qeth_flush_buffers(struct qeth_qdio_out_q *queue, int index,
qdio_flags |= QDIO_FLAG_PCI_OUT;
rc = do_QDIO(CARD_DDEV(queue->card), qdio_flags,
queue->queue_no, index, count);
+
+ /* Fake the TX completion interrupt: */
+ if (IS_IQD(card))
+ napi_schedule(&queue->napi);
+
if (rc) {
/* ignore temporary SIGA errors without busy condition */
if (rc == -ENOBUFS)
@@ -3452,48 +3471,12 @@ static void qeth_qdio_output_handler(struct ccw_device *ccwdev,
int bidx = i % QDIO_MAX_BUFFERS_PER_Q;
buffer = queue->bufs[bidx];
qeth_handle_send_error(card, buffer, qdio_error);
-
- if (queue->bufstates &&
- (queue->bufstates[bidx].flags &
- QDIO_OUTBUF_STATE_FLAG_PENDING) != 0) {
- WARN_ON_ONCE(card->options.cq != QETH_CQ_ENABLED);
-
- if (atomic_cmpxchg(&buffer->state,
- QETH_QDIO_BUF_PRIMED,
- QETH_QDIO_BUF_PENDING) ==
- QETH_QDIO_BUF_PRIMED) {
- qeth_notify_skbs(queue, buffer,
- TX_NOTIFY_PENDING);
- }
- QETH_CARD_TEXT_(queue->card, 5, "pel%d", bidx);
-
- /* prepare the queue slot for re-use: */
- qeth_scrub_qdio_buffer(buffer->buffer,
- queue->max_elements);
- if (qeth_init_qdio_out_buf(queue, bidx)) {
- QETH_CARD_TEXT(card, 2, "outofbuf");
- qeth_schedule_recovery(card);
- }
- } else {
- if (card->options.cq == QETH_CQ_ENABLED) {
- enum iucv_tx_notify n;
-
- n = qeth_compute_cq_notification(
- buffer->buffer->element[15].sflags, 0);
- qeth_notify_skbs(queue, buffer, n);
- }
-
- qeth_clear_output_buffer(queue, buffer, qdio_error);
- }
- qeth_cleanup_handled_pending(queue, bidx, 0);
+ qeth_clear_output_buffer(queue, buffer, qdio_error);
}
+
atomic_sub(count, &queue->used_buffers);
- /* check if we need to do something on this outbound queue */
- if (!IS_IQD(card))
- qeth_check_outbound_queue(queue);
+ qeth_check_outbound_queue(queue);
- if (IS_IQD(card))
- __queue = qeth_iqd_translate_txq(dev, __queue);
txq = netdev_get_tx_queue(dev, __queue);
/* xmit may have observed the full-condition, but not yet stopped the
* txq. In which case the code below won't trigger. So before returning,
@@ -4740,7 +4723,7 @@ static int qeth_qdio_establish(struct qeth_card *card)
init_data.input_sbal_addr_array = in_sbal_ptrs;
init_data.output_sbal_addr_array = out_sbal_ptrs;
init_data.output_sbal_state_array = card->qdio.out_bufstates;
- init_data.scan_threshold = IS_IQD(card) ? 1 : 32;
+ init_data.scan_threshold = IS_IQD(card) ? 0 : 32;
if (atomic_cmpxchg(&card->qdio.state, QETH_QDIO_ALLOCATED,
QETH_QDIO_ESTABLISHED) == QETH_QDIO_ALLOCATED) {
@@ -5154,6 +5137,99 @@ int qeth_poll(struct napi_struct *napi, int budget)
}
EXPORT_SYMBOL_GPL(qeth_poll);
+static void qeth_iqd_tx_complete(struct qeth_qdio_out_q *queue,
+ unsigned int bidx, bool error)
+{
+ struct qeth_qdio_out_buffer *buffer = queue->bufs[bidx];
+ u8 sflags = buffer->buffer->element[15].sflags;
+ struct qeth_card *card = queue->card;
+
+ if (queue->bufstates && (queue->bufstates[bidx].flags &
+ QDIO_OUTBUF_STATE_FLAG_PENDING)) {
+ WARN_ON_ONCE(card->options.cq != QETH_CQ_ENABLED);
+
+ if (atomic_cmpxchg(&buffer->state, QETH_QDIO_BUF_PRIMED,
+ QETH_QDIO_BUF_PENDING) ==
+ QETH_QDIO_BUF_PRIMED)
+ qeth_notify_skbs(queue, buffer, TX_NOTIFY_PENDING);
+
+ QETH_CARD_TEXT_(card, 5, "pel%u", bidx);
+
+ /* prepare the queue slot for re-use: */
+ qeth_scrub_qdio_buffer(buffer->buffer, queue->max_elements);
+ if (qeth_init_qdio_out_buf(queue, bidx)) {
+ QETH_CARD_TEXT(card, 2, "outofbuf");
+ qeth_schedule_recovery(card);
+ }
+
+ return;
+ }
+
+ if (card->options.cq == QETH_CQ_ENABLED)
+ qeth_notify_skbs(queue, buffer,
+ qeth_compute_cq_notification(sflags, 0));
+ qeth_clear_output_buffer(queue, buffer, error);
+}
+
+static int qeth_tx_poll(struct napi_struct *napi, int budget)
+{
+ struct qeth_qdio_out_q *queue = qeth_napi_to_out_queue(napi);
+ unsigned int queue_no = queue->queue_no;
+ struct qeth_card *card = queue->card;
+ struct net_device *dev = card->dev;
+ unsigned int work_done = 0;
+ struct netdev_queue *txq;
+
+ txq = netdev_get_tx_queue(dev, qeth_iqd_translate_txq(dev, queue_no));
+
+ while (1) {
+ unsigned int start, error, i;
+ int completed;
+
+ if (qeth_out_queue_is_empty(queue)) {
+ napi_complete(napi);
+ return 0;
+ }
+
+ /* Give the CPU a breather: */
+ if (work_done >= QDIO_MAX_BUFFERS_PER_Q) {
+ QETH_TXQ_STAT_INC(queue, completion_yield);
+ if (napi_complete_done(napi, 0))
+ napi_schedule(napi);
+ return 0;
+ }
+
+ completed = qdio_inspect_queue(CARD_DDEV(card), queue_no, false,
+ &start, &error);
+ if (completed <= 0) {
+ /* Ensure we see TX completion for pending work: */
+ if (napi_complete_done(napi, 0))
+ qeth_tx_arm_timer(queue);
+ return 0;
+ }
+
+ for (i = start; i < start + completed; i++) {
+ unsigned int bidx = QDIO_BUFNR(i);
+
+ qeth_handle_send_error(card, queue->bufs[bidx], error);
+ qeth_iqd_tx_complete(queue, bidx, error);
+ qeth_cleanup_handled_pending(queue, bidx, false);
+ }
+
+ atomic_sub(completed, &queue->used_buffers);
+ work_done += completed;
+
+ /* xmit may have observed the full-condition, but not yet
+ * stopped the txq. In which case the code below won't trigger.
+ * So before returning, xmit will re-check the txq's fill level
+ * and wake it up if needed.
+ */
+ if (netif_tx_queue_stopped(txq) &&
+ !qeth_out_queue_is_full(queue))
+ netif_tx_wake_queue(txq);
+ }
+}
+
static int qeth_setassparms_inspect_rc(struct qeth_ipa_cmd *cmd)
{
if (!cmd->hdr.return_code)
@@ -6100,6 +6176,17 @@ int qeth_open(struct net_device *dev)
napi_enable(&card->napi);
local_bh_disable();
napi_schedule(&card->napi);
+ if (IS_IQD(card)) {
+ struct qeth_qdio_out_q *queue;
+ unsigned int i;
+
+ qeth_for_each_output_queue(card, queue, i) {
+ netif_tx_napi_add(dev, &queue->napi, qeth_tx_poll,
+ QETH_NAPI_WEIGHT);
+ napi_enable(&queue->napi);
+ napi_schedule(&queue->napi);
+ }
+ }
/* kick-start the NAPI softirq: */
local_bh_enable();
return 0;
@@ -6111,7 +6198,26 @@ int qeth_stop(struct net_device *dev)
struct qeth_card *card = dev->ml_priv;
QETH_CARD_TEXT(card, 4, "qethstop");
- netif_tx_disable(dev);
+ if (IS_IQD(card)) {
+ struct qeth_qdio_out_q *queue;
+ unsigned int i;
+
+ /* Quiesce the NAPI instances: */
+ qeth_for_each_output_queue(card, queue, i) {
+ napi_disable(&queue->napi);
+ del_timer_sync(&queue->timer);
+ }
+
+ /* Stop .ndo_start_xmit, might still access queue->napi. */
+ netif_tx_disable(dev);
+
+ /* Queues may get re-allocated, so remove the NAPIs here. */
+ qeth_for_each_output_queue(card, queue, i)
+ netif_napi_del(&queue->napi);
+ } else {
+ netif_tx_disable(dev);
+ }
+
napi_disable(&card->napi);
return 0;
}
diff --git a/drivers/s390/net/qeth_ethtool.c b/drivers/s390/net/qeth_ethtool.c
index 4166eb29f0bd..096698df3886 100644
--- a/drivers/s390/net/qeth_ethtool.c
+++ b/drivers/s390/net/qeth_ethtool.c
@@ -39,6 +39,8 @@ static const struct qeth_stats txq_stats[] = {
QETH_TXQ_STAT("TSO bytes", tso_bytes),
QETH_TXQ_STAT("Packing mode switches", packing_mode_switch),
QETH_TXQ_STAT("Queue stopped", stopped),
+ QETH_TXQ_STAT("Completion yield", completion_yield),
+ QETH_TXQ_STAT("Completion timer", completion_timer),
};
static const struct qeth_stats card_stats[] = {
--
2.17.1
^ permalink raw reply related
* [PATCH net-next 7/7] s390/qeth: add xmit_more support for IQD devices
From: Julian Wiedmann @ 2019-08-23 9:48 UTC (permalink / raw)
To: David Miller
Cc: netdev, linux-s390, Heiko Carstens, Stefan Raspl, Ursula Braun,
Julian Wiedmann
In-Reply-To: <20190823094853.63814-1-jwi@linux.ibm.com>
IQD devices offer limited support for bulking: all frames in a TX buffer
need to have the same target. qeth_iqd_may_bulk() implements this
constraint, and allows us to defer the TX doorbell until
(a) the buffer is full (since each buffer needs its own doorbell), or
(b) the entire TX queue is full, or
(b) we reached the BQL limit.
Signed-off-by: Julian Wiedmann <jwi@linux.ibm.com>
---
drivers/s390/net/qeth_core.h | 24 ++++++
drivers/s390/net/qeth_core_main.c | 128 ++++++++++++++++++++----------
2 files changed, 109 insertions(+), 43 deletions(-)
diff --git a/drivers/s390/net/qeth_core.h b/drivers/s390/net/qeth_core.h
index d5f796380cd0..e4b55f9aa062 100644
--- a/drivers/s390/net/qeth_core.h
+++ b/drivers/s390/net/qeth_core.h
@@ -378,6 +378,28 @@ enum qeth_header_ids {
#define QETH_HDR_EXT_CSUM_TRANSP_REQ 0x20
#define QETH_HDR_EXT_UDP 0x40 /*bit off for TCP*/
+static inline bool qeth_l2_same_vlan(struct qeth_hdr_layer2 *h1,
+ struct qeth_hdr_layer2 *h2)
+{
+ return !((h1->flags[2] ^ h2->flags[2]) & QETH_LAYER2_FLAG_VLAN) &&
+ h1->vlan_id == h2->vlan_id;
+}
+
+static inline bool qeth_l3_iqd_same_vlan(struct qeth_hdr_layer3 *h1,
+ struct qeth_hdr_layer3 *h2)
+{
+ return !((h1->ext_flags ^ h2->ext_flags) & QETH_HDR_EXT_VLAN_FRAME) &&
+ h1->vlan_id == h2->vlan_id;
+}
+
+static inline bool qeth_l3_same_next_hop(struct qeth_hdr_layer3 *h1,
+ struct qeth_hdr_layer3 *h2)
+{
+ return !((h1->flags ^ h2->flags) & QETH_HDR_IPV6) &&
+ ipv6_addr_equal(&h1->next_hop.ipv6_addr,
+ &h2->next_hop.ipv6_addr);
+}
+
enum qeth_qdio_info_states {
QETH_QDIO_UNINITIALIZED,
QETH_QDIO_ALLOCATED,
@@ -508,6 +530,8 @@ struct qeth_qdio_out_q {
atomic_t set_pci_flags_count;
struct napi_struct napi;
struct timer_list timer;
+ struct qeth_hdr *prev_hdr;
+ u8 bulk_start;
};
#define qeth_for_each_output_queue(card, q, i) \
diff --git a/drivers/s390/net/qeth_core_main.c b/drivers/s390/net/qeth_core_main.c
index 4c7c7d320c9c..8b4ea5f2832b 100644
--- a/drivers/s390/net/qeth_core_main.c
+++ b/drivers/s390/net/qeth_core_main.c
@@ -2671,6 +2671,8 @@ int qeth_init_qdio_queues(struct qeth_card *card)
queue->max_elements = QETH_MAX_BUFFER_ELEMENTS(card);
queue->next_buf_to_fill = 0;
queue->do_pack = 0;
+ queue->prev_hdr = NULL;
+ queue->bulk_start = 0;
atomic_set(&queue->used_buffers, 0);
atomic_set(&queue->set_pci_flags_count, 0);
atomic_set(&queue->state, QETH_OUT_Q_UNLOCKED);
@@ -3314,6 +3316,14 @@ static void qeth_flush_buffers(struct qeth_qdio_out_q *queue, int index,
}
}
+static void qeth_flush_queue(struct qeth_qdio_out_q *queue)
+{
+ qeth_flush_buffers(queue, queue->bulk_start, 1);
+
+ queue->bulk_start = QDIO_BUFNR(queue->bulk_start + 1);
+ queue->prev_hdr = NULL;
+}
+
static void qeth_check_outbound_queue(struct qeth_qdio_out_q *queue)
{
int index;
@@ -3669,9 +3679,32 @@ static int qeth_add_hw_header(struct qeth_qdio_out_q *queue,
return 0;
}
-static void __qeth_fill_buffer(struct sk_buff *skb,
- struct qeth_qdio_out_buffer *buf,
- bool is_first_elem, unsigned int offset)
+static bool qeth_iqd_may_bulk(struct qeth_qdio_out_q *queue,
+ struct qeth_qdio_out_buffer *buffer,
+ struct sk_buff *curr_skb,
+ struct qeth_hdr *curr_hdr)
+{
+ struct qeth_hdr *prev_hdr = queue->prev_hdr;
+
+ if (!prev_hdr)
+ return true;
+
+ /* All packets must have the same target: */
+ if (curr_hdr->hdr.l2.id == QETH_HEADER_TYPE_LAYER2) {
+ struct sk_buff *prev_skb = skb_peek(&buffer->skb_list);
+
+ return ether_addr_equal(eth_hdr(prev_skb)->h_dest,
+ eth_hdr(curr_skb)->h_dest) &&
+ qeth_l2_same_vlan(&prev_hdr->hdr.l2, &curr_hdr->hdr.l2);
+ }
+
+ return qeth_l3_same_next_hop(&prev_hdr->hdr.l3, &curr_hdr->hdr.l3) &&
+ qeth_l3_iqd_same_vlan(&prev_hdr->hdr.l3, &curr_hdr->hdr.l3);
+}
+
+static unsigned int __qeth_fill_buffer(struct sk_buff *skb,
+ struct qeth_qdio_out_buffer *buf,
+ bool is_first_elem, unsigned int offset)
{
struct qdio_buffer *buffer = buf->buffer;
int element = buf->next_element_to_fill;
@@ -3728,24 +3761,21 @@ static void __qeth_fill_buffer(struct sk_buff *skb,
if (buffer->element[element - 1].eflags)
buffer->element[element - 1].eflags = SBAL_EFLAGS_LAST_FRAG;
buf->next_element_to_fill = element;
+ return element;
}
/**
* qeth_fill_buffer() - map skb into an output buffer
- * @queue: QDIO queue to submit the buffer on
* @buf: buffer to transport the skb
* @skb: skb to map into the buffer
* @hdr: qeth_hdr for this skb. Either at skb->data, or allocated
* from qeth_core_header_cache.
* @offset: when mapping the skb, start at skb->data + offset
* @hd_len: if > 0, build a dedicated header element of this size
- * flush: Prepare the buffer to be flushed, regardless of its fill level.
*/
-static int qeth_fill_buffer(struct qeth_qdio_out_q *queue,
- struct qeth_qdio_out_buffer *buf,
- struct sk_buff *skb, struct qeth_hdr *hdr,
- unsigned int offset, unsigned int hd_len,
- bool flush)
+static unsigned int qeth_fill_buffer(struct qeth_qdio_out_buffer *buf,
+ struct sk_buff *skb, struct qeth_hdr *hdr,
+ unsigned int offset, unsigned int hd_len)
{
struct qdio_buffer *buffer = buf->buffer;
bool is_first_elem = true;
@@ -3765,36 +3795,22 @@ static int qeth_fill_buffer(struct qeth_qdio_out_q *queue,
buf->next_element_to_fill++;
}
- __qeth_fill_buffer(skb, buf, is_first_elem, offset);
-
- if (!queue->do_pack) {
- QETH_CARD_TEXT(queue->card, 6, "fillbfnp");
- } else {
- QETH_CARD_TEXT(queue->card, 6, "fillbfpa");
-
- QETH_TXQ_STAT_INC(queue, skbs_pack);
- /* If the buffer still has free elements, keep using it. */
- if (!flush &&
- buf->next_element_to_fill < queue->max_elements)
- return 0;
- }
-
- /* flush out the buffer */
- atomic_set(&buf->state, QETH_QDIO_BUF_PRIMED);
- queue->next_buf_to_fill = (queue->next_buf_to_fill + 1) %
- QDIO_MAX_BUFFERS_PER_Q;
- return 1;
+ return __qeth_fill_buffer(skb, buf, is_first_elem, offset);
}
-static int qeth_do_send_packet_fast(struct qeth_qdio_out_q *queue,
- struct sk_buff *skb, struct qeth_hdr *hdr,
- unsigned int offset, unsigned int hd_len)
+static int __qeth_xmit(struct qeth_card *card, struct qeth_qdio_out_q *queue,
+ struct sk_buff *skb, unsigned int elements,
+ struct qeth_hdr *hdr, unsigned int offset,
+ unsigned int hd_len)
{
- int index = queue->next_buf_to_fill;
- struct qeth_qdio_out_buffer *buffer = queue->bufs[index];
+ struct qeth_qdio_out_buffer *buffer = queue->bufs[queue->bulk_start];
unsigned int bytes = qdisc_pkt_len(skb);
+ unsigned int next_element;
struct netdev_queue *txq;
bool stopped = false;
+ bool flush;
+
+ txq = netdev_get_tx_queue(card->dev, skb_get_queue_mapping(skb));
/* Just a sanity check, the wake/stop logic should ensure that we always
* get a free buffer.
@@ -3802,9 +3818,19 @@ static int qeth_do_send_packet_fast(struct qeth_qdio_out_q *queue,
if (atomic_read(&buffer->state) != QETH_QDIO_BUF_EMPTY)
return -EBUSY;
- txq = netdev_get_tx_queue(queue->card->dev, skb_get_queue_mapping(skb));
+ if ((buffer->next_element_to_fill + elements > queue->max_elements) ||
+ !qeth_iqd_may_bulk(queue, buffer, skb, hdr)) {
+ atomic_set(&buffer->state, QETH_QDIO_BUF_PRIMED);
+ qeth_flush_queue(queue);
+ buffer = queue->bufs[queue->bulk_start];
- if (atomic_inc_return(&queue->used_buffers) >= QDIO_MAX_BUFFERS_PER_Q) {
+ /* Sanity-check again: */
+ if (atomic_read(&buffer->state) != QETH_QDIO_BUF_EMPTY)
+ return -EBUSY;
+ }
+
+ if (buffer->next_element_to_fill == 0 &&
+ atomic_inc_return(&queue->used_buffers) >= QDIO_MAX_BUFFERS_PER_Q) {
/* If a TX completion happens right _here_ and misses to wake
* the txq, then our re-check below will catch the race.
*/
@@ -3813,11 +3839,17 @@ static int qeth_do_send_packet_fast(struct qeth_qdio_out_q *queue,
stopped = true;
}
- qeth_fill_buffer(queue, buffer, skb, hdr, offset, hd_len, stopped);
- netdev_tx_sent_queue(txq, bytes);
+ next_element = qeth_fill_buffer(buffer, skb, hdr, offset, hd_len);
buffer->bytes += bytes;
+ queue->prev_hdr = hdr;
- qeth_flush_buffers(queue, index, 1);
+ flush = __netdev_tx_sent_queue(txq, bytes,
+ !stopped && netdev_xmit_more());
+
+ if (flush || next_element >= queue->max_elements) {
+ atomic_set(&buffer->state, QETH_QDIO_BUF_PRIMED);
+ qeth_flush_queue(queue);
+ }
if (stopped && !qeth_out_queue_is_full(queue))
netif_tx_start_queue(txq);
@@ -3830,6 +3862,7 @@ int qeth_do_send_packet(struct qeth_card *card, struct qeth_qdio_out_q *queue,
int elements_needed)
{
struct qeth_qdio_out_buffer *buffer;
+ unsigned int next_element;
struct netdev_queue *txq;
bool stopped = false;
int start_index;
@@ -3892,8 +3925,17 @@ int qeth_do_send_packet(struct qeth_card *card, struct qeth_qdio_out_q *queue,
stopped = true;
}
- flush_count += qeth_fill_buffer(queue, buffer, skb, hdr, offset, hd_len,
- stopped);
+ next_element = qeth_fill_buffer(buffer, skb, hdr, offset, hd_len);
+
+ if (queue->do_pack)
+ QETH_TXQ_STAT_INC(queue, skbs_pack);
+ if (!queue->do_pack || stopped || next_element >= queue->max_elements) {
+ flush_count++;
+ atomic_set(&buffer->state, QETH_QDIO_BUF_PRIMED);
+ queue->next_buf_to_fill = (queue->next_buf_to_fill + 1) %
+ QDIO_MAX_BUFFERS_PER_Q;
+ }
+
if (flush_count)
qeth_flush_buffers(queue, start_index, flush_count);
else if (!atomic_read(&queue->set_pci_flags_count))
@@ -3989,8 +4031,8 @@ int qeth_xmit(struct qeth_card *card, struct sk_buff *skb,
frame_len - proto_len, skb, proto_len);
if (IS_IQD(card)) {
- rc = qeth_do_send_packet_fast(queue, skb, hdr, data_offset,
- hd_len);
+ rc = __qeth_xmit(card, queue, skb, elements, hdr, data_offset,
+ hd_len);
} else {
/* TODO: drop skb_orphan() once TX completion is fast enough */
skb_orphan(skb);
--
2.17.1
^ permalink raw reply related
* [PATCH net-next 6/7] s390/qeth: add BQL support for IQD devices
From: Julian Wiedmann @ 2019-08-23 9:48 UTC (permalink / raw)
To: David Miller
Cc: netdev, linux-s390, Heiko Carstens, Stefan Raspl, Ursula Braun,
Julian Wiedmann
In-Reply-To: <20190823094853.63814-1-jwi@linux.ibm.com>
Each TX buffer may contain multiple skbs. So just accumulate the sent
byte count in the buffer struct, and later use the same count when
completing the buffer.
Signed-off-by: Julian Wiedmann <jwi@linux.ibm.com>
---
drivers/s390/net/qeth_core.h | 1 +
drivers/s390/net/qeth_core_main.c | 16 +++++++++++++++-
2 files changed, 16 insertions(+), 1 deletion(-)
diff --git a/drivers/s390/net/qeth_core.h b/drivers/s390/net/qeth_core.h
index ae2ae17e3e76..d5f796380cd0 100644
--- a/drivers/s390/net/qeth_core.h
+++ b/drivers/s390/net/qeth_core.h
@@ -426,6 +426,7 @@ struct qeth_qdio_out_buffer {
struct qdio_buffer *buffer;
atomic_t state;
int next_element_to_fill;
+ unsigned int bytes;
struct sk_buff_head skb_list;
int is_header[QDIO_MAX_ELEMENTS_PER_BUFFER];
diff --git a/drivers/s390/net/qeth_core_main.c b/drivers/s390/net/qeth_core_main.c
index 70c7e675431e..4c7c7d320c9c 100644
--- a/drivers/s390/net/qeth_core_main.c
+++ b/drivers/s390/net/qeth_core_main.c
@@ -1142,6 +1142,7 @@ static void qeth_clear_output_buffer(struct qeth_qdio_out_q *queue,
qeth_scrub_qdio_buffer(buf->buffer, queue->max_elements);
buf->next_element_to_fill = 0;
+ buf->bytes = 0;
atomic_set(&buf->state, QETH_QDIO_BUF_EMPTY);
}
@@ -2673,6 +2674,7 @@ int qeth_init_qdio_queues(struct qeth_card *card)
atomic_set(&queue->used_buffers, 0);
atomic_set(&queue->set_pci_flags_count, 0);
atomic_set(&queue->state, QETH_OUT_Q_UNLOCKED);
+ netdev_tx_reset_queue(netdev_get_tx_queue(card->dev, i));
}
return 0;
}
@@ -3790,6 +3792,7 @@ static int qeth_do_send_packet_fast(struct qeth_qdio_out_q *queue,
{
int index = queue->next_buf_to_fill;
struct qeth_qdio_out_buffer *buffer = queue->bufs[index];
+ unsigned int bytes = qdisc_pkt_len(skb);
struct netdev_queue *txq;
bool stopped = false;
@@ -3811,6 +3814,9 @@ static int qeth_do_send_packet_fast(struct qeth_qdio_out_q *queue,
}
qeth_fill_buffer(queue, buffer, skb, hdr, offset, hd_len, stopped);
+ netdev_tx_sent_queue(txq, bytes);
+ buffer->bytes += bytes;
+
qeth_flush_buffers(queue, index, 1);
if (stopped && !qeth_out_queue_is_full(queue))
@@ -5186,6 +5192,8 @@ static int qeth_tx_poll(struct napi_struct *napi, int budget)
while (1) {
unsigned int start, error, i;
+ unsigned int packets = 0;
+ unsigned int bytes = 0;
int completed;
if (qeth_out_queue_is_empty(queue)) {
@@ -5211,13 +5219,19 @@ static int qeth_tx_poll(struct napi_struct *napi, int budget)
}
for (i = start; i < start + completed; i++) {
+ struct qeth_qdio_out_buffer *buffer;
unsigned int bidx = QDIO_BUFNR(i);
- qeth_handle_send_error(card, queue->bufs[bidx], error);
+ buffer = queue->bufs[bidx];
+ packets += skb_queue_len(&buffer->skb_list);
+ bytes += buffer->bytes;
+
+ qeth_handle_send_error(card, buffer, error);
qeth_iqd_tx_complete(queue, bidx, error, budget);
qeth_cleanup_handled_pending(queue, bidx, false);
}
+ netdev_tx_completed_queue(txq, packets, bytes);
atomic_sub(completed, &queue->used_buffers);
work_done += completed;
--
2.17.1
^ permalink raw reply related
* [PATCH net-next 2/7] s390/qdio: let drivers opt-out from Output Queue scanning
From: Julian Wiedmann @ 2019-08-23 9:48 UTC (permalink / raw)
To: David Miller
Cc: netdev, linux-s390, Heiko Carstens, Stefan Raspl, Ursula Braun,
Julian Wiedmann
In-Reply-To: <20190823094853.63814-1-jwi@linux.ibm.com>
If a driver wants to use the new Output Queue poll code, then the qdio
layer must disable its internal Queue scanning. Let the driver select
this mode by passing a special scan_threshold of 0.
As the scan_threshold is the same for all Output Queues, also move it
into the main qdio_irq struct. This allows for fast opt-out checking, a
driver is expected to operate either _all_ or none of its Output Queues
in polling mode.
Signed-off-by: Julian Wiedmann <jwi@linux.ibm.com>
Acked-by: Vasily Gorbik <gor@linux.ibm.com>
---
arch/s390/include/asm/qdio.h | 2 +-
drivers/s390/cio/qdio.h | 3 +--
drivers/s390/cio/qdio_main.c | 11 ++++++++---
drivers/s390/cio/qdio_setup.c | 2 +-
4 files changed, 11 insertions(+), 7 deletions(-)
diff --git a/arch/s390/include/asm/qdio.h b/arch/s390/include/asm/qdio.h
index 79b4a3e9dc5d..556d3e703fae 100644
--- a/arch/s390/include/asm/qdio.h
+++ b/arch/s390/include/asm/qdio.h
@@ -359,7 +359,7 @@ struct qdio_initialize {
qdio_handler_t *output_handler;
void (**queue_start_poll_array) (struct ccw_device *, int,
unsigned long);
- int scan_threshold;
+ unsigned int scan_threshold;
unsigned long int_parm;
struct qdio_buffer **input_sbal_addr_array;
struct qdio_buffer **output_sbal_addr_array;
diff --git a/drivers/s390/cio/qdio.h b/drivers/s390/cio/qdio.h
index a06944399865..a58b45df95d7 100644
--- a/drivers/s390/cio/qdio.h
+++ b/drivers/s390/cio/qdio.h
@@ -206,8 +206,6 @@ struct qdio_output_q {
struct qdio_outbuf_state *sbal_state;
/* timer to check for more outbound work */
struct timer_list timer;
- /* used SBALs before tasklet schedule */
- int scan_threshold;
};
/*
@@ -295,6 +293,7 @@ struct qdio_irq {
struct qdio_ssqd_desc ssqd_desc;
void (*orig_handler) (struct ccw_device *, unsigned long, struct irb *);
+ unsigned int scan_threshold; /* used SBALs before tasklet schedule */
int perf_stat_enabled;
struct qdr *qdr;
diff --git a/drivers/s390/cio/qdio_main.c b/drivers/s390/cio/qdio_main.c
index 5efba0d29190..5b63c505a2f7 100644
--- a/drivers/s390/cio/qdio_main.c
+++ b/drivers/s390/cio/qdio_main.c
@@ -880,7 +880,7 @@ static inline void qdio_check_outbound_pci_queues(struct qdio_irq *irq)
struct qdio_q *out;
int i;
- if (!pci_out_supported(irq))
+ if (!pci_out_supported(irq) || !irq->scan_threshold)
return;
for_each_output_queue(irq, out, i)
@@ -973,7 +973,7 @@ static void qdio_int_handler_pci(struct qdio_irq *irq_ptr)
}
}
- if (!pci_out_supported(irq_ptr))
+ if (!pci_out_supported(irq_ptr) || !irq_ptr->scan_threshold)
return;
for_each_output_queue(irq_ptr, q, i) {
@@ -1528,6 +1528,7 @@ static int handle_inbound(struct qdio_q *q, unsigned int callflags,
static int handle_outbound(struct qdio_q *q, unsigned int callflags,
int bufnr, int count)
{
+ const unsigned int scan_threshold = q->irq_ptr->scan_threshold;
unsigned char state = 0;
int used, rc = 0;
@@ -1566,8 +1567,12 @@ static int handle_outbound(struct qdio_q *q, unsigned int callflags,
rc = qdio_kick_outbound_q(q, 0);
}
+ /* Let drivers implement their own completion scanning: */
+ if (!scan_threshold)
+ return rc;
+
/* in case of SIGA errors we must process the error immediately */
- if (used >= q->u.out.scan_threshold || rc)
+ if (used >= scan_threshold || rc)
qdio_tasklet_schedule(q);
else
/* free the SBALs in case of no further traffic */
diff --git a/drivers/s390/cio/qdio_setup.c b/drivers/s390/cio/qdio_setup.c
index d4101cecdc8d..f4ca1d29d61b 100644
--- a/drivers/s390/cio/qdio_setup.c
+++ b/drivers/s390/cio/qdio_setup.c
@@ -248,7 +248,6 @@ static void setup_queues(struct qdio_irq *irq_ptr,
output_sbal_state_array += QDIO_MAX_BUFFERS_PER_Q;
q->is_input_q = 0;
- q->u.out.scan_threshold = qdio_init->scan_threshold;
setup_storage_lists(q, irq_ptr, output_sbal_array, i);
output_sbal_array += QDIO_MAX_BUFFERS_PER_Q;
@@ -474,6 +473,7 @@ int qdio_setup_irq(struct qdio_initialize *init_data)
irq_ptr->nr_input_qs = init_data->no_input_qs;
irq_ptr->nr_output_qs = init_data->no_output_qs;
irq_ptr->cdev = init_data->cdev;
+ irq_ptr->scan_threshold = init_data->scan_threshold;
ccw_device_get_schid(irq_ptr->cdev, &irq_ptr->schid);
setup_queues(irq_ptr, init_data);
--
2.17.1
^ permalink raw reply related
* Re: [PATCH v3] tun: fix use-after-free when register netdev failed
From: Yang Yingliang @ 2019-08-23 9:36 UTC (permalink / raw)
To: Jason Wang
Cc: David Miller, netdev, eric dumazet, xiyou wangcong, weiyongjun1
In-Reply-To: <1676209666.10068041.1566529505528.JavaMail.zimbra@redhat.com>
On 2019/8/23 11:05, Jason Wang wrote:
> ----- Original Message -----
>>
>> On 2019/8/22 14:07, Yang Yingliang wrote:
>>>
>>> On 2019/8/22 10:13, Jason Wang wrote:
>>>> On 2019/8/20 上午10:28, Jason Wang wrote:
>>>>> On 2019/8/20 上午9:25, David Miller wrote:
>>>>>> From: Yang Yingliang <yangyingliang@huawei.com>
>>>>>> Date: Mon, 19 Aug 2019 21:31:19 +0800
>>>>>>
>>>>>>> Call tun_attach() after register_netdevice() to make sure tfile->tun
>>>>>>> is not published until the netdevice is registered. So the read/write
>>>>>>> thread can not use the tun pointer that may freed by free_netdev().
>>>>>>> (The tun and dev pointer are allocated by alloc_netdev_mqs(), they
>>>>>>> can
>>>>>>> be freed by netdev_freemem().)
>>>>>> register_netdevice() must always be the last operation in the order of
>>>>>> network device setup.
>>>>>>
>>>>>> At the point register_netdevice() is called, the device is visible
>>>>>> globally
>>>>>> and therefore all of it's software state must be fully initialized and
>>>>>> ready for us.
>>>>>>
>>>>>> You're going to have to find another solution to these problems.
>>>>>
>>>>> The device is loosely coupled with sockets/queues. Each side is
>>>>> allowed to be go away without caring the other side. So in this
>>>>> case, there's a small window that network stack think the device has
>>>>> one queue but actually not, the code can then safely drop them.
>>>>> Maybe it's ok here with some comments?
>>>>>
>>>>> Or if not, we can try to hold the device before tun_attach and drop
>>>>> it after register_netdevice().
>>>>
>>>> Hi Yang:
>>>>
>>>> I think maybe we can try to hold refcnt instead of playing real num
>>>> queues here. Do you want to post a V4?
>>> I think the refcnt can prevent freeing the memory in this case.
>>> When register_netdevice() failed, free_netdev() will be called directly,
>>> dev->pcpu_refcnt and dev are freed without checking refcnt of dev.
>> How about using patch-v1 that using a flag to check whether the device
>> registered successfully.
>>
> As I said, it lacks sufficient locks or barriers. To be clear, I meant
> something like (compile-test only):
>
> diff --git a/drivers/net/tun.c b/drivers/net/tun.c
> index db16d7a13e00..e52678f9f049 100644
> --- a/drivers/net/tun.c
> +++ b/drivers/net/tun.c
> @@ -2828,6 +2828,7 @@ static int tun_set_iff(struct net *net, struct file *file, struct ifreq *ifr)
> (ifr->ifr_flags & TUN_FEATURES);
>
> INIT_LIST_HEAD(&tun->disabled);
> + dev_hold(dev);
> err = tun_attach(tun, file, false, ifr->ifr_flags & IFF_NAPI,
> ifr->ifr_flags & IFF_NAPI_FRAGS);
> if (err < 0)
> @@ -2836,6 +2837,7 @@ static int tun_set_iff(struct net *net, struct file *file, struct ifreq *ifr)
> err = register_netdevice(tun->dev);
> if (err < 0)
> goto err_detach;
> + dev_put(dev);
> }
>
> netif_carrier_on(tun->dev);
> @@ -2852,11 +2854,13 @@ static int tun_set_iff(struct net *net, struct file *file, struct ifreq *ifr)
> return 0;
>
> err_detach:
> + dev_put(dev);
> tun_detach_all(dev);
> /* register_netdevice() already called tun_free_netdev() */
> goto err_free_dev;
>
> err_free_flow:
> + dev_put(dev);
> tun_flow_uninit(tun);
> security_tun_dev_free_security(tun->security);
> err_free_stat:
>
> What's your thought?
The dev pointer are freed without checking the refcount in free_netdev() called by err_free_dev
path, so I don't understand how the refcount protects this pointer.
Thanks,
Yang
>
> Thanks
>
> .
>
^ permalink raw reply
* Re: [PATCH v6 5/7] nfc: pn533: add UART phy driver
From: Lars Poeschel @ 2019-08-23 10:06 UTC (permalink / raw)
To: Claudiu.Beznea
Cc: gregkh, tglx, swinslow, allison, opensource, kstewart,
linux-kernel, netdev, johan
In-Reply-To: <909777a0-a70e-2174-4455-4afa0591a462@microchip.com>
On Thu, Aug 22, 2019 at 10:09:09AM +0000, Claudiu.Beznea@microchip.com wrote:
> Hi Lars,
>
> On 20.08.2019 15:03, Lars Poeschel wrote:
> > This adds the UART phy interface for the pn533 driver.
> > The pn533 driver can be used through UART interface this way.
> > It is implemented as a serdev device.
> >
> > Cc: Johan Hovold <johan@kernel.org>
> > Signed-off-by: Lars Poeschel <poeschel@lemonage.de>
> > ---
> > Changes in v6:
> > - Rebased the patch series on v5.3-rc5
> >
> > Changes in v5:
> > - Use the splitted pn53x_common_init and pn53x_register_nfc
> > and pn53x_common_clean and pn53x_unregister_nfc alike
> >
> > Changes in v4:
> > - SPDX-License-Identifier: GPL-2.0+
> > - Source code comments above refering items
> > - Error check for serdev_device_write's
> > - Change if (xxx == NULL) to if (!xxx)
> > - Remove device name from a dev_err
> > - move pn533_register in _probe a bit towards the end of _probe
> > - make use of newly added dev_up / dev_down phy_ops
> > - control send_wakeup variable from dev_up / dev_down
> >
> > Changes in v3:
> > - depend on SERIAL_DEV_BUS in Kconfig
> >
> > Changes in v2:
> > - switched from tty line discipline to serdev, resulting in many
> > simplifications
> > - SPDX License Identifier
> >
> > drivers/nfc/pn533/Kconfig | 11 ++
> > drivers/nfc/pn533/Makefile | 2 +
> > drivers/nfc/pn533/pn533.h | 8 +
> > drivers/nfc/pn533/uart.c | 316 +++++++++++++++++++++++++++++++++++++
> > 4 files changed, 337 insertions(+)
> > create mode 100644 drivers/nfc/pn533/uart.c
> >
> > diff --git a/drivers/nfc/pn533/Kconfig b/drivers/nfc/pn533/Kconfig
> > index f6d6b345ba0d..7fe1bbe26568 100644
> > --- a/drivers/nfc/pn533/Kconfig
> > +++ b/drivers/nfc/pn533/Kconfig
> > @@ -26,3 +26,14 @@ config NFC_PN533_I2C
> >
> > If you choose to build a module, it'll be called pn533_i2c.
> > Say N if unsure.
> > +
> > +config NFC_PN532_UART
> > + tristate "NFC PN532 device support (UART)"
> > + depends on SERIAL_DEV_BUS
> > + select NFC_PN533
> > + ---help---
> > + This module adds support for the NXP pn532 UART interface.
> > + Select this if your platform is using the UART bus.
> > +
> > + If you choose to build a module, it'll be called pn532_uart.
> > + Say N if unsure.
> > diff --git a/drivers/nfc/pn533/Makefile b/drivers/nfc/pn533/Makefile
> > index 43c25b4f9466..b9648337576f 100644
> > --- a/drivers/nfc/pn533/Makefile
> > +++ b/drivers/nfc/pn533/Makefile
> > @@ -4,7 +4,9 @@
> > #
> > pn533_usb-objs = usb.o
> > pn533_i2c-objs = i2c.o
> > +pn532_uart-objs = uart.o
> >
> > obj-$(CONFIG_NFC_PN533) += pn533.o
> > obj-$(CONFIG_NFC_PN533_USB) += pn533_usb.o
> > obj-$(CONFIG_NFC_PN533_I2C) += pn533_i2c.o
> > +obj-$(CONFIG_NFC_PN532_UART) += pn532_uart.o
> > diff --git a/drivers/nfc/pn533/pn533.h b/drivers/nfc/pn533/pn533.h
> > index 510ddebbd896..6541088fad73 100644
> > --- a/drivers/nfc/pn533/pn533.h
> > +++ b/drivers/nfc/pn533/pn533.h
> > @@ -43,6 +43,11 @@
> >
> > /* Preamble (1), SoPC (2), ACK Code (2), Postamble (1) */
> > #define PN533_STD_FRAME_ACK_SIZE 6
> > +/*
> > + * Preamble (1), SoPC (2), Packet Length (1), Packet Length Checksum (1),
> > + * Specific Application Level Error Code (1) , Postamble (1)
> > + */
> > +#define PN533_STD_ERROR_FRAME_SIZE 8
> > #define PN533_STD_FRAME_CHECKSUM(f) (f->data[f->datalen])
> > #define PN533_STD_FRAME_POSTAMBLE(f) (f->data[f->datalen + 1])
> > /* Half start code (3), LEN (4) should be 0xffff for extended frame */
> > @@ -84,6 +89,9 @@
> > #define PN533_CMD_MI_MASK 0x40
> > #define PN533_CMD_RET_SUCCESS 0x00
> >
> > +#define PN533_FRAME_DATALEN_ACK 0x00
> > +#define PN533_FRAME_DATALEN_ERROR 0x01
> > +#define PN533_FRAME_DATALEN_EXTENDED 0xFF
> >
> > enum pn533_protocol_type {
> > PN533_PROTO_REQ_ACK_RESP = 0,
> > diff --git a/drivers/nfc/pn533/uart.c b/drivers/nfc/pn533/uart.c
> > new file mode 100644
> > index 000000000000..f1cc2354a4fd
> > --- /dev/null
> > +++ b/drivers/nfc/pn533/uart.c
> > @@ -0,0 +1,316 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + * Driver for NXP PN532 NFC Chip - UART transport layer
> > + *
> > + * Copyright (C) 2018 Lemonage Software GmbH
> > + * Author: Lars Pöschel <poeschel@lemonage.de>
> > + * All rights reserved.
> > + */
> > +
> > +#include <linux/device.h>
> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
> > +#include <linux/nfc.h>
> > +#include <linux/netdevice.h>
> > +#include <linux/of.h>
> > +#include <linux/serdev.h>
> > +#include "pn533.h"
> > +
> > +#define PN532_UART_SKB_BUFF_LEN (PN533_CMD_DATAEXCH_DATA_MAXLEN * 2)
> > +
> > +enum send_wakeup {
> > + PN532_SEND_NO_WAKEUP = 0,
> > + PN532_SEND_WAKEUP,
> > + PN532_SEND_LAST_WAKEUP,
> > +};
> > +
> > +
> > +struct pn532_uart_phy {
> > + struct serdev_device *serdev;
> > + struct sk_buff *recv_skb;
> > + struct pn533 *priv;
> > + enum send_wakeup send_wakeup;
>
> Could there be any concurrency issues w/ regards to accessing this
> variable? I see it is accessed in pn532_uart_send_frame(), pn532_dev_up(),
> pn532_dev_down() which may be called from the following wq:
>
> INIT_WORK(&priv->mi_tm_rx_work, pn533_wq_tm_mi_recv);
>
> INIT_WORK(&priv->mi_tm_tx_work, pn533_wq_tm_mi_send);
>
> INIT_DELAYED_WORK(&priv->poll_work, pn533_wq_poll);
>
>
> and from net/nfc/core.c via dev_up()/dev_down().
Well, I spend some minutes thinking about this. There should be no real
problem. The code in pn533.c ensures, that commands are transmitted
sequencially. And it always is command - response. So if a command is
send, the driver waits for a response from the chip.
So pn532_uart_send_frame should not be called multiple times without
reaching at least serdev_device_write, but at this point the race is
already over.
There is one exception, this is the abort command. This command can be
sent without receiving a previous response. So there is the possibility
of a successful race.
The send_wakeup variable is used to control if we need to send a
wakeup request to the pn532 chip prior to the actual command we would
like to send.
Worst thing that I see could happen - if the race succeeds - is that we
send a wakeup to the chip that is propably not needed as it is already
awake. But this does not hurt as a wakeup send to the pn532 is
essentially a no-op if the chip is awake already. I could have
implemented it so, that a wakeup is sent in front of every command
without thinking and the driver would work.
The same is with pn532_dev_up. It could be that there is one wakeup sent
to much, but it does not hurt.
pn532_dev_down is not problematic I think.
To sum it up: There is maybe a very little probability, but it does
nothing bad. Question is now: Is it worth mutex'ing the send_wakeup
variable or can we leave it as-is ?
Thank you for your review, Claudiu.
Regards,
Lars
^ permalink raw reply
* Re: [PATCH v2 11/11] vsock_test: wait for the remote to close the connection
From: Stefan Hajnoczi @ 2019-08-23 10:09 UTC (permalink / raw)
To: Stefano Garzarella
Cc: netdev, kvm, Dexuan Cui, virtualization, David S. Miller,
Jorgen Hansen, linux-kernel
In-Reply-To: <20190822091546.qcns2kot6tzju7yv@steredhat>
[-- Attachment #1: Type: text/plain, Size: 1639 bytes --]
On Thu, Aug 22, 2019 at 11:15:46AM +0200, Stefano Garzarella wrote:
> On Tue, Aug 20, 2019 at 09:28:28AM +0100, Stefan Hajnoczi wrote:
> > On Thu, Aug 01, 2019 at 05:25:41PM +0200, Stefano Garzarella wrote:
> > > +/* Wait for the remote to close the connection */
> > > +void vsock_wait_remote_close(int fd)
> > > +{
> > > + struct epoll_event ev;
> > > + int epollfd, nfds;
> > > +
> > > + epollfd = epoll_create1(0);
> > > + if (epollfd == -1) {
> > > + perror("epoll_create1");
> > > + exit(EXIT_FAILURE);
> > > + }
> > > +
> > > + ev.events = EPOLLRDHUP | EPOLLHUP;
> > > + ev.data.fd = fd;
> > > + if (epoll_ctl(epollfd, EPOLL_CTL_ADD, fd, &ev) == -1) {
> > > + perror("epoll_ctl");
> > > + exit(EXIT_FAILURE);
> > > + }
> > > +
> > > + nfds = epoll_wait(epollfd, &ev, 1, TIMEOUT * 1000);
> > > + if (nfds == -1) {
> > > + perror("epoll_wait");
> > > + exit(EXIT_FAILURE);
> > > + }
> > > +
> > > + if (nfds == 0) {
> > > + fprintf(stderr, "epoll_wait timed out\n");
> > > + exit(EXIT_FAILURE);
> > > + }
> > > +
> > > + assert(nfds == 1);
> > > + assert(ev.events & (EPOLLRDHUP | EPOLLHUP));
> > > + assert(ev.data.fd == fd);
> > > +
> > > + close(epollfd);
> > > +}
> >
> > Please use timeout_begin()/timeout_end() so that the test cannot hang.
> >
>
> I used the TIMEOUT macro in the epoll_wait() to avoid the hang.
> Do you think is better to use the timeout_begin()/timeout_end()?
> In this case, should I remove the timeout in the epoll_wait()?
Oops, you're right. There are no other blocking calls in this function
so the existing patch is fine.
Thanks,
Stefan
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply
* Re: [PATCH net-next 01/10] net: sched: protect block offload-related fields with rw_semaphore
From: Vlad Buslov @ 2019-08-23 10:25 UTC (permalink / raw)
To: Jakub Kicinski
Cc: Vlad Buslov, netdev@vger.kernel.org, jhs@mojatatu.com,
xiyou.wangcong@gmail.com, jiri@resnulli.us, davem@davemloft.net,
pablo@netfilter.org, Jiri Pirko
In-Reply-To: <20190822151530.09f7ca04@cakuba.netronome.com>
On Fri 23 Aug 2019 at 01:15, Jakub Kicinski <jakub.kicinski@netronome.com> wrote:
> On Thu, 22 Aug 2019 15:43:44 +0300, Vlad Buslov wrote:
>> @@ -2987,19 +3007,26 @@ int tc_setup_cb_call(struct tcf_block *block, enum tc_setup_type type,
>> int ok_count = 0;
>> int err;
>>
>> + down_read(&block->cb_lock);
>> /* Make sure all netdevs sharing this block are offload-capable. */
>> - if (block->nooffloaddevcnt && err_stop)
>> - return -EOPNOTSUPP;
>> + if (block->nooffloaddevcnt && err_stop) {
>> + ok_count = -EOPNOTSUPP;
>> + goto errout;
>> + }
>>
>> list_for_each_entry(block_cb, &block->flow_block.cb_list, list) {
>> err = block_cb->cb(type, type_data, block_cb->cb_priv);
>> if (err) {
>> - if (err_stop)
>> - return err;
>> + if (err_stop) {
>> + ok_count = err;
>> + goto errout;
>> + }
>> } else {
>> ok_count++;
>> }
>> }
>> +errout:
>
> Please name the labels with the first action they perform. Here:
>
> err_unlock:
Thanks for reviewing. Will fix in V2.
>
>> + up_read(&block->cb_lock);
>> return ok_count;
^ permalink raw reply
* Re: kernel BUG at include/linux/skbuff.h:LINE! (2)
From: Xin Long @ 2019-08-23 10:26 UTC (permalink / raw)
To: Dmitry Vyukov
Cc: syzbot, davem, LKML, linux-sctp, Marcelo Ricardo Leitner,
network dev, Neil Horman, syzkaller-bugs, Vlad Yasevich
In-Reply-To: <CADvbK_c7BXurbHyAqjX+0h2ZYtmJ0802zxmQB3qv8=GLv2ig9g@mail.gmail.com>
On Mon, Aug 19, 2019 at 10:44 PM Xin Long <lucien.xin@gmail.com> wrote:
>
> On Sun, Aug 18, 2019 at 10:13 PM Dmitry Vyukov <dvyukov@google.com> wrote:
> >
> > On Sun, Aug 18, 2019 at 7:07 AM Xin Long <lucien.xin@gmail.com> wrote:
> > >
> > > On Sat, Aug 17, 2019 at 2:38 AM syzbot
> > > <syzbot+eb349eeee854e389c36d@syzkaller.appspotmail.com> wrote:
> > > >
> > > > Hello,
> > > >
> > > > syzbot found the following crash on:
> > > >
> > > > HEAD commit: 459c5fb4 Merge branch 'mscc-PTP-support'
> > > > git tree: net-next
> > > > console output: https://syzkaller.appspot.com/x/log.txt?x=13f2d33c600000
> > > > kernel config: https://syzkaller.appspot.com/x/.config?x=d4cf1ffb87d590d7
> > > > dashboard link: https://syzkaller.appspot.com/bug?extid=eb349eeee854e389c36d
> > > > compiler: gcc (GCC) 9.0.0 20181231 (experimental)
> > > > syz repro: https://syzkaller.appspot.com/x/repro.syz?x=111849e2600000
> > > > C reproducer: https://syzkaller.appspot.com/x/repro.c?x=1442c25a600000
> > > >
> > > > IMPORTANT: if you fix the bug, please add the following tag to the commit:
> > > > Reported-by: syzbot+eb349eeee854e389c36d@syzkaller.appspotmail.com
> > > >
> > > > ------------[ cut here ]------------
> > > > kernel BUG at include/linux/skbuff.h:2225!
> > > > invalid opcode: 0000 [#1] PREEMPT SMP KASAN
> > > > CPU: 0 PID: 9030 Comm: syz-executor649 Not tainted 5.3.0-rc3+ #134
> > > > Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS
> > > > Google 01/01/2011
> > > > RIP: 0010:__skb_pull include/linux/skbuff.h:2225 [inline]
> > > > RIP: 0010:__skb_pull include/linux/skbuff.h:2222 [inline]
> > > > RIP: 0010:skb_pull_inline include/linux/skbuff.h:2231 [inline]
> > > > RIP: 0010:skb_pull+0xea/0x110 net/core/skbuff.c:1902
> > > > Code: 9d c8 00 00 00 49 89 dc 49 89 9d c8 00 00 00 e8 9c e5 dd fb 4c 89 e0
> > > > 5b 41 5c 41 5d 41 5e 5d c3 45 31 e4 eb ea e8 86 e5 dd fb <0f> 0b e8 df 13
> > > > 18 fc e9 44 ff ff ff e8 d5 13 18 fc eb 8a e8 ee 13
> > > > RSP: 0018:ffff88808ac96e10 EFLAGS: 00010293
> > > > RAX: ffff88809c546000 RBX: 0000000000000004 RCX: ffffffff8594a3a6
> > > > RDX: 0000000000000000 RSI: ffffffff8594a3fa RDI: 0000000000000004
> > > > RBP: ffff88808ac96e30 R08: ffff88809c546000 R09: fffffbfff14a8f4f
> > > > R10: fffffbfff14a8f4e R11: ffffffff8a547a77 R12: 0000000095e28bcc
> > > > R13: ffff88808ac97478 R14: 00000000ffff8880 R15: ffff88808ac97478
> > > > FS: 0000555556549880(0000) GS:ffff8880ae800000(0000) knlGS:0000000000000000
> > > > CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> > > > CR2: 0000000020000100 CR3: 0000000089c3c000 CR4: 00000000001406f0
> > > > Call Trace:
> > > > sctp_inq_pop+0x2f1/0xd80 net/sctp/inqueue.c:202
> > > > sctp_endpoint_bh_rcv+0x184/0x8d0 net/sctp/endpointola.c:385
> > > > sctp_inq_push+0x1e4/0x280 net/sctp/inqueue.c:80
> > > > sctp_rcv+0x2807/0x3590 net/sctp/input.c:256
> > > > sctp6_rcv+0x17/0x30 net/sctp/ipv6.c:1049
> > > > ip6_protocol_deliver_rcu+0x2fe/0x1660 net/ipv6/ip6_input.c:397
> > > > ip6_input_finish+0x84/0x170 net/ipv6/ip6_input.c:438
> > > > NF_HOOK include/linux/netfilter.h:305 [inline]
> > > > NF_HOOK include/linux/netfilter.h:299 [inline]
> > > > ip6_input+0xe4/0x3f0 net/ipv6/ip6_input.c:447
> > > > dst_input include/net/dst.h:442 [inline]
> > > > ip6_sublist_rcv_finish+0x98/0x1e0 net/ipv6/ip6_input.c:84
> > > Looks skb_list_del_init() should be called in ip6_sublist_rcv_finish,
> > > as does in ip_sublist_rcv_finish().
> >
> > This was recently introduced, right? Only in net-next and linux-next.
> > Otherwise, is it a remote DoS? If so and if it's present in any
> > releases, may need a CVE.
> I need to reproduce and confirm it, will let you know.
The panic could be triggered since the listified RX support for
GRO_NORMAL skbs:
https://patchwork.ozlabs.org/cover/1142808/
(it's only in net-next now, I will post a fix soon)
But the bug itself is not really related with the patch series above.
the issue here is pretty much like what this patch fixed:
https://patchwork.ozlabs.org/patch/942541/
I didn't see a CVE for it, maybe because it was only on net-next too.
^ permalink raw reply
* Re: [RFC bpf-next 0/5] Convert iproute2 to use libbpf (WIP)
From: Jesper Dangaard Brouer @ 2019-08-23 10:27 UTC (permalink / raw)
To: Andrii Nakryiko
Cc: Toke Høiland-Jørgensen, Stephen Hemminger,
Daniel Borkmann, Alexei Starovoitov, Martin KaFai Lau, Song Liu,
Yonghong Song, David Miller, Networking, bpf, brouer,
Anton Protopopov, Stanislav Fomichev, Yoel Caspersen
In-Reply-To: <CAEf4BzZxb7qZabw6aDVaTqnhr3AGtwEo+DbuBR9U9tJr+qVuyg@mail.gmail.com>
On Wed, 21 Aug 2019 13:30:09 -0700
Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote:
> On Tue, Aug 20, 2019 at 4:47 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
> >
> > iproute2 uses its own bpf loader to load eBPF programs, which has
> > evolved separately from libbpf. Since we are now standardising on
> > libbpf, this becomes a problem as iproute2 is slowly accumulating
> > feature incompatibilities with libbpf-based loaders. In particular,
> > iproute2 has its own (expanded) version of the map definition struct,
> > which makes it difficult to write programs that can be loaded with both
> > custom loaders and iproute2.
> >
> > This series seeks to address this by converting iproute2 to using libbpf
> > for all its bpf needs. This version is an early proof-of-concept RFC, to
> > get some feedback on whether people think this is the right direction.
> >
> > What this series does is the following:
> >
> > - Updates the libbpf map definition struct to match that of iproute2
> > (patch 1).
>
>
> Hi Toke,
>
> Thanks for taking a stab at unifying libbpf and iproute2 loaders. I'm
> totally in support of making iproute2 use libbpf to load/initialize
> BPF programs. But I'm against adding iproute2-specific fields to
> libbpf's bpf_map_def definitions to support this.
>
> I've proposed the plan of extending libbpf's supported features so
> that it can be used to load iproute2-style BPF programs earlier,
> please see discussions in [0] and [1]. I think instead of emulating
> iproute2 way of matching everything based on user-specified internal
> IDs, which doesn't provide good user experience and is quite easy to
> get wrong, we should support same scenarios with better declarative
> syntax and in a less error-prone way. I believe we can do that by
> relying on BTF more heavily (again, please check some of my proposals
> in [0], [1], and discussion with Daniel in those threads). It will
> feel more natural and be more straightforward to follow. It would be
> great if you can lend a hand in implementing pieces of that plan!
>
> I'm currently on vacation, so my availability is very sparse, but I'd
> be happy to discuss this further, if need be.
>
> [0] https://lore.kernel.org/bpf/CAEf4BzbfdG2ub7gCi0OYqBrUoChVHWsmOntWAkJt47=FE+km+A@mail.gmail.com/
> [1] https://www.spinics.net/lists/bpf/msg03976.html
>
> > - Adds functionality to libbpf to support automatic pinning of maps when
> > loading an eBPF program, while re-using pinned maps if they already
> > exist (patches 2-3).
For production use-cases, libbpf really need an easier higher-level API
for re-using pinned maps, for establishing shared maps between
programs. The existing libbpf API bpf_object__pin_maps() and
bpf_object__unpin_maps(), which don't re-use pinned maps, are not
really usable, because they pin/unpin ALL maps in the ELF file.
What users really need is an easy way to specify, on a per map basis,
what kind of pinning and reuse/sharing they want. E.g. like iproute2
have, "global", "object-scope", and "no-pinning". ("ifindex-scope" would
be nice for XDP).
Today users have to split/reimplement bpf_prog_load_xattr(), and
use/add bpf_map__reuse_fd(). Which is that I ended doing for
xdp-cpumap-tc[2] (used in production at ISP) resulting in 142 lines of
extra code[3] that should have been hidden inside libbpf. And worse,
in this solution[4] the maps for reuse-pinning is specified in the code
by name. Thus, they cannot use a generic loader. That I why, I want
to mark the maps via a pinning member, like iproute2.
I really hope this moves in a practical direction, as I have the next
production request lined up (also from an ISP), and I hate to have to
advice them to choose the same route as [3].
[2] https://github.com/xdp-project/xdp-cpumap-tc/
[3] https://github.com/xdp-project/xdp-cpumap-tc/blob/master/src/xdp_iphash_to_cpu_user.c#L262-L403
[4] https://github.com/xdp-project/xdp-cpumap-tc/blob/master/src/xdp_iphash_to_cpu_user.c#L431-L441
--
Best regards,
Jesper Dangaard Brouer
MSc.CS, Principal Kernel Engineer at Red Hat
LinkedIn: http://www.linkedin.com/in/brouer
^ permalink raw reply
* Re: [PATCH v2 2/5] spi: spi-fsl-dspi: Exit the ISR with IRQ_NONE when it's not ours
From: Mark Brown @ 2019-08-23 10:28 UTC (permalink / raw)
To: Vladimir Oltean; +Cc: linux-spi, linux-kernel, devicetree, netdev
In-Reply-To: <20190822211514.19288-3-olteanv@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 523 bytes --]
On Fri, Aug 23, 2019 at 12:15:11AM +0300, Vladimir Oltean wrote:
> The DSPI interrupt can be shared between two controllers at least on the
> LX2160A. In that case, the driver for one controller might misbehave and
> consume the other's interrupt. Fix this by actually checking if any of
> the bits in the status register have been asserted.
It would be better to have done this as the first patch before
the restructuring, that way we could send this as a fix - the
refactoring while good doesn't really fit with stable.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply
* Re: [PATCH v2 2/5] spi: spi-fsl-dspi: Exit the ISR with IRQ_NONE when it's not ours
From: Vladimir Oltean @ 2019-08-23 10:30 UTC (permalink / raw)
To: Mark Brown; +Cc: linux-spi, lkml, devicetree, netdev
In-Reply-To: <20190823102816.GN23391@sirena.co.uk>
Hi Mark,
On Fri, 23 Aug 2019 at 13:28, Mark Brown <broonie@kernel.org> wrote:
>
> On Fri, Aug 23, 2019 at 12:15:11AM +0300, Vladimir Oltean wrote:
> > The DSPI interrupt can be shared between two controllers at least on the
> > LX2160A. In that case, the driver for one controller might misbehave and
> > consume the other's interrupt. Fix this by actually checking if any of
> > the bits in the status register have been asserted.
>
> It would be better to have done this as the first patch before
> the restructuring, that way we could send this as a fix - the
> refactoring while good doesn't really fit with stable.
Did you see this?
https://lkml.org/lkml/2019/8/22/1542
Regards,
-Vladimir
^ permalink raw reply
* [RESEND PATCH 3/5] bluetooth: hci_bcm: Give more time to come out of reset
From: megous @ 2019-08-23 10:31 UTC (permalink / raw)
To: Maxime Ripard, Chen-Yu Tsai, Rob Herring, Marcel Holtmann,
Johan Hedberg
Cc: Mark Rutland, David S. Miller, netdev, devicetree, linux-kernel,
linux-arm-kernel, linux-bluetooth, Ondrej Jirman
In-Reply-To: <20190823103139.17687-1-megous@megous.com>
From: Ondrej Jirman <megous@megous.com>
Some supported devices need more time to come out of reset (eg.
BCM4345C5 in AP6256).
I don't have/found a datasheet, so the value was arrive at
experimentally with the Oprange Pi 3 board. Without increased delay,
I got intermittent failures during probe. This is a Bluetooth 5.0
device, so maybe that's why it takes longer to initialize than the
others.
Signed-off-by: Ondrej Jirman <megous@megous.com>
---
drivers/bluetooth/hci_bcm.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/bluetooth/hci_bcm.c b/drivers/bluetooth/hci_bcm.c
index 95c312ae94cf..7646636f2d18 100644
--- a/drivers/bluetooth/hci_bcm.c
+++ b/drivers/bluetooth/hci_bcm.c
@@ -260,7 +260,7 @@ static int bcm_gpio_set_power(struct bcm_device *dev, bool powered)
}
/* wait for device to power on and come out of reset */
- usleep_range(10000, 20000);
+ usleep_range(100000, 120000);
dev->res_enabled = powered;
--
2.23.0
^ permalink raw reply related
* [RESEND PATCH 5/5] arm64: dts: allwinner: orange-pi-3: Enable UART1 / Bluetooth
From: megous @ 2019-08-23 10:31 UTC (permalink / raw)
To: Maxime Ripard, Chen-Yu Tsai, Rob Herring, Marcel Holtmann,
Johan Hedberg
Cc: Mark Rutland, David S. Miller, netdev, devicetree, linux-kernel,
linux-arm-kernel, linux-bluetooth, Ondrej Jirman
In-Reply-To: <20190823103139.17687-1-megous@megous.com>
From: Ondrej Jirman <megous@megous.com>
The board contains AP6256 WiFi/BT module that has its bluetooth part
connected to SoC's UART1 port. Enable this port, and add node for the
bluetooth device.
Bluetooth part is named bcm4345c5.
You'll need a BCM4345C5.hcd firmware file that can be found in the
Xulongs's repository for H6:
https://github.com/orangepi-xunlong/OrangePiH6_external/tree/master/ap6256
The driver expects the firmware at the following path relative to the
firmware directory:
brcm/BCM4345C5.hcd
Signed-off-by: Ondrej Jirman <megous@megous.com>
---
.../dts/allwinner/sun50i-h6-orangepi-3.dts | 19 +++++++++++++++++++
1 file changed, 19 insertions(+)
diff --git a/arch/arm64/boot/dts/allwinner/sun50i-h6-orangepi-3.dts b/arch/arm64/boot/dts/allwinner/sun50i-h6-orangepi-3.dts
index 49d954369087..a9e776446c35 100644
--- a/arch/arm64/boot/dts/allwinner/sun50i-h6-orangepi-3.dts
+++ b/arch/arm64/boot/dts/allwinner/sun50i-h6-orangepi-3.dts
@@ -15,6 +15,7 @@
aliases {
serial0 = &uart0;
+ serial1 = &uart1;
};
chosen {
@@ -271,6 +272,24 @@
status = "okay";
};
+/* There's the BT part of the AP6256 connected to that UART */
+&uart1 {
+ pinctrl-names = "default";
+ pinctrl-0 = <&uart1_pins>, <&uart1_rts_cts_pins>;
+ uart-has-rtscts;
+ status = "okay";
+
+ bluetooth {
+ compatible = "brcm,bcm4345c5";
+ clocks = <&rtc 1>;
+ clock-names = "lpo";
+ device-wakeup-gpios = <&r_pio 1 2 GPIO_ACTIVE_HIGH>; /* PM2 */
+ host-wakeup-gpios = <&r_pio 1 1 GPIO_ACTIVE_HIGH>; /* PM1 */
+ shutdown-gpios = <&r_pio 1 4 GPIO_ACTIVE_HIGH>; /* PM4 */
+ max-speed = <1500000>;
+ };
+};
+
&usb2otg {
/*
* This board doesn't have a controllable VBUS even though it
--
2.23.0
^ permalink raw reply related
* [RESEND PATCH 4/5] arm64: dts: allwinner: h6: Add pin configs for uart1
From: megous @ 2019-08-23 10:31 UTC (permalink / raw)
To: Maxime Ripard, Chen-Yu Tsai, Rob Herring, Marcel Holtmann,
Johan Hedberg
Cc: Mark Rutland, David S. Miller, netdev, devicetree, linux-kernel,
linux-arm-kernel, linux-bluetooth, Ondrej Jirman
In-Reply-To: <20190823103139.17687-1-megous@megous.com>
From: Ondrej Jirman <megous@megous.com>
Orange Pi 3 uses UART1 for bluetooth. Add pinconfigs so that we can use
them.
Signed-off-by: Ondrej Jirman <megous@megous.com>
---
arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi b/arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi
index 67f920e0fc33..7657e816096b 100644
--- a/arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi
+++ b/arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi
@@ -298,6 +298,16 @@
pins = "PH0", "PH1";
function = "uart0";
};
+
+ uart1_pins: uart1-pins {
+ pins = "PG6", "PG7";
+ function = "uart1";
+ };
+
+ uart1_rts_cts_pins: uart1-rts-cts-pins {
+ pins = "PG8", "PG9";
+ function = "uart1";
+ };
};
gic: interrupt-controller@3021000 {
--
2.23.0
^ permalink raw reply related
* [RESEND PATCH 0/5] Add bluetooth support for Orange Pi 3
From: megous @ 2019-08-23 10:31 UTC (permalink / raw)
To: Maxime Ripard, Chen-Yu Tsai, Rob Herring, Marcel Holtmann,
Johan Hedberg
Cc: Mark Rutland, David S. Miller, netdev, devicetree, linux-kernel,
linux-arm-kernel, linux-bluetooth, Ondrej Jirman
From: Ondrej Jirman <megous@megous.com>
(Resend to add missing lists, sorry for the noise.)
This series implements bluetooth support for Xunlong Orange Pi 3 board.
The board uses AP6256 WiFi/BT 5.0 chip.
Summary of changes:
- add more delay to let initialize the chip
- let the kernel detect firmware file path
- add new compatible and update dt-bindings
- update Orange Pi 3 / H6 DTS
Please take a look.
thank you and regards,
Ondrej Jirman
Ondrej Jirman (5):
dt-bindings: net: Add compatible for BCM4345C5 bluetooth device
bluetooth: bcm: Add support for loading firmware for BCM4345C5
bluetooth: hci_bcm: Give more time to come out of reset
arm64: dts: allwinner: h6: Add pin configs for uart1
arm64: dts: allwinner: orange-pi-3: Enable UART1 / Bluetooth
.../bindings/net/broadcom-bluetooth.txt | 1 +
.../dts/allwinner/sun50i-h6-orangepi-3.dts | 19 +++++++++++++++++++
arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi | 10 ++++++++++
drivers/bluetooth/btbcm.c | 3 +++
drivers/bluetooth/hci_bcm.c | 3 ++-
5 files changed, 35 insertions(+), 1 deletion(-)
--
2.23.0
^ permalink raw reply
* [RESEND PATCH 2/5] bluetooth: bcm: Add support for loading firmware for BCM4345C5
From: megous @ 2019-08-23 10:31 UTC (permalink / raw)
To: Maxime Ripard, Chen-Yu Tsai, Rob Herring, Marcel Holtmann,
Johan Hedberg
Cc: Mark Rutland, David S. Miller, netdev, devicetree, linux-kernel,
linux-arm-kernel, linux-bluetooth, Ondrej Jirman
In-Reply-To: <20190823103139.17687-1-megous@megous.com>
From: Ondrej Jirman <megous@megous.com>
Detect BCM4345C5 and load a corresponding firmware file.
Signed-off-by: Ondrej Jirman <megous@megous.com>
---
Checkpatch reports a spurious error.
drivers/bluetooth/btbcm.c | 3 +++
drivers/bluetooth/hci_bcm.c | 1 +
2 files changed, 4 insertions(+)
diff --git a/drivers/bluetooth/btbcm.c b/drivers/bluetooth/btbcm.c
index 124ef0a3e1dd..2d2e6d862068 100644
--- a/drivers/bluetooth/btbcm.c
+++ b/drivers/bluetooth/btbcm.c
@@ -23,6 +23,7 @@
#define BDADDR_BCM43430A0 (&(bdaddr_t) {{0xac, 0x1f, 0x12, 0xa0, 0x43, 0x43}})
#define BDADDR_BCM4324B3 (&(bdaddr_t) {{0x00, 0x00, 0x00, 0xb3, 0x24, 0x43}})
#define BDADDR_BCM4330B1 (&(bdaddr_t) {{0x00, 0x00, 0x00, 0xb1, 0x30, 0x43}})
+#define BDADDR_BCM4345C5 (&(bdaddr_t) {{0xac, 0x1f, 0x00, 0xc5, 0x45, 0x43}})
#define BDADDR_BCM43341B (&(bdaddr_t) {{0xac, 0x1f, 0x00, 0x1b, 0x34, 0x43}})
int btbcm_check_bdaddr(struct hci_dev *hdev)
@@ -73,6 +74,7 @@ int btbcm_check_bdaddr(struct hci_dev *hdev)
!bacmp(&bda->bdaddr, BDADDR_BCM2076B1) ||
!bacmp(&bda->bdaddr, BDADDR_BCM4324B3) ||
!bacmp(&bda->bdaddr, BDADDR_BCM4330B1) ||
+ !bacmp(&bda->bdaddr, BDADDR_BCM4345C5) ||
!bacmp(&bda->bdaddr, BDADDR_BCM43430A0) ||
!bacmp(&bda->bdaddr, BDADDR_BCM43341B)) {
bt_dev_info(hdev, "BCM: Using default device address (%pMR)",
@@ -332,6 +334,7 @@ static const struct bcm_subver_table bcm_uart_subver_table[] = {
{ 0x2122, "BCM4343A0" }, /* 001.001.034 */
{ 0x2209, "BCM43430A1" }, /* 001.002.009 */
{ 0x6119, "BCM4345C0" }, /* 003.001.025 */
+ { 0x6606, "BCM4345C5" }, /* 003.006.006 */
{ 0x230f, "BCM4356A2" }, /* 001.003.015 */
{ 0x220e, "BCM20702A1" }, /* 001.002.014 */
{ 0x4217, "BCM4329B1" }, /* 002.002.023 */
diff --git a/drivers/bluetooth/hci_bcm.c b/drivers/bluetooth/hci_bcm.c
index 4d9de20bab7b..95c312ae94cf 100644
--- a/drivers/bluetooth/hci_bcm.c
+++ b/drivers/bluetooth/hci_bcm.c
@@ -1419,6 +1419,7 @@ static void bcm_serdev_remove(struct serdev_device *serdev)
#ifdef CONFIG_OF
static const struct of_device_id bcm_bluetooth_of_match[] = {
{ .compatible = "brcm,bcm20702a1" },
+ { .compatible = "brcm,bcm4345c5" },
{ .compatible = "brcm,bcm4330-bt" },
{ .compatible = "brcm,bcm43438-bt" },
{ },
--
2.23.0
^ permalink raw reply related
* [RESEND PATCH 1/5] dt-bindings: net: Add compatible for BCM4345C5 bluetooth device
From: megous @ 2019-08-23 10:31 UTC (permalink / raw)
To: Maxime Ripard, Chen-Yu Tsai, Rob Herring, Marcel Holtmann,
Johan Hedberg
Cc: Mark Rutland, David S. Miller, netdev, devicetree, linux-kernel,
linux-arm-kernel, linux-bluetooth, Ondrej Jirman
In-Reply-To: <20190823103139.17687-1-megous@megous.com>
From: Ondrej Jirman <megous@megous.com>
This is present in the AP6526 WiFi/Bluetooth 5.0 module.
Signed-off-by: Ondrej Jirman <megous@megous.com>
---
Documentation/devicetree/bindings/net/broadcom-bluetooth.txt | 1 +
1 file changed, 1 insertion(+)
diff --git a/Documentation/devicetree/bindings/net/broadcom-bluetooth.txt b/Documentation/devicetree/bindings/net/broadcom-bluetooth.txt
index c26f4e11037c..4fa00e2eafcf 100644
--- a/Documentation/devicetree/bindings/net/broadcom-bluetooth.txt
+++ b/Documentation/devicetree/bindings/net/broadcom-bluetooth.txt
@@ -13,6 +13,7 @@ Required properties:
* "brcm,bcm20702a1"
* "brcm,bcm4330-bt"
* "brcm,bcm43438-bt"
+ * "brcm,bcm4345c5"
Optional properties:
--
2.23.0
^ permalink raw reply related
* Re: [PATCH net-next 03/10] net: sched: refactor block offloads counter usage
From: Vlad Buslov @ 2019-08-23 10:39 UTC (permalink / raw)
To: Jakub Kicinski
Cc: Vlad Buslov, netdev@vger.kernel.org, jhs@mojatatu.com,
xiyou.wangcong@gmail.com, jiri@resnulli.us, davem@davemloft.net,
pablo@netfilter.org, Jiri Pirko
In-Reply-To: <20190822155358.0171852c@cakuba.netronome.com>
On Fri 23 Aug 2019 at 01:53, Jakub Kicinski <jakub.kicinski@netronome.com> wrote:
> On Thu, 22 Aug 2019 15:43:46 +0300, Vlad Buslov wrote:
>> Without rtnl lock protection filters can no longer safely manage block
>> offloads counter themselves. Refactor cls API to protect block offloadcnt
>> with tcf_block->cb_lock that is already used to protect driver callback
>> list and nooffloaddevcnt counter. The counter can be modified by concurrent
>> tasks by new functions that execute block callbacks (which is safe with
>> previous patch that changed its type to atomic_t), however, block
>> bind/unbind code that checks the counter value takes cb_lock in write mode
>> to exclude any concurrent modifications. This approach prevents race
>> conditions between bind/unbind and callback execution code but allows for
>> concurrency for tc rule update path.
>>
>> Move block offload counter, filter in hardware counter and filter flags
>> management from classifiers into cls hardware offloads API. Make functions
>> tcf_block_offload_inc() and tcf_block_offload_dec() to be cls API private.
>> Implement following new cls API to be used instead:
>>
>> tc_setup_cb_add() - non-destructive filter add. If filter that wasn't
>> already in hardware is successfully offloaded, increment block offloads
>> counter, set filter in hardware counter and flag. On failure, previously
>> offloaded filter is considered to be intact and offloads counter is not
>> decremented.
>>
>> tc_setup_cb_replace() - destructive filter replace. Release existing
>> filter block offload counter and reset its in hardware counter and flag.
>> Set new filter in hardware counter and flag. On failure, previously
>> offloaded filter is considered to be destroyed and offload counter is
>> decremented.
>>
>> tc_setup_cb_destroy() - filter destroy. Unconditionally decrement block
>> offloads counter.
>>
>> Refactor all offload-capable classifiers to atomically offload filters to
>> hardware, change block offload counter, and set filter in hardware counter
>> and flag by means of the new cls API functions.
>>
>> Signed-off-by: Vlad Buslov <vladbu@mellanox.com>
>> Acked-by: Jiri Pirko <jiri@mellanox.com>
>
> Looks good, minor nits
>
>> diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
>> index 8502bd006b37..4215c849f4a3 100644
>> --- a/net/sched/cls_api.c
>> +++ b/net/sched/cls_api.c
>> @@ -3000,13 +3000,97 @@ int tcf_exts_dump_stats(struct sk_buff *skb, struct tcf_exts *exts)
>> }
>> EXPORT_SYMBOL(tcf_exts_dump_stats);
>>
>> -int tc_setup_cb_call(struct tcf_block *block, enum tc_setup_type type,
>> - void *type_data, bool err_stop)
>> +static void tcf_block_offload_inc(struct tcf_block *block, u32 *flags)
>> +{
>> + if (*flags & TCA_CLS_FLAGS_IN_HW)
>> + return;
>> + *flags |= TCA_CLS_FLAGS_IN_HW;
>> + atomic_inc(&block->offloadcnt);
>> +}
>> +
>> +static void tcf_block_offload_dec(struct tcf_block *block, u32 *flags)
>> +{
>> + if (!(*flags & TCA_CLS_FLAGS_IN_HW))
>> + return;
>> + *flags &= ~TCA_CLS_FLAGS_IN_HW;
>> + atomic_dec(&block->offloadcnt);
>> +}
>> +
>> +void tc_cls_offload_cnt_update(struct tcf_block *block, struct tcf_proto *tp,
>> + u32 *cnt, u32 *flags, u32 diff, bool add)
>> +{
>> + lockdep_assert_held(&block->cb_lock);
>> +
>> + spin_lock(&tp->lock);
>> + if (add) {
>> + if (!*cnt)
>> + tcf_block_offload_inc(block, flags);
>> + (*cnt) += diff;
>
> brackets unnecessary
>
>> + } else {
>> + (*cnt) -= diff;
>> + if (!*cnt)
>> + tcf_block_offload_dec(block, flags);
>> + }
>> + spin_unlock(&tp->lock);
>> +}
>> +EXPORT_SYMBOL(tc_cls_offload_cnt_update);
>> +
>> +static void
>> +tc_cls_offload_cnt_reset(struct tcf_block *block, struct tcf_proto *tp,
>> + u32 *cnt, u32 *flags)
>> +{
>> + lockdep_assert_held(&block->cb_lock);
>> +
>> + spin_lock(&tp->lock);
>> + tcf_block_offload_dec(block, flags);
>> + (*cnt) = 0;
>
> ditto
>
>> + spin_unlock(&tp->lock);
>> +}
>> +
>> +static int
>> +__tc_setup_cb_call(struct tcf_block *block, enum tc_setup_type type,
>> + void *type_data, bool err_stop)
>> {
>> struct flow_block_cb *block_cb;
>> int ok_count = 0;
>> int err;
>>
>> + list_for_each_entry(block_cb, &block->flow_block.cb_list, list) {
>> + err = block_cb->cb(type, type_data, block_cb->cb_priv);
>> + if (err) {
>> + if (err_stop)
>> + return err;
>> + } else {
>> + ok_count++;
>> + }
>> + }
>> + return ok_count;
>> +}
>> +
>> +int tc_setup_cb_call(struct tcf_block *block, enum tc_setup_type type,
>> + void *type_data, bool err_stop, bool rtnl_held)
>> +{
>> + int ok_count;
>> +
>> + down_read(&block->cb_lock);
>> + ok_count = __tc_setup_cb_call(block, type, type_data, err_stop);
>> + up_read(&block->cb_lock);
>> + return ok_count;
>> +}
>> +EXPORT_SYMBOL(tc_setup_cb_call);
>> +
>> +/* Non-destructive filter add. If filter that wasn't already in hardware is
>> + * successfully offloaded, increment block offloads counter. On failure,
>> + * previously offloaded filter is considered to be intact and offloads counter
>> + * is not decremented.
>> + */
>> +
>
> Spurious new line here?
>
>> +int tc_setup_cb_add(struct tcf_block *block, struct tcf_proto *tp,
>> + enum tc_setup_type type, void *type_data, bool err_stop,
>> + u32 *flags, unsigned int *in_hw_count, bool rtnl_held)
>> +{
>> + int ok_count;
>> +
>> down_read(&block->cb_lock);
>> /* Make sure all netdevs sharing this block are offload-capable. */
>> if (block->nooffloaddevcnt && err_stop) {
>> @@ -3014,22 +3098,67 @@ int tc_setup_cb_call(struct tcf_block *block, enum tc_setup_type type,
>> goto errout;
>> }
>>
>> - list_for_each_entry(block_cb, &block->flow_block.cb_list, list) {
>> - err = block_cb->cb(type, type_data, block_cb->cb_priv);
>> - if (err) {
>> - if (err_stop) {
>> - ok_count = err;
>> - goto errout;
>> - }
>> - } else {
>> - ok_count++;
>> - }
>> + ok_count = __tc_setup_cb_call(block, type, type_data, err_stop);
>> + if (ok_count > 0)
>> + tc_cls_offload_cnt_update(block, tp, in_hw_count, flags,
>> + ok_count, true);
>> +errout:
>
> and the labels again
>
>> + up_read(&block->cb_lock);
>> + return ok_count;
>> +}
>> +EXPORT_SYMBOL(tc_setup_cb_add);
>> +
>> +/* Destructive filter replace. If filter that wasn't already in hardware is
>> + * successfully offloaded, increment block offload counter. On failure,
>> + * previously offloaded filter is considered to be destroyed and offload counter
>> + * is decremented.
>> + */
>> +
>
> spurious new line?
>
>> +int tc_setup_cb_replace(struct tcf_block *block, struct tcf_proto *tp,
>> + enum tc_setup_type type, void *type_data, bool err_stop,
>> + u32 *old_flags, unsigned int *old_in_hw_count,
>> + u32 *new_flags, unsigned int *new_in_hw_count,
>> + bool rtnl_held)
>> +{
>> + int ok_count;
>> +
>> + down_read(&block->cb_lock);
>> + /* Make sure all netdevs sharing this block are offload-capable. */
>> + if (block->nooffloaddevcnt && err_stop) {
>> + ok_count = -EOPNOTSUPP;
>> + goto errout;
>> }
>> +
>> + tc_cls_offload_cnt_reset(block, tp, old_in_hw_count, old_flags);
>> +
>> + ok_count = __tc_setup_cb_call(block, type, type_data, err_stop);
>> + if (ok_count > 0)
>> + tc_cls_offload_cnt_update(block, tp, new_in_hw_count, new_flags,
>> + ok_count, true);
>> errout:
>> up_read(&block->cb_lock);
>> return ok_count;
>> }
>> -EXPORT_SYMBOL(tc_setup_cb_call);
>> +EXPORT_SYMBOL(tc_setup_cb_replace);
>> +
>> +/* Destroy filter and decrement block offload counter, if filter was previously
>> + * offloaded.
>> + */
>> +
>
> hm.. is this gap between comment and function it pertains to
> intentional?
Majority of function comments in cls_api.c have newline after them (not
all of them though). I don't have any strong opinions regarding this.
You suggest it is better not to have blank lines after function
comments?
>
>> +int tc_setup_cb_destroy(struct tcf_block *block, struct tcf_proto *tp,
>> + enum tc_setup_type type, void *type_data, bool err_stop,
>> + u32 *flags, unsigned int *in_hw_count, bool rtnl_held)
>> +{
>> + int ok_count;
>> +
>> + down_read(&block->cb_lock);
>> + ok_count = __tc_setup_cb_call(block, type, type_data, err_stop);
>> +
>> + tc_cls_offload_cnt_reset(block, tp, in_hw_count, flags);
>> + up_read(&block->cb_lock);
>> + return ok_count;
>> +}
>> +EXPORT_SYMBOL(tc_setup_cb_destroy);
>>
>> int tc_setup_flow_action(struct flow_action *flow_action,
>> const struct tcf_exts *exts)
>> diff --git a/net/sched/cls_bpf.c b/net/sched/cls_bpf.c
>> index 3f7a9c02b70c..7f304db7e697 100644
>> --- a/net/sched/cls_bpf.c
>> +++ b/net/sched/cls_bpf.c
>> @@ -162,17 +162,21 @@ static int cls_bpf_offload_cmd(struct tcf_proto *tp, struct cls_bpf_prog *prog,
>> cls_bpf.name = obj->bpf_name;
>> cls_bpf.exts_integrated = obj->exts_integrated;
>>
>> - if (oldprog)
>> - tcf_block_offload_dec(block, &oldprog->gen_flags);
>> + if (cls_bpf.oldprog)
>
> why the change from oldprog to cls_bpf.oldprog?
No reason. Looks like a mistake I made when rewriting the conditional
for new tc_setup_cb_*() API.
>
>> + err = tc_setup_cb_replace(block, tp, TC_SETUP_CLSBPF, &cls_bpf,
>> + skip_sw, &oldprog->gen_flags,
>> + &oldprog->in_hw_count,
>> + &prog->gen_flags, &prog->in_hw_count,
>> + true);
>> + else
>> + err = tc_setup_cb_add(block, tp, TC_SETUP_CLSBPF, &cls_bpf,
>> + skip_sw, &prog->gen_flags,
>> + &prog->in_hw_count, true);
>>
>> - err = tc_setup_cb_call(block, TC_SETUP_CLSBPF, &cls_bpf, skip_sw);
>> if (prog) {
>> if (err < 0) {
>> cls_bpf_offload_cmd(tp, oldprog, prog, extack);
>> return err;
>> - } else if (err > 0) {
>> - prog->in_hw_count = err;
>> - tcf_block_offload_inc(block, &prog->gen_flags);
>> }
>> }
>>
>> @@ -230,7 +234,7 @@ static void cls_bpf_offload_update_stats(struct tcf_proto *tp,
>> cls_bpf.name = prog->bpf_name;
>> cls_bpf.exts_integrated = prog->exts_integrated;
>>
>> - tc_setup_cb_call(block, TC_SETUP_CLSBPF, &cls_bpf, false);
>> + tc_setup_cb_call(block, TC_SETUP_CLSBPF, &cls_bpf, false, true);
>> }
>>
>> static int cls_bpf_init(struct tcf_proto *tp)
>> @@ -680,8 +684,8 @@ static int cls_bpf_reoffload(struct tcf_proto *tp, bool add, flow_setup_cb_t *cb
>> continue;
>> }
>>
>> - tc_cls_offload_cnt_update(block, &prog->in_hw_count,
>> - &prog->gen_flags, add);
>> + tc_cls_offload_cnt_update(block, tp, &prog->in_hw_count,
>> + &prog->gen_flags, 1, add);
>
> Since we're adding those higher level add/replace/destroy helpers,
> would it also be possible to have a helper which takes care of
> reoffload? tc_cls_offload_cnt_update() is kind of low level now, it'd
> be cool to also hide it in the core.
Agree. I'll try to come up with something more elegant.
>
>> }
>>
>> return 0;
>> diff --git a/net/sched/cls_flower.c b/net/sched/cls_flower.c
>> index 054123742e32..0001a933d48b 100644
>> --- a/net/sched/cls_flower.c
>> +++ b/net/sched/cls_flower.c
>> @@ -419,10 +419,10 @@ static void fl_hw_destroy_filter(struct tcf_proto *tp, struct cls_fl_filter *f,
>> cls_flower.command = FLOW_CLS_DESTROY;
>> cls_flower.cookie = (unsigned long) f;
>>
>> - tc_setup_cb_call(block, TC_SETUP_CLSFLOWER, &cls_flower, false);
>> + tc_setup_cb_destroy(block, tp, TC_SETUP_CLSFLOWER, &cls_flower, false,
>> + &f->flags, &f->in_hw_count, true);
>> spin_lock(&tp->lock);
>> list_del_init(&f->hw_list);
>> - tcf_block_offload_dec(block, &f->flags);
>> spin_unlock(&tp->lock);
>>
>> if (!rtnl_held)
>> @@ -466,18 +466,15 @@ static int fl_hw_replace_filter(struct tcf_proto *tp,
>> goto errout;
>> }
>>
>> - err = tc_setup_cb_call(block, TC_SETUP_CLSFLOWER, &cls_flower, skip_sw);
>> + err = tc_setup_cb_add(block, tp, TC_SETUP_CLSFLOWER, &cls_flower,
>> + skip_sw, &f->flags, &f->in_hw_count, true);
>> kfree(cls_flower.rule);
>>
>> if (err < 0) {
>> fl_hw_destroy_filter(tp, f, true, NULL);
>> goto errout;
>> } else if (err > 0) {
>> - f->in_hw_count = err;
>> err = 0;
>
> Why does the tc_setup_cb* API still return the positive values, the
> callers should no longer care, right?
Yep. I'll refactor this for V2 and simplify related conditionals in
classifiers.
>
>> - spin_lock(&tp->lock);
>> - tcf_block_offload_inc(block, &f->flags);
>> - spin_unlock(&tp->lock);
>> }
>>
>> if (skip_sw && !(f->flags & TCA_CLS_FLAGS_IN_HW)) {
^ permalink raw reply
* Re: [PATCH net-next 06/10] net: sched: conditionally obtain rtnl lock in cls hw offloads API
From: Vlad Buslov @ 2019-08-23 10:42 UTC (permalink / raw)
To: Jakub Kicinski
Cc: Vlad Buslov, netdev@vger.kernel.org, jhs@mojatatu.com,
xiyou.wangcong@gmail.com, jiri@resnulli.us, davem@davemloft.net,
pablo@netfilter.org, Jiri Pirko
In-Reply-To: <20190822160328.46f7fab7@cakuba.netronome.com>
On Fri 23 Aug 2019 at 02:03, Jakub Kicinski <jakub.kicinski@netronome.com> wrote:
> On Thu, 22 Aug 2019 15:43:49 +0300, Vlad Buslov wrote:
>> diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
>> index 02a547aa77c0..bda42f1b5514 100644
>> --- a/net/sched/cls_api.c
>> +++ b/net/sched/cls_api.c
>> @@ -3076,11 +3076,28 @@ __tc_setup_cb_call(struct tcf_block *block, enum tc_setup_type type,
>> int tc_setup_cb_call(struct tcf_block *block, enum tc_setup_type type,
>> void *type_data, bool err_stop, bool rtnl_held)
>> {
>> + bool take_rtnl = false;
>
> Should we perhaps speculatively:
>
> bool take_rtnl = READ_ONCE(block->lockeddevcnt);
>
> here? It shouldn't hurt, really, and otherwise every offload that
> requires rtnl will have to re-lock cb_lock, every single time..
Great idea! This looks like a straightforward opportunity for
optimization.
>
>> int ok_count;
>>
>> +retry:
>> + if (take_rtnl)
>> + rtnl_lock();
>> down_read(&block->cb_lock);
>> + /* Need to obtain rtnl lock if block is bound to devs that require it.
>> + * In block bind code cb_lock is obtained while holding rtnl, so we must
>> + * obtain the locks in same order here.
>> + */
>> + if (!rtnl_held && !take_rtnl && block->lockeddevcnt) {
>> + up_read(&block->cb_lock);
>> + take_rtnl = true;
>> + goto retry;
>> + }
>> +
>> ok_count = __tc_setup_cb_call(block, type, type_data, err_stop);
>> +
>> up_read(&block->cb_lock);
>> + if (take_rtnl)
>> + rtnl_unlock();
>> return ok_count;
>> }
>> EXPORT_SYMBOL(tc_setup_cb_call);
^ permalink raw reply
* Re: [PATCH v2 2/5] spi: spi-fsl-dspi: Exit the ISR with IRQ_NONE when it's not ours
From: Mark Brown @ 2019-08-23 10:50 UTC (permalink / raw)
To: Vladimir Oltean; +Cc: linux-spi, lkml, devicetree, netdev
In-Reply-To: <CA+h21hoUfbW8Gpyfa+a-vqVp_qARYoq1_eyFfZFh-5USNGNE2g@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 1077 bytes --]
On Fri, Aug 23, 2019 at 01:30:27PM +0300, Vladimir Oltean wrote:
> On Fri, 23 Aug 2019 at 13:28, Mark Brown <broonie@kernel.org> wrote:
> > It would be better to have done this as the first patch before
> > the restructuring, that way we could send this as a fix - the
> > refactoring while good doesn't really fit with stable.
> Did you see this?
> https://lkml.org/lkml/2019/8/22/1542
I'm not online enough to readily follow that link right now, I
did apply another patch for a similar issue though. If that's
a different version of the same change please don't do that,
sending multiple conflicting versions of the same thing creates
conflicts and makes everything harder to work with.
Please include human readable descriptions of things like commits and
issues being discussed in e-mail in your mails, this makes them much
easier for humans to read especially when they have no internet access.
I do frequently catch up on my mail on flights or while otherwise
travelling so this is even more pressing for me than just being about
making things a bit easier to read.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply
* Re: [PATCH v2 2/5] spi: spi-fsl-dspi: Exit the ISR with IRQ_NONE when it's not ours
From: Mark Brown @ 2019-08-23 10:59 UTC (permalink / raw)
To: Vladimir Oltean; +Cc: linux-spi, lkml, devicetree, netdev
In-Reply-To: <20190823105044.GO23391@sirena.co.uk>
[-- Attachment #1: Type: text/plain, Size: 786 bytes --]
On Fri, Aug 23, 2019 at 11:50:44AM +0100, Mark Brown wrote:
> On Fri, Aug 23, 2019 at 01:30:27PM +0300, Vladimir Oltean wrote:
> > Did you see this?
> > https://lkml.org/lkml/2019/8/22/1542
> I'm not online enough to readily follow that link right now, I
> did apply another patch for a similar issue though. If that's
> a different version of the same change please don't do that,
> sending multiple conflicting versions of the same thing creates
> conflicts and makes everything harder to work with.
Oh, I guess this was due to there being an existing refactoring
in -next that meant the fix wouldn't apply directly. I sorted
that out now I think, but in general the same thing applies -
it's better to put fixes before anything else in the series,
it'll flag up when reviewing.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ 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