2005-01-05 02:23:01

by James Nelson

[permalink] [raw]
Subject: [PATCH /3] sh64: remove cli()/sti() from arch/sh64/*

This series of patches is to remove the last cli()/sti() function calls in arch/sh64.

These are the only instances in active code that grep could find.

kernel/time.c | 4 ++--
mach-cayman/irq.c | 8 ++++----
mm/fault.c | 2 +-
3 files changed, 7 insertions(+), 7 deletions(-)


2005-01-05 02:23:15

by James Nelson

[permalink] [raw]
Subject: [PATCH /3] sh64: remove cli()/sti() in arch/sh64/kernel/time.c

Signed-off-by: James Nelson <[email protected]>

diff -urN --exclude='*~' linux-2.6.10-mm1-original/arch/sh64/kernel/time.c linux-2.6.10-mm1/arch/sh64/kernel/time.c
--- linux-2.6.10-mm1-original/arch/sh64/kernel/time.c 2004-12-24 16:34:58.000000000 -0500
+++ linux-2.6.10-mm1/arch/sh64/kernel/time.c 2005-01-04 21:06:31.232945929 -0500
@@ -424,7 +424,7 @@
*/
register unsigned long long __rtc_irq_flag __asm__ ("r3");

- sti();
+ local_irq_enable();
do {} while (ctrl_inb(R64CNT) != 0);
ctrl_outb(RCR1_CIE, RCR1); /* Enable carry interrupt */

@@ -443,7 +443,7 @@
"getcon " __CTC ", %0\n\t"
: "=r"(ctc_val), "=r" (__dummy), "=r" (__rtc_irq_flag)
: "0" (0));
- cli();
+ local_irq_disable();
/*
* SH-3:
* CPU clock = 4 stages * loop

2005-01-05 02:34:28

by Al Viro

[permalink] [raw]
Subject: Re: [PATCH /3] sh64: remove cli()/sti() from arch/sh64/*

On Tue, Jan 04, 2005 at 08:22:47PM -0600, James Nelson wrote:
> This series of patches is to remove the last cli()/sti() function calls in arch/sh64.

Wait a minute. Is that just a blanket search-and-replace job? There is
a reason why cli/sti is marked obsolete instead of being silently #define'd
that way. Namely, in a lot of cases users of cli/sti are actually racy.

For such instances replacing these with local_... would not improve anything
(obviously) *and* would hide a trouble spot by silencing a warning.

I'm not familiar with the architectures in question, so it might very well
be that all replacements so far had been correct. However, I would really
like to see rationale for each of those warning removals to go along with
the patches.

Note that basically you are doing "remove the warning in foo.c:42 and
keep the current behaviour". The missing part is "current behaviour is,
in fact, correct in that place and does not deserve a warning because
<list of reasons>".

2005-01-05 03:11:09

by James Nelson

[permalink] [raw]
Subject: Re: [PATCH /3] sh64: remove cli()/sti() from arch/sh64/*

Al Viro wrote:

>On Tue, Jan 04, 2005 at 08:22:47PM -0600, James Nelson wrote:
>
>
>>This series of patches is to remove the last cli()/sti() function calls in arch/sh64.
>>
>>
>
>Wait a minute. Is that just a blanket search-and-replace job? There is
>a reason why cli/sti is marked obsolete instead of being silently #define'd
>that way. Namely, in a lot of cases users of cli/sti are actually racy.
>
>For such instances replacing these with local_... would not improve anything
>(obviously) *and* would hide a trouble spot by silencing a warning.
>
>I'm not familiar with the architectures in question, so it might very well
>be that all replacements so far had been correct. However, I would really
>like to see rationale for each of those warning removals to go along with
>the patches.
>
>Note that basically you are doing "remove the warning in foo.c:42 and
>keep the current behaviour". The missing part is "current behaviour is,
>in fact, correct in that place and does not deserve a warning because
><list of reasons>".
>
>
>
Everything I've looked at so far has been for single-processor systems
AFAICT - embedded processors, evaluation boards, etc. I do not pretend
to have intimate familiarity with the hardware in question, and I will
be much more careful when I reach anything that can be plugged into an
SMP box, but I was grabbing the low-hanging fruit first. The nasty
stuff (drivers/char, for example) will come later.

That's why I cc'd the arch maintainers - figured they'd whack me with a
cluebat if I'd overlooked something.

2005-01-05 17:32:10

by Paul Mundt

[permalink] [raw]
Subject: Re: [PATCH /3] sh64: remove cli()/sti() from arch/sh64/*

On Tue, Jan 04, 2005 at 10:11:23PM -0500, Jim Nelson wrote:
> Everything I've looked at so far has been for single-processor systems
> AFAICT - embedded processors, evaluation boards, etc. I do not pretend
> to have intimate familiarity with the hardware in question, and I will
> be much more careful when I reach anything that can be plugged into an
> SMP box, but I was grabbing the low-hanging fruit first. The nasty
> stuff (drivers/char, for example) will come later.
>
> That's why I cc'd the arch maintainers - figured they'd whack me with a
> cluebat if I'd overlooked something.

Yes, these are all UP cases, so at least for the sh and sh64 cases these
specific changes are fine.

sh ripped out the save_and_cli() mess a long time ago (during early 2.5
before Linus added the UP compat stuff), so these at least shouldn't
have been there in the first place (mpc1211 wasn't added until after the
fact however and seems to have gotten overlooked).

Anyways, I'll apply these patches to their respective trees, thanks.


Attachments:
(No filename) (1.01 kB)
(No filename) (189.00 B)
Download all attachments