netdev - RE: [PATCH V2 1/1] 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:	Fri, 4 Jul 2014 13:47:59 +0000
From:	"bhupesh.sharma@...escale.com" <bhupesh.sharma@...escale.com>
To:	Marc Kleine-Budde <mkl@...gutronix.de>,
	"linux-can@...r.kernel.org" <linux-can@...r.kernel.org>
CC:	"wg@...ndegger.com" <wg@...ndegger.com>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: RE: [PATCH V2 1/1] net: can: Remodel FlexCAN register read/write APIs
 for BE instances

Hi Marc,

Thanks for your review.

> -----Original Message-----
> From: Marc Kleine-Budde [mailto:mkl@...gutronix.de]
> Sent: Friday, July 04, 2014 7:11 PM
> To: Sharma Bhupesh-B45370; linux-can@...r.kernel.org
> Cc: wg@...ndegger.com; netdev@...r.kernel.org
> Subject: Re: [PATCH V2 1/1] net: can: Remodel FlexCAN register read/write
> APIs for BE instances
> 
> On 07/04/2014 03:01 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.
> >
> > More details about the LS1021A SoC can be seen here:
> > http://www.freescale.com/webapp/sps/site/prod_summary.jsp?code=LS1021A
> > &nodeId=018rH325E4017B#
> >
> > 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.
> >
> > Tested on LS1021A-QDS board.
> >
> > Signed-off-by: Bhupesh Sharma <bhupesh.sharma@...escale.com>
> > ---
> > Changes since v1:
> > - Addressed Marc's review comments.
> > - Also tried on ARM big-endian kernel
> >
> > Rebased against v3.16-rc2
> 
> Please use net-next or linux-can-next, but I think this makes no
> difference here.
> 
> >  drivers/net/can/flexcan.c |  192
> > +++++++++++++++++++++++++++------------------
> >  1 file changed, 114 insertions(+), 78 deletions(-)
> 
> I'm missing the DT documentation update.

Yes. I just wanted to get some early comments on this.

> [...]
> 
> 		of_property_read_u32(pdev->dev.of_node,
> > @@ -1149,6 +1166,25 @@ static int flexcan_probe(struct platform_device
> *pdev)
> >  	dev->flags |= IFF_ECHO;
> >
> >  	priv = netdev_priv(dev);
> > +
> > +	if (IS_ENABLED(CONFIG_CPU_BIG_ENDIAN))
> > +		core_is_little = false;
> > +
> > +	if (of_property_read_bool(dev->dev.of_node, "big-endian"))
> > +		module_is_little = false;
> > +
> > +	if ((core_is_little && module_is_little) ||
> > +	    (!core_is_little && !module_is_little)) {
> 
> I think this is broken on PPC, where both core and module are BE. Please
> assume native endianess an default, if neither big-endian nor little-
> endian is present.

On PPC platforms (which are BE) the IP is essentially LE (I verified this on P1010 RDB).

If both are BE, then we need no swap operations, right? Or am I missing something.

> 
> > +		priv->read = flexcan_read_le;
> > +		priv->write = flexcan_write_le;
> > +	}
> > +
> > +	if ((!core_is_little && module_is_little) ||
> > +	    (core_is_little && !module_is_little)) {
> > +		priv->read = flexcan_read_be;
> > +		priv->write = flexcan_write_be;
> > +	}
> > +
> >  	priv->can.clock.freq = clock_freq;
> >  	priv->can.bittiming_const = &flexcan_bittiming_const;
> >  	priv->can.do_set_mode = flexcan_set_mode;
> >
> 


Regards,
Bhupesh