* re: bus: arm-ccn: cpumask attribute
@ 2015-05-14 10:13 Dan Carpenter
2015-05-14 10:50 ` Pawel Moll
0 siblings, 1 reply; 7+ messages in thread
From: Dan Carpenter @ 2015-05-14 10:13 UTC (permalink / raw)
To: kernel-janitors
Hello Pawel Moll,
The patch ffa415245b86: "bus: arm-ccn: cpumask attribute" from Apr
16, 2015, leads to the following static checker warning:
drivers/bus/arm-ccn.c:1188 arm_ccn_pmu_cpu_notifier()
warn: unsigned 'target' is never less than zero.
drivers/bus/arm-ccn.c
1175 static int arm_ccn_pmu_cpu_notifier(struct notifier_block *nb,
1176 unsigned long action, void *hcpu)
1177 {
1178 struct arm_ccn_dt *dt = container_of(nb, struct arm_ccn_dt, cpu_nb);
1179 struct arm_ccn *ccn = container_of(dt, struct arm_ccn, dt);
1180 unsigned int cpu = (long)hcpu; /* for (long) see kernel/cpu.c */
1181 unsigned int target;
1182
1183 switch (action & ~CPU_TASKS_FROZEN) {
1184 case CPU_DOWN_PREPARE:
1185 if (!cpumask_test_and_clear_cpu(cpu, &dt->cpu))
1186 break;
1187 target = cpumask_any_but(cpu_online_mask, cpu);
1188 if (target < 0)
^^^^^^^^^^
target is unsigned and cpumask_any_but() returns unsigned.
1189 break;
1190 perf_pmu_migrate_context(&dt->pmu, cpu, target);
1191 cpumask_set_cpu(target, &dt->cpu);
1192 WARN_ON(irq_set_affinity(ccn->irq, &dt->cpu) != 0);
1193 default:
1194 break;
1195 }
1196
1197 return NOTIFY_OK;
1198 }
regards,
dan carpenter
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH] bus: arm-ccn: Handle correctly no-more-cpus case
2015-05-14 10:13 bus: arm-ccn: cpumask attribute Dan Carpenter
@ 2015-05-14 10:50 ` Pawel Moll
0 siblings, 0 replies; 7+ messages in thread
From: Pawel Moll @ 2015-05-14 10:50 UTC (permalink / raw)
To: linux-arm-kernel
When migrating events the driver picks another cpu using
cpumask_any_but() function, which returns value >= nr_cpu_ids
when there is none available, not a negative value as the code
assumed. Fixed now.
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: Pawel Moll <pawel.moll@arm.com>
---
Another day, another arm-ccn.c update...
This time Dan's static checker spotted unsigned int target
being expected to carry negative values. Fixed now.
Interestingly enough, cpumask_any_but() implementation (and its
normal prototype) returns int, but version for NR_CPUS = 1 case,
inlined in linux/cpumask.h returns unsigned int...
drivers/bus/arm-ccn.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/bus/arm-ccn.c b/drivers/bus/arm-ccn.c
index 7d9879e..cc322fb 100644
--- a/drivers/bus/arm-ccn.c
+++ b/drivers/bus/arm-ccn.c
@@ -1184,7 +1184,7 @@ static int arm_ccn_pmu_cpu_notifier(struct notifier_block *nb,
if (!cpumask_test_and_clear_cpu(cpu, &dt->cpu))
break;
target = cpumask_any_but(cpu_online_mask, cpu);
- if (target < 0)
+ if (target >= nr_cpu_ids)
break;
perf_pmu_migrate_context(&dt->pmu, cpu, target);
cpumask_set_cpu(target, &dt->cpu);
--
2.1.0
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH] bus: arm-ccn: Handle correctly no-more-cpus case
@ 2015-05-14 10:50 ` Pawel Moll
0 siblings, 0 replies; 7+ messages in thread
From: Pawel Moll @ 2015-05-14 10:50 UTC (permalink / raw)
To: linux-arm-kernel
When migrating events the driver picks another cpu using
cpumask_any_but() function, which returns value >= nr_cpu_ids
when there is none available, not a negative value as the code
assumed. Fixed now.
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: Pawel Moll <pawel.moll@arm.com>
---
Another day, another arm-ccn.c update...
This time Dan's static checker spotted unsigned int target
being expected to carry negative values. Fixed now.
Interestingly enough, cpumask_any_but() implementation (and its
normal prototype) returns int, but version for NR_CPUS == 1 case,
inlined in linux/cpumask.h returns unsigned int...
drivers/bus/arm-ccn.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/bus/arm-ccn.c b/drivers/bus/arm-ccn.c
index 7d9879e..cc322fb 100644
--- a/drivers/bus/arm-ccn.c
+++ b/drivers/bus/arm-ccn.c
@@ -1184,7 +1184,7 @@ static int arm_ccn_pmu_cpu_notifier(struct notifier_block *nb,
if (!cpumask_test_and_clear_cpu(cpu, &dt->cpu))
break;
target = cpumask_any_but(cpu_online_mask, cpu);
- if (target < 0)
+ if (target >= nr_cpu_ids)
break;
perf_pmu_migrate_context(&dt->pmu, cpu, target);
cpumask_set_cpu(target, &dt->cpu);
--
2.1.0
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] bus: arm-ccn: Handle correctly no-more-cpus case
2015-05-14 10:50 ` Pawel Moll
@ 2015-05-14 11:04 ` Mark Rutland
-1 siblings, 0 replies; 7+ messages in thread
From: Mark Rutland @ 2015-05-14 11:04 UTC (permalink / raw)
To: linux-arm-kernel
On Thu, May 14, 2015 at 11:50:24AM +0100, Pawel Moll wrote:
> When migrating events the driver picks another cpu using
> cpumask_any_but() function, which returns value >= nr_cpu_ids
> when there is none available, not a negative value as the code
> assumed. Fixed now.
The fix looks good to me:
Acked-by: Mark Rutland <mark.rutland@arm.com>
Does this need to be CC'd to stable? What does perf_pmu_migrate_context
do when passed a target >= nr_cpus?
The original bug seems to have been copied over from arm-cci.c, which
will need the same fix. That appears to be my fault -- I'd mostly been
following the x86 uncore PMU drivers, but they figure out the target CPU
in a different way for which -1 is a sane error case.
I'll spin a patch for arm-cci.c momentarily.
Thanks,
Mark.
>
> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> Signed-off-by: Pawel Moll <pawel.moll@arm.com>
> ---
> Another day, another arm-ccn.c update...
>
> This time Dan's static checker spotted unsigned int target
> being expected to carry negative values. Fixed now.
>
> Interestingly enough, cpumask_any_but() implementation (and its
> normal prototype) returns int, but version for NR_CPUS = 1 case,
> inlined in linux/cpumask.h returns unsigned int...
>
> drivers/bus/arm-ccn.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/bus/arm-ccn.c b/drivers/bus/arm-ccn.c
> index 7d9879e..cc322fb 100644
> --- a/drivers/bus/arm-ccn.c
> +++ b/drivers/bus/arm-ccn.c
> @@ -1184,7 +1184,7 @@ static int arm_ccn_pmu_cpu_notifier(struct notifier_block *nb,
> if (!cpumask_test_and_clear_cpu(cpu, &dt->cpu))
> break;
> target = cpumask_any_but(cpu_online_mask, cpu);
> - if (target < 0)
> + if (target >= nr_cpu_ids)
> break;
> perf_pmu_migrate_context(&dt->pmu, cpu, target);
> cpumask_set_cpu(target, &dt->cpu);
> --
> 2.1.0
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH] bus: arm-ccn: Handle correctly no-more-cpus case
@ 2015-05-14 11:04 ` Mark Rutland
0 siblings, 0 replies; 7+ messages in thread
From: Mark Rutland @ 2015-05-14 11:04 UTC (permalink / raw)
To: linux-arm-kernel
On Thu, May 14, 2015 at 11:50:24AM +0100, Pawel Moll wrote:
> When migrating events the driver picks another cpu using
> cpumask_any_but() function, which returns value >= nr_cpu_ids
> when there is none available, not a negative value as the code
> assumed. Fixed now.
The fix looks good to me:
Acked-by: Mark Rutland <mark.rutland@arm.com>
Does this need to be CC'd to stable? What does perf_pmu_migrate_context
do when passed a target >= nr_cpus?
The original bug seems to have been copied over from arm-cci.c, which
will need the same fix. That appears to be my fault -- I'd mostly been
following the x86 uncore PMU drivers, but they figure out the target CPU
in a different way for which -1 is a sane error case.
I'll spin a patch for arm-cci.c momentarily.
Thanks,
Mark.
>
> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> Signed-off-by: Pawel Moll <pawel.moll@arm.com>
> ---
> Another day, another arm-ccn.c update...
>
> This time Dan's static checker spotted unsigned int target
> being expected to carry negative values. Fixed now.
>
> Interestingly enough, cpumask_any_but() implementation (and its
> normal prototype) returns int, but version for NR_CPUS == 1 case,
> inlined in linux/cpumask.h returns unsigned int...
>
> drivers/bus/arm-ccn.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/bus/arm-ccn.c b/drivers/bus/arm-ccn.c
> index 7d9879e..cc322fb 100644
> --- a/drivers/bus/arm-ccn.c
> +++ b/drivers/bus/arm-ccn.c
> @@ -1184,7 +1184,7 @@ static int arm_ccn_pmu_cpu_notifier(struct notifier_block *nb,
> if (!cpumask_test_and_clear_cpu(cpu, &dt->cpu))
> break;
> target = cpumask_any_but(cpu_online_mask, cpu);
> - if (target < 0)
> + if (target >= nr_cpu_ids)
> break;
> perf_pmu_migrate_context(&dt->pmu, cpu, target);
> cpumask_set_cpu(target, &dt->cpu);
> --
> 2.1.0
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] bus: arm-ccn: Handle correctly no-more-cpus case
2015-05-14 11:04 ` Mark Rutland
@ 2015-05-14 11:26 ` Mark Rutland
-1 siblings, 0 replies; 7+ messages in thread
From: Mark Rutland @ 2015-05-14 11:26 UTC (permalink / raw)
To: linux-arm-kernel
On Thu, May 14, 2015 at 12:04:28PM +0100, Mark Rutland wrote:
> On Thu, May 14, 2015 at 11:50:24AM +0100, Pawel Moll wrote:
> > When migrating events the driver picks another cpu using
> > cpumask_any_but() function, which returns value >= nr_cpu_ids
> > when there is none available, not a negative value as the code
> > assumed. Fixed now.
>
> The fix looks good to me:
>
> Acked-by: Mark Rutland <mark.rutland@arm.com>
>
> Does this need to be CC'd to stable? What does perf_pmu_migrate_context
> do when passed a target >= nr_cpus?
Never mind, it looks like we can never encounter that case anyway. In
the case of kexec or reset we won't notify CPU_DOWN_PREPARE on the final
CPU, and we can't hotplug the final CPU.
Which means that the target check is irrelevant as we should always get
a valid cpu back from cpumask_any_but in the cases we'll call it. So we
could just delete it entirely, assuming I haven't missed a
CPU_DOWN_PREPARE notification path...
Mark.
> > diff --git a/drivers/bus/arm-ccn.c b/drivers/bus/arm-ccn.c
> > index 7d9879e..cc322fb 100644
> > --- a/drivers/bus/arm-ccn.c
> > +++ b/drivers/bus/arm-ccn.c
> > @@ -1184,7 +1184,7 @@ static int arm_ccn_pmu_cpu_notifier(struct notifier_block *nb,
> > if (!cpumask_test_and_clear_cpu(cpu, &dt->cpu))
> > break;
> > target = cpumask_any_but(cpu_online_mask, cpu);
> > - if (target < 0)
> > + if (target >= nr_cpu_ids)
> > break;
> > perf_pmu_migrate_context(&dt->pmu, cpu, target);
> > cpumask_set_cpu(target, &dt->cpu);
> > --
> > 2.1.0
> >
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH] bus: arm-ccn: Handle correctly no-more-cpus case
@ 2015-05-14 11:26 ` Mark Rutland
0 siblings, 0 replies; 7+ messages in thread
From: Mark Rutland @ 2015-05-14 11:26 UTC (permalink / raw)
To: linux-arm-kernel
On Thu, May 14, 2015 at 12:04:28PM +0100, Mark Rutland wrote:
> On Thu, May 14, 2015 at 11:50:24AM +0100, Pawel Moll wrote:
> > When migrating events the driver picks another cpu using
> > cpumask_any_but() function, which returns value >= nr_cpu_ids
> > when there is none available, not a negative value as the code
> > assumed. Fixed now.
>
> The fix looks good to me:
>
> Acked-by: Mark Rutland <mark.rutland@arm.com>
>
> Does this need to be CC'd to stable? What does perf_pmu_migrate_context
> do when passed a target >= nr_cpus?
Never mind, it looks like we can never encounter that case anyway. In
the case of kexec or reset we won't notify CPU_DOWN_PREPARE on the final
CPU, and we can't hotplug the final CPU.
Which means that the target check is irrelevant as we should always get
a valid cpu back from cpumask_any_but in the cases we'll call it. So we
could just delete it entirely, assuming I haven't missed a
CPU_DOWN_PREPARE notification path...
Mark.
> > diff --git a/drivers/bus/arm-ccn.c b/drivers/bus/arm-ccn.c
> > index 7d9879e..cc322fb 100644
> > --- a/drivers/bus/arm-ccn.c
> > +++ b/drivers/bus/arm-ccn.c
> > @@ -1184,7 +1184,7 @@ static int arm_ccn_pmu_cpu_notifier(struct notifier_block *nb,
> > if (!cpumask_test_and_clear_cpu(cpu, &dt->cpu))
> > break;
> > target = cpumask_any_but(cpu_online_mask, cpu);
> > - if (target < 0)
> > + if (target >= nr_cpu_ids)
> > break;
> > perf_pmu_migrate_context(&dt->pmu, cpu, target);
> > cpumask_set_cpu(target, &dt->cpu);
> > --
> > 2.1.0
> >
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2015-05-14 11:26 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-05-14 10:13 bus: arm-ccn: cpumask attribute Dan Carpenter
2015-05-14 10:50 ` [PATCH] bus: arm-ccn: Handle correctly no-more-cpus case Pawel Moll
2015-05-14 10:50 ` Pawel Moll
2015-05-14 11:04 ` Mark Rutland
2015-05-14 11:04 ` Mark Rutland
2015-05-14 11:26 ` Mark Rutland
2015-05-14 11:26 ` Mark Rutland
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.