linux-kernel - Re: [PATCH v7][2/4] mmc: Add Synopsys DesignWare mmc cmdq host 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] [thread-next>] [day] [month] [year] [list]

Message-ID: <23011695aafca595c3c8722fda2a8e194c5318df.camel@pengutronix.de>
Date:   Mon, 27 Nov 2023 13:51:19 +0100
From:   Philipp Zabel <p.zabel@...gutronix.de>
To:     Jyan Chou <jyanchou@...ltek.com>, ulf.hansson@...aro.org,
        adrian.hunter@...el.com, jh80.chung@...sung.com,
        riteshh@...eaurora.org, robh+dt@...nel.org,
        krzysztof.kozlowski+dt@...aro.org
Cc:     conor+dt@...nel.org, asutoshd@...eaurora.org,
        linux-mmc@...r.kernel.org, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org, arnd@...db.de,
        briannorris@...omium.org, doug@...morgal.com,
        tonyhuang.sunplus@...il.com, abel.vesa@...aro.org,
        william.qiu@...rfivetech.com
Subject: Re: [PATCH v7][2/4] mmc: Add Synopsys DesignWare mmc cmdq host
 driver

Hi,

On Di, 2023-11-21 at 17:10 +0800, Jyan Chou wrote:
> We implemented cmdq feature on Synopsys DesignWare mmc driver.
> The difference between dw_mmc.c and dw_mmc_cqe.c were distinct
> register definitions, mmc user flow and the addition of cmdq.
> 
> New version of User Guide had modify mmc driver's usage flow,
> we may need to renew code to precisely follow user guide.
> 
> More over, We added a wait status function to satisfy synopsys
> user guide's description, since this flow might be specific in
> synopsys host driver only.
> 
> Signed-off-by: Jyan Chou <jyanchou@...ltek.com>
> 
> —--
[...]
> diff --git a/drivers/mmc/host/dw_mmc_cqe.c b/drivers/mmc/host/dw_mmc_cqe.c
> new file mode 100644
> index 000000000000..eb00d6a474b2
> --- /dev/null
> +++ b/drivers/mmc/host/dw_mmc_cqe.c
> @@ -0,0 +1,1467 @@
[...]
> +#ifdef CONFIG_OF
> +static struct dw_mci_board *dw_mci_cqe_parse_dt(struct dw_mci *host)
> +{
> +	struct dw_mci_board *pdata;
> +	struct device *dev = host->dev;
> +	const struct dw_mci_drv_data *drv_data = host->drv_data;
> +	int ret;
> +
> +	pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
> +	if (!pdata)
> +		return ERR_PTR(-ENOMEM);
> +
> +	pdata->rstc = devm_reset_control_get_optional_exclusive(dev, NULL);
> +	if (IS_ERR(pdata->rstc)) {
> +		if (PTR_ERR(pdata->rstc) == -EPROBE_DEFER)
> +			return ERR_PTR(-EPROBE_DEFER);

This should

		return ERR_CAST(pdata->rstc);

instead.

There is no reason to hide device tree parsing errors here, and I'd
argue pdata should not be returned with rstc set to an error value.
devm_reset_control_get_optional_exclusive() returns NULL if there are
no errors and no reset is specified in the device tree.

Then you can just use dev_err_probe() at the call site in
dw_mci_cqe_probe().


[...]
> +int dw_mci_cqe_probe(struct dw_mci *host)
> +{
[...]
> +	if (!IS_ERR(host->pdata->rstc)) {
> +		reset_control_assert(host->pdata->rstc);
> +		usleep_range(10, 50);
> +		reset_control_deassert(host->pdata->rstc);
> +	}

This should be changed to

	if (host->pdata->rstc) {
		reset_control_assert(host->pdata->rstc);
		usleep_range(10, 50);
		reset_control_deassert(host->pdata->rstc);
	}

[...]
> +	return 0;
> +
> +err_dmaunmap:
> +	if (!IS_ERR(host->pdata->rstc))
> +		reset_control_assert(host->pdata->rstc);

This should be just

	reset_control_assert(host->pdata->rstc);

as reset_control_assert() is a no-op if host->pdata->rstc == NULL.

[...]
> +void dw_mci_cqe_remove(struct dw_mci *host)
> +{
[...]
> +	if (!IS_ERR(host->pdata->rstc))
> +		reset_control_assert(host->pdata->rstc);

	reset_control_assert(host->pdata->rstc);


regards
Philipp