2007-06-14 17:24:50

by Oleg Nesterov

Subject: w1_therm_read_bin: suspicious usage of flush_signals()

drivers/w1/slaves/w1_therm.c:w1_therm_read_bin()

while (tm) {
tm = msleep_interruptible(tm);
if (signal_pending(current))
flush_signals(current);
}

current is user-space task, yes?

this looks just wrong, could you please explain?

Oleg.

2007-06-15 04:29:27

by Evgeniy Polyakov

Subject: Re: w1_therm_read_bin: suspicious usage of flush_signals()

On Thu, Jun 14, 2007 at 09:24:39PM +0400, Oleg Nesterov ([email protected]) wrote:
> drivers/w1/slaves/w1_therm.c:w1_therm_read_bin()
>
> while (tm) {
> tm = msleep_interruptible(tm);
> if (signal_pending(current))
> flush_signals(current);
> }
>
> current is user-space task, yes?
>
> this looks just wrong, could you please explain?

Hi Oleg.

Well, it can be uninterruptible sleep, but why?
It is not allowed to return to userspace until transaction is completed,
so having uninterruptible sleep will result in exactly same lost of
signals.

> Oleg.

--
Evgeniy Polyakov

2007-06-15 05:10:32

by Roland McGrath

Subject: Re: w1_therm_read_bin: suspicious usage of flush_signals()

> Well, it can be uninterruptible sleep, but why?
> It is not allowed to return to userspace until transaction is completed,
> so having uninterruptible sleep will result in exactly same lost of
> signals.

Delay, not loss.

2007-06-15 05:53:19

by Evgeniy Polyakov

Subject: Re: w1_therm_read_bin: suspicious usage of flush_signals()

On Thu, Jun 14, 2007 at 10:10:07PM -0700, Roland McGrath ([email protected]) wrote:
> > Well, it can be uninterruptible sleep, but why?
> > It is not allowed to return to userspace until transaction is completed,
> > so having uninterruptible sleep will result in exactly same lost of
> > signals.
>
> Delay, not loss.

Yep, you are right. I missed that state checks are performed after
signal was queued.

I've chcked other usage of signals in w1 and they are only related to
the control thread and unloading time when signals are used only to
indicate that another check must be performed.
This patch resolves the issue with reading from temperature sensor.

Thanks for pointing to that issue.

Signed-off-by: Evgeniy Polyakov <[email protected]>

diff --git a/drivers/w1/slaves/w1_therm.c b/drivers/w1/slaves/w1_therm.c
index 732db47..1a6937d 100644
--- a/drivers/w1/slaves/w1_therm.c
+++ b/drivers/w1/slaves/w1_therm.c
@@ -191,11 +191,7 @@ static ssize_t w1_therm_read_bin(struct kobject *kobj, char *buf, loff_t off, si

w1_write_8(dev, W1_CONVERT_TEMP);

- while (tm) {
- tm = msleep_interruptible(tm);
- if (signal_pending(current))
- flush_signals(current);
- }
+ msleep(tm);

if (!w1_reset_select_slave(sl)) {



Signed-

--
Evgeniy Polyakov