infiniband BKL leftovers

infiniband BKL leftovers

[Thomas Gleixner]

Roland,

I'm looking into removing cycle_kernel_lock() from
drivers/infiniband/hw/ipath/ipath_file_ops.c .

cycle_kernel_lock() was pushed down into open functions to "emulate"
the previous BKL protected semantics by "serializing" the open
function against driver init in progress. Nobody knows how effective
this "serialization" was in reality. :)

This protection is not necessary when everything what might be needed
by write/aio_write/poll/mmap should be in place before
ipath_user_add() creates the device node.

But looking at the callsite ipath_init_one() I'm not so sure about that.

At the end of ipath_init_one() I see:

        ipath_device_create_group(&pdev->dev, dd);
        ipathfs_add_device(dd);
        ipath_user_add(dd);
        ipath_diag_add(dd);
        ipath_register_ib_device(dd);

ipath_user_add() is called before ipath_register_ib_device() which
registers the device with the infiniband core. That makes me wonder
whether the device node is really ready to use right after it is
created.

Aside of that I noticed that all the functions above have elaborate
error pathes, but non of the return values of these functions is ever
checked. Intersting approach :)

Thanks,

	tglx

Re: infiniband BKL leftovers

[Roland Dreier]

[Adding a few CCs]

 > I'm looking into removing cycle_kernel_lock() from
 > drivers/infiniband/hw/ipath/ipath_file_ops.c .
 > 
 > cycle_kernel_lock() was pushed down into open functions to "emulate"
 > the previous BKL protected semantics by "serializing" the open
 > function against driver init in progress. Nobody knows how effective
 > this "serialization" was in reality. :)
 > 
 > This protection is not necessary when everything what might be needed
 > by write/aio_write/poll/mmap should be in place before
 > ipath_user_add() creates the device node.
 > 
 > But looking at the callsite ipath_init_one() I'm not so sure about that.
 > 
 > At the end of ipath_init_one() I see:
 > 
 >         ipath_device_create_group(&pdev->dev, dd);
 >         ipathfs_add_device(dd);
 >         ipath_user_add(dd);
 >         ipath_diag_add(dd);
 >         ipath_register_ib_device(dd);
 > 
 > ipath_user_add() is called before ipath_register_ib_device() which
 > registers the device with the infiniband core. That makes me wonder
 > whether the device node is really ready to use right after it is
 > created.
 > 
 > Aside of that I noticed that all the functions above have elaborate
 > error pathes, but non of the return values of these functions is ever
 > checked. Intersting approach :)

I guess you guys are getting serious about BKL removal.  I think I made
about as much progress on the ipath driver when the BKL pushdown first
happened: I looked at the code with an eye to getting rid of the BKL and
got too confused and scared to proceed.

I'm not sure what the best way to handle this is; as far as I know the
ipath code is abandoned by Qlogic (they have a new driver that also
supports newer hw that they have not tried to get upstream yet) and BKL
issues aside the code looks racy, eg

	if (atomic_inc_return(&user_count) == 1) {
		ret = user_init();

makes sure that only one thread does user_init() but it doesn't make
sure that user_init() finishes before another thread proceeds.

So I guess if the BKL stuff is blocking you in any way, we can just drop
it from ipath and leave it as yet another race condition in a rotting
old driver.

 - R.

Re: infiniband BKL leftovers

[Roland Dreier]

[Adding Ralph's direct email]

And the address in MAINTAINERS of infinipath@qlogic.com appears to be
bouncing... so I think it's pretty safe to say that Qlogic doesn't care
too much about the ipath driver any more.

 - R.

Re: infiniband BKL leftovers

[Thomas Gleixner]

On Sun, 11 Oct 2009, Roland Dreier wrote:

> [Adding Ralph's direct email]
> 
> And the address in MAINTAINERS of infinipath@qlogic.com appears to be
> bouncing... so I think it's pretty safe to say that Qlogic doesn't care
> too much about the ipath driver any more.

Time to create drivers/bitrotting which serves the opposite purpose of
drivers/staging :) I have a huge list of candidates for that.

Thanks,

	tglx

[tip:bkl/drivers] inifiband: Remove BKL from ipath_open()

[tip-bot for Thomas Gleixner]

Commit-ID:  f96d3015e9f7f7fff4cab7ed1d467664cc980061
Gitweb:     http://git.kernel.org/tip/f96d3015e9f7f7fff4cab7ed1d467664cc980061
Author:     Thomas Gleixner <tglx@linutronix.de>
AuthorDate: Wed, 14 Oct 2009 16:36:26 +0200
Committer:  Thomas Gleixner <tglx@linutronix.de>
CommitDate: Wed, 14 Oct 2009 17:36:54 +0200

inifiband: Remove BKL from ipath_open()

cycle_kernel_lock() got pushed down to ipath_open(). I tried hard to
understand what it might protect, but finally gave up.

Roland noted that qlogic seems to have abandoned the ipath driver and
came to the following wise conclusion: "So I guess if the BKL stuff is
blocking you in any way, we can just drop it from ipath and leave it
as yet another race condition in a rotting old driver."

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
LKML-Reference: <adad44tj090.fsf@cisco.com>
Cc: Roland Dreier <rdreier@cisco.com>
---
 drivers/infiniband/hw/ipath/ipath_file_ops.c |    1 -
 1 files changed, 0 insertions(+), 1 deletions(-)

diff --git a/drivers/infiniband/hw/ipath/ipath_file_ops.c b/drivers/infiniband/hw/ipath/ipath_file_ops.c
index 40dbe54..73933a4 100644
--- a/drivers/infiniband/hw/ipath/ipath_file_ops.c
+++ b/drivers/infiniband/hw/ipath/ipath_file_ops.c
@@ -1821,7 +1821,6 @@ done:
 static int ipath_open(struct inode *in, struct file *fp)
 {
 	/* The real work is performed later in ipath_assign_port() */
-	cycle_kernel_lock();
 	fp->private_data = kzalloc(sizeof(struct ipath_filedata), GFP_KERNEL);
 	return fp->private_data ? 0 : -ENOMEM;
 }