Herton Ronaldo Krzesinski wrote:
> Em Terça 09 Dezembro 2008, às 14:33:29, Larry Finger escreveu:
>
> Hi, I looked at the patch and also the discussion on LKML about the p54usb,
> just reading the patch now spotted some minor things, see below. Otherwise
> everything else looks fine.
--snip--
>> if (unlikely(!skb)) {
>> - usb_free_urb(urb);
>> /* TODO check rx queue length and refill *somewhere* */
>> return;
>> }
>
> May be remove { } ? Probably checkpatch.pl didn't spot this because of the
> comment using one line, but if it's allowed to keep {} in this case because of
> 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 = 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);
>
> may be we should also skb_unlink and dev_kfree_skb_irq skb here like on
> p54usb? although it should be freed anyway later on rtl8187_stop
Done. I think the whole error return philosophy in rtl8187 needs some checking.
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 = dev->priv;
>> - struct urb *entry;
>> + struct urb *entry = NULL;
>> struct sk_buff *skb;
>> struct rtl8187_rx_info *info;
>> + int ret = 0;
>>
>> while (skb_queue_len(&priv->rx_queue) < 8) {
>> skb = __dev_alloc_skb(RTL8187_MAX_RX, GFP_KERNEL);
>> - if (!skb)
>> - break;
>> + if (!skb) {
>> + ret = -ENOMEM;
>> + goto err;
>> + }
>> entry = usb_alloc_urb(0, GFP_KERNEL);
>> if (!entry) {
>> kfree_skb(skb);
>
> kfree_skb is also called after err: too now.
Fixed.
>> - break;
>> + ret = -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;
>> }
>
> 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-wireless" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
