linux-kernel - Re: [PATCH 2/3] spi: meson: Add support for Amlogic Meson SPIFC

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:	Mon, 10 Nov 2014 15:11:40 +0000
From:	Mark Brown <broonie@...nel.org>
To:	Beniamino Galvani <b.galvani@...il.com>
Cc:	linux-spi@...r.kernel.org, Carlo Caione <carlo@...one.org>,
	linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
	devicetree@...r.kernel.org, Rob Herring <robh+dt@...nel.org>,
	Pawel Moll <pawel.moll@....com>,
	Mark Rutland <mark.rutland@....com>,
	Ian Campbell <ijc+devicetree@...lion.org.uk>,
	Kumar Gala <galak@...eaurora.org>,
	Jerry Cao <jerry.cao@...ogic.com>,
	Victor Wan <victor.wan@...ogic.com>
Subject: Re: [PATCH 2/3] spi: meson: Add support for Amlogic Meson SPIFC

On Sun, Nov 09, 2014 at 11:56:50PM +0100, Beniamino Galvani wrote:
> On Sun, Nov 09, 2014 at 10:17:12AM +0000, Mark Brown wrote:

> > This will busy wait for up to a second, that seems like a long time to
> > busy wait.  We also appear to be using this for the entire duration of
> > the transfer which could be a fairly long time even during normal
> > operation if doing a large transfer such as a firmware download, or if
> > the bus speed is low.

> Yes, probably the timeout value is too long since the maximum length
> of a basic transfer is 64 bytes. Can you suggest a reasonable value?

10ms?  It depends somewhat 

> > > +	while (done < xfer->len && !ret) {
> > > +		len = min_t(int, xfer->len - done, SPIFC_BUFFER_SIZE);
> > > +		ret = meson_spifc_txrx(spifc, xfer, done, len,
> > > +				       last_xfer, done + len >= xfer->len);
> > > +		done += len;
> > > +	}

> > I noticed that the handling of /CS was done in the spifc_txrx() function
> > - will this do the right thing if the transfer needs to be split for the
> > buffer size?

> It should. When the transfer gets split, CS is kept active for all the
> chunks and the value of CS after that depends on the value of
> cs_change.

Can you be more specific about how that works?  I'm just not seeing the
code that handles this.

> > > +	if (!ret && xfer->delay_usecs)
> > > +		udelay(xfer->delay_usecs);

> > The core will do this for you if you implement this as transfer_one().

> Please correct me if I'm wrong, but I think that transfer_one() can't
> be used in this case. The hardware doesn't support direct manipulation
> of CS and allows only to specify if CS must be kept active after the
> current transfer. So I need to know for each transfer if it's the last
> and this can be achieved only implementing transfer_one_message().

This is already in a function that's operating at the transfer_one()
level, the function is even called transfer_one() and besides it's
clearly not something specific to this hardware so should be factored
out into the core instead of open coded.

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