Re: [RFC][RFT][PATCH] p54usb: rx refill revamp

[Christian Lamparter <chunkeey@web.de>]

On Thursday 22 January 2009 06:40:56 Artur Skawina wrote:
> Artur Skawina wrote:
> > Christian Lamparter wrote:
> >> A updated patch is attached (as file)
> > 
> > Will test.
> 
> Did a quick test of your patch on top of
>   current w-t/pending
> +
>   "p54usb: fix packet loss with first generation devices"
> + this:
> 
> diff --git a/drivers/net/wireless/p54/p54usb.c b/drivers/net/wireless/p54/p54usb.c
> index f754798..20818d6 100644
> --- a/drivers/net/wireless/p54/p54usb.c
> +++ b/drivers/net/wireless/p54/p54usb.c
> @@ -92,7 +92,7 @@ static void p54u_rx_cb(struct urb *urb)
>  
>         if (unlikely(urb->status)) {
>                 dev_kfree_skb_irq(skb);
> -           return;
> +         goto stash_urb;
>         }
>  
>         skb_put(skb, urb->actual_length);
> @@ -141,7 +141,7 @@ static void p54u_rx_cb(struct urb *urb)
>          * the less critical workqueue thread.
>          * This eases the memory pressure and prevents latency spikes.
>          */
> -
> +stash_urb:
>         urb->transfer_buffer = NULL;
>         urb->context = NULL;
>  
> That didn't work, various symptoms such as hostapd getting stuck, other unrelated
> usb devices breaking, machine frozen etc.
> 
> After applying the changes below it seemed to work alright, but i only tested for
> a few minutes.
> [line #s are wrong]
> 
> diff --git a/drivers/net/wireless/p54/p54usb.c b/drivers/net/wireless/p54/p54usb.c
> index 20818d6..643d517 100644
> --- a/drivers/net/wireless/p54/p54usb.c
> +++ b/drivers/net/wireless/p54/p54usb.c
> @@ -160,7 +173,7 @@ stash_urb:
>  		 * Huh? mac80211 isn't fully initialized yet?
>  		 * Please check your system, something bad is going on.
>  		 */
> -		WARN_ON(1);
> +		printk(KERN_WARNING "p54u_rx_cb workqueue missing\n");
>  		return;
>  	}
>  
> @@ -196,9 +210,9 @@ static void p54u_free_urbs(struct ieee80211_hw *dev)
>  {
>  	struct p54u_priv *priv = dev->priv;
>  
>  	cancel_work_sync(&priv->rx_refill_work);
> -	p54u_free_rx_refill_list(dev);
>  	usb_kill_anchored_urbs(&priv->submitted);
> +	p54u_free_rx_refill_list(dev);
>  }
>  
>  static int p54u_rx_refill(struct ieee80211_hw *dev)
> @@ -281,9 +297,9 @@ static int p54u_init_urbs(struct ieee80211_hw *dev)
>  	}
>  
>  	p54u_rx_refill(dev);
> -
>  err:
> -	p54u_free_rx_refill_list(dev);
> +	if (ret)
> +		p54u_free_rx_refill_list(dev);
>  	return ret;
>  }
>  
> The 'workqueue-missing' warnings appear on module init; there are several
> p54u_init_urbs(), p54u_free_urbs() call sequences.
> p54u_free_urbs() triggers the unlikely(urb->status) path while killing
> the urbs, and the first time this happens you get 32 warnings.
> After that everything's fine.

the workqueue is available as soon as ieee80211_register_hw completed,
however before we can do that, we have to get the eeprom for the MAC...

The reason why this works without "warning" is that we recycle the skb
of the eeprom readback... so the queue_work is never executed on start up
and if it is then something is very wrong with the device itself.
 
> I put the machine under some load and while there was usually 28..31 RX
> urbs queued, at one point the refilling stalled and the number went down
> from 31 to 1, then 6, slowly grew to ~30, then fell to 13, then recovered.
> All of this within ~5 mins of testing, w/ only a small amount of http 
> traffic generated by the client. it looks like i will have to try 
> allocating in the interrupt after all.

was this with your patch to use rx_refills urb pool for tx, or without?

Regards,
	Chr