netdev - Re: [PATCH] net: can: Remodel FlexCAN register read/write APIs for BE instances

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:	Wed, 25 Jun 2014 10:27:49 +0200
From:	Marc Kleine-Budde <mkl@...gutronix.de>
To:	Bhupesh Sharma <bhupesh.sharma@...escale.com>,
	linux-can@...r.kernel.org
CC:	wg@...ndegger.com, netdev@...r.kernel.org
Subject: Re: [PATCH] net: can: Remodel FlexCAN register read/write APIs for
 BE instances

On 06/24/2014 05:54 PM, Bhupesh Sharma wrote:
> The FlexCAN IP on certain SoCs like (Freescale's LS1021A) is
> modelled in a big-endian fashion, i.e. the registers and the
> message buffers are organized in a BE way.

Do you have any idea, why fsl did this? The messed up the network
controller on the mx28, too. :/

> More details about the LS1021A SoC can be seen here:
> http://www.freescale.com/webapp/sps/site/prod_summary.jsp?code=LS1021A&nodeId=018rH325E4017B#

Is there any "real" documentation for this SoC available?

> This patch ensures that the register read/write APIs are remodelled
> to address such cases, while ensuring that existing platforms (where
> the FlexCAN IP was modelled in LE way) do not break.

I'm not sure if it's better to handle this via the DT attributes
big-endian, little-endian, no attribute would mean native endianess for
backwards compatibility.

With this solution, you're breaking all ARM non DT boards, as the struct
platform_device_id still uses fsl_p1010_devtype_data. You're breaking DT
based mx35, as struct of_device_id has no entry for mx35.

With this patch fsl,p1010-flexcan isn't compatible with the imx/mxs any
more, please change the device trees in the kernel.

Please update the "FLEXCAN hardware feature flags" table in the driver
and check if any of the mentioned quirks are needed for the LS1021A.

See comment inline.....

> Signed-off-by: Bhupesh Sharma <bhupesh.sharma@...escale.com>
> ---
> Rebased againt v3.16-rc1
> 
>  drivers/net/can/flexcan.c |  213 +++++++++++++++++++++++++++------------------
>  1 file changed, 126 insertions(+), 87 deletions(-)
> 
> diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c
> index f425ec2..00c4740 100644
> --- a/drivers/net/can/flexcan.c
> +++ b/drivers/net/can/flexcan.c

[...]

>  static const struct can_bittiming_const flexcan_bittiming_const = {
> @@ -237,32 +256,36 @@ static const struct can_bittiming_const flexcan_bittiming_const = {
>  };
>  
>  /*
> - * Abstract off the read/write for arm versus ppc. This
> - * assumes that PPC uses big-endian registers and everything
> - * else uses little-endian registers, independent of CPU
> - * endianess.
> + * FlexCAN module is essentially modelled as a little-endian IP in most
> + * SoCs, i.e the registers as well as the message buffer areas are
> + * implemented in a little-endian fashion.
> + *
> + * However there are some SoCs (e.g. LS1021A) which implement the FlexCAN
> + * module in a big-endian fashion (i.e the registers as well as the
> + * message buffer areas are implemented in a big-endian way).
> + *
> + * In addition, the FlexCAN module can be found on SoCs having ARM or
> + * PPC cores. So, we need to abstract off the register read/write
> + * functions, ensuring that these cater to all the combinations of module
> + * endianess and underlying CPU endianess.
>   */
> -#if defined(CONFIG_PPC)
> -static inline u32 flexcan_read(void __iomem *addr)
> +static inline u32 flexcan_read(const struct flexcan_priv *priv,
> +			       void __iomem *addr)
>  {
> -	return in_be32(addr);
> -}
> -
> -static inline void flexcan_write(u32 val, void __iomem *addr)
> -{
> -	out_be32(addr, val);
> -}
> -#else
> -static inline u32 flexcan_read(void __iomem *addr)
> -{
> -	return readl(addr);
> +	if (priv->devtype_data->module_is_big_endian)
> +		return ioread32be(addr);
> +	else
> +		return ioread32(addr);
>  }
>  
> -static inline void flexcan_write(u32 val, void __iomem *addr)
> +static inline void flexcan_write(const struct flexcan_priv *priv,
> +				 u32 val, void __iomem *addr)
>  {
> -	writel(val, addr);
> +	if (priv->devtype_data->module_is_big_endian)

Please move the devtype_data into the struct flexcan_priv, so that you
don't need a double pointer dereference in the hot path.

> +		iowrite32be(val, addr);
> +	else
> +		iowrite32(val, addr);
>  }
> -#endif

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" (243 bytes)