On Mon, 29 Oct 2007 22:51:42 +0100 Florian Fainelli <[EMAIL PROTECTED]> wrote:
> This patch adds support for the RDC R6040 MAC we can find in the RDC R-321x > System-on-chips. > This driver really needs improvements especially on the NAPI part which > probably does not > fully use the new NAPI structure. > You will need the RDC PCI identifiers if you want to test this driver which > are the following ones : > > RDC_PCI_VENDOR_ID = 0x17f3 > RDC_PCI_DEVICE_ID_RDC_R6040 = 0x6040 > > Thank you very much in advance for your comments. > > Signed-off-by: Sten Wang <[EMAIL PROTECTED]> > Signed-off-by: Daniel Gimpelevich <[EMAIL PROTECTED]> > Signed-off-by: Florian Fainelli <[EMAIL PROTECTED]> **** BUG *** Don't call kfree() to free the network device; use free_netdev() * Don't define use uppercase for variable names (NUM_MAC_TABLE) * Use get_random_ether_addr() rather than a hardcoded table of mac addresses. * checkpatch complains about some extra blanks, and several lines > 80 chars. * use ethtool stubs for check_link * add ethtool get_settings to allow use by bonding/bridging, etc. * this is unusual coding style: + do {} while ((i++ < 2048) && (inw(ioaddr + 0x04) & 0x1)); * add a blank line after declarations and before code in a function * use of global NAPI_status should be replaced by putting it in priv * the handling of shared IRQ is wrong. - need to check for status == 0 || status == 0xffff and return IRQ_NONE * don't call napi_disable() with irq's disabled in r6040_close * poll routine shouldn't call dev_kfree_skb_irq() to free Tx buffers because that means going through TX softirq, just call dev_kfree_skb() * the down routine calls pci_unmap_single with wrong length when handling TX buffers. * pci id table can be cleaned up: static struct pci_device_id r6040_pci_tbl[] = { { PCI_DEVICE(PCI_VENDOR_ID_RDC, 0x6040) }, { PCI_DEVICE(PCI_VENDOR_VIA, 0x3065) }, { 0 } }; * use netdev_priv() consistently rather than dev->priv. Yes they are the same now, but that will be fixed in future. * eliminate check for dev being NULL in IRQ handler. * reorder functions to eliminate need for forward declarations * get rid of R6040_PCI_CMD and pci_flags field it is unused. * do you really have to have the whole chip_info at all? The only usage seems to be to validate the pci region size. Do you have platforms with busted BIOS that set it wrong or something?? --- WARNING: no space between function name and open parenthesis '(' #1071: FILE: drivers/net/r6040.c:958: +static int __init r6040_init (void) WARNING: no space between function name and open parenthesis '(' #1073: FILE: drivers/net/r6040.c:960: + return pci_register_driver (&r6040_driver); WARNING: no space between function name and open parenthesis '(' #1077: FILE: drivers/net/r6040.c:964: +static void __exit r6040_cleanup (void) WARNING: no space between function name and open parenthesis '(' #1079: FILE: drivers/net/r6040.c:966: + pci_unregister_driver (&r6040_driver); total: 0 errors, 36 warnings, 1001 lines checked Your patch has style problems, please review. If any of these errors are false positives report them to the maintainer, see CHECKPATCH in MAINTAINERS. - To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html