linux-kernel - Re: [Patch v6 2/7] slimbus: Add messaging APIs to slimbus framework

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: <20171007122901.rimfp6nkusj5wwy2@latitude>
Date:   Sat, 7 Oct 2017 14:29:01 +0200
From:   Jonathan Neuschäfer <j.neuschaefer@....net>
To:     Srinivas Kandagatla <srinivas.kandagatla@...aro.org>
Cc:     Jonathan Neuschäfer <j.neuschaefer@....net>,
        gregkh@...uxfoundation.org, broonie@...nel.org,
        alsa-devel@...a-project.org, sdharia@...eaurora.org, bp@...e.de,
        poeschel@...onage.de, treding@...dia.com,
        gong.chen@...ux.intel.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,
        kheitke@...ience.com, linux-arm-msm@...r.kernel.org, arnd@...db.de
Subject: Re: [Patch v6 2/7] slimbus: Add messaging APIs to slimbus framework

On Sat, Oct 07, 2017 at 11:24:33AM +0100, Srinivas Kandagatla wrote:
> Thanks for the comments.
> 
> On 07/10/17 07:42, Jonathan Neuschäfer wrote:
> > Hi,
> > 
> > On Fri, Oct 06, 2017 at 05:51:31PM +0200, srinivas.kandagatla@...aro.org wrote:
> > > From: Sagar Dharia <sdharia@...eaurora.org>
[...]
> > > +int slim_xfer_msg(struct slim_controller *ctrl,
> > > +			struct slim_device *sbdev, struct slim_val_inf *msg,
> > > +			u8 mc)
> > > +{
> > > +	DEFINE_SLIM_LDEST_TXN(txn_stack, mc, 6, sbdev->laddr, msg);
> > > +	struct slim_msg_txn *txn = &txn_stack;
> > > +	int ret;
> > > +	u16 sl, cur;
> > > +
> > > +	ret = slim_val_inf_sanity(ctrl, msg, mc);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	sl = slim_slicesize(msg->num_bytes);
> > > +
> > > +	dev_dbg(&ctrl->dev, "SB xfer msg:os:%x, len:%d, MC:%x, sl:%x\n",
> > > +		msg->start_offset, msg->num_bytes, mc, sl);
> > > +
> > > +	cur = slim_slicecodefromsize(sl);
> > > +	txn->ec = ((sl | (1 << 3)) | ((msg->start_offset & 0xFFF) << 4));
> > 
> > Shouldn't this be (cur | (1 << 3)?

I misread the code here: I thought that cur was assigned the
"compressed" message length (in the range 0..7) here, but that's not
true, as slim_slicecodefromsize returns an "uncompressed" number. Thus
cur is a "quantized"[1] version of msg->num_bytes.

> cur seems to be redundant TBH, the only difference between cur and sl is
> that the slim_slicesize() can give slice size to program for any lengths
> between 1-16 bytes. However the slim_slicecodefromsize() can only handle
> 1,2,3,4, 6,8,12,16 byte sizes.

In any case, cur is only assigned and not used, as the code currently
is.

> So we can delete slim_slicecodefromsize() call and function together.
> looks like it was a leftover from downstream.

I agree. I don't know how it *might* be used, because I haven't read the
SLIMbus spec, but it is unused here.

> > (Also, what does cur mean? Cursor? Current?)
> No Idea!! :-) it is supposed to return slice size as per number of bytes.

Another problem solved by deleting slim_slicecodefromsize :-)

(As a small side-note, I think slim_slicesize and slim_slicecodefromsize
are named backwards: I would call sl, as used above, a "slice code",
because it encodes the message length)


> > > +/*
> > > + * slim_request_val_element: change and request a given value element
> 
> name should be fixed here..

Good catch.

> > > + * @sb: client handle requesting elemental message reads, writes.
> > > + * @msg: Input structure for start-offset, number of bytes to write.
> > > + * context: can sleep
> > > + * Returns:
> > > + * -EINVAL: Invalid parameters
> > > + * -ETIMEDOUT: If transmission of this message timed out (e.g. due to bus lines
> > > + *	not being clocked or driven by controller)
> > > + * -ENOTCONN: If the transmitted message was not ACKed by destination device.
> > 
> > Does rbuf contain the old value after this function finishes?
> > 
> Yep, device should send a reply value with the old value with matching tid.

I think you should document this in the comment to help readers.


Thanks,
Jonathan Neuschäfer

[1]: https://en.wikipedia.org/wiki/Quantization

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