| 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: <1d50d8fa62cf4bdf969c468bd4ed9776@realtek.com>
Date: Wed, 18 Oct 2023 06:17:17 +0000
From: Jyan Chou [周芷安] <jyanchou@...ltek.com>
To: Adrian Hunter <adrian.hunter@...el.com>,
"ulf.hansson@...aro.org" <ulf.hansson@...aro.org>,
"jh80.chung@...sung.com" <jh80.chung@...sung.com>
CC: "linux-mmc@...r.kernel.org" <linux-mmc@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
James Tai [戴志峰] <james.tai@...ltek.com>,
Ben Chuang <benchuanggli@...il.com>
Subject: RE: [PATCH 2/3] [02/03] mmc: Add Synopsys DesignWare mmc cmdq host driver
Hi Adrian,
Thanks for your review and advice.
We had updated our new version patches and corrected our code to your suggestions.
https://patchwork.kernel.org/project/linux-mmc/patch/20231018055326.18256-2-jyanchou@realtek.com/
https://patchwork.kernel.org/project/linux-mmc/patch/20231018055326.18256-3-jyanchou@realtek.com/
https://patchwork.kernel.org/project/linux-mmc/patch/20231018055326.18256-4-jyanchou@realtek.com/
https://patchwork.kernel.org/project/linux-mmc/patch/20231018055326.18256-5-jyanchou@realtek.com/
>> Please put version numbers in the patch subject.
We modified version numbers this time, thanks.
> Register differences can be abstracted away, for example by providing callbacks for reading / writing registers.
> So this still needs much more explanation.
We had added more description to explain.
> It would be preferable to use a callback only for setting the descriptor.
> Please see comments about dwcmshc_cqhci_set_tran_desc() and dwcmshc_cqhci_prep_tran_desc() made here:
> https://lore.kernel.org/linux-mmc/0932b124-16da-495c-9706-bbadadb3b076@intel.com/
We EXPORT cqhci_set_tran_desc and used it to replace dw_mci_cqe_set_tran_desc, thanks.
> This should not be necessary. Instead, please try to use
> ->pre_enable() and ->post_disable() like in mtk-sd.c
Thanks for your advice, we had removed unnecessary check code and added cqhci_host_ops callback function.
Best Regards,
Jyan
-----Original Message-----
From: Adrian Hunter <adrian.hunter@...el.com>
Sent: Tuesday, October 10, 2023 3:04 PM
To: Jyan Chou [周芷安] <jyanchou@...ltek.com>; ulf.hansson@...aro.org; jh80.chung@...sung.com
Cc: linux-mmc@...r.kernel.org; linux-kernel@...r.kernel.org; James Tai [戴志峰] <james.tai@...ltek.com>
Subject: Re: [PATCH 2/3] [02/03] mmc: Add Synopsys DesignWare mmc cmdq host driver
External mail.
Please put version numbers in the patch subject. Refer to:
https://www.kernel.org/doc/html/latest/process/submitting-patches.html
On 6/09/23 12:28, 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 and the addition of cmdq.
Register differences can be abstracted away, for example by providing callbacks for reading / writing registers.
So this still needs much more explanation.
>
> More over, the flow of abort command sequence had change.
> 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>
>
> —
> v0 to v1 change:
> 1. Seperate different support into single patch.
> 2. Fix the compiler complains.
> ---
> drivers/mmc/host/Kconfig | 13 +
> drivers/mmc/host/Makefile | 1 +
> drivers/mmc/host/dw_mmc_cqe.c | 1634
> +++++++++++++++++++++++++++++++++ drivers/mmc/host/dw_mmc_cqe.h |
> 443 +++++++++
> 4 files changed, 2091 insertions(+)
> create mode 100644 drivers/mmc/host/dw_mmc_cqe.c create mode 100644
> drivers/mmc/host/dw_mmc_cqe.h
My comments pertain only to the use of cqhci.
[SNIP]
> +static void dw_mci_cqe_set_tran_desc(u8 *desc,
> + dma_addr_t addr,
> + int len,
> + bool end,
> + bool dma64) {
> + __le32 *attr = (__le32 __force *)desc;
> +
> + *attr = (CQHCI_VALID(1) |
> + CQHCI_END(end ? 1 : 0) |
> + CQHCI_INT(0) |
> + CQHCI_ACT(0x4) |
> + CQHCI_DAT_LENGTH(len));
> +
> + if (dma64) {
> + __le64 *dataddr = (__le64 __force *)(desc + 4);
> +
> + dataddr[0] = cpu_to_le64(addr);
> + } else {
> + __le32 *dataddr = (__le32 __force *)(desc + 4);
> +
> + dataddr[0] = cpu_to_le32(addr);
> + }
> +}
> +
> +static void dw_mci_cqe_setup_tran_desc(struct mmc_data *data,
> + struct cqhci_host *cq_host,
> + u8 *desc,
> + int sg_count) {
> + struct scatterlist *sg;
> + u32 cur_blk_cnt, remain_blk_cnt;
> + unsigned int begin, end;
> + int i, len;
> + bool last = false;
> + bool dma64 = cq_host->dma64;
> + dma_addr_t addr;
> +
> + for_each_sg(data->sg, sg, sg_count, i) {
> + addr = sg_dma_address(sg);
> + len = sg_dma_len(sg);
> + remain_blk_cnt = len >> 9;
> +
> + while (remain_blk_cnt) {
> + /*DW_MCI_MAX_SCRIPT_BLK is tha max for each descriptor record*/
> + if (remain_blk_cnt > DW_MCI_MAX_SCRIPT_BLK)
> + cur_blk_cnt = DW_MCI_MAX_SCRIPT_BLK;
> + else
> + cur_blk_cnt = remain_blk_cnt;
> +
> + /* In Synopsys DesignWare Databook Page 84,
> + * They mentioned the DMA 128MB restriction
> + */
> + begin = addr / SZ_128M;
> + end = (addr + cur_blk_cnt * SZ_512) / SZ_128M;
> +
> + if (begin != end)
> + cur_blk_cnt = (end * SZ_128M - addr) /
> + SZ_512;
> +
> + if ((i+1) == sg_count && (remain_blk_cnt == cur_blk_cnt))
> + last = true;
> +
> + dw_mci_cqe_set_tran_desc(desc, addr,
> + (cur_blk_cnt << 9), last,
> + dma64);
> +
> + addr = addr + (cur_blk_cnt << 9);
> + remain_blk_cnt -= cur_blk_cnt;
> + desc += cq_host->trans_desc_len;
> + }
> + }
> +}
It would be preferable to use a callback only for setting the descriptor.
Please see comments about dwcmshc_cqhci_set_tran_desc() and dwcmshc_cqhci_prep_tran_desc() made here:
https://lore.kernel.org/linux-mmc/0932b124-16da-495c-9706-bbadadb3b076@intel.com/
[SNIP]
> +static void dw_mci_cqe_request(struct mmc_host *mmc, struct
> +mmc_request *mrq) {
> + struct dw_mci_slot *slot = mmc_priv(mmc);
> + struct dw_mci *host = slot->host;
> +
> + WARN_ON(slot->mrq);
> +
> + /*
> + * The check for card presence and queueing of the request must be
> + * atomic, otherwise the card could be removed in between and the
> + * request wouldn't fail until another card was inserted.
> + */
> +
> + if (!dw_mci_cqe_get_cd(mmc)) {
> + mrq->cmd->error = -ENOMEDIUM;
> + mmc_request_done(mmc, mrq);
> + return;
> + }
> +
> + down_write(&host->cr_rw_sem);
> +
> + /*cmdq case needs extra check*/
> + if (host->pdata && (host->pdata->caps2 & MMC_CAP2_CQE)) {
> + if ((host->cqe) == NULL) {
> + dev_err(host->dev, "dw_mci_request_cqe not done yet\n");
> + mdelay(2);
> + }
> +
> + if (mmc->cqe_on == false && host->cqe->activated == true)
> + cqhci_deactivate(mmc);
This should not be necessary. Instead, please try to use
->pre_enable() and ->post_disable() like in mtk-sd.c
[SNIP]
Powered by blists - more mailing lists