Missing put_cpu() in arch/ia64/sn/kernel/sn2/sn_hwperf.c?

Missing put_cpu() in arch/ia64/sn/kernel/sn2/sn_hwperf.c?

[Thomas Meyer]

The function sn_hwperf_op_cpu() seems to miss a corresponding put_cpu().

Or is this done in another function? I didn't find it.

thomas

---

 Check for get/put_cpu() imbalances

 The simplified semantic patch that makes this report is:

* get_cpu()
... when != put_cpu()
? get_cpu()

drivers/crypto/n2_core.c:986:25-26: WARNING: Possible missing put_cpu()!
drivers/crypto/n2_core.c:1041:25-26: WARNING: Possible missing put_cpu()!
drivers/scsi/fcoe/fcoe.c:1163:29-30: WARNING: Possible missing put_cpu()!
drivers/scsi/fcoe/fcoe.c:1630:47-48: WARNING: Possible missing put_cpu()!
arch/um/sys-i386/ldt.c:53:17-18: WARNING: Possible missing put_cpu()!
arch/powerpc/kernel/machine_kexec_64.c:188:18-19: WARNING: Possible missing put_cpu()!
arch/ia64/sn/kernel/sn2/sn_hwperf.c:618:52-53: WARNING: Possible missing put_cpu()!
arch/s390/oprofile/hwsampler.c:553:16-17: WARNING: Possible missing put_cpu()!
arch/s390/kernel/vtime.c:430:22-23: WARNING: Possible missing put_cpu()!
arch/s390/kernel/vtime.c:473:15-16: WARNING: Possible missing put_cpu()!
arch/blackfin/kernel/cplbinfo.c:89:9-10: WARNING: Possible missing put_cpu()!

This list contains some false positives.

Thanks to Julia for the semantic patch to find these kind of errors!

Re: Missing put_cpu() in arch/ia64/sn/kernel/sn2/sn_hwperf.c?

[Tony Luck]

[-- Attachment #1: Type: text/plain, Size: 453 bytes --]

On Mon, Aug 8, 2011 at 11:05 AM, Thomas Meyer <thomas@m3y3r.de> wrote:
> The function sn_hwperf_op_cpu() seems to miss a corresponding put_cpu().
>
> Or is this done in another function? I didn't find it.

It would be hard to do it elsewhere - this function may not have done
a get_cpu() [in the cpu == SN_HWPERF_ARG_ANY_CPU case].

The logic is a bit tortuous here ... perhaps simpler to split the tests
up.  Does the attached patch look right?

-Tony

[-- Attachment #2: sn_hwperf_op_cpu.patch --]
[-- Type: text/x-patch, Size: 777 bytes --]

diff --git a/arch/ia64/sn/kernel/sn2/sn_hwperf.c b/arch/ia64/sn/kernel/sn2/sn_hwperf.c
index 30862c0..2de41d4 100644
--- a/arch/ia64/sn/kernel/sn2/sn_hwperf.c
+++ b/arch/ia64/sn/kernel/sn2/sn_hwperf.c
@@ -615,11 +615,15 @@ static int sn_hwperf_op_cpu(struct sn_hwperf_op_info *op_info)
 		}
 	}
 
-	if (cpu == SN_HWPERF_ARG_ANY_CPU || cpu == get_cpu()) {
-		/* don't care, or already on correct cpu */
+	if (cpu == SN_HWPERF_ARG_ANY_CPU) {
+		/* don't care which cpu */
 		sn_hwperf_call_sal(op_info);
-	}
-	else {
+	} else if (cpu == get_cpu()) {
+		/* already on correct cpu */
+		sn_hwperf_call_sal(op_info);
+		put_cpu();
+	} else {
+		put_cpu();
 		if (use_ipi) {
 			/* use an interprocessor interrupt to call SAL */
 			smp_call_function_single(cpu, sn_hwperf_call_sal,

Re: Missing put_cpu() in arch/ia64/sn/kernel/sn2/sn_hwperf.c?

[Jes Sorensen]

On 08/08/11 23:37, Tony Luck wrote:
> On Mon, Aug 8, 2011 at 11:05 AM, Thomas Meyer <thomas@m3y3r.de> wrote:
>> The function sn_hwperf_op_cpu() seems to miss a corresponding put_cpu().
>>
>> Or is this done in another function? I didn't find it.
> 
> It would be hard to do it elsewhere - this function may not have done
> a get_cpu() [in the cpu == SN_HWPERF_ARG_ANY_CPU case].
> 
> The logic is a bit tortuous here ... perhaps simpler to split the tests
> up.  Does the attached patch look right?

Hi Tony,

You probably want someone from SGI to look at it, so I'll forward to
Robin Holt.

I haven't been at SGI for about 2 years now :)

Cheers,
Jes

Re: Missing put_cpu() in arch/ia64/sn/kernel/sn2/sn_hwperf.c?

[Robin Holt]

On Thu, Aug 11, 2011 at 08:12:45AM +0200, Jes Sorensen wrote:
> On 08/08/11 23:37, Tony Luck wrote:
> > On Mon, Aug 8, 2011 at 11:05 AM, Thomas Meyer <thomas@m3y3r.de> wrote:
> >> The function sn_hwperf_op_cpu() seems to miss a corresponding put_cpu().
> >>
> >> Or is this done in another function? I didn't find it.
> > 
> > It would be hard to do it elsewhere - this function may not have done
> > a get_cpu() [in the cpu == SN_HWPERF_ARG_ANY_CPU case].
> > 
> > The logic is a bit tortuous here ... perhaps simpler to split the tests
> > up.  Does the attached patch look right?
> 
> Hi Tony,
> 
> You probably want someone from SGI to look at it, so I'll forward to
> Robin Holt.
> 
> I haven't been at SGI for about 2 years now :)

And I will bounce the question on to Jack.  I believe he will probably
pass it on to somebody more familiar with sn_hwperf.  Tony's patch looks
much clearer to me.  I would have answered that I thought this was the
right patch until I saw the '... else { put_cpu(); ...' which made me
think we need a closer look.

Thanks,
Robin

Re: Missing put_cpu() in arch/ia64/sn/kernel/sn2/sn_hwperf.c?

[Jack Steiner]

On Thu, Aug 11, 2011 at 05:58:29AM -0500, Robin Holt wrote:
> On Thu, Aug 11, 2011 at 08:12:45AM +0200, Jes Sorensen wrote:
> > On 08/08/11 23:37, Tony Luck wrote:
> > > On Mon, Aug 8, 2011 at 11:05 AM, Thomas Meyer <thomas@m3y3r.de> wrote:
> > >> The function sn_hwperf_op_cpu() seems to miss a corresponding put_cpu().
> > >>
> > >> Or is this done in another function? I didn't find it.
> > > 
> > > It would be hard to do it elsewhere - this function may not have done
> > > a get_cpu() [in the cpu == SN_HWPERF_ARG_ANY_CPU case].
> > > 
> > > The logic is a bit tortuous here ... perhaps simpler to split the tests
> > > up.  Does the attached patch look right?
> > 
> > Hi Tony,
> > 
> > You probably want someone from SGI to look at it, so I'll forward to
> > Robin Holt.
> > 
> > I haven't been at SGI for about 2 years now :)
> 
> And I will bounce the question on to Jack.  I believe he will probably
> pass it on to somebody more familiar with sn_hwperf.  Tony's patch looks
> much clearer to me.  I would have answered that I thought this was the
> right patch until I saw the '... else { put_cpu(); ...' which made me
> think we need a closer look.

Agree - looks like a missing put_cpu(). Tony's patch looks good to me. Much clearer.

Acked-by: Jack Steiner <steiner@sgi.com>


--- jack

Re: Missing put_cpu() in arch/ia64/sn/kernel/sn2/sn_hwperf.c?

[Thomas Meyer]

Am Donnerstag, den 11.08.2011, 08:12 +0200 schrieb Jes Sorensen:

> I haven't been at SGI for about 2 years now :)
> 

you may want to change this:

$ ./scripts/get_maintainer.pl -f arch/ia64/sn/kernel/sn2/sn_hwperf.c 
Jes Sorensen <jes@sgi.com> (maintainer:SN-IA64 (Itanium)...)
Tony Luck <tony.luck@intel.com> (maintainer:IA64 (Itanium) PL...)
Fenghua Yu <fenghua.yu@intel.com> (maintainer:IA64 (Itanium) PL...)
Arnd Bergmann <arnd@arndb.de> (commit_signer:1/1=100%)
linux-altix@sgi.com (open list:SN-IA64 (Itanium)...)
linux-ia64@vger.kernel.org (open list:SN-IA64 (Itanium)...)
linux-kernel@vger.kernel.org (open list)

with kind regards
thomas

Re: Missing put_cpu() in arch/ia64/sn/kernel/sn2/sn_hwperf.c?

[Mike Frysinger]

On Mon, Aug 8, 2011 at 14:05, Thomas Meyer wrote:
>  Check for get/put_cpu() imbalances
>
>  The simplified semantic patch that makes this report is:
>
> * get_cpu()
> ... when != put_cpu()
> ? get_cpu()
>
> arch/blackfin/kernel/cplbinfo.c:89:9-10: WARNING: Possible missing put_cpu()!

the get_cpu() is in the seq_operations.start while the put_cpu() is in
the seq_operations.stop.  i believe this is OK and so this is a false
positive for blackfin.
-mike