netdev - Re: [PATCH]NET/KS8695: add support NAPI for Rx

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: <20091026162721.GD9480@digital-scurf.org>
Date:	Mon, 26 Oct 2009 16:27:21 +0000
From:	Daniel Silverstone <dsilvers@...tec.co.uk>
To:	"Figo.zhang" <figo1802@...il.com>
Cc:	"David S. Miller" <davem@...emloft.net>, netdev@...r.kernel.org,
	Vincent Sanders <vince@...tec.co.uk>,
	Ben Dooks <ben@...tec.co.uk>
Subject: Re: [PATCH]NET/KS8695: add support NAPI for Rx

On Tue, Oct 27, 2009 at 12:00:28AM +0800, Figo.zhang wrote:
> +#ifdef KS8695NET_NAPI
> +static irqreturn_t
> +ks8695_rx_irq(int irq, void *dev_id)

This routine lacks its documentation comment.  This driver is fully documented
in order to serve as a good example for others.  Indeed this lack of
documentation comments continues through your patch, I won't bring up each
instance, instead trusting you to go back over your patch and sort them out.

> +	status = __raw_readl(KS8695_IRQ_VA + KS8695_INTST);
[snip]
> +	__raw_writel(status | mask_bit , KS8695_IRQ_VA + KS8695_INTST);
[snip]
> +			__raw_writel(status , KS8695_IRQ_VA + KS8695_INTEN);
[snip]
> +	unsigned long isr = __raw_readl(KS8695_IRQ_VA + KS8695_INTEN);
[snip]
> +		__raw_writel(isr | mask_bit, KS8695_IRQ_VA + KS8695_INTEN);

Please don't use __raw_readl or __raw_writel.  This driver was nice and clean,
don't ruin it.

Also, as an aside, you seem to add a spinlock (rx_lock) which afaict is only
used by NAPI related routines, and yet you include it regardless of NAPI being
enabled or not.  Did I misread your patch, or is this an oversight?

Regards,

Daniel.

-- 
Daniel Silverstone                              http://www.simtec.co.uk/

--
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