| Message ID | 20181024084958.4627-3-kai.heng.feng@canonical.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Keep rtsx_usb suspended when there's no card | expand |
On 24 October 2018 at 10:49, Kai-Heng Feng <kai.heng.feng@canonical.com> wrote: > We can use MEMSTICK_POWER_{ON,OFF} along with pm_runtime_{get,put} > helpers to let memstick host support runtime pm. > > There's a small window between memstick_detect_change() and its queued > work, memstick_check(). In this window the rpm count may go down to zero > before the memstick host powers on, so the host can be inadvertently > suspended. > > Increment rpm count before calling memstick_check(), and decrement rpm > count afterward, as now we are sure the memstick host should be > suspended or not. > > Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com> > --- > drivers/memstick/core/memstick.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/drivers/memstick/core/memstick.c b/drivers/memstick/core/memstick.c > index 76382c858c35..5f16a8826401 100644 > --- a/drivers/memstick/core/memstick.c > +++ b/drivers/memstick/core/memstick.c > @@ -18,6 +18,7 @@ > #include <linux/delay.h> > #include <linux/slab.h> > #include <linux/module.h> > +#include <linux/pm_runtime.h> > > #define DRIVER_NAME "memstick" > > @@ -209,6 +210,7 @@ static int memstick_dummy_check(struct memstick_dev *card) > */ > void memstick_detect_change(struct memstick_host *host) > { > + pm_runtime_get_noresume(host->dev.parent); > queue_work(workqueue, &host->media_checker); > } > EXPORT_SYMBOL(memstick_detect_change); > @@ -479,6 +481,8 @@ static void memstick_check(struct work_struct *work) > host->set_param(host, MEMSTICK_POWER, MEMSTICK_POWER_OFF); > > mutex_unlock(&host->lock); > + > + pm_runtime_put(host->dev.parent); > dev_dbg(&host->dev, "memstick_check finished\n"); > } > I am not sure this works, sorry. More precisely, I don't think there is a guarantee that the calls to pm_runtime_get|put*() becomes properly balanced. In principle memstick_detect_change() could be called, without actually causing a new work to be scheduled if there is already such a work in the queue (depends on the workqueue configuration). Isn't it so? Kind regards Uffe
> On Oct 29, 2018, at 20:25, Ulf Hansson <ulf.hansson@linaro.org> wrote: > > On 24 October 2018 at 10:49, Kai-Heng Feng <kai.heng.feng@canonical.com> wrote: >> We can use MEMSTICK_POWER_{ON,OFF} along with pm_runtime_{get,put} >> helpers to let memstick host support runtime pm. >> >> There's a small window between memstick_detect_change() and its queued >> work, memstick_check(). In this window the rpm count may go down to zero >> before the memstick host powers on, so the host can be inadvertently >> suspended. >> >> Increment rpm count before calling memstick_check(), and decrement rpm >> count afterward, as now we are sure the memstick host should be >> suspended or not. >> >> Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com> >> --- >> drivers/memstick/core/memstick.c | 4 ++++ >> 1 file changed, 4 insertions(+) >> >> diff --git a/drivers/memstick/core/memstick.c b/drivers/memstick/core/memstick.c >> index 76382c858c35..5f16a8826401 100644 >> --- a/drivers/memstick/core/memstick.c >> +++ b/drivers/memstick/core/memstick.c >> @@ -18,6 +18,7 @@ >> #include <linux/delay.h> >> #include <linux/slab.h> >> #include <linux/module.h> >> +#include <linux/pm_runtime.h> >> >> #define DRIVER_NAME "memstick" >> >> @@ -209,6 +210,7 @@ static int memstick_dummy_check(struct memstick_dev *card) >> */ >> void memstick_detect_change(struct memstick_host *host) >> { >> + pm_runtime_get_noresume(host->dev.parent); >> queue_work(workqueue, &host->media_checker); >> } >> EXPORT_SYMBOL(memstick_detect_change); >> @@ -479,6 +481,8 @@ static void memstick_check(struct work_struct *work) >> host->set_param(host, MEMSTICK_POWER, MEMSTICK_POWER_OFF); >> >> mutex_unlock(&host->lock); >> + >> + pm_runtime_put(host->dev.parent); >> dev_dbg(&host->dev, "memstick_check finished\n"); >> } >> > > I am not sure this works, sorry. > > More precisely, I don't think there is a guarantee that the calls to > pm_runtime_get|put*() becomes properly balanced. In principle > memstick_detect_change() could be called, without actually causing a > new work to be scheduled if there is already such a work in the queue > (depends on the workqueue configuration). Isn't it so? You are right. We can use test_and_set_bit() or alike to properly balance pm_runtime helpers, but the most straightforward solution in my mind is to merge memstick_detect_change() and memstick_check() as one function. memstick_detect_change() it’s the only user of memstick_check() anyway. Or is there a better way in your mind? Kai-Heng > > Kind regards > Uffe
On 29 October 2018 at 17:31, Kai Heng Feng <kai.heng.feng@canonical.com> wrote: > > >> On Oct 29, 2018, at 20:25, Ulf Hansson <ulf.hansson@linaro.org> wrote: >> >> On 24 October 2018 at 10:49, Kai-Heng Feng <kai.heng.feng@canonical.com> wrote: >>> We can use MEMSTICK_POWER_{ON,OFF} along with pm_runtime_{get,put} >>> helpers to let memstick host support runtime pm. >>> >>> There's a small window between memstick_detect_change() and its queued >>> work, memstick_check(). In this window the rpm count may go down to zero >>> before the memstick host powers on, so the host can be inadvertently >>> suspended. >>> >>> Increment rpm count before calling memstick_check(), and decrement rpm >>> count afterward, as now we are sure the memstick host should be >>> suspended or not. >>> >>> Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com> >>> --- >>> drivers/memstick/core/memstick.c | 4 ++++ >>> 1 file changed, 4 insertions(+) >>> >>> diff --git a/drivers/memstick/core/memstick.c b/drivers/memstick/core/memstick.c >>> index 76382c858c35..5f16a8826401 100644 >>> --- a/drivers/memstick/core/memstick.c >>> +++ b/drivers/memstick/core/memstick.c >>> @@ -18,6 +18,7 @@ >>> #include <linux/delay.h> >>> #include <linux/slab.h> >>> #include <linux/module.h> >>> +#include <linux/pm_runtime.h> >>> >>> #define DRIVER_NAME "memstick" >>> >>> @@ -209,6 +210,7 @@ static int memstick_dummy_check(struct memstick_dev *card) >>> */ >>> void memstick_detect_change(struct memstick_host *host) >>> { >>> + pm_runtime_get_noresume(host->dev.parent); >>> queue_work(workqueue, &host->media_checker); >>> } >>> EXPORT_SYMBOL(memstick_detect_change); >>> @@ -479,6 +481,8 @@ static void memstick_check(struct work_struct *work) >>> host->set_param(host, MEMSTICK_POWER, MEMSTICK_POWER_OFF); >>> >>> mutex_unlock(&host->lock); >>> + >>> + pm_runtime_put(host->dev.parent); >>> dev_dbg(&host->dev, "memstick_check finished\n"); >>> } >>> >> >> I am not sure this works, sorry. >> >> More precisely, I don't think there is a guarantee that the calls to >> pm_runtime_get|put*() becomes properly balanced. In principle >> memstick_detect_change() could be called, without actually causing a >> new work to be scheduled if there is already such a work in the queue >> (depends on the workqueue configuration). Isn't it so? > > You are right. > > We can use test_and_set_bit() or alike to properly balance pm_runtime > helpers, but the most straightforward solution in my mind is to merge > memstick_detect_change() and memstick_check() as one function. > > memstick_detect_change() it’s the only user of memstick_check() anyway. I suspect memstick_detect_change() is supposed to be called by host drivers, when they receive some kind of notification due to a card being inserted or removed. I guess that happen (at least hypothetically) also from atomic (IRQ) context. As memstick_check() is doing hole bunch of operations, I am not sure bypassing the work-queue is a good idea, if that is what you are proposing. > > Or is there a better way in your mind? I don't know. Well, I am not sure I understand why you need to call pm_runtime_get_noresume() from memstick_detect_change() in the first place. Could you explain that in more detail? Kind regards Uffe
> On Oct 30, 2018, at 21:03, Ulf Hansson <ulf.hansson@linaro.org> wrote: > > On 29 October 2018 at 17:31, Kai Heng Feng <kai.heng.feng@canonical.com> wrote: >> >> >>> On Oct 29, 2018, at 20:25, Ulf Hansson <ulf.hansson@linaro.org> wrote: >>> >>> On 24 October 2018 at 10:49, Kai-Heng Feng <kai.heng.feng@canonical.com> wrote: >>>> We can use MEMSTICK_POWER_{ON,OFF} along with pm_runtime_{get,put} >>>> helpers to let memstick host support runtime pm. >>>> >>>> There's a small window between memstick_detect_change() and its queued >>>> work, memstick_check(). In this window the rpm count may go down to zero >>>> before the memstick host powers on, so the host can be inadvertently >>>> suspended. >>>> >>>> Increment rpm count before calling memstick_check(), and decrement rpm >>>> count afterward, as now we are sure the memstick host should be >>>> suspended or not. >>>> >>>> Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com> >>>> --- >>>> drivers/memstick/core/memstick.c | 4 ++++ >>>> 1 file changed, 4 insertions(+) >>>> >>>> diff --git a/drivers/memstick/core/memstick.c b/drivers/memstick/core/memstick.c >>>> index 76382c858c35..5f16a8826401 100644 >>>> --- a/drivers/memstick/core/memstick.c >>>> +++ b/drivers/memstick/core/memstick.c >>>> @@ -18,6 +18,7 @@ >>>> #include <linux/delay.h> >>>> #include <linux/slab.h> >>>> #include <linux/module.h> >>>> +#include <linux/pm_runtime.h> >>>> >>>> #define DRIVER_NAME "memstick" >>>> >>>> @@ -209,6 +210,7 @@ static int memstick_dummy_check(struct memstick_dev *card) >>>> */ >>>> void memstick_detect_change(struct memstick_host *host) >>>> { >>>> + pm_runtime_get_noresume(host->dev.parent); >>>> queue_work(workqueue, &host->media_checker); >>>> } >>>> EXPORT_SYMBOL(memstick_detect_change); >>>> @@ -479,6 +481,8 @@ static void memstick_check(struct work_struct *work) >>>> host->set_param(host, MEMSTICK_POWER, MEMSTICK_POWER_OFF); >>>> >>>> mutex_unlock(&host->lock); >>>> + >>>> + pm_runtime_put(host->dev.parent); >>>> dev_dbg(&host->dev, "memstick_check finished\n"); >>>> } >>>> >>> >>> I am not sure this works, sorry. >>> >>> More precisely, I don't think there is a guarantee that the calls to >>> pm_runtime_get|put*() becomes properly balanced. In principle >>> memstick_detect_change() could be called, without actually causing a >>> new work to be scheduled if there is already such a work in the queue >>> (depends on the workqueue configuration). Isn't it so? >> >> You are right. >> >> We can use test_and_set_bit() or alike to properly balance pm_runtime >> helpers, but the most straightforward solution in my mind is to merge >> memstick_detect_change() and memstick_check() as one function. >> >> memstick_detect_change() it’s the only user of memstick_check() anyway. > > I suspect memstick_detect_change() is supposed to be called by host > drivers, when they receive some kind of notification due to a card > being inserted or removed. I guess that happen (at least > hypothetically) also from atomic (IRQ) context. > > As memstick_check() is doing hole bunch of operations, I am not sure > bypassing the work-queue is a good idea, if that is what you are > proposing. Okay, it’s better to keep it that way. > >> >> Or is there a better way in your mind? > > I don't know. > > Well, I am not sure I understand why you need to call > pm_runtime_get_noresume() from memstick_detect_change() in the first > place. Could you explain that in more detail? I guess it didn’t explain it well enough in the log, let me add some detail: There's a small window between memstick_detect_change() and its queued work, memstick_check(). In this window the rpm count may go down to zero before the memstick host powers on, where I use pm_runtime_get_noresume() to increment the rpm count. memstick_check() uses some functions in rtsx_usb_ms that have pm_runtime_put*() so the rpm count may go down to zero, before the memstick host powers on. Kai-Heng > > Kind regards > Uffe
On 30 October 2018 at 16:23, Kai Heng Feng <kai.heng.feng@canonical.com> wrote: > > >> On Oct 30, 2018, at 21:03, Ulf Hansson <ulf.hansson@linaro.org> wrote: >> >> On 29 October 2018 at 17:31, Kai Heng Feng <kai.heng.feng@canonical.com> wrote: >>> >>> >>>> On Oct 29, 2018, at 20:25, Ulf Hansson <ulf.hansson@linaro.org> wrote: >>>> >>>> On 24 October 2018 at 10:49, Kai-Heng Feng <kai.heng.feng@canonical.com> wrote: >>>>> We can use MEMSTICK_POWER_{ON,OFF} along with pm_runtime_{get,put} >>>>> helpers to let memstick host support runtime pm. >>>>> >>>>> There's a small window between memstick_detect_change() and its queued >>>>> work, memstick_check(). In this window the rpm count may go down to zero >>>>> before the memstick host powers on, so the host can be inadvertently >>>>> suspended. >>>>> >>>>> Increment rpm count before calling memstick_check(), and decrement rpm >>>>> count afterward, as now we are sure the memstick host should be >>>>> suspended or not. >>>>> >>>>> Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com> >>>>> --- >>>>> drivers/memstick/core/memstick.c | 4 ++++ >>>>> 1 file changed, 4 insertions(+) >>>>> >>>>> diff --git a/drivers/memstick/core/memstick.c b/drivers/memstick/core/memstick.c >>>>> index 76382c858c35..5f16a8826401 100644 >>>>> --- a/drivers/memstick/core/memstick.c >>>>> +++ b/drivers/memstick/core/memstick.c >>>>> @@ -18,6 +18,7 @@ >>>>> #include <linux/delay.h> >>>>> #include <linux/slab.h> >>>>> #include <linux/module.h> >>>>> +#include <linux/pm_runtime.h> >>>>> >>>>> #define DRIVER_NAME "memstick" >>>>> >>>>> @@ -209,6 +210,7 @@ static int memstick_dummy_check(struct memstick_dev *card) >>>>> */ >>>>> void memstick_detect_change(struct memstick_host *host) >>>>> { >>>>> + pm_runtime_get_noresume(host->dev.parent); >>>>> queue_work(workqueue, &host->media_checker); >>>>> } >>>>> EXPORT_SYMBOL(memstick_detect_change); >>>>> @@ -479,6 +481,8 @@ static void memstick_check(struct work_struct *work) >>>>> host->set_param(host, MEMSTICK_POWER, MEMSTICK_POWER_OFF); >>>>> >>>>> mutex_unlock(&host->lock); >>>>> + >>>>> + pm_runtime_put(host->dev.parent); >>>>> dev_dbg(&host->dev, "memstick_check finished\n"); >>>>> } >>>>> >>>> >>>> I am not sure this works, sorry. >>>> >>>> More precisely, I don't think there is a guarantee that the calls to >>>> pm_runtime_get|put*() becomes properly balanced. In principle >>>> memstick_detect_change() could be called, without actually causing a >>>> new work to be scheduled if there is already such a work in the queue >>>> (depends on the workqueue configuration). Isn't it so? >>> >>> You are right. >>> >>> We can use test_and_set_bit() or alike to properly balance pm_runtime >>> helpers, but the most straightforward solution in my mind is to merge >>> memstick_detect_change() and memstick_check() as one function. >>> >>> memstick_detect_change() it’s the only user of memstick_check() anyway. >> >> I suspect memstick_detect_change() is supposed to be called by host >> drivers, when they receive some kind of notification due to a card >> being inserted or removed. I guess that happen (at least >> hypothetically) also from atomic (IRQ) context. >> >> As memstick_check() is doing hole bunch of operations, I am not sure >> bypassing the work-queue is a good idea, if that is what you are >> proposing. > > Okay, it’s better to keep it that way. > >> >>> >>> Or is there a better way in your mind? >> >> I don't know. >> >> Well, I am not sure I understand why you need to call >> pm_runtime_get_noresume() from memstick_detect_change() in the first >> place. Could you explain that in more detail? > > I guess it didn’t explain it well enough in the log, let me add some detail: > There's a small window between memstick_detect_change() and its queued > work, memstick_check(). In this window the rpm count may go down to zero > before the memstick host powers on, where I use > pm_runtime_get_noresume() to increment the rpm count. > > memstick_check() uses some functions in rtsx_usb_ms that have > pm_runtime_put*() so the rpm count may go down to zero, before the > memstick host powers on. So then, why doesn't memstick_check() early on calls pm_runtime_get_sync() and when it has finished with probing for a card, balance that with a call pm_runtime_put()? Kind regards Uffe
> On Oct 31, 2018, at 12:04 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote: > > On 30 October 2018 at 16:23, Kai Heng Feng <kai.heng.feng@canonical.com> wrote: >> >> >>> On Oct 30, 2018, at 21:03, Ulf Hansson <ulf.hansson@linaro.org> wrote: >>> >>> On 29 October 2018 at 17:31, Kai Heng Feng <kai.heng.feng@canonical.com> wrote: >>>> >>>> >>>>> On Oct 29, 2018, at 20:25, Ulf Hansson <ulf.hansson@linaro.org> wrote: >>>>> >>>>> On 24 October 2018 at 10:49, Kai-Heng Feng <kai.heng.feng@canonical.com> wrote: >>>>>> We can use MEMSTICK_POWER_{ON,OFF} along with pm_runtime_{get,put} >>>>>> helpers to let memstick host support runtime pm. >>>>>> >>>>>> There's a small window between memstick_detect_change() and its queued >>>>>> work, memstick_check(). In this window the rpm count may go down to zero >>>>>> before the memstick host powers on, so the host can be inadvertently >>>>>> suspended. >>>>>> >>>>>> Increment rpm count before calling memstick_check(), and decrement rpm >>>>>> count afterward, as now we are sure the memstick host should be >>>>>> suspended or not. >>>>>> >>>>>> Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com> >>>>>> --- >>>>>> drivers/memstick/core/memstick.c | 4 ++++ >>>>>> 1 file changed, 4 insertions(+) >>>>>> >>>>>> diff --git a/drivers/memstick/core/memstick.c b/drivers/memstick/core/memstick.c >>>>>> index 76382c858c35..5f16a8826401 100644 >>>>>> --- a/drivers/memstick/core/memstick.c >>>>>> +++ b/drivers/memstick/core/memstick.c >>>>>> @@ -18,6 +18,7 @@ >>>>>> #include <linux/delay.h> >>>>>> #include <linux/slab.h> >>>>>> #include <linux/module.h> >>>>>> +#include <linux/pm_runtime.h> >>>>>> >>>>>> #define DRIVER_NAME "memstick" >>>>>> >>>>>> @@ -209,6 +210,7 @@ static int memstick_dummy_check(struct memstick_dev *card) >>>>>> */ >>>>>> void memstick_detect_change(struct memstick_host *host) >>>>>> { >>>>>> + pm_runtime_get_noresume(host->dev.parent); >>>>>> queue_work(workqueue, &host->media_checker); >>>>>> } >>>>>> EXPORT_SYMBOL(memstick_detect_change); >>>>>> @@ -479,6 +481,8 @@ static void memstick_check(struct work_struct *work) >>>>>> host->set_param(host, MEMSTICK_POWER, MEMSTICK_POWER_OFF); >>>>>> >>>>>> mutex_unlock(&host->lock); >>>>>> + >>>>>> + pm_runtime_put(host->dev.parent); >>>>>> dev_dbg(&host->dev, "memstick_check finished\n"); >>>>>> } >>>>>> >>>>> >>>>> I am not sure this works, sorry. >>>>> >>>>> More precisely, I don't think there is a guarantee that the calls to >>>>> pm_runtime_get|put*() becomes properly balanced. In principle >>>>> memstick_detect_change() could be called, without actually causing a >>>>> new work to be scheduled if there is already such a work in the queue >>>>> (depends on the workqueue configuration). Isn't it so? >>>> >>>> You are right. >>>> >>>> We can use test_and_set_bit() or alike to properly balance pm_runtime >>>> helpers, but the most straightforward solution in my mind is to merge >>>> memstick_detect_change() and memstick_check() as one function. >>>> >>>> memstick_detect_change() it’s the only user of memstick_check() anyway. >>> >>> I suspect memstick_detect_change() is supposed to be called by host >>> drivers, when they receive some kind of notification due to a card >>> being inserted or removed. I guess that happen (at least >>> hypothetically) also from atomic (IRQ) context. >>> >>> As memstick_check() is doing hole bunch of operations, I am not sure >>> bypassing the work-queue is a good idea, if that is what you are >>> proposing. >> >> Okay, it’s better to keep it that way. >> >>> >>>> >>>> Or is there a better way in your mind? >>> >>> I don't know. >>> >>> Well, I am not sure I understand why you need to call >>> pm_runtime_get_noresume() from memstick_detect_change() in the first >>> place. Could you explain that in more detail? >> >> I guess it didn’t explain it well enough in the log, let me add some detail: >> There's a small window between memstick_detect_change() and its queued >> work, memstick_check(). In this window the rpm count may go down to zero >> before the memstick host powers on, where I use >> pm_runtime_get_noresume() to increment the rpm count. >> >> memstick_check() uses some functions in rtsx_usb_ms that have >> pm_runtime_put*() so the rpm count may go down to zero, before the >> memstick host powers on. > > So then, why doesn't memstick_check() early on calls > pm_runtime_get_sync() and when it has finished with probing for a > card, balance that with a call pm_runtime_put()? This will do, not sure what I was thinking. Thanks for pointing out. Kai-Heng > > Kind regards > Uffe
diff --git a/drivers/memstick/core/memstick.c b/drivers/memstick/core/memstick.c index 76382c858c35..5f16a8826401 100644 --- a/drivers/memstick/core/memstick.c +++ b/drivers/memstick/core/memstick.c @@ -18,6 +18,7 @@ #include <linux/delay.h> #include <linux/slab.h> #include <linux/module.h> +#include <linux/pm_runtime.h> #define DRIVER_NAME "memstick" @@ -209,6 +210,7 @@ static int memstick_dummy_check(struct memstick_dev *card) */ void memstick_detect_change(struct memstick_host *host) { + pm_runtime_get_noresume(host->dev.parent); queue_work(workqueue, &host->media_checker); } EXPORT_SYMBOL(memstick_detect_change); @@ -479,6 +481,8 @@ static void memstick_check(struct work_struct *work) host->set_param(host, MEMSTICK_POWER, MEMSTICK_POWER_OFF); mutex_unlock(&host->lock); + + pm_runtime_put(host->dev.parent); dev_dbg(&host->dev, "memstick_check finished\n"); }
We can use MEMSTICK_POWER_{ON,OFF} along with pm_runtime_{get,put} helpers to let memstick host support runtime pm. There's a small window between memstick_detect_change() and its queued work, memstick_check(). In this window the rpm count may go down to zero before the memstick host powers on, so the host can be inadvertently suspended. Increment rpm count before calling memstick_check(), and decrement rpm count afterward, as now we are sure the memstick host should be suspended or not. Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com> --- drivers/memstick/core/memstick.c | 4 ++++ 1 file changed, 4 insertions(+)