* 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 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
* [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
* [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 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 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 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 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 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 0/7] s390/qeth: updates 2019-08-23
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
Hi Dave,
please apply one more round of qeth patches. These implement support for
a bunch of TX-related features - namely TX NAPI, BQL and xmit_more.
Note that this includes two qdio patches which lay the necessary
groundwork, and have been acked by Vasily.
Thanks,
Julian
Julian Wiedmann (7):
s390/qdio: enable drivers to poll for Output completions
s390/qdio: let drivers opt-out from Output Queue scanning
s390/qeth: collect accurate TX statistics
s390/qeth: add TX NAPI support for IQD devices
s390/qeth: when in TX NAPI mode, use napi_consume_skb()
s390/qeth: add BQL support for IQD devices
s390/qeth: add xmit_more support for IQD devices
arch/s390/include/asm/qdio.h | 6 +-
drivers/s390/cio/qdio.h | 3 +-
drivers/s390/cio/qdio_main.c | 75 ++++--
drivers/s390/cio/qdio_setup.c | 2 +-
drivers/s390/net/qeth_core.h | 52 ++++
drivers/s390/net/qeth_core_main.c | 410 +++++++++++++++++++++---------
drivers/s390/net/qeth_ethtool.c | 2 +
drivers/s390/net/qeth_l2_main.c | 12 +-
drivers/s390/net/qeth_l3_main.c | 9 +-
9 files changed, 414 insertions(+), 157 deletions(-)
--
2.17.1
^ permalink raw reply
* Re: [PATCH net] s390/qeth: reject oversized SNMP requests
From: Julian Wiedmann @ 2019-08-23 9:31 UTC (permalink / raw)
To: David Miller
Cc: netdev, linux-s390, Heiko Carstens, Stefan Raspl, Ursula Braun,
Dan Carpenter
In-Reply-To: <20190823092923.8507-1-jwi@linux.ibm.com>
+cc Dan
On 23.08.19 11:29, Julian Wiedmann wrote:
> Commit d4c08afafa04 ("s390/qeth: streamline SNMP cmd code") removed
> the bounds checking for req_len, under the assumption that the check in
> qeth_alloc_cmd() would suffice.
>
> But that code path isn't sufficiently robust to handle a user-provided
> data_length, which could overflow (when adding the cmd header overhead)
> before being checked against QETH_BUFSIZE. We end up allocating just a
> tiny iob, and the subsequent copy_from_user() writes past the end of
> that iob.
>
> Special-case this path and add a coarse bounds check, to protect against
> maliciuous requests. This let's the subsequent code flow do its normal
> job and precise checking, without risk of overflow.
>
> Fixes: d4c08afafa04 ("s390/qeth: streamline SNMP cmd code")
> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> Signed-off-by: Julian Wiedmann <jwi@linux.ibm.com>
> Reviewed-by: Ursula Braun <ubraun@linux.ibm.com>
> ---
> drivers/s390/net/qeth_core_main.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/drivers/s390/net/qeth_core_main.c b/drivers/s390/net/qeth_core_main.c
> index 9c3310c4d61d..6502b148541e 100644
> --- a/drivers/s390/net/qeth_core_main.c
> +++ b/drivers/s390/net/qeth_core_main.c
> @@ -4374,6 +4374,10 @@ static int qeth_snmp_command(struct qeth_card *card, char __user *udata)
> get_user(req_len, &ureq->hdr.req_len))
> return -EFAULT;
>
> + /* Sanitize user input, to avoid overflows in iob size calculation: */
> + if (req_len > QETH_BUFSIZE)
> + return -EINVAL;
> +
> iob = qeth_get_adapter_cmd(card, IPA_SETADP_SET_SNMP_CONTROL, req_len);
> if (!iob)
> return -ENOMEM;
>
^ permalink raw reply
* [PATCH net] s390/qeth: reject oversized SNMP requests
From: Julian Wiedmann @ 2019-08-23 9:29 UTC (permalink / raw)
To: David Miller
Cc: netdev, linux-s390, Heiko Carstens, Stefan Raspl, Ursula Braun,
Julian Wiedmann
Commit d4c08afafa04 ("s390/qeth: streamline SNMP cmd code") removed
the bounds checking for req_len, under the assumption that the check in
qeth_alloc_cmd() would suffice.
But that code path isn't sufficiently robust to handle a user-provided
data_length, which could overflow (when adding the cmd header overhead)
before being checked against QETH_BUFSIZE. We end up allocating just a
tiny iob, and the subsequent copy_from_user() writes past the end of
that iob.
Special-case this path and add a coarse bounds check, to protect against
maliciuous requests. This let's the subsequent code flow do its normal
job and precise checking, without risk of overflow.
Fixes: d4c08afafa04 ("s390/qeth: streamline SNMP cmd code")
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: Julian Wiedmann <jwi@linux.ibm.com>
Reviewed-by: Ursula Braun <ubraun@linux.ibm.com>
---
drivers/s390/net/qeth_core_main.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/drivers/s390/net/qeth_core_main.c b/drivers/s390/net/qeth_core_main.c
index 9c3310c4d61d..6502b148541e 100644
--- a/drivers/s390/net/qeth_core_main.c
+++ b/drivers/s390/net/qeth_core_main.c
@@ -4374,6 +4374,10 @@ static int qeth_snmp_command(struct qeth_card *card, char __user *udata)
get_user(req_len, &ureq->hdr.req_len))
return -EFAULT;
+ /* Sanitize user input, to avoid overflows in iob size calculation: */
+ if (req_len > QETH_BUFSIZE)
+ return -EINVAL;
+
iob = qeth_get_adapter_cmd(card, IPA_SETADP_SET_SNMP_CONTROL, req_len);
if (!iob)
return -ENOMEM;
--
2.17.1
^ permalink raw reply related
* Re: libbpf distro packaging
From: Jiri Olsa @ 2019-08-23 9:22 UTC (permalink / raw)
To: Julia Kartseva
Cc: Andrii Nakryiko, labbott@redhat.com, acme@kernel.org,
debian-kernel@lists.debian.org, netdev@vger.kernel.org,
Andrii Nakryiko, Andrey Ignatov, Alexei Starovoitov,
Yonghong Song, jolsa@kernel.org
In-Reply-To: <20190821210906.GA31031@krava>
On Wed, Aug 21, 2019 at 11:09:06PM +0200, Jiri Olsa wrote:
> On Tue, Aug 20, 2019 at 10:27:23PM +0000, Julia Kartseva wrote:
> >
> >
> > On 8/19/19, 11:08 AM, "Julia Kartseva" <hex@fb.com> wrote:
> >
> > On 8/13/19, 11:24 AM, "Andrii Nakryiko" <andrii.nakryiko@gmail.com> wrote:
> >
> > On Tue, Aug 13, 2019 at 5:26 AM Jiri Olsa <jolsa@redhat.com> wrote:
> > >
> > > On Mon, Aug 12, 2019 at 07:04:12PM +0000, Julia Kartseva wrote:
> > > > I would like to bring up libbpf publishing discussion started at [1].
> > > > The present state of things is that libbpf is built from kernel tree, e.g. [2]
> > > > For Debian and [3] for Fedora whereas the better way would be having a
> > > > package built from github mirror. The advantages of the latter:
> > > > - Consistent, ABI matching versioning across distros
> > > > - The mirror has integration tests
> > > > - No need in kernel tree to build a package
> > > > - Changes can be merged directly to github w/o waiting them to be merged
> > > > through bpf-next -> net-next -> main
> > > > There is a PR introducing a libbpf.spec which can be used as a starting point: [4]
> > > > Any comments regarding the spec itself can be posted there.
> > > > In the future it may be used as a source of truth.
> > > > Please consider switching libbpf packaging to the github mirror instead
> > > > of the kernel tree.
> > > > Thanks
> > > >
> > > > [1] https://urldefense.proofpoint.com/v2/url?u=https-3A__lists.iovisor.org_g_iovisor-2Ddev_message_1521&d=DwIBaQ&c=5VD0RTtNlTh3ycd41b3MUw&r=zUrDY_Sp_5PqcGtRQPNeDA&m=prYVDiu3-aH1o2PWH4ZcP7lEQRCQAcTwcWPrJrtaroQ&s=dYAc2jLhFg0wtCZ_ms2HF5bWANoHzA3UMug5TNCeBtE&e=
> > > > [2] https://urldefense.proofpoint.com/v2/url?u=https-3A__packages.debian.org_sid_libbpf4.19&d=DwIBaQ&c=5VD0RTtNlTh3ycd41b3MUw&r=zUrDY_Sp_5PqcGtRQPNeDA&m=prYVDiu3-aH1o2PWH4ZcP7lEQRCQAcTwcWPrJrtaroQ&s=lq1MpF-bt6y6ZEtFc57eT-BO_wMBx8uUBACJooWbUYk&e=
> > > > [3] https://urldefense.proofpoint.com/v2/url?u=http-3A__rpmfind.net_linux_RPM_fedora_devel_rawhide_x86-5F64_l_libbpf-2D5.3.0-2D0.rc2.git0.1.fc31.x86-5F64.html&d=DwIBaQ&c=5VD0RTtNlTh3ycd41b3MUw&r=zUrDY_Sp_5PqcGtRQPNeDA&m=prYVDiu3-aH1o2PWH4ZcP7lEQRCQAcTwcWPrJrtaroQ&s=NoolYHL57G2KhzE768iWdy6v5LD2GfJQyqPmtjy196E&e=
> > > > [4] https://github.com/libbpf/libbpf/pull/64
> > >
> > > hi,
> > > Fedora has libbpf as kernel-tools subpackage, so I think
> > > we'd need to create new package and deprecate the current
> > >
> > > but I like the ABI stability by using github .. how's actually
> > > the sync (in both directions) with kernel sources going on?
> >
> > Sync is always in one direction, from kernel sources into Github repo.
> > Right now it's triggered by a human (usually me), but we are using a
> > script that automates entire process (see
> > https://github.com/libbpf/libbpf/blob/master/scripts/sync-kernel.sh).
> > It cherry-pick relevant commits from kernel, transforms them to match
> > Github's file layout and re-applies those changes to Github repo.
> >
> > There is never a sync from Github back to kernel, but Github repo
> > contains some extra stuff that's not in kernel. E.g., the script I
> > mentioned, plus Github's Makefile is different, because it can't rely
> > on kernel's kbuild setup.
> >
> > Hi Jiri,
> > I'm curious if you have any comments regarding sync procedure described
> > By Andrii. Or if there is anything else you'd like us to address so Fedora
> > can be switched to libbpf built from the github mirror?
>
> hi,
> yea, I think it's ok.. just need to check the implications
> for rhel packaging and I'll let you know
btw, the libbpf GH repo tag v0.0.4 has 0.0.3 version set in Makefile:
VERSION = 0
PATCHLEVEL = 0
EXTRAVERSION = 3
current code takes version from libbpf.map so it's fine,
but would be great to start from 0.0.5 so we don't need to
bother with rpm patches.. is 0.0.5 planned soon?
thanks,
jirka
^ permalink raw reply
* Re: Bridge zeros out skb->cb in 5.2 (not in 5.1)
From: Florian Westphal @ 2019-08-23 9:12 UTC (permalink / raw)
To: Dan Siemon; +Cc: netdev, bpf
In-Reply-To: <CAB2AaTmtwpHKvuOi6a-54SW1JXUxt1N03Lb=GXMVv_-y+zYyEA@mail.gmail.com>
Dan Siemon <dan@coverfire.com> wrote:
[ CCing bpf maling list ]
> Commit f12064d1b402c60c5db9c4b63d5ed6d7facb33f6 zeros out skb->cb in
> br_input.c:
>
> memset(skb->cb, 0, sizeof(struct br_input_skb_cb));
>
> https://github.com/torvalds/linux/commit/f12064d1b402c60c5db9c4b63d5ed6d7facb33f6
>
> Prior to 5.2, it was possible to store information in skb->cb and have it
> pass through the bridge to the output link.
I did not know this was even possible.
Any owner of the skb (bridge, ip stack, etc.) use skb->cb[] as they see fit.
> We leveraged this to have a BPF
> prog that runs on ingress and does custom packet parsing and stores the
> output qdisc:class in skb->cb. This enabled the egress BPF filter to be
> super simple and avoid having to parse the entire packet again.
>
> Note I haven't built with this patch removed so it's possible this isn't
> the problem but the memset is unconditional...
You're not exactly saying what the problem is, so I have no idea.
> Is this a regression? Is it expected that the bridge would wipe this field
> when just passing frames?
Even if you remove the memset, that commit br_input_skb_cb
has existed, and is used. Fields were just cleared on-demand rather
than unconditionally at the start.
I think the latter is better practice and also what other owners do.
So please explain what exactly the problem is and/or check that the
cb clearing "is the problem".
If it is, I have no idea how to fix it.
^ permalink raw reply
* Re: [PATCH v6 4/7] nfc: pn533: Split pn533 init & nfc_register
From: Lars Poeschel @ 2019-08-23 9:07 UTC (permalink / raw)
To: Claudiu.Beznea
Cc: gregkh, allison, swinslow, tglx, kstewart, gustavo, keescook,
opensource, netdev, linux-kernel, johan
In-Reply-To: <67dbe5a1-35e7-0ac9-efbc-6425b3628b18@microchip.com>
On Thu, Aug 22, 2019 at 10:08:40AM +0000, Claudiu.Beznea@microchip.com wrote:
>
>
> On 20.08.2019 15:03, Lars Poeschel wrote:
> > There is a problem in the initialisation and setup of the pn533: It
> > registers with nfc too early. It could happen, that it finished
> > registering with nfc and someone starts using it. But setup of the pn533
> > is not yet finished. Bad or at least unintended things could happen.
> > So I split out nfc registering (and unregistering) to seperate functions
> > that have to be called late in probe then.
> >
> > 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:
> > - This patch is new in v5
> >
> > drivers/nfc/pn533/i2c.c | 17 +++++-----
> > drivers/nfc/pn533/pn533.c | 66 ++++++++++++++++++++-------------------
> > drivers/nfc/pn533/pn533.h | 11 ++++---
> > drivers/nfc/pn533/usb.c | 12 +++++--
> > 4 files changed, 59 insertions(+), 47 deletions(-)
> >
> > diff --git a/drivers/nfc/pn533/i2c.c b/drivers/nfc/pn533/i2c.c
> > index 1abd40398a5a..e9e5a1ec8857 100644
> > --- a/drivers/nfc/pn533/i2c.c
> > +++ b/drivers/nfc/pn533/i2c.c
> > @@ -193,12 +193,10 @@ static int pn533_i2c_probe(struct i2c_client *client,
> > phy->i2c_dev = client;
> > i2c_set_clientdata(client, phy);
> >
> > - priv = pn533_register_device(PN533_DEVICE_PN532,
> > - PN533_NO_TYPE_B_PROTOCOLS,
> > + priv = pn53x_common_init(PN533_DEVICE_PN532,
> > PN533_PROTO_REQ_ACK_RESP,
> > phy, &i2c_phy_ops, NULL,
> > - &phy->i2c_dev->dev,
> > - &client->dev);
> > + &phy->i2c_dev->dev);
> >
> > if (IS_ERR(priv)) {
> > r = PTR_ERR(priv);
> > @@ -220,13 +218,17 @@ static int pn533_i2c_probe(struct i2c_client *client,
> > if (r)
> > goto fn_setup_err;
> >
> > - return 0;
> > + r = pn53x_register_nfc(priv, PN533_NO_TYPE_B_PROTOCOLS, &client->dev);
> > + if (r)
> > + goto fn_setup_err;
> > +
> > + return r;
> >
> > fn_setup_err:
> > free_irq(client->irq, phy);
> >
> > irq_rqst_err:
> > - pn533_unregister_device(phy->priv);
> > + pn53x_common_clean(phy->priv);
> >
> > return r;
> > }
> > @@ -239,7 +241,8 @@ static int pn533_i2c_remove(struct i2c_client *client)
> >
> > free_irq(client->irq, phy);
> >
> > - pn533_unregister_device(phy->priv);
> > + pn53x_unregister_nfc(phy->priv);
> > + pn53x_common_clean(phy->priv);
> >
> > return 0;
> > }
> > diff --git a/drivers/nfc/pn533/pn533.c b/drivers/nfc/pn533/pn533.c
> > index 64836c727aee..a8c756caa678 100644
> > --- a/drivers/nfc/pn533/pn533.c
> > +++ b/drivers/nfc/pn533/pn533.c
> > @@ -2590,14 +2590,12 @@ int pn533_finalize_setup(struct pn533 *dev)
> > }
> > EXPORT_SYMBOL_GPL(pn533_finalize_setup);
> >
> > -struct pn533 *pn533_register_device(u32 device_type,
> > - u32 protocols,
> > +struct pn533 *pn53x_common_init(u32 device_type,
> > enum pn533_protocol_type protocol_type,
> > void *phy,
> > struct pn533_phy_ops *phy_ops,
> > struct pn533_frame_ops *fops,
> > - struct device *dev,
> > - struct device *parent)
> > + struct device *dev)
> > {
> > struct pn533 *priv;
> > int rc = -ENOMEM;
> > @@ -2638,43 +2636,18 @@ struct pn533 *pn533_register_device(u32 device_type,
> > skb_queue_head_init(&priv->fragment_skb);
> >
> > INIT_LIST_HEAD(&priv->cmd_queue);
> > -
> > - priv->nfc_dev = nfc_allocate_device(&pn533_nfc_ops, protocols,
> > - priv->ops->tx_header_len +
> > - PN533_CMD_DATAEXCH_HEAD_LEN,
> > - priv->ops->tx_tail_len);
> > - if (!priv->nfc_dev) {
> > - rc = -ENOMEM;
> > - goto destroy_wq;
> > - }
> > -
> > - nfc_set_parent_dev(priv->nfc_dev, parent);
> > - nfc_set_drvdata(priv->nfc_dev, priv);
> > -
> > - rc = nfc_register_device(priv->nfc_dev);
> > - if (rc)
> > - goto free_nfc_dev;
> > -
> > return priv;
> >
> > -free_nfc_dev:
> > - nfc_free_device(priv->nfc_dev);
> > -
> > -destroy_wq:
> > - destroy_workqueue(priv->wq);
> > error:
> > kfree(priv);
> > return ERR_PTR(rc);
> > }
> > -EXPORT_SYMBOL_GPL(pn533_register_device);
> > +EXPORT_SYMBOL_GPL(pn53x_common_init);
> >
> > -void pn533_unregister_device(struct pn533 *priv)
> > +void pn53x_common_clean(struct pn533 *priv)
> > {
> > struct pn533_cmd *cmd, *n;
> >
> > - nfc_unregister_device(priv->nfc_dev);
> > - nfc_free_device(priv->nfc_dev);
> > -
> > flush_delayed_work(&priv->poll_work);
> > destroy_workqueue(priv->wq);
> >
> > @@ -2689,8 +2662,37 @@ void pn533_unregister_device(struct pn533 *priv)
> >
> > kfree(priv);
> > }
> > -EXPORT_SYMBOL_GPL(pn533_unregister_device);
> > +EXPORT_SYMBOL_GPL(pn53x_common_clean);
> > +
> > +int pn53x_register_nfc(struct pn533 *priv, u32 protocols,
> > + struct device *parent)
> > +{
> > + int rc = -ENOMEM;
>
> No need to initialize rc here... or just return rc below.
I will remove the initialization. Looks better to me.
> > +
> > + priv->nfc_dev = nfc_allocate_device(&pn533_nfc_ops, protocols,
> > + priv->ops->tx_header_len +
> > + PN533_CMD_DATAEXCH_HEAD_LEN,
> > + priv->ops->tx_tail_len);
> > + if (!priv->nfc_dev)
> > + return -ENOMEM;
> > +
> > + nfc_set_parent_dev(priv->nfc_dev, parent);
> > + nfc_set_drvdata(priv->nfc_dev, priv);
> > +
> > + rc = nfc_register_device(priv->nfc_dev);
> > + if (rc)
> > + nfc_free_device(priv->nfc_dev);
> > +
> > + return rc;
> > +}
> > +EXPORT_SYMBOL_GPL(pn53x_register_nfc);
> >
> > +void pn53x_unregister_nfc(struct pn533 *priv)
> > +{
> > + nfc_unregister_device(priv->nfc_dev);
> > + nfc_free_device(priv->nfc_dev);
> > +}
> > +EXPORT_SYMBOL_GPL(pn53x_unregister_nfc);
> >
> > MODULE_AUTHOR("Lauro Ramos Venancio <lauro.venancio@openbossa.org>");
> > MODULE_AUTHOR("Aloisio Almeida Jr <aloisio.almeida@openbossa.org>");
> > diff --git a/drivers/nfc/pn533/pn533.h b/drivers/nfc/pn533/pn533.h
> > index 570ee0a3e832..510ddebbd896 100644
> > --- a/drivers/nfc/pn533/pn533.h
> > +++ b/drivers/nfc/pn533/pn533.h
> > @@ -219,18 +219,19 @@ struct pn533_phy_ops {
> > };
> >
> >
> > -struct pn533 *pn533_register_device(u32 device_type,
> > - u32 protocols,
> > +struct pn533 *pn53x_common_init(u32 device_type,
> > enum pn533_protocol_type protocol_type,
> > void *phy,
> > struct pn533_phy_ops *phy_ops,
> > struct pn533_frame_ops *fops,
> > - struct device *dev,
> > - struct device *parent);
> > + struct device *dev);
> >
> > int pn533_finalize_setup(struct pn533 *dev);
> > -void pn533_unregister_device(struct pn533 *priv);
> > +void pn53x_common_clean(struct pn533 *priv);
> > void pn533_recv_frame(struct pn533 *dev, struct sk_buff *skb, int status);
> > +int pn53x_register_nfc(struct pn533 *priv, u32 protocols,
> > + struct device *parent);
> > +void pn53x_unregister_nfc(struct pn533 *priv);
> >
> > bool pn533_rx_frame_is_cmd_response(struct pn533 *dev, void *frame);
> > bool pn533_rx_frame_is_ack(void *_frame);
> > diff --git a/drivers/nfc/pn533/usb.c b/drivers/nfc/pn533/usb.c
> > index c5289eaf17ee..a1c6a41944c6 100644
> > --- a/drivers/nfc/pn533/usb.c
> > +++ b/drivers/nfc/pn533/usb.c
> > @@ -534,9 +534,9 @@ static int pn533_usb_probe(struct usb_interface *interface,
> > goto error;
> > }
> >
> > - priv = pn533_register_device(id->driver_info, protocols, protocol_type,
> > + priv = pn53x_common_init(id->driver_info, protocol_type,
> > phy, &usb_phy_ops, fops,
> > - &phy->udev->dev, &interface->dev);
> > + &phy->udev->dev);
> >
> > if (IS_ERR(priv)) {
> > rc = PTR_ERR(priv);
> > @@ -550,9 +550,14 @@ static int pn533_usb_probe(struct usb_interface *interface,
> > goto error;
> >
> > usb_set_intfdata(interface, phy);
>
> Above this instruction there is this code:
>
> rc = pn533_finalize_setup(priv);
> if (rc)
> goto error;
>
> Instead of "goto error;" you should have "goto err_clean;"
Thank you for catching this one. I will change it.
> > + rc = pn53x_register_nfc(priv, protocols, &interface->dev);
> > + if (rc)
> > + goto err_clean;
> >
> > return 0;
> >
> > +err_clean:
> > + pn53x_common_clean(priv);
> > error:
> > usb_free_urb(phy->in_urb);
> > usb_free_urb(phy->out_urb);
> > @@ -570,7 +575,8 @@ static void pn533_usb_disconnect(struct usb_interface *interface)
> > if (!phy)
> > return;
> >
> > - pn533_unregister_device(phy->priv);
> > + pn53x_unregister_nfc(phy->priv);
> > + pn53x_common_clean(phy->priv);
> >
> > usb_set_intfdata(interface, NULL);
> >
> >
^ permalink raw reply
* [PATCH net 2/2] r8152: avoid using napi_disable after netif_napi_del.
From: Hayes Wang @ 2019-08-23 8:53 UTC (permalink / raw)
To: netdev; +Cc: nic_swsd, linux-kernel, jslaby, Hayes Wang
In-Reply-To: <1394712342-15778-314-Taiwan-albertk@realtek.com>
Exchange netif_napi_del() and unregister_netdev() in rtl8152_disconnect()
to avoid using napi_disable() after netif_napi_del().
Signed-off-by: Hayes Wang <hayeswang@realtek.com>
---
drivers/net/usb/r8152.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
index 690a24d1ef82..29390eda5251 100644
--- a/drivers/net/usb/r8152.c
+++ b/drivers/net/usb/r8152.c
@@ -5364,8 +5364,8 @@ static void rtl8152_disconnect(struct usb_interface *intf)
if (tp) {
rtl_set_unplug(tp);
- netif_napi_del(&tp->napi);
unregister_netdev(tp->netdev);
+ netif_napi_del(&tp->napi);
cancel_delayed_work_sync(&tp->hw_phy_work);
tp->rtl_ops.unload(tp);
free_netdev(tp->netdev);
--
2.21.0
^ permalink raw reply related
* [PATCH net 1/2] Revert "r8152: napi hangup fix after disconnect"
From: Hayes Wang @ 2019-08-23 8:53 UTC (permalink / raw)
To: netdev; +Cc: nic_swsd, linux-kernel, jslaby, Hayes Wang
In-Reply-To: <1394712342-15778-314-Taiwan-albertk@realtek.com>
This reverts commit 0ee1f4734967af8321ecebaf9c74221ace34f2d5.
This conflicts with commit ffa9fec30ca0 ("r8152: set
RTL8152_UNPLUG only for real disconnection").
Signed-off-by: Hayes Wang <hayeswang@realtek.com>
---
drivers/net/usb/r8152.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
index 0cc03a9ff545..690a24d1ef82 100644
--- a/drivers/net/usb/r8152.c
+++ b/drivers/net/usb/r8152.c
@@ -4018,8 +4018,7 @@ static int rtl8152_close(struct net_device *netdev)
#ifdef CONFIG_PM_SLEEP
unregister_pm_notifier(&tp->pm_notifier);
#endif
- if (!test_bit(RTL8152_UNPLUG, &tp->flags))
- napi_disable(&tp->napi);
+ napi_disable(&tp->napi);
clear_bit(WORK_ENABLE, &tp->flags);
usb_kill_urb(tp->intr_urb);
cancel_delayed_work_sync(&tp->schedule);
--
2.21.0
^ permalink raw reply related
* [PATCH net 0/2] r8152: fix side effect
From: Hayes Wang @ 2019-08-23 8:53 UTC (permalink / raw)
To: netdev; +Cc: nic_swsd, linux-kernel, jslaby, Hayes Wang
The commit 0ee1f4734967 ("r8152: napi hangup fix after disconnect")
add a check to avoid using napi_disable after netif_napi_del. However,
the commit ffa9fec30ca0 ("r8152: set RTL8152_UNPLUG only for real
disconnection") let the check useless.
Therefore, I revert commit 0ee1f4734967 ("r8152: napi hangup fix
after disconnect") first, and add another patch to fix it.
Hayes Wang (2):
Revert "r8152: napi hangup fix after disconnect"
r8152: avoid using napi_disable after netif_napi_del.
drivers/net/usb/r8152.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
--
2.21.0
^ permalink raw reply
* [PATCH] wimax/i2400m: fix calculation of index, remove sizeof
From: Colin King @ 2019-08-23 8:52 UTC (permalink / raw)
To: Inaky Perez-Gonzalez, linux-wimax, David S . Miller, netdev
Cc: kernel-janitors, linux-kernel
From: Colin Ian King <colin.king@canonical.com>
The subtraction of the two pointers is automatically scaled by the
size of the size of the object the pointers point to, so the division
by sizeof(*i2400m->barker) is incorrect. Fix this by removing the
division. Also make index an unsigned int to clean up a checkpatch
warning.
Addresses-Coverity: ("Extra sizeof expression")
Fixes: aba3792ac2d7 ("wimax/i2400m: rework bootrom initialization to be more flexible")
Signed-off-by: Colin Ian King <colin.king@canonical.com>
---
drivers/net/wimax/i2400m/fw.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/net/wimax/i2400m/fw.c b/drivers/net/wimax/i2400m/fw.c
index 489cba9b284d..599a703af6eb 100644
--- a/drivers/net/wimax/i2400m/fw.c
+++ b/drivers/net/wimax/i2400m/fw.c
@@ -399,8 +399,7 @@ int i2400m_is_boot_barker(struct i2400m *i2400m,
* associated with the device. */
if (i2400m->barker
&& !memcmp(buf, i2400m->barker, sizeof(i2400m->barker->data))) {
- unsigned index = (i2400m->barker - i2400m_barker_db)
- / sizeof(*i2400m->barker);
+ unsigned int index = i2400m->barker - i2400m_barker_db;
d_printf(2, dev, "boot barker cache-confirmed #%u/%08x\n",
index, le32_to_cpu(i2400m->barker->data[0]));
return 0;
--
2.20.1
^ permalink raw reply related
* Re: [PATCH net 0/9] rxrpc: Fix use of skb_cow_data()
From: David Howells @ 2019-08-23 8:52 UTC (permalink / raw)
To: David Miller; +Cc: dhowells, netdev, linux-afs, linux-kernel
In-Reply-To: <20190822.121207.731320146177703787.davem@davemloft.net>
David Miller <davem@davemloft.net> wrote:
> Why don't you just do an skb_unshare() at the beginning when you know that
> you'll need to do that?
I was trying to defer any copying to process context rather than doing it in
softirq context to spend less time in softirq context - plus that way I can
use GFP_NOIO (kafs) or GFP_KERNEL (direct AF_RXRPC socket) rather than
GFP_ATOMIC if the api supports it.
I don't remember now why I used skb_cow_data() rather than skb_unshare() - but
it was probably because the former leaves the sk_buff object itself intact,
whereas the latter replaces it. I can switch to using skb_unshare() instead.
Question for you: how likely is a newly received buffer, through a UDP socket,
to be 'cloned'?
David
^ permalink raw reply
* Re: [PATCH] brcmfmac: replace strncpy() by strscpy()
From: Arend Van Spriel @ 2019-08-23 8:18 UTC (permalink / raw)
To: Xulin Sun, kvalo
Cc: stefan.wahren, linux-kernel, netdev, brcm80211-dev-list,
brcm80211-dev-list.pdl, linux-wireless, franky.lin,
hante.meuleman, chi-hsien.lin, wright.feng, davem, stanley.hsu
In-Reply-To: <20190823074708.20081-1-xulin.sun@windriver.com>
On 8/23/2019 9:47 AM, Xulin Sun wrote:
> The strncpy() may truncate the copied string,
> replace it by the safer strscpy().
>
> To avoid below compile warning with gcc 8.2:
>
> drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c:In function 'brcmf_vndr_ie':
> drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c:4227:2:
> warning: 'strncpy' output truncated before terminating nul copying 3 bytes from a string of the same length [-Wstringop-truncation]
> strncpy(iebuf, add_del_cmd, VNDR_IE_CMD_LEN - 1);
> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Acked-by: Arend van Spriel <arend.vanspriel@broadcom.com>
> Signed-off-by: Xulin Sun <xulin.sun@windriver.com>
> ---
> drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c | 4 +---
> 1 file changed, 1 insertion(+), 3 deletions(-)
^ permalink raw reply
* RE: [PATCH v2 0/2] Simplify mtty driver and mdev core
From: Parav Pandit @ 2019-08-23 8:14 UTC (permalink / raw)
To: Jiri Pirko
Cc: Alex Williamson, Jiri Pirko, David S . Miller, Kirti Wankhede,
Cornelia Huck, kvm@vger.kernel.org, linux-kernel@vger.kernel.org,
cjia, netdev@vger.kernel.org
In-Reply-To: <20190823081221.GG2276@nanopsycho.orion>
Hi Alex,
> -----Original Message-----
> From: Jiri Pirko <jiri@resnulli.us>
> Sent: Friday, August 23, 2019 1:42 PM
> To: Parav Pandit <parav@mellanox.com>
> Cc: Alex Williamson <alex.williamson@redhat.com>; Jiri Pirko
> <jiri@mellanox.com>; David S . Miller <davem@davemloft.net>; Kirti
> Wankhede <kwankhede@nvidia.com>; Cornelia Huck <cohuck@redhat.com>;
> kvm@vger.kernel.org; linux-kernel@vger.kernel.org; cjia <cjia@nvidia.com>;
> netdev@vger.kernel.org
> Subject: Re: [PATCH v2 0/2] Simplify mtty driver and mdev core
>
> Thu, Aug 22, 2019 at 03:33:30PM CEST, parav@mellanox.com wrote:
> >
> >
> >> -----Original Message-----
> >> From: Jiri Pirko <jiri@resnulli.us>
> >> Sent: Thursday, August 22, 2019 5:50 PM
> >> To: Parav Pandit <parav@mellanox.com>
> >> Cc: Alex Williamson <alex.williamson@redhat.com>; Jiri Pirko
> >> <jiri@mellanox.com>; David S . Miller <davem@davemloft.net>; Kirti
> >> Wankhede <kwankhede@nvidia.com>; Cornelia Huck
> <cohuck@redhat.com>;
> >> kvm@vger.kernel.org; linux-kernel@vger.kernel.org; cjia
> >> <cjia@nvidia.com>; netdev@vger.kernel.org
> >> Subject: Re: [PATCH v2 0/2] Simplify mtty driver and mdev core
> >>
> >> Thu, Aug 22, 2019 at 12:04:02PM CEST, parav@mellanox.com wrote:
> >> >
> >> >
> >> >> -----Original Message-----
> >> >> From: Jiri Pirko <jiri@resnulli.us>
> >> >> Sent: Thursday, August 22, 2019 3:28 PM
> >> >> To: Parav Pandit <parav@mellanox.com>
> >> >> Cc: Alex Williamson <alex.williamson@redhat.com>; Jiri Pirko
> >> >> <jiri@mellanox.com>; David S . Miller <davem@davemloft.net>; Kirti
> >> >> Wankhede <kwankhede@nvidia.com>; Cornelia Huck
> >> <cohuck@redhat.com>;
> >> >> kvm@vger.kernel.org; linux-kernel@vger.kernel.org; cjia
> >> >> <cjia@nvidia.com>; netdev@vger.kernel.org
> >> >> Subject: Re: [PATCH v2 0/2] Simplify mtty driver and mdev core
> >> >>
> >> >> Thu, Aug 22, 2019 at 11:42:13AM CEST, parav@mellanox.com wrote:
> >> >> >
> >> >> >
> >> >> >> -----Original Message-----
> >> >> >> From: Jiri Pirko <jiri@resnulli.us>
> >> >> >> Sent: Thursday, August 22, 2019 2:59 PM
> >> >> >> To: Parav Pandit <parav@mellanox.com>
> >> >> >> Cc: Alex Williamson <alex.williamson@redhat.com>; Jiri Pirko
> >> >> >> <jiri@mellanox.com>; David S . Miller <davem@davemloft.net>;
> >> >> >> Kirti Wankhede <kwankhede@nvidia.com>; Cornelia Huck
> >> >> <cohuck@redhat.com>;
> >> >> >> kvm@vger.kernel.org; linux-kernel@vger.kernel.org; cjia
> >> >> >> <cjia@nvidia.com>; netdev@vger.kernel.org
> >> >> >> Subject: Re: [PATCH v2 0/2] Simplify mtty driver and mdev core
> >> >> >>
> >> >> >> Wed, Aug 21, 2019 at 08:23:17AM CEST, parav@mellanox.com wrote:
> >> >> >> >
> >> >> >> >
> >> >> >> >> -----Original Message-----
> >> >> >> >> From: Alex Williamson <alex.williamson@redhat.com>
> >> >> >> >> Sent: Wednesday, August 21, 2019 10:56 AM
> >> >> >> >> To: Parav Pandit <parav@mellanox.com>
> >> >> >> >> Cc: Jiri Pirko <jiri@mellanox.com>; David S . Miller
> >> >> >> >> <davem@davemloft.net>; Kirti Wankhede
> >> >> >> >> <kwankhede@nvidia.com>; Cornelia Huck <cohuck@redhat.com>;
> >> >> >> >> kvm@vger.kernel.org; linux-kernel@vger.kernel.org; cjia
> >> >> >> >> <cjia@nvidia.com>; netdev@vger.kernel.org
> >> >> >> >> Subject: Re: [PATCH v2 0/2] Simplify mtty driver and mdev
> >> >> >> >> core
> >> >> >> >>
> >> >> >> >> > > > > Just an example of the alias, not proposing how it's set.
> >> >> >> >> > > > > In fact, proposing that the user does not set it,
> >> >> >> >> > > > > mdev-core provides one
> >> >> >> >> > > automatically.
> >> >> >> >> > > > >
> >> >> >> >> > > > > > > Since there seems to be some prefix overhead, as
> >> >> >> >> > > > > > > I ask about above in how many characters we
> >> >> >> >> > > > > > > actually have to work with in IFNAMESZ, maybe we
> >> >> >> >> > > > > > > start with
> >> >> >> >> > > > > > > 8 characters (matching your "index" namespace)
> >> >> >> >> > > > > > > and expand as necessary for
> >> >> >> disambiguation.
> >> >> >> >> > > > > > > If we can eliminate overhead in IFNAMESZ, let's
> >> >> >> >> > > > > > > start with
> >> 12.
> >> >> >> >> > > > > > > Thanks,
> >> >> >> >> > > > > > >
> >> >> >> >> > > > > > If user is going to choose the alias, why does it
> >> >> >> >> > > > > > have to be limited to
> >> >> >> >> sha1?
> >> >> >> >> > > > > > Or you just told it as an example?
> >> >> >> >> > > > > >
> >> >> >> >> > > > > > It can be an alpha-numeric string.
> >> >> >> >> > > > >
> >> >> >> >> > > > > No, I'm proposing a different solution where
> >> >> >> >> > > > > mdev-core creates an alias based on an abbreviated
> >> >> >> >> > > > > sha1. The user does not provide the
> >> >> >> >> alias.
> >> >> >> >> > > > >
> >> >> >> >> > > > > > Instead of mdev imposing number of characters on
> >> >> >> >> > > > > > the alias, it should be best
> >> >> >> >> > > > > left to the user.
> >> >> >> >> > > > > > Because in future if netdev improves on the naming
> >> >> >> >> > > > > > scheme, mdev will be
> >> >> >> >> > > > > limiting it, which is not right.
> >> >> >> >> > > > > > So not restricting alias size seems right to me.
> >> >> >> >> > > > > > User configuring mdev for networking devices in a
> >> >> >> >> > > > > > given kernel knows what
> >> >> >> >> > > > > user is doing.
> >> >> >> >> > > > > > So user can choose alias name size as it finds suitable.
> >> >> >> >> > > > >
> >> >> >> >> > > > > That's not what I'm proposing, please read again.
> >> >> >> >> > > > > Thanks,
> >> >> >> >> > > >
> >> >> >> >> > > > I understood your point. But mdev doesn't know how
> >> >> >> >> > > > user is going to use
> >> >> >> >> > > udev/systemd to name the netdev.
> >> >> >> >> > > > So even if mdev chose to pick 12 characters, it could
> >> >> >> >> > > > result in
> >> >> collision.
> >> >> >> >> > > > Hence the proposal to provide the alias by the user,
> >> >> >> >> > > > as user know the best
> >> >> >> >> > > policy for its use case in the environment its using.
> >> >> >> >> > > > So 12 character sha1 method will still work by user.
> >> >> >> >> > >
> >> >> >> >> > > Haven't you already provided examples where certain
> >> >> >> >> > > drivers or subsystems have unique netdev prefixes? If
> >> >> >> >> > > mdev provides a unique alias within the subsystem,
> >> >> >> >> > > couldn't we simply define a netdev prefix for the mdev
> >> >> >> >> > > subsystem and avoid all other collisions? I'm not in
> >> >> >> >> > > favor of the user providing both a uuid and an
> >> >> >> >> > > alias/instance. Thanks,
> >> >> >> >> > >
> >> >> >> >> > For a given prefix, say ens2f0, can two UUID->sha1 first 9
> >> >> >> >> > characters have
> >> >> >> >> collision?
> >> >> >> >>
> >> >> >> >> I think it would be a mistake to waste so many chars on a
> >> >> >> >> prefix, but
> >> >> >> >> 9 characters of sha1 likely wouldn't have a collision before
> >> >> >> >> we have 10s of thousands of devices. Thanks,
> >> >> >> >>
> >> >> >> >> Alex
> >> >> >> >
> >> >> >> >Jiri, Dave,
> >> >> >> >Are you ok with it for devlink/netdev part?
> >> >> >> >Mdev core will create an alias from a UUID.
> >> >> >> >
> >> >> >> >This will be supplied during devlink port attr set such as,
> >> >> >> >
> >> >> >> >devlink_port_attrs_mdev_set(struct devlink_port *port, const
> >> >> >> >char *mdev_alias);
> >> >> >> >
> >> >> >> >This alias is used to generate representor netdev's phys_port_name.
> >> >> >> >This alias from the mdev device's sysfs will be used by the
> >> >> >> >udev/systemd to
> >> >> >> generate predicable netdev's name.
> >> >> >> >Example: enm<mdev_alias_first_12_chars>
> >> >> >>
> >> >> >> What happens in unlikely case of 2 UUIDs collide?
> >> >> >>
> >> >> >Since users sees two devices with same phys_port_name, user
> >> >> >should destroy
> >> >> recently created mdev and recreate mdev with different UUID?
> >> >>
> >> >> Driver should make sure phys port name wont collide,
> >> >So when mdev creation is initiated, mdev core calculates the alias
> >> >and if there
> >> is any other mdev with same alias exist, it returns -EEXIST error
> >> before progressing further.
> >> >This way user will get to know upfront in event of collision before
> >> >the mdev
> >> device gets created.
> >> >How about that?
> >>
> >> Sounds fine to me. Now the question is how many chars do we want to have.
> >>
> >12 characters from Alex's suggestion similar to git?
>
> Ok.
>
Can you please confirm this scheme looks good now? I like to get patches started.
> >
> >> >
> >> >
> >> >> in this case that it does
> >> >> not provide 2 same attrs for 2 different ports.
> >> >> Hmm, so the order of creation matters. That is not good.
> >> >>
> >> >> >>
> >> >> >> >I took Ethernet mdev as an example.
> >> >> >> >New prefix 'm' stands for mediated device.
> >> >> >> >Remaining 12 characters are first 12 chars of the mdev alias.
> >> >> >>
> >> >> >> Does this resolve the identification of devlink port representor?
> >> >> >Not sure if I understood your question correctly, attemping to
> >> >> >answer
> >> below.
> >> >> >phys_port_name of devlink port is defined by the first 12
> >> >> >characters of mdev
> >> >> alias.
> >> >> >> I assume you want to use the same 12(or so) chars, don't you?
> >> >> >Mdev's netdev will also use the same mdev alias from the sysfs to
> >> >> >rename
> >> >> netdev name from ethX to enm<mdev_alias>, where en=Etherenet,
> >> m=mdev.
> >> >> >
> >> >> >So yes, same 12 characters are use for mdev's netdev and mdev
> >> >> >devlink port's
> >> >> phys_port_name.
> >> >> >
> >> >> >Is that what are you asking?
> >> >>
> >> >> Yes. Then you have 3 chars to handle the rest of the name (pci, pf)...
^ permalink raw reply
* Re: [PATCH v2 0/2] Simplify mtty driver and mdev core
From: Jiri Pirko @ 2019-08-23 8:12 UTC (permalink / raw)
To: Parav Pandit
Cc: Alex Williamson, Jiri Pirko, David S . Miller, Kirti Wankhede,
Cornelia Huck, kvm@vger.kernel.org, linux-kernel@vger.kernel.org,
cjia, netdev@vger.kernel.org
In-Reply-To: <AM0PR05MB4866F9650CF73FC671972127D1A50@AM0PR05MB4866.eurprd05.prod.outlook.com>
Thu, Aug 22, 2019 at 03:33:30PM CEST, parav@mellanox.com wrote:
>
>
>> -----Original Message-----
>> From: Jiri Pirko <jiri@resnulli.us>
>> Sent: Thursday, August 22, 2019 5:50 PM
>> To: Parav Pandit <parav@mellanox.com>
>> Cc: Alex Williamson <alex.williamson@redhat.com>; Jiri Pirko
>> <jiri@mellanox.com>; David S . Miller <davem@davemloft.net>; Kirti
>> Wankhede <kwankhede@nvidia.com>; Cornelia Huck <cohuck@redhat.com>;
>> kvm@vger.kernel.org; linux-kernel@vger.kernel.org; cjia <cjia@nvidia.com>;
>> netdev@vger.kernel.org
>> Subject: Re: [PATCH v2 0/2] Simplify mtty driver and mdev core
>>
>> Thu, Aug 22, 2019 at 12:04:02PM CEST, parav@mellanox.com wrote:
>> >
>> >
>> >> -----Original Message-----
>> >> From: Jiri Pirko <jiri@resnulli.us>
>> >> Sent: Thursday, August 22, 2019 3:28 PM
>> >> To: Parav Pandit <parav@mellanox.com>
>> >> Cc: Alex Williamson <alex.williamson@redhat.com>; Jiri Pirko
>> >> <jiri@mellanox.com>; David S . Miller <davem@davemloft.net>; Kirti
>> >> Wankhede <kwankhede@nvidia.com>; Cornelia Huck
>> <cohuck@redhat.com>;
>> >> kvm@vger.kernel.org; linux-kernel@vger.kernel.org; cjia
>> >> <cjia@nvidia.com>; netdev@vger.kernel.org
>> >> Subject: Re: [PATCH v2 0/2] Simplify mtty driver and mdev core
>> >>
>> >> Thu, Aug 22, 2019 at 11:42:13AM CEST, parav@mellanox.com wrote:
>> >> >
>> >> >
>> >> >> -----Original Message-----
>> >> >> From: Jiri Pirko <jiri@resnulli.us>
>> >> >> Sent: Thursday, August 22, 2019 2:59 PM
>> >> >> To: Parav Pandit <parav@mellanox.com>
>> >> >> Cc: Alex Williamson <alex.williamson@redhat.com>; Jiri Pirko
>> >> >> <jiri@mellanox.com>; David S . Miller <davem@davemloft.net>; Kirti
>> >> >> Wankhede <kwankhede@nvidia.com>; Cornelia Huck
>> >> <cohuck@redhat.com>;
>> >> >> kvm@vger.kernel.org; linux-kernel@vger.kernel.org; cjia
>> >> >> <cjia@nvidia.com>; netdev@vger.kernel.org
>> >> >> Subject: Re: [PATCH v2 0/2] Simplify mtty driver and mdev core
>> >> >>
>> >> >> Wed, Aug 21, 2019 at 08:23:17AM CEST, parav@mellanox.com wrote:
>> >> >> >
>> >> >> >
>> >> >> >> -----Original Message-----
>> >> >> >> From: Alex Williamson <alex.williamson@redhat.com>
>> >> >> >> Sent: Wednesday, August 21, 2019 10:56 AM
>> >> >> >> To: Parav Pandit <parav@mellanox.com>
>> >> >> >> Cc: Jiri Pirko <jiri@mellanox.com>; David S . Miller
>> >> >> >> <davem@davemloft.net>; Kirti Wankhede <kwankhede@nvidia.com>;
>> >> >> >> Cornelia Huck <cohuck@redhat.com>; kvm@vger.kernel.org;
>> >> >> >> linux-kernel@vger.kernel.org; cjia <cjia@nvidia.com>;
>> >> >> >> netdev@vger.kernel.org
>> >> >> >> Subject: Re: [PATCH v2 0/2] Simplify mtty driver and mdev core
>> >> >> >>
>> >> >> >> > > > > Just an example of the alias, not proposing how it's set.
>> >> >> >> > > > > In fact, proposing that the user does not set it,
>> >> >> >> > > > > mdev-core provides one
>> >> >> >> > > automatically.
>> >> >> >> > > > >
>> >> >> >> > > > > > > Since there seems to be some prefix overhead, as I
>> >> >> >> > > > > > > ask about above in how many characters we actually
>> >> >> >> > > > > > > have to work with in IFNAMESZ, maybe we start with
>> >> >> >> > > > > > > 8 characters (matching your "index" namespace) and
>> >> >> >> > > > > > > expand as necessary for
>> >> >> disambiguation.
>> >> >> >> > > > > > > If we can eliminate overhead in IFNAMESZ, let's start with
>> 12.
>> >> >> >> > > > > > > Thanks,
>> >> >> >> > > > > > >
>> >> >> >> > > > > > If user is going to choose the alias, why does it
>> >> >> >> > > > > > have to be limited to
>> >> >> >> sha1?
>> >> >> >> > > > > > Or you just told it as an example?
>> >> >> >> > > > > >
>> >> >> >> > > > > > It can be an alpha-numeric string.
>> >> >> >> > > > >
>> >> >> >> > > > > No, I'm proposing a different solution where mdev-core
>> >> >> >> > > > > creates an alias based on an abbreviated sha1. The
>> >> >> >> > > > > user does not provide the
>> >> >> >> alias.
>> >> >> >> > > > >
>> >> >> >> > > > > > Instead of mdev imposing number of characters on the
>> >> >> >> > > > > > alias, it should be best
>> >> >> >> > > > > left to the user.
>> >> >> >> > > > > > Because in future if netdev improves on the naming
>> >> >> >> > > > > > scheme, mdev will be
>> >> >> >> > > > > limiting it, which is not right.
>> >> >> >> > > > > > So not restricting alias size seems right to me.
>> >> >> >> > > > > > User configuring mdev for networking devices in a
>> >> >> >> > > > > > given kernel knows what
>> >> >> >> > > > > user is doing.
>> >> >> >> > > > > > So user can choose alias name size as it finds suitable.
>> >> >> >> > > > >
>> >> >> >> > > > > That's not what I'm proposing, please read again.
>> >> >> >> > > > > Thanks,
>> >> >> >> > > >
>> >> >> >> > > > I understood your point. But mdev doesn't know how user
>> >> >> >> > > > is going to use
>> >> >> >> > > udev/systemd to name the netdev.
>> >> >> >> > > > So even if mdev chose to pick 12 characters, it could
>> >> >> >> > > > result in
>> >> collision.
>> >> >> >> > > > Hence the proposal to provide the alias by the user, as
>> >> >> >> > > > user know the best
>> >> >> >> > > policy for its use case in the environment its using.
>> >> >> >> > > > So 12 character sha1 method will still work by user.
>> >> >> >> > >
>> >> >> >> > > Haven't you already provided examples where certain drivers
>> >> >> >> > > or subsystems have unique netdev prefixes? If mdev
>> >> >> >> > > provides a unique alias within the subsystem, couldn't we
>> >> >> >> > > simply define a netdev prefix for the mdev subsystem and
>> >> >> >> > > avoid all other collisions? I'm not in favor of the user
>> >> >> >> > > providing both a uuid and an alias/instance. Thanks,
>> >> >> >> > >
>> >> >> >> > For a given prefix, say ens2f0, can two UUID->sha1 first 9
>> >> >> >> > characters have
>> >> >> >> collision?
>> >> >> >>
>> >> >> >> I think it would be a mistake to waste so many chars on a
>> >> >> >> prefix, but
>> >> >> >> 9 characters of sha1 likely wouldn't have a collision before we
>> >> >> >> have 10s of thousands of devices. Thanks,
>> >> >> >>
>> >> >> >> Alex
>> >> >> >
>> >> >> >Jiri, Dave,
>> >> >> >Are you ok with it for devlink/netdev part?
>> >> >> >Mdev core will create an alias from a UUID.
>> >> >> >
>> >> >> >This will be supplied during devlink port attr set such as,
>> >> >> >
>> >> >> >devlink_port_attrs_mdev_set(struct devlink_port *port, const char
>> >> >> >*mdev_alias);
>> >> >> >
>> >> >> >This alias is used to generate representor netdev's phys_port_name.
>> >> >> >This alias from the mdev device's sysfs will be used by the
>> >> >> >udev/systemd to
>> >> >> generate predicable netdev's name.
>> >> >> >Example: enm<mdev_alias_first_12_chars>
>> >> >>
>> >> >> What happens in unlikely case of 2 UUIDs collide?
>> >> >>
>> >> >Since users sees two devices with same phys_port_name, user should
>> >> >destroy
>> >> recently created mdev and recreate mdev with different UUID?
>> >>
>> >> Driver should make sure phys port name wont collide,
>> >So when mdev creation is initiated, mdev core calculates the alias and if there
>> is any other mdev with same alias exist, it returns -EEXIST error before
>> progressing further.
>> >This way user will get to know upfront in event of collision before the mdev
>> device gets created.
>> >How about that?
>>
>> Sounds fine to me. Now the question is how many chars do we want to have.
>>
>12 characters from Alex's suggestion similar to git?
Ok.
>
>> >
>> >
>> >> in this case that it does
>> >> not provide 2 same attrs for 2 different ports.
>> >> Hmm, so the order of creation matters. That is not good.
>> >>
>> >> >>
>> >> >> >I took Ethernet mdev as an example.
>> >> >> >New prefix 'm' stands for mediated device.
>> >> >> >Remaining 12 characters are first 12 chars of the mdev alias.
>> >> >>
>> >> >> Does this resolve the identification of devlink port representor?
>> >> >Not sure if I understood your question correctly, attemping to answer
>> below.
>> >> >phys_port_name of devlink port is defined by the first 12 characters
>> >> >of mdev
>> >> alias.
>> >> >> I assume you want to use the same 12(or so) chars, don't you?
>> >> >Mdev's netdev will also use the same mdev alias from the sysfs to
>> >> >rename
>> >> netdev name from ethX to enm<mdev_alias>, where en=Etherenet,
>> m=mdev.
>> >> >
>> >> >So yes, same 12 characters are use for mdev's netdev and mdev
>> >> >devlink port's
>> >> phys_port_name.
>> >> >
>> >> >Is that what are you asking?
>> >>
>> >> Yes. Then you have 3 chars to handle the rest of the name (pci, pf)...
^ permalink raw reply
* [PATCH] brcmfmac: replace strncpy() by strscpy()
From: Xulin Sun @ 2019-08-23 7:47 UTC (permalink / raw)
To: kvalo
Cc: stefan.wahren, xulin.sun, linux-kernel, netdev,
brcm80211-dev-list, brcm80211-dev-list.pdl, linux-wireless,
arend.vanspriel, franky.lin, hante.meuleman, chi-hsien.lin,
wright.feng, davem, stanley.hsu
The strncpy() may truncate the copied string,
replace it by the safer strscpy().
To avoid below compile warning with gcc 8.2:
drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c:In function 'brcmf_vndr_ie':
drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c:4227:2:
warning: 'strncpy' output truncated before terminating nul copying 3 bytes from a string of the same length [-Wstringop-truncation]
strncpy(iebuf, add_del_cmd, VNDR_IE_CMD_LEN - 1);
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Signed-off-by: Xulin Sun <xulin.sun@windriver.com>
---
drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
index b6d0df354b36..7ad60374fa96 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
@@ -4226,9 +4226,7 @@ brcmf_parse_vndr_ies(const u8 *vndr_ie_buf, u32 vndr_ie_len,
static u32
brcmf_vndr_ie(u8 *iebuf, s32 pktflag, u8 *ie_ptr, u32 ie_len, s8 *add_del_cmd)
{
-
- strncpy(iebuf, add_del_cmd, VNDR_IE_CMD_LEN - 1);
- iebuf[VNDR_IE_CMD_LEN - 1] = '\0';
+ strscpy(iebuf, add_del_cmd, VNDR_IE_CMD_LEN);
put_unaligned_le32(1, &iebuf[VNDR_IE_COUNT_OFFSET]);
--
2.17.1
^ permalink raw reply related
* [PATCH net-next v4 0/2] r8152: save EEE
From: Hayes Wang @ 2019-08-23 7:33 UTC (permalink / raw)
To: netdev; +Cc: nic_swsd, linux-kernel, Hayes Wang
In-Reply-To: <1394712342-15778-304-Taiwan-albertk@realtek.com>
v4:
For patch #2, remove redundant calling of "ocp_reg_write(tp, OCP_EEE_ADV, 0)".
v3:
For patch #2, fix the mistake caused by copying and pasting.
v2:
Adjust patch #1. The EEE has been disabled in the beginning of
r8153_hw_phy_cfg() and r8153b_hw_phy_cfg(), so only check if
it is necessary to enable EEE.
Add the patch #2 for the helper function.
v1:
Saving the settings of EEE to avoid they become the default settings
after reset_resume().
Hayes Wang (2):
r8152: saving the settings of EEE
r8152: add a helper function about setting EEE
drivers/net/usb/r8152.c | 182 +++++++++++++++++++++-------------------
1 file changed, 95 insertions(+), 87 deletions(-)
--
2.21.0
^ 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