[PATCH] fb: udlfb: fix hang at disconnect

[PATCH] fb: udlfb: fix hang at disconnect

[Alexander Holler]

When a device was disconnected the driver may hang at waiting for urbs it never
will get. Fix this by using a timeout while waiting for the used semaphore.

There is still a memory leak if a timeout happens, but at least the driver
now continues his disconnect routine.

Cc: <stable@vger.kernel.org>
Signed-off-by: Alexander Holler <holler@ahsoftware.de>
---
 drivers/video/udlfb.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/video/udlfb.c b/drivers/video/udlfb.c
index 86d449e..cc4a8d1 100644
--- a/drivers/video/udlfb.c
+++ b/drivers/video/udlfb.c
@@ -1832,8 +1832,8 @@ static void dlfb_free_urb_list(struct dlfb_data *dev)
 	/* keep waiting and freeing, until we've got 'em all */
 	while (count--) {
 
-		/* Getting interrupted means a leak, but ok at disconnect */
-		ret = down_interruptible(&dev->urbs.limit_sem);
+		/* Timeout likely occurs at disconnect (resulting in a leak) */
+		ret = down_timeout(&dev->urbs.limit_sem, GET_URB_TIMEOUT);
 		if (ret)
 			break;
 
-- 
1.7.11.7

Re: [PATCH] fb: udlfb: fix hang at disconnect

[Bernie Thompson]

Hi Alexander,

On Sat, Jan 12, 2013 at 5:20 AM, Alexander Holler <holler@ahsoftware.de> wrote:
> When a device was disconnected the driver may hang at waiting for urbs it never
> will get. Fix this by using a timeout while waiting for the used semaphore.

The code used to be this way, but it used to cause nasty shutdown hangs:
http://git.plugable.com/gitphp/index.php?p=udlfb&a=commitdiff&h=1dd39a65001deb5a84088dfabb788d3274fbb6b6

Which is why the code is the way it is today.

Can you say under what situations you're hitting hangs on device
disconnect?  Have you tested extensively to confirm no shutdown hangs
with your patch?

Stepping back, there was another recent patch from the community to
udlfb to work around issues of sleeping in the wrong context. The fix
involved introducing another scheduled workitem. This slows everything
down when it's in the main path, and isn't really desirable if we can
avoid it.

Another option to eliminate all these problems -- long considered but
never implemented -- is to get rid of all semaphores and potential
sleeps in udlfb entirely.  That would require a strategy to throttle
rendering in some way other than by waiting in kernel (without some
throttling strategy, the USB bus can be a bottleneck which can flood
the system with rendered but untransmitted pixels).

Options might be:

1) When transfer buffers are full, keep track of dirty rectangles for
the rest and pick up where we left off the next time we're entered
(avoiding flooding by potentially having pixels in the dirty regions
be written over multiple times before we get to rendering them once)

2 ) If we "bet" on page-fault-based defio dirty pixel detection, we
could allocate buffers dynamically but increase the scheduling time to
transfer as our outstanding buffer count grows, and reduce the latency
only when the buffer count goes down (again, pixels will be
potentially rendered many times before being transfered once, avoiding
flooding).

Any other ideas on the specific or general case are welcome.  Also
note that udlfb is being largely superceeded by the udl DRM driver -
so any decisions here should also be considered in that codebase.

In any case, thanks for giving the DisplayLink USB 2.0 graphics
drivers attention - it's much appreciated!

Bernie Thompson
http://plugable.com/

Re: [PATCH] fb: udlfb: fix hang at disconnect

[Alexander Holler]

Am 12.01.2013 23:22, schrieb Bernie Thompson:
> Hi Alexander,
>
> On Sat, Jan 12, 2013 at 5:20 AM, Alexander Holler <holler@ahsoftware.de> wrote:
>> When a device was disconnected the driver may hang at waiting for urbs it never
>> will get. Fix this by using a timeout while waiting for the used semaphore.
>
> The code used to be this way, but it used to cause nasty shutdown hangs:
> http://git.plugable.com/gitphp/index.php?p=udlfb&a=commitdiff&h=1dd39a65001deb5a84088dfabb788d3274fbb6b6
>
> Which is why the code is the way it is today.
>
> Can you say under what situations you're hitting hangs on device
> disconnect?  Have you tested extensively to confirm no shutdown hangs
> with your patch?
>

The driver almost always (2/3) hangs here when the device gets 
disconnected. It is easy to see when the device gets attached again as 
nothing will happen if the driver (already) hangs (in addition that a 
shutdown isn't possible).

I didn't test it extensively, but without the patch the driver isn't 
usable here. Maybe my previous patch which moves damages to a workqueue 
is the reason that it's more likely that urbs get missing, but the 
problem already existed because an urb might get missed on disconnect. I 
don't know what problems existed before, maybe people just had a problem 
with the BUG_ON(ret). If that _interrupted_ is really needed, it could 
make sense to implement a down_timeout_interruptible() for semaphores.

> Stepping back, there was another recent patch from the community to
> udlfb to work around issues of sleeping in the wrong context. The fix
> involved introducing another scheduled workitem. This slows everything
> down when it's in the main path, and isn't really desirable if we can
> avoid it.

Do you mean the one I've recently posted? It is needed, at least for 3.7 
(I don't know since when those "schedule while atomic" messages appear).
It might slow down refreshes, but it is needed, at least until someone 
gets around those semaphores or removes the spinlocks in upper layers 
(as Alan Cox suggested with the "I am crap" helper for printk).

Maybe using a WQ_HIGHPRI for the workqueue with the damages will speed 
up things.

More optimizations might be doable too (e.g. combining multiple queued 
damages).

> Another option to eliminate all these problems -- long considered but
> never implemented -- is to get rid of all semaphores and potential
> sleeps in udlfb entirely.  That would require a strategy to throttle
> rendering in some way other than by waiting in kernel (without some
> throttling strategy, the USB bus can be a bottleneck which can flood
> the system with rendered but untransmitted pixels).
>
> Options might be:
>
> 1) When transfer buffers are full, keep track of dirty rectangles for
> the rest and pick up where we left off the next time we're entered
> (avoiding flooding by potentially having pixels in the dirty regions
> be written over multiple times before we get to rendering them once)
>
> 2 ) If we "bet" on page-fault-based defio dirty pixel detection, we
> could allocate buffers dynamically but increase the scheduling time to
> transfer as our outstanding buffer count grows, and reduce the latency
> only when the buffer count goes down (again, pixels will be
> potentially rendered many times before being transfered once, avoiding
> flooding).
>
> Any other ideas on the specific or general case are welcome.  Also
> note that udlfb is being largely superceeded by the udl DRM driver -
> so any decisions here should also be considered in that codebase.
>
> In any case, thanks for giving the DisplayLink USB 2.0 graphics
> drivers attention - it's much appreciated!

Thanks for the sugestions, but I don't feel the need to spend a lot of 
time here. I just wanted to use the console with the device and a kernel 
3.7.x and neither udlfb nor udl currently worked (and I'm pretty sure 
I've used one of them some time before, likely udlfb).

Btw, to see the console again after a disconnect and connect, I'm 
currently using the following (necessary) quick&dirty hack:

---------
         /* if clients still have us open, will be freed on last close */
-       if (dev->fb_count == 0)
+//     if (dev->fb_count == 0)
                 schedule_delayed_work(&dev->free_framebuffer_work, 0);
---------

Without that the framebuffer will never get unregistered (because just 
unlinking it doesn't remove the fb-console which counts for one client) 
with the result that the new one (after connecting the device again) 
will not get the console.

Regards,

Alexander

Re: [PATCH] fb: udlfb: fix hang at disconnect

[Alexander Holler]

Am 13.01.2013 13:05, schrieb Alexander Holler:
> Am 12.01.2013 23:22, schrieb Bernie Thompson:

> I didn't test it extensively, but without the patch the driver isn't
> usable here. Maybe my previous patch which moves damages to a workqueue

To add some more explanations, I'm currently only testing it with a 
statically linked udlfb (for fbcon) as that is what I'm mainly using the 
device for (with otherwise headless boxes). When udlfb is a module, I 
don't see those "schedule while atomic" messages (I don't know why), but 
having a console only after the modules got loaded isn't always an option.

Regards,

Alexander

[PATCH 1/3] semaphore: introduce down_timeout_killable()

[Alexander Holler]

The name says it all, it's like down_timeout() but returns on fatal signals
too.

Signed-off-by: Alexander Holler <holler@ahsoftware.de>
---
 include/linux/semaphore.h |  2 ++
 kernel/semaphore.c        | 37 +++++++++++++++++++++++++++++++++++++
 2 files changed, 39 insertions(+)

diff --git a/include/linux/semaphore.h b/include/linux/semaphore.h
index dc368b8..b508d4d 100644
--- a/include/linux/semaphore.h
+++ b/include/linux/semaphore.h
@@ -41,6 +41,8 @@ extern int __must_check down_interruptible(struct semaphore *sem);
 extern int __must_check down_killable(struct semaphore *sem);
 extern int __must_check down_trylock(struct semaphore *sem);
 extern int __must_check down_timeout(struct semaphore *sem, long jiffies);
+extern int __must_check down_timeout_killable(struct semaphore *sem,
+						long jiffies);
 extern void up(struct semaphore *sem);
 
 #endif /* __LINUX_SEMAPHORE_H */
diff --git a/kernel/semaphore.c b/kernel/semaphore.c
index 4567fc0..4a5e823 100644
--- a/kernel/semaphore.c
+++ b/kernel/semaphore.c
@@ -37,6 +37,8 @@ static noinline void __down(struct semaphore *sem);
 static noinline int __down_interruptible(struct semaphore *sem);
 static noinline int __down_killable(struct semaphore *sem);
 static noinline int __down_timeout(struct semaphore *sem, long jiffies);
+static noinline int __down_timeout_killable(struct semaphore *sem,
+						long jiffies);
 static noinline void __up(struct semaphore *sem);
 
 /**
@@ -169,6 +171,35 @@ int down_timeout(struct semaphore *sem, long jiffies)
 EXPORT_SYMBOL(down_timeout);
 
 /**
+ * down_timeout_killable - acquire the semaphore within a specified time or
+ * until a fatal signal arrives.
+ * @sem: the semaphore to be acquired
+ * @jiffies: how long to wait before failing
+ *
+ * Attempts to acquire the semaphore.  If no more tasks are allowed to
+ * acquire the semaphore, calling this function will put the task to sleep.
+ * If the semaphore is not released within the specified number of jiffies,
+ * this function returns -ETIME. If the sleep is interrupted by a fatal signal,
+ * this function will return -EINTR. If the semaphore is successfully acquired,
+ * this function returns 0.
+ */
+int down_timeout_killable(struct semaphore *sem, long jiffies)
+{
+	unsigned long flags;
+	int result = 0;
+
+	raw_spin_lock_irqsave(&sem->lock, flags);
+	if (likely(sem->count > 0))
+		sem->count--;
+	else
+		result = __down_timeout_killable(sem, jiffies);
+	raw_spin_unlock_irqrestore(&sem->lock, flags);
+
+	return result;
+}
+EXPORT_SYMBOL(down_timeout_killable);
+
+/**
  * up - release the semaphore
  * @sem: the semaphore to release
  *
@@ -253,6 +284,12 @@ static noinline int __sched __down_timeout(struct semaphore *sem, long jiffies)
 	return __down_common(sem, TASK_UNINTERRUPTIBLE, jiffies);
 }
 
+static noinline int __sched __down_timeout_killable(struct semaphore *sem,
+							long jiffies)
+{
+	return __down_common(sem, TASK_KILLABLE, jiffies);
+}
+
 static noinline void __sched __up(struct semaphore *sem)
 {
 	struct semaphore_waiter *waiter = list_first_entry(&sem->wait_list,
-- 
1.7.11.7

[PATCH 2/3 v2] fb: udlfb: fix hang at disconnect

[Alexander Holler]

When a device was disconnected the driver may hang at waiting for urbs it never
will get. Fix this by using a timeout while waiting for the used semaphore.

There is still a memory leak if a timeout happens, but at least the driver
now continues his disconnect routine.

Cc: <stable@vger.kernel.org>
Signed-off-by: Alexander Holler <holler@ahsoftware.de>
---
 drivers/video/udlfb.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/video/udlfb.c b/drivers/video/udlfb.c
index 86d449e..db6cc66 100644
--- a/drivers/video/udlfb.c
+++ b/drivers/video/udlfb.c
@@ -1832,8 +1832,9 @@ static void dlfb_free_urb_list(struct dlfb_data *dev)
 	/* keep waiting and freeing, until we've got 'em all */
 	while (count--) {
 
-		/* Getting interrupted means a leak, but ok at disconnect */
-		ret = down_interruptible(&dev->urbs.limit_sem);
+		/* Timeout likely occurs at disconnect (resulting in a leak) */
+		ret = down_timeout_killable(&dev->urbs.limit_sem,
+						FREE_URB_TIMEOUT);
 		if (ret)
 			break;
 
-- 
1.7.11.7

[PATCH 3/3] fb: smscufx: fix hang at disconnect

[Alexander Holler]

When a device was disconnected the driver may hang at waiting for urbs it never
will get. Fix this by using a timeout while waiting for the used semaphore.

There is still a memory leak if a timeout happens, but at least the driver
now continues his disconnect routine.

Cc: <stable@vger.kernel.org>
Signed-off-by: Alexander Holler <holler@ahsoftware.de>
---
 drivers/video/smscufx.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/video/smscufx.c b/drivers/video/smscufx.c
index 97bd662..f26fa4f 100644
--- a/drivers/video/smscufx.c
+++ b/drivers/video/smscufx.c
@@ -1838,8 +1838,9 @@ static void ufx_free_urb_list(struct ufx_data *dev)
 
 	/* keep waiting and freeing, until we've got 'em all */
 	while (count--) {
-		/* Getting interrupted means a leak, but ok at shutdown*/
-		ret = down_interruptible(&dev->urbs.limit_sem);
+		/* Timeout likely occurs at disconnect (resulting in a leak) */
+		ret = down_timeout_killable(&dev->urbs.limit_sem,
+						FREE_URB_TIMEOUT);
 		if (ret)
 			break;
 
-- 
1.7.11.7

Re: [PATCH 2/3 v2] fb: udlfb: fix hang at disconnect

[Andrew Morton]

On Fri, 25 Jan 2013 19:49:27 +0100
Alexander Holler <holler@ahsoftware.de> wrote:

> When a device was disconnected the driver may hang at waiting for urbs it never
> will get. Fix this by using a timeout while waiting for the used semaphore.
> 
> There is still a memory leak if a timeout happens, but at least the driver
> now continues his disconnect routine.
> 
> ...
>
> --- a/drivers/video/udlfb.c
> +++ b/drivers/video/udlfb.c
> @@ -1832,8 +1832,9 @@ static void dlfb_free_urb_list(struct dlfb_data *dev)
>  	/* keep waiting and freeing, until we've got 'em all */
>  	while (count--) {
>  
> -		/* Getting interrupted means a leak, but ok at disconnect */
> -		ret = down_interruptible(&dev->urbs.limit_sem);
> +		/* Timeout likely occurs at disconnect (resulting in a leak) */
> +		ret = down_timeout_killable(&dev->urbs.limit_sem,
> +						FREE_URB_TIMEOUT);
>  		if (ret)
>  			break;

This is rather a hack.  Do you have an understanding of the underlying
bug?  Why is the driver waiting for things which will never happen?

Re: [PATCH 2/3 v2] fb: udlfb: fix hang at disconnect

[Alexander Holler]

Am 29.01.2013 01:22, schrieb Andrew Morton:
> On Fri, 25 Jan 2013 19:49:27 +0100
> Alexander Holler <holler@ahsoftware.de> wrote:
> 
>> When a device was disconnected the driver may hang at waiting for urbs it never
>> will get. Fix this by using a timeout while waiting for the used semaphore.
>>
>> There is still a memory leak if a timeout happens, but at least the driver
>> now continues his disconnect routine.
>>
>> ...
>>
>> --- a/drivers/video/udlfb.c
>> +++ b/drivers/video/udlfb.c
>> @@ -1832,8 +1832,9 @@ static void dlfb_free_urb_list(struct dlfb_data *dev)
>>  	/* keep waiting and freeing, until we've got 'em all */
>>  	while (count--) {
>>  
>> -		/* Getting interrupted means a leak, but ok at disconnect */
>> -		ret = down_interruptible(&dev->urbs.limit_sem);
>> +		/* Timeout likely occurs at disconnect (resulting in a leak) */
>> +		ret = down_timeout_killable(&dev->urbs.limit_sem,
>> +						FREE_URB_TIMEOUT);
>>  		if (ret)
>>  			break;
> 
> This is rather a hack.  Do you have an understanding of the underlying
> bug?  Why is the driver waiting for things which will never happen?

It is a hack, but I don't want to rewrite the whole driver. There is
already an attempt to do so, udl. The hack is a quick solution which
doesn't make something worse, just better. The only drawback might be
the few additional bytes for the implementation of
down_timeout_killable(), but I thought such should be already available,
and wondered it wasn't.

Fixing the underlying problem (including removing the leaks) would just
end up in another rewrite and I prefer to have a look at udl, maybe
fixing the current problems to use such a device as console there.

I've only posted this small patch series, because some (longterm) stable
kernels don't have udl, and smscufx, which is in large parts identical
to udlfb might want that patch too.

Just in case: I don't know anything about smscufx and have discovered
the similarities between those drivers only when I did a git grep to
search for something I fixed with a previous patch.

Regards,

Alexander

Re: [PATCH 2/3 v2] fb: udlfb: fix hang at disconnect

[Alexander Holler]

Am 29.01.2013 01:56, schrieb Alexander Holler:
> Am 29.01.2013 01:22, schrieb Andrew Morton:
>> On Fri, 25 Jan 2013 19:49:27 +0100
>> Alexander Holler <holler@ahsoftware.de> wrote:
>>
>>> When a device was disconnected the driver may hang at waiting for urbs it never
>>> will get. Fix this by using a timeout while waiting for the used semaphore.
>>>
>>> There is still a memory leak if a timeout happens, but at least the driver
>>> now continues his disconnect routine.
>>>
>>> ...
>>>
>>> --- a/drivers/video/udlfb.c
>>> +++ b/drivers/video/udlfb.c
>>> @@ -1832,8 +1832,9 @@ static void dlfb_free_urb_list(struct dlfb_data *dev)
>>>   	/* keep waiting and freeing, until we've got 'em all */
>>>   	while (count--) {
>>>
>>> -		/* Getting interrupted means a leak, but ok at disconnect */
>>> -		ret = down_interruptible(&dev->urbs.limit_sem);
>>> +		/* Timeout likely occurs at disconnect (resulting in a leak) */
>>> +		ret = down_timeout_killable(&dev->urbs.limit_sem,
>>> +						FREE_URB_TIMEOUT);
>>>   		if (ret)
>>>   			break;
>>
>> This is rather a hack.  Do you have an understanding of the underlying
>> bug?  Why is the driver waiting for things which will never happen?

To add a bit more explanation:

I've experienced that bug after moving the fb-damage-handling into a 
workqueue (to make the driver usable as console). This likely has 
increased the possibility that an urb gets missed when the usb-stack 
calls the (usb-)disconnect function of the driver. But I don't know as I 
couldn't use the driver before (as fbcon) so I don't really have a 
comparison.

What currently happens here is something like that:

fb -> damage -> workload which sends urb and waits for answer
device disconnect -> dlfb_usb_disconnect() -> stall (no answer to the 
above urb)

I don't know why the disconnect waits for all urbs. The code looks like 
it does that just to free the allocated memory. As I'm not very familiar 
with the usb-stack, I would have to read up about the urb-handling to 
find out how to free the memory otherwise.

As the previous comment in the code suggests that urbs already got 
missed (on shutdown) before, I assume that even without my patch, which 
moved the damage into a workqueue, the problem could occur which then 
prevents a shutdown as there is no timeout. As I've experienced that 
problem not only on disconnect, but on shutdown too (no shutdown was 
possible), I have to assume, that the previous used down_interruptible() 
didn't get a signal on shutdown (if the driver is used as fbcon), 
therefor I consider the timeout as necessary.

Regards,

Alexander

Re: [PATCH 2/3 v2] fb: udlfb: fix hang at disconnect

[Alexander Holler]

Am 29.01.2013 11:35, schrieb Alexander Holler:
> Am 29.01.2013 01:56, schrieb Alexander Holler:
>> Am 29.01.2013 01:22, schrieb Andrew Morton:
>>> On Fri, 25 Jan 2013 19:49:27 +0100
>>> Alexander Holler <holler@ahsoftware.de> wrote:
>>>
>>>> When a device was disconnected the driver may hang at waiting for
>>>> urbs it never
>>>> will get. Fix this by using a timeout while waiting for the used
>>>> semaphore.
>>>>
>>>> There is still a memory leak if a timeout happens, but at least the
>>>> driver
>>>> now continues his disconnect routine.
>>>>
>>>> ...
>>>>
>>>> --- a/drivers/video/udlfb.c
>>>> +++ b/drivers/video/udlfb.c
>>>> @@ -1832,8 +1832,9 @@ static void dlfb_free_urb_list(struct
>>>> dlfb_data *dev)
>>>>       /* keep waiting and freeing, until we've got 'em all */
>>>>       while (count--) {
>>>>
>>>> -        /* Getting interrupted means a leak, but ok at disconnect */
>>>> -        ret = down_interruptible(&dev->urbs.limit_sem);
>>>> +        /* Timeout likely occurs at disconnect (resulting in a
>>>> leak) */
>>>> +        ret = down_timeout_killable(&dev->urbs.limit_sem,
>>>> +                        FREE_URB_TIMEOUT);
>>>>           if (ret)
>>>>               break;
>>>
>>> This is rather a hack.  Do you have an understanding of the underlying
>>> bug?  Why is the driver waiting for things which will never happen?
>
> To add a bit more explanation:
>
> I've experienced that bug after moving the fb-damage-handling into a
> workqueue (to make the driver usable as console). This likely has
> increased the possibility that an urb gets missed when the usb-stack
> calls the (usb-)disconnect function of the driver. But I don't know as I
> couldn't use the driver before (as fbcon) so I don't really have a
> comparison.
>
> What currently happens here is something like that:
>
> fb -> damage -> workload which sends urb and waits for answer
> device disconnect -> dlfb_usb_disconnect() -> stall (no answer to the
> above urb)
>
> I don't know why the disconnect waits for all urbs. The code looks like
> it does that just to free the allocated memory. As I'm not very familiar
> with the usb-stack, I would have to read up about the urb-handling to
> find out how to free the memory otherwise.
>
> As the previous comment in the code suggests that urbs already got
> missed (on shutdown) before, I assume that even without my patch, which
> moved the damage into a workqueue, the problem could occur which then
> prevents a shutdown as there is no timeout. As I've experienced that
> problem not only on disconnect, but on shutdown too (no shutdown was
> possible), I have to assume, that the previous used down_interruptible()
> didn't get a signal on shutdown (if the driver is used as fbcon),
> therefor I consider the timeout as necessary.

To explain the problem on shutdown a bit further, I think the following 
happens (usb and driver are statically linked and started by the kernel):

shutdown -> kill signal -> usb stack shuts down -> udlfb waits (forever) 
for a kill or an urb which it doesn't get.

Maybe the sequence is different if the usb-stack and udlfb are used as a 
module and/or udlfb is used only for X/fb. I'm not sure what actually 
does shut down the usb-stack in such a case, but maybe more than one 
kill signal might be thrown around.

Regards,

Alexander

Re: [PATCH 2/3 v2] fb: udlfb: fix hang at disconnect

[Alexander Holler]

Am 29.01.2013 12:11, schrieb Alexander Holler:

>
> To explain the problem on shutdown a bit further, I think the following
> happens (usb and driver are statically linked and started by the kernel):
>
> shutdown -> kill signal -> usb stack shuts down -> udlfb waits (forever)
> for a kill or an urb which it doesn't get.

Having a second look at what I've written above, I'm not even sure if 
the kernel sends one or more fatal signals on shutdown at all. I've just 
assumed it because otherwise down_interruptible() wouldn't have worked 
before (it would have stalled on shutdown too (if an urb got missed), 
not only on disconnect).

Sounds like an interesting question I should read about (if/when fatal 
signals are issued by the kernel). ;)

> Maybe the sequence is different if the usb-stack and udlfb are used as a
> module and/or udlfb is used only for X/fb. I'm not sure what actually
> does shut down the usb-stack in such a case, but maybe more than one
> kill signal might be thrown around.

Regards,

Alexander

Re: [PATCH 2/3 v2] fb: udlfb: fix hang at disconnect

[Alexander Holler]

Am 29.01.2013 16:51, schrieb Alexander Holler:
> Am 29.01.2013 12:11, schrieb Alexander Holler:
>
>>
>> To explain the problem on shutdown a bit further, I think the following
>> happens (usb and driver are statically linked and started by the kernel):
>>
>> shutdown -> kill signal -> usb stack shuts down -> udlfb waits (forever)
>> for a kill or an urb which it doesn't get.
>
> Having a second look at what I've written above, I'm not even sure if
> the kernel sends one or more fatal signals on shutdown at all. I've just
> assumed it because otherwise down_interruptible() wouldn't have worked
> before (it would have stalled on shutdown too (if an urb got missed),
> not only on disconnect).
>
> Sounds like an interesting question I should read about (if/when fatal
> signals are issued by the kernel). ;)
>
>> Maybe the sequence is different if the usb-stack and udlfb are used as a
>> module and/or udlfb is used only for X/fb. I'm not sure what actually
>> does shut down the usb-stack in such a case, but maybe more than one
>> kill signal might be thrown around.

If anyone still follows my monologue: The question was interesting
enough that I couldn't resist for long. ;)

(all pasted => format broken)

In drivers/tty/sysrq.c there is

------
static void send_sig_all(int sig)
{
         struct task_struct *p;

         read_lock(&tasklist_lock);
         for_each_process(p) {
                 if (p->flags & PF_KTHREAD)
                         continue;
                 if (is_global_init(p))
                         continue;

                 do_send_sig_info(sig, SEND_SIG_FORCED, p, true);
         }
         read_unlock(&tasklist_lock);
}

static void sysrq_handle_term(int key)
{
         send_sig_all(SIGTERM);
         console_loglevel = 8;
}

(...)

static void sysrq_handle_kill(int key)
{
         send_sig_all(SIGKILL);
         console_loglevel = 8;
}
------

Now I've done some learning by doing (kernel 3.7.5 + some patches):

------
diff --git a/drivers/video/udlfb.c b/drivers/video/udlfb.c
index df249f3..db8a86c 100644
--- a/drivers/video/udlfb.c
+++ b/drivers/video/udlfb.c
@@ -1876,14 +1876,18 @@ static void dlfb_free_urb_list(struct dlfb_data
*dev)
         unsigned long flags;

         pr_notice("Freeing all render urbs\n");
+       if (current->flags & PF_KTHREAD)
+               pr_info("AHO: I'm a kernel thread\n");

         /* keep waiting and freeing, until we've got 'em all */
         while (count--) {

                 /* Timeout likely occurs at disconnect (resulting in a
leak) */
                 ret = down_timeout_killable(&dev->urbs.limit_sem,
FREE_URB_TIMEOUT);
-               if (ret)
+               if (ret) {
+                       pr_info("AHO: ret %d\n", ret);
                         break;
+               }

                 spin_lock_irqsave(&dev->urbs.lock, flags);
------

Now I've disconnected the display. And, as send_sig_all() already 
suggests, the result was (besides discovering an oops in 
call_timer_fn.isra (once)):

------
[  120.963010] udlfb: AHO: I'm a kernel thread
[  122.957024] udlfb: AHO: ret -62
------
(-62 is -ETIME)

So, if the above down_timeout_killable() is only down_interruptible(), 
as in kernel 3.7.5, the  box would not shutdown afterwards, because on 
shutdown no signal would be send to that kernel-thread which called 
dlfb_free_urb_list().

A last note: dlfb_usb_disconnect() (thus dlfb_free_urb_list()) isn't 
called on shutdown if the device would still be connected. So the 
problem only might happen, if the screen will be disconnected before 
shutdown (and an urb gets missed). So the subject of my patch is correct. ;)

</monologue>

Regards,

Alexander

Re: [PATCH 2/3 v2] fb: udlfb: fix hang at disconnect

[Alexander Holler]

Am 29.01.2013 21:35, schrieb Alexander Holler:
>
> So, if the above down_timeout_killable() is only down_interruptible(),
> as in kernel 3.7.5, the  box would not shutdown afterwards, because on
> shutdown no signal would be send to that kernel-thread which called
> dlfb_free_urb_list().
>
> A last note: dlfb_usb_disconnect() (thus dlfb_free_urb_list()) isn't
> called on shutdown if the device would still be connected. So the
> problem only might happen, if the screen will be disconnected before
> shutdown (and an urb gets missed). So the subject of my patch is
> correct. ;)
>
> </monologue>

Hmm, wrong, sorry, I still have something to add: As no signal arrives 
at all, v1 of that patch is enough and the implementation of 
down_timeout_killable() isn't necessary at all.

If there is a chance that the patch would be Acked-by by someone, I 
would made a v3, replacing

+		ret = down_timeout(&dev->urbs.limit_sem, GET_URB_TIMEOUT);

in v1 with

+		ret = down_timeout(&dev->urbs.limit_sem, FREE_URB_TIMEOUT);

as this seems to be what it should be.

Regards,

Alexander

Re: [PATCH 2/3 v2] fb: udlfb: fix hang at disconnect

[Greg KH]

On Tue, Jan 29, 2013 at 09:35:42PM +0100, Alexander Holler wrote:
> Am 29.01.2013 16:51, schrieb Alexander Holler:
> >Am 29.01.2013 12:11, schrieb Alexander Holler:
> >
> >>
> >>To explain the problem on shutdown a bit further, I think the following
> >>happens (usb and driver are statically linked and started by the kernel):
> >>
> >>shutdown -> kill signal -> usb stack shuts down -> udlfb waits (forever)
> >>for a kill or an urb which it doesn't get.
> >
> >Having a second look at what I've written above, I'm not even sure if
> >the kernel sends one or more fatal signals on shutdown at all. I've just
> >assumed it because otherwise down_interruptible() wouldn't have worked
> >before (it would have stalled on shutdown too (if an urb got missed),
> >not only on disconnect).
> >
> >Sounds like an interesting question I should read about (if/when fatal
> >signals are issued by the kernel). ;)
> >
> >>Maybe the sequence is different if the usb-stack and udlfb are used as a
> >>module and/or udlfb is used only for X/fb. I'm not sure what actually
> >>does shut down the usb-stack in such a case, but maybe more than one
> >>kill signal might be thrown around.
> 
> If anyone still follows my monologue: The question was interesting
> enough that I couldn't resist for long. ;)
> 
> (all pasted => format broken)
> 
> In drivers/tty/sysrq.c there is
> 
> ------
> static void send_sig_all(int sig)
> {
>         struct task_struct *p;
> 
>         read_lock(&tasklist_lock);
>         for_each_process(p) {
>                 if (p->flags & PF_KTHREAD)
>                         continue;
>                 if (is_global_init(p))
>                         continue;
> 
>                 do_send_sig_info(sig, SEND_SIG_FORCED, p, true);
>         }
>         read_unlock(&tasklist_lock);
> }
> 
> static void sysrq_handle_term(int key)
> {
>         send_sig_all(SIGTERM);
>         console_loglevel = 8;
> }
> 
> (...)
> 
> static void sysrq_handle_kill(int key)
> {
>         send_sig_all(SIGKILL);
>         console_loglevel = 8;
> }
> ------
> 
> Now I've done some learning by doing (kernel 3.7.5 + some patches):
> 
> ------
> diff --git a/drivers/video/udlfb.c b/drivers/video/udlfb.c
> index df249f3..db8a86c 100644
> --- a/drivers/video/udlfb.c
> +++ b/drivers/video/udlfb.c
> @@ -1876,14 +1876,18 @@ static void dlfb_free_urb_list(struct dlfb_data
> *dev)
>         unsigned long flags;
> 
>         pr_notice("Freeing all render urbs\n");
> +       if (current->flags & PF_KTHREAD)
> +               pr_info("AHO: I'm a kernel thread\n");
> 
>         /* keep waiting and freeing, until we've got 'em all */
>         while (count--) {
> 
>                 /* Timeout likely occurs at disconnect (resulting in a
> leak) */
>                 ret = down_timeout_killable(&dev->urbs.limit_sem,
> FREE_URB_TIMEOUT);
> -               if (ret)
> +               if (ret) {
> +                       pr_info("AHO: ret %d\n", ret);
>                         break;
> +               }
> 
>                 spin_lock_irqsave(&dev->urbs.lock, flags);
> ------
> 
> Now I've disconnected the display. And, as send_sig_all() already
> suggests, the result was (besides discovering an oops in
> call_timer_fn.isra (once)):
> 
> ------
> [  120.963010] udlfb: AHO: I'm a kernel thread
> [  122.957024] udlfb: AHO: ret -62
> ------
> (-62 is -ETIME)
> 
> So, if the above down_timeout_killable() is only
> down_interruptible(), as in kernel 3.7.5, the  box would not
> shutdown afterwards, because on shutdown no signal would be send to
> that kernel-thread which called dlfb_free_urb_list().
> 
> A last note: dlfb_usb_disconnect() (thus dlfb_free_urb_list()) isn't
> called on shutdown if the device would still be connected. So the
> problem only might happen, if the screen will be disconnected before
> shutdown (and an urb gets missed). So the subject of my patch is
> correct. ;)

Yes, we don't disconnect all devices from the USB bus on shutdown
because, I think, we didn't tear down all of the PCI devices originally,
so your USB bus never knew it was going to be shutdown.

This is how things have always worked, and shutting down PCI devices in
the past have been known to cause problems.  I think.  I vaguely
remember some issues when I tried to do this 10+ years or so ago in the
2.5 kernel days, but I could be totally wrong given that I can't
remember what I was working on last month at times...

So you are right in that your driver will wait for forever for a
disconnect() to happen, as it will never be called.  I don't understand
the problem that this is causing when it happens.  What's wrong with
udlfb that having the cpu suddently reset as the powerdown happened
without it knowing about it?

thanks,

greg k-h

Re: [PATCH 2/3 v2] fb: udlfb: fix hang at disconnect

[Alexander Holler]

Am 04.02.2013 02:14, schrieb Greg KH:

> So you are right in that your driver will wait for forever for a
> disconnect() to happen, as it will never be called.  I don't understand
> the problem that this is causing when it happens.  What's wrong with
> udlfb that having the cpu suddently reset as the powerdown happened
> without it knowing about it?

There is nothing wrong with that. I've just explained why a problem 
doesn't occur on shutdown but on disconnect (of the device).

Regards,

Alexander

Re: [PATCH 2/3 v2] fb: udlfb: fix hang at disconnect

[Alexander Holler]

Am 04.02.2013 13:05, schrieb Alexander Holler:
> Am 04.02.2013 02:14, schrieb Greg KH:
>
>> So you are right in that your driver will wait for forever for a
>> disconnect() to happen, as it will never be called.  I don't understand
>> the problem that this is causing when it happens.  What's wrong with
>> udlfb that having the cpu suddently reset as the powerdown happened
>> without it knowing about it?
>
> There is nothing wrong with that. I've just explained why a problem
> doesn't occur on shutdown but on disconnect (of the device).

Maybe my explanation before was just to long and I try to explain it a 
bit shorter:

If a device gets disconnected, the disconnect in udlfb might wait 
forever in down_interruptible() (because it waits for an urb it never 
receives). This even prevents a shutdown afterwards, because that 
down_interruptible() never receives a signal (at shutdown, because 
kernel threads don't get such).

So the change from down_timeout() to down_interruptible() in 
dlfb_free_urb_list() with commit 
33077b8d3042e01da61924973e372abe589ba297 only results in that the 
following code (thus the break there) will never be reached if an urb 
got missed (because of a disconnect).

And the accompanying comment (... at shutdown) is misleading, because on 
shutdown, the kernel thread which calls dlfb_free_urb_list() never gets 
a signal, so the interruption just never happens.

As I've experienced the "missing urb on disconnect" problem quiet often, 
I've changed that down_interruptible() to down_timeout() (in v1 and in 
v2 to down_timeout_interruptible, because I wasn't aware that no signal 
arrives on shutdown).

Hmm, ok, that explanation isn't much shorter. ;)

Regards,

Alexander

Re: [PATCH 2/3 v2] fb: udlfb: fix hang at disconnect

[Greg KH]

On Mon, Feb 04, 2013 at 08:17:04PM +0100, Alexander Holler wrote:
> Am 04.02.2013 13:05, schrieb Alexander Holler:
> >Am 04.02.2013 02:14, schrieb Greg KH:
> >
> >>So you are right in that your driver will wait for forever for a
> >>disconnect() to happen, as it will never be called.  I don't understand
> >>the problem that this is causing when it happens.  What's wrong with
> >>udlfb that having the cpu suddently reset as the powerdown happened
> >>without it knowing about it?
> >
> >There is nothing wrong with that. I've just explained why a problem
> >doesn't occur on shutdown but on disconnect (of the device).
> 
> Maybe my explanation before was just to long and I try to explain it
> a bit shorter:
> 
> If a device gets disconnected, the disconnect in udlfb might wait
> forever in down_interruptible() (because it waits for an urb it
> never receives). This even prevents a shutdown afterwards, because
> that down_interruptible() never receives a signal (at shutdown,
> because kernel threads don't get such).

Where was that urb when the disconnect happened?  The USB core should
call your urb callback for any outstanding urbs at that point in time,
with the proper error flag being set, are you handling that properly?

thanks,

greg k-h

Re: [PATCH 2/3 v2] fb: udlfb: fix hang at disconnect

[Alexander Holler]

Am 04.02.2013 20:25, schrieb Greg KH:
> On Mon, Feb 04, 2013 at 08:17:04PM +0100, Alexander Holler wrote:
>> Am 04.02.2013 13:05, schrieb Alexander Holler:
>>> Am 04.02.2013 02:14, schrieb Greg KH:
>>>
>>>> So you are right in that your driver will wait for forever for a
>>>> disconnect() to happen, as it will never be called.  I don't understand
>>>> the problem that this is causing when it happens.  What's wrong with
>>>> udlfb that having the cpu suddently reset as the powerdown happened
>>>> without it knowing about it?
>>>
>>> There is nothing wrong with that. I've just explained why a problem
>>> doesn't occur on shutdown but on disconnect (of the device).
>>
>> Maybe my explanation before was just to long and I try to explain it
>> a bit shorter:
>>
>> If a device gets disconnected, the disconnect in udlfb might wait
>> forever in down_interruptible() (because it waits for an urb it
>> never receives). This even prevents a shutdown afterwards, because
>> that down_interruptible() never receives a signal (at shutdown,
>> because kernel threads don't get such).
> 
> Where was that urb when the disconnect happened?  The USB core should
> call your urb callback for any outstanding urbs at that point in time,
> with the proper error flag being set, are you handling that properly?

I don't know where that urb is as I don't handle it. I just know that
_interruptible doesn't make any sense and _timeout is necessary here.
But as nobody else seems to have a problem, nobody else see seems to see
the problem there and I seem to be unable to explain it, just ignore
that patch.

Regards,

Alexander

Re: [PATCH 2/3 v2] fb: udlfb: fix hang at disconnect

[Greg KH]

On Tue, Feb 05, 2013 at 08:08:28AM +0100, Alexander Holler wrote:
> Am 04.02.2013 20:25, schrieb Greg KH:
> > On Mon, Feb 04, 2013 at 08:17:04PM +0100, Alexander Holler wrote:
> >> Am 04.02.2013 13:05, schrieb Alexander Holler:
> >>> Am 04.02.2013 02:14, schrieb Greg KH:
> >>>
> >>>> So you are right in that your driver will wait for forever for a
> >>>> disconnect() to happen, as it will never be called.  I don't understand
> >>>> the problem that this is causing when it happens.  What's wrong with
> >>>> udlfb that having the cpu suddently reset as the powerdown happened
> >>>> without it knowing about it?
> >>>
> >>> There is nothing wrong with that. I've just explained why a problem
> >>> doesn't occur on shutdown but on disconnect (of the device).
> >>
> >> Maybe my explanation before was just to long and I try to explain it
> >> a bit shorter:
> >>
> >> If a device gets disconnected, the disconnect in udlfb might wait
> >> forever in down_interruptible() (because it waits for an urb it
> >> never receives). This even prevents a shutdown afterwards, because
> >> that down_interruptible() never receives a signal (at shutdown,
> >> because kernel threads don't get such).
> > 
> > Where was that urb when the disconnect happened?  The USB core should
> > call your urb callback for any outstanding urbs at that point in time,
> > with the proper error flag being set, are you handling that properly?
> 
> I don't know where that urb is as I don't handle it.

What do you mean by that?  The urb is being sent back to your driver,
right?  If not, that's a bug, but please be sure that your urb callback
isn't really being called.

thanks,

greg k-h

Re: [PATCH 2/3 v2] fb: udlfb: fix hang at disconnect

[Alexander Holler]

Am 05.02.2013 18:22, schrieb Greg KH:
> On Tue, Feb 05, 2013 at 08:08:28AM +0100, Alexander Holler wrote:
>> Am 04.02.2013 20:25, schrieb Greg KH:

>>> Where was that urb when the disconnect happened?  The USB core should
>>> call your urb callback for any outstanding urbs at that point in time,
>>> with the proper error flag being set, are you handling that properly?
>>
>> I don't know where that urb is as I don't handle it.
>
> What do you mean by that?  The urb is being sent back to your driver,
> right?  If not, that's a bug, but please be sure that your urb callback
> isn't really being called.

I meant it isn't _my_ driver. ;)

I'm just trying to add some würgarounds without having the need to 
rewrite the whole driver.

In regard to that "urb missing problem", I think I've just named it 
wrong and the actual problem is a race-condition between the semaphore 
handling (which is used to keep track of the urbs) and the urb handling 
inside the driver.

But I've just switched to udl (instead of udlfb) and will see if I can 
fix the bugs there to make it usable as a console. udl is a rewrite of 
udlfb with some additional features (e.g. drm), so hopefully fixing the 
remaining problems there will require less work.

Regards,

Alexander

Re: [PATCH 2/3 v2] fb: udlfb: fix hang at disconnect

[Dave Airlie]

>
> But I've just switched to udl (instead of udlfb) and will see if I can fix
> the bugs there to make it usable as a console. udl is a rewrite of udlfb
> with some additional features (e.g. drm), so hopefully fixing the remaining
> problems there will require less work.

I may have fixed the major udl problem with being a console, patch was
posted to dri-devel yesterday, and I've pushed it into -next.

Dave.

Re: [PATCH 2/3 v2] fb: udlfb: fix hang at disconnect

[Alexander Holler]

Am 08.02.2013 05:07, schrieb Dave Airlie:
>>
>> But I've just switched to udl (instead of udlfb) and will see if I can fix
>> the bugs there to make it usable as a console. udl is a rewrite of udlfb
>> with some additional features (e.g. drm), so hopefully fixing the remaining
>> problems there will require less work.
>
> I may have fixed the major udl problem with being a console, patch was
> posted to dri-devel yesterday, and I've pushed it into -next.

Thanks a lot. I will check it.

Regards,

Alexander