| Message ID | 1470684652-16295-3-git-send-email-robert.jarzmik@free.fr (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 08/08/2016 09:30 PM, Robert Jarzmik wrote: > The mt9m111 can be a removable module : the only case where the module > should be kept loaded is while it is used, ie. while an active > transation is ongoing on it. > > The notion of active transaction is mapped on the power state of the > module : if powered on the removal is prohibited. I don't really see the purpose of this patch: if this driver is loaded by a platform driver (such as pxa_camera), then the module count should be 1 and it isn't possible to unload. So you shouldn't need this patch. Am I missing something? No other driver in drivers/media/i2c does something like this. Regards, Hans > > Signed-off-by: Robert Jarzmik <robert.jarzmik@free.fr> > --- > drivers/media/i2c/soc_camera/mt9m111.c | 14 ++++++++++++-- > 1 file changed, 12 insertions(+), 2 deletions(-) > > diff --git a/drivers/media/i2c/soc_camera/mt9m111.c b/drivers/media/i2c/soc_camera/mt9m111.c > index a7efaa5964d1..ea5b5e709402 100644 > --- a/drivers/media/i2c/soc_camera/mt9m111.c > +++ b/drivers/media/i2c/soc_camera/mt9m111.c > @@ -780,23 +780,33 @@ static int mt9m111_power_on(struct mt9m111 *mt9m111) > struct i2c_client *client = v4l2_get_subdevdata(&mt9m111->subdev); > int ret; > > + if (!try_module_get(THIS_MODULE)) > + return -ENXIO; > + > ret = v4l2_clk_enable(mt9m111->clk); > if (ret < 0) > - return ret; > + goto out_module_put; > > ret = mt9m111_resume(mt9m111); > if (ret < 0) { > dev_err(&client->dev, "Failed to resume the sensor: %d\n", ret); > - v4l2_clk_disable(mt9m111->clk); > + goto out_clk_disable; > } > > return ret; > + > +out_clk_disable: > + v4l2_clk_disable(mt9m111->clk); > +out_module_put: > + module_put(THIS_MODULE); > + return ret; > } > > static void mt9m111_power_off(struct mt9m111 *mt9m111) > { > mt9m111_suspend(mt9m111); > v4l2_clk_disable(mt9m111->clk); > + module_put(THIS_MODULE); > } > > static int mt9m111_s_power(struct v4l2_subdev *sd, int on) > -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hans Verkuil <hverkuil@xs4all.nl> writes: > On 08/08/2016 09:30 PM, Robert Jarzmik wrote: >> The mt9m111 can be a removable module : the only case where the module >> should be kept loaded is while it is used, ie. while an active >> transation is ongoing on it. >> >> The notion of active transaction is mapped on the power state of the >> module : if powered on the removal is prohibited. > > I don't really see the purpose of this patch: if this driver is loaded > by a platform driver (such as pxa_camera), then the module count should be > 1 and it isn't possible to unload. > > So you shouldn't need this patch. Am I missing something? Well, what you are missing is ugly and twisted. With the current patchset, pxa_camera doesn't acquire the module (see pxa_camera_sensor_bound()). That is the ugly part. The reason behind is that if it acquires it, it's totally impossible to rmmod either pxa_camera or mt9m111 because of the cross-dependency I wasn't able to solve up to now. The dependency chain goes as follows : - pxa_camera should +1 refcount mt9m111 in pxa_camera_sensor_bound() - mt9m111 should +1 refcount pxa_camera through the MCLK get_clock() Without MCLK, mt9m111 is not clocked, and no I2C is available, hence no control and a total loss of context The purpose of this patch was the mt9m111 aquired a +1 refcount on pxa_camera so that pxa_camera is prevented from removal while a transfer is underway, and while the mt9m111 has the MCLK refcount acquired. I perfectly know this is suboptimal, but I didn't find a way yet to solve this. I was thinking to use pxac_fops_camera_open() to refcount +1 mt9m111 module and pxac_fops_camera_release() to refcound -1 mt9m111. But that didn't work because in order for mt9m111 to probe, the I2C has to be functional, and therefore the MCLK has to be provided. > No other driver in drivers/media/i2c does something like this. Yeah, I would bet either no other driver has the crossed dependency or they can never be rmmoded. Let me see how I can drop this patch and still prevent a kernel panic upon an rmmod of either mt9m111 or pxa_camera. Cheers. -- Robert -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/media/i2c/soc_camera/mt9m111.c b/drivers/media/i2c/soc_camera/mt9m111.c index a7efaa5964d1..ea5b5e709402 100644 --- a/drivers/media/i2c/soc_camera/mt9m111.c +++ b/drivers/media/i2c/soc_camera/mt9m111.c @@ -780,23 +780,33 @@ static int mt9m111_power_on(struct mt9m111 *mt9m111) struct i2c_client *client = v4l2_get_subdevdata(&mt9m111->subdev); int ret; + if (!try_module_get(THIS_MODULE)) + return -ENXIO; + ret = v4l2_clk_enable(mt9m111->clk); if (ret < 0) - return ret; + goto out_module_put; ret = mt9m111_resume(mt9m111); if (ret < 0) { dev_err(&client->dev, "Failed to resume the sensor: %d\n", ret); - v4l2_clk_disable(mt9m111->clk); + goto out_clk_disable; } return ret; + +out_clk_disable: + v4l2_clk_disable(mt9m111->clk); +out_module_put: + module_put(THIS_MODULE); + return ret; } static void mt9m111_power_off(struct mt9m111 *mt9m111) { mt9m111_suspend(mt9m111); v4l2_clk_disable(mt9m111->clk); + module_put(THIS_MODULE); } static int mt9m111_s_power(struct v4l2_subdev *sd, int on)
The mt9m111 can be a removable module : the only case where the module should be kept loaded is while it is used, ie. while an active transation is ongoing on it. The notion of active transaction is mapped on the power state of the module : if powered on the removal is prohibited. Signed-off-by: Robert Jarzmik <robert.jarzmik@free.fr> --- drivers/media/i2c/soc_camera/mt9m111.c | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-)