Re: [PATCH] 2.6.0-test2 wanXL driver

[Krzysztof Halasa <khc@pm.waw.pl>]

Francois Romieu <romieu@fr.zoreil.com> writes:

> unsigned long timeout = jiffies + HZ;
> 
> do {
> ...
> } while (time_after(timeout, jiffies));

That's the same (my version deals with wraps too) but yes, time_after
looks nicer. I'll change it in next version.


> +	switch (pdev->device) {
> +	case PCI_DEVICE_ID_SBE_WANXL100: ports = 1; break;
> +	case PCI_DEVICE_ID_SBE_WANXL200: ports = 2; break;
> +	default: ports = 4;
> +	}
> 
> You can do it this way:
> static struct pci_device_id wanxl_pci_tbl[] __devinitdata = {
> 	{ PCI_VENDOR_ID_SBE, PCI_DEVICE_ID_SBE_WANXL100, PCI_ANY_ID,
> 	  PCI_ANY_ID, 0, 0, 1 },
>         { PCI_VENDOR_ID_SBE, PCI_DEVICE_ID_SBE_WANXL200, PCI_ANY_ID,
>           PCI_ANY_ID, 0, 0, 2 },
>         { PCI_VENDOR_ID_SBE, PCI_DEVICE_ID_SBE_WANXL400, PCI_ANY_ID,
>           PCI_ANY_ID, 0, 0, 4 },
>         { 0, }
> };
> 
> ports = ent->driver_data;
> 
> (imho turning 1, 2, 4 into #define wouldn't be bad then)

Yes, but I like having the variable local to this function a little more.

> +	while ((stat = readl(card->plx + PLX_MAILBOX_0)) != 0) {
> +		if (jiffies - start >= 20 * HZ) {
> +			printk(KERN_WARNING "wanXL %s: timeout waiting for"
> +			       " PUTS to complete\n", card_name(pdev));
> +			return -ENODEV;
> 
> This return leaks kmalloced card_t *card, pci_requested_regions and
> ioremaped area (same thing for the return a few lines below in the
> same function).

Right. Will fix in next version.

> +	for (i = 0; i < RX_QUEUE_LENGTH; i++) {
> +		struct sk_buff *skb = dev_alloc_skb(BUFFER_LENGTH);
> +		card->rx_skbs[i] = skb;
> +		if (skb)
> +			card->rx_descs[i].address = virt_to_bus(skb->data);
> 
> dma_map_single() is probably preferred over virt_to_bus().

Never heard of it in fact :-)
Will check.

Thanks.
-- 
Krzysztof Halasa
Network Administrator