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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Fri, 29 Oct 2010 19:58:23 +0200
From:	Marc Kleine-Budde <mkl@...gutronix.de>
To:	Oliver Hartkopp <socketcan@...tkopp.net>
CC:	Tomoya <tomoya-linux@....okisemi.com>,
	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 10/29/2010 06:46 PM, Oliver Hartkopp wrote:
> 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

That _should_ work on x86. On the contrary on ARM some registers in SOCs
allow only full 32 bit access.

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

or use this, (as proposed in a earlier mail)

	for (i = 0; i < cf->can_dlc; i += 2) {
		reg = ioread32(&priv->regs->if1_data[i >> 1]);
		*(__be16 *)cf->data[i] = cpu_to_be16(reg);
	}

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

According to the datasheet the layout is LE.

cheers, Marc

-- 
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)