netdev - 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: <4CCAFA68.2030903@hartkopp.net>
Date:	Fri, 29 Oct 2010 18:46:32 +0200
From:	Oliver Hartkopp <socketcan@...tkopp.net>
To:	Marc Kleine-Budde <mkl@...gutronix.de>,
	Tomoya <tomoya-linux@....okisemi.com>
CC:	Wolfgang Grandegger <wg@...ndegger.com>,
	"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: Fix build
 warnings

On 29.10.2010 14:57, Marc Kleine-Budde wrote:
> Hello,
> 
> On 10/29/2010 12:37 PM, Tomoya wrote:

>>>> If you figured out how to use the endianess conversion functions from
>>>> the cpu_to_{le,be}-{le,to}_to_cpup family use them here, too.
> 
>> Sorry, I misunderstood the spec of Topcliff CAN endianess.
>> I have understood endianess conversion is not necessary.
>> (CAN data is set as Big-Endian in Topcliff CAN register)
> 
>>> You have to change the definition of the regs struct a bit:
>>>> 	u32 if1_mcont;
>>>> 	u32 if1_data[4];
>>>> 	u32 reserve2;
>> Uh, I can't find this. Where is this ?
> 
> Here's a patch to illustrate what I meant:
> 
> diff --git a/drivers/net/can/pch_can.c b/drivers/net/can/pch_can.c
> index 55ec324..5ee7589 100644
> --- a/drivers/net/can/pch_can.c
> +++ b/drivers/net/can/pch_can.c
> @@ -150,10 +150,7 @@ struct pch_can_regs {
>         u32 if1_id1;
>         u32 if1_id2;
>         u32 if1_mcont;
> -       u32 if1_dataa1;
> -       u32 if1_dataa2;
> -       u32 if1_datab1;
> -       u32 if1_datab2;
> +       u32 if1_data[4];
>         u32 reserve2;
>         u32 reserve3[12];
>         u32 if2_creq;
> 

Indeed the access to the data registers does not seem to be endian aware.

See in pch_can_rx_normal():

+			cf->can_dlc = get_can_dlc((ioread32(&priv->regs->
+						   if1_mcont)) & 0xF);
+			*(u16 *)(cf->data + 0) = ioread16(&priv->regs->
+							  if1_dataa1);
+			*(u16 *)(cf->data + 2) = ioread16(&priv->regs->
+							  if1_dataa2);
+			*(u16 *)(cf->data + 4) = ioread16(&priv->regs->
+							  if1_datab1);
+			*(u16 *)(cf->data + 6) = ioread16(&priv->regs->
+							  if1_datab2);

See in pch_xmit():

+	/* Copy data to register */
+	if (cf->can_dlc > 0) {
+		u32 data1 = *((u16 *)&cf->data[0]);
+		iowrite32(data1, &priv->regs->if2_dataa1);
+	}
+	if (cf->can_dlc > 2) {
+		u32 data1 = *((u16 *)&cf->data[2]);
+		iowrite32(data1, &priv->regs->if2_dataa2);
+	}
+	if (cf->can_dlc > 4) {
+		u32 data1 = *((u16 *)&cf->data[4]);
+		iowrite32(data1, &priv->regs->if2_datab1);
+	}
+	if (cf->can_dlc > 6) {
+		u32 data1 = *((u16 *)&cf->data[6]);
+		iowrite32(data1, &priv->regs->if2_datab2);
+	}
+

It's just a question if the driver for an Intel Chipset should/could ignore
the endian problematic.

If this is intended the driver should only appear in Kconfig depending on X86
or little endian systems.

Besides this remark, the struct pch_can_regs could also be defined in a way
that every single CAN payload data byte could be accessed directly to allow
e.g. this code in pch_can_rx_normal():

cf->data[0] = ioread8(&priv->regs->if1_data0);
cf->data[1] = ioread8(&priv->regs->if1_data1);
cf->data[2] = ioread8(&priv->regs->if1_data2);
(..)
cf->data[6] = ioread8(&priv->regs->if1_data6);
cf->data[7] = ioread8(&priv->regs->if1_data7);

This is easy to understand and additionally endian aware.

In opposite to this:

+	if (cf->can_dlc > 2) {
+		u32 data1 = *((u16 *)&cf->data[2]);
+		iowrite32(data1, &priv->regs->if2_dataa2);
+	}

Which puts a little? endian u16 into the big endian register layout.
Sorry i'm just getting curious on this register access implementation.

Regards,
Oliver
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html