Re: [PATCH/RFC] core: add a function to safely try to get device driver owner

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Greg,

On Tuesday 30 November 2010 18:15:09 Greg KH wrote:
> 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:

[snip]

> > > > > > > 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?

It depends on your definition of rude. I would consider the kernel even more 
rude if it accepted my unload request and then crashed.

I've recently run into a problem similar to Guennadi's with the OMAP3 ISP 
driver. The driver instantiates several V4L2 I2C sub-devices for the camera 
sensors and the lens and flash controllers. The sub-device drivers get 
platform data when they're probed, and receive callbacks to the board code to 
turn power on/off and configure clocks (it's a bit more complex than just 
that, but you get the idea). The board code callbacks then call to the OMAP3 
ISP driver to configure clocks, because the sensor clock is provided by the 
OMAP3 ISP.

Now, when the user opens the sensor's subdev device node (/dev/v4l-subdev*), 
the subdev open function will turn the sensor clock on. To do that it will 
call the OMAP3 ISP driver through board code. If the OMAP3 ISP driver is 
unloaded at that point things will go pretty bad.

The way we deal with this is to try_module_get() on the OMAP3 ISP driver in 
the subdev open() handlers. I'm of course opened to alternatives.

> 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?

We need to lock the module in place if its code can be called from another 
driver. Coming up with a lock-free way to handle this would be similar to 
removing the try_module_get() call from cdev_get(). Maybe it could be done, 
but I'm not sure it should.

-- 
Regards,

Laurent Pinchart