linux-kernel - Re: [PATCH net-next-2.6 16/17] can: EG20T PCH: Fix incorrect return processing

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] [day] [month] [year] [list]

Date:	Fri, 19 Nov 2010 22:44:21 +0100
From:	Marc Kleine-Budde <mkl@...gutronix.de>
To:	Tomoya MORINAGA <tomoya-linux@....okisemi.com>
CC:	Wolfgang Grandegger <wg@...ndegger.com>,
	Wolfram Sang <w.sang@...gutronix.de>,
	Christian Pellegrin <chripell@...e.org>,
	Barry Song <21cnbao@...il.com>,
	Samuel Ortiz <sameo@...ux.intel.com>,
	socketcan-core@...ts.berlios.de, netdev@...r.kernel.org,
	linux-kernel@...r.kernel.org,
	"David S. Miller" <davem@...emloft.net>,
	andrew.chih.howe.khor@...el.com, qi.wang@...el.com,
	margie.foster@...el.com, yong.y.wang@...el.com,
	kok.howg.ewe@...el.com, joel.clark@...el.com
Subject: Re: [PATCH net-next-2.6 16/17] can: EG20T PCH: Fix incorrect return
 processing

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@....okisemi.com>
> ---
>  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   |


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