| Message ID | 20221020124458.22153-1-farbere@amazon.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] edac: fix period calculation in edac_device_reset_delay_period() | expand |
On Thu, Oct 20, 2022 at 12:44:58PM +0000, Eliav Farber wrote: > Fix period calculation in case user sets a value of 1000. > The input of round_jiffies_relative() should be in jiffies and not in > milli-seconds. > > Signed-off-by: Eliav Farber <farbere@amazon.com> Fixes: c4cf3b454eca ("EDAC: Rework workqueue handling") I guess. Also, I think the one-liner below does the same, no? --- diff --git a/drivers/edac/edac_device.c b/drivers/edac/edac_device.c index 19522c568aa5..88942a6edc2c 100644 --- a/drivers/edac/edac_device.c +++ b/drivers/edac/edac_device.c @@ -399,7 +399,7 @@ void edac_device_reset_delay_period(struct edac_device_ctl_info *edac_dev, unsigned long jiffs = msecs_to_jiffies(value); if (value == 1000) - jiffs = round_jiffies_relative(value); + jiffs = round_jiffies_relative(jiffs); edac_dev->poll_msec = value; edac_dev->delay = jiffs;
On 12/29/2022 5:23 PM, Borislav Petkov wrote: > On Thu, Oct 20, 2022 at 12:44:58PM +0000, Eliav Farber wrote: >> Fix period calculation in case user sets a value of 1000. >> The input of round_jiffies_relative() should be in jiffies and not in >> milli-seconds. >> >> Signed-off-by: Eliav Farber <farbere@amazon.com> > > Fixes: c4cf3b454eca ("EDAC: Rework workqueue handling") > > I guess. > > Also, I think the one-liner below does the same, no? The one-liner below will not work. See the comment in edac_device_workq_setup() that explains why round is used: " optimize here for the 1 second case, which will be normal value, to fire ON the 1 second time event. This helps reduce all sorts of timers firing on sub-second basis, while they are happy to fire together on the 1 second exactly " So only the first schedule should be rounded. But all other schedules after that should be 1000ms. When rounding jiffs and saving it in edac_dev->delay it means that all future schedules will not be with the correct delay. The fix I suggested is the same logic used in: - edac_device_workq_setup() - edac_device_workq_function() -- Regards, Eliav > --- > > diff --git a/drivers/edac/edac_device.c b/drivers/edac/edac_device.c > index 19522c568aa5..88942a6edc2c 100644 > --- a/drivers/edac/edac_device.c > +++ b/drivers/edac/edac_device.c > @@ -399,7 +399,7 @@ void edac_device_reset_delay_period(struct > edac_device_ctl_info *edac_dev, > unsigned long jiffs = msecs_to_jiffies(value); > > if (value == 1000) > - jiffs = round_jiffies_relative(value); > + jiffs = round_jiffies_relative(jiffs); > > edac_dev->poll_msec = value; > edac_dev->delay = jiffs; > > -- > Regards/Gruss, > Boris. > > https://people.kernel.org/tglx/notes-about-netiquette
On Thu, Dec 29, 2022 at 10:17:48PM +0200, Farber, Eliav wrote: > The one-liner below will not work. See the comment in > edac_device_workq_setup() that explains why round is used: Bah, right you are - there's that "optimization". Ok, I've applied this: --- From: Eliav Farber <farbere@amazon.com> Date: Thu, 20 Oct 2022 12:44:58 +0000 Subject: [PATCH] EDAC/device: Fix period calculation in edac_device_reset_delay_period() Fix period calculation in case user sets a value of 1000. The input of round_jiffies_relative() should be in jiffies and not in milli-seconds. [ bp: Use the same code pattern as in edac_device_workq_setup() for clarity. ] Fixes: c4cf3b454eca ("EDAC: Rework workqueue handling") Signed-off-by: Eliav Farber <farbere@amazon.com> Signed-off-by: Borislav Petkov (AMD) <bp@alien8.de> Link: https://lore.kernel.org/r/20221020124458.22153-1-farbere@amazon.com --- drivers/edac/edac_device.c | 17 ++++++++--------- drivers/edac/edac_module.h | 2 +- 2 files changed, 9 insertions(+), 10 deletions(-) diff --git a/drivers/edac/edac_device.c b/drivers/edac/edac_device.c index 19522c568aa5..878deb4880cd 100644 --- a/drivers/edac/edac_device.c +++ b/drivers/edac/edac_device.c @@ -394,17 +394,16 @@ static void edac_device_workq_teardown(struct edac_device_ctl_info *edac_dev) * Then restart the workq on the new delay */ void edac_device_reset_delay_period(struct edac_device_ctl_info *edac_dev, - unsigned long value) + unsigned long msec) { - unsigned long jiffs = msecs_to_jiffies(value); - - if (value == 1000) - jiffs = round_jiffies_relative(value); - - edac_dev->poll_msec = value; - edac_dev->delay = jiffs; + edac_dev->poll_msec = msec; + edac_dev->delay = msecs_to_jiffies(msec); - edac_mod_work(&edac_dev->work, jiffs); + /* See comment in edac_device_workq_setup() above */ + if (edac_dev->poll_msec == 1000) + edac_mod_work(&edac_dev->work, round_jiffies_relative(edac_dev->delay)); + else + edac_mod_work(&edac_dev->work, edac_dev->delay); } int edac_device_alloc_index(void) diff --git a/drivers/edac/edac_module.h b/drivers/edac/edac_module.h index 763c076d96f2..47593afdc234 100644 --- a/drivers/edac/edac_module.h +++ b/drivers/edac/edac_module.h @@ -53,7 +53,7 @@ bool edac_stop_work(struct delayed_work *work); bool edac_mod_work(struct delayed_work *work, unsigned long delay); extern void edac_device_reset_delay_period(struct edac_device_ctl_info - *edac_dev, unsigned long value); + *edac_dev, unsigned long msec); extern void edac_mc_reset_delay_period(unsigned long value); /*
diff --git a/drivers/edac/edac_device.c b/drivers/edac/edac_device.c index 19522c568aa5..e944dd9b3593 100644 --- a/drivers/edac/edac_device.c +++ b/drivers/edac/edac_device.c @@ -398,13 +398,13 @@ void edac_device_reset_delay_period(struct edac_device_ctl_info *edac_dev, { unsigned long jiffs = msecs_to_jiffies(value); - if (value == 1000) - jiffs = round_jiffies_relative(value); - edac_dev->poll_msec = value; edac_dev->delay = jiffs; - edac_mod_work(&edac_dev->work, jiffs); + if (value == 1000) + edac_mod_work(&edac_dev->work, round_jiffies_relative(jiffs)); + else + edac_mod_work(&edac_dev->work, jiffs); } int edac_device_alloc_index(void)
Fix period calculation in case user sets a value of 1000. The input of round_jiffies_relative() should be in jiffies and not in milli-seconds. Signed-off-by: Eliav Farber <farbere@amazon.com> --- v2 --> v1: - Fix the bug without modifying jiffs which is used to set edac_dev->delay. drivers/edac/edac_device.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)