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

[Richard Cochran <richardcochran@gmail.com>]

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