Re: Active URB submitted twice in pegasus driver

[Sarah Sharp <sarah.a.sharp-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>]

On Tue, Mar 26, 2013 at 10:54:25PM +0200, Petko Manolov wrote:
> On Tue, 26 Mar 2013, Sarah Sharp wrote:
> 
> >But considering the multicast code is pretty old, I bet I'm running into
> >a different bug...
> 
> The fix is rather small.  Approximately one gigabyte was transferred
> while alternating in and out of promiscuous mode.  No issues.
> 
> I guess this should move the driver out of the equation.
> 
> 
> cheers,
> Petko

> diff --git a/drivers/net/usb/pegasus.c b/drivers/net/usb/pegasus.c
> index 73051d1..879da39 100644
> --- a/drivers/net/usb/pegasus.c
> +++ b/drivers/net/usb/pegasus.c
> @@ -1220,8 +1220,6 @@ static void pegasus_set_multicast(struct net_device *net)
>  		pegasus->eth_regs[EthCtrl2] &= ~RX_PROMISCUOUS;
>  	}
>  
> -	pegasus->ctrl_urb->status = 0;
> -
>  	pegasus->flags |= ETH_REGS_CHANGE;
>  	ctrl_callback(pegasus->ctrl_urb);
>  }

This patch does avoid writes to the URB, however...

static void ctrl_callback(struct urb *urb)
{
        pegasus_t *pegasus = urb->context;
        int status = urb->status;

        if (!pegasus)
                return;

        switch (status) {
        case 0:
                if (pegasus->flags & ETH_REGS_CHANGE) {
                        pegasus->flags &= ~ETH_REGS_CHANGE;
                        pegasus->flags |= ETH_REGS_CHANGED;
                        update_eth_regs_async(pegasus);
                        return;
                }
                break;

ctrl_callback is still reading the URB status, and using it in the
switch statement.  It's also using the urb->context as well, to dig out
a pointer (pegasus_t) that the pegasus_set_multicast already has access
to.  What happens if an URB to get the registers completes at the same
time pegasus_set_multicast calls ctrl_callback?  If the URB failed for
some reason (e.g. bad cable, stall), you'll miss that if statement.  I
don't think that's what you intended to do.

I think the fix should be to just to move the if block into a new
function, and call it both in ctrl_callback() and
pegasus_set_multicast().

Further, is it possible to call any of these functions asynchronously?
 - get_registers
 - set_registers
 - update_eth_regs_async

What happens if the upper layer calls get_registers and
update_eth_regs_async in rapid succession?  You'll attempt to use the
control URB in both places, which will trigger the original warning I
was reporting.  Or is there some locking somewhere I'm missing?

Sarah Sharp
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html