diff mbox series

[v4,1/3] mmc: host: Introduce the request_atomic() for the host

Message ID 04c25ff9f9b17d41d734c498e4be61dee745bb30.1585896945.git.baolin.wang7@gmail.com (mailing list archive)
State New, archived
Headers show
Series Introduce the request_atomic() for the host | expand

Commit Message

Baolin Wang April 3, 2020, 7:05 a.m. UTC
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 host software queue to reduce the
latency. Thus this patch adds a new API request_atomic() for the host
controller, as well as adding support for host software queue to submit
a request by the new request_atomic() API.

Moreover there is an unusual case that the card is busy when trying to
send a command, and we can not polling the card status in interrupt
context by using request_atomic() to dispatch requests. Thus we should
queue a work to try again in the non-atomic context in case the host
releases the busy signal later.

Suggested-by: Adrian Hunter <adrian.hunter@intel.com>
Signed-off-by: Baolin Wang <baolin.wang7@gmail.com>
---
 drivers/mmc/host/mmc_hsq.c | 29 ++++++++++++++++++++++++++++-
 drivers/mmc/host/mmc_hsq.h |  1 +
 include/linux/mmc/host.h   |  3 +++
 3 files changed, 32 insertions(+), 1 deletion(-)

Comments

Adrian Hunter April 7, 2020, 6:37 a.m. UTC | #1
On 3/04/20 10:05 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 host software queue to reduce the
> latency. Thus this patch adds a new API request_atomic() for the host
> controller, as well as adding support for host software queue to submit
> a request by the new request_atomic() API.
> 
> Moreover there is an unusual case that the card is busy when trying to
> send a command, and we can not polling the card status in interrupt
> context by using request_atomic() to dispatch requests. Thus we should
> queue a work to try again in the non-atomic context in case the host
> releases the busy signal later.
> 
> Suggested-by: Adrian Hunter <adrian.hunter@intel.com>
> Signed-off-by: Baolin Wang <baolin.wang7@gmail.com>


One minor point below, otherwise:

Acked-by: Adrian Hunter <adrian.hunter@intel.com>

> ---
>  drivers/mmc/host/mmc_hsq.c | 29 ++++++++++++++++++++++++++++-
>  drivers/mmc/host/mmc_hsq.h |  1 +
>  include/linux/mmc/host.h   |  3 +++
>  3 files changed, 32 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/mmc/host/mmc_hsq.c b/drivers/mmc/host/mmc_hsq.c
> index b90b2c9..a57f802 100644
> --- a/drivers/mmc/host/mmc_hsq.c
> +++ b/drivers/mmc/host/mmc_hsq.c
> @@ -16,11 +16,20 @@
>  #define HSQ_NUM_SLOTS	64
>  #define HSQ_INVALID_TAG	HSQ_NUM_SLOTS
>  
> +static void mmc_hsq_retry_handler(struct work_struct *work)
> +{
> +	struct mmc_hsq *hsq = container_of(work, struct mmc_hsq, retry_work);
> +	struct mmc_host *mmc = hsq->mmc;
> +
> +	mmc->ops->request(mmc, hsq->mrq);
> +}
> +
>  static void mmc_hsq_pump_requests(struct mmc_hsq *hsq)
>  {
>  	struct mmc_host *mmc = hsq->mmc;
>  	struct hsq_slot *slot;
>  	unsigned long flags;
> +	int ret = 0;
>  
>  	spin_lock_irqsave(&hsq->lock, flags);
>  
> @@ -42,7 +51,24 @@ static void mmc_hsq_pump_requests(struct mmc_hsq *hsq)
>  
>  	spin_unlock_irqrestore(&hsq->lock, flags);
>  
> -	mmc->ops->request(mmc, hsq->mrq);
> +	if (mmc->ops->request_atomic)
> +		ret = mmc->ops->request_atomic(mmc, hsq->mrq);
> +	else
> +		mmc->ops->request(mmc, hsq->mrq);
> +
> +	/*
> +	 * If returning BUSY from request_atomic(), which means the card
> +	 * may be busy now, and we should change to non-atomic context to
> +	 * try again for this unusual case, to avoid time-consuming operations
> +	 * in the atomic context.
> +	 *
> +	 * Note: we just give a warning for other error cases, since the host
> +	 * driver will handle them.
> +	 */
> +	if (ret == -EBUSY)
> +		schedule_work(&hsq->retry_work);
> +	else
> +		WARN_ON_ONCE(ret && ret != -EBUSY);

'ret != -EBUSY' is redundant because it is always true in the 'else' clause.

>  }
>  
>  static void mmc_hsq_update_next_tag(struct mmc_hsq *hsq, int remains)
> @@ -325,6 +351,7 @@ int mmc_hsq_init(struct mmc_hsq *hsq, struct mmc_host *mmc)
>  	hsq->mmc->cqe_private = hsq;
>  	mmc->cqe_ops = &mmc_hsq_ops;
>  
> +	INIT_WORK(&hsq->retry_work, mmc_hsq_retry_handler);
>  	spin_lock_init(&hsq->lock);
>  	init_waitqueue_head(&hsq->wait_queue);
>  
> diff --git a/drivers/mmc/host/mmc_hsq.h b/drivers/mmc/host/mmc_hsq.h
> index 18b9cf5..ffdd9cd 100644
> --- a/drivers/mmc/host/mmc_hsq.h
> +++ b/drivers/mmc/host/mmc_hsq.h
> @@ -12,6 +12,7 @@ struct mmc_hsq {
>  	wait_queue_head_t wait_queue;
>  	struct hsq_slot *slot;
>  	spinlock_t lock;
> +	struct work_struct retry_work;
>  
>  	int next_tag;
>  	int num_slots;
> diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
> index c318fb5..d4a50e5d 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. */
> +	int	(*request_atomic)(struct mmc_host *host,
> +				  struct mmc_request *req);
>  
>  	/*
>  	 * Avoid calling the next three functions too often or in a "fast
>
Baolin Wang April 7, 2020, 7:21 a.m. UTC | #2
On Tue, Apr 7, 2020 at 2:38 PM Adrian Hunter <adrian.hunter@intel.com> wrote:
>
> On 3/04/20 10:05 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 host software queue to reduce the
> > latency. Thus this patch adds a new API request_atomic() for the host
> > controller, as well as adding support for host software queue to submit
> > a request by the new request_atomic() API.
> >
> > Moreover there is an unusual case that the card is busy when trying to
> > send a command, and we can not polling the card status in interrupt
> > context by using request_atomic() to dispatch requests. Thus we should
> > queue a work to try again in the non-atomic context in case the host
> > releases the busy signal later.
> >
> > Suggested-by: Adrian Hunter <adrian.hunter@intel.com>
> > Signed-off-by: Baolin Wang <baolin.wang7@gmail.com>
>
>
> One minor point below, otherwise:
>
> Acked-by: Adrian Hunter <adrian.hunter@intel.com>
>
> > ---
> >  drivers/mmc/host/mmc_hsq.c | 29 ++++++++++++++++++++++++++++-
> >  drivers/mmc/host/mmc_hsq.h |  1 +
> >  include/linux/mmc/host.h   |  3 +++
> >  3 files changed, 32 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/mmc/host/mmc_hsq.c b/drivers/mmc/host/mmc_hsq.c
> > index b90b2c9..a57f802 100644
> > --- a/drivers/mmc/host/mmc_hsq.c
> > +++ b/drivers/mmc/host/mmc_hsq.c
> > @@ -16,11 +16,20 @@
> >  #define HSQ_NUM_SLOTS        64
> >  #define HSQ_INVALID_TAG      HSQ_NUM_SLOTS
> >
> > +static void mmc_hsq_retry_handler(struct work_struct *work)
> > +{
> > +     struct mmc_hsq *hsq = container_of(work, struct mmc_hsq, retry_work);
> > +     struct mmc_host *mmc = hsq->mmc;
> > +
> > +     mmc->ops->request(mmc, hsq->mrq);
> > +}
> > +
> >  static void mmc_hsq_pump_requests(struct mmc_hsq *hsq)
> >  {
> >       struct mmc_host *mmc = hsq->mmc;
> >       struct hsq_slot *slot;
> >       unsigned long flags;
> > +     int ret = 0;
> >
> >       spin_lock_irqsave(&hsq->lock, flags);
> >
> > @@ -42,7 +51,24 @@ static void mmc_hsq_pump_requests(struct mmc_hsq *hsq)
> >
> >       spin_unlock_irqrestore(&hsq->lock, flags);
> >
> > -     mmc->ops->request(mmc, hsq->mrq);
> > +     if (mmc->ops->request_atomic)
> > +             ret = mmc->ops->request_atomic(mmc, hsq->mrq);
> > +     else
> > +             mmc->ops->request(mmc, hsq->mrq);
> > +
> > +     /*
> > +      * If returning BUSY from request_atomic(), which means the card
> > +      * may be busy now, and we should change to non-atomic context to
> > +      * try again for this unusual case, to avoid time-consuming operations
> > +      * in the atomic context.
> > +      *
> > +      * Note: we just give a warning for other error cases, since the host
> > +      * driver will handle them.
> > +      */
> > +     if (ret == -EBUSY)
> > +             schedule_work(&hsq->retry_work);
> > +     else
> > +             WARN_ON_ONCE(ret && ret != -EBUSY);
>
> 'ret != -EBUSY' is redundant because it is always true in the 'else' clause.

Ah, Yes, thanks for pointing this out and I will fix it ine next version.

By the way, could you help to review patch 2 and 3 in this patch set? Thanks.

> >  }
> >
> >  static void mmc_hsq_update_next_tag(struct mmc_hsq *hsq, int remains)
> > @@ -325,6 +351,7 @@ int mmc_hsq_init(struct mmc_hsq *hsq, struct mmc_host *mmc)
> >       hsq->mmc->cqe_private = hsq;
> >       mmc->cqe_ops = &mmc_hsq_ops;
> >
> > +     INIT_WORK(&hsq->retry_work, mmc_hsq_retry_handler);
> >       spin_lock_init(&hsq->lock);
> >       init_waitqueue_head(&hsq->wait_queue);
> >
> > diff --git a/drivers/mmc/host/mmc_hsq.h b/drivers/mmc/host/mmc_hsq.h
> > index 18b9cf5..ffdd9cd 100644
> > --- a/drivers/mmc/host/mmc_hsq.h
> > +++ b/drivers/mmc/host/mmc_hsq.h
> > @@ -12,6 +12,7 @@ struct mmc_hsq {
> >       wait_queue_head_t wait_queue;
> >       struct hsq_slot *slot;
> >       spinlock_t lock;
> > +     struct work_struct retry_work;
> >
> >       int next_tag;
> >       int num_slots;
> > diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
> > index c318fb5..d4a50e5d 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. */
> > +     int     (*request_atomic)(struct mmc_host *host,
> > +                               struct mmc_request *req);
> >
> >       /*
> >        * Avoid calling the next three functions too often or in a "fast
> >
>
Adrian Hunter April 7, 2020, 10:14 a.m. UTC | #3
On 7/04/20 10:21 am, Baolin Wang wrote:
> On Tue, Apr 7, 2020 at 2:38 PM Adrian Hunter <adrian.hunter@intel.com> wrote:
>>
>> On 3/04/20 10:05 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 host software queue to reduce the
>>> latency. Thus this patch adds a new API request_atomic() for the host
>>> controller, as well as adding support for host software queue to submit
>>> a request by the new request_atomic() API.
>>>
>>> Moreover there is an unusual case that the card is busy when trying to
>>> send a command, and we can not polling the card status in interrupt
>>> context by using request_atomic() to dispatch requests. Thus we should
>>> queue a work to try again in the non-atomic context in case the host
>>> releases the busy signal later.
>>>
>>> Suggested-by: Adrian Hunter <adrian.hunter@intel.com>
>>> Signed-off-by: Baolin Wang <baolin.wang7@gmail.com>
>>
>>
>> One minor point below, otherwise:
>>
>> Acked-by: Adrian Hunter <adrian.hunter@intel.com>
>>
>>> ---
>>>  drivers/mmc/host/mmc_hsq.c | 29 ++++++++++++++++++++++++++++-
>>>  drivers/mmc/host/mmc_hsq.h |  1 +
>>>  include/linux/mmc/host.h   |  3 +++
>>>  3 files changed, 32 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/mmc/host/mmc_hsq.c b/drivers/mmc/host/mmc_hsq.c
>>> index b90b2c9..a57f802 100644
>>> --- a/drivers/mmc/host/mmc_hsq.c
>>> +++ b/drivers/mmc/host/mmc_hsq.c
>>> @@ -16,11 +16,20 @@
>>>  #define HSQ_NUM_SLOTS        64
>>>  #define HSQ_INVALID_TAG      HSQ_NUM_SLOTS
>>>
>>> +static void mmc_hsq_retry_handler(struct work_struct *work)
>>> +{
>>> +     struct mmc_hsq *hsq = container_of(work, struct mmc_hsq, retry_work);
>>> +     struct mmc_host *mmc = hsq->mmc;
>>> +
>>> +     mmc->ops->request(mmc, hsq->mrq);
>>> +}
>>> +
>>>  static void mmc_hsq_pump_requests(struct mmc_hsq *hsq)
>>>  {
>>>       struct mmc_host *mmc = hsq->mmc;
>>>       struct hsq_slot *slot;
>>>       unsigned long flags;
>>> +     int ret = 0;
>>>
>>>       spin_lock_irqsave(&hsq->lock, flags);
>>>
>>> @@ -42,7 +51,24 @@ static void mmc_hsq_pump_requests(struct mmc_hsq *hsq)
>>>
>>>       spin_unlock_irqrestore(&hsq->lock, flags);
>>>
>>> -     mmc->ops->request(mmc, hsq->mrq);
>>> +     if (mmc->ops->request_atomic)
>>> +             ret = mmc->ops->request_atomic(mmc, hsq->mrq);
>>> +     else
>>> +             mmc->ops->request(mmc, hsq->mrq);
>>> +
>>> +     /*
>>> +      * If returning BUSY from request_atomic(), which means the card
>>> +      * may be busy now, and we should change to non-atomic context to
>>> +      * try again for this unusual case, to avoid time-consuming operations
>>> +      * in the atomic context.
>>> +      *
>>> +      * Note: we just give a warning for other error cases, since the host
>>> +      * driver will handle them.
>>> +      */
>>> +     if (ret == -EBUSY)
>>> +             schedule_work(&hsq->retry_work);
>>> +     else
>>> +             WARN_ON_ONCE(ret && ret != -EBUSY);
>>
>> 'ret != -EBUSY' is redundant because it is always true in the 'else' clause.
> 
> Ah, Yes, thanks for pointing this out and I will fix it ine next version.
> 
> By the way, could you help to review patch 2 and 3 in this patch set? Thanks.
> 

I'd like to handle the inhibit wait differently.  I will make some patches
for that and send them out.
Baolin Wang April 8, 2020, 2:18 a.m. UTC | #4
On Tue, Apr 7, 2020 at 6:15 PM Adrian Hunter <adrian.hunter@intel.com> wrote:
>
> On 7/04/20 10:21 am, Baolin Wang wrote:
> > On Tue, Apr 7, 2020 at 2:38 PM Adrian Hunter <adrian.hunter@intel.com> wrote:
> >>
> >> On 3/04/20 10:05 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 host software queue to reduce the
> >>> latency. Thus this patch adds a new API request_atomic() for the host
> >>> controller, as well as adding support for host software queue to submit
> >>> a request by the new request_atomic() API.
> >>>
> >>> Moreover there is an unusual case that the card is busy when trying to
> >>> send a command, and we can not polling the card status in interrupt
> >>> context by using request_atomic() to dispatch requests. Thus we should
> >>> queue a work to try again in the non-atomic context in case the host
> >>> releases the busy signal later.
> >>>
> >>> Suggested-by: Adrian Hunter <adrian.hunter@intel.com>
> >>> Signed-off-by: Baolin Wang <baolin.wang7@gmail.com>
> >>
> >>
> >> One minor point below, otherwise:
> >>
> >> Acked-by: Adrian Hunter <adrian.hunter@intel.com>
> >>
> >>> ---
> >>>  drivers/mmc/host/mmc_hsq.c | 29 ++++++++++++++++++++++++++++-
> >>>  drivers/mmc/host/mmc_hsq.h |  1 +
> >>>  include/linux/mmc/host.h   |  3 +++
> >>>  3 files changed, 32 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/drivers/mmc/host/mmc_hsq.c b/drivers/mmc/host/mmc_hsq.c
> >>> index b90b2c9..a57f802 100644
> >>> --- a/drivers/mmc/host/mmc_hsq.c
> >>> +++ b/drivers/mmc/host/mmc_hsq.c
> >>> @@ -16,11 +16,20 @@
> >>>  #define HSQ_NUM_SLOTS        64
> >>>  #define HSQ_INVALID_TAG      HSQ_NUM_SLOTS
> >>>
> >>> +static void mmc_hsq_retry_handler(struct work_struct *work)
> >>> +{
> >>> +     struct mmc_hsq *hsq = container_of(work, struct mmc_hsq, retry_work);
> >>> +     struct mmc_host *mmc = hsq->mmc;
> >>> +
> >>> +     mmc->ops->request(mmc, hsq->mrq);
> >>> +}
> >>> +
> >>>  static void mmc_hsq_pump_requests(struct mmc_hsq *hsq)
> >>>  {
> >>>       struct mmc_host *mmc = hsq->mmc;
> >>>       struct hsq_slot *slot;
> >>>       unsigned long flags;
> >>> +     int ret = 0;
> >>>
> >>>       spin_lock_irqsave(&hsq->lock, flags);
> >>>
> >>> @@ -42,7 +51,24 @@ static void mmc_hsq_pump_requests(struct mmc_hsq *hsq)
> >>>
> >>>       spin_unlock_irqrestore(&hsq->lock, flags);
> >>>
> >>> -     mmc->ops->request(mmc, hsq->mrq);
> >>> +     if (mmc->ops->request_atomic)
> >>> +             ret = mmc->ops->request_atomic(mmc, hsq->mrq);
> >>> +     else
> >>> +             mmc->ops->request(mmc, hsq->mrq);
> >>> +
> >>> +     /*
> >>> +      * If returning BUSY from request_atomic(), which means the card
> >>> +      * may be busy now, and we should change to non-atomic context to
> >>> +      * try again for this unusual case, to avoid time-consuming operations
> >>> +      * in the atomic context.
> >>> +      *
> >>> +      * Note: we just give a warning for other error cases, since the host
> >>> +      * driver will handle them.
> >>> +      */
> >>> +     if (ret == -EBUSY)
> >>> +             schedule_work(&hsq->retry_work);
> >>> +     else
> >>> +             WARN_ON_ONCE(ret && ret != -EBUSY);
> >>
> >> 'ret != -EBUSY' is redundant because it is always true in the 'else' clause.
> >
> > Ah, Yes, thanks for pointing this out and I will fix it ine next version.
> >
> > By the way, could you help to review patch 2 and 3 in this patch set? Thanks.
> >
>
> I'd like to handle the inhibit wait differently.  I will make some patches
> for that and send them out.

OK, great. I'd like to test them. Thanks.
diff mbox series

Patch

diff --git a/drivers/mmc/host/mmc_hsq.c b/drivers/mmc/host/mmc_hsq.c
index b90b2c9..a57f802 100644
--- a/drivers/mmc/host/mmc_hsq.c
+++ b/drivers/mmc/host/mmc_hsq.c
@@ -16,11 +16,20 @@ 
 #define HSQ_NUM_SLOTS	64
 #define HSQ_INVALID_TAG	HSQ_NUM_SLOTS
 
+static void mmc_hsq_retry_handler(struct work_struct *work)
+{
+	struct mmc_hsq *hsq = container_of(work, struct mmc_hsq, retry_work);
+	struct mmc_host *mmc = hsq->mmc;
+
+	mmc->ops->request(mmc, hsq->mrq);
+}
+
 static void mmc_hsq_pump_requests(struct mmc_hsq *hsq)
 {
 	struct mmc_host *mmc = hsq->mmc;
 	struct hsq_slot *slot;
 	unsigned long flags;
+	int ret = 0;
 
 	spin_lock_irqsave(&hsq->lock, flags);
 
@@ -42,7 +51,24 @@  static void mmc_hsq_pump_requests(struct mmc_hsq *hsq)
 
 	spin_unlock_irqrestore(&hsq->lock, flags);
 
-	mmc->ops->request(mmc, hsq->mrq);
+	if (mmc->ops->request_atomic)
+		ret = mmc->ops->request_atomic(mmc, hsq->mrq);
+	else
+		mmc->ops->request(mmc, hsq->mrq);
+
+	/*
+	 * If returning BUSY from request_atomic(), which means the card
+	 * may be busy now, and we should change to non-atomic context to
+	 * try again for this unusual case, to avoid time-consuming operations
+	 * in the atomic context.
+	 *
+	 * Note: we just give a warning for other error cases, since the host
+	 * driver will handle them.
+	 */
+	if (ret == -EBUSY)
+		schedule_work(&hsq->retry_work);
+	else
+		WARN_ON_ONCE(ret && ret != -EBUSY);
 }
 
 static void mmc_hsq_update_next_tag(struct mmc_hsq *hsq, int remains)
@@ -325,6 +351,7 @@  int mmc_hsq_init(struct mmc_hsq *hsq, struct mmc_host *mmc)
 	hsq->mmc->cqe_private = hsq;
 	mmc->cqe_ops = &mmc_hsq_ops;
 
+	INIT_WORK(&hsq->retry_work, mmc_hsq_retry_handler);
 	spin_lock_init(&hsq->lock);
 	init_waitqueue_head(&hsq->wait_queue);
 
diff --git a/drivers/mmc/host/mmc_hsq.h b/drivers/mmc/host/mmc_hsq.h
index 18b9cf5..ffdd9cd 100644
--- a/drivers/mmc/host/mmc_hsq.h
+++ b/drivers/mmc/host/mmc_hsq.h
@@ -12,6 +12,7 @@  struct mmc_hsq {
 	wait_queue_head_t wait_queue;
 	struct hsq_slot *slot;
 	spinlock_t lock;
+	struct work_struct retry_work;
 
 	int next_tag;
 	int num_slots;
diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
index c318fb5..d4a50e5d 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. */
+	int	(*request_atomic)(struct mmc_host *host,
+				  struct mmc_request *req);
 
 	/*
 	 * Avoid calling the next three functions too often or in a "fast