Date: Tue, 15 Dec 2009 17:50:29 -0700
From: Jonathan Corbet <corbet@lwn.net>
To: Keith Mannthey <kmannth@us.ibm.com>
Cc: lkml <linux-kernel@vger.kernel.org>, John Stultz <johnstul@us.ibm.com>
Subject: Re: [RFC][Patch] IBM Real-Time "SMI Free" mode drive -v2
Message-ID: <20091215175029.1062f139@bike.lwn.net>
In-Reply-To: <1260923885.6521.41.camel@keith-laptop>
References: <1260907788.6521.10.camel@keith-laptop>
	<20091215164950.1fd41caf@bike.lwn.net>
	<1260923885.6521.41.camel@keith-laptop>
Organization: LWN.net
Mime-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 8bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 1943
Lines: 51

On Tue, 15 Dec 2009 16:38:05 -0800
Keith Mannthey <kmannth@us.ibm.com> wrote:

> > Do you need to create a new directory for a single .c file?
> 
> Perhaps not. I have a single .h and a single .c file. 
> Do you think I should move them right into misc (with maybe a better
> name the rtl.h)? 

That's what I would do, but this is a detail.

> > For a driver which is intended to help reduce latencies, a 10ms delay with
> > a spinlock held seems a little harsh.  It seems like maybe you could drop
> > the lock and use msleep()?
> 
> Irony is that doing this special write actually triggers an SMI. 
> 
> Well I really don't want any other possible action to happen until after
> the command finishes.  I can definitely shorten the amount of time of
> the mdelay but I don't want any possible way to start another write
> until the command finishes.  

Why not protect it with a mutex and use msleep()?  I see no reason why you
need a spinlock there.

> > > +			if (count++ > 10000) {
> > 
> > ...that's 100 seconds total - a *long* time...
> 
> The is a "possible" error condition that I have given a large window
> too. More than happy to shorten it up to say a small human amount of
> time maybe 2 seconds?

Whatever makes sense for the hardware.  Certainly there comes a point where
you can conclude it's not going to get back to you.  If you get rid of the
busy wait, you can delay a lot longer here without creating trouble.

> > Again, you should use ioread32() here.
> 
> I should never directly access the ioremap region? 

No, you really shouldn't.  Probably, on any machine where this driver is
relevant you get away with it just fine, but it's not the way to write
portable code.

jon
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/