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

[Greg KH]

On Tue, Nov 30, 2010 at 06:55:54PM +0100, Laurent Pinchart wrote:
> 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 totally agree, and that is a bug that should be fixed, but shouldn't
have anything to do with this proposed interface (i.e. locking the
module in place is not the proper fix.)

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

Then the interface to call that driver should be properly reference
counted, right?  That has nothing to do with the driver core locking
modules into place.

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

Do it like the rest of the kernel does it, lock the module in place with
the module pointer it passed to you before calling open in that module.
Nothing new here at all.

thanks,

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