On Tue, Mar 17, 2020 at 3:32 PM Adrian Hunter <adrian.hunter@xxxxxxxxx> wrote: > > On 17/03/20 5:36 am, Baolin Wang wrote: > > On Mon, Mar 16, 2020 at 9:09 PM Adrian Hunter <adrian.hunter@xxxxxxxxx> 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@xxxxxxxxx> > >>> Signed-off-by: Baolin Wang <baolin.wang7@xxxxxxxxx> > >>> --- > >>> 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