linux-kernel - Re: [PATCH] cpsw: Fix interrupt storm among other things

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: <20130128182451.GC2445@netboy.at.omicron.at>
Date:	Mon, 28 Jan 2013 19:24:51 +0100
From:	Richard Cochran <richardcochran@...il.com>
To:	Pantelis Antoniou <panto@...oniou-consulting.com>
Cc:	Mugunthan V N <mugunthanvnm@...com>, Matt Porter <mporter@...com>,
	Chase Maupin <chase.maupin@...com>, Jason Kridner <jdk@...com>,
	Tony Lindgren <tony@...mide.com>, linux-omap@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] cpsw: Fix interrupt storm among other things

On Mon, Jan 28, 2013 at 03:11:08PM +0200, Pantelis Antoniou wrote:
> Fix interrupt storm on bone A4 cause by non-by-the-book interrupt handling.
> While at it, added a non-NAPI mode (which is easier to debug), plus
> some general fixes.

I have a few issues with this patch:

1. This is a networking patch. It should be addressed to netdev, it it
   needs to have davem on CC.

2. The description is poor. You need to tell us more about this
   "storm". How can one trigger it? What is the effect? Does the
   system lock up, or is the throughput poor? Tell us exactly what the
   problem is. Tell us what is wrong in the interrupt handling, and
   how the patch improves the situation.

3. Don't just say "general fixes", but do say exactly what you fixed.

4. Adding non-NAPI code is going backwards. Don't do that (and see the
   recent discussion on netdev on just this very topic: Frank Li and
   the fec driver).

> diff --git a/drivers/net/ethernet/ti/cpsw.c b/drivers/net/ethernet/ti/cpsw.c
> index 40aff68..b6ca4af 100644
> --- a/drivers/net/ethernet/ti/cpsw.c
> +++ b/drivers/net/ethernet/ti/cpsw.c
> @@ -148,10 +148,37 @@ struct cpsw_wr_regs {
>  	u32	soft_reset;
>  	u32	control;
>  	u32	int_control;
> -	u32	rx_thresh_en;
> -	u32	rx_en;
> -	u32	tx_en;
> -	u32	misc_en;
> +	u32	c0_rx_thresh_en;
> +	u32	c0_rx_en;
> +	u32	c0_tx_en;
> +	u32	c0_misc_en;

How does renaming these help?

(If you really think that new names are needed, then put the cosmetic
renaming changes into its a separate patch.)

> +	u32	c1_rx_thresh_en;
> +	u32	c1_rx_en;
> +	u32	c1_tx_en;
> +	u32	c1_misc_en;

You added a bunch of new fields, but you don't use any of them.

Thanks,
Richard
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/