linux-kernel - Re: [PATCH v3 1/2] spi: Add MXIC controller driver

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] [day] [month] [year] [list]

Message-ID: <20181001153501.GD19099@sirena.org.uk>
Date:   Mon, 1 Oct 2018 16:35:01 +0100
From:   Mark Brown <broonie@...nel.org>
To:     masonccyang@...c.com.tw
Cc:     tpiepho@...inj.com, linux-kernel@...r.kernel.org,
        linux-spi@...r.kernel.org, boris.brezillon@...tlin.com,
        juliensu@...c.com.tw, zhengxunli@...c.com.tw
Subject: Re: [PATCH v3 1/2] spi: Add MXIC controller driver

On Fri, Sep 28, 2018 at 03:35:11PM +0800, masonccyang@...c.com.tw wrote:

This looks mostly good, a couple of fairly small things:

> +static int mxic_spi_clk_enable(struct mxic_spi *mxic)
> +{
> +	int ret;
> +
> +	ret = clk_prepare_enable(mxic->send_clk);
> +	if (ret)
> +		goto err_send_clk;
> +
> +	ret = clk_prepare_enable(mxic->send_dly_clk);
> +	if (ret)
> +		goto err_send_dly_clk;
> +
> +	return ret;
> +
> +err_send_dly_clk:
> +	clk_disable_unprepare(mxic->send_dly_clk);
> +err_send_clk:
> +	clk_disable_unprepare(mxic->send_clk);

You should only undo operations that succeeded in the error path, if
something failed you don't need to revert it.

> +static int mxic_spi_setup(struct spi_device *spi)
> +{
> +	struct mxic_spi *mxic = spi_master_get_devdata(spi->master);
> +	unsigned long freq = spi->max_speed_hz;
> +	int ret;
> +
> +	ret = clk_set_rate(mxic->send_clk, freq);
> +	if (ret)
> +		return ret;
> +
> +	ret = clk_set_rate(mxic->send_dly_clk, freq);
> +	if (ret)
> +		return ret;
> +
> +	/*
> +	 * A constant delay range from 0x0 ~ 0x1F for input delay,
> +	 * the unit is 78 ps, the max input delay is 2.418 ns.
> +	 */
> +	mxic_spi_set_input_delay_dqs(mxic, 0xf);

This is changing the hardware state for all slaves, not just the device
being configured - setup() isn't supposed to do that (yes, it's a
confusing API but unfortunately a legacy one that'll be a pain to clean
up).  prepare_transfer_hardware() is usually a better place to do this.

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