Message-ID: <20101130171509.GA18794@kroah.com>
Date:	Tue, 30 Nov 2010 09:15:09 -0800
From:	Greg KH <greg@...ah.com>
To:	Guennadi Liakhovetski <g.liakhovetski@....de>
Cc:	Jonathan Corbet <corbet@....net>, linux-kernel@...r.kernel.org,
	Linux Media Mailing List <linux-media@...r.kernel.org>
Subject: Re: [PATCH/RFC] core: add a function to safely try to get device
 driver owner

On Tue, Nov 30, 2010 at 06:09:46PM +0100, Guennadi Liakhovetski wrote:
> On Tue, 30 Nov 2010, Greg KH wrote:
> 
> > On Tue, Nov 30, 2010 at 12:11:42AM +0100, Guennadi Liakhovetski wrote:
> > > On Mon, 29 Nov 2010, Greg KH wrote:
> > > 
> > > > On Mon, Nov 29, 2010 at 11:10:50PM +0100, Guennadi Liakhovetski wrote:
> > > > > On Mon, 29 Nov 2010, Greg KH wrote:
> > > > > 
> > > > > > On Mon, Nov 29, 2010 at 09:54:10PM +0100, Guennadi Liakhovetski wrote:
> > > > > > > Hi Jon
> > > > > > > 
> > > > > > > On Mon, 29 Nov 2010, Jonathan Corbet wrote:
> > > > > > > 
> > > > > > > > On Mon, 29 Nov 2010 20:43:28 +0100 (CET)
> > > > > > > > Guennadi Liakhovetski <g.liakhovetski@....de> wrote:
> > > > > > > > 
> > > > > > > > > When two drivers interoperate without an explicit dependency, it is often
> > > > > > > > > required to prevent one of them from being unloaded safely by dereferencing
> > > > > > > > > dev->driver->owner. This patch provides a generic function to do this in a
> > > > > > > > > race-free way.
> > > > > > > > 
> > > > > > > > I must ask: why not, instead, make the dependency explicit?  In
> > > > > > > > particular, this looks like an application for the proposed media
> > > > > > > > controller code, which is meant to model the connections between otherwise
> > > > > > > > independent devices.  The fact that your example comes from V4L2 (which is
> > > > > > > > the current domain of the media controller) also argues that way.
> > > > > > > 
> > > > > > > Sorry, don't see a good way to do this. This function is for a general 
> > > > > > > dependency, where you don't have that driver, we are checking for register 
> > > > > > > with us, so, the only way to get to it is via dev->driver->owner.
> > > > > > 
> > > > > > Wait, what?  The device is already bound to a driver, right, so why
> > > > > > would you care about "locking" the module into memory?  What could this
> > > > > > possibly be used for?
> > > > > 
> > > > > To protect against rmmod -> driver_unregister -> dev->driver = NULL?
> > > > 
> > > > But again, why would some other driver ever care about what some random
> > > > dev->driver would be?
> > > 
> > > It's not a random one, call it a "companion device."
> > 
> > Ok, but again go back to Jon's original proposal to just call the
> > functions in that driver from yours, causing the implicit module
> > ordering issue to be automatically resolved.
> 
> Greg, in this specific case - yes, I could do this. But (1) there is no 
> need for that - both drivers implement and use the v4l2-subdev API and 
> thus stay generic. In the host driver this adds the convenience, that it 
> doesn't have to call to the CSI2 driver explicitly at all - it just calls 
> the v4l2-subdev function like "call .s_mbus_fmt for all subdev drivers" 
> and the function is called for the sensor and the CSI2 driver. (2) what 
> about the other location I pointed out earlier in the v4l2 core? There 
> drivers are absolutely generic. I also suspect these are not the only 
> cases, where this helper would come in handy. I added the media list to CC 
> for any more opinions on this matter.

I agree, it probably would not solve all of the different issues that
people might have for this type of thing, and this isn't the first time
I've heard it be requested either.

But, this patch is just trying to increment a module owner of a device
that is bound to a driver, which is the wrong level to be thinking of
it.

If you request a module to be loaded, what would possibly cause it to be
unbound that you need to have this "safely" in place?  Why would the
module be unloaded?  And if it was unloaded, doesn't that imply that
someone else wanted it unloaded so keeping that from happening would be
a bit rude, right?

Look at network modules, we always allow them to be unloaded, even if
the device is "in use" and that doesn't cause problems.  So why would
you need to do this when we are trying (over the past 10 years or so) to
move away from the "lock the module in place because we know better than
the user" model?

thanks,

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

Powered by blists - more mailing lists