From: Lorenzo Bianconi <[email protected]>
Use linear fragment and not a single usb scatter-gather buffer in mt76u
{tx,rx} datapath if the usb controller has sg data length constraints.
Moreover add disable_usb_sg module parameter in order to explicitly
disable scatter-gather. SG I/O is not supported by all host drivers and
some users have reported sg issues on AMD IOMMU.
This series has been tested on AMD IOMMU cpus/motherboards and on rpi3+
Changes since RFC:
- rebased on top of 'fix multiple issues in mt76u error path'
https://patchwork.kernel.org/cover/10804919/
I am resending the series since the first attempt seems to be rejected by
the ML
Lorenzo Bianconi (4):
mt76: usb: move mt76u_check_sg in usb.c
mt76: usb: do not use sg buffers for mcu messages
mt76: usb: use a linear buffer for tx/rx datapath if sg is not
supported
mt76: usb: introduce disable_usb_sg parameter
drivers/net/wireless/mediatek/mt76/mt76.h | 14 +-
.../net/wireless/mediatek/mt76/mt76x0/usb.c | 2 +-
.../wireless/mediatek/mt76/mt76x02_usb_mcu.c | 3 +-
.../wireless/mediatek/mt76/mt76x2/usb_init.c | 2 +-
drivers/net/wireless/mediatek/mt76/usb.c | 133 +++++++++++++-----
drivers/net/wireless/mediatek/mt76/usb_mcu.c | 5 +-
6 files changed, 105 insertions(+), 54 deletions(-)
--
2.20.1
From: Lorenzo Bianconi <[email protected]>
Move mt76u_check_sg routine in usb.c and introduce sg_en variable
in mt76_usb in order to check if scatter-gather is supported by
mt76u layer
Signed-off-by: Lorenzo Bianconi <[email protected]>
---
drivers/net/wireless/mediatek/mt76/mt76.h | 11 +----------
drivers/net/wireless/mediatek/mt76/mt76x0/usb.c | 2 +-
.../net/wireless/mediatek/mt76/mt76x2/usb_init.c | 2 +-
drivers/net/wireless/mediatek/mt76/usb.c | 14 +++++++++++++-
4 files changed, 16 insertions(+), 13 deletions(-)
diff --git a/drivers/net/wireless/mediatek/mt76/mt76.h b/drivers/net/wireless/mediatek/mt76/mt76.h
index 13f6febc9b0f..0eb9152c5d18 100644
--- a/drivers/net/wireless/mediatek/mt76/mt76.h
+++ b/drivers/net/wireless/mediatek/mt76/mt76.h
@@ -379,6 +379,7 @@ struct mt76_usb {
u16 out_max_packet;
u8 in_ep[__MT_EP_IN_MAX];
u16 in_max_packet;
+ bool sg_en;
struct mt76u_mcu {
struct mutex mutex;
@@ -726,16 +727,6 @@ static inline u8 q2ep(u8 qid)
return qid + 1;
}
-static inline bool mt76u_check_sg(struct mt76_dev *dev)
-{
- struct usb_interface *intf = to_usb_interface(dev->dev);
- struct usb_device *udev = interface_to_usbdev(intf);
-
- return (udev->bus->sg_tablesize > 0 &&
- (udev->bus->no_sg_constraint ||
- udev->speed == USB_SPEED_WIRELESS));
-}
-
static inline int
mt76u_bulk_msg(struct mt76_dev *dev, void *data, int len, int timeout)
{
diff --git a/drivers/net/wireless/mediatek/mt76/mt76x0/usb.c b/drivers/net/wireless/mediatek/mt76/mt76x0/usb.c
index 2f259bc0ad9e..da9d05f6074d 100644
--- a/drivers/net/wireless/mediatek/mt76/mt76x0/usb.c
+++ b/drivers/net/wireless/mediatek/mt76/mt76x0/usb.c
@@ -206,7 +206,7 @@ static int mt76x0u_register_device(struct mt76x02_dev *dev)
goto out_err;
/* check hw sg support in order to enable AMSDU */
- if (mt76u_check_sg(&dev->mt76))
+ if (dev->mt76.usb.sg_en)
hw->max_tx_fragments = MT_SG_MAX_SIZE;
else
hw->max_tx_fragments = 1;
diff --git a/drivers/net/wireless/mediatek/mt76/mt76x2/usb_init.c b/drivers/net/wireless/mediatek/mt76/mt76x2/usb_init.c
index 006e430e374e..090aaf71b3ef 100644
--- a/drivers/net/wireless/mediatek/mt76/mt76x2/usb_init.c
+++ b/drivers/net/wireless/mediatek/mt76/mt76x2/usb_init.c
@@ -228,7 +228,7 @@ int mt76x2u_register_device(struct mt76x02_dev *dev)
goto fail;
/* check hw sg support in order to enable AMSDU */
- if (mt76u_check_sg(&dev->mt76))
+ if (dev->mt76.usb.sg_en)
hw->max_tx_fragments = MT_SG_MAX_SIZE;
else
hw->max_tx_fragments = 1;
diff --git a/drivers/net/wireless/mediatek/mt76/usb.c b/drivers/net/wireless/mediatek/mt76/usb.c
index c7db8a9f6acc..829128a07701 100644
--- a/drivers/net/wireless/mediatek/mt76/usb.c
+++ b/drivers/net/wireless/mediatek/mt76/usb.c
@@ -241,6 +241,16 @@ mt76u_rd_rp(struct mt76_dev *dev, u32 base,
return mt76u_req_rd_rp(dev, base, data, n);
}
+static bool mt76u_check_sg(struct mt76_dev *dev)
+{
+ struct usb_interface *intf = to_usb_interface(dev->dev);
+ struct usb_device *udev = interface_to_usbdev(intf);
+
+ return (udev->bus->sg_tablesize > 0 &&
+ (udev->bus->no_sg_constraint ||
+ udev->speed == USB_SPEED_WIRELESS));
+}
+
static int
mt76u_set_endpoints(struct usb_interface *intf,
struct mt76_usb *usb)
@@ -536,7 +546,7 @@ static int mt76u_alloc_rx(struct mt76_dev *dev)
if (!q->entry)
return -ENOMEM;
- if (mt76u_check_sg(dev)) {
+ if (dev->usb.sg_en) {
q->buf_size = MT_RX_BUF_SIZE;
nsgs = MT_SG_MAX_SIZE;
} else {
@@ -881,6 +891,8 @@ int mt76u_init(struct mt76_dev *dev,
dev->bus = &mt76u_ops;
dev->queue_ops = &usb_queue_ops;
+ usb->sg_en = mt76u_check_sg(dev);
+
return mt76u_set_endpoints(intf, usb);
}
EXPORT_SYMBOL_GPL(mt76u_init);
--
2.20.1
From: Lorenzo Bianconi <[email protected]>
Do not use scatter-gather buffers for mcu commands.
Introduce mt76u_buf_alloc and mt76u_buf_alloc_sg routines.
Signed-off-by: Lorenzo Bianconi <[email protected]>
---
drivers/net/wireless/mediatek/mt76/mt76.h | 3 +-
.../wireless/mediatek/mt76/mt76x02_usb_mcu.c | 3 +-
drivers/net/wireless/mediatek/mt76/usb.c | 38 +++++++++++++++----
drivers/net/wireless/mediatek/mt76/usb_mcu.c | 5 +--
4 files changed, 36 insertions(+), 13 deletions(-)
diff --git a/drivers/net/wireless/mediatek/mt76/mt76.h b/drivers/net/wireless/mediatek/mt76/mt76.h
index 0eb9152c5d18..f55dc621e060 100644
--- a/drivers/net/wireless/mediatek/mt76/mt76.h
+++ b/drivers/net/wireless/mediatek/mt76/mt76.h
@@ -87,6 +87,7 @@ struct mt76u_buf {
struct mt76_dev *dev;
struct urb *urb;
size_t len;
+ void *buf;
bool done;
};
@@ -748,7 +749,7 @@ void mt76u_single_wr(struct mt76_dev *dev, const u8 req,
int mt76u_init(struct mt76_dev *dev, struct usb_interface *intf);
void mt76u_deinit(struct mt76_dev *dev);
int mt76u_buf_alloc(struct mt76_dev *dev, struct mt76u_buf *buf,
- int nsgs, int len, int sglen, gfp_t gfp);
+ int len, int data_len, gfp_t gfp);
void mt76u_buf_free(struct mt76u_buf *buf);
int mt76u_submit_buf(struct mt76_dev *dev, int dir, int index,
struct mt76u_buf *buf, gfp_t gfp,
diff --git a/drivers/net/wireless/mediatek/mt76/mt76x02_usb_mcu.c b/drivers/net/wireless/mediatek/mt76/mt76x02_usb_mcu.c
index 64c777298cee..e469e383cb88 100644
--- a/drivers/net/wireless/mediatek/mt76/mt76x02_usb_mcu.c
+++ b/drivers/net/wireless/mediatek/mt76/mt76x02_usb_mcu.c
@@ -63,9 +63,9 @@ static int mt76x02u_mcu_wait_resp(struct mt76_dev *dev, u8 seq)
struct mt76_usb *usb = &dev->usb;
struct mt76u_buf *buf = &usb->mcu.res;
struct urb *urb = buf->urb;
+ u8 *data = buf->buf;
int i, ret;
u32 rxfce;
- u8 *data;
for (i = 0; i < 5; i++) {
if (!wait_for_completion_timeout(&usb->mcu.cmpl,
@@ -75,7 +75,6 @@ static int mt76x02u_mcu_wait_resp(struct mt76_dev *dev, u8 seq)
if (urb->status)
return -EIO;
- data = sg_virt(&urb->sg[0]);
if (usb->mcu.rp)
mt76x02u_multiple_mcu_reads(dev, data + 4,
urb->actual_length - 8);
diff --git a/drivers/net/wireless/mediatek/mt76/usb.c b/drivers/net/wireless/mediatek/mt76/usb.c
index 829128a07701..66c9451cb6f3 100644
--- a/drivers/net/wireless/mediatek/mt76/usb.c
+++ b/drivers/net/wireless/mediatek/mt76/usb.c
@@ -319,8 +319,9 @@ mt76u_fill_rx_sg(struct mt76_dev *dev, struct mt76u_buf *buf,
return i ? : -ENOMEM;
}
-int mt76u_buf_alloc(struct mt76_dev *dev, struct mt76u_buf *buf,
- int nsgs, int len, int sglen, gfp_t gfp)
+static int
+mt76u_buf_alloc_sg(struct mt76_dev *dev, struct mt76u_buf *buf,
+ int nsgs, int len, int sglen, gfp_t gfp)
{
buf->urb = usb_alloc_urb(0, gfp);
if (!buf->urb)
@@ -337,6 +338,25 @@ int mt76u_buf_alloc(struct mt76_dev *dev, struct mt76u_buf *buf,
return mt76u_fill_rx_sg(dev, buf, nsgs, len, sglen);
}
+int mt76u_buf_alloc(struct mt76_dev *dev, struct mt76u_buf *buf,
+ int len, int data_len, gfp_t gfp)
+{
+ struct mt76_queue *q = &dev->q_rx[MT_RXQ_MAIN];
+
+ buf->urb = usb_alloc_urb(0, gfp);
+ if (!buf->urb)
+ return -ENOMEM;
+
+ buf->buf = page_frag_alloc(&q->rx_page, len, gfp);
+ if (!buf->buf)
+ return -ENOMEM;
+
+ buf->len = data_len;
+ buf->dev = dev;
+
+ return 0;
+}
+
void mt76u_buf_free(struct mt76u_buf *buf)
{
struct urb *urb = buf->urb;
@@ -350,6 +370,9 @@ void mt76u_buf_free(struct mt76u_buf *buf)
skb_free_frag(sg_virt(sg));
}
+ if (buf->buf)
+ skb_free_frag(buf->buf);
+
usb_free_urb(buf->urb);
}
EXPORT_SYMBOL_GPL(mt76u_buf_free);
@@ -360,6 +383,7 @@ int mt76u_submit_buf(struct mt76_dev *dev, int dir, int index,
{
struct usb_interface *intf = to_usb_interface(dev->dev);
struct usb_device *udev = interface_to_usbdev(intf);
+ u8 *data = buf->urb->num_sgs ? NULL : buf->buf;
unsigned int pipe;
if (dir == USB_DIR_IN)
@@ -367,7 +391,7 @@ int mt76u_submit_buf(struct mt76_dev *dev, int dir, int index,
else
pipe = usb_sndbulkpipe(udev, dev->usb.out_ep[index]);
- usb_fill_bulk_urb(buf->urb, udev, pipe, NULL, buf->len,
+ usb_fill_bulk_urb(buf->urb, udev, pipe, data, buf->len,
complete_fn, context);
trace_submit_urb(dev, buf->urb);
@@ -556,10 +580,10 @@ static int mt76u_alloc_rx(struct mt76_dev *dev)
q->ndesc = MT_NUM_RX_ENTRIES;
for (i = 0; i < q->ndesc; i++) {
- err = mt76u_buf_alloc(dev, &q->entry[i].ubuf,
- nsgs, q->buf_size,
- SKB_WITH_OVERHEAD(q->buf_size),
- GFP_KERNEL);
+ err = mt76u_buf_alloc_sg(dev, &q->entry[i].ubuf,
+ nsgs, q->buf_size,
+ SKB_WITH_OVERHEAD(q->buf_size),
+ GFP_KERNEL);
if (err < 0)
return err;
}
diff --git a/drivers/net/wireless/mediatek/mt76/usb_mcu.c b/drivers/net/wireless/mediatek/mt76/usb_mcu.c
index 9527e1216f3d..72c8607da4b4 100644
--- a/drivers/net/wireless/mediatek/mt76/usb_mcu.c
+++ b/drivers/net/wireless/mediatek/mt76/usb_mcu.c
@@ -29,9 +29,8 @@ int mt76u_mcu_init_rx(struct mt76_dev *dev)
struct mt76_usb *usb = &dev->usb;
int err;
- err = mt76u_buf_alloc(dev, &usb->mcu.res, 1,
- MCU_RESP_URB_SIZE, MCU_RESP_URB_SIZE,
- GFP_KERNEL);
+ err = mt76u_buf_alloc(dev, &usb->mcu.res, MCU_RESP_URB_SIZE,
+ MCU_RESP_URB_SIZE, GFP_KERNEL);
if (err < 0)
return err;
--
2.20.1
From: Lorenzo Bianconi <[email protected]>
Use linear fragment and not a single usb scatter-gather buffer in mt76u
{tx,rx} datapath if the usb controller has sg data length constraints
Signed-off-by: Lorenzo Bianconi <[email protected]>
---
drivers/net/wireless/mediatek/mt76/usb.c | 87 +++++++++++++++---------
1 file changed, 54 insertions(+), 33 deletions(-)
diff --git a/drivers/net/wireless/mediatek/mt76/usb.c b/drivers/net/wireless/mediatek/mt76/usb.c
index 66c9451cb6f3..7e7da6b49a88 100644
--- a/drivers/net/wireless/mediatek/mt76/usb.c
+++ b/drivers/net/wireless/mediatek/mt76/usb.c
@@ -432,10 +432,11 @@ static int mt76u_get_rx_entry_len(u8 *data, u32 data_len)
}
static int
-mt76u_process_rx_entry(struct mt76_dev *dev, struct urb *urb)
+mt76u_process_rx_entry(struct mt76_dev *dev, struct mt76u_buf *buf)
{
struct mt76_queue *q = &dev->q_rx[MT_RXQ_MAIN];
- u8 *data = sg_virt(&urb->sg[0]);
+ struct urb *urb = buf->urb;
+ u8 *data = urb->num_sgs ? sg_virt(&urb->sg[0]) : buf->buf;
int data_len, len, nsgs = 1;
struct sk_buff *skb;
@@ -446,7 +447,8 @@ mt76u_process_rx_entry(struct mt76_dev *dev, struct urb *urb)
if (len < 0)
return 0;
- data_len = min_t(int, len, urb->sg[0].length - MT_DMA_HDR_LEN);
+ data_len = urb->num_sgs ? urb->sg[0].length : buf->len;
+ data_len = min_t(int, len, data_len - MT_DMA_HDR_LEN);
if (MT_DMA_HDR_LEN + data_len > SKB_WITH_OVERHEAD(q->buf_size))
return 0;
@@ -458,7 +460,7 @@ mt76u_process_rx_entry(struct mt76_dev *dev, struct urb *urb)
__skb_put(skb, data_len);
len -= data_len;
- while (len > 0) {
+ while (len > 0 && urb->num_sgs) {
data_len = min_t(int, len, urb->sg[nsgs].length);
skb_add_rx_frag(skb, skb_shinfo(skb)->nr_frags,
sg_page(&urb->sg[nsgs]),
@@ -504,12 +506,26 @@ static void mt76u_complete_rx(struct urb *urb)
spin_unlock_irqrestore(&q->lock, flags);
}
+static int
+mt76u_refill_rx(struct mt76_dev *dev, struct mt76_queue *q,
+ struct mt76u_buf *buf, int nsgs)
+{
+ if (dev->usb.sg_en) {
+ return mt76u_fill_rx_sg(dev, buf, nsgs, q->buf_size,
+ SKB_WITH_OVERHEAD(q->buf_size));
+ } else {
+ buf->buf = page_frag_alloc(&q->rx_page, q->buf_size,
+ GFP_ATOMIC);
+ return buf->buf ? 0 : -ENOMEM;
+ }
+}
+
static void mt76u_rx_tasklet(unsigned long data)
{
struct mt76_dev *dev = (struct mt76_dev *)data;
struct mt76_queue *q = &dev->q_rx[MT_RXQ_MAIN];
- int err, nsgs, buf_len = q->buf_size;
struct mt76u_buf *buf;
+ int err, count;
rcu_read_lock();
@@ -518,11 +534,9 @@ static void mt76u_rx_tasklet(unsigned long data)
if (!buf)
break;
- nsgs = mt76u_process_rx_entry(dev, buf->urb);
- if (nsgs > 0) {
- err = mt76u_fill_rx_sg(dev, buf, nsgs,
- buf_len,
- SKB_WITH_OVERHEAD(buf_len));
+ count = mt76u_process_rx_entry(dev, buf);
+ if (count > 0) {
+ err = mt76u_refill_rx(dev, q, buf, count);
if (err < 0)
break;
}
@@ -560,7 +574,7 @@ EXPORT_SYMBOL_GPL(mt76u_submit_rx_buffers);
static int mt76u_alloc_rx(struct mt76_dev *dev)
{
struct mt76_queue *q = &dev->q_rx[MT_RXQ_MAIN];
- int i, err, nsgs;
+ int i, err;
spin_lock_init(&q->rx_page_lock);
spin_lock_init(&q->lock);
@@ -570,20 +584,19 @@ static int mt76u_alloc_rx(struct mt76_dev *dev)
if (!q->entry)
return -ENOMEM;
- if (dev->usb.sg_en) {
- q->buf_size = MT_RX_BUF_SIZE;
- nsgs = MT_SG_MAX_SIZE;
- } else {
- q->buf_size = PAGE_SIZE;
- nsgs = 1;
- }
-
+ q->buf_size = dev->usb.sg_en ? MT_RX_BUF_SIZE : PAGE_SIZE;
q->ndesc = MT_NUM_RX_ENTRIES;
for (i = 0; i < q->ndesc; i++) {
- err = mt76u_buf_alloc_sg(dev, &q->entry[i].ubuf,
- nsgs, q->buf_size,
- SKB_WITH_OVERHEAD(q->buf_size),
- GFP_KERNEL);
+ if (dev->usb.sg_en)
+ err = mt76u_buf_alloc_sg(dev, &q->entry[i].ubuf,
+ MT_SG_MAX_SIZE, q->buf_size,
+ SKB_WITH_OVERHEAD(q->buf_size),
+ GFP_KERNEL);
+ else
+ err = mt76u_buf_alloc(dev, &q->entry[i].ubuf,
+ q->buf_size,
+ SKB_WITH_OVERHEAD(q->buf_size),
+ GFP_KERNEL);
if (err < 0)
return err;
}
@@ -731,7 +744,7 @@ mt76u_tx_queue_skb(struct mt76_dev *dev, struct mt76_queue *q,
{
struct usb_interface *intf = to_usb_interface(dev->dev);
struct usb_device *udev = interface_to_usbdev(intf);
- u8 ep = q2ep(q->hw_idx);
+ u8 *data = NULL, ep = q2ep(q->hw_idx);
struct mt76u_buf *buf;
u16 idx = q->tail;
unsigned int pipe;
@@ -748,12 +761,16 @@ mt76u_tx_queue_skb(struct mt76_dev *dev, struct mt76_queue *q,
buf = &q->entry[idx].ubuf;
buf->done = false;
- err = mt76u_tx_build_sg(skb, buf->urb);
- if (err < 0)
- return err;
+ if (dev->usb.sg_en) {
+ err = mt76u_tx_build_sg(skb, buf->urb);
+ if (err < 0)
+ return err;
+ } else {
+ data = skb->data;
+ }
pipe = usb_sndbulkpipe(udev, dev->usb.out_ep[ep]);
- usb_fill_bulk_urb(buf->urb, udev, pipe, NULL, skb->len,
+ usb_fill_bulk_urb(buf->urb, udev, pipe, data, skb->len,
mt76u_complete_tx, buf);
q->tail = (q->tail + 1) % q->ndesc;
@@ -789,10 +806,8 @@ static int mt76u_alloc_tx(struct mt76_dev *dev)
{
struct mt76u_buf *buf;
struct mt76_queue *q;
- size_t size;
int i, j;
- size = MT_SG_MAX_SIZE * sizeof(struct scatterlist);
for (i = 0; i < IEEE80211_NUM_ACS; i++) {
q = &dev->q_tx[i];
spin_lock_init(&q->lock);
@@ -814,9 +829,15 @@ static int mt76u_alloc_tx(struct mt76_dev *dev)
if (!buf->urb)
return -ENOMEM;
- buf->urb->sg = devm_kzalloc(dev->dev, size, GFP_KERNEL);
- if (!buf->urb->sg)
- return -ENOMEM;
+ if (dev->usb.sg_en) {
+ size_t size = MT_SG_MAX_SIZE *
+ sizeof(struct scatterlist);
+
+ buf->urb->sg = devm_kzalloc(dev->dev, size,
+ GFP_KERNEL);
+ if (!buf->urb->sg)
+ return -ENOMEM;
+ }
}
}
return 0;
--
2.20.1
From: Lorenzo Bianconi <[email protected]>
Add disable_usb_sg module parameter to disable scatter-gather on demand
Signed-off-by: Lorenzo Bianconi <[email protected]>
---
drivers/net/wireless/mediatek/mt76/usb.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/drivers/net/wireless/mediatek/mt76/usb.c b/drivers/net/wireless/mediatek/mt76/usb.c
index 7e7da6b49a88..78191968b4fa 100644
--- a/drivers/net/wireless/mediatek/mt76/usb.c
+++ b/drivers/net/wireless/mediatek/mt76/usb.c
@@ -22,6 +22,10 @@
#define MT_VEND_REQ_MAX_RETRY 10
#define MT_VEND_REQ_TOUT_MS 300
+static bool disable_usb_sg;
+module_param_named(disable_usb_sg, disable_usb_sg, bool, 0644);
+MODULE_PARM_DESC(disable_usb_sg, "Disable usb scatter-gather support");
+
/* should be called with usb_ctrl_mtx locked */
static int __mt76u_vendor_request(struct mt76_dev *dev, u8 req,
u8 req_type, u16 val, u16 offset,
@@ -246,7 +250,7 @@ static bool mt76u_check_sg(struct mt76_dev *dev)
struct usb_interface *intf = to_usb_interface(dev->dev);
struct usb_device *udev = interface_to_usbdev(intf);
- return (udev->bus->sg_tablesize > 0 &&
+ return (!disable_usb_sg && udev->bus->sg_tablesize > 0 &&
(udev->bus->no_sg_constraint ||
udev->speed == USB_SPEED_WIRELESS));
}
--
2.20.1
On Tue, Feb 12, 2019 at 02:24:47PM +0100, [email protected] wrote:
> From: Lorenzo Bianconi <[email protected]>
>
> Use linear fragment and not a single usb scatter-gather buffer in mt76u
> {tx,rx} datapath if the usb controller has sg data length constraints.
> Moreover add disable_usb_sg module parameter in order to explicitly
> disable scatter-gather. SG I/O is not supported by all host drivers and
> some users have reported sg issues on AMD IOMMU.
Again. This is not right approach. SG issues should be fixed
not workarounded.
> This series has been tested on AMD IOMMU cpus/motherboards and on rpi3+
> - rebased on top of 'fix multiple issues in mt76u error path'
> https://patchwork.kernel.org/cover/10804919/
>
> Lorenzo Bianconi (4):
> mt76: usb: move mt76u_check_sg in usb.c
> mt76: usb: do not use sg buffers for mcu messages
> mt76: usb: use a linear buffer for tx/rx datapath if sg is not
> supported
> mt76: usb: introduce disable_usb_sg parameter
>
> drivers/net/wireless/mediatek/mt76/mt76.h | 14 +-
> .../net/wireless/mediatek/mt76/mt76x0/usb.c | 2 +-
> .../wireless/mediatek/mt76/mt76x02_usb_mcu.c | 3 +-
> .../wireless/mediatek/mt76/mt76x2/usb_init.c | 2 +-
> drivers/net/wireless/mediatek/mt76/usb.c | 133 +++++++++++++-----
> drivers/net/wireless/mediatek/mt76/usb_mcu.c | 5 +-
> 6 files changed, 105 insertions(+), 54 deletions(-)
I really think my approach is simpler. Diffstat from my proposed patch is:
drivers/net/wireless/mediatek/mt76/mt76.h | 1 +
drivers/net/wireless/mediatek/mt76/usb.c | 55 ++++++++++++++---------
2 files changed, 36 insertions(+), 20 deletions(-)
Also this fix bug(s), presumably regression for mt76x0, should be
backported.
Stanislaw
(repost with corrected Lorenzo email)
On Tue, Feb 12, 2019 at 02:45:50PM +0100, Stanislaw Gruszka wrote:
> On Tue, Feb 12, 2019 at 02:24:47PM +0100, [email protected] wrote:
> > From: Lorenzo Bianconi <[email protected]>
> >
> > Use linear fragment and not a single usb scatter-gather buffer in mt76u
> > {tx,rx} datapath if the usb controller has sg data length constraints.
> > Moreover add disable_usb_sg module parameter in order to explicitly
> > disable scatter-gather. SG I/O is not supported by all host drivers and
> > some users have reported sg issues on AMD IOMMU.
>
> Again. This is not right approach. SG issues should be fixed
> not workarounded.
>
> > This series has been tested on AMD IOMMU cpus/motherboards and on rpi3+
>
> > - rebased on top of 'fix multiple issues in mt76u error path'
> > https://patchwork.kernel.org/cover/10804919/
> >
> > Lorenzo Bianconi (4):
> > mt76: usb: move mt76u_check_sg in usb.c
> > mt76: usb: do not use sg buffers for mcu messages
> > mt76: usb: use a linear buffer for tx/rx datapath if sg is not
> > supported
> > mt76: usb: introduce disable_usb_sg parameter
> >
> > drivers/net/wireless/mediatek/mt76/mt76.h | 14 +-
> > .../net/wireless/mediatek/mt76/mt76x0/usb.c | 2 +-
> > .../wireless/mediatek/mt76/mt76x02_usb_mcu.c | 3 +-
> > .../wireless/mediatek/mt76/mt76x2/usb_init.c | 2 +-
> > drivers/net/wireless/mediatek/mt76/usb.c | 133 +++++++++++++-----
> > drivers/net/wireless/mediatek/mt76/usb_mcu.c | 5 +-
> > 6 files changed, 105 insertions(+), 54 deletions(-)
>
> I really think my approach is simpler. Diffstat from my proposed patch is:
>
> drivers/net/wireless/mediatek/mt76/mt76.h | 1 +
> drivers/net/wireless/mediatek/mt76/usb.c | 55 ++++++++++++++---------
> 2 files changed, 36 insertions(+), 20 deletions(-)
>
> Also this fix bug(s), presumably regression for mt76x0, should be
> backported.
>
> Stanislaw
>
>
> (repost with corrected Lorenzo email)
>
> On Tue, Feb 12, 2019 at 02:45:50PM +0100, Stanislaw Gruszka wrote:
> > On Tue, Feb 12, 2019 at 02:24:47PM +0100, [email protected] wrote:
> > > From: Lorenzo Bianconi <[email protected]>
> > >
> > > Use linear fragment and not a single usb scatter-gather buffer in mt76u
> > > {tx,rx} datapath if the usb controller has sg data length constraints.
> > > Moreover add disable_usb_sg module parameter in order to explicitly
> > > disable scatter-gather. SG I/O is not supported by all host drivers and
> > > some users have reported sg issues on AMD IOMMU.
> >
> > Again. This is not right approach. SG issues should be fixed
> > not workarounded.
Hi Stanislaw,
here we do not use SG, so num_sg is 0 and we use transfer_buffer. I do
not see how I am working around the issue.
Moreover with this approach we avoid some unnecessary operation in the hotpath
Regards,
Lorenzo
> >
> > > This series has been tested on AMD IOMMU cpus/motherboards and on rpi3+
> >
> > > - rebased on top of 'fix multiple issues in mt76u error path'
> > > https://patchwork.kernel.org/cover/10804919/
> > >
> > > Lorenzo Bianconi (4):
> > > mt76: usb: move mt76u_check_sg in usb.c
> > > mt76: usb: do not use sg buffers for mcu messages
> > > mt76: usb: use a linear buffer for tx/rx datapath if sg is not
> > > supported
> > > mt76: usb: introduce disable_usb_sg parameter
> > >
> > > drivers/net/wireless/mediatek/mt76/mt76.h | 14 +-
> > > .../net/wireless/mediatek/mt76/mt76x0/usb.c | 2 +-
> > > .../wireless/mediatek/mt76/mt76x02_usb_mcu.c | 3 +-
> > > .../wireless/mediatek/mt76/mt76x2/usb_init.c | 2 +-
> > > drivers/net/wireless/mediatek/mt76/usb.c | 133 +++++++++++++-----
> > > drivers/net/wireless/mediatek/mt76/usb_mcu.c | 5 +-
> > > 6 files changed, 105 insertions(+), 54 deletions(-)
> >
> > I really think my approach is simpler. Diffstat from my proposed patch is:
> >
> > drivers/net/wireless/mediatek/mt76/mt76.h | 1 +
> > drivers/net/wireless/mediatek/mt76/usb.c | 55 ++++++++++++++---------
> > 2 files changed, 36 insertions(+), 20 deletions(-)
> >
> > Also this fix bug(s), presumably regression for mt76x0, should be
> > backported.
> >
> > Stanislaw
> >
On Tue, Feb 12, 2019 at 03:09:53PM +0100, Lorenzo Bianconi wrote:
> >
> > (repost with corrected Lorenzo email)
> >
> > On Tue, Feb 12, 2019 at 02:45:50PM +0100, Stanislaw Gruszka wrote:
> > > On Tue, Feb 12, 2019 at 02:24:47PM +0100, [email protected] wrote:
> > > > From: Lorenzo Bianconi <[email protected]>
> > > >
> > > > Use linear fragment and not a single usb scatter-gather buffer in mt76u
> > > > {tx,rx} datapath if the usb controller has sg data length constraints.
> > > > Moreover add disable_usb_sg module parameter in order to explicitly
> > > > disable scatter-gather. SG I/O is not supported by all host drivers and
> > > > some users have reported sg issues on AMD IOMMU.
> > >
> > > Again. This is not right approach. SG issues should be fixed
> > > not workarounded.
>
> Hi Stanislaw,
>
> here we do not use SG, so num_sg is 0 and we use transfer_buffer. I do
> not see how I am working around the issue.
By avoiding SG buffer allocation and configuration which most likely
need to be fixed.
> Moreover with this approach we avoid some unnecessary operation in the hotpath
What unnecessary operation ?
Stanislaw
> On Tue, Feb 12, 2019 at 03:09:53PM +0100, Lorenzo Bianconi wrote:
> > >
> > > (repost with corrected Lorenzo email)
> > >
> > > On Tue, Feb 12, 2019 at 02:45:50PM +0100, Stanislaw Gruszka wrote:
> > > > On Tue, Feb 12, 2019 at 02:24:47PM +0100, [email protected] wrote:
> > > > > From: Lorenzo Bianconi <[email protected]>
> > > > >
> > > > > Use linear fragment and not a single usb scatter-gather buffer in mt76u
> > > > > {tx,rx} datapath if the usb controller has sg data length constraints.
> > > > > Moreover add disable_usb_sg module parameter in order to explicitly
> > > > > disable scatter-gather. SG I/O is not supported by all host drivers and
> > > > > some users have reported sg issues on AMD IOMMU.
> > > >
> > > > Again. This is not right approach. SG issues should be fixed
> > > > not workarounded.
> >
> > Hi Stanislaw,
> >
> > here we do not use SG, so num_sg is 0 and we use transfer_buffer. I do
> > not see how I am working around the issue.
>
> By avoiding SG buffer allocation and configuration which most likely
> need to be fixed.
In my series I:
1- set num_sg to 0
2- use transfer_buffer
please correct me if I am wrong but in your solution you did the same since AFAIK
PageHighMem is always 0 so you end up setting num_sg to 0 and using
transfer_buffer as well. Is my understanding correct?
>
> > Moreover with this approach we avoid some unnecessary operation in the hotpath
>
> What unnecessary operation ?
the ones in mt76u_fill_bulk_urb()
Regards,
Lorenzo
>
> Stanislaw
On Tue, Feb 12, 2019 at 03:25:30PM +0100, Lorenzo Bianconi wrote:
> > On Tue, Feb 12, 2019 at 03:09:53PM +0100, Lorenzo Bianconi wrote:
> > > >
> > > > (repost with corrected Lorenzo email)
> > > >
> > > > On Tue, Feb 12, 2019 at 02:45:50PM +0100, Stanislaw Gruszka wrote:
> > > > > On Tue, Feb 12, 2019 at 02:24:47PM +0100, [email protected] wrote:
> > > > > > From: Lorenzo Bianconi <[email protected]>
> > > > > >
> > > > > > Use linear fragment and not a single usb scatter-gather buffer in mt76u
> > > > > > {tx,rx} datapath if the usb controller has sg data length constraints.
> > > > > > Moreover add disable_usb_sg module parameter in order to explicitly
> > > > > > disable scatter-gather. SG I/O is not supported by all host drivers and
> > > > > > some users have reported sg issues on AMD IOMMU.
> > > > >
> > > > > Again. This is not right approach. SG issues should be fixed
> > > > > not workarounded.
> > >
> > > Hi Stanislaw,
> > >
> > > here we do not use SG, so num_sg is 0 and we use transfer_buffer. I do
> > > not see how I am working around the issue.
> >
> > By avoiding SG buffer allocation and configuration which most likely
> > need to be fixed.
>
> In my series I:
> 1- set num_sg to 0
> 2- use transfer_buffer
>
> please correct me if I am wrong but in your solution you did the same since AFAIK
> PageHighMem is always 0 so you end up setting num_sg to 0 and using
> transfer_buffer as well. Is my understanding correct?
Yes. But it still using all existing SG allocation and setup code and
buffer is tracked in urb->sg[0].
> > > Moreover with this approach we avoid some unnecessary operation in the hotpath
> >
> > What unnecessary operation ?
>
> the ones in mt76u_fill_bulk_urb()
Your patches also add extra operations on hotpath due to urb->num_sgs and
dev->usb.sg_en checks.
Stanislaw
> On Tue, Feb 12, 2019 at 03:25:30PM +0100, Lorenzo Bianconi wrote:
> > > On Tue, Feb 12, 2019 at 03:09:53PM +0100, Lorenzo Bianconi wrote:
> > > > >
> > > > > (repost with corrected Lorenzo email)
> > > > >
> > > > > On Tue, Feb 12, 2019 at 02:45:50PM +0100, Stanislaw Gruszka wrote:
> > > > > > On Tue, Feb 12, 2019 at 02:24:47PM +0100, [email protected] wrote:
> > > > > > > From: Lorenzo Bianconi <[email protected]>
> > > > > > >
> > > > > > > Use linear fragment and not a single usb scatter-gather buffer in mt76u
> > > > > > > {tx,rx} datapath if the usb controller has sg data length constraints.
> > > > > > > Moreover add disable_usb_sg module parameter in order to explicitly
> > > > > > > disable scatter-gather. SG I/O is not supported by all host drivers and
> > > > > > > some users have reported sg issues on AMD IOMMU.
> > > > > >
> > > > > > Again. This is not right approach. SG issues should be fixed
> > > > > > not workarounded.
> > > >
> > > > Hi Stanislaw,
> > > >
> > > > here we do not use SG, so num_sg is 0 and we use transfer_buffer. I do
> > > > not see how I am working around the issue.
> > >
> > > By avoiding SG buffer allocation and configuration which most likely
> > > need to be fixed.
> >
> > In my series I:
> > 1- set num_sg to 0
> > 2- use transfer_buffer
> >
> > please correct me if I am wrong but in your solution you did the same since AFAIK
> > PageHighMem is always 0 so you end up setting num_sg to 0 and using
> > transfer_buffer as well. Is my understanding correct?
>
> Yes. But it still using all existing SG allocation and setup code and
> buffer is tracked in urb->sg[0].
both of them are from page_frag_alloc()
>
> > > > Moreover with this approach we avoid some unnecessary operation in the hotpath
> > >
> > > What unnecessary operation ?
> >
> > the ones in mt76u_fill_bulk_urb()
>
> Your patches also add extra operations on hotpath due to urb->num_sgs and
> dev->usb.sg_en checks.
here I guess you should use mt76u_check_sg() instead of
udev->bus->sg_tablesize > 0 so I think you need to precompute it since it is in
the hotpath
Moreover the RFC series has been tested by multiple users and on multiple
devices
Regards,
Lorenzo
>
> Stanislaw
On Tue, Feb 12, 2019 at 04:08:37PM +0100, Lorenzo Bianconi wrote:
> > > > What unnecessary operation ?
> > >
> > > the ones in mt76u_fill_bulk_urb()
> >
> > Your patches also add extra operations on hotpath due to urb->num_sgs and
> > dev->usb.sg_en checks.
>
> here I guess you should use mt76u_check_sg() instead of
> udev->bus->sg_tablesize > 0 so I think you need to precompute it since it is in
> the hotpath
It's not necessary. If udev->bus->sg_tablesize > 0 is true and rest
of mt76u_check_sg() is not, we will submit urb with urb->num_sgs = 1 to
usb host driver, what is fine.
> Moreover the RFC series has been tested by multiple users and on multiple
> devices
I know. Perhaps you could test my patch on rpi ?
Stanislaw
> On Tue, Feb 12, 2019 at 04:08:37PM +0100, Lorenzo Bianconi wrote:
> > > > > What unnecessary operation ?
> > > >
> > > > the ones in mt76u_fill_bulk_urb()
> > >
> > > Your patches also add extra operations on hotpath due to urb->num_sgs and
> > > dev->usb.sg_en checks.
> >
> > here I guess you should use mt76u_check_sg() instead of
> > udev->bus->sg_tablesize > 0 so I think you need to precompute it since it is in
> > the hotpath
>
> It's not necessary. If udev->bus->sg_tablesize > 0 is true and rest
> of mt76u_check_sg() is not, we will submit urb with urb->num_sgs = 1 to
> usb host driver, what is fine.
>
> > Moreover the RFC series has been tested by multiple users and on multiple
> > devices
>
> I know. Perhaps you could test my patch on rpi ?
sure, I will do it later
Regards,
Lorenzo
>
> Stanislaw
>
>
> > On Tue, Feb 12, 2019 at 04:08:37PM +0100, Lorenzo Bianconi wrote:
> > > > > > What unnecessary operation ?
> > > > >
> > > > > the ones in mt76u_fill_bulk_urb()
> > > >
> > > > Your patches also add extra operations on hotpath due to urb->num_sgs and
> > > > dev->usb.sg_en checks.
> > >
> > > here I guess you should use mt76u_check_sg() instead of
> > > udev->bus->sg_tablesize > 0 so I think you need to precompute it since it is in
> > > the hotpath
> >
> > It's not necessary. If udev->bus->sg_tablesize > 0 is true and rest
> > of mt76u_check_sg() is not, we will submit urb with urb->num_sgs = 1 to
> > usb host driver, what is fine.
> >
> > > Moreover the RFC series has been tested by multiple users and on multiple
> > > devices
> >
> > I know. Perhaps you could test my patch on rpi ?
>
> sure, I will do it later
I confirm that even with Stanislaw's patch the usb dongle is properly working
on rpi3+
Regards,
Lorenzo
>
> Regards,
> Lorenzo
>
> >
> > Stanislaw
> >
> >
On Tue, Feb 12, 2019 at 11:09:03PM +0100, Lorenzo Bianconi wrote:
> > > On Tue, Feb 12, 2019 at 04:08:37PM +0100, Lorenzo Bianconi wrote:
> > > > > > > What unnecessary operation ?
> > > > > >
> > > > > > the ones in mt76u_fill_bulk_urb()
> > > > >
> > > > > Your patches also add extra operations on hotpath due to urb->num_sgs and
> > > > > dev->usb.sg_en checks.
> > > >
> > > > here I guess you should use mt76u_check_sg() instead of
> > > > udev->bus->sg_tablesize > 0 so I think you need to precompute it since it is in
> > > > the hotpath
> > >
> > > It's not necessary. If udev->bus->sg_tablesize > 0 is true and rest
> > > of mt76u_check_sg() is not, we will submit urb with urb->num_sgs = 1 to
> > > usb host driver, what is fine.
> > >
> > > > Moreover the RFC series has been tested by multiple users and on multiple
> > > > devices
> > >
> > > I know. Perhaps you could test my patch on rpi ?
> >
> > sure, I will do it later
>
> I confirm that even with Stanislaw's patch the usb dongle is properly working
> on rpi3+
Thanks for testing. Would be ok to you to post my patch against
wireless-drivers tree and cc stable as fix for non-SG usb hosts,
drop your set for -next and work for fix for SG issue on AMD IOMMU?
Stanislaw
On Feb 13, Stanislaw Gruszka wrote:
> On Tue, Feb 12, 2019 at 11:09:03PM +0100, Lorenzo Bianconi wrote:
> > > > On Tue, Feb 12, 2019 at 04:08:37PM +0100, Lorenzo Bianconi wrote:
> > > > > > > > What unnecessary operation ?
> > > > > > >
> > > > > > > the ones in mt76u_fill_bulk_urb()
> > > > > >
> > > > > > Your patches also add extra operations on hotpath due to urb->num_sgs and
> > > > > > dev->usb.sg_en checks.
> > > > >
> > > > > here I guess you should use mt76u_check_sg() instead of
> > > > > udev->bus->sg_tablesize > 0 so I think you need to precompute it since it is in
> > > > > the hotpath
> > > >
> > > > It's not necessary. If udev->bus->sg_tablesize > 0 is true and rest
> > > > of mt76u_check_sg() is not, we will submit urb with urb->num_sgs = 1 to
> > > > usb host driver, what is fine.
> > > >
> > > > > Moreover the RFC series has been tested by multiple users and on multiple
> > > > > devices
> > > >
> > > > I know. Perhaps you could test my patch on rpi ?
> > >
> > > sure, I will do it later
> >
> > I confirm that even with Stanislaw's patch the usb dongle is properly working
> > on rpi3+
>
> Thanks for testing. Would be ok to you to post my patch against
> wireless-drivers tree and cc stable as fix for non-SG usb hosts,
> drop your set for -next and work for fix for SG issue on AMD IOMMU?
I agree that your patch works (since it does not use SG I/O :)) but
I think it is more clear (and manageable) to have two separated
routines for memory allocation.
Moreover I think that this check has to be done in the control plane instead
of the data plane, so I would like to spend some more time in order to see if it is
possible to remove some checks in the hot-path
Regards,
Lorenzo
>
> Stanislaw
On Wed, Feb 13, 2019 at 12:00:42PM +0100, Lorenzo Bianconi wrote:
> On Feb 13, Stanislaw Gruszka wrote:
> > On Tue, Feb 12, 2019 at 11:09:03PM +0100, Lorenzo Bianconi wrote:
> > > > > On Tue, Feb 12, 2019 at 04:08:37PM +0100, Lorenzo Bianconi wrote:
> > > > > > > > > What unnecessary operation ?
> > > > > > > >
> > > > > > > > the ones in mt76u_fill_bulk_urb()
> > > > > > >
> > > > > > > Your patches also add extra operations on hotpath due to urb->num_sgs and
> > > > > > > dev->usb.sg_en checks.
> > > > > >
> > > > > > here I guess you should use mt76u_check_sg() instead of
> > > > > > udev->bus->sg_tablesize > 0 so I think you need to precompute it since it is in
> > > > > > the hotpath
> > > > >
> > > > > It's not necessary. If udev->bus->sg_tablesize > 0 is true and rest
> > > > > of mt76u_check_sg() is not, we will submit urb with urb->num_sgs = 1 to
> > > > > usb host driver, what is fine.
> > > > >
> > > > > > Moreover the RFC series has been tested by multiple users and on multiple
> > > > > > devices
> > > > >
> > > > > I know. Perhaps you could test my patch on rpi ?
> > > >
> > > > sure, I will do it later
> > >
> > > I confirm that even with Stanislaw's patch the usb dongle is properly working
> > > on rpi3+
> >
> > Thanks for testing. Would be ok to you to post my patch against
> > wireless-drivers tree and cc stable as fix for non-SG usb hosts,
> > drop your set for -next and work for fix for SG issue on AMD IOMMU?
>
> I agree that your patch works (since it does not use SG I/O :)) but
It still use SG in mt76usb driver (urb->sg, sg_set_page(),
sg_init_marker(), etc ...) for all usb host controllers. It just
not submit urb->num_sgs > 1 to USB host driver if it does not
support SG.
> I think it is more clear (and manageable) to have two separated
> routines for memory allocation.
Hmm, I disagree on that and don't understand how you consider clearer
and manageable design with two separate buffer allocation methods
and bunch of extra ->num_sgs and ->sg_en checks, compered to solution
with one allocation method and without those checks.
Also some additional cleanups/simplification can be done after
applying my patch in mt76usb not possible after applying your set.
> Moreover I think that this check has to be done in the control plane instead
> of the data plane, so I would like to spend some more time in order to see if it is
> possible to remove some checks in the hot-path
I'm not sure what you mean by data-pane and control-plane in this context.
I considered to do mt76u_fill_buk_urb:
urb->num_sgs = (buf->num_sgs > 1) ? buf->num_sgs : 0;
Instead of usb->bus->sg_tablesize check. It should work, but it's not
what usb_sg_init() does and as pointed by Alan that function always
do the right thing, so I just copied code from there.
Beside that I don't think this check in mt76u_fill_buk_urb()
affects performance whatsoever.
Stanislaw
On 2019-02-12 14:42, [email protected] wrote:
> From: Lorenzo Bianconi <[email protected]>
>
> Use linear fragment and not a single usb scatter-gather buffer in mt76u
> {tx,rx} datapath if the usb controller has sg data length constraints.
> Moreover add disable_usb_sg module parameter in order to explicitly
> disable scatter-gather. SG I/O is not supported by all host drivers and
> some users have reported sg issues on AMD IOMMU.
> This series has been tested on AMD IOMMU cpus/motherboards and on rpi3+
>
> Changes since RFC:
> - rebased on top of 'fix multiple issues in mt76u error path'
> https://patchwork.kernel.org/cover/10804919/
>
> I am resending the series since the first attempt seems to be rejected by
> the ML
Applied, thanks.
- Felix