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  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:   Thu, 23 Nov 2017 17:42:12 +0100
From:   Jonathan Neuschäfer <j.neuschaefer@....net>
To:     srinivas.kandagatla@...aro.org,
        Charles Keepax <ckeepax@...nsource.cirrus.com>
Cc:     gregkh@...uxfoundation.org, broonie@...nel.org,
        alsa-devel@...a-project.org, sdharia@...eaurora.org, bp@...e.de,
        poeschel@...onage.de, treding@...dia.com, andreas.noever@...il.com,
        alan@...ux.intel.com, mathieu.poirier@...aro.org, daniel@...ll.ch,
        jkosina@...e.cz, sharon.dvir1@...l.huji.ac.il, joe@...ches.com,
        davem@...emloft.net, james.hogan@...tec.com,
        michael.opdenacker@...e-electrons.com, robh+dt@...nel.org,
        pawel.moll@....com, mark.rutland@....com,
        devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-arm-msm@...r.kernel.org, vinod.koul@...el.com, arnd@...db.de
Subject: Re: [PATCH v7 11/13] slimbus: qcom: Add Qualcomm Slimbus controller
 driver

Hello Srinivas and Charles,

On Thu, Nov 23, 2017 at 10:07:03AM +0000, Charles Keepax wrote:
> On Wed, Nov 15, 2017 at 02:10:41PM +0000, srinivas.kandagatla@...aro.org wrote:
> > From: Sagar Dharia <sdharia@...eaurora.org>
> > 
> > This controller driver programs manager, interface, and framer
> > devices for Qualcomm's slimbus HW block.
> > Manager component currently implements logical address setting,
> > and messaging interface.
> > Interface device reports bus synchronization information, and framer
> > device clocks the bus from the time it's woken up, until clock-pause
> > is executed by the manager device.
> > 
> > Signed-off-by: Sagar Dharia <sdharia@...eaurora.org>
> > Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@...aro.org>
> > ---
[...]
> > +static void qcom_slim_rxwq(struct work_struct *work)
> > +{
> > +	u8 buf[SLIM_MSGQ_BUF_LEN];
> > +	u8 mc, mt, len;
> > +	int i, ret;
> > +	struct qcom_slim_ctrl *ctrl = container_of(work, struct qcom_slim_ctrl,
> > +						 wd);
> > +
> > +	while ((slim_get_current_rxbuf(ctrl, buf)) != -ENODATA) {
> > +		len = SLIM_HEADER_GET_RL(buf[0]);
> > +		mt = SLIM_HEADER_GET_MT(buf[0]);
> > +		mc = SLIM_HEADER_GET_MC(buf[1]);
> > +		if (mt == SLIM_MSG_MT_CORE &&
> > +			mc == SLIM_MSG_MC_REPORT_PRESENT) {
> > +			u8 laddr;
> > +			struct slim_eaddr ea;
> > +			u8 e_addr[6];
> > +
> > +			for (i = 0; i < 6; i++)
> > +				e_addr[i] = buf[7-i];
> > +
> > +			ea.manf_id = (u16)(e_addr[5] << 8) | e_addr[4];
> > +			ea.prod_code = (u16)(e_addr[3] << 8) | e_addr[2];
> > +			ea.dev_index = e_addr[1];
> > +			ea.instance = e_addr[0];
> 
> If we are just bitshifting this out of the bytes does it really
> make it much more clear to reverse the byte order first? Feels
> like you might as well shift it out of buf directly.

In any case, there is a predefined function to make this code a little
nicer in <asm/byteorder.h>:

	le16_to_cpu(x):  Converts the 16-bit little endian value x to
	                 CPU-endian
	le16_to_cpup(p): Converts the 16-bit little endian value pointed
	                 to by p to CPU-endian

If you use le16_to_cpup, you need to cast your pointer to __le16 *:

	ea.manf_id = le16_to_cpup((__le16 *)&e_addr[4]);

Like Charles, I don't quite see the point of the for loop that fills
e_addr. I guess it did effectively a byteswap, so the original code,
that assumed little-endian, could simply dereference a u16 *. This does
not make a lot of sense anymore, once you use properly (CPU-)endian-
independent code. (Of course, you'll need to replace le16 with be16 if
you drop that loop.)


Thanks,
Jonathan Neuschäfer

Download attachment "signature.asc" of type "application/pgp-signature" (820 bytes)

Powered by blists - more mailing lists