Re: [RFC PATCH 08/13] driver core: firmware loader: fix device lifetime

[Borislav Petkov <bp@amd64.org>]

On Wed, Jul 25, 2012 at 01:00:08AM +0800, Ming Lei wrote:
> Callers of request_firmware* must hold the reference count of
> @device, otherwise it is easy to trigger oops since the firmware
> loader device is the child of @device.
> 
> This patch adds comments about the usage. In fact, most of drivers
> call request_firmware* in its probe() or open(), so the constraint
> should be reasonable and satisfied easily.
> 
> Also this patch holds the reference cound of @device before

					count

> schedule_work() in request_firmware_nowait() to avoid that
> the @device dies after request_firmware_nowait returns and before
> the work is scheduled.
> 
> Also request_firmware_nowait should be called in atomic context now,
> so fix the obsolete comments.
> 
> Signed-off-by: Ming Lei <ming.lei@canonical.com>
> ---
>  drivers/base/firmware_class.c |    9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
> index 674cb11..540b2e1 100644
> --- a/drivers/base/firmware_class.c
> +++ b/drivers/base/firmware_class.c
> @@ -717,6 +717,8 @@ err_put_dev:
>   *      @name will be used as $FIRMWARE in the uevent environment and
>   *      should be distinctive enough not to be confused with any other
>   *      firmware image for this or any other device.
> + *
> + *	Caller must hold the reference count of @device.
>   **/
>  int
>  request_firmware(const struct firmware **firmware_p, const char *name,
> @@ -798,6 +800,7 @@ static void request_firmware_work_func(struct work_struct *work)
>  
>   out:
>  	fw_work->cont(fw, fw_work->context);
> +	put_device(fw_work->device);
>  
>  	module_put(fw_work->module);
>  	kfree(fw_work);
> @@ -816,9 +819,10 @@ static void request_firmware_work_func(struct work_struct *work)
>   * @cont: function will be called asynchronously when the firmware
>   *	request is over.
>   *
> + *	Caller must hold the reference count of @device.
> + *
>   *	Asynchronous variant of request_firmware() for user contexts where
> - *	it is not possible to sleep for long time. It can't be called
> - *	in atomic contexts.
> + *	it is not possible to sleep for long time.

Let's state it explicitly:

	"it is not allowed to sleep for it is called in atomic context."

>   **/
>  int
>  request_firmware_nowait(
> @@ -844,6 +848,7 @@ request_firmware_nowait(
>  		return -EFAULT;
>  	}
>  
> +	get_device(fw_work->device);
>  	INIT_WORK(&fw_work->work, request_firmware_work_func);
>  	schedule_work(&fw_work->work);
>  	return 0;

Thanks.

-- 
Regards/Gruss,
Boris.

Advanced Micro Devices GmbH
Einsteinring 24, 85609 Dornach
GM: Alberto Bozzo
Reg: Dornach, Landkreis Muenchen
HRB Nr. 43632 WEEE Registernr: 129 19551