Re: [PATCH] rtl8187: Use usb anchor facilities to manage urbs

[Larry Finger <Larry.Finger@lwfinger.net>]

Herton Ronaldo Krzesinski wrote:
> Em Ter=E7a 09 Dezembro 2008, =E0s 14:33:29, Larry Finger escreveu:
>=20
> Hi, I looked at the patch and also the discussion on LKML about the p=
54usb,=20
> just reading the patch now spotted some minor things, see below. Othe=
rwise=20
> everything else looks fine.

--snip--

>>  	if (unlikely(!skb)) {
>> -		usb_free_urb(urb);
>>  		/* TODO check rx queue length and refill *somewhere* */
>>  		return;
>>  	}
>=20
> May be remove { } ? Probably checkpatch.pl didn't spot this because o=
f the=20
> comment using one line, but if it's allowed to keep {} in this case b=
ecause of=20
> the comment no problem.

I don't know what policy is for this, but I removed the {} even though
checkpatch didn't flag it.

>> @@ -373,24 +373,30 @@ static void rtl8187_rx_cb(struct urb *ur
>>  	urb->context =3D skb;
>>  	skb_queue_tail(&priv->rx_queue, skb);
>>
>> -	usb_submit_urb(urb, GFP_ATOMIC);
>> +	usb_anchor_urb(urb, &priv->anchored);
>> +	if (usb_submit_urb(urb, GFP_ATOMIC))
>> +		usb_unanchor_urb(urb);
>=20
> may be we should also skb_unlink and dev_kfree_skb_irq skb here like =
on=20
> p54usb? although it should be freed anyway later on rtl8187_stop

Done. I think the whole error return philosophy in rtl8187 needs some c=
hecking.
There are a number of routines, such as this one, that detect an error =
but never
report it anywhere. That seems wrong.

>>  }
>>
>>  static int rtl8187_init_urbs(struct ieee80211_hw *dev)
>>  {
>>  	struct rtl8187_priv *priv =3D dev->priv;
>> -	struct urb *entry;
>> +	struct urb *entry =3D NULL;
>>  	struct sk_buff *skb;
>>  	struct rtl8187_rx_info *info;
>> +	int ret =3D 0;
>>
>>  	while (skb_queue_len(&priv->rx_queue) < 8) {
>>  		skb =3D __dev_alloc_skb(RTL8187_MAX_RX, GFP_KERNEL);
>> -		if (!skb)
>> -			break;
>> +		if (!skb) {
>> +			ret =3D -ENOMEM;
>> +			goto err;
>> +		}
>>  		entry =3D usb_alloc_urb(0, GFP_KERNEL);
>>  		if (!entry) {
>>  			kfree_skb(skb);
>=20
> kfree_skb is also called after err: too now.

=46ixed.

>> -			break;
>> +			ret =3D -ENOMEM;
>> +			goto err;
>>  		}

--snip--

>> @@ -415,7 +433,6 @@ static void rtl8187b_status_cb(struct ur
>>  	unsigned int cmd_type;
>>
>>  	if (unlikely(urb->status)) {
>> -		usb_free_urb(urb);
>>  		return;
>>  	}
>=20
> remove { }

Done. I wonder why checkpatch didn't complain here. I guess it doesn't =
handle
the case where the patch changes a 2-line if clause into a one-liner.

Thanks for the review.

Larry

--
To unsubscribe from this list: send the line "unsubscribe linux-wireles=
s" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html