Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1461738 > unrolled thread

Re: [PATCH v3 02/14] media: mt9m111: prevent module removal while in use

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v3 02/14] media: mt9m111: prevent module removal while in  use Hans Verkuil <hverkuil@xs4all.nl> - 2016-08-14 11:10 +0200
    Re: [PATCH v3 02/14] media: mt9m111: prevent module removal while in use Robert Jarzmik <robert.jarzmik@free.fr> - 2016-08-14 21:40 +0200

#1461738 — Re: [PATCH v3 02/14] media: mt9m111: prevent module removal while in use

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

[toc] | [next] | [standalone]

#1462304 — Re: [PATCH v3 02/14] media: mt9m111: prevent module removal while in use

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web