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]

Message-ID: <20141109101712.GM2722@sirena.org.uk>
Date:	Sun, 9 Nov 2014 10:17:12 +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 10:25:12AM +0100, Beniamino Galvani wrote:

> +static int meson_spifc_wait_ready(struct meson_spifc *spifc)
> +{
> +	unsigned long deadline = jiffies + msecs_to_jiffies(1000);
> +	u32 data;
> +
> +	do {
> +		regmap_read(spifc->regmap, REG_SLAVE, &data);
> +		if (data & SLAVE_TRST_DONE)
> +			return 0;
> +		cond_resched();
> +	} while (time_before(jiffies, deadline));

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.

> +	meson_spifc_setup_speed(spifc, xfer->speed_hz ? xfer->speed_hz :
> +				spi->max_speed_hz);
> +

Please avoid the ternery operator, it does nothing for legibility and in
this case it's not needed as the core will always ensure that there is a
per-transfer speed set.

> +	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?

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

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

> +static int meson_spifc_transfer_one_message(struct spi_master *master,
> +					    struct spi_message *msg)

This appears to do nothing that the core won't do - just implement
transfer_one() and remove this.

> +	spifc = spi_master_get_devdata(master);
> +	memset(spifc, 0, sizeof(struct meson_spifc));

There should be no need for this memset.

> +	spifc_regmap_config.max_register = resource_size(res) - 4;
> +	spifc_regmap_config.name = "amlogic,meson-spifc";

If you're dynamically initializing the structure you need to work with a
copy of it rather than directly since there may be multiple instances.
I'm not seeing a reason to override the regmap name here, this is only
really intended for devices with multiple register maps.

> +	ret = clk_prepare_enable(spifc->clk);
> +	if (ret) {
> +		dev_err(spifc->dev, "can't prepare clock\n");
> +		goto out_err;
> +	}

You should really implement runtime PM operations to disable this when
not in use and use auto_runtime_pm to make sure this happens.

> +	master->bus_num = pdev->id;

Leave this blank for DT only devices (and for non-DT devices this won't
work if you get two different buses).

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