linux-kernel - Re: [RESEND PATCH 1/3] mmc: host: Introduce the request

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] [day] [month] [year] [list]
Date:   Tue, 17 Mar 2020 17:17:21 +0800
From:   Baolin Wang <baolin.wang7@...il.com>
To:     Adrian Hunter <adrian.hunter@...el.com>
Cc:     Ulf Hansson <ulf.hansson@...aro.org>,
        Orson Zhai <orsonzhai@...il.com>,
        Chunyan Zhang <zhang.lyra@...il.com>,
        Arnd Bergmann <arnd@...db.de>,
        linux-mmc <linux-mmc@...r.kernel.org>,
        LKML <linux-kernel@...r.kernel.org>
Subject: Re: [RESEND PATCH 1/3] mmc: host: Introduce the request_atomic() for
 the host

On Tue, Mar 17, 2020 at 3:32 PM Adrian Hunter <adrian.hunter@...el.com> wrote:
>
> On 17/03/20 5:36 am, Baolin Wang wrote:
> > On Mon, Mar 16, 2020 at 9:09 PM Adrian Hunter <adrian.hunter@...el.com> wrote:
> >>
> >> On 4/03/20 9:42 am, Baolin Wang wrote:
> >>> The SD host controller can process one request in the atomic context if
> >>> the card is nonremovable, which means we can submit next request in the
> >>> irq hard handler when using the MMC software queue to reduce the latency.
> >>> Thus this patch adds a new API request_atomic() for the host controller
> >>> and implement it for the SD host controller.
> >>>
> >>> Suggested-by: Adrian Hunter <adrian.hunter@...el.com>
> >>> Signed-off-by: Baolin Wang <baolin.wang7@...il.com>
> >>> ---
> >>>  drivers/mmc/host/sdhci.c | 27 +++++++++++++++++++--------
> >>>  drivers/mmc/host/sdhci.h |  1 +
> >>>  include/linux/mmc/host.h |  3 +++
> >>>  3 files changed, 23 insertions(+), 8 deletions(-)
> >>>
> >>> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> >>> index 9c37451..4febbcb 100644
> >>> --- a/drivers/mmc/host/sdhci.c
> >>> +++ b/drivers/mmc/host/sdhci.c
> >>> @@ -2016,17 +2016,12 @@ void sdhci_set_power(struct sdhci_host *host, unsigned char mode,
> >>>   *                                                                           *
> >>>  \*****************************************************************************/
> >>>
> >>> -void sdhci_request(struct mmc_host *mmc, struct mmc_request *mrq)
> >>> +static void sdhci_start_request(struct mmc_host *mmc, struct mmc_request *mrq,
> >>> +                             int present)
> >>>  {
> >>> -     struct sdhci_host *host;
> >>> -     int present;
> >>> +     struct sdhci_host *host = mmc_priv(mmc);
> >>>       unsigned long flags;
> >>>
> >>> -     host = mmc_priv(mmc);
> >>> -
> >>> -     /* Firstly check card presence */
> >>> -     present = mmc->ops->get_cd(mmc);
> >>> -
> >>>       spin_lock_irqsave(&host->lock, flags);
> >>>
> >>>       sdhci_led_activate(host);
> >>> @@ -2043,6 +2038,22 @@ void sdhci_request(struct mmc_host *mmc, struct mmc_request *mrq)
> >>>
> >>>       spin_unlock_irqrestore(&host->lock, flags);
> >>>  }
> >>> +
> >>> +void sdhci_request_atomic(struct mmc_host *mmc, struct mmc_request *mrq)
> >>> +{
> >>> +     sdhci_start_request(mmc, mrq, 1);
> >>> +}
> >>> +EXPORT_SYMBOL_GPL(sdhci_request_atomic);
> >>> +
> >>> +void sdhci_request(struct mmc_host *mmc, struct mmc_request *mrq)
> >>> +{
> >>> +     int present;
> >>> +
> >>> +     /* Firstly check card presence */
> >>> +     present = mmc->ops->get_cd(mmc);
> >>> +
> >>> +     sdhci_start_request(mmc, mrq, present);
> >>> +}
> >>>  EXPORT_SYMBOL_GPL(sdhci_request);
> >>>
> >>>  void sdhci_set_bus_width(struct sdhci_host *host, int width)
> >>> diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
> >>> index cac2d97..5507a73 100644
> >>> --- a/drivers/mmc/host/sdhci.h
> >>> +++ b/drivers/mmc/host/sdhci.h
> >>> @@ -775,6 +775,7 @@ void sdhci_set_power(struct sdhci_host *host, unsigned char mode,
> >>>  void sdhci_set_power_noreg(struct sdhci_host *host, unsigned char mode,
> >>>                          unsigned short vdd);
> >>>  void sdhci_request(struct mmc_host *mmc, struct mmc_request *mrq);
> >>> +void sdhci_request_atomic(struct mmc_host *mmc, struct mmc_request *mrq);
> >>>  void sdhci_set_bus_width(struct sdhci_host *host, int width);
> >>>  void sdhci_reset(struct sdhci_host *host, u8 mask);
> >>>  void sdhci_set_uhs_signaling(struct sdhci_host *host, unsigned timing);
> >>> diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
> >>> index 562ed06..db5e59c 100644
> >>> --- a/include/linux/mmc/host.h
> >>> +++ b/include/linux/mmc/host.h
> >>> @@ -92,6 +92,9 @@ struct mmc_host_ops {
> >>>                           int err);
> >>>       void    (*pre_req)(struct mmc_host *host, struct mmc_request *req);
> >>>       void    (*request)(struct mmc_host *host, struct mmc_request *req);
> >>> +     /* Submit one request to host in atomic context. */
> >>> +     void    (*request_atomic)(struct mmc_host *host,
> >>> +                               struct mmc_request *req);
> >>
> >> This doesn't have the flexibility to return "busy".  For example,
> >> sdhci_send_command() will potentially wait quite some time if the inhibit
> >> bits are set.  That is not good in interrupt context.  It would be better to
> >> return immediately in that case and have the caller fall back to a
> >> non-atomic context.  Thoughts?
> >
> > Yes, I unserstood your concern. But the sdhci_send_command() is
> > already under the spin_lock_irqsave() protection, which will also
> > disable the interrupt for some time if the inhibit bits are set. That
> > is same with moving it in interrupt context.
>
> It is, but I would like to fix that too.

OK. Like you suggested, cause we've aleady decided how to complete a
request by sdhci_defer_done(), I need think about how to change to a
non-atomic context for this unusual case.

And since the original sdhci_send_command() has the same problem as I
said, I perfer to create another patch set to fix this issue.


> > Moreover, if the previous command complete interrupt and transfer
> > complete interrupt are normal, we should not meet this issue of
> > polling inhibit bits (I have not met this issue on my platform). So I
> > think we can remove the polling here? If the inhibit bits are set, I
> > think the command complete interrupt or the transfer complete
> > interrupt have been abnormal, so we can just return the error here.
> > What do you think? Thanks.
> >
>
> I suspect the inhibit polling might be needed for some host controllers in
> some situations.  ie. taking it out would likely break things.

Make sense.


--
Baolin Wang