From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jean Delvare Subject: Re: [PATCH] i2c: Push ioctl BKL down into the i2c code Date: Fri, 23 May 2008 18:40:49 +0200 Message-ID: <20080523184049.109ccc0a@hyperion.delvare> References: <20080522222327.1af72794@core> <20080523093545.175c769c@hyperion.delvare> <9e4733910805230723n2bbe9d4erf363b3c7b430d415@mail.gmail.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <9e4733910805230723n2bbe9d4erf363b3c7b430d415-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: i2c-bounces-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org Errors-To: i2c-bounces-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org To: Jon Smirl Cc: i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org, Alan Cox List-Id: linux-i2c@vger.kernel.org Hi Jon, On Fri, 23 May 2008 10:23:46 -0400, Jon Smirl wrote: > These BKL patches are part of a giant series of patches making the BLK > easier to remove. > There is a write up at LWN about the BKL. > http://lwn.net/Articles/281437/ Thanks for the pointer, it all makes sense now. I indeed didn't know that all ioctls were taking the BKL. > Higher up in the kernel a global lock (BKL - big kernel lock) was > being taken before each IOCTL was called (and some other things). This > has the effect of serializing all IOCTL calls in the kernel. There is > no real need to serialize these IOCTLs in most cases. > > Alan has removed the global lock/unlock code and now replicated it > across the 100s of IOCTLS in the kernel. This patch makes the > lock/unlock calls explicitly visible in the i2c code. He is trying to > remove this global lock since it hurts performance on SMP machines. > > Now some programming needs to happen. The i2c ioctls need to be > inspected to see if they really need to be locked against multiple SMP > processors simultaneously entering them. There are multiple answers to > this question, Do i2c IOCTLs need to be globally locked against all > other BLK users? Or do you only need a lock which is local to i2c? Or > is no lock needed at all? These question need to be answered for every > IOCTL in Linux. My impression is that we don't need any locking at all. But I am no locking expert, and it's easy to screw up that kind of things. > i2c probably needs a lock per bus or controller to keep two SMP CPUs > from trying to program the controller simultaneously. Previously the > BKL had been serializing this. This problem already exists (and is already solved) for the various I2C chip drivers inside the kernel: two drivers with chips on the same I2C bus might try to access the bus concurrently. We have a per-adapter mutex to protect against this. I fail to see how user-space access through i2c-dev would be fundamentally different. The only difference I can think of is that i2c-dev can create two clients at the same address while kernel drivers can't - but it doesn't seem to make a difference as far as locking is concerned. > So, the patch Alan supplied can be applied as is, it is just a > mechanically generated push down of the BKL. Applying it will continue > the same behavior as currently implemented in Linux. But Alan's patch > is probably not the optimal solution for i2c. OK... But my impression is that it is adding code which we will end up removing quickly because it's simply not needed. So I wonder if we shouldn't simply keep: > > > diff --git a/drivers/i2c/i2c-dev.c b/drivers/i2c/i2c-dev.c > > > index d34c14c..16cf0f2 100644 > > > --- a/drivers/i2c/i2c-dev.c > > > +++ b/drivers/i2c/i2c-dev.c > > > (...) > > > @@ -487,7 +509,7 @@ static const struct file_operations i2cdev_fops = { > > > .llseek = no_llseek, > > > .read = i2cdev_read, > > > .write = i2cdev_write, > > > - .ioctl = i2cdev_ioctl, > > > + .unlocked_ioctl = i2cdev_ioctl, > > > .open = i2cdev_open, > > > .release = i2cdev_release, > > > }; And discard all the rest. -- Jean Delvare _______________________________________________ i2c mailing list i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org http://lists.lm-sensors.org/mailman/listinfo/i2c