linux-kernel - Re: [PATCH 2/2] spi: Add Qualcomm QUP SPI controller support

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: <20140210170900.GC13533@sirena.org.uk>
Date:	Mon, 10 Feb 2014 17:09:00 +0000
From:	Mark Brown <broonie@...nel.org>
To:	"Ivan T. Ivanov" <iivanov@...sol.com>
Cc:	Grant Likely <grant.likely@...aro.org>,
	Rob Herring <robh+dt@...nel.org>, linux-spi@...r.kernel.org,
	linux-arm-msm@...r.kernel.org, linux-kernel@...r.kernel.org,
	devicetree@...r.kernel.org, Alok Chauhan <alokc@...eaurora.org>,
	Gilad Avidov <gavidov@...eaurora.org>,
	Kiran Gunda <kgunda@...eaurora.org>,
	Sagar Dharia <sdharia@...eaurora.org>
Subject: Re: [PATCH 2/2] spi: Add Qualcomm QUP SPI controller support

On Mon, Feb 10, 2014 at 06:29:26PM +0200, Ivan T. Ivanov wrote:
> On Fri, 2014-02-07 at 17:12 +0000, Mark Brown wrote: 

> > > +	if (!xfer)
> > > +		return IRQ_HANDLED;

> > Are you sure?  It seems wrong to just ignore interrupts, some comments
> > would help explain why.

> Of course this should never happen. This was left from initial stage
> of the implementation.

OK, so reporting them as errors seems better then.

> > > +	if (controller->speed_hz != chip->speed_hz) {
> > > +		ret = clk_set_rate(controller->cclk, chip->speed_hz);
> > > +		if (ret) {
> > > +			dev_err(controller->dev, "fail to set frequency %d",
> > > +				chip->speed_hz);
> > > +			return -EIO;
> > > +		}
> > > +	}

> > Is calling into the clock framework really so expensive that we need to
> > avoid doing it?  

> Probably not. But why to call it if the frequency is the same.

It's less code that could go wrong and the check is already there in the
clock framework hopefully.

> > You also shouldn't be interacting with the hardware in
> > setup().

> This is internal hw-setup, not master::setup() or I am
> missing something?

The naming could probably be clearer then - config or something.

> > > +	if (chip->bits_per_word <= 8)
> > > +		controller->bytes_per_word = 1;
> > > +	else if (chip->bits_per_word <= 16)
> > > +		controller->bytes_per_word = 2;
> > > +	else
> > > +		controller->bytes_per_word = 4;

> > This looks like a switch statement, and looking at the above it's not
> > clear that the device actually supports anything other than whole bytes.
> > I'm not sure what that would mean from an API point of view.

> SPI API didn't validate struct spi_transfer::len field.

It's supposed to; if the validation is incomplete then that should be
fixed.

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