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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
