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:   Tue, 23 Nov 2021 13:48:27 +0100
From:   Lukas Wunner <lukas@...ner.de>
To:     "LH.Kuo" <lhjeff911@...il.com>
Cc:     p.zabel@...gutronix.de, broonie@...nel.org, robh+dt@...nel.org,
        linux-spi@...r.kernel.org, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org, dvorkin@...bo.com,
        qinjian@...lus1.com, wells.lu@...plus.com,
        "LH.Kuo" <lh.kuo@...plus.com>
Subject: Re: [PATCH v3 1/2] SPI: Add SPI driver for Sunplus SP7021

On Mon, Nov 22, 2021 at 10:33:32AM +0800, LH.Kuo wrote:
> +static int sp7021_spi_controller_probe(struct platform_device *pdev)
> +{
[...]
> +	ret = devm_spi_register_controller(&pdev->dev, ctlr);
> +	if (ret != 0) {
> +		dev_err(&pdev->dev, "spi_register_master fail\n");
> +		goto disable_runtime_pm;
> +	}

You need to use spi_register_controller() here (*not* the devm_ variant)
because you're using spi_unregister_controller() in
sp7021_spi_controller_remove().

> +
> +	// clk
> +	pspim->spi_clk = devm_clk_get(&pdev->dev, NULL);
> +	if (IS_ERR(pspim->spi_clk)) {
> +		return dev_err_probe(&pdev->dev, PTR_ERR(pspim->spi_clk),
> +				     "devm_clk_get fail\n");
> +	}
> +
> +	// reset
> +	pspim->rstc = devm_reset_control_get_exclusive(&pdev->dev, NULL);
> +	dev_dbg(&pdev->dev, "pspim->rstc : 0x%x\n", (unsigned int)pspim->rstc);
> +	if (IS_ERR(pspim->rstc)) {
> +		return dev_err_probe(&pdev->dev, PTR_ERR(pspim->rstc),
> +				     "devm_rst_get fail\n");
> +	}
> +
> +	ret = clk_prepare_enable(pspim->spi_clk);
> +	if (ret)
> +		return dev_err_probe(&pdev->dev, ret,
> +			"failed to enable clk\n");
> +
> +	ret = reset_control_deassert(pspim->rstc);
> +	if (ret) {
> +		dev_err_probe(&pdev->dev, ret,
> +			"failed to deassert reset\n");
> +		goto free_reset_assert;
> +
> +	}

You need to move the above steps *before* the call to
spi_register_controller().  Once spi_register_controller() returns,
it must be able to perform SPI transactions.  So you have to enable
all required clocks before calling it.  You also have to perform the
reset step before registration to avoid interfering with an ongoing
transaction.  The order of these steps must mirror the order in
sp7021_spi_controller_remove():  There you're unregistering the
controller *before* disabling the clock and asserting reset,
so the order must be inverted here.


> +static int sp7021_spi_controller_remove(struct platform_device *pdev)
> +{
> +	struct spi_controller *ctlr = dev_get_drvdata(&pdev->dev);
> +	struct sp7021_spi_ctlr *pspim = spi_master_get_devdata(ctlr);
> +
> +	pm_runtime_disable(&pdev->dev);
> +	pm_runtime_set_suspended(&pdev->dev);
> +
> +	spi_unregister_controller(pspim->ctlr);
> +	clk_disable_unprepare(pspim->spi_clk);
> +	reset_control_assert(pspim->rstc);
> +
> +	return 0;
> +}

I think the two calls to pm_runtime_* should be moved after
spi_unregister_controller() but that's probably not critical.

Thanks,

Lukas

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ