linux-kernel - Re: [PATCH net-next-2.6 v2] can: Topcliff: PCH

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]

Date:	Mon, 15 Nov 2010 15:21:30 +0100
From:	Wolfgang Grandegger <wg@...ndegger.com>
To:	Tomoya MORINAGA <tomoya-linux@....okisemi.com>
CC:	"David S. Miller" <davem@...emloft.net>,
	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, andrew.chih.howe.khor@...el.com,
	qi.wang@...el.com, margie.foster@...el.com, yong.y.wang@...el.com,
	Masayuki Ohtake <masa-korg@....okisemi.com>,
	kok.howg.ewe@...el.com, joel.clark@...el.com
Subject: Re: [PATCH net-next-2.6 v2] can: Topcliff: PCH_CAN driver: Add Flow
 control,

Hello,

On 11/15/2010 09:30 AM, Tomoya MORINAGA wrote:
>  * Add Flow control
>  * Fix Data copy issue (endianness)
>  * Add Macro prefix "PCH_"
>  * Separate interface register structure
>  * Some functions are unified.
>  * Change MessageObject indication(PCH_RX_OBJ_START, etc..)
>  * Enumerate LEC macro
>  * Move MSI processing from open/close to probe/remove processing
>  * Use BIT(x)
>  * and more...
> 
> Signed-off-by: Tomoya MORINAGA <tomoya-linux@....okisemi.com>
> ---
>  drivers/net/can/pch_can.c | 1348 ++++++++++++++++++++-------------------------
>  1 files changed, 595 insertions(+), 753 deletions(-)
> 
> diff --git a/drivers/net/can/pch_can.c b/drivers/net/can/pch_can.c
> index 6727182..6a38593 100644
> --- a/drivers/net/can/pch_can.c
> +++ b/drivers/net/can/pch_can.c
...
> -	if (status & PCH_LEC_ALL) {
> +	lec = status & PCH_LEC_ALL;
> +	switch (lec) {
> +	case PCH_STUF_ERR:
> +		cf->data[2] |= CAN_ERR_PROT_STUFF;
>  		priv->can.can_stats.bus_error++;
>  		stats->rx_errors++;
> -		switch (status & PCH_LEC_ALL) {
> -		case PCH_STUF_ERR:
> -			cf->data[2] |= CAN_ERR_PROT_STUFF;
> -			break;
> -		case PCH_FORM_ERR:
> -			cf->data[2] |= CAN_ERR_PROT_FORM;
> -			break;
> -		case PCH_ACK_ERR:
> -			cf->data[2] |= CAN_ERR_PROT_LOC_ACK |
> -				       CAN_ERR_PROT_LOC_ACK_DEL;
> -			break;
> -		case PCH_BIT1_ERR:
> -		case PCH_BIT0_ERR:
> -			cf->data[2] |= CAN_ERR_PROT_BIT;
> -			break;
> -		case PCH_CRC_ERR:
> -			cf->data[2] |= CAN_ERR_PROT_LOC_CRC_SEQ |
> -				       CAN_ERR_PROT_LOC_CRC_DEL;
> -			break;
> -		default:
> -			iowrite32(status | PCH_LEC_ALL, &priv->regs->stat);
> -			break;
> -		}
> -
> +		break;
> +	case PCH_FORM_ERR:
> +		cf->data[2] |= CAN_ERR_PROT_FORM;
> +		priv->can.can_stats.bus_error++;
> +		stats->rx_errors++;
> +		break;
> +	case PCH_ACK_ERR:
> +		cf->can_id |= CAN_ERR_ACK;
> +		priv->can.can_stats.bus_error++;
> +		stats->rx_errors++;
> +		break;
> +	case PCH_BIT1_ERR:
> +	case PCH_BIT0_ERR:
> +		cf->data[2] |= CAN_ERR_PROT_BIT;
> +		priv->can.can_stats.bus_error++;
> +		stats->rx_errors++;
> +		break;
> +	case PCH_CRC_ERR:
> +		cf->data[2] |= CAN_ERR_PROT_LOC_CRC_SEQ |
> +			       CAN_ERR_PROT_LOC_CRC_DEL;
> +		priv->can.can_stats.bus_error++;
> +		stats->rx_errors++;
> +		break;
> +	case PCH_LEC_ALL: /* Written by CPU. No error status */
> +		break;
>  	}

More comments to the lec handling below.

> +	cf->data[6] = ioread32(&priv->regs->errc) & PCH_TEC;
> +	cf->data[7] = (ioread32(&priv->regs->errc) & PCH_REC) >> 8;

Could be handle with just *one* register access.

...
>  static int pch_can_rx_poll(struct napi_struct *napi, int quota)
>  {
>  	struct net_device *ndev = napi->dev;
>  	struct pch_can_priv *priv = netdev_priv(ndev);
> -	struct net_device_stats *stats = &(priv->ndev->stats);
> -	u32 dlc;
>  	u32 int_stat;
>  	int rcv_pkts = 0;
>  	u32 reg_stat;
> -	unsigned long flags;
>  
>  	int_stat = pch_can_int_pending(priv);
>  	if (!int_stat)
> -		return 0;
> +		goto end;
>  
> -INT_STAT:
> -	if (int_stat == CAN_STATUS_INT) {
> +	if ((int_stat == PCH_STATUS_INT) && (quota > 0)) {
>  		reg_stat = ioread32(&priv->regs->stat);
>  		if (reg_stat & (PCH_BUS_OFF | PCH_LEC_ALL)) {
> -			if ((reg_stat & PCH_LEC_ALL) != PCH_LEC_ALL)
> +			if ((reg_stat & PCH_LEC_ALL) != PCH_LEC_ALL) {
>  				pch_can_error(ndev, reg_stat);
> +				quota--;
> +			}

Should be:

  		if (reg_stat & PCH_BUS_OFF ||
		    (reg_stat & PCH_LEC_ALL) != PCH_LEC_ALL) {

Your lec handling is still not correc, I believe. The driver needs to
write PCH_LEC_ALL to the "stat" register once in the initialization code
and then after each error observed (lec != PCH_LEC_ALL). I still do not
find such code. Could you show us the output of

  "# candump any,0:0,#FFFFFFFF"

when yo send CAN messages *without* a cable connected?.

Thanks,

Wolfgang.
--
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/