netdev - Re: [PATCH] Add support the Korina (IDT RC32434) Ethernet MAC

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: <1204886746.6387.17.camel@johannes.berg>
Date:	Fri, 07 Mar 2008 11:45:46 +0100
From:	Johannes Berg <johannes@...solutions.net>
To:	Florian Fainelli <florian.fainelli@...ecomint.eu>
Cc:	David Miller <davem@...emloft.net>, netdev@...r.kernel.org,
	Jeff Garzik <jeff@...zik.org>, Felix Fietkau <nbd@...nwrt.org>
Subject: Re: [PATCH] Add support the Korina (IDT RC32434) Ethernet MAC

 
> +config KORINA
> +	tristate "Korina (IDT rc32434) Ethernet support"
> +	depends on NET_ETHERNET && MIKROTIK_RB500
> +	help
> +	  If you have a Mikrotik RouterBoard 500 or IDT RC32434
> +	  based system say Y. Otherwise say N.

Since you'll never going to see that prompt if you don't have such a
board, that seems pretty unhelpful :)

> +#define TX_TIMEOUT 	(HZ * 100)

Not sure where this is used, but 100 seconds seems quite long, no?

> +enum status { filled, empty };

I'd use names with some prefixes, maybe descriptor_filled? Matter of
taste though I guess.

> +static inline void korina_start_dma(struct dma_reg *ch, u32 dma_addr)

> +static inline void korina_abort_dma(struct net_device *dev,

> +static inline void korina_chain_dma(struct dma_reg *ch, u32 dma_addr)

> +static inline void korina_abort_tx(struct net_device *dev)
> +{
> +	struct korina_private *lp = netdev_priv(dev);
> +
> +	korina_abort_dma(dev, lp->tx_dma_regs);
> +}

All those inlines seems a bit excessive since korina_abort_dma is an
inline too and then inlined into two other functions... Are those really
necessary?

> +static inline void korina_start_tx(struct korina_private *lp,
> +					struct dma_desc *td)
> +{
> +	korina_start_dma(lp->tx_dma_regs, CPHYSADDR(td));

Similarly here although korina_start_dma is a lot shorter than stop_dma.

> +	/* stop queue when full, drop pkts if queue already full */
> +	if (lp->tx_count >= (KORINA_NUM_TDS - 2)) {

> +		if (lp->tx_count == (KORINA_NUM_TDS - 2))
> +			netif_stop_queue(dev);

> +	if (readl(&(lp->tx_dma_regs->dmandptr)) == 0) {

Why so many parentheses? :)

> +		/* Mask D H E bit in Rx DMA */
> +		dmasm = readl(&lp->rx_dma_regs->dmasm);
> +		writel(dmasm | (DMA_STAT_DONE |
> +				DMA_STAT_HALT | DMA_STAT_ERR),

Pretty useless comment especially since you have to read the code to
understand it! Or was this intended as a comment on how the register
write works? Similar instances all over the driver. Maybe a single
comment at the top of the file on how that register works would be good
instead.

> +static void korina_multicast_list(struct net_device *dev)
> +{
> +	/* listen to broadcasts always and to treat 	*/
> +	/*       IFF bits independantly	*/

typo: independently

> +	struct korina_private *lp = netdev_priv(dev);
> +	unsigned long flags;
> +	u32 recognise = ETH_ARC_AB;	/* always accept broadcasts */
> +
> +	/* Set promiscuouts mode */

Typo: promiscuous.

> +	if ((dev->flags & IFF_ALLMULTI) || (dev->mc_count > 15))
> +		/* All multicast and broadcast */
> +		recognise |= ETH_ARC_AM;
> +	else if (dev->mc_count > 0)
> +		recognise |= ETH_ARC_AM;

Some stuff missing here? the mc_count > 15 checks seems superfluous
since >15 is always >0.

> +static struct net_device_stats *korina_get_stats(struct net_device *dev)
> +{
> +	return &dev->stats;
> +}

Unnecessary, internal_stats is default and does exactly this.

> +/*
> + *  * Restart the RC32434 ethernet controller.
> + *   */

Oddly formatted comment :)

> +static int korina_close(struct net_device *dev)
> +{
> +	struct korina_private *lp = netdev_priv(dev);
> +	u32 tmp;
> +
> +	/* Disable interrupts */
> +	disable_irq(lp->rx_irq);
> +	disable_irq(lp->tx_irq);
> +	disable_irq(lp->ovr_irq);
> +	disable_irq(lp->und_irq);
> +
> +	tmp = readl(&lp->tx_dma_regs->dmasm);
> +	tmp = tmp | DMA_STAT_FINI | DMA_STAT_ERR;
> +	writel(tmp, &lp->tx_dma_regs->dmasm);
> +
> +	tmp = readl(&lp->rx_dma_regs->dmasm);
> +	tmp = tmp | DMA_STAT_DONE | DMA_STAT_HALT | DMA_STAT_ERR;
> +	writel(tmp, &lp->rx_dma_regs->dmasm);
> +
> +	free_irq(lp->rx_irq, dev);
> +	free_irq(lp->tx_irq, dev);
> +	free_irq(lp->ovr_irq, dev);
> +	free_irq(lp->und_irq, dev);
> +	return 0;

No need to abort DMA?

johannes

Download attachment "signature.asc" of type "application/pgp-signature" (829 bytes)