Re: [RFC PATCH 12/13] driver core: firmware loader: use small timeout for cache device firmware
On Fri, Jul 27, 2012 at 6:35 PM, Borislav Petkov wrote: > On Fri, Jul 27, 2012 at 09:54:25AM +0800, Ming Lei wrote: >> On Fri, Jul 27, 2012 at 1:54 AM, Borislav Petkov wrote: >> >> >> No, it is not what I was saying. >> >> I just mean the point is not mentioned in my commit log, but I admit it >> should >> be a appropriate cause. >> >> > >> > Ok, maybe I'm not understanding this then. So explain to me this: why >> > do you need that timeout value of 10, how did we decide it to be 10 >> >> If one firmware image was loaded successfully before, the probability of >> loading it successfully at this time should be much higher than the 1st time >> because something crazy(for example, the firmware is deleted) happens >> with low probability. > > Believe it or not, I'm addressing exactly the possibility of the > firmware disappearing from under us in the AMD microcode driver > currently :) (and some other annoyances, of course). Of course, it is possible since user can delete it anytime, but with very low probability. > >> Choosing 10 secs is just a estimation for loading time because the maximum >> size of firmware in current distributions is about 2M bytes, since we know >> it has been loaded successfully before. > > This is exactly the comment we want over the code to explain to others > why we're choosing 10 secs. Simply add that sentence above the 10s > assignment and we're perfect! :-) OK, will add the comments in -v1. 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: [RFC PATCH 12/13] driver core: firmware loader: use small timeout for cache device firmware
On Fri, Jul 27, 2012 at 09:54:25AM +0800, Ming Lei wrote: > On Fri, Jul 27, 2012 at 1:54 AM, Borislav Petkov wrote: > > >> No, it is not what I was saying. > > I just mean the point is not mentioned in my commit log, but I admit it should > be a appropriate cause. > > > > > Ok, maybe I'm not understanding this then. So explain to me this: why > > do you need that timeout value of 10, how did we decide it to be 10 > > If one firmware image was loaded successfully before, the probability of > loading it successfully at this time should be much higher than the 1st time > because something crazy(for example, the firmware is deleted) happens > with low probability. Believe it or not, I'm addressing exactly the possibility of the firmware disappearing from under us in the AMD microcode driver currently :) (and some other annoyances, of course). > Choosing 10 secs is just a estimation for loading time because the maximum > size of firmware in current distributions is about 2M bytes, since we know > it has been loaded successfully before. This is exactly the comment we want over the code to explain to others why we're choosing 10 secs. Simply add that sentence above the 10s assignment and we're perfect! :-) > > (and not 20 or 30 or whatever)? Generally, why do we need to reprogram > > the timer to a smaller timeout instead of simply doing the completion > > without a timeout? > > No, it should be crazy without a timeout, and it can be triggered in init call > easily. Ok. 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 -- 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: [RFC PATCH 12/13] driver core: firmware loader: use small timeout for cache device firmware
On Fri, Jul 27, 2012 at 1:54 AM, Borislav Petkov wrote: >> No, it is not what I was saying. I just mean the point is not mentioned in my commit log, but I admit it should be a appropriate cause. > > Ok, maybe I'm not understanding this then. So explain to me this: why > do you need that timeout value of 10, how did we decide it to be 10 If one firmware image was loaded successfully before, the probability of loading it successfully at this time should be much higher than the 1st time because something crazy(for example, the firmware is deleted) happens with low probability. Choosing 10 secs is just a estimation for loading time because the maximum size of firmware in current distributions is about 2M bytes, since we know it has been loaded successfully before. > (and not 20 or 30 or whatever)? Generally, why do we need to reprogram > the timer to a smaller timeout instead of simply doing the completion > without a timeout? No, it should be crazy without a timeout, and it can be triggered in init call easily. 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: [RFC PATCH 12/13] driver core: firmware loader: use small timeout for cache device firmware
On Thu, Jul 26, 2012 at 11:48:17PM +0800, Ming Lei wrote: > On Thu, Jul 26, 2012 at 8:36 PM, Borislav Petkov wrote: > > On Wed, Jul 25, 2012 at 01:00:12AM +0800, Ming Lei wrote: > >> Because device_cache_firmwares only cache the firmware which has been > >> loaded sucessfully at leat once, using a small loading timeout should > > > > least > > > >> be OK. > > > > Your commit message doesn't explain why exactly we decrease the timeout: > > I have explained it. Because the firmware has been loaded successfully at > least > once, so it is very probably to not timeout. > > > you should probably say that this patch overrides the default 60s > > timeout because we're in pre-suspend/-hibernate mode where we have > > userspace and are able to load the firmware quickly. > > No, it is not what I was saying. Ok, maybe I'm not understanding this then. So explain to me this: why do you need that timeout value of 10, how did we decide it to be 10 (and not 20 or 30 or whatever)? Generally, why do we need to reprogram the timer to a smaller timeout instead of simply doing the completion without a timeout? -- 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 -- 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: [RFC PATCH 12/13] driver core: firmware loader: use small timeout for cache device firmware
On Thu, Jul 26, 2012 at 8:36 PM, Borislav Petkov wrote: > On Wed, Jul 25, 2012 at 01:00:12AM +0800, Ming Lei wrote: >> Because device_cache_firmwares only cache the firmware which has been >> loaded sucessfully at leat once, using a small loading timeout should > > least > >> be OK. > > Your commit message doesn't explain why exactly we decrease the timeout: I have explained it. Because the firmware has been loaded successfully at least once, so it is very probably to not timeout. > you should probably say that this patch overrides the default 60s > timeout because we're in pre-suspend/-hibernate mode where we have > userspace and are able to load the firmware quickly. No, it is not what I was saying. 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: [RFC PATCH 12/13] driver core: firmware loader: use small timeout for cache device firmware
On Wed, Jul 25, 2012 at 01:00:12AM +0800, Ming Lei wrote: > Because device_cache_firmwares only cache the firmware which has been > loaded sucessfully at leat once, using a small loading timeout should least > be OK. Your commit message doesn't explain why exactly we decrease the timeout: you should probably say that this patch overrides the default 60s timeout because we're in pre-suspend/-hibernate mode where we have userspace and are able to load the firmware quickly. 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 -- 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/
[RFC PATCH 12/13] driver core: firmware loader: use small timeout for cache device firmware
Because device_cache_firmwares only cache the firmware which has been
loaded sucessfully at leat once, using a small loading timeout should
be OK.
Signed-off-by: Ming Lei
---
drivers/base/firmware_class.c | 11 +++
1 file changed, 11 insertions(+)
diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index 7a96e75..0918b26 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -1130,10 +1130,19 @@ static void device_cache_firmwares(void)
{
struct firmware_cache *fwc = &fw_cache;
struct device *dev;
+ int old_timeout;
DEFINE_WAIT(wait);
pr_debug("%s\n", __func__);
+ /*
+* use small loading timeout for caching devces firmwares
+* because all these firmwares have been loaded successfully
+* at lease once
+*/
+ old_timeout = loading_timeout;
+ loading_timeout = 10;
+
device_pm_lock();
list_for_each_entry(dev, &dpm_list, power.entry)
dev_cache_firmware(dev);
@@ -1157,6 +1166,8 @@ static void device_cache_firmwares(void)
}
spin_unlock(&fwc->name_lock);
finish_wait(&fwc->wait_queue, &wait);
+
+ loading_timeout = old_timeout;
}
/**
--
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/
