The driver may sleep under a spinlock.
The function call path is:
bdisp_device_run (acquire the spinlock)
bdisp_hw_reset
msleep --> may sleep
To fix it, msleep is replaced with mdelay.
This bug is found by my static analysis tool(DSAC) and checked by my code review.
Signed-off-by: Jia-Ju Bai <[email protected]>
---
drivers/media/platform/sti/bdisp/bdisp-hw.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/media/platform/sti/bdisp/bdisp-hw.c b/drivers/media/platform/sti/bdisp/bdisp-hw.c
index b7892f3..4b62ceb 100644
--- a/drivers/media/platform/sti/bdisp/bdisp-hw.c
+++ b/drivers/media/platform/sti/bdisp/bdisp-hw.c
@@ -382,7 +382,7 @@ int bdisp_hw_reset(struct bdisp_dev *bdisp)
for (i = 0; i < POLL_RST_MAX; i++) {
if (readl(bdisp->regs + BLT_STA1) & BLT_STA1_IDLE)
break;
- msleep(POLL_RST_DELAY_MS);
+ mdelay(POLL_RST_DELAY_MS);
}
if (i == POLL_RST_MAX)
dev_err(bdisp->dev, "Reset timeout\n");
--
1.7.9.5
Fabien or Benjamin, can you take a look at these two patches?
I'm a bit hesitant applying this since e.g. this bdisp_hw_reset() function might wait
for up to a second, which is a mite long for an interrupt :-)
Regards,
Hans
On 12/12/17 14:47, Jia-Ju Bai wrote:
> The driver may sleep under a spinlock.
> The function call path is:
> bdisp_device_run (acquire the spinlock)
> bdisp_hw_reset
> msleep --> may sleep
>
> To fix it, msleep is replaced with mdelay.
>
> This bug is found by my static analysis tool(DSAC) and checked by my code review.
>
> Signed-off-by: Jia-Ju Bai <[email protected]>
> ---
> drivers/media/platform/sti/bdisp/bdisp-hw.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/media/platform/sti/bdisp/bdisp-hw.c b/drivers/media/platform/sti/bdisp/bdisp-hw.c
> index b7892f3..4b62ceb 100644
> --- a/drivers/media/platform/sti/bdisp/bdisp-hw.c
> +++ b/drivers/media/platform/sti/bdisp/bdisp-hw.c
> @@ -382,7 +382,7 @@ int bdisp_hw_reset(struct bdisp_dev *bdisp)
> for (i = 0; i < POLL_RST_MAX; i++) {
> if (readl(bdisp->regs + BLT_STA1) & BLT_STA1_IDLE)
> break;
> - msleep(POLL_RST_DELAY_MS);
> + mdelay(POLL_RST_DELAY_MS);
> }
> if (i == POLL_RST_MAX)
> dev_err(bdisp->dev, "Reset timeout\n");
>
Hi
On 12/12/17 14:47, Jia-Ju Bai wrote:
> The driver may sleep under a spinlock.
> The function call path is:
> bdisp_device_run (acquire the spinlock)
> bdisp_hw_reset
> msleep --> may sleep
>
> To fix it, msleep is replaced with mdelay.
May I suggest you to use readl_poll_timeout_atomic (instead of the whole
"for" block): this fixes the problem and simplifies the code?
>
> This bug is found by my static analysis tool(DSAC) and checked by my code review.
>
> Signed-off-by: Jia-Ju Bai <[email protected]>
> ---
> drivers/media/platform/sti/bdisp/bdisp-hw.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/media/platform/sti/bdisp/bdisp-hw.c b/drivers/media/platform/sti/bdisp/bdisp-hw.c
> index b7892f3..4b62ceb 100644
> --- a/drivers/media/platform/sti/bdisp/bdisp-hw.c
> +++ b/drivers/media/platform/sti/bdisp/bdisp-hw.c
> @@ -382,7 +382,7 @@ int bdisp_hw_reset(struct bdisp_dev *bdisp)
> for (i = 0; i < POLL_RST_MAX; i++) {
> if (readl(bdisp->regs + BLT_STA1) & BLT_STA1_IDLE)
> break;
> - msleep(POLL_RST_DELAY_MS);
> + mdelay(POLL_RST_DELAY_MS);
> }
> if (i == POLL_RST_MAX)
> dev_err(bdisp->dev, "Reset timeout\n");
Hi,
On 2017/12/15 22:51, Fabien DESSENNE wrote:
> Hi
>
> On 12/12/17 14:47, Jia-Ju Bai wrote:
>> The driver may sleep under a spinlock.
>> The function call path is:
>> bdisp_device_run (acquire the spinlock)
>> bdisp_hw_reset
>> msleep --> may sleep
>>
>> To fix it, msleep is replaced with mdelay.
> May I suggest you to use readl_poll_timeout_atomic (instead of the whole
> "for" block): this fixes the problem and simplifies the code?
Okay, I have submitted a patch according to your advice.
You can have a look :)
Thanks,
Jia-Ju Bai
>> This bug is found by my static analysis tool(DSAC) and checked by my code review.
>>
>> Signed-off-by: Jia-Ju Bai <[email protected]>
>> ---
>> drivers/media/platform/sti/bdisp/bdisp-hw.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/media/platform/sti/bdisp/bdisp-hw.c b/drivers/media/platform/sti/bdisp/bdisp-hw.c
>> index b7892f3..4b62ceb 100644
>> --- a/drivers/media/platform/sti/bdisp/bdisp-hw.c
>> +++ b/drivers/media/platform/sti/bdisp/bdisp-hw.c
>> @@ -382,7 +382,7 @@ int bdisp_hw_reset(struct bdisp_dev *bdisp)
>> for (i = 0; i < POLL_RST_MAX; i++) {
>> if (readl(bdisp->regs + BLT_STA1) & BLT_STA1_IDLE)
>> break;
>> - msleep(POLL_RST_DELAY_MS);
>> + mdelay(POLL_RST_DELAY_MS);
>> }
>> if (i == POLL_RST_MAX)
>> dev_err(bdisp->dev, "Reset timeout\n");
Em Sat, 16 Dec 2017 19:53:55 +0800
Jia-Ju Bai <[email protected]> escreveu:
> Hi,
>
> On 2017/12/15 22:51, Fabien DESSENNE wrote:
> > Hi
> >
> > On 12/12/17 14:47, Jia-Ju Bai wrote:
> >> The driver may sleep under a spinlock.
> >> The function call path is:
> >> bdisp_device_run (acquire the spinlock)
> >> bdisp_hw_reset
> >> msleep --> may sleep
> >>
> >> To fix it, msleep is replaced with mdelay.
> > May I suggest you to use readl_poll_timeout_atomic (instead of the whole
> > "for" block): this fixes the problem and simplifies the code?
>
> Okay, I have submitted a patch according to your advice.
> You can have a look :)
This can still be usind mdelay() to wait for a long time.
It doesn't seem wise to do that, as it could cause system
contention. Couldn't this be reworked in a way to avoid
having the spin locked while sleeping?
Once we had a similar issue on Siano, and it was solved by this
commit 3cdadc50bbe8f04c1231c8af614cafd7ddd622bf
Author: Richard Zidlicky <[email protected]>
Date: Tue Aug 24 09:52:36 2010 -0300
V4L/DVB: dvb: fix smscore_getbuffer() logic
Drivers shouldn't sleep while holding a spinlock. A previous workaround
were to release the spinlock before callinc schedule().
This patch uses a different approach: it just waits for the
siano hardware to answer.
Signed-off-by: Richard Zidlicky <[email protected]>
Cc: [email protected]
Signed-off-by: Mauro Carvalho Chehab <[email protected]>
The code as changed to use wait_event() at the kthread that was
waiting for data to arrive. Only when the data is ready, the
code with the spin lock is called.
It made the driver a way more stable, and didn't add any penalties
of needing to do long delays on a non-interruptible code.
Thanks,
Mauro
On 16/12/17 15:14, Mauro Carvalho Chehab wrote:
> Em Sat, 16 Dec 2017 19:53:55 +0800
> Jia-Ju Bai <[email protected]> escreveu:
>
>> Hi,
>>
>> On 2017/12/15 22:51, Fabien DESSENNE wrote:
>>> Hi
>>>
>>> On 12/12/17 14:47, Jia-Ju Bai wrote:
>>>> The driver may sleep under a spinlock.
>>>> The function call path is:
>>>> bdisp_device_run (acquire the spinlock)
>>>> bdisp_hw_reset
>>>> msleep --> may sleep
>>>>
>>>> To fix it, msleep is replaced with mdelay.
>>> May I suggest you to use readl_poll_timeout_atomic (instead of the whole
>>> "for" block): this fixes the problem and simplifies the code?
>> Okay, I have submitted a patch according to your advice.
>> You can have a look :)
> This can still be usind mdelay() to wait for a long time.
>
> It doesn't seem wise to do that, as it could cause system
> contention. Couldn't this be reworked in a way to avoid
> having the spin locked while sleeping?
>
> Once we had a similar issue on Siano, and it was solved by this
>
> commit 3cdadc50bbe8f04c1231c8af614cafd7ddd622bf
> Author: Richard Zidlicky <[email protected]>
> Date: Tue Aug 24 09:52:36 2010 -0300
>
> V4L/DVB: dvb: fix smscore_getbuffer() logic
>
> Drivers shouldn't sleep while holding a spinlock. A previous workaround
> were to release the spinlock before callinc schedule().
>
> This patch uses a different approach: it just waits for the
> siano hardware to answer.
>
> Signed-off-by: Richard Zidlicky <[email protected]>
> Cc: [email protected]
> Signed-off-by: Mauro Carvalho Chehab <[email protected]>
>
> The code as changed to use wait_event() at the kthread that was
> waiting for data to arrive. Only when the data is ready, the
> code with the spin lock is called.
>
> It made the driver a way more stable, and didn't add any penalties
> of needing to do long delays on a non-interruptible code.
>
> Thanks,
> Mauro
I have checked what was done there but I cannot see a simple way to do
the same in bdisp where the context is a bit different (the lock is
taken out in the central device_run, not locally in hw_reset) without
taking the risk to have unexpected side effects
Moreover, the bdisp_hw_reset() function called from bdisp_device_run is
not expected to last for a long time. The "one second" delay we are
talking about is a very large timeout protection. From my past
observations, the reset is applied instantly and we even never reach the
msleep() call (not saying it never happens).
For those two reasons, using readl_poll_timeout_atomic() seems to be the
best option.
BR
Fabien
Em Tue, 19 Dec 2017 09:01:41 +0000
Fabien DESSENNE <[email protected]> escreveu:
> On 16/12/17 15:14, Mauro Carvalho Chehab wrote:
> > Em Sat, 16 Dec 2017 19:53:55 +0800
> > Jia-Ju Bai <[email protected]> escreveu:
> >
> >> Hi,
> >>
> >> On 2017/12/15 22:51, Fabien DESSENNE wrote:
> >>> Hi
> >>>
> >>> On 12/12/17 14:47, Jia-Ju Bai wrote:
> >>>> The driver may sleep under a spinlock.
> >>>> The function call path is:
> >>>> bdisp_device_run (acquire the spinlock)
> >>>> bdisp_hw_reset
> >>>> msleep --> may sleep
> >>>>
> >>>> To fix it, msleep is replaced with mdelay.
> >>> May I suggest you to use readl_poll_timeout_atomic (instead of the whole
> >>> "for" block): this fixes the problem and simplifies the code?
> >> Okay, I have submitted a patch according to your advice.
> >> You can have a look :)
> > This can still be usind mdelay() to wait for a long time.
> >
> > It doesn't seem wise to do that, as it could cause system
> > contention. Couldn't this be reworked in a way to avoid
> > having the spin locked while sleeping?
> >
> > Once we had a similar issue on Siano, and it was solved by this
> >
> > commit 3cdadc50bbe8f04c1231c8af614cafd7ddd622bf
> > Author: Richard Zidlicky <[email protected]>
> > Date: Tue Aug 24 09:52:36 2010 -0300
> >
> > V4L/DVB: dvb: fix smscore_getbuffer() logic
> >
> > Drivers shouldn't sleep while holding a spinlock. A previous workaround
> > were to release the spinlock before callinc schedule().
> >
> > This patch uses a different approach: it just waits for the
> > siano hardware to answer.
> >
> > Signed-off-by: Richard Zidlicky <[email protected]>
> > Cc: [email protected]
> > Signed-off-by: Mauro Carvalho Chehab <[email protected]>
> >
> > The code as changed to use wait_event() at the kthread that was
> > waiting for data to arrive. Only when the data is ready, the
> > code with the spin lock is called.
> >
> > It made the driver a way more stable, and didn't add any penalties
> > of needing to do long delays on a non-interruptible code.
> >
> > Thanks,
> > Mauro
> I have checked what was done there but I cannot see a simple way to do
> the same in bdisp where the context is a bit different (the lock is
> taken out in the central device_run, not locally in hw_reset) without
> taking the risk to have unexpected side effects
>
> Moreover, the bdisp_hw_reset() function called from bdisp_device_run is
> not expected to last for a long time. The "one second" delay we are
> talking about is a very large timeout protection. From my past
> observations, the reset is applied instantly and we even never reach the
> msleep() call (not saying it never happens).
>
> For those two reasons, using readl_poll_timeout_atomic() seems to be the
> best option.
OK. The best is then to document it at the source code, for others
to be aware, while reviewing the code, that, despite the large timeout,
most of the time the reset happens without needing any delays.
Thanks,
Mauro