some fixes to my last mail:


quoted text
>> This is a usb driver for the intellon int51x1 based PLC
>> (Powerline Communications) like devolo dlan duo.
>>
>> Signed-off-by: Peter Holik <peter@holik.at>
>> ---
>>  drivers/net/usb/Kconfig   |    8 ++
>>  drivers/net/usb/Makefile  |    1 +
>>  drivers/net/usb/int51x1.c |  268 +++++++++++++++++++++++++++++++++++++++++++++
>> 3 files changed, 277 insertions(+), 0 deletions(-)
>>  create mode 100644 drivers/net/usb/int51x1.c
>>
>> diff --git a/drivers/net/usb/int51x1.c b/drivers/net/usb/int51x1.c new file mode 100644
>> index 0000000..44a8a09
>> --- /dev/null
>> +++ b/drivers/net/usb/int51x1.c
>> @@ -0,0 +1,268 @@
>
>> +static int int51x1_rx_fixup(struct usbnet *dev, struct sk_buff *skb)
>> +{
>> +	int len;
>> +
>> +	if (unlikely(skb->len < INT51X1_HEADER_SIZE)) {
>
> pskb_may_pull(...)

ok, done:

static int int51x1_rx_fixup(struct usbnet *dev, struct sk_buff *skb) {
        int len;

        if (!(pskb_may_pull(skb, INT51X1_HEADER_SIZE))) {
                deverr(dev, "unexpected tiny rx frame");
                return 0;
        }

        len = le16_to_cpu(*(__le16 *)&skb->data[skb->len - 2]);

        skb_trim(skb, len);

        return 1;
}

quoted text
>> +		deverr(dev, "unexpected tiny rx frame");
>> +		return 0;
>> +	}
>> +
>> +	len = le16_to_cpu(*(__le16 *)&skb->data[skb->len - 2]);
>> +
>> +	skb_trim(skb, len);
>> +
>> +	return 1;
>> +}
>> +
>> +static struct sk_buff *int51x1_tx_fixup(struct usbnet *dev,
>> +		struct sk_buff *skb, gfp_t flags)
>> +{
>> +	int pack_len = skb->len;
>> +	int headroom = skb_headroom(skb);
>> +	int tailroom = skb_tailroom(skb);
>> +	int need_tail = 0;
>> +	__le16 *len;
>> +
>> +	/*
>> +	 * usbnet would send a ZLP if packetlength mod urbsize == 0 for us,
>> +	 * but we need to know ourself, because this would add to the length
>> +	 * we send down to the device...
>> +	 */
>> +	if (!((pack_len + INT51X1_HEADER_SIZE) % dev->maxpacket))
>> +		need_tail = 1;
>> +
>> +	/* if packet and our header is smaler than 64 pad to 64 (+ ZLP) */
>> +	if ((pack_len + INT51X1_HEADER_SIZE + need_tail) < dev->maxpacket + 1)
>> +		need_tail = dev->maxpacket + 1 - pack_len - INT51X1_HEADER_SIZE;
>
> This is totally crazy code fragment, first need_tail is used like a boolean?
> But on the same some +1 scalar trick is being done?
> Is there some reason why DIV_ROUND_UP (linux/kernel.h)
> won't do what you want here and then you can trivally find diff = new - old ?
>


sorry i can't follow, can you convert this code to DIV_ROUND_UP


quoted text
>> +	if (!skb_cloned(skb) &&
>> +			(headroom + tailroom >= need_tail + INT51X1_HEADER_SIZE)) {
>> +		if (headroom < INT51X1_HEADER_SIZE || tailroom < need_tail) {
>> +			skb->data = memmove(skb->head + INT51X1_HEADER_SIZE,
>> +					skb->data, skb->len);
>> +			skb_set_tail_pointer(skb, skb->len);
>> +		}
>> +	} else {
>> +		struct sk_buff *skb2;
>> +
>> +		skb2 = skb_copy_expand(skb,
>> +				INT51X1_HEADER_SIZE,
>> +				need_tail,
>> +				flags);
>> +		dev_kfree_skb_any(skb);
>> +		if (!skb2)
>> +			return NULL;
>> +		skb = skb2;
>> +	}
>> +
>> +	pack_len += need_tail;
>> +	pack_len &= 0x07ff;
>> +
>> +	len = (__le16 *) __skb_push(skb, INT51X1_HEADER_SIZE);
>> +	*len = cpu_to_le16(pack_len);
>> +
>> +	if(need_tail)
>> +		memset(__skb_put(skb, need_tail), 0, need_tail);
>> +
>> +	return skb;
>> +}
>> +
>> +static void int51x1_async_cmd_callback(struct urb *urb)
>> +{
>> +	struct usb_ctrlrequest *req = (struct usb_ctrlrequest *)urb->context;
>> +	int status = urb->status;
>> +
>> +	if (status < 0)
>> +		dev_warn(&urb->dev->dev, "async callback failed with %d\n", status);
>> +
>> +	kfree(req);
>> +	usb_free_urb(urb);
>> +}
>> +
>> +static void int51x1_set_multicast(struct net_device *netdev)
>> +{
>> +	struct usb_ctrlrequest *req;
>> +	int status;
>> +	struct urb *urb;
>> +	struct usbnet *dev = netdev_priv(netdev);
>> +	u16 filter = PACKET_TYPE_DIRECTED |
>> +		     PACKET_TYPE_BROADCAST;
>
> Won't this fit on a single line?
>

ok, done

quoted text
>> +
>> +	if (netdev->flags & IFF_PROMISC) {
>> +		/* do not expect to see traffic of other PLCs */
>> +		filter |= PACKET_TYPE_PROMISCUOUS;
>> +		devinfo(dev, "promiscuous mode enabled");
>> +	} else if (netdev->mc_count ||
>> +		  (netdev->flags & IFF_ALLMULTI)) {
>> +		filter |= PACKET_TYPE_ALL_MULTICAST;
>> +		devdbg(dev, "receive all multicast enabled");
>> +	} else {
>> +		/* ~PROMISCUOUS, ~MULTICAST */
>> +		devdbg(dev, "receive own packets only");
>> +	}
>> +
>> +	urb = usb_alloc_urb(0, GFP_ATOMIC);
>> +	if (!urb) {
>> +		devwarn(dev, "Error allocating URB");
>> +		return;
>> +	}
>> +
>> +	req = kmalloc(sizeof *req, GFP_ATOMIC);
>
> sizeof(*req)
>

ok, done

quoted text
>> +	if (!req) {
>> +		devwarn(dev, "Error allocating control msg");
>> +		usb_free_urb(urb);
>> +		return;
>
> I'd use gotos instead for error handling since you need similar call in the later
> error handling block too. Gotos make it somewhat harder to miss some necessary
> rollback actions in one of the error blocks (not that this case is buggy atm).
>

ok, done like you suggested - see at the end

quoted text
>> +	}
>> +
>> +	req->bRequestType = USB_DIR_OUT | USB_TYPE_CLASS | USB_RECIP_INTERFACE;
>> +	req->bRequest = SET_ETHERNET_PACKET_FILTER;
>> +	req->wValue = cpu_to_le16(filter);
>> +	req->wIndex = 0;
>> +	req->wLength = 0;
>> +
>> +	usb_fill_control_urb(urb, dev->udev, usb_sndctrlpipe(dev->udev, 0),
>> +		(void *)req, NULL, 0,
>> +		int51x1_async_cmd_callback,
>> +		(void *)req);
>> +
>> +	status = usb_submit_urb(urb, GFP_ATOMIC);
>> +	if (status < 0) {
>> +		devwarn(dev, "Error submitting control msg, sts=%d", status);
>> +		kfree(req);
>> +		usb_free_urb(urb);
>
> Ditto.

static void int51x1_set_multicast(struct net_device *netdev)
{
        struct usb_ctrlrequest *req;
        int status;
        struct urb *urb;
        struct usbnet *dev = netdev_priv(netdev);
        u16 filter = PACKET_TYPE_DIRECTED | PACKET_TYPE_BROADCAST;

        if (netdev->flags & IFF_PROMISC) {
                /* do not expect to see traffic of other PLCs */
                filter |= PACKET_TYPE_PROMISCUOUS;
                devinfo(dev, "promiscuous mode enabled");
        } else if (netdev->mc_count ||
                  (netdev->flags & IFF_ALLMULTI)) {
                filter |= PACKET_TYPE_ALL_MULTICAST;
                devdbg(dev, "receive all multicast enabled");
        } else {
                /* ~PROMISCUOUS, ~MULTICAST */
                devdbg(dev, "receive own packets only");
        }

        urb = usb_alloc_urb(0, GFP_ATOMIC);
        if (!urb) {
                devwarn(dev, "Error allocating URB");
                return;
        }

        req = kmalloc(sizeof(*req), GFP_ATOMIC);
        if (!req) {
                devwarn(dev, "Error allocating control msg");
                goto out;
        }

        req->bRequestType = USB_DIR_OUT | USB_TYPE_CLASS | USB_RECIP_INTERFACE;
        req->bRequest = SET_ETHERNET_PACKET_FILTER;
        req->wValue = cpu_to_le16(filter);
        req->wIndex = 0;
        req->wLength = 0;

        usb_fill_control_urb(urb, dev->udev, usb_sndctrlpipe(dev->udev, 0),
                (void *)req, NULL, 0,
                int51x1_async_cmd_callback,
                (void *)req);

        status = usb_submit_urb(urb, GFP_ATOMIC);
        if (status < 0) {
                devwarn(dev, "Error submitting control msg, sts=%d", status);
                goto out1;
        }
        return;
out1:
        kfree(req);
out:
        usb_free_urb(urb);
}


cu Peter



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