| 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: <be484941-1aa8-479e-a491-8fa414e3b9b6@foss.st.com>
Date: Fri, 27 Oct 2023 15:09:59 +0200
From: Yann Gautier <yann.gautier@...s.st.com>
To: Ulf Hansson <ulf.hansson@...aro.org>
CC: Russell King <linux@...linux.org.uk>,
Linus Walleij <linus.walleij@...aro.org>,
Alexandre Torgue <alexandre.torgue@...s.st.com>,
Maxime Coquelin <mcoquelin.stm32@...il.com>,
Christophe Kerello <christophe.kerello@...s.st.com>,
Yang Yingliang <yangyingliang@...wei.com>,
Rob Herring <robh@...nel.org>, <linux-mmc@...r.kernel.org>,
<linux-kernel@...r.kernel.org>,
<linux-stm32@...md-mailman.stormreply.com>,
<linux-arm-kernel@...ts.infradead.org>
Subject: Re: [PATCH v2] mmc: mmci: stm32: add SDIO in-band interrupt mode
On 9/26/23 12:29, Ulf Hansson wrote:
> On Thu, 14 Sept 2023 at 17:09, Yann Gautier <yann.gautier@...s.st.com> wrote:
>>
>> From: Christophe Kerello <christophe.kerello@...s.st.com>
>>
>> Add the support of SDIO in-band interrupt mode for STM32 and Ux500
>> variants.
>> It allows the SD I/O card to interrupt the host on SDMMC_D1 data line.
>> It is not enabled by default on Ux500 variant as this is unstable and
>> Ux500 users should use out-of-band IRQs.
>>
>> Signed-off-by: Christophe Kerello <christophe.kerello@...s.st.com>
>> Signed-off-by: Yann Gautier <yann.gautier@...s.st.com>
>> ---
>> Updates on v2:
>> * rename use_sdio_irq to supports_sdio_irq and change it to bool
>> * use common code for ux500 and stm32 variants
>>
>> ---
>> drivers/mmc/host/mmci.c | 85 +++++++++++++++++++++++++++++
>> drivers/mmc/host/mmci.h | 7 +++
>> drivers/mmc/host/mmci_stm32_sdmmc.c | 2 +
>> 3 files changed, 94 insertions(+)
>>
>> diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c
>> index dda756a563793..65cc03ee7f23b 100644
>> --- a/drivers/mmc/host/mmci.c
>> +++ b/drivers/mmc/host/mmci.c
>> @@ -272,6 +272,7 @@ static struct variant_data variant_stm32_sdmmc = {
>> .datactrl_mask_sdio = MCI_DPSM_ST_SDIOEN,
>> .stm32_idmabsize_mask = GENMASK(12, 5),
>> .stm32_idmabsize_align = BIT(5),
>> + .supports_sdio_irq = true,
>> .busy_timeout = true,
>> .busy_detect = true,
>> .busy_detect_flag = MCI_STM32_BUSYD0,
>> @@ -299,6 +300,7 @@ static struct variant_data variant_stm32_sdmmcv2 = {
>> .datactrl_mask_sdio = MCI_DPSM_ST_SDIOEN,
>> .stm32_idmabsize_mask = GENMASK(16, 5),
>> .stm32_idmabsize_align = BIT(5),
>> + .supports_sdio_irq = true,
>> .dma_lli = true,
>> .busy_timeout = true,
>> .busy_detect = true,
>> @@ -327,6 +329,7 @@ static struct variant_data variant_stm32_sdmmcv3 = {
>> .datactrl_mask_sdio = MCI_DPSM_ST_SDIOEN,
>> .stm32_idmabsize_mask = GENMASK(16, 6),
>> .stm32_idmabsize_align = BIT(6),
>> + .supports_sdio_irq = true,
>> .dma_lli = true,
>> .busy_timeout = true,
>> .busy_detect = true,
>> @@ -423,6 +426,11 @@ static void mmci_write_datactrlreg(struct mmci_host *host, u32 datactrl)
>> /* Keep busy mode in DPSM if enabled */
>> datactrl |= host->datactrl_reg & host->variant->busy_dpsm_flag;
>>
>> + /* Keep SD I/O interrupt mode enabled */
>> + if (host->variant->supports_sdio_irq &&
>> + host->mmc->caps & MMC_CAP_SDIO_IRQ)
>> + datactrl |= host->variant->datactrl_mask_sdio;
>> +
>
> This doesn't look entirely correct to me, as it will make the
> ->datactrl_mask_sdio bit to be set even when it shouldn't. If I
> understand correctly, we really want the bit to be set if the SDIO
> irqs has been enabled, but otherwise leave it for mmci_start_data() to
> manage it, right?
>
> That said, perhaps the comment a few lines above, deserves some
> clarification too. Would rephrasing it into "Keep the SDIO mode bit if
> SDIO irqs are enabled" make it more clear?
>
> From an implementation point of view, an idea is to add a
> "host->datactrl_reg_add" variable (we have this for the clk and pwr
> registers already). mmci_write_datactrlreg() should then OR these bits
> when writing to the register. In this way, mmci_enable_sdio_irq() can
> update the host->datactrl_reg_add with ->datactrl_mask_sdio, when
> needed. This should also work for the ->busy_dpsm_flag a few lines
> above, I think.
>
>> if (host->datactrl_reg != datactrl) {
>> host->datactrl_reg = datactrl;
>> writel(datactrl, host->base + MMCIDATACTRL);
>> @@ -817,6 +825,25 @@ static bool ux500_busy_complete(struct mmci_host *host, struct mmc_command *cmd,
>> return (host->busy_state == MMCI_BUSY_DONE);
>> }
>>
>> +void ux500_and_stm32_enable_sdio_irq(struct mmci_host *host, int enable)
>> +{
>> + void __iomem *base = host->base;
>> + u32 mask = readl_relaxed(base + MMCIMASK0);
>> +
>> + if (enable)
>> + writel_relaxed(mask | MCI_ST_SDIOITMASK, base + MMCIMASK0);
>> + else
>> + writel_relaxed(mask & ~MCI_ST_SDIOITMASK, base + MMCIMASK0);
>> +}
>> +
>> +void ux500_and_stm32_sdio_irq(struct mmci_host *host, u32 status)
>> +{
>> + if (status & MCI_ST_SDIOIT) {
>> + ux500_and_stm32_enable_sdio_irq(host, 0);
>> + sdio_signal_irq(host->mmc);
>> + }
>> +}
>> +
>> /*
>> * All the DMA operation mode stuff goes inside this ifdef.
>> * This assumes that you have a generic DMA device interface,
>> @@ -1191,6 +1218,8 @@ static void ux500_variant_init(struct mmci_host *host)
>> {
>> host->ops = &mmci_variant_ops;
>> host->ops->busy_complete = ux500_busy_complete;
>> + host->ops->enable_sdio_irq = ux500_and_stm32_enable_sdio_irq;
>> + host->ops->sdio_irq = ux500_and_stm32_sdio_irq;
>> }
>>
>> static void ux500v2_variant_init(struct mmci_host *host)
>> @@ -1198,6 +1227,8 @@ static void ux500v2_variant_init(struct mmci_host *host)
>> host->ops = &mmci_variant_ops;
>> host->ops->busy_complete = ux500_busy_complete;
>> host->ops->get_datactrl_cfg = ux500v2_get_dctrl_cfg;
>> + host->ops->enable_sdio_irq = ux500_and_stm32_enable_sdio_irq;
>> + host->ops->sdio_irq = ux500_and_stm32_sdio_irq;
>> }
>
> It looks to me that the extra layer of mmci variant callbacks is a bit
> "heavy" at this point. ux500 and the st variants seem to work very
> similarly in this regard. So maybe just the mmci_ops callbacks
> directly and stick to the mmci* prefix of the function names. At least
> until we see a better reason to have the extra layer of callbacks.
>
> Of course, this also means that we need to assign
> mmci_ops->enable_sdio_irq|ack_sdio_irq() conditionally during probe,
> based upon the variant->supports_sdio_irq bit.
>
>>
>> static void mmci_pre_request(struct mmc_host *mmc, struct mmc_request *mrq)
>> @@ -1805,6 +1836,11 @@ static irqreturn_t mmci_irq(int irq, void *dev_id)
>> mmci_data_irq(host, host->data, status);
>> }
>>
>> + if (host->variant->supports_sdio_irq &&
>> + host->mmc->caps & MMC_CAP_SDIO_IRQ &&
>
> Checking the caps seems superfluous. The SDIO irqs must not be
> enabled, unless MMC_CAP_SDIO_IRQ is supported, right?
>
>> + host->ops && host->ops->sdio_irq)
>> + host->ops->sdio_irq(host, status);
>> +
>> /*
>> * Busy detection has been handled by mmci_cmd_irq() above.
>> * Clear the status bit to prevent polling in IRQ context.
>> @@ -2041,6 +2077,45 @@ static int mmci_sig_volt_switch(struct mmc_host *mmc, struct mmc_ios *ios)
>> return ret;
>> }
>>
>> +static void mmci_enable_sdio_irq(struct mmc_host *mmc, int enable)
>> +{
>> + struct mmci_host *host = mmc_priv(mmc);
>> + unsigned long flags;
>> +
>> + if (!host->variant->supports_sdio_irq)
>> + return;
>
> According to the earlier comment above about the extra layers of
> callbacks, this can then be checked during probe instead and dropped
> from here.
>
>> +
>> + if (host->ops && host->ops->enable_sdio_irq) {
>> + if (enable)
>> + /* Keep device active while SDIO IRQ is enabled */
>> + pm_runtime_get_sync(mmc_dev(mmc));
>> +
>> + spin_lock_irqsave(&host->lock, flags);
>> + host->ops->enable_sdio_irq(host, enable);
>> + spin_unlock_irqrestore(&host->lock, flags);
>> +
>> + if (!enable) {
>> + pm_runtime_mark_last_busy(mmc_dev(mmc));
>> + pm_runtime_put_autosuspend(mmc_dev(mmc));
>> + }
>> + }
>> +}
>> +
>> +static void mmci_ack_sdio_irq(struct mmc_host *mmc)
>> +{
>> + struct mmci_host *host = mmc_priv(mmc);
>> + unsigned long flags;
>> +
>> + if (!host->variant->supports_sdio_irq)
>> + return;
>
> Ditto.
>
>> +
>> + if (host->ops && host->ops->enable_sdio_irq) {
>> + spin_lock_irqsave(&host->lock, flags);
>> + host->ops->enable_sdio_irq(host, 1);
>> + spin_unlock_irqrestore(&host->lock, flags);
>> + }
>> +}
>> +
>> static struct mmc_host_ops mmci_ops = {
>> .request = mmci_request,
>> .pre_req = mmci_pre_request,
>> @@ -2049,6 +2124,8 @@ static struct mmc_host_ops mmci_ops = {
>> .get_ro = mmc_gpio_get_ro,
>> .get_cd = mmci_get_cd,
>> .start_signal_voltage_switch = mmci_sig_volt_switch,
>> + .enable_sdio_irq = mmci_enable_sdio_irq,
>> + .ack_sdio_irq = mmci_ack_sdio_irq,
>> };
>>
>> static void mmci_probe_level_translator(struct mmc_host *mmc)
>> @@ -2316,6 +2393,14 @@ static int mmci_probe(struct amba_device *dev,
>> mmc->caps |= MMC_CAP_WAIT_WHILE_BUSY;
>> }
>>
>> + if (variant->supports_sdio_irq && host->mmc->caps & MMC_CAP_SDIO_IRQ) {
>> + mmc->caps2 |= MMC_CAP2_SDIO_IRQ_NOTHREAD;
>> +
>> + if (variant->datactrl_mask_sdio)
>> + mmci_write_datactrlreg(host,
>> + host->variant->datactrl_mask_sdio);
>
> As I stated earlier, it looks to me that this should be managed when
> enabling/disabling the SDIO irqs and not during probe. No?
>
>> + }
>> +
>> /* Variants with mandatory busy timeout in HW needs R1B responses. */
>> if (variant->busy_timeout)
>> mmc->caps |= MMC_CAP_NEED_RSP_BUSY;
>> diff --git a/drivers/mmc/host/mmci.h b/drivers/mmc/host/mmci.h
>> index 253197f132fca..5ea4975c18ec5 100644
>> --- a/drivers/mmc/host/mmci.h
>> +++ b/drivers/mmc/host/mmci.h
>> @@ -331,6 +331,7 @@ enum mmci_busy_state {
>> * register.
>> * @opendrain: bitmask identifying the OPENDRAIN bit inside MMCIPOWER register
>> * @dma_lli: true if variant has dma link list feature.
>> + * @supports_sdio_irq: allow SD I/O card to interrupt the host
>> * @stm32_idmabsize_mask: stm32 sdmmc idma buffer size.
>> */
>> struct variant_data {
>> @@ -376,6 +377,7 @@ struct variant_data {
>> u32 start_err;
>> u32 opendrain;
>> u8 dma_lli:1;
>> + bool supports_sdio_irq;
>> u32 stm32_idmabsize_mask;
>> u32 stm32_idmabsize_align;
>> void (*init)(struct mmci_host *host);
>> @@ -400,6 +402,8 @@ struct mmci_host_ops {
>> bool (*busy_complete)(struct mmci_host *host, struct mmc_command *cmd, u32 status, u32 err_msk);
>> void (*pre_sig_volt_switch)(struct mmci_host *host);
>> int (*post_sig_volt_switch)(struct mmci_host *host, struct mmc_ios *ios);
>> + void (*enable_sdio_irq)(struct mmci_host *host, int enable);
>> + void (*sdio_irq)(struct mmci_host *host, u32 status);
>> };
>>
>> struct mmci_host {
>> @@ -481,6 +485,9 @@ void mmci_dmae_finalize(struct mmci_host *host, struct mmc_data *data);
>> void mmci_dmae_error(struct mmci_host *host);
>> #endif
>>
>> +void ux500_and_stm32_enable_sdio_irq(struct mmci_host *host, int enable);
>> +void ux500_and_stm32_sdio_irq(struct mmci_host *host, u32 status);
>> +
>> #ifdef CONFIG_MMC_QCOM_DML
>> void qcom_variant_init(struct mmci_host *host);
>> #else
>> diff --git a/drivers/mmc/host/mmci_stm32_sdmmc.c b/drivers/mmc/host/mmci_stm32_sdmmc.c
>> index 35067e1e6cd80..fbfaa0bcec51e 100644
>> --- a/drivers/mmc/host/mmci_stm32_sdmmc.c
>> +++ b/drivers/mmc/host/mmci_stm32_sdmmc.c
>> @@ -681,6 +681,8 @@ static struct mmci_host_ops sdmmc_variant_ops = {
>> .busy_complete = sdmmc_busy_complete,
>> .pre_sig_volt_switch = sdmmc_pre_sig_volt_vswitch,
>> .post_sig_volt_switch = sdmmc_post_sig_volt_switch,
>> + .enable_sdio_irq = ux500_and_stm32_enable_sdio_irq,
>> + .sdio_irq = ux500_and_stm32_sdio_irq,
>> };
>>
>> static struct sdmmc_tuning_ops dlyb_tuning_mp15_ops = {
>> --
>> 2.34.1
>>
>
> Kind regards
> Uffe
Hi Ulf,
Thanks for the review and sorry for the late reply.
Christophe and I aggreed with the proposed changes.
A new patch version should be sent within a couple of weeks.
Best regards,
Yann
Powered by blists - more mailing lists