Re: [PATCH 1/2] driver core: firmware loader: don't cache FW_ACTION_NOHOTPLUG firmware
On Mon, May 27, 2013 at 6:52 PM, Ming Lei wrote: > On Mon, May 27, 2013 at 8:40 PM, anish singh > wrote: >> On Mon, May 27, 2013 at 5:53 PM, Takashi Iwai wrote: >>> At Mon, 27 May 2013 17:26:22 +0530, >>> anish singh wrote: On Mon, May 27, 2013 at 4:00 PM, 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 >> 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@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
On Mon, May 27, 2013 at 8:40 PM, anish singh wrote: > On Mon, May 27, 2013 at 5:53 PM, Takashi Iwai wrote: >> At Mon, 27 May 2013 17:26:22 +0530, >> anish singh wrote: >>> >>> On Mon, May 27, 2013 at 4:00 PM, 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 > 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@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
On Mon, May 27, 2013 at 5:53 PM, Takashi Iwai wrote: > At Mon, 27 May 2013 17:26:22 +0530, > anish singh wrote: >> >> On Mon, May 27, 2013 at 4:00 PM, 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 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 >> > Signed-off-by: Ming Lei >> > --- >> > 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 majord...@vger.kernel.org >> > More majordomo info at http://vger.kernel.org/majordomo-info.html >> > Please read the FAQ at http://www.tux.org/lkml/ >> -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@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
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 Reviewed-by: Takashi Iwai > Signed-off-by: Ming Lei > --- > 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 majord...@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
At Mon, 27 May 2013 17:26:22 +0530, anish singh wrote: > > On Mon, May 27, 2013 at 4:00 PM, 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 > 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 > > Signed-off-by: Ming Lei > > --- > > 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 majord...@vger.kernel.org > > More majordomo info at http://vger.kernel.org/majordomo-info.html > > Please read the FAQ at http://www.tux.org/lkml/ > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@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
On Mon, May 27, 2013 at 4:00 PM, 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 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 > Signed-off-by: Ming Lei > --- > 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 majord...@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > Please read the FAQ at http://www.tux.org/lkml/ -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH 1/2] driver core: firmware loader: don't cache FW_ACTION_NOHOTPLUG firmware
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 Signed-off-by: Ming Lei --- 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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/