Re: [PATCH] fb: Rework locking to fix lock ordering on takeover

Re: [PATCH] fb: Rework locking to fix lock ordering on takeover

[Alexander Holler]

Am 28.12.2012 13:40, schrieb Borislav Petkov:
> On Fri, Dec 28, 2012 at 07:50:27PM +0800, Shawn Guo wrote:
>> +1
>>
>> http://thread.gmane.org/gmane.linux.kernel/1413953/focus\x1415070
>
> Cool, works fine here too. Is Linus on CC? (/me checks.. ) Yes he is,
> good.
>
> Linus, Alan's patch works at least in 2 cases, you might consider
> picking it up directly since the fb maintainer is absent, reportedly.


Btw. I think all the usb-fb's (udlfb, smscufx and udl) are broken, at 
least on ARM(v5). When I have linked in udlfb the following happens on 
boot (with an attached USB-LCD and with or without the "Rework locking 
patch):

dlfb_init_framebuffer_work() (work)
         register_framebuffer() (lock mutex registration_lock)
                 (vt_console_print) (spinlock printing_lock)
                 (fbcon_scroll)
                 (fbcon_redraw)
                 (fbcon_putcs)
                 (bit_putcs)
                 dlfb_ops_imageblit()
                         dlfb_handle_damage()
                                 dlfb_get_urb()
                                         down_timeout(semaphore)
					BUG: scheduling while atomic
                 (vt_console_print) (release spinlock printing_lock)
         register_framebuffer() (unlock mutex registration_lock)

The above is without the "Rework locking" patch. But I get the same BUG 
with the patch (so the patch doesn't do any harm), I just haven't looked 
in detail what changed with the patch.

I don't get the BUG when I try the same on a x86_64 machine, not sure 
why. I've just started to read me through the code, documentation and 
the "Rework locking" patch. And I'm slow in doing so (spare time).
But maybe someone else with more knowledge about the inner workings of 
this stuff is faster than I.

Regards,

Alexander

Re: [PATCH] fb: Rework locking to fix lock ordering on takeover

[Alan Cox]

On Fri, 04 Jan 2013 13:50:37 +0100
Alexander Holler <holler@ahsoftware.de> wrote:

> Am 28.12.2012 13:40, schrieb Borislav Petkov:
> > On Fri, Dec 28, 2012 at 07:50:27PM +0800, Shawn Guo wrote:
> >> +1
> >>
> >> http://thread.gmane.org/gmane.linux.kernel/1413953/focus\x1415070
> >
> > Cool, works fine here too. Is Linus on CC? (/me checks.. ) Yes he is,
> > good.
> >
> > Linus, Alan's patch works at least in 2 cases, you might consider
> > picking it up directly since the fb maintainer is absent, reportedly.
> 
> 
> Btw. I think all the usb-fb's (udlfb, smscufx and udl) are broken, at 
> least on ARM(v5). When I have linked in udlfb the following happens on 
> boot (with an attached USB-LCD and with or without the "Rework locking 
> patch):

They are broken if used as the system console (has been known for years).
Perhaps your x86 test has the system console still on another device ?

For the udl layer it shouldn't matter as Dave Airlie wrote a DRM driver
for udl which obsoletes the old fb layer one and works much better
(although the error handling is still totally broken and leaks like a
sieve if it fails)

Fixing the console isn't that difficult - you just need to make your
device queue the console I/O to a worker thread of some kind. We don't
want to do that by default because we want to get the messages out
reliably and immediately on saner hardware. Given there are several
such cases a general helper and a console "I am crap" flag might be better
than hacking each driver.

Alan

Re: [PATCH] fb: Rework locking to fix lock ordering on takeover

[Alexander Holler]

Am 04.01.2013 14:25, schrieb Alan Cox:
> On Fri, 04 Jan 2013 13:50:37 +0100
> Alexander Holler <holler@ahsoftware.de> wrote:
>
>> Am 28.12.2012 13:40, schrieb Borislav Petkov:
>>> On Fri, Dec 28, 2012 at 07:50:27PM +0800, Shawn Guo wrote:
>>>> +1
>>>>
>>>> http://thread.gmane.org/gmane.linux.kernel/1413953/focus\x1415070
>>>
>>> Cool, works fine here too. Is Linus on CC? (/me checks.. ) Yes he is,
>>> good.
>>>
>>> Linus, Alan's patch works at least in 2 cases, you might consider
>>> picking it up directly since the fb maintainer is absent, reportedly.
>>
>>
>> Btw. I think all the usb-fb's (udlfb, smscufx and udl) are broken, at
>> least on ARM(v5). When I have linked in udlfb the following happens on
>> boot (with an attached USB-LCD and with or without the "Rework locking
>> patch):
>
> They are broken if used as the system console (has been known for years).

Ah. Thats why I didn't see it before. Usually I've used the serial as 
system console. So thats why it worked before. ;)

> Perhaps your x86 test has the system console still on another device ?

Exactly thats the case. Thanks for pointing it out.

> For the udl layer it shouldn't matter as Dave Airlie wrote a DRM driver
> for udl which obsoletes the old fb layer one and works much better
> (although the error handling is still totally broken and leaks like a
> sieve if it fails)
>
> Fixing the console isn't that difficult - you just need to make your
> device queue the console I/O to a worker thread of some kind. We don't

That is what I wanted to try next. ;)

> want to do that by default because we want to get the messages out
> reliably and immediately on saner hardware. Given there are several
> such cases a general helper and a console "I am crap" flag might be better
> than hacking each driver.

All those drivers look very similiar. I will see if I'm successfull in 
writing such an IamCrapHelper. Might need some time, but I will post a 
patch for review, if I've done and tested it. I'm only using the USB-LCD 
on occasion, so it doesn't have high priority for me because I don't 
really need it.

Thanks for the hints.

Regards,

Alexander

Re: [PATCH] fb: Rework locking to fix lock ordering on takeover

[Alexander Holler]

Am 04.01.2013 14:36, schrieb Alexander Holler:
> Am 04.01.2013 14:25, schrieb Alan Cox:
>> On Fri, 04 Jan 2013 13:50:37 +0100
>> Alexander Holler <holler@ahsoftware.de> wrote:

...
>>> Btw. I think all the usb-fb's (udlfb, smscufx and udl) are broken, at
>>> least on ARM(v5). When I have linked in udlfb the following happens on
>>> boot (with an attached USB-LCD and with or without the "Rework locking
>>> patch):
>>
>> They are broken if used as the system console (has been known for years).

>> Fixing the console isn't that difficult - you just need to make your
>> device queue the console I/O to a worker thread of some kind. We don't
>
> That is what I wanted to try next. ;)
>
>> want to do that by default because we want to get the messages out
>> reliably and immediately on saner hardware. Given there are several
>> such cases a general helper and a console "I am crap" flag might be
>> better
>> than hacking each driver.
>
> All those drivers look very similiar. I will see if I'm successfull in
> writing such an IamCrapHelper. Might need some time, but I will post a
> patch for review, if I've done and tested it. I'm only using the USB-LCD
> on occasion, so it doesn't have high priority for me because I don't
> really need it.

I've just added a work queue for dlfb_handle_damage. Up to now I've only 
tested the console, seems to work without any problems (even with lock 
checking on).

In regard to that "I am crap" handler, I'm not sure how to do that.

Just queuing the ops wherever they are used outside the drivers doesn't 
work, because e.g.

if(i_am_crap)
   queue_work(ops.fb_imageblit(..., image))
else
   ops.fb_imageblit(..., image)

doesn't work, because e.g. image will become destroyed before the work 
gets executed. And copying the whole image doesn't make sense. 
handle_damage() in contrast just needs the coordinates for a rectangle 
(x, y, w, h).

So to add such an "I am crap" flag my idea would be to add an 
.fb_handle_damage to struct fb_ops and then call that (if exists) 
whenever something was changed.

But I don't like that very much. I think that might end up in more 
changes than just changing those 3 very similiar drivers (I'm not sure 
if the queuing is needed for udl at all).

Maybe it would make sense, to unify the stuff in those 3 similiar 
drivers moving the shared functions to one file which is used by them 
all. udl seems to have already split some stuff into different files.

My patch (for udlfb) follows as an reply to this message. If that patch 
is ok, it should be applied to smscufx too (I would make it). In regard 
to udl I don't know, I haven't had a deeper look at it nor used it up to 
now.

Regards,

Alexander

[PATCH] fb: udlfb: fix scheduling while atomic.

[Alexander Holler]

The console functions are using spinlocks while calling fb-driver ops
but udlfb waits for a semaphore in many ops. This results in the BUG
"scheduling while atomic". One of those call flows is e.g.

vt_console_print() (spinlock printing_lock)
	(...)
	dlfb_ops_imageblit()
                        dlfb_handle_damage()
                                dlfb_get_urb()
					down_timeout(semaphore)
BUG: scheduling while atomic
(...)
vt_console_print() (release spinlock printing_lock)

Fix this through a workqueue for dlfb_handle_damage().

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

diff --git a/drivers/video/udlfb.c b/drivers/video/udlfb.c
index 86d449e..5aadcb2 100644
--- a/drivers/video/udlfb.c
+++ b/drivers/video/udlfb.c
@@ -569,7 +569,7 @@ static int dlfb_render_hline(struct dlfb_data *dev, struct urb **urb_ptr,
 	return 0;
 }
 
-int dlfb_handle_damage(struct dlfb_data *dev, int x, int y,
+int dlfb_handle_damage_queued(struct dlfb_data *dev, int x, int y,
 	       int width, int height, char *data)
 {
 	int i, ret;
@@ -630,6 +630,44 @@ error:
 	return 0;
 }
 
+static struct workqueue_struct *dlfb_handle_damage_wq;
+
+struct dlfb_handle_damage_work {
+	struct work_struct my_work;
+	struct dlfb_data *dev;
+	char *data;
+	int x, y, width, height;
+};
+
+static void dlfb_handle_damage_work(struct work_struct *work)
+{
+	struct dlfb_handle_damage_work *my_work +		(struct dlfb_handle_damage_work *)work;
+
+	dlfb_handle_damage_queued(my_work->dev, my_work->x, my_work->y,
+			my_work->width, my_work->height, my_work->data);
+	kfree(work);
+	return;
+}
+
+void dlfb_handle_damage(struct dlfb_data *dev, int x, int y,
+	       int width, int height, char *data)
+{
+	struct dlfb_handle_damage_work *work +		kmalloc(sizeof(struct dlfb_handle_damage_work), GFP_KERNEL);
+
+	if (work) {
+		INIT_WORK((struct work_struct *)work, dlfb_handle_damage_work);
+		work->dev = dev;
+		work->x = x;
+		work->y = y;
+		work->width = width;
+		work->height = height;
+		work->data = data;
+		queue_work(dlfb_handle_damage_wq, (struct work_struct *)work);
+	}
+}
+
 /*
  * Path triggered by usermode clients who write to filesystem
  * e.g. cat filename > /dev/fb1
@@ -945,6 +983,9 @@ static void dlfb_free_framebuffer(struct dlfb_data *dev)
 
 		unregister_framebuffer(info);
 
+		if (dlfb_handle_damage_wq)
+			destroy_workqueue(dlfb_handle_damage_wq);
+
 		if (info->cmap.len != 0)
 			fb_dealloc_cmap(&info->cmap);
 		if (info->monspecs.modedb)
@@ -1626,13 +1667,13 @@ static int dlfb_usb_probe(struct usb_interface *interface,
 		dev->sku_pixel_limit = pixel_limit;
 	}
 
-
 	if (!dlfb_alloc_urb_list(dev, WRITES_IN_FLIGHT, MAX_TRANSFER)) {
 		retval = -ENOMEM;
 		pr_err("dlfb_alloc_urb_list failed\n");
 		goto error;
 	}
 
+
 	kref_get(&dev->kref); /* matching kref_put in free_framebuffer_work */
 
 	/* We don't register a new USB class. Our client interface is fbdev */
@@ -1694,6 +1735,13 @@ static void dlfb_init_framebuffer_work(struct work_struct *work)
 		goto error;
 	}
 
+	dlfb_handle_damage_wq = alloc_workqueue("udlfb_damage",
+						WQ_MEM_RECLAIM, 0);
+	if (dlfb_handle_damage_wq = NULL) {
+		pr_err("unable to allocate workqueue\n");
+		goto error;
+	}
+
 	/* ready to begin using device */
 
 	atomic_set(&dev->usb_active, 1);
@@ -1702,6 +1750,7 @@ static void dlfb_init_framebuffer_work(struct work_struct *work)
 	dlfb_ops_check_var(&info->var, info);
 	dlfb_ops_set_par(info);
 
+
 	retval = register_framebuffer(info);
 	if (retval < 0) {
 		pr_err("register_framebuffer failed %d\n", retval);
-- 
1.7.11.7

Re: [PATCH] fb: Rework locking to fix lock ordering on takeover

[Alexander Holler]

Am 05.01.2013 13:07, schrieb Alan Cox:
>> So to add such an "I am crap" flag my idea would be to add an
>> .fb_handle_damage to struct fb_ops and then call that (if exists)
>> whenever something was changed.
>
> I was thinking much higher level - ie at the printk kind of level
>
>> My patch (for udlfb) follows as an reply to this message. If that patch
>> is ok, it should be applied to smscufx too (I would make it). In regard
>> to udl I don't know, I haven't had a deeper look at it nor used it up to
>> now.
>
> Looks pretty clean as a solution to me.

Thanks and sorry for the two empty lines in the patch. I swear I had a 
look at the patch before sending it out, but haven't seen them.

So should I make the same patch for smscufx  and while beeing there, 
send out at v2 without those 2 empty lines?

Regards,

Alexander

Re: [PATCH] fb: Rework locking to fix lock ordering on takeover

[Alan Cox]

> So to add such an "I am crap" flag my idea would be to add an 
> .fb_handle_damage to struct fb_ops and then call that (if exists) 
> whenever something was changed.

I was thinking much higher level - ie at the printk kind of level

> My patch (for udlfb) follows as an reply to this message. If that patch 
> is ok, it should be applied to smscufx too (I would make it). In regard 
> to udl I don't know, I haven't had a deeper look at it nor used it up to 
> now.

Looks pretty clean as a solution to me.

Re: [PATCH] fb: udlfb: fix scheduling while atomic.

[Alexander Holler]

Am 05.01.2013 12:42, schrieb Alexander Holler:
> The console functions are using spinlocks while calling fb-driver ops
> but udlfb waits for a semaphore in many ops. This results in the BUG
> "scheduling while atomic". One of those call flows is e.g.
>
> vt_console_print() (spinlock printing_lock)
> 	(...)
> 	dlfb_ops_imageblit()
>                          dlfb_handle_damage()
>                                  dlfb_get_urb()
> 					down_timeout(semaphore)
> BUG: scheduling while atomic
> (...)
> vt_console_print() (release spinlock printing_lock)
>
> Fix this through a workqueue for dlfb_handle_damage().
>
> Cc: <stable@vger.kernel.org>
> Signed-off-by: Alexander Holler <holler@ahsoftware.de>


Having had a second look at my patch for udlfb, I'm not sure it will 
work with more than one of those devices attached. I think my approach 
to just add one (static) workqueue might not work in such a case, at 
least it looks so to me. But I'm unable to test it, as I only have one 
of those devices.

Having had a look at udl, I wonder why udlfb still has to be around. But 
because udl currently doesn't work here too, I'm not sure what 
functionality udl misses which udlfb still has.

So to conclude, my patch works as a workaround if only one of those 
devices will be attached, but currently should not be included into the 
kernel.

I don't know if I will make another version of that patch, as I will 
first have a deeper look at udl (if I find the time).

Regards,

Alexander

[PATCH v2] fb: udlfb: fix scheduling while atomic.

[Alexander Holler]

The console functions are using spinlocks while calling fb-driver ops
but udlfb waits for a semaphore in many ops. This results in the BUG
"scheduling while atomic". One of those call flows is e.g.

vt_console_print() (spinlock printing_lock)
	(...)
	dlfb_ops_imageblit()
                        dlfb_handle_damage()
                                dlfb_get_urb()
					down_timeout(semaphore)
BUG: scheduling while atomic
(...)
vt_console_print() (release spinlock printing_lock)

Fix this through a workqueue for dlfb_handle_damage().

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

diff --git a/drivers/video/udlfb.c b/drivers/video/udlfb.c
index 86d449e..4a90784 100644
--- a/drivers/video/udlfb.c
+++ b/drivers/video/udlfb.c
@@ -569,7 +569,7 @@ static int dlfb_render_hline(struct dlfb_data *dev, struct urb **urb_ptr,
 	return 0;
 }
 
-int dlfb_handle_damage(struct dlfb_data *dev, int x, int y,
+int dlfb_handle_damage_queued(struct dlfb_data *dev, int x, int y,
 	       int width, int height, char *data)
 {
 	int i, ret;
@@ -630,6 +630,44 @@ error:
 	return 0;
 }
 
+struct dlfb_handle_damage_work {
+	struct work_struct my_work;
+	struct dlfb_data *dev;
+	char *data;
+	int x, y, width, height;
+};
+
+static void dlfb_handle_damage_work(struct work_struct *work)
+{
+	struct dlfb_handle_damage_work *my_work +		(struct dlfb_handle_damage_work *)work;
+
+	dlfb_handle_damage_queued(my_work->dev, my_work->x, my_work->y,
+			my_work->width, my_work->height, my_work->data);
+	kfree(work);
+	return;
+}
+
+void dlfb_handle_damage(struct dlfb_data *dev, int x, int y,
+	       int width, int height, char *data)
+{
+	struct dlfb_handle_damage_work *work +		kmalloc(sizeof(struct dlfb_handle_damage_work), GFP_KERNEL);
+
+	if (!work) {
+		pr_err("unable to allocate work\n");
+		return;
+	}
+	INIT_WORK((struct work_struct *)work, dlfb_handle_damage_work);
+	work->dev = dev;
+	work->x = x;
+	work->y = y;
+	work->width = width;
+	work->height = height;
+	work->data = data;
+	queue_work(dev->handle_damage_wq, (struct work_struct *)work);
+}
+
 /*
  * Path triggered by usermode clients who write to filesystem
  * e.g. cat filename > /dev/fb1
@@ -945,6 +983,9 @@ static void dlfb_free_framebuffer(struct dlfb_data *dev)
 
 		unregister_framebuffer(info);
 
+		if (dev->handle_damage_wq)
+			destroy_workqueue(dev->handle_damage_wq);
+
 		if (info->cmap.len != 0)
 			fb_dealloc_cmap(&info->cmap);
 		if (info->monspecs.modedb)
@@ -1694,6 +1735,13 @@ static void dlfb_init_framebuffer_work(struct work_struct *work)
 		goto error;
 	}
 
+	dev->handle_damage_wq = alloc_workqueue("udlfb_damage",
+						WQ_MEM_RECLAIM, 0);
+	if (dev->handle_damage_wq = NULL) {
+		pr_err("unable to allocate workqueue\n");
+		goto error;
+	}
+
 	/* ready to begin using device */
 
 	atomic_set(&dev->usb_active, 1);
diff --git a/include/video/udlfb.h b/include/video/udlfb.h
index f9466fa..1e765f9 100644
--- a/include/video/udlfb.h
+++ b/include/video/udlfb.h
@@ -43,6 +43,7 @@ struct dlfb_data {
 	bool virtualized; /* true when physical usb device not present */
 	struct delayed_work init_framebuffer_work;
 	struct delayed_work free_framebuffer_work;
+	struct workqueue_struct *handle_damage_wq;
 	atomic_t usb_active; /* 0 = update virtual buffer, but no usb traffic */
 	atomic_t lost_pixels; /* 1 = a render op failed. Need screen refresh */
 	char *edid; /* null until we read edid from hw or get from sysfs */
-- 
1.7.11.7