netdev - Re: [PATCH][RFC] Add support for the RDC R6040 Fast Ethernet controller

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <47284392.80009@garzik.org>
Date:	Wed, 31 Oct 2007 04:57:54 -0400
From:	Jeff Garzik <jeff@...zik.org>
To:	Florian Fainelli <florian.fainelli@...ecomint.eu>
CC:	netdev@...r.kernel.org
Subject: Re: [PATCH][RFC] Add support for the RDC R6040 Fast Ethernet controller

Florian Fainelli 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 <sten.wang@....com.tw>
> Signed-off-by: Daniel Gimpelevich <daniel@...pelevich.san-francisco.ca.us>
> Signed-off-by: Florian Fainelli <florian.fainelli@...ecomint.eu>

Looks nice and clean to me.  Pre-merge stuff I think needs fixing:

* clean up NAPI as you describe (and delete non-NAPI code paths, unless 
there is a strong reason to keep them).

* unconditional local_irq_{enable,disable} stuff

* spin_lock_irqsave() should not be needed in interrupt handler. 
[perhaps you did this rather than put the slower locking in 
->poll_controller()]

* remove changelog from C header (git repository log is our changelog)

* handle large dev->mc_count, as you note in the C header

* use __le32 and similar data types. validate with sparse 
(Documentation/sparse.txt)

* consider using ioread{8,16,32} and iowrite{8,16,32}, if your platform 
permits.  Then switch from 'unsigned long' to special marker 'void 
__iomem *' for all I/O port addresses

* use DMA_32BIT_MASK rather than 0xffffffff in pci_set_dma_mask() call

* in r6040_init_one() call is_valid_ether_addr(), rather than 
hand-rolling the same code yourself

* you need to note carrier state when it changes, using 
netif_carrier_on() and netif_carrier_off()

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