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]

Message-ID: <4CD18AAE.9040300@grandegger.com>
Date:	Wed, 03 Nov 2010 17:15:42 +0100
From:	Wolfgang Grandegger <wg@...ndegger.com>
To:	Marc Kleine-Budde <mkl@...gutronix.de>
CC:	andrew.chih.howe.khor@...el.com,
	Masayuki Ohtake <masa-korg@....okisemi.com>,
	Samuel Ortiz <sameo@...ux.intel.com>, margie.foster@...el.com,
	netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
	socketcan-core@...ts.berlios.de, yong.y.wang@...el.com,
	kok.howg.ewe@...el.com, joel.clark@...el.com,
	"David S. Miller" <davem@...emloft.net>,
	Christian Pellegrin <chripell@...e.org>, qi.wang@...el.com
Subject: Re: [PATCH net-next-2.6 v2] can: Topcliff: PCH_CAN driver: Fix build
 warnings

On 11/01/2010 12:05 PM, Marc Kleine-Budde wrote:
> Hello,
> 
> On 10/29/2010 09:32 PM, Wolfgang Grandegger wrote:
> 
> some comments inline.
> 
> [...]
...
>>>> +	if (status & PCH_LEC_ALL) {
>>>> +		priv->can.can_stats.bus_error++;
>>>> +		stats->rx_errors++;
>>>> +		switch (status & PCH_LEC_ALL) {
>>>
>>> I suggest to convert to a if-bit-set because there might be more than
>>> one bit set.
>>
>> Marc, what do you mean here. It's an enumeraton. Maybe the following
>> code is more clear:
> 
> Oh, I haven't looked this one up in the datasheet.
>>
>> 	lec = status & PCH_LEC_ALL;
>> 	if (lec > 0) {
> 
> or "if (lec)", YMMV
> 
>> 		switch (lec) {
>>
>>>> +		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;
>>
>> Could you check what that type of bus error that is? Usually it's a ack
>> lost error.
>>
>>>> +			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;
>>>> +		}
>>>> +
>>>> +	}

At a closer look, I doubt that the above LEC handling is correct.
According to the manual: "when the LEC shows the value “7,” this value
was written to the LEC by the CPU; thus, no CAN bus event was detected"
Therefore the LEC bits should be properly *initialized* and *reset* to
0x7, which I do not find in the code.

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/