* [PATCH 0/3] mmc: Improve/fix support for SDIO IRQs
@ 2017-04-18 12:32 Ulf Hansson
2017-04-18 12:32 ` [PATCH 1/3] mmc: sdio: Add API to manage SDIO IRQs from a workqueue Ulf Hansson
` (2 more replies)
0 siblings, 3 replies; 12+ messages in thread
From: Ulf Hansson @ 2017-04-18 12:32 UTC (permalink / raw)
To: linux-mmc, Ulf Hansson
Cc: Jaehoon Chung, Adrian Hunter, Brian Norris, Shawn Lin, Doug Anderson
Regressions for SDIO IRQs have been reported for dw_mmc, caused by enabling
runtime PM support for it.
This series extends and improves the SDIO IRQs support in the core, such that
it better suites the need for dw_mmc, particulary around runtime PM.
Do note, so far this is only compile tested. I would thus appreciate help in
testing and of course also in reviewing.
I am not sure this is material for stable, so perhaps we anyway should pick up
Dough's earlier quick fix for dw_mmc. Thoughts?
Ulf Hansson (3):
mmc: sdio: Add API to manage SDIO IRQs from a workqueue
mmc: dw_mmc: Convert to use MMC_CAP2_SDIO_IRQ_NOTHREAD for SDIO IRQs
mmc: dw_mmc: Prevent runtime PM suspend when SDIO IRQs are enabled
drivers/mmc/core/host.c | 2 ++
drivers/mmc/core/sdio_irq.c | 32 ++++++++++++++++++++++++++++++--
drivers/mmc/core/sdio_ops.h | 2 ++
drivers/mmc/host/dw_mmc.c | 37 ++++++++++++++++++++++++++++++++++---
include/linux/mmc/host.h | 3 +++
5 files changed, 71 insertions(+), 5 deletions(-)
--
2.7.4
^ permalink raw reply [flat|nested] 12+ messages in thread* [PATCH 1/3] mmc: sdio: Add API to manage SDIO IRQs from a workqueue 2017-04-18 12:32 [PATCH 0/3] mmc: Improve/fix support for SDIO IRQs Ulf Hansson @ 2017-04-18 12:32 ` Ulf Hansson 2017-04-18 21:43 ` Doug Anderson 2017-04-18 12:32 ` [PATCH 2/3] mmc: dw_mmc: Convert to use MMC_CAP2_SDIO_IRQ_NOTHREAD for SDIO IRQs Ulf Hansson 2017-04-18 12:32 ` [PATCH 3/3] mmc: dw_mmc: Prevent runtime PM suspend when SDIO IRQs are enabled Ulf Hansson 2 siblings, 1 reply; 12+ messages in thread From: Ulf Hansson @ 2017-04-18 12:32 UTC (permalink / raw) To: linux-mmc, Ulf Hansson Cc: Jaehoon Chung, Adrian Hunter, Brian Norris, Shawn Lin, Doug Anderson For hosts not supporting MMC_CAP2_SDIO_IRQ_NOTHREAD but MMC_CAP_SDIO_IRQ, the SDIO IRQs are processed from a dedicated kernel thread. For these cases, the host calls mmc_signal_sdio_irq() from its ISR to signal a new SDIO IRQ. Signaling an SDIO IRQ makes the host's ->enable_sdio_irq() callback to be invoked to temporary disable the IRQs, before the kernel thread is woken up to process it. When processing of the IRQs are completed, they are re-enabled by the kernel thread, again via invoking the host's ->enable_sdio_irq(). The observation from this, is that the execution path is being unnecessary complex, as the host driver already knows that it needs to temporary disable the IRQs before signaling a new one. Moreover, replacing the kernel thread with a work/workqueue would greatly simplify the code. To address the above problems, let's continue to build upon the support for MMC_CAP2_SDIO_IRQ_NOTHREAD, as it already implements SDIO IRQs to be processed without using the clumsy kernel thread, but it also avoids the ping-ponging calls of the host's ->enable_sdio_irq() callback for each processed IRQ. Therefore, let's add new API sdio_signal_irq(), which enables hosts to signal/process SDIO IRQs by using a work/workqueue, rather than using the kernel thread. Add also a new host callback ->ack_sdio_irq(), which the work invokes when the SDIO IRQs are processed. This informs the host about when it can re-enable the SDIO IRQs. Potentially, we could re-use the existing ->enable_sdio_irq() callback for this matter, however it has turned out that it's more convenient for hosts to get this information via a separate callback. Hosts needs to enable MMC_CAP2_SDIO_IRQ_NOTHREAD to use this new feature, however the feature is optional for already existing hosts suppporting MMC_CAP2_SDIO_IRQ_NOTHREAD. It's likely that all host can convert to use MMC_CAP2_SDIO_IRQ_NOTHREAD and benefit from this feature. Further changes will have to tell. Until then the old path using the kernel thread remains possible. Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org> --- drivers/mmc/core/host.c | 2 ++ drivers/mmc/core/sdio_irq.c | 32 ++++++++++++++++++++++++++++++-- drivers/mmc/core/sdio_ops.h | 2 ++ include/linux/mmc/host.h | 3 +++ 4 files changed, 37 insertions(+), 2 deletions(-) diff --git a/drivers/mmc/core/host.c b/drivers/mmc/core/host.c index 3f8c85d..77058cb 100644 --- a/drivers/mmc/core/host.c +++ b/drivers/mmc/core/host.c @@ -30,6 +30,7 @@ #include "host.h" #include "slot-gpio.h" #include "pwrseq.h" +#include "sdio_ops.h" #define cls_dev_to_mmc_host(d) container_of(d, struct mmc_host, class_dev) @@ -379,6 +380,7 @@ struct mmc_host *mmc_alloc_host(int extra, struct device *dev) spin_lock_init(&host->lock); init_waitqueue_head(&host->wq); INIT_DELAYED_WORK(&host->detect, mmc_rescan); + INIT_WORK(&host->sdio_irq_work, sdio_irq_work); setup_timer(&host->retune_timer, mmc_retune_timer, (unsigned long)host); /* diff --git a/drivers/mmc/core/sdio_irq.c b/drivers/mmc/core/sdio_irq.c index 6d4b720..1b6006d 100644 --- a/drivers/mmc/core/sdio_irq.c +++ b/drivers/mmc/core/sdio_irq.c @@ -92,15 +92,43 @@ static int process_sdio_pending_irqs(struct mmc_host *host) return ret; } -void sdio_run_irqs(struct mmc_host *host) +static void __sdio_run_irqs(struct mmc_host *host) { - mmc_claim_host(host); host->sdio_irq_pending = true; process_sdio_pending_irqs(host); +} + +void sdio_run_irqs(struct mmc_host *host) +{ + mmc_claim_host(host); + __sdio_run_irqs(host); mmc_release_host(host); } EXPORT_SYMBOL_GPL(sdio_run_irqs); +void sdio_irq_work(struct work_struct *work) +{ + struct mmc_host *host = + container_of(work, struct mmc_host, sdio_irq_work); + + mmc_claim_host(host); + __sdio_run_irqs(host); + if (host->ops->ack_sdio_irq) + host->ops->ack_sdio_irq(host); + mmc_release_host(host); +} + +void sdio_signal_irq(struct mmc_host *host) +{ + /* + * The system_freezable_wq helps us to avoid processing IRQs while being + * system PM suspended. Instead these IRQs becomes deferred and managed + * when userspace is unfrozen. + */ + queue_work(system_freezable_wq, &host->sdio_irq_work); +} +EXPORT_SYMBOL_GPL(sdio_signal_irq); + static int sdio_irq_thread(void *_host) { struct mmc_host *host = _host; diff --git a/drivers/mmc/core/sdio_ops.h b/drivers/mmc/core/sdio_ops.h index bed8a83..836e405 100644 --- a/drivers/mmc/core/sdio_ops.h +++ b/drivers/mmc/core/sdio_ops.h @@ -17,6 +17,7 @@ struct mmc_host; struct mmc_card; +struct work_struct; int mmc_send_io_op_cond(struct mmc_host *host, u32 ocr, u32 *rocr); int mmc_io_rw_direct(struct mmc_card *card, int write, unsigned fn, @@ -25,6 +26,7 @@ int mmc_io_rw_extended(struct mmc_card *card, int write, unsigned fn, unsigned addr, int incr_addr, u8 *buf, unsigned blocks, unsigned blksz); int sdio_reset(struct mmc_host *host); unsigned int mmc_align_data_size(struct mmc_card *card, unsigned int sz); +void sdio_irq_work(struct work_struct *work); static inline bool mmc_is_io_op(u32 opcode) { diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h index 21385ac..f03df539 100644 --- a/include/linux/mmc/host.h +++ b/include/linux/mmc/host.h @@ -130,6 +130,7 @@ struct mmc_host_ops { int (*get_cd)(struct mmc_host *host); void (*enable_sdio_irq)(struct mmc_host *host, int enable); + void (*ack_sdio_irq)(struct mmc_host *host); /* optional callback for HC quirks */ void (*init_card)(struct mmc_host *host, struct mmc_card *card); @@ -358,6 +359,7 @@ struct mmc_host { unsigned int sdio_irqs; struct task_struct *sdio_irq_thread; + struct work_struct sdio_irq_work; bool sdio_irq_pending; atomic_t sdio_irq_thread_abort; @@ -428,6 +430,7 @@ static inline void mmc_signal_sdio_irq(struct mmc_host *host) } void sdio_run_irqs(struct mmc_host *host); +void sdio_signal_irq(struct mmc_host *host); #ifdef CONFIG_REGULATOR int mmc_regulator_get_ocrmask(struct regulator *supply); -- 2.7.4 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH 1/3] mmc: sdio: Add API to manage SDIO IRQs from a workqueue 2017-04-18 12:32 ` [PATCH 1/3] mmc: sdio: Add API to manage SDIO IRQs from a workqueue Ulf Hansson @ 2017-04-18 21:43 ` Doug Anderson 2017-04-19 10:48 ` Ulf Hansson 0 siblings, 1 reply; 12+ messages in thread From: Doug Anderson @ 2017-04-18 21:43 UTC (permalink / raw) To: Ulf Hansson Cc: linux-mmc, Jaehoon Chung, Adrian Hunter, Brian Norris, Shawn Lin Hi, On Tue, Apr 18, 2017 at 5:32 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote: > For hosts not supporting MMC_CAP2_SDIO_IRQ_NOTHREAD but MMC_CAP_SDIO_IRQ, > the SDIO IRQs are processed from a dedicated kernel thread. For these > cases, the host calls mmc_signal_sdio_irq() from its ISR to signal a new > SDIO IRQ. > > Signaling an SDIO IRQ makes the host's ->enable_sdio_irq() callback to be > invoked to temporary disable the IRQs, before the kernel thread is woken up > to process it. When processing of the IRQs are completed, they are > re-enabled by the kernel thread, again via invoking the host's > ->enable_sdio_irq(). > > The observation from this, is that the execution path is being unnecessary > complex, as the host driver already knows that it needs to temporary > disable the IRQs before signaling a new one. Moreover, replacing the kernel > thread with a work/workqueue would greatly simplify the code. > > To address the above problems, let's continue to build upon the support for > MMC_CAP2_SDIO_IRQ_NOTHREAD, as it already implements SDIO IRQs to be > processed without using the clumsy kernel thread, but it also avoids the > ping-ponging calls of the host's ->enable_sdio_irq() callback for each > processed IRQ. > > Therefore, let's add new API sdio_signal_irq(), which enables hosts to > signal/process SDIO IRQs by using a work/workqueue, rather than using the > kernel thread. > > Add also a new host callback ->ack_sdio_irq(), which the work invokes when > the SDIO IRQs are processed. This informs the host about when it can > re-enable the SDIO IRQs. Potentially, we could re-use the existing > ->enable_sdio_irq() callback for this matter, however it has turned out > that it's more convenient for hosts to get this information via a separate > callback. > > Hosts needs to enable MMC_CAP2_SDIO_IRQ_NOTHREAD to use this new feature, > however the feature is optional for already existing hosts suppporting > MMC_CAP2_SDIO_IRQ_NOTHREAD. > > It's likely that all host can convert to use MMC_CAP2_SDIO_IRQ_NOTHREAD and > benefit from this feature. Further changes will have to tell. Until then > the old path using the kernel thread remains possible. So one other subtle problem with the new approach is that you totally lose all of the polling logic in sdio_irq_thread(). ...so if I take your series and then comment out "cap-sdio-irq;" in the veyron "dtsi" then things stop working. Right now dw_mmc only enables SDIO Interrupts if that bit is set and relies on polling otherwise. Presumably there's not a _huge_ reason why you would need to make dw_mmc work without actual SDIO IRQ signaling, but the way the code is structured right now things will probably break for some users out there. One note is that I remember on exynos5250-snow that we needed to enable a hybrid interrupt/polling mechanism. The problem we ran into was terribly rare and it was never root caused if there was just some subtle bug or if certain versions of dw_mmc sometimes just dropped interrupts (and the patch was never upstreamed), so possibly we don't care. ...but having the polling code there as a fallback seems like it could have a benefit. ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/3] mmc: sdio: Add API to manage SDIO IRQs from a workqueue 2017-04-18 21:43 ` Doug Anderson @ 2017-04-19 10:48 ` Ulf Hansson 2017-04-19 19:29 ` Doug Anderson 0 siblings, 1 reply; 12+ messages in thread From: Ulf Hansson @ 2017-04-19 10:48 UTC (permalink / raw) To: Doug Anderson Cc: linux-mmc, Jaehoon Chung, Adrian Hunter, Brian Norris, Shawn Lin On 18 April 2017 at 23:43, Doug Anderson <dianders@google.com> wrote: > Hi, > > On Tue, Apr 18, 2017 at 5:32 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote: >> For hosts not supporting MMC_CAP2_SDIO_IRQ_NOTHREAD but MMC_CAP_SDIO_IRQ, >> the SDIO IRQs are processed from a dedicated kernel thread. For these >> cases, the host calls mmc_signal_sdio_irq() from its ISR to signal a new >> SDIO IRQ. >> >> Signaling an SDIO IRQ makes the host's ->enable_sdio_irq() callback to be >> invoked to temporary disable the IRQs, before the kernel thread is woken up >> to process it. When processing of the IRQs are completed, they are >> re-enabled by the kernel thread, again via invoking the host's >> ->enable_sdio_irq(). >> >> The observation from this, is that the execution path is being unnecessary >> complex, as the host driver already knows that it needs to temporary >> disable the IRQs before signaling a new one. Moreover, replacing the kernel >> thread with a work/workqueue would greatly simplify the code. >> >> To address the above problems, let's continue to build upon the support for >> MMC_CAP2_SDIO_IRQ_NOTHREAD, as it already implements SDIO IRQs to be >> processed without using the clumsy kernel thread, but it also avoids the >> ping-ponging calls of the host's ->enable_sdio_irq() callback for each >> processed IRQ. >> >> Therefore, let's add new API sdio_signal_irq(), which enables hosts to >> signal/process SDIO IRQs by using a work/workqueue, rather than using the >> kernel thread. >> >> Add also a new host callback ->ack_sdio_irq(), which the work invokes when >> the SDIO IRQs are processed. This informs the host about when it can >> re-enable the SDIO IRQs. Potentially, we could re-use the existing >> ->enable_sdio_irq() callback for this matter, however it has turned out >> that it's more convenient for hosts to get this information via a separate >> callback. >> >> Hosts needs to enable MMC_CAP2_SDIO_IRQ_NOTHREAD to use this new feature, >> however the feature is optional for already existing hosts suppporting >> MMC_CAP2_SDIO_IRQ_NOTHREAD. >> >> It's likely that all host can convert to use MMC_CAP2_SDIO_IRQ_NOTHREAD and >> benefit from this feature. Further changes will have to tell. Until then >> the old path using the kernel thread remains possible. > > So one other subtle problem with the new approach is that you totally > lose all of the polling logic in sdio_irq_thread(). The polling is still there, as I haven't removed the kthread in this series. I was also thinking of the next step, which could move the polling inside the work, simply by re-schedule itself. > > ...so if I take your series and then comment out "cap-sdio-irq;" in > the veyron "dtsi" then things stop working. Right now dw_mmc only > enables SDIO Interrupts if that bit is set and relies on polling > otherwise. Presumably there's not a _huge_ reason why you would need > to make dw_mmc work without actual SDIO IRQ signaling, but the way the > code is structured right now things will probably break for some users > out there. Did you actually test this or the conclusion was theoretical? I *was* actually thinking of the polling case and I think it should be addressed, unless I am missing something of course. More precisely, in patch2, I make sure MMC_CAP2_SDIO_IRQ_NOTHREAD becomes set for dw_mmc, only *if* MMC_CAP_SDIO_IRQ is set. That means the polling with the kthread is done for cases when MMC_CAP_SDIO_IRQ is unset. Right!? > > One note is that I remember on exynos5250-snow that we needed to > enable a hybrid interrupt/polling mechanism. The problem we ran into > was terribly rare and it was never root caused if there was just some > subtle bug or if certain versions of dw_mmc sometimes just dropped > interrupts (and the patch was never upstreamed), so possibly we don't > care. ...but having the polling code there as a fallback seems like > it could have a benefit. I see. To be clear, removing the polling is not my intent and isn't what the series tries to do. Kind regards Uffe ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/3] mmc: sdio: Add API to manage SDIO IRQs from a workqueue 2017-04-19 10:48 ` Ulf Hansson @ 2017-04-19 19:29 ` Doug Anderson 2017-04-20 12:14 ` Ulf Hansson 0 siblings, 1 reply; 12+ messages in thread From: Doug Anderson @ 2017-04-19 19:29 UTC (permalink / raw) To: Ulf Hansson Cc: linux-mmc, Jaehoon Chung, Adrian Hunter, Brian Norris, Shawn Lin Hi, On Wed, Apr 19, 2017 at 3:48 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote: > On 18 April 2017 at 23:43, Doug Anderson <dianders@google.com> wrote: >> Hi, >> >> On Tue, Apr 18, 2017 at 5:32 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote: >>> For hosts not supporting MMC_CAP2_SDIO_IRQ_NOTHREAD but MMC_CAP_SDIO_IRQ, >>> the SDIO IRQs are processed from a dedicated kernel thread. For these >>> cases, the host calls mmc_signal_sdio_irq() from its ISR to signal a new >>> SDIO IRQ. >>> >>> Signaling an SDIO IRQ makes the host's ->enable_sdio_irq() callback to be >>> invoked to temporary disable the IRQs, before the kernel thread is woken up >>> to process it. When processing of the IRQs are completed, they are >>> re-enabled by the kernel thread, again via invoking the host's >>> ->enable_sdio_irq(). >>> >>> The observation from this, is that the execution path is being unnecessary >>> complex, as the host driver already knows that it needs to temporary >>> disable the IRQs before signaling a new one. Moreover, replacing the kernel >>> thread with a work/workqueue would greatly simplify the code. >>> >>> To address the above problems, let's continue to build upon the support for >>> MMC_CAP2_SDIO_IRQ_NOTHREAD, as it already implements SDIO IRQs to be >>> processed without using the clumsy kernel thread, but it also avoids the >>> ping-ponging calls of the host's ->enable_sdio_irq() callback for each >>> processed IRQ. >>> >>> Therefore, let's add new API sdio_signal_irq(), which enables hosts to >>> signal/process SDIO IRQs by using a work/workqueue, rather than using the >>> kernel thread. >>> >>> Add also a new host callback ->ack_sdio_irq(), which the work invokes when >>> the SDIO IRQs are processed. This informs the host about when it can >>> re-enable the SDIO IRQs. Potentially, we could re-use the existing >>> ->enable_sdio_irq() callback for this matter, however it has turned out >>> that it's more convenient for hosts to get this information via a separate >>> callback. >>> >>> Hosts needs to enable MMC_CAP2_SDIO_IRQ_NOTHREAD to use this new feature, >>> however the feature is optional for already existing hosts suppporting >>> MMC_CAP2_SDIO_IRQ_NOTHREAD. >>> >>> It's likely that all host can convert to use MMC_CAP2_SDIO_IRQ_NOTHREAD and >>> benefit from this feature. Further changes will have to tell. Until then >>> the old path using the kernel thread remains possible. >> >> So one other subtle problem with the new approach is that you totally >> lose all of the polling logic in sdio_irq_thread(). > > The polling is still there, as I haven't removed the kthread in this series. The code still exists, but it won't be called, right? Oh! Shoot, I see that you only enable the new code in dw_mmc when the cap is set. Hrm. > I was also thinking of the next step, which could move the polling > inside the work, simply by re-schedule itself. > >> >> ...so if I take your series and then comment out "cap-sdio-irq;" in >> the veyron "dtsi" then things stop working. Right now dw_mmc only >> enables SDIO Interrupts if that bit is set and relies on polling >> otherwise. Presumably there's not a _huge_ reason why you would need >> to make dw_mmc work without actual SDIO IRQ signaling, but the way the >> code is structured right now things will probably break for some users >> out there. > > Did you actually test this or the conclusion was theoretical? I did, but I had confirmation bias so upon the first sign of failure I decided "I must be right--it doesn't work". :( Maybe something else was causing problems. Trying again now. OK, let's see: With "cap-sdio-irq" commented out but without your 3 patches: => Boot up, "ifconfig eth0 down; ping -c 20 8.8.8.8". Seems OK. With "cap-sdio-irq" commented out but _with_ your 3 patches: => Boot up, "ifconfig eth0 down; ping -c 20 8.8.8.8". Seems OK. So I guess the conclusion is that I missed the part about your patch only enabling the new features if MMC_CAP_SDIO_IRQ. Sorry. :( ...and then I must have hit some other unrelated failure that I can't reproduce now and assumed it was your patch's fault. So basically I would say that I've lightly tested your code. It's not code I've stressed a ton, but it survived some basic tests anyway... :) The code also looks pretty sane to me. > I *was* actually thinking of the polling case and I think it should be > addressed, unless I am missing something of course. > > More precisely, in patch2, I make sure MMC_CAP2_SDIO_IRQ_NOTHREAD > becomes set for dw_mmc, only *if* MMC_CAP_SDIO_IRQ is set. That means > the polling with the kthread is done for cases when MMC_CAP_SDIO_IRQ > is unset. Right!? Yeah, looks right to me now that I have my glasses on. >> One note is that I remember on exynos5250-snow that we needed to >> enable a hybrid interrupt/polling mechanism. The problem we ran into >> was terribly rare and it was never root caused if there was just some >> subtle bug or if certain versions of dw_mmc sometimes just dropped >> interrupts (and the patch was never upstreamed), so possibly we don't >> care. ...but having the polling code there as a fallback seems like >> it could have a benefit. > > I see. To be clear, removing the polling is not my intent and isn't > what the series tries to do. OK, makes sense. Just figured I'd mention this in case you had future plans around this code. :) ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/3] mmc: sdio: Add API to manage SDIO IRQs from a workqueue 2017-04-19 19:29 ` Doug Anderson @ 2017-04-20 12:14 ` Ulf Hansson 2017-04-28 20:31 ` Doug Anderson 0 siblings, 1 reply; 12+ messages in thread From: Ulf Hansson @ 2017-04-20 12:14 UTC (permalink / raw) To: Doug Anderson Cc: linux-mmc, Jaehoon Chung, Adrian Hunter, Brian Norris, Shawn Lin [...] > > With "cap-sdio-irq" commented out but without your 3 patches: > => Boot up, "ifconfig eth0 down; ping -c 20 8.8.8.8". Seems OK. > > With "cap-sdio-irq" commented out but _with_ your 3 patches: > => Boot up, "ifconfig eth0 down; ping -c 20 8.8.8.8". Seems OK. > > So I guess the conclusion is that I missed the part about your patch > only enabling the new features if MMC_CAP_SDIO_IRQ. Sorry. :( > ...and then I must have hit some other unrelated failure that I can't > reproduce now and assumed it was your patch's fault. > > So basically I would say that I've lightly tested your code. It's not > code I've stressed a ton, but it survived some basic tests anyway... > :) The code also looks pretty sane to me. Thanks a lot for your feedback a for running a new round of tests. This seems promising then! When you have the time, it we awesome if you could run yet another new round of test with the new version of the series. I posted it yesterday evening my local time. I would also be very interested to know if converting to the work queue approach has any impact on throughput. Maybe you have some simple test suite to also verify that? [...] Kind regards Uffe ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/3] mmc: sdio: Add API to manage SDIO IRQs from a workqueue 2017-04-20 12:14 ` Ulf Hansson @ 2017-04-28 20:31 ` Doug Anderson 0 siblings, 0 replies; 12+ messages in thread From: Doug Anderson @ 2017-04-28 20:31 UTC (permalink / raw) To: Ulf Hansson Cc: linux-mmc, Jaehoon Chung, Adrian Hunter, Brian Norris, Shawn Lin Ulf On Thu, Apr 20, 2017 at 5:14 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote: > [...] > >> >> With "cap-sdio-irq" commented out but without your 3 patches: >> => Boot up, "ifconfig eth0 down; ping -c 20 8.8.8.8". Seems OK. >> >> With "cap-sdio-irq" commented out but _with_ your 3 patches: >> => Boot up, "ifconfig eth0 down; ping -c 20 8.8.8.8". Seems OK. >> >> So I guess the conclusion is that I missed the part about your patch >> only enabling the new features if MMC_CAP_SDIO_IRQ. Sorry. :( >> ...and then I must have hit some other unrelated failure that I can't >> reproduce now and assumed it was your patch's fault. >> >> So basically I would say that I've lightly tested your code. It's not >> code I've stressed a ton, but it survived some basic tests anyway... >> :) The code also looks pretty sane to me. > > Thanks a lot for your feedback a for running a new round of tests. > This seems promising then! > > When you have the time, it we awesome if you could run yet another new > round of test with the new version of the series. I posted it > yesterday evening my local time. Sorry for taking so long to get to this. I've done this now and things seem to be working fine with your new series. As with before, I'm not doing really stressful testing here, but what I've done seems OK. > I would also be very interested to know if converting to the work > queue approach has any impact on throughput. Maybe you have some > simple test suite to also verify that? I don't personally. To really get something repeatable for WiFi, you need things in a chamber and need a test server setup. I did this quick test with the device sitting on my desk (with whatever the closest WiFi access point was), though: ifconfig eth0 down for i in $(seq 5); do curl http://SomeBigFile > /dev/null done >From this very simple test, the performance looks like a wash. -- Before your changes: % Total % Received % Xferd Average Speed Time Time Time Current Dload Upload Total Spent Left Speed 100 54.0M 100 54.0M 0 0 2753k 0 0:00:20 0:00:20 --:--:-- 2952k 100 54.0M 100 54.0M 0 0 1073k 0 0:00:51 0:00:51 --:--:-- 902k 100 54.0M 100 54.0M 0 0 2494k 0 0:00:22 0:00:22 --:--:-- 3061k 100 54.0M 100 54.0M 0 0 2455k 0 0:00:22 0:00:22 --:--:-- 2890k 100 54.0M 100 54.0M 0 0 1934k 0 0:00:28 0:00:28 --:--:-- 2301k After your changes: % Total % Received % Xferd Average Speed Time Time Time Current Dload Upload Total Spent Left Speed 100 54.0M 100 54.0M 0 0 2376k 0 0:00:23 0:00:23 --:--:-- 2739k 100 54.0M 100 54.0M 0 0 2303k 0 0:00:24 0:00:24 --:--:-- 2698k 100 54.0M 100 54.0M 0 0 2512k 0 0:00:22 0:00:22 --:--:-- 2359k 100 54.0M 100 54.0M 0 0 1774k 0 0:00:31 0:00:31 --:--:-- 1742k 100 54.0M 100 54.0M 0 0 2124k 0 0:00:26 0:00:26 --:--:-- 2753k -- It's possible that Brian Norris (CCed) might have an easy way to do a better test? -Doug ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 2/3] mmc: dw_mmc: Convert to use MMC_CAP2_SDIO_IRQ_NOTHREAD for SDIO IRQs 2017-04-18 12:32 [PATCH 0/3] mmc: Improve/fix support for SDIO IRQs Ulf Hansson 2017-04-18 12:32 ` [PATCH 1/3] mmc: sdio: Add API to manage SDIO IRQs from a workqueue Ulf Hansson @ 2017-04-18 12:32 ` Ulf Hansson 2017-04-18 21:25 ` Doug Anderson 2017-04-18 12:32 ` [PATCH 3/3] mmc: dw_mmc: Prevent runtime PM suspend when SDIO IRQs are enabled Ulf Hansson 2 siblings, 1 reply; 12+ messages in thread From: Ulf Hansson @ 2017-04-18 12:32 UTC (permalink / raw) To: linux-mmc, Ulf Hansson Cc: Jaehoon Chung, Adrian Hunter, Brian Norris, Shawn Lin, Doug Anderson Convert to use the more lightweight method for processing SDIO IRQs, which involves the following changes: - Enable MMC_CAP2_SDIO_IRQ_NOTHREAD when SDIO IRQ is supported. - Mask SDIO IRQ when signaling it for processing. - Re-enable (unmask) the SDIO IRQ from the ->ack_sdio_irq() callback. Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org> --- drivers/mmc/host/dw_mmc.c | 29 ++++++++++++++++++++++++++--- 1 file changed, 26 insertions(+), 3 deletions(-) diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c index 249ded6..f086791 100644 --- a/drivers/mmc/host/dw_mmc.c +++ b/drivers/mmc/host/dw_mmc.c @@ -1635,9 +1635,8 @@ static void dw_mci_init_card(struct mmc_host *mmc, struct mmc_card *card) } } -static void dw_mci_enable_sdio_irq(struct mmc_host *mmc, int enb) +static void __dw_mci_enable_sdio_irq(struct dw_mci_slot *slot, int enb) { - struct dw_mci_slot *slot = mmc_priv(mmc); struct dw_mci *host = slot->host; unsigned long irqflags; u32 int_mask; @@ -1655,6 +1654,20 @@ static void dw_mci_enable_sdio_irq(struct mmc_host *mmc, int enb) spin_unlock_irqrestore(&host->irq_lock, irqflags); } +static void dw_mci_enable_sdio_irq(struct mmc_host *mmc, int enb) +{ + struct dw_mci_slot *slot = mmc_priv(mmc); + + __dw_mci_enable_sdio_irq(slot, enb); +} + +static void dw_mci_ack_sdio_irq(struct mmc_host *mmc) +{ + struct dw_mci_slot *slot = mmc_priv(mmc); + + __dw_mci_enable_sdio_irq(slot, 1); +} + static int dw_mci_execute_tuning(struct mmc_host *mmc, u32 opcode) { struct dw_mci_slot *slot = mmc_priv(mmc); @@ -1756,6 +1769,7 @@ static const struct mmc_host_ops dw_mci_ops = { .get_cd = dw_mci_get_cd, .hw_reset = dw_mci_hw_reset, .enable_sdio_irq = dw_mci_enable_sdio_irq, + .ack_sdio_irq = dw_mci_ack_sdio_irq, .execute_tuning = dw_mci_execute_tuning, .card_busy = dw_mci_card_busy, .start_signal_voltage_switch = dw_mci_switch_voltage, @@ -2645,9 +2659,14 @@ static irqreturn_t dw_mci_interrupt(int irq, void *dev_id) continue; if (pending & SDMMC_INT_SDIO(slot->sdio_id)) { + u32 int_mask; + mci_writel(host, RINTSTS, SDMMC_INT_SDIO(slot->sdio_id)); - mmc_signal_sdio_irq(slot->mmc); + int_mask = mci_readl(host, INTMASK); + int_mask &= ~SDMMC_INT_SDIO(slot->sdio_id); + mci_writel(host, INTMASK, int_mask); + sdio_signal_irq(slot->mmc); } } @@ -2748,6 +2767,10 @@ static int dw_mci_init_slot(struct dw_mci *host, unsigned int id) if (ret) goto err_host_allocated; + /* Process SDIO IRQs through the sdio_irq_work. */ + if (mmc->caps & MMC_CAP_SDIO_IRQ) + mmc->caps2 |= MMC_CAP2_SDIO_IRQ_NOTHREAD; + /* Useful defaults if platform data is unset. */ if (host->use_dma == TRANS_MODE_IDMAC) { mmc->max_segs = host->ring_size; -- 2.7.4 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH 2/3] mmc: dw_mmc: Convert to use MMC_CAP2_SDIO_IRQ_NOTHREAD for SDIO IRQs 2017-04-18 12:32 ` [PATCH 2/3] mmc: dw_mmc: Convert to use MMC_CAP2_SDIO_IRQ_NOTHREAD for SDIO IRQs Ulf Hansson @ 2017-04-18 21:25 ` Doug Anderson 2017-04-19 12:10 ` Ulf Hansson 0 siblings, 1 reply; 12+ messages in thread From: Doug Anderson @ 2017-04-18 21:25 UTC (permalink / raw) To: Ulf Hansson Cc: linux-mmc, Jaehoon Chung, Adrian Hunter, Brian Norris, Shawn Lin Hi, On Tue, Apr 18, 2017 at 5:32 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote: > Convert to use the more lightweight method for processing SDIO IRQs, which > involves the following changes: > > - Enable MMC_CAP2_SDIO_IRQ_NOTHREAD when SDIO IRQ is supported. > - Mask SDIO IRQ when signaling it for processing. > - Re-enable (unmask) the SDIO IRQ from the ->ack_sdio_irq() callback. > > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org> > --- > drivers/mmc/host/dw_mmc.c | 29 ++++++++++++++++++++++++++--- > 1 file changed, 26 insertions(+), 3 deletions(-) > > diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c > index 249ded6..f086791 100644 > --- a/drivers/mmc/host/dw_mmc.c > +++ b/drivers/mmc/host/dw_mmc.c > @@ -1635,9 +1635,8 @@ static void dw_mci_init_card(struct mmc_host *mmc, struct mmc_card *card) > } > } > > -static void dw_mci_enable_sdio_irq(struct mmc_host *mmc, int enb) > +static void __dw_mci_enable_sdio_irq(struct dw_mci_slot *slot, int enb) > { > - struct dw_mci_slot *slot = mmc_priv(mmc); > struct dw_mci *host = slot->host; > unsigned long irqflags; > u32 int_mask; > @@ -1655,6 +1654,20 @@ static void dw_mci_enable_sdio_irq(struct mmc_host *mmc, int enb) > spin_unlock_irqrestore(&host->irq_lock, irqflags); > } > > +static void dw_mci_enable_sdio_irq(struct mmc_host *mmc, int enb) > +{ > + struct dw_mci_slot *slot = mmc_priv(mmc); > + > + __dw_mci_enable_sdio_irq(slot, enb); > +} > + > +static void dw_mci_ack_sdio_irq(struct mmc_host *mmc) > +{ > + struct dw_mci_slot *slot = mmc_priv(mmc); > + > + __dw_mci_enable_sdio_irq(slot, 1); I have some slight paranoia that some code out there might decide to call enable_sdio_irq(0) while an interrupt is being processed. In that case we'll be turning interrupts back on here. It seems like it would be "better safe than sorry" to keep track of the "enabled / disabled" state somewhere. ...and when we "unmask" we treat it as a no-op if the interrupt is currently disabled. > +} > + > static int dw_mci_execute_tuning(struct mmc_host *mmc, u32 opcode) > { > struct dw_mci_slot *slot = mmc_priv(mmc); > @@ -1756,6 +1769,7 @@ static const struct mmc_host_ops dw_mci_ops = { > .get_cd = dw_mci_get_cd, > .hw_reset = dw_mci_hw_reset, > .enable_sdio_irq = dw_mci_enable_sdio_irq, > + .ack_sdio_irq = dw_mci_ack_sdio_irq, > .execute_tuning = dw_mci_execute_tuning, > .card_busy = dw_mci_card_busy, > .start_signal_voltage_switch = dw_mci_switch_voltage, > @@ -2645,9 +2659,14 @@ static irqreturn_t dw_mci_interrupt(int irq, void *dev_id) > continue; > > if (pending & SDMMC_INT_SDIO(slot->sdio_id)) { > + u32 int_mask; > + > mci_writel(host, RINTSTS, > SDMMC_INT_SDIO(slot->sdio_id)); > - mmc_signal_sdio_irq(slot->mmc); > + int_mask = mci_readl(host, INTMASK); > + int_mask &= ~SDMMC_INT_SDIO(slot->sdio_id); > + mci_writel(host, INTMASK, int_mask); Seems like you should be calling "__dw_mci_enable_sdio_irq(slot, 0)" here. Specifically the interrupt handler won't have the spinlock so you're doing an unsafe read/modify/write here. Yeah, the spinlock is kinda silly in dw_mmc. Really the whole interrupt handling needs to be re-done to use a threaded IRQ instead of the current solution... > + sdio_signal_irq(slot->mmc); > } > } > > @@ -2748,6 +2767,10 @@ static int dw_mci_init_slot(struct dw_mci *host, unsigned int id) > if (ret) > goto err_host_allocated; > > + /* Process SDIO IRQs through the sdio_irq_work. */ > + if (mmc->caps & MMC_CAP_SDIO_IRQ) > + mmc->caps2 |= MMC_CAP2_SDIO_IRQ_NOTHREAD; > + > /* Useful defaults if platform data is unset. */ > if (host->use_dma == TRANS_MODE_IDMAC) { > mmc->max_segs = host->ring_size; > -- > 2.7.4 > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 2/3] mmc: dw_mmc: Convert to use MMC_CAP2_SDIO_IRQ_NOTHREAD for SDIO IRQs 2017-04-18 21:25 ` Doug Anderson @ 2017-04-19 12:10 ` Ulf Hansson 2017-04-19 18:39 ` Doug Anderson 0 siblings, 1 reply; 12+ messages in thread From: Ulf Hansson @ 2017-04-19 12:10 UTC (permalink / raw) To: Doug Anderson Cc: linux-mmc, Jaehoon Chung, Adrian Hunter, Brian Norris, Shawn Lin On 18 April 2017 at 23:25, Doug Anderson <dianders@google.com> wrote: > Hi, > > On Tue, Apr 18, 2017 at 5:32 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote: >> Convert to use the more lightweight method for processing SDIO IRQs, which >> involves the following changes: >> >> - Enable MMC_CAP2_SDIO_IRQ_NOTHREAD when SDIO IRQ is supported. >> - Mask SDIO IRQ when signaling it for processing. >> - Re-enable (unmask) the SDIO IRQ from the ->ack_sdio_irq() callback. >> >> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org> >> --- >> drivers/mmc/host/dw_mmc.c | 29 ++++++++++++++++++++++++++--- >> 1 file changed, 26 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c >> index 249ded6..f086791 100644 >> --- a/drivers/mmc/host/dw_mmc.c >> +++ b/drivers/mmc/host/dw_mmc.c >> @@ -1635,9 +1635,8 @@ static void dw_mci_init_card(struct mmc_host *mmc, struct mmc_card *card) >> } >> } >> >> -static void dw_mci_enable_sdio_irq(struct mmc_host *mmc, int enb) >> +static void __dw_mci_enable_sdio_irq(struct dw_mci_slot *slot, int enb) >> { >> - struct dw_mci_slot *slot = mmc_priv(mmc); >> struct dw_mci *host = slot->host; >> unsigned long irqflags; >> u32 int_mask; >> @@ -1655,6 +1654,20 @@ static void dw_mci_enable_sdio_irq(struct mmc_host *mmc, int enb) >> spin_unlock_irqrestore(&host->irq_lock, irqflags); >> } >> >> +static void dw_mci_enable_sdio_irq(struct mmc_host *mmc, int enb) >> +{ >> + struct dw_mci_slot *slot = mmc_priv(mmc); >> + >> + __dw_mci_enable_sdio_irq(slot, enb); >> +} >> + >> +static void dw_mci_ack_sdio_irq(struct mmc_host *mmc) >> +{ >> + struct dw_mci_slot *slot = mmc_priv(mmc); >> + >> + __dw_mci_enable_sdio_irq(slot, 1); > > I have some slight paranoia that some code out there might decide to > call enable_sdio_irq(0) while an interrupt is being processed. In > that case we'll be turning interrupts back on here. It seems like it > would be "better safe than sorry" to keep track of the "enabled / > disabled" state somewhere. ...and when we "unmask" we treat it as a > no-op if the interrupt is currently disabled. I understand your concern and your paranoia, which probably relates to the current tricky code that involves running our own kthread in sdio_irq_thread(). :-) For example, the sdio_irq_thread() need to release the host, mmc_release_host(), before it invokes ->enable_sdio_irq(), which is after it has processed the SDIO IRQs. This is actually wrong, as host drivers expects the host to be claimed when any of the host ops callbacks are being invoked, particularly from runtime PM point of view. Anyway, the current code *seems* to work - but for sure it's fragile and it has been so for too long. That said, you have a point about keeping track of the enabled/disable state. However, by digging a bit deeper into this, I realized the problem is actually even worse. Let me explain a bit more: ->ack_sdio_irq() is *only* called from the work that processes the SDIO IRQ. The difference compared to kthread is that the host is being claimed throughout the entire process when using the work, which by itself is an improvement. This also means, that the only reason to why ->enable_sdio_irq(0) can be called, is because an SDIO func driver decides to release the SDIO IRQ. However, for it to do that, it must first claim the host. This leads us to two scenarios: 1) The work manages to claim the host before the SDIO func driver. Then everything should be fine, simply because the work processes and acks the IRQ, before the SDIO func driver gets permission to release it. 2) The SDIO func driver gets to claim the host before the work. That means it releases the IRQ before the work gets permission to run and process the IRQ. This means we are into trouble. Not only as you say, ->enable_sdio_irq(0) becomes called before ->ack_sdio_irq(), but the actual processing of the IRQ, mmc_io_rw_direct() etc, becomes executed when it shouldn't. So, to fix the problems, I think a better solution than keeping track of the enabled/disabled state, is to actually prevent the IRQ from being processed in scenario 2. Including to prevent invoking ->ack_sdio_irq() from the work. Allow me to cook a separate patch for this, because I think this is already an existing problem when using MMC_CAP2_SDIO_IRQ_NOTHREAD. > >> +} >> + >> static int dw_mci_execute_tuning(struct mmc_host *mmc, u32 opcode) >> { >> struct dw_mci_slot *slot = mmc_priv(mmc); >> @@ -1756,6 +1769,7 @@ static const struct mmc_host_ops dw_mci_ops = { >> .get_cd = dw_mci_get_cd, >> .hw_reset = dw_mci_hw_reset, >> .enable_sdio_irq = dw_mci_enable_sdio_irq, >> + .ack_sdio_irq = dw_mci_ack_sdio_irq, >> .execute_tuning = dw_mci_execute_tuning, >> .card_busy = dw_mci_card_busy, >> .start_signal_voltage_switch = dw_mci_switch_voltage, >> @@ -2645,9 +2659,14 @@ static irqreturn_t dw_mci_interrupt(int irq, void *dev_id) >> continue; >> >> if (pending & SDMMC_INT_SDIO(slot->sdio_id)) { >> + u32 int_mask; >> + >> mci_writel(host, RINTSTS, >> SDMMC_INT_SDIO(slot->sdio_id)); >> - mmc_signal_sdio_irq(slot->mmc); >> + int_mask = mci_readl(host, INTMASK); >> + int_mask &= ~SDMMC_INT_SDIO(slot->sdio_id); >> + mci_writel(host, INTMASK, int_mask); > > Seems like you should be calling "__dw_mci_enable_sdio_irq(slot, 0)" > here. Specifically the interrupt handler won't have the spinlock so > you're doing an unsafe read/modify/write here. Yeah, the spinlock is > kinda silly in dw_mmc. Really the whole interrupt handling needs to > be re-done to use a threaded IRQ instead of the current solution... Thanks, this make sense! New version on its way. > >> + sdio_signal_irq(slot->mmc); >> } >> } >> >> @@ -2748,6 +2767,10 @@ static int dw_mci_init_slot(struct dw_mci *host, unsigned int id) >> if (ret) >> goto err_host_allocated; >> >> + /* Process SDIO IRQs through the sdio_irq_work. */ >> + if (mmc->caps & MMC_CAP_SDIO_IRQ) >> + mmc->caps2 |= MMC_CAP2_SDIO_IRQ_NOTHREAD; >> + >> /* Useful defaults if platform data is unset. */ >> if (host->use_dma == TRANS_MODE_IDMAC) { >> mmc->max_segs = host->ring_size; >> -- >> 2.7.4 >> Kind regards Uffe ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 2/3] mmc: dw_mmc: Convert to use MMC_CAP2_SDIO_IRQ_NOTHREAD for SDIO IRQs 2017-04-19 12:10 ` Ulf Hansson @ 2017-04-19 18:39 ` Doug Anderson 0 siblings, 0 replies; 12+ messages in thread From: Doug Anderson @ 2017-04-19 18:39 UTC (permalink / raw) To: Ulf Hansson Cc: linux-mmc, Jaehoon Chung, Adrian Hunter, Brian Norris, Shawn Lin Hi, On Wed, Apr 19, 2017 at 5:10 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote: > On 18 April 2017 at 23:25, Doug Anderson <dianders@google.com> wrote: >> Hi, >> >> On Tue, Apr 18, 2017 at 5:32 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote: >>> Convert to use the more lightweight method for processing SDIO IRQs, which >>> involves the following changes: >>> >>> - Enable MMC_CAP2_SDIO_IRQ_NOTHREAD when SDIO IRQ is supported. >>> - Mask SDIO IRQ when signaling it for processing. >>> - Re-enable (unmask) the SDIO IRQ from the ->ack_sdio_irq() callback. >>> >>> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org> >>> --- >>> drivers/mmc/host/dw_mmc.c | 29 ++++++++++++++++++++++++++--- >>> 1 file changed, 26 insertions(+), 3 deletions(-) >>> >>> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c >>> index 249ded6..f086791 100644 >>> --- a/drivers/mmc/host/dw_mmc.c >>> +++ b/drivers/mmc/host/dw_mmc.c >>> @@ -1635,9 +1635,8 @@ static void dw_mci_init_card(struct mmc_host *mmc, struct mmc_card *card) >>> } >>> } >>> >>> -static void dw_mci_enable_sdio_irq(struct mmc_host *mmc, int enb) >>> +static void __dw_mci_enable_sdio_irq(struct dw_mci_slot *slot, int enb) >>> { >>> - struct dw_mci_slot *slot = mmc_priv(mmc); >>> struct dw_mci *host = slot->host; >>> unsigned long irqflags; >>> u32 int_mask; >>> @@ -1655,6 +1654,20 @@ static void dw_mci_enable_sdio_irq(struct mmc_host *mmc, int enb) >>> spin_unlock_irqrestore(&host->irq_lock, irqflags); >>> } >>> >>> +static void dw_mci_enable_sdio_irq(struct mmc_host *mmc, int enb) >>> +{ >>> + struct dw_mci_slot *slot = mmc_priv(mmc); >>> + >>> + __dw_mci_enable_sdio_irq(slot, enb); >>> +} >>> + >>> +static void dw_mci_ack_sdio_irq(struct mmc_host *mmc) >>> +{ >>> + struct dw_mci_slot *slot = mmc_priv(mmc); >>> + >>> + __dw_mci_enable_sdio_irq(slot, 1); >> >> I have some slight paranoia that some code out there might decide to >> call enable_sdio_irq(0) while an interrupt is being processed. In >> that case we'll be turning interrupts back on here. It seems like it >> would be "better safe than sorry" to keep track of the "enabled / >> disabled" state somewhere. ...and when we "unmask" we treat it as a >> no-op if the interrupt is currently disabled. > > I understand your concern and your paranoia, which probably relates to > the current tricky code that involves running our own kthread in > sdio_irq_thread(). :-) > > For example, the sdio_irq_thread() need to release the host, > mmc_release_host(), before it invokes ->enable_sdio_irq(), which is > after it has processed the SDIO IRQs. This is actually wrong, as host > drivers expects the host to be claimed when any of the host ops > callbacks are being invoked, particularly from runtime PM point of > view. Yeah, I remember that causing problems in the past... ...but in general we can't assume that the host is claimed in enable_sdio_irq() because (historically) it's called directly from an IRQ. We can't claim the host from the IRQ.. > Anyway, the current code *seems* to work - but for sure it's fragile > and it has been so for too long. > > That said, you have a point about keeping track of the enabled/disable > state. However, by digging a bit deeper into this, I realized the > problem is actually even worse. Let me explain a bit more: > > ->ack_sdio_irq() is *only* called from the work that processes the > SDIO IRQ. The difference compared to kthread is that the host is being > claimed throughout the entire process when using the work, which by > itself is an improvement. This also means, that the only reason to why > ->enable_sdio_irq(0) can be called, is because an SDIO func driver > decides to release the SDIO IRQ. However, for it to do that, it must > first claim the host. It took me a little while to understand this, but I think you're talking about my paranoia case of the func driver tries to call sdio_release_irq() while an interrupt is pending? That could effectively call enable_sdio_irq(0). ...and if the work hasn't processed yet then we'll be in trouble. > This leads us to two scenarios: > 1) The work manages to claim the host before the SDIO func driver. > Then everything should be fine, simply because the work processes and > acks the IRQ, before the SDIO func driver gets permission to release > it. > > 2) The SDIO func driver gets to claim the host before the work. That > means it releases the IRQ before the work gets permission to run and > process the IRQ. This means we are into trouble. Not only as you say, > ->enable_sdio_irq(0) becomes called before ->ack_sdio_irq(), but the > actual processing of the IRQ, mmc_io_rw_direct() etc, becomes executed > when it shouldn't. > > So, to fix the problems, I think a better solution than keeping track > of the enabled/disabled state, is to actually prevent the IRQ from > being processed in scenario 2. Including to prevent invoking > ->ack_sdio_irq() from the work. > > Allow me to cook a separate patch for this, because I think this is > already an existing problem when using MMC_CAP2_SDIO_IRQ_NOTHREAD. Yeah, you're right that there could be more serious problems here if a host releases the IRQ while it's pending. Even with the fixes it still makes me nervous that we could be mixed up. If it were up to me I'd love to see at least some sort of warning if you "acked" a disabled interrupt, but I won't push for it if nobody else agrees. -Doug ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 3/3] mmc: dw_mmc: Prevent runtime PM suspend when SDIO IRQs are enabled 2017-04-18 12:32 [PATCH 0/3] mmc: Improve/fix support for SDIO IRQs Ulf Hansson 2017-04-18 12:32 ` [PATCH 1/3] mmc: sdio: Add API to manage SDIO IRQs from a workqueue Ulf Hansson 2017-04-18 12:32 ` [PATCH 2/3] mmc: dw_mmc: Convert to use MMC_CAP2_SDIO_IRQ_NOTHREAD for SDIO IRQs Ulf Hansson @ 2017-04-18 12:32 ` Ulf Hansson 2 siblings, 0 replies; 12+ messages in thread From: Ulf Hansson @ 2017-04-18 12:32 UTC (permalink / raw) To: linux-mmc, Ulf Hansson Cc: Jaehoon Chung, Adrian Hunter, Brian Norris, Shawn Lin, Doug Anderson To be able to handle SDIO IRQs the dw_mmc device needs to be powered and providing clock to the SDIO card. Therefore, we must not allow the device to be runtime PM suspended while SDIO IRQs are enabled. To fix this, let's increase the runtime PM usage count while the mmc core enables SDIO IRQs. Later when the mmc core tells dw_mmc to disable SDIO IRQs, we drop the usage count to again allow runtime PM suspend. This now becomes the default behaviour for dw_mmc. In cases where SDIO IRQs can be re-routed as GPIO wake-ups during runtime PM suspend, one could potentially allow runtime PM suspend. However, that will have to be addressed as a separate change on top of this one. Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org> --- drivers/mmc/host/dw_mmc.c | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c index f086791..4256957 100644 --- a/drivers/mmc/host/dw_mmc.c +++ b/drivers/mmc/host/dw_mmc.c @@ -38,6 +38,7 @@ #include <linux/of.h> #include <linux/of_gpio.h> #include <linux/mmc/slot-gpio.h> +#include <linux/pm_runtime.h> #include "dw_mmc.h" @@ -1657,8 +1658,15 @@ static void __dw_mci_enable_sdio_irq(struct dw_mci_slot *slot, int enb) static void dw_mci_enable_sdio_irq(struct mmc_host *mmc, int enb) { struct dw_mci_slot *slot = mmc_priv(mmc); + struct dw_mci *host = slot->host; __dw_mci_enable_sdio_irq(slot, enb); + + /* Avoid runtime suspending the device when SDIO IRQ is enabled */ + if (enb) + pm_runtime_get_noresume(host->dev); + else + pm_runtime_put_noidle(host->dev); } static void dw_mci_ack_sdio_irq(struct mmc_host *mmc) -- 2.7.4 ^ permalink raw reply related [flat|nested] 12+ messages in thread
end of thread, other threads:[~2017-04-28 20:31 UTC | newest] Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-04-18 12:32 [PATCH 0/3] mmc: Improve/fix support for SDIO IRQs Ulf Hansson 2017-04-18 12:32 ` [PATCH 1/3] mmc: sdio: Add API to manage SDIO IRQs from a workqueue Ulf Hansson 2017-04-18 21:43 ` Doug Anderson 2017-04-19 10:48 ` Ulf Hansson 2017-04-19 19:29 ` Doug Anderson 2017-04-20 12:14 ` Ulf Hansson 2017-04-28 20:31 ` Doug Anderson 2017-04-18 12:32 ` [PATCH 2/3] mmc: dw_mmc: Convert to use MMC_CAP2_SDIO_IRQ_NOTHREAD for SDIO IRQs Ulf Hansson 2017-04-18 21:25 ` Doug Anderson 2017-04-19 12:10 ` Ulf Hansson 2017-04-19 18:39 ` Doug Anderson 2017-04-18 12:32 ` [PATCH 3/3] mmc: dw_mmc: Prevent runtime PM suspend when SDIO IRQs are enabled Ulf Hansson
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.