With current driver, it is observed that a URB is not
completed while the USB disconnect is initiated. Due to
that, the URB completion hanlder is trying to access
the resource which was freed as a part of USB disconnect.
Managing the URBs with anchors will make sure that all
the URBs are handled gracefully before device gets
disconnected.
Signed-off-by: Vishal Thanki <[email protected]>
---
drivers/net/wireless/ralink/rt2x00/rt2x00usb.c | 35 ++++++++++++++++++++++++--
1 file changed, 33 insertions(+), 2 deletions(-)
diff --git a/drivers/net/wireless/ralink/rt2x00/rt2x00usb.c b/drivers/net/wireless/ralink/rt2x00/rt2x00usb.c
index 7627af6..a2ed3e1 100644
--- a/drivers/net/wireless/ralink/rt2x00/rt2x00usb.c
+++ b/drivers/net/wireless/ralink/rt2x00/rt2x00usb.c
@@ -34,6 +34,15 @@
/*
* Interfacing with the HW.
*/
+
+struct rt2x00usb_anchors {
+ struct usb_anchor async_urb;
+ struct usb_anchor tx_submitted;
+ struct usb_anchor rx_submitted;
+};
+
+static struct rt2x00usb_anchors *anchors;
+
int rt2x00usb_vendor_request(struct rt2x00_dev *rt2x00dev,
const u8 request, const u8 requesttype,
const u16 offset, const u16 value,
@@ -171,8 +180,11 @@ static void rt2x00usb_register_read_async_cb(struct urb *urb)
{
struct rt2x00_async_read_data *rd = urb->context;
if (rd->callback(rd->rt2x00dev, urb->status, le32_to_cpu(rd->reg))) {
- if (usb_submit_urb(urb, GFP_ATOMIC) < 0)
+ usb_anchor_urb(urb, &anchors->async_urb);
+ if (usb_submit_urb(urb, GFP_ATOMIC) < 0) {
+ usb_unanchor_urb(urb);
kfree(rd);
+ }
} else
kfree(rd);
}
@@ -206,8 +218,11 @@ void rt2x00usb_register_read_async(struct rt2x00_dev *rt2x00dev,
usb_fill_control_urb(urb, usb_dev, usb_rcvctrlpipe(usb_dev, 0),
(unsigned char *)(&rd->cr), &rd->reg, sizeof(rd->reg),
rt2x00usb_register_read_async_cb, rd);
- if (usb_submit_urb(urb, GFP_ATOMIC) < 0)
+ usb_anchor_urb(urb, &anchors->async_urb);
+ if (usb_submit_urb(urb, GFP_ATOMIC) < 0) {
+ usb_unanchor_urb(urb);
kfree(rd);
+ }
usb_free_urb(urb);
}
EXPORT_SYMBOL_GPL(rt2x00usb_register_read_async);
@@ -313,8 +328,10 @@ static bool rt2x00usb_kick_tx_entry(struct queue_entry *entry, void *data)
entry->skb->data, length,
rt2x00usb_interrupt_txdone, entry);
+ usb_anchor_urb(entry_priv->urb, &anchors->tx_submitted);
status = usb_submit_urb(entry_priv->urb, GFP_ATOMIC);
if (status) {
+ usb_unanchor_urb(entry_priv->urb);
if (status == -ENODEV)
clear_bit(DEVICE_STATE_PRESENT, &rt2x00dev->flags);
set_bit(ENTRY_DATA_IO_FAILED, &entry->flags);
@@ -402,8 +419,10 @@ static bool rt2x00usb_kick_rx_entry(struct queue_entry *entry, void *data)
entry->skb->data, entry->skb->len,
rt2x00usb_interrupt_rxdone, entry);
+ usb_anchor_urb(entry_priv->urb, &anchors->rx_submitted);
status = usb_submit_urb(entry_priv->urb, GFP_ATOMIC);
if (status) {
+ usb_unanchor_urb(entry_priv->urb);
if (status == -ENODEV)
clear_bit(DEVICE_STATE_PRESENT, &rt2x00dev->flags);
set_bit(ENTRY_DATA_IO_FAILED, &entry->flags);
@@ -818,6 +837,14 @@ int rt2x00usb_probe(struct usb_interface *usb_intf,
if (retval)
goto exit_free_reg;
+ anchors = devm_kmalloc(&usb_dev->dev, sizeof(struct rt2x00usb_anchors),
+ GFP_KERNEL);
+ if (!anchors)
+ goto exit_free_reg;
+
+ init_usb_anchor(&anchors->async_urb);
+ init_usb_anchor(&anchors->tx_submitted);
+ init_usb_anchor(&anchors->rx_submitted);
return 0;
exit_free_reg:
@@ -840,6 +867,10 @@ void rt2x00usb_disconnect(struct usb_interface *usb_intf)
struct ieee80211_hw *hw = usb_get_intfdata(usb_intf);
struct rt2x00_dev *rt2x00dev = hw->priv;
+ usb_kill_anchored_urbs(&anchors->async_urb);
+ usb_kill_anchored_urbs(&anchors->tx_submitted);
+ usb_kill_anchored_urbs(&anchors->rx_submitted);
+
/*
* Free all allocated data.
*/
--
2.4.3
Hi,
On Thu, Mar 17, 2016 at 09:43:32AM +0100, Stanislaw Gruszka wrote:
> Hi
>
> On Wed, Mar 16, 2016 at 06:28:51PM +0100, Vishal Thanki wrote:
> > +struct rt2x00usb_anchors {
> > + struct usb_anchor async_urb;
> > + struct usb_anchor tx_submitted;
> > + struct usb_anchor rx_submitted;
> > +};
>
> I don't think we need 3 different usb_anchor's, one should be
> enough.
>
Ok, I will change that.
> > +static struct rt2x00usb_anchors *anchors;
>
> usb_anchor structure should be embedded in rt2x00_dev structure,
> otherwise you can kill urb's from other rt2x00 devices, when
> disconnecting another one.
>
rt2x00_dev structure does not contain any bus specific data structures
(for example no structures present for USB/PCI). Should I add a void
*ptr in rt2x00_dev and cast it as usb_anchor in rt2x00usb.c?
Thanks,
Vishal
> Stanislaw
Hi
On Wed, Mar 16, 2016 at 06:28:51PM +0100, Vishal Thanki wrote:
> +struct rt2x00usb_anchors {
> + struct usb_anchor async_urb;
> + struct usb_anchor tx_submitted;
> + struct usb_anchor rx_submitted;
> +};
I don't think we need 3 different usb_anchor's, one should be
enough.
> +static struct rt2x00usb_anchors *anchors;
usb_anchor structure should be embedded in rt2x00_dev structure,
otherwise you can kill urb's from other rt2x00 devices, when
disconnecting another one.
Stanislaw
On Thu, Mar 17, 2016 at 10:53:18AM +0100, Vishal Thanki wrote:
> > usb_anchor structure should be embedded in rt2x00_dev structure,
> > otherwise you can kill urb's from other rt2x00 devices, when
> > disconnecting another one.
> >
>
> rt2x00_dev structure does not contain any bus specific data structures
> (for example no structures present for USB/PCI). Should I add a void
> *ptr in rt2x00_dev and cast it as usb_anchor in rt2x00usb.c?
No, just add usb_anchor to rt2x00_dev (and include proper header if
needed).
Stanislaw