| 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
| ||
|
Message-ID: <24b03128ab594363a29494c0171e9f5f@realtek.com>
Date: Tue, 28 Nov 2023 07:05:32 +0000
From: Jyan Chou [周芷安] <jyanchou@...ltek.com>
To: Philipp Zabel <p.zabel@...gutronix.de>,
"ulf.hansson@...aro.org" <ulf.hansson@...aro.org>,
"adrian.hunter@...el.com" <adrian.hunter@...el.com>,
"jh80.chung@...sung.com" <jh80.chung@...sung.com>,
"riteshh@...eaurora.org" <riteshh@...eaurora.org>,
"robh+dt@...nel.org" <robh+dt@...nel.org>,
"krzysztof.kozlowski+dt@...aro.org"
<krzysztof.kozlowski+dt@...aro.org>
CC: "conor+dt@...nel.org" <conor+dt@...nel.org>,
"asutoshd@...eaurora.org" <asutoshd@...eaurora.org>,
"linux-mmc@...r.kernel.org" <linux-mmc@...r.kernel.org>,
"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"arnd@...db.de" <arnd@...db.de>,
"briannorris@...omium.org" <briannorris@...omium.org>,
"doug@...morgal.com" <doug@...morgal.com>,
"tonyhuang.sunplus@...il.com" <tonyhuang.sunplus@...il.com>,
"abel.vesa@...aro.org" <abel.vesa@...aro.org>,
"william.qiu@...rfivetech.com" <william.qiu@...rfivetech.com>
Subject: RE: [PATCH v7][2/4] mmc: Add Synopsys DesignWare mmc cmdq host driver
Hi Philipp,
>> 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().
Thanks for your advice, we will correct it in our next version.
> [...]
>> +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);
Also, we will correct this in our new version, thanks.
Best Regards,
Jyan
-----Original Message-----
From: Philipp Zabel <p.zabel@...gutronix.de>
Sent: Monday, November 27, 2023 8:51 PM
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
External mail.
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
Powered by blists - more mailing lists