From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ben Dooks Subject: Re: i2c: cdev lock_kernel() pushdown Date: Tue, 15 Jul 2008 18:33:44 +0100 Message-ID: <20080715173344.GM30539@fluff.org.uk> References: <20080715174155.2d52e6f6@hyperion.delvare> <20080715101406.6b4517e3@bike.lwn.net> <20080715183552.7e1b797a@hyperion.delvare> <9e4733910807151001r88ed121o4f13546e1d3d93ff@mail.gmail.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Content-Disposition: inline In-Reply-To: <9e4733910807151001r88ed121o4f13546e1d3d93ff-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: Linux I2C , Jonathan Corbet List-Id: linux-i2c@vger.kernel.org On Tue, Jul 15, 2008 at 01:01:07PM -0400, Jon Smirl wrote: > On 7/15/08, Jean Delvare wrote: > > On Tue, 15 Jul 2008 10:14:06 -0600, Jonathan Corbet wrote: > > > Hi, Jean, > > > > > > > I am looking at this patch of yours: > > > > http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff;h=3db633ee352bfe20d4a2b0c3c8a46ce31a6c7149 > > > > > > > > I believe that no locking is needed in i2cdev_open(). Do you have any > > > > reason to think it does? If not, can I simply revert this patch? > > > > > > Before now, i2cdev_open() has always had the protection of the BKL. > > > When I pushed that locking down into the individual open() functions, I > > > really had to take a pretty conservative approach and assume that the > > > BKL was needed unless that was really obviously not the case. In > > > i2cdev_open(), for example, there's: > > > > > > i2c_dev = i2c_dev_get_by_minor(minor); > > > > > > and I really don't know what keeps *i2c_dev from going away during the > > > rest of the call. I'm *not* saying there is a problem here; I just > > > don't know. So the only thing I could really do is to push the BKL > > > down and let the maintainers sort it out. > > > > > > ...all of which is my long-winded way of saying that, if you're > > > convinced that i2cdev_open() is safe in the absence of the BKL, feel > > > free to take it out. > > > > > > Good point. i2c_dev is guaranteed to stay thanks to the call to > > i2c_get_adapter(), however it happens _after_ the call to > > i2c_dev_get_by_minor(), so without additional locking, this is racy. > > That being said, I'm not sure how lock_kernel() is supposed to help. Is > > the BKL held when i2cdev_detach_adapter() is called? If not, then I > > suspect that the current code is already racy. > > > > I'll look into this, thanks for the hint. > > Is i2c-dev vulnerable to the i2c device disappearing, for example > rmmod it? Would combining i2c-dev into i2c core and integrating it > with the core's lock protection make things easier to lock? You could > make a compile time option to remove it for small systems. If it's in > the core is attach/detach adapter still needed? I haven't looked at > any of this in detail, but i2c-dev is only 6K of code. Half of the 6K > might disappear if integrated into the core. The i2c-dev code calls i2c_get_adapter() op open, so as long as the device stays open the adapter should not be able to go away as i2c_get_adater() calls try_module_get() on the adapter's module. The i2c-dev has it's own THIS_MODULE in the fops field, so should be kept as long as there is a file open. > What happens if user space and an in-kernel user both try using the > device? I've never tried doing that. Should the presence of an > in-kernel user make the user space device disappear? > > -- > Jon Smirl > jonsmirl-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org > > _______________________________________________ > i2c mailing list > i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org > http://lists.lm-sensors.org/mailman/listinfo/i2c -- Ben (ben-elnMNo+KYs3YtjvyW6yDsg@public.gmane.org, http://www.fluff.org/) 'a smiley only costs 4 bytes' _______________________________________________ i2c mailing list i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org http://lists.lm-sensors.org/mailman/listinfo/i2c