[PATCH 0/2] firmware loader: avoid blocking suspend by !uevent requests

[PATCH 0/2] firmware loader: avoid blocking suspend by !uevent requests

[Ming Lei]

Hi,

Recently, Takashi reported that requests with FW_ACTION_NOHOTPLUG may block
shutdown and fixed that by killing pending requests before shutdown. But
these requests still may block suspend, so the 2 patches is introduced to
fix the problem during suspend.

The 2nd patch depends on Takashi Iwai's patch of "firmware: Avoid deadlock of
usermodehelper lock at shutdown".

Thanks,
--
Ming Lei

[PATCH 1/2] driver core: firmware loader: don't cache FW_ACTION_NOHOTPLUG firmware

[Ming Lei]

Generally there are only two drivers which don't need uevent to
handle firmware loading, so don't cache these firmwares during
suspend for these drivers since doing that may block firmware
loading forever.

Both the two drivers are involved in private firmware images, so
they don't hit in direct loading too.

Cc: Takashi Iwai <tiwai@suse.de>
Signed-off-by: Ming Lei <ming.lei@canonical.com>
---
 drivers/base/firmware_class.c |    9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index e650c25..64e7870 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -993,7 +993,8 @@ _request_firmware_prepare(struct firmware **firmware_p, const char *name,
 	return 1; /* need to load */
 }
 
-static int assign_firmware_buf(struct firmware *fw, struct device *device)
+static int assign_firmware_buf(struct firmware *fw, struct device *device,
+				bool skip_cache)
 {
 	struct firmware_buf *buf = fw->priv;
 
@@ -1010,7 +1011,7 @@ static int assign_firmware_buf(struct firmware *fw, struct device *device)
 	 * device may has been deleted already, but the problem
 	 * should be fixed in devres or driver core.
 	 */
-	if (device)
+	if (device && !skip_cache)
 		fw_add_devm_name(device, buf->fw_id);
 
 	/*
@@ -1066,8 +1067,10 @@ _request_firmware(const struct firmware **firmware_p, const char *name,
 	if (!fw_get_filesystem_firmware(device, fw->priv))
 		ret = fw_load_from_user_helper(fw, name, device,
 					       uevent, nowait, timeout);
+
+	/* don't cache firmware handled without uevent */
 	if (!ret)
-		ret = assign_firmware_buf(fw, device);
+		ret = assign_firmware_buf(fw, device, !uevent);
 
 	usermodehelper_read_unlock();
 
-- 
1.7.9.5

Re: [PATCH 1/2] driver core: firmware loader: don't cache FW_ACTION_NOHOTPLUG firmware

[anish singh]

On Mon, May 27, 2013 at 4:00 PM, Ming Lei <ming.lei@canonical.com> wrote:
> Generally there are only two drivers which don't need uevent to
> handle firmware loading, so don't cache these firmwares during
Sorry but this statement confuses me i.e. "drivers which don't need
uevent to handle firmware loading". Does this mean that driver is
dependent on uevent to load the firmware?or does this mean
that driver generates uevent to userspace and userpace in turn
provides the firmware?
> suspend for these drivers since doing that may block firmware
> loading forever.
Explanation about why would it block would really help me or
for that matter anyone who reads this commit. Or may be
a url which discussed this problem.
>
> Both the two drivers are involved in private firmware images, so
> they don't hit in direct loading too.
>
> Cc: Takashi Iwai <tiwai@suse.de>
> Signed-off-by: Ming Lei <ming.lei@canonical.com>
> ---
>  drivers/base/firmware_class.c |    9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
> index e650c25..64e7870 100644
> --- a/drivers/base/firmware_class.c
> +++ b/drivers/base/firmware_class.c
> @@ -993,7 +993,8 @@ _request_firmware_prepare(struct firmware **firmware_p, const char *name,
>         return 1; /* need to load */
>  }
>
> -static int assign_firmware_buf(struct firmware *fw, struct device *device)
> +static int assign_firmware_buf(struct firmware *fw, struct device *device,
> +                               bool skip_cache)
>  {
>         struct firmware_buf *buf = fw->priv;
>
> @@ -1010,7 +1011,7 @@ static int assign_firmware_buf(struct firmware *fw, struct device *device)
>          * device may has been deleted already, but the problem
>          * should be fixed in devres or driver core.
>          */
> -       if (device)
> +       if (device && !skip_cache)
>                 fw_add_devm_name(device, buf->fw_id);
>
>         /*
> @@ -1066,8 +1067,10 @@ _request_firmware(const struct firmware **firmware_p, const char *name,
>         if (!fw_get_filesystem_firmware(device, fw->priv))
>                 ret = fw_load_from_user_helper(fw, name, device,
>                                                uevent, nowait, timeout);
> +
> +       /* don't cache firmware handled without uevent */
>         if (!ret)
> -               ret = assign_firmware_buf(fw, device);
> +               ret = assign_firmware_buf(fw, device, !uevent);
>
>         usermodehelper_read_unlock();
>
> --
> 1.7.9.5
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 1/2] driver core: firmware loader: don't cache FW_ACTION_NOHOTPLUG firmware

[Takashi Iwai]

At Mon, 27 May 2013 17:26:22 +0530,
anish singh wrote:
> 
> On Mon, May 27, 2013 at 4:00 PM, Ming Lei <ming.lei@canonical.com> wrote:
> > Generally there are only two drivers which don't need uevent to
> > handle firmware loading, so don't cache these firmwares during
> Sorry but this statement confuses me i.e. "drivers which don't need
> uevent to handle firmware loading". Does this mean that driver is
> dependent on uevent to load the firmware?

No.

> or does this mean
> that driver generates uevent to userspace and userpace in turn
> provides the firmware?

No.

The userspace doesn't require uevent, and the driver doesn't generate
uevent, either.  The userspace just loads the file when ready.
See Documentation/dell_rbu.txt for example.  (And yes, it's a bad
design.)


Takashi

> > suspend for these drivers since doing that may block firmware
> > loading forever.
> Explanation about why would it block would really help me or
> for that matter anyone who reads this commit. Or may be
> a url which discussed this problem.
> >
> > Both the two drivers are involved in private firmware images, so
> > they don't hit in direct loading too.
> >
> > Cc: Takashi Iwai <tiwai@suse.de>
> > Signed-off-by: Ming Lei <ming.lei@canonical.com>
> > ---
> >  drivers/base/firmware_class.c |    9 ++++++---
> >  1 file changed, 6 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
> > index e650c25..64e7870 100644
> > --- a/drivers/base/firmware_class.c
> > +++ b/drivers/base/firmware_class.c
> > @@ -993,7 +993,8 @@ _request_firmware_prepare(struct firmware **firmware_p, const char *name,
> >         return 1; /* need to load */
> >  }
> >
> > -static int assign_firmware_buf(struct firmware *fw, struct device *device)
> > +static int assign_firmware_buf(struct firmware *fw, struct device *device,
> > +                               bool skip_cache)
> >  {
> >         struct firmware_buf *buf = fw->priv;
> >
> > @@ -1010,7 +1011,7 @@ static int assign_firmware_buf(struct firmware *fw, struct device *device)
> >          * device may has been deleted already, but the problem
> >          * should be fixed in devres or driver core.
> >          */
> > -       if (device)
> > +       if (device && !skip_cache)
> >                 fw_add_devm_name(device, buf->fw_id);
> >
> >         /*
> > @@ -1066,8 +1067,10 @@ _request_firmware(const struct firmware **firmware_p, const char *name,
> >         if (!fw_get_filesystem_firmware(device, fw->priv))
> >                 ret = fw_load_from_user_helper(fw, name, device,
> >                                                uevent, nowait, timeout);
> > +
> > +       /* don't cache firmware handled without uevent */
> >         if (!ret)
> > -               ret = assign_firmware_buf(fw, device);
> > +               ret = assign_firmware_buf(fw, device, !uevent);
> >
> >         usermodehelper_read_unlock();
> >
> > --
> > 1.7.9.5
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > Please read the FAQ at  http://www.tux.org/lkml/
> 

Re: [PATCH 1/2] driver core: firmware loader: don't cache FW_ACTION_NOHOTPLUG firmware

[anish singh]

On Mon, May 27, 2013 at 5:53 PM, Takashi Iwai <tiwai@suse.de> wrote:
> At Mon, 27 May 2013 17:26:22 +0530,
> anish singh wrote:
>>
>> On Mon, May 27, 2013 at 4:00 PM, Ming Lei <ming.lei@canonical.com> wrote:
>> > Generally there are only two drivers which don't need uevent to
>> > handle firmware loading, so don't cache these firmwares during
Sorry but it confuses me further,
If driver doesn't have information about these firmware then how
does it cache it?
>> Sorry but this statement confuses me i.e. "drivers which don't need
>> uevent to handle firmware loading". Does this mean that driver is
>> dependent on uevent to load the firmware?
>
> No.
>
>> or does this mean
>> that driver generates uevent to userspace and userpace in turn
>> provides the firmware?
>
> No.
>
> The userspace doesn't require uevent, and the driver doesn't generate
> uevent, either.  The userspace just loads the file when ready.
> See Documentation/dell_rbu.txt for example.  (And yes, it's a bad
> design.)
>
>
> Takashi
>
>> > suspend for these drivers since doing that may block firmware
>> > loading forever.
>> Explanation about why would it block would really help me or
>> for that matter anyone who reads this commit. Or may be
>> a url which discussed this problem.
>> >
>> > Both the two drivers are involved in private firmware images, so
>> > they don't hit in direct loading too.
>> >
>> > Cc: Takashi Iwai <tiwai@suse.de>
>> > Signed-off-by: Ming Lei <ming.lei@canonical.com>
>> > ---
>> >  drivers/base/firmware_class.c |    9 ++++++---
>> >  1 file changed, 6 insertions(+), 3 deletions(-)
>> >
>> > diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
>> > index e650c25..64e7870 100644
>> > --- a/drivers/base/firmware_class.c
>> > +++ b/drivers/base/firmware_class.c
>> > @@ -993,7 +993,8 @@ _request_firmware_prepare(struct firmware **firmware_p, const char *name,
>> >         return 1; /* need to load */
>> >  }
>> >
>> > -static int assign_firmware_buf(struct firmware *fw, struct device *device)
>> > +static int assign_firmware_buf(struct firmware *fw, struct device *device,
>> > +                               bool skip_cache)
>> >  {
>> >         struct firmware_buf *buf = fw->priv;
>> >
>> > @@ -1010,7 +1011,7 @@ static int assign_firmware_buf(struct firmware *fw, struct device *device)
>> >          * device may has been deleted already, but the problem
>> >          * should be fixed in devres or driver core.
>> >          */
>> > -       if (device)
>> > +       if (device && !skip_cache)
>> >                 fw_add_devm_name(device, buf->fw_id);
>> >
>> >         /*
>> > @@ -1066,8 +1067,10 @@ _request_firmware(const struct firmware **firmware_p, const char *name,
>> >         if (!fw_get_filesystem_firmware(device, fw->priv))
>> >                 ret = fw_load_from_user_helper(fw, name, device,
>> >                                                uevent, nowait, timeout);
>> > +
>> > +       /* don't cache firmware handled without uevent */
>> >         if (!ret)
>> > -               ret = assign_firmware_buf(fw, device);
>> > +               ret = assign_firmware_buf(fw, device, !uevent);
>> >
>> >         usermodehelper_read_unlock();
>> >
>> > --
>> > 1.7.9.5
>> >
>> > --
>> > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
>> > the body of a message to majordomo@vger.kernel.org
>> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
>> > Please read the FAQ at  http://www.tux.org/lkml/
>>

Re: [PATCH 1/2] driver core: firmware loader: don't cache FW_ACTION_NOHOTPLUG firmware

[Ming Lei]

On Mon, May 27, 2013 at 8:40 PM, anish singh
<anish198519851985@gmail.com> wrote:
> On Mon, May 27, 2013 at 5:53 PM, Takashi Iwai <tiwai@suse.de> wrote:
>> At Mon, 27 May 2013 17:26:22 +0530,
>> anish singh wrote:
>>>
>>> On Mon, May 27, 2013 at 4:00 PM, Ming Lei <ming.lei@canonical.com> wrote:
>>> > Generally there are only two drivers which don't need uevent to
>>> > handle firmware loading, so don't cache these firmwares during
> Sorry but it confuses me further,

Please put one blank line before your reply, otherwise it is a bit
difficult to find your reply in email.

> If driver doesn't have information about these firmware then how
> does it cache it?

Driver only needs firmware's name for caching it, but driver can choose
if it generates uevent to let userspace handle it. Without one uevent, the
user space still can handle the request by waiting for the firmware sysfs
device in advance. But if there is no one user space to handle the request,
the request in kernel won't be completed and there is no timeout for this
case, so will block suspend.


Thanks,
--
Ming Lei

Re: [PATCH 1/2] driver core: firmware loader: don't cache FW_ACTION_NOHOTPLUG firmware

[anish singh]

On Mon, May 27, 2013 at 6:52 PM, Ming Lei <ming.lei@canonical.com> wrote:
> On Mon, May 27, 2013 at 8:40 PM, anish singh
> <anish198519851985@gmail.com> wrote:
>> On Mon, May 27, 2013 at 5:53 PM, Takashi Iwai <tiwai@suse.de> wrote:
>>> At Mon, 27 May 2013 17:26:22 +0530,
>>> anish singh wrote:
>>>>
>>>> On Mon, May 27, 2013 at 4:00 PM, Ming Lei <ming.lei@canonical.com> wrote:
>>>> > Generally there are only two drivers which don't need uevent to
>>>> > handle firmware loading, so don't cache these firmwares during
>> Sorry but it confuses me further,
>
> Please put one blank line before your reply, otherwise it is a bit
> difficult to find your reply in email.

sorry.

>
>> If driver doesn't have information about these firmware then how
>> does it cache it?
>
> Driver only needs firmware's name for caching it, but driver can choose
> if it generates uevent to let userspace handle it. Without one uevent, the
> user space still can handle the request by waiting for the firmware sysfs
> device in advance. But if there is no one user space to handle the request,
> the request in kernel won't be completed and there is no timeout for this
> case, so will block suspend.

Now makes sense.Thanks!!
>
>
> Thanks,
> --
> Ming Lei

Re: [PATCH 1/2] driver core: firmware loader: don't cache FW_ACTION_NOHOTPLUG firmware

[Takashi Iwai]

At Mon, 27 May 2013 18:30:03 +0800,
Ming Lei wrote:
> 
> Generally there are only two drivers which don't need uevent to
> handle firmware loading, so don't cache these firmwares during
> suspend for these drivers since doing that may block firmware
> loading forever.
> 
> Both the two drivers are involved in private firmware images, so
> they don't hit in direct loading too.
> 
> Cc: Takashi Iwai <tiwai@suse.de>

Reviewed-by: Takashi Iwai <tiwai@suse.de>


> Signed-off-by: Ming Lei <ming.lei@canonical.com>
> ---
>  drivers/base/firmware_class.c |    9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
> index e650c25..64e7870 100644
> --- a/drivers/base/firmware_class.c
> +++ b/drivers/base/firmware_class.c
> @@ -993,7 +993,8 @@ _request_firmware_prepare(struct firmware **firmware_p, const char *name,
>  	return 1; /* need to load */
>  }
>  
> -static int assign_firmware_buf(struct firmware *fw, struct device *device)
> +static int assign_firmware_buf(struct firmware *fw, struct device *device,
> +				bool skip_cache)
>  {
>  	struct firmware_buf *buf = fw->priv;
>  
> @@ -1010,7 +1011,7 @@ static int assign_firmware_buf(struct firmware *fw, struct device *device)
>  	 * device may has been deleted already, but the problem
>  	 * should be fixed in devres or driver core.
>  	 */
> -	if (device)
> +	if (device && !skip_cache)
>  		fw_add_devm_name(device, buf->fw_id);
>  
>  	/*
> @@ -1066,8 +1067,10 @@ _request_firmware(const struct firmware **firmware_p, const char *name,
>  	if (!fw_get_filesystem_firmware(device, fw->priv))
>  		ret = fw_load_from_user_helper(fw, name, device,
>  					       uevent, nowait, timeout);
> +
> +	/* don't cache firmware handled without uevent */
>  	if (!ret)
> -		ret = assign_firmware_buf(fw, device);
> +		ret = assign_firmware_buf(fw, device, !uevent);
>  
>  	usermodehelper_read_unlock();
>  
> -- 
> 1.7.9.5
> 

[PATCH 2/2] driver core: firmware loader: kill FW_ACTION_NOHOTPLUG requests before suspend

[Ming Lei]

This patch kills the firmware loading requests of FW_ACTION_NOHOTPLUG
before suspend to avoid blocking suspend because there is no timeout
for these requests.

Cc: Takashi Iwai <tiwai@suse.de>
Signed-off-by: Ming Lei <ming.lei@canonical.com>
---

This patch depends on Takashi Iwai's patch of "firmware: Avoid deadlock of
usermodehelper lock at shutdown"

 drivers/base/firmware_class.c |   17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index 64e7870..5690e72 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -128,6 +128,7 @@ struct firmware_buf {
 	size_t size;
 #ifdef CONFIG_FW_LOADER_USER_HELPER
 	bool is_paged_buf;
+	bool need_uevent;
 	struct page **pages;
 	int nr_pages;
 	int page_array_size;
@@ -873,6 +874,7 @@ static int _request_firmware_load(struct firmware_priv *fw_priv, bool uevent,
 	}
 
 	if (uevent) {
+		buf->need_uevent = true;
 		dev_set_uevent_suppress(f_dev, false);
 		dev_dbg(f_dev, "firmware: requesting %s\n", buf->fw_id);
 		if (timeout != MAX_SCHEDULE_TIMEOUT)
@@ -1407,6 +1409,20 @@ static void __device_uncache_fw_images(void)
 	spin_unlock(&fwc->name_lock);
 }
 
+/* kill pending requests without uevent to avoid blocking suspend */
+static void kill_requests_without_uevent(void)
+{
+	struct firmware_buf *buf;
+	struct firmware_buf *next;
+
+	mutex_lock(&fw_lock);
+	list_for_each_entry_safe(buf, next, &pending_fw_head, pending_list) {
+		if (!buf->need_uevent)
+			 fw_load_abort(buf);
+	}
+	mutex_unlock(&fw_lock);
+}
+
 /**
  * device_cache_fw_images - cache devices' firmware
  *
@@ -1486,6 +1502,7 @@ static int fw_pm_notify(struct notifier_block *notify_block,
 	switch (mode) {
 	case PM_HIBERNATION_PREPARE:
 	case PM_SUSPEND_PREPARE:
+		kill_requests_without_uevent();
 		device_cache_fw_images();
 		break;
 
-- 
1.7.9.5

Re: [PATCH 2/2] driver core: firmware loader: kill FW_ACTION_NOHOTPLUG requests before suspend

[Takashi Iwai]

At Mon, 27 May 2013 18:30:04 +0800,
Ming Lei wrote:
> 
> This patch kills the firmware loading requests of FW_ACTION_NOHOTPLUG
> before suspend to avoid blocking suspend because there is no timeout
> for these requests.
> 
> Cc: Takashi Iwai <tiwai@suse.de>
> Signed-off-by: Ming Lei <ming.lei@canonical.com>
> ---
> 
> This patch depends on Takashi Iwai's patch of "firmware: Avoid deadlock of
> usermodehelper lock at shutdown"

Reviewed-by: Takashi Iwai <tiwai@suse.de>


thanks,

Takashi

> 
>  drivers/base/firmware_class.c |   17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
> 
> diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
> index 64e7870..5690e72 100644
> --- a/drivers/base/firmware_class.c
> +++ b/drivers/base/firmware_class.c
> @@ -128,6 +128,7 @@ struct firmware_buf {
>  	size_t size;
>  #ifdef CONFIG_FW_LOADER_USER_HELPER
>  	bool is_paged_buf;
> +	bool need_uevent;
>  	struct page **pages;
>  	int nr_pages;
>  	int page_array_size;
> @@ -873,6 +874,7 @@ static int _request_firmware_load(struct firmware_priv *fw_priv, bool uevent,
>  	}
>  
>  	if (uevent) {
> +		buf->need_uevent = true;
>  		dev_set_uevent_suppress(f_dev, false);
>  		dev_dbg(f_dev, "firmware: requesting %s\n", buf->fw_id);
>  		if (timeout != MAX_SCHEDULE_TIMEOUT)
> @@ -1407,6 +1409,20 @@ static void __device_uncache_fw_images(void)
>  	spin_unlock(&fwc->name_lock);
>  }
>  
> +/* kill pending requests without uevent to avoid blocking suspend */
> +static void kill_requests_without_uevent(void)
> +{
> +	struct firmware_buf *buf;
> +	struct firmware_buf *next;
> +
> +	mutex_lock(&fw_lock);
> +	list_for_each_entry_safe(buf, next, &pending_fw_head, pending_list) {
> +		if (!buf->need_uevent)
> +			 fw_load_abort(buf);
> +	}
> +	mutex_unlock(&fw_lock);
> +}
> +
>  /**
>   * device_cache_fw_images - cache devices' firmware
>   *
> @@ -1486,6 +1502,7 @@ static int fw_pm_notify(struct notifier_block *notify_block,
>  	switch (mode) {
>  	case PM_HIBERNATION_PREPARE:
>  	case PM_SUSPEND_PREPARE:
> +		kill_requests_without_uevent();
>  		device_cache_fw_images();
>  		break;
>  
> -- 
> 1.7.9.5
>