Re: [PATCH net-next-2.6 16/17] can: EG20T PCH: Fix incorrect returnprocessing

[Marc Kleine-Budde]

On 11/19/2010 06:51 AM, Tomoya MORINAGA wrote:
> Fix incorrect return processing

The description is correct. But you change several things that have
nothing to do. The frist hunk fixes shared irq handlers, the others the
quota calculation in the napi handler.

Marc

> 
> Signed-off-by: Tomoya MORINAGA <tomoya-linux@xxxxxxxxxxxxxxx>
> ---
>  drivers/net/can/pch_can.c |   20 ++++++++++++--------
>  1 files changed, 12 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/net/can/pch_can.c b/drivers/net/can/pch_can.c
> index c612a99..48f4a2e 100644
> --- a/drivers/net/can/pch_can.c
> +++ b/drivers/net/can/pch_can.c
> @@ -589,10 +589,12 @@ static irqreturn_t pch_can_interrupt(int irq, void *dev_id)
>  	struct net_device *ndev = (struct net_device *)dev_id;
>  	struct pch_can_priv *priv = netdev_priv(ndev);
>  
> -	pch_can_set_int_enables(priv, PCH_CAN_NONE);
> -	napi_schedule(&priv->napi);
> -
> -	return IRQ_HANDLED;
> +	if ((pch_can_int_pending(priv) > 0) && (dev_id != NULL)) {

dev_id is always != NULL, because you registered your IRQ handler with
it. (BTW: dev_id has already been dereferenced in netdev_priv(), so if
this code is executed, dev_if is != NULL)

Just write:

if (!pch_can_int_pending(priv))
	return IRQ_NONE;

> +		pch_can_set_int_enables(priv, PCH_CAN_NONE);
> +		napi_schedule(&priv->napi);
> +		return IRQ_HANDLED;
> +	}
> +	return IRQ_NONE;
>  }
>  
>  static void pch_fifo_thresh(struct pch_can_priv *priv, int obj_id)
> @@ -674,7 +676,7 @@ static int pch_can_rx_normal(struct net_device *ndev, u32 obj_num, int quota)
>  		if (reg & PCH_IF_MCONT_MSGLOST) {
>  			rtn = pch_can_rx_msg_lost(ndev, obj_num);
>  			if (!rtn)
> -				return rtn;
> +				return rcv_pkts;
>  			rcv_pkts++;
>  			quota--;
>  			obj_num++;
> @@ -777,10 +779,12 @@ static int pch_can_poll(struct napi_struct *napi, int quota)
>  		goto end;
>  
>  	if ((int_stat >= PCH_RX_OBJ_START) && (int_stat <= PCH_RX_OBJ_END)) {
> -		rcv_pkts += pch_can_rx_normal(ndev, int_stat, quota);
> -		quota -= rcv_pkts;
> -		if (quota < 0)
> +		rcv_pkts = pch_can_rx_normal(ndev, int_stat, quota);

maybe it's better to rx as much packages in rx_normal as possible and
not return if you failed to alloc a can_frame.

> +		if (rcv_pkts < 0) {
> +			rcv_pkts = 0;
>  			goto end;
> +		}
> +		quota -= rcv_pkts;
>  	} else if ((int_stat >= PCH_TX_OBJ_START) &&
>  		   (int_stat <= PCH_TX_OBJ_END)) {
>  		/* Handle transmission interrupt */


-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |

Attachment: signature.asc
Description: OpenPGP digital signature