Re: i8k: Don't revert affinity in i8k_smm
On Mon, 18 Aug 2014 07:32:15 PM Guenter Roeck wrote: > On Tue, Aug 19, 2014 at 09:19:55AM +1000, Con Kolivas wrote: > > As a followup to this discussion: > > > > On Tue, 15 Jul 2014 08:01:13 PM Sam Asadi wrote: > > > Commit f36fdb9f0266 (i8k: Force SMM to run on CPU 0) adds support > > > for multi-core CPUs to the driver. Unfortunately, that causes it > > > to fail loading if compiled without SMP support, at least on > > > 32 bit kernels. Kernel log shows "i8k: unable to get SMM Dell > > > signature", and function i8k_smm is found to return -EINVAL. > > > > > > Testing revealed that the culprit is the missing return value check > > > of set_cpus_allowed_ptr. > > > > It appears that the original commit f36fdb9f0266 changes the affinity for > > the duration of i8k_smm function and then unconditionally reverts the > > affinity to the old cpu mask regardless of whether the function succeeds > > or fails. As this must run on CPU 0 at all times it does not make sense > > to revert the affinity at the end of the function. Proposed patch > > attached. > > Sorry, I must have missed the rest of the discussion. What problem is this > patch supposed to fix ? Or, in other words, is there a problem with the > current code ? I also don't really understand the argument above. Why does > it not make sense to revert to the original affinity ? After all, only the > SMM call must run on CPU 0. Why does it not make sense to let the rest of > the code run on another CPU ? My mistake. If only the i8k_smm function needs to run on CPU 0 then it is appropriate to return affinity to the previous CPU mask. Please disregard and apologies for the noise. Thanks, Con -- -ck -- 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: i8k: Don't revert affinity in i8k_smm
On Mon, 18 Aug 2014 07:32:15 PM Guenter Roeck wrote: On Tue, Aug 19, 2014 at 09:19:55AM +1000, Con Kolivas wrote: As a followup to this discussion: On Tue, 15 Jul 2014 08:01:13 PM Sam Asadi wrote: Commit f36fdb9f0266 (i8k: Force SMM to run on CPU 0) adds support for multi-core CPUs to the driver. Unfortunately, that causes it to fail loading if compiled without SMP support, at least on 32 bit kernels. Kernel log shows i8k: unable to get SMM Dell signature, and function i8k_smm is found to return -EINVAL. Testing revealed that the culprit is the missing return value check of set_cpus_allowed_ptr. It appears that the original commit f36fdb9f0266 changes the affinity for the duration of i8k_smm function and then unconditionally reverts the affinity to the old cpu mask regardless of whether the function succeeds or fails. As this must run on CPU 0 at all times it does not make sense to revert the affinity at the end of the function. Proposed patch attached. Sorry, I must have missed the rest of the discussion. What problem is this patch supposed to fix ? Or, in other words, is there a problem with the current code ? I also don't really understand the argument above. Why does it not make sense to revert to the original affinity ? After all, only the SMM call must run on CPU 0. Why does it not make sense to let the rest of the code run on another CPU ? My mistake. If only the i8k_smm function needs to run on CPU 0 then it is appropriate to return affinity to the previous CPU mask. Please disregard and apologies for the noise. Thanks, Con -- -ck -- 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: i8k: Don't revert affinity in i8k_smm
On Tue, Aug 19, 2014 at 09:19:55AM +1000, Con Kolivas wrote: > As a followup to this discussion: > > On Tue, 15 Jul 2014 08:01:13 PM Sam Asadi wrote: > > Commit f36fdb9f0266 (i8k: Force SMM to run on CPU 0) adds support > > for multi-core CPUs to the driver. Unfortunately, that causes it > > to fail loading if compiled without SMP support, at least on > > 32 bit kernels. Kernel log shows "i8k: unable to get SMM Dell > > signature", and function i8k_smm is found to return -EINVAL. > > > > Testing revealed that the culprit is the missing return value check > > of set_cpus_allowed_ptr. > > It appears that the original commit f36fdb9f0266 changes the affinity for the > duration of i8k_smm function and then unconditionally reverts the affinity to > the old cpu mask regardless of whether the function succeeds or fails. As > this > must run on CPU 0 at all times it does not make sense to revert the affinity > at > the end of the function. Proposed patch attached. > Sorry, I must have missed the rest of the discussion. What problem is this patch supposed to fix ? Or, in other words, is there a problem with the current code ? I also don't really understand the argument above. Why does it not make sense to revert to the original affinity ? After all, only the SMM call must run on CPU 0. Why does it not make sense to let the rest of the code run on another CPU ? Thanks, Guenter > Signed-off-by: Con Kolivas > > --- > drivers/char/i8k.c |6 -- > 1 file changed, 6 deletions(-) > > Index: linux-3.16-ck1/drivers/char/i8k.c > === > --- linux-3.16-ck1.orig/drivers/char/i8k.c2014-08-12 14:07:49.0 > +1000 > +++ linux-3.16-ck1/drivers/char/i8k.c 2014-08-19 09:09:57.939056696 +1000 > @@ -132,12 +132,8 @@ static int i8k_smm(struct smm_regs *regs > { > int rc; > int eax = regs->eax; > - cpumask_var_t old_mask; > > /* SMM requires CPU 0 */ > - if (!alloc_cpumask_var(_mask, GFP_KERNEL)) > - return -ENOMEM; > - cpumask_copy(old_mask, >cpus_allowed); > rc = set_cpus_allowed_ptr(current, cpumask_of(0)); > if (rc) > goto out; > @@ -203,8 +199,6 @@ static int i8k_smm(struct smm_regs *regs > rc = -EINVAL; > > out: > - set_cpus_allowed_ptr(current, old_mask); > - free_cpumask_var(old_mask); > return rc; > } > > > -- > -ck > -- 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/
i8k: Don't revert affinity in i8k_smm
As a followup to this discussion: On Tue, 15 Jul 2014 08:01:13 PM Sam Asadi wrote: > Commit f36fdb9f0266 (i8k: Force SMM to run on CPU 0) adds support > for multi-core CPUs to the driver. Unfortunately, that causes it > to fail loading if compiled without SMP support, at least on > 32 bit kernels. Kernel log shows "i8k: unable to get SMM Dell > signature", and function i8k_smm is found to return -EINVAL. > > Testing revealed that the culprit is the missing return value check > of set_cpus_allowed_ptr. It appears that the original commit f36fdb9f0266 changes the affinity for the duration of i8k_smm function and then unconditionally reverts the affinity to the old cpu mask regardless of whether the function succeeds or fails. As this must run on CPU 0 at all times it does not make sense to revert the affinity at the end of the function. Proposed patch attached. Signed-off-by: Con Kolivas --- drivers/char/i8k.c |6 -- 1 file changed, 6 deletions(-) Index: linux-3.16-ck1/drivers/char/i8k.c === --- linux-3.16-ck1.orig/drivers/char/i8k.c 2014-08-12 14:07:49.0 +1000 +++ linux-3.16-ck1/drivers/char/i8k.c 2014-08-19 09:09:57.939056696 +1000 @@ -132,12 +132,8 @@ static int i8k_smm(struct smm_regs *regs { int rc; int eax = regs->eax; - cpumask_var_t old_mask; /* SMM requires CPU 0 */ - if (!alloc_cpumask_var(_mask, GFP_KERNEL)) - return -ENOMEM; - cpumask_copy(old_mask, >cpus_allowed); rc = set_cpus_allowed_ptr(current, cpumask_of(0)); if (rc) goto out; @@ -203,8 +199,6 @@ static int i8k_smm(struct smm_regs *regs rc = -EINVAL; out: - set_cpus_allowed_ptr(current, old_mask); - free_cpumask_var(old_mask); return rc; } -- -ck -- 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/
i8k: Don't revert affinity in i8k_smm
As a followup to this discussion: On Tue, 15 Jul 2014 08:01:13 PM Sam Asadi wrote: Commit f36fdb9f0266 (i8k: Force SMM to run on CPU 0) adds support for multi-core CPUs to the driver. Unfortunately, that causes it to fail loading if compiled without SMP support, at least on 32 bit kernels. Kernel log shows i8k: unable to get SMM Dell signature, and function i8k_smm is found to return -EINVAL. Testing revealed that the culprit is the missing return value check of set_cpus_allowed_ptr. It appears that the original commit f36fdb9f0266 changes the affinity for the duration of i8k_smm function and then unconditionally reverts the affinity to the old cpu mask regardless of whether the function succeeds or fails. As this must run on CPU 0 at all times it does not make sense to revert the affinity at the end of the function. Proposed patch attached. Signed-off-by: Con Kolivas ker...@kolivas.org --- drivers/char/i8k.c |6 -- 1 file changed, 6 deletions(-) Index: linux-3.16-ck1/drivers/char/i8k.c === --- linux-3.16-ck1.orig/drivers/char/i8k.c 2014-08-12 14:07:49.0 +1000 +++ linux-3.16-ck1/drivers/char/i8k.c 2014-08-19 09:09:57.939056696 +1000 @@ -132,12 +132,8 @@ static int i8k_smm(struct smm_regs *regs { int rc; int eax = regs-eax; - cpumask_var_t old_mask; /* SMM requires CPU 0 */ - if (!alloc_cpumask_var(old_mask, GFP_KERNEL)) - return -ENOMEM; - cpumask_copy(old_mask, current-cpus_allowed); rc = set_cpus_allowed_ptr(current, cpumask_of(0)); if (rc) goto out; @@ -203,8 +199,6 @@ static int i8k_smm(struct smm_regs *regs rc = -EINVAL; out: - set_cpus_allowed_ptr(current, old_mask); - free_cpumask_var(old_mask); return rc; } -- -ck -- 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: i8k: Don't revert affinity in i8k_smm
On Tue, Aug 19, 2014 at 09:19:55AM +1000, Con Kolivas wrote: As a followup to this discussion: On Tue, 15 Jul 2014 08:01:13 PM Sam Asadi wrote: Commit f36fdb9f0266 (i8k: Force SMM to run on CPU 0) adds support for multi-core CPUs to the driver. Unfortunately, that causes it to fail loading if compiled without SMP support, at least on 32 bit kernels. Kernel log shows i8k: unable to get SMM Dell signature, and function i8k_smm is found to return -EINVAL. Testing revealed that the culprit is the missing return value check of set_cpus_allowed_ptr. It appears that the original commit f36fdb9f0266 changes the affinity for the duration of i8k_smm function and then unconditionally reverts the affinity to the old cpu mask regardless of whether the function succeeds or fails. As this must run on CPU 0 at all times it does not make sense to revert the affinity at the end of the function. Proposed patch attached. Sorry, I must have missed the rest of the discussion. What problem is this patch supposed to fix ? Or, in other words, is there a problem with the current code ? I also don't really understand the argument above. Why does it not make sense to revert to the original affinity ? After all, only the SMM call must run on CPU 0. Why does it not make sense to let the rest of the code run on another CPU ? Thanks, Guenter Signed-off-by: Con Kolivas ker...@kolivas.org --- drivers/char/i8k.c |6 -- 1 file changed, 6 deletions(-) Index: linux-3.16-ck1/drivers/char/i8k.c === --- linux-3.16-ck1.orig/drivers/char/i8k.c2014-08-12 14:07:49.0 +1000 +++ linux-3.16-ck1/drivers/char/i8k.c 2014-08-19 09:09:57.939056696 +1000 @@ -132,12 +132,8 @@ static int i8k_smm(struct smm_regs *regs { int rc; int eax = regs-eax; - cpumask_var_t old_mask; /* SMM requires CPU 0 */ - if (!alloc_cpumask_var(old_mask, GFP_KERNEL)) - return -ENOMEM; - cpumask_copy(old_mask, current-cpus_allowed); rc = set_cpus_allowed_ptr(current, cpumask_of(0)); if (rc) goto out; @@ -203,8 +199,6 @@ static int i8k_smm(struct smm_regs *regs rc = -EINVAL; out: - set_cpus_allowed_ptr(current, old_mask); - free_cpumask_var(old_mask); return rc; } -- -ck -- 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/