[PATCH 0/2] BKL in i810 and i830 drivers

[PATCH 0/2] BKL in i810 and i830 drivers

[Arnd Bergmann]

In the final stages of the big kernel removal, we still have
these two DRM drivers left. If nobody is going to fix them,
we can either mark them as BROKEN_ON_SMP or remove the drivers
entirely (provided that there are no real users left, which I
cannot judge).

While preparing the second patch, I noticed that one of my
previous patches was incomplete and needs a fixup, hence the
first patch.

Please apply the first patch now, and let me know what you think
about the second one.

	Arnd

Arnd Bergmann (2):
  drm: i810/i830: fix locked ioctl variant
  drm: i810/i830: kill BKL, mark as BROKEN_ON_SMP

 drivers/gpu/drm/Kconfig         |    6 ++----
 drivers/gpu/drm/i810/i810_dma.c |   18 +-----------------
 drivers/gpu/drm/i810/i810_drv.c |    2 +-
 drivers/gpu/drm/i830/i830_dma.c |   18 +-----------------
 drivers/gpu/drm/i830/i830_drv.c |    2 +-
 5 files changed, 6 insertions(+), 40 deletions(-)

[PATCH 1/2] drm: i810/i830: fix locked ioctl variant

[Arnd Bergmann]

The i810 and i830 device drivers may replace their file operations
on an open file descriptor. My previous patch to move the BKL
out of the common DRM code into these drivers only caught the
default file operations, not the ones that actually end up being
used.

Found while trying to come up with a way to kill the BKL for
good in these drivers.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 drivers/gpu/drm/i810/i810_dma.c |    2 +-
 drivers/gpu/drm/i830/i830_dma.c |    2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i810/i810_dma.c b/drivers/gpu/drm/i810/i810_dma.c
index 00f1bda..ff33e53 100644
--- a/drivers/gpu/drm/i810/i810_dma.c
+++ b/drivers/gpu/drm/i810/i810_dma.c
@@ -116,7 +116,7 @@ static int i810_mmap_buffers(struct file *filp, struct vm_area_struct *vma)
 static const struct file_operations i810_buffer_fops = {
 	.open = drm_open,
 	.release = drm_release,
-	.unlocked_ioctl = drm_ioctl,
+	.unlocked_ioctl = i810_ioctl,
 	.mmap = i810_mmap_buffers,
 	.fasync = drm_fasync,
 	.llseek = noop_llseek,
diff --git a/drivers/gpu/drm/i830/i830_dma.c b/drivers/gpu/drm/i830/i830_dma.c
index 5c6eb65..ca6f31f 100644
--- a/drivers/gpu/drm/i830/i830_dma.c
+++ b/drivers/gpu/drm/i830/i830_dma.c
@@ -118,7 +118,7 @@ static int i830_mmap_buffers(struct file *filp, struct vm_area_struct *vma)
 static const struct file_operations i830_buffer_fops = {
 	.open = drm_open,
 	.release = drm_release,
-	.unlocked_ioctl = drm_ioctl,
+	.unlocked_ioctl = i830_ioctl,
 	.mmap = i830_mmap_buffers,
 	.fasync = drm_fasync,
 	.llseek = noop_llseek,
-- 
1.7.1

[PATCH 2/2] drm: i810/i830: kill BKL, mark as BROKEN_ON_SMP

[Arnd Bergmann]

The i810 and i830 drivers are the only hardware drivers that
still use the BKL without anyone volunteering to fix them.

The hardware is rather old and typically used on non-SMP
systems. Mark them as BROKEN_ON_SMP and remove the BKL
now so that people can still use the driver once the BKL
is entirely gone.

I could not actually find a reason why the BKL is required
here, the same operations that need it also seem to take
the mmap_sem. If anyone can prove that it's really not
necessary, please remove the BROKEN_ON_SMP requirement.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 drivers/gpu/drm/Kconfig         |    6 ++----
 drivers/gpu/drm/i810/i810_dma.c |   18 +-----------------
 drivers/gpu/drm/i810/i810_drv.c |    2 +-
 drivers/gpu/drm/i830/i830_dma.c |   18 +-----------------
 drivers/gpu/drm/i830/i830_drv.c |    2 +-
 5 files changed, 6 insertions(+), 40 deletions(-)

diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
index b755301..fe4b94f 100644
--- a/drivers/gpu/drm/Kconfig
+++ b/drivers/gpu/drm/Kconfig
@@ -73,8 +73,7 @@ source "drivers/gpu/drm/radeon/Kconfig"
 
 config DRM_I810
 	tristate "Intel I810"
-	# BKL usage in order to avoid AB-BA deadlocks, may become BROKEN_ON_SMP
-	depends on DRM && AGP && AGP_INTEL && BKL
+	depends on DRM && AGP && AGP_INTEL && BROKEN_ON_SMP
 	help
 	  Choose this option if you have an Intel I810 graphics card.  If M is
 	  selected, the module will be called i810.  AGP support is required
@@ -87,8 +86,7 @@ choice
 
 config DRM_I830
 	tristate "i830 driver"
-	# BKL usage in order to avoid AB-BA deadlocks, may become BROKEN_ON_SMP
-	depends on BKL
+	depends on BROKEN_ON_SMP
 	help
 	  Choose this option if you have a system that has Intel 830M, 845G,
 	  852GM, 855GM or 865G integrated graphics.  If M is selected, the
diff --git a/drivers/gpu/drm/i810/i810_dma.c b/drivers/gpu/drm/i810/i810_dma.c
index ff33e53..8f371e8 100644
--- a/drivers/gpu/drm/i810/i810_dma.c
+++ b/drivers/gpu/drm/i810/i810_dma.c
@@ -37,7 +37,6 @@
 #include <linux/interrupt.h>	/* For task queue support */
 #include <linux/delay.h>
 #include <linux/slab.h>
-#include <linux/smp_lock.h>
 #include <linux/pagemap.h>
 
 #define I810_BUF_FREE		2
@@ -94,7 +93,6 @@ static int i810_mmap_buffers(struct file *filp, struct vm_area_struct *vma)
 	struct drm_buf *buf;
 	drm_i810_buf_priv_t *buf_priv;
 
-	lock_kernel();
 	dev = priv->minor->dev;
 	dev_priv = dev->dev_private;
 	buf = dev_priv->mmap_buffer;
@@ -104,7 +102,6 @@ static int i810_mmap_buffers(struct file *filp, struct vm_area_struct *vma)
 	vma->vm_file = filp;
 
 	buf_priv->currently_mapped = I810_BUF_MAPPED;
-	unlock_kernel();
 
 	if (io_remap_pfn_range(vma, vma->vm_start,
 			       vma->vm_pgoff,
@@ -116,7 +113,7 @@ static int i810_mmap_buffers(struct file *filp, struct vm_area_struct *vma)
 static const struct file_operations i810_buffer_fops = {
 	.open = drm_open,
 	.release = drm_release,
-	.unlocked_ioctl = i810_ioctl,
+	.unlocked_ioctl = drm_ioctl,
 	.mmap = i810_mmap_buffers,
 	.fasync = drm_fasync,
 	.llseek = noop_llseek,
@@ -1242,19 +1239,6 @@ int i810_driver_dma_quiescent(struct drm_device *dev)
 	return 0;
 }
 
-/*
- * call the drm_ioctl under the big kernel lock because
- * to lock against the i810_mmap_buffers function.
- */
-long i810_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
-{
-	int ret;
-	lock_kernel();
-	ret = drm_ioctl(file, cmd, arg);
-	unlock_kernel();
-	return ret;
-}
-
 struct drm_ioctl_desc i810_ioctls[] = {
 	DRM_IOCTL_DEF_DRV(I810_INIT, i810_dma_init, DRM_AUTH|DRM_MASTER|DRM_ROOT_ONLY|DRM_UNLOCKED),
 	DRM_IOCTL_DEF_DRV(I810_VERTEX, i810_dma_vertex, DRM_AUTH|DRM_UNLOCKED),
diff --git a/drivers/gpu/drm/i810/i810_drv.c b/drivers/gpu/drm/i810/i810_drv.c
index 88bcd33..7544ece 100644
--- a/drivers/gpu/drm/i810/i810_drv.c
+++ b/drivers/gpu/drm/i810/i810_drv.c
@@ -57,7 +57,7 @@ static struct drm_driver driver = {
 		 .owner = THIS_MODULE,
 		 .open = drm_open,
 		 .release = drm_release,
-		 .unlocked_ioctl = i810_ioctl,
+		 .unlocked_ioctl = drm_ioctl,
 		 .mmap = drm_mmap,
 		 .poll = drm_poll,
 		 .fasync = drm_fasync,
diff --git a/drivers/gpu/drm/i830/i830_dma.c b/drivers/gpu/drm/i830/i830_dma.c
index ca6f31f..0320030 100644
--- a/drivers/gpu/drm/i830/i830_dma.c
+++ b/drivers/gpu/drm/i830/i830_dma.c
@@ -36,7 +36,6 @@
 #include "i830_drm.h"
 #include "i830_drv.h"
 #include <linux/interrupt.h>	/* For task queue support */
-#include <linux/smp_lock.h>
 #include <linux/pagemap.h>
 #include <linux/delay.h>
 #include <linux/slab.h>
@@ -96,7 +95,6 @@ static int i830_mmap_buffers(struct file *filp, struct vm_area_struct *vma)
 	struct drm_buf *buf;
 	drm_i830_buf_priv_t *buf_priv;
 
-	lock_kernel();
 	dev = priv->minor->dev;
 	dev_priv = dev->dev_private;
 	buf = dev_priv->mmap_buffer;
@@ -106,7 +104,6 @@ static int i830_mmap_buffers(struct file *filp, struct vm_area_struct *vma)
 	vma->vm_file = filp;
 
 	buf_priv->currently_mapped = I830_BUF_MAPPED;
-	unlock_kernel();
 
 	if (io_remap_pfn_range(vma, vma->vm_start,
 			       vma->vm_pgoff,
@@ -118,7 +115,7 @@ static int i830_mmap_buffers(struct file *filp, struct vm_area_struct *vma)
 static const struct file_operations i830_buffer_fops = {
 	.open = drm_open,
 	.release = drm_release,
-	.unlocked_ioctl = i830_ioctl,
+	.unlocked_ioctl = drm_ioctl,
 	.mmap = i830_mmap_buffers,
 	.fasync = drm_fasync,
 	.llseek = noop_llseek,
@@ -1511,19 +1508,6 @@ int i830_driver_dma_quiescent(struct drm_device *dev)
 	return 0;
 }
 
-/*
- * call the drm_ioctl under the big kernel lock because
- * to lock against the i830_mmap_buffers function.
- */
-long i830_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
-{
-	int ret;
-	lock_kernel();
-	ret = drm_ioctl(file, cmd, arg);
-	unlock_kernel();
-	return ret;
-}
-
 struct drm_ioctl_desc i830_ioctls[] = {
 	DRM_IOCTL_DEF_DRV(I830_INIT, i830_dma_init, DRM_AUTH|DRM_MASTER|DRM_ROOT_ONLY|DRM_UNLOCKED),
 	DRM_IOCTL_DEF_DRV(I830_VERTEX, i830_dma_vertex, DRM_AUTH|DRM_UNLOCKED),
diff --git a/drivers/gpu/drm/i830/i830_drv.c b/drivers/gpu/drm/i830/i830_drv.c
index f655ab7..38378c9 100644
--- a/drivers/gpu/drm/i830/i830_drv.c
+++ b/drivers/gpu/drm/i830/i830_drv.c
@@ -68,7 +68,7 @@ static struct drm_driver driver = {
 		 .owner = THIS_MODULE,
 		 .open = drm_open,
 		 .release = drm_release,
-		 .unlocked_ioctl = i830_ioctl,
+		 .unlocked_ioctl = drm_ioctl,
 		 .mmap = drm_mmap,
 		 .poll = drm_poll,
 		 .fasync = drm_fasync,
-- 
1.7.1

Re: [PATCH 2/2] drm: i810/i830: kill BKL, mark as BROKEN_ON_SMP

[Dave Airlie]

On Wed, 2010-09-29 at 17:47 +0200, Arnd Bergmann wrote:
> The i810 and i830 drivers are the only hardware drivers that
> still use the BKL without anyone volunteering to fix them.

The problem is the userspace interface is badly designed and ABI, so
fixing these without the hw is messy, we know people have the hardware
because it breaks they still give out, we don't know anyone who cares
enough to fix it at this point in time.

> 
> The hardware is rather old and typically used on non-SMP
> systems. Mark them as BROKEN_ON_SMP and remove the BKL
> now so that people can still use the driver once the BKL
> is entirely gone.

You cannot get an SMP system with i810 or i830 support hardware in it,
however no distro ships UP kernels anymore they all ship SMP kernels
that hotplug/patch the second CPU. So the thing is although technically
these drivers are broken on SMP, they won't ever get run in SMP mode on
a combined UP/SMP kernel, and I'm not really sure BROKEN_ON_SMP takes
care of this.

I'm nearly sure the mmap_sem covers the problem anyways, and I should
try and dig out the i815 box I do have access to. Again though I've no
way of knowing we've not broken anything just by booting I assume. If we
introduce a race we would need real testing to find it.

Dave.

Re: [PATCH 2/2] drm: i810/i830: kill BKL, mark as BROKEN_ON_SMP

[Dave Airlie]

On Thu, Sep 30, 2010 at 8:15 AM, Dave Airlie <airlied@redhat.com> wrote:
> On Wed, 2010-09-29 at 17:47 +0200, Arnd Bergmann wrote:
>> The i810 and i830 drivers are the only hardware drivers that
>> still use the BKL without anyone volunteering to fix them.
>
> The problem is the userspace interface is badly designed and ABI, so
> fixing these without the hw is messy, we know people have the hardware
> because it breaks they still give out, we don't know anyone who cares
> enough to fix it at this point in time.
>
>>
>> The hardware is rather old and typically used on non-SMP
>> systems. Mark them as BROKEN_ON_SMP and remove the BKL
>> now so that people can still use the driver once the BKL
>> is entirely gone.
>
> You cannot get an SMP system with i810 or i830 support hardware in it,
> however no distro ships UP kernels anymore they all ship SMP kernels
> that hotplug/patch the second CPU. So the thing is although technically
> these drivers are broken on SMP, they won't ever get run in SMP mode on
> a combined UP/SMP kernel, and I'm not really sure BROKEN_ON_SMP takes
> care of this.

Okay so you can get a hyper-threaded 845 I've been informed, suck. But
anyways anyone using those GPUs is most likely running the i915
driver. i810 is the only one I really care about as it has no equiv.

Dave.