Google Groups no longer supports new Usenet posts or subscriptions. Historical content remains viewable.
Dismiss

[PATCH 5/5] uml: Clean up asm/system.h

3 views
Skip to first unread message

Jan Kiszka

unread,
Apr 19, 2010, 6:00:02 PM4/19/10
to
Remove duplicates and unused prototypes.

Signed-off-by: Jan Kiszka <jan.k...@web.de>
---
arch/um/include/asm/system.h | 3 ---
1 files changed, 0 insertions(+), 3 deletions(-)

diff --git a/arch/um/include/asm/system.h b/arch/um/include/asm/system.h
index 753346e..93af1cf 100644
--- a/arch/um/include/asm/system.h
+++ b/arch/um/include/asm/system.h
@@ -3,11 +3,8 @@

#include "sysdep/system.h"

-extern void *switch_to(void *prev, void *next, void *last);
-
extern int get_signals(void);
extern int set_signals(int enable);
-extern int get_signals(void);
extern void block_signals(void);
extern void unblock_signals(void);

--
1.6.0.2

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majo...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Jan Kiszka

unread,
Apr 19, 2010, 6:00:03 PM4/19/10
to
Signed-off-by: Jan Kiszka <jan.k...@web.de>
---
arch/um/drivers/line.c | 1 -
1 files changed, 0 insertions(+), 1 deletions(-)

diff --git a/arch/um/drivers/line.c b/arch/um/drivers/line.c
index 7a656bd..7f7338c 100644
--- a/arch/um/drivers/line.c
+++ b/arch/um/drivers/line.c
@@ -19,7 +19,6 @@ static irqreturn_t line_interrupt(int irq, void *data)
{
struct chan *chan = data;
struct line *line = chan->line;
- struct tty_struct *tty;

if (line)
chan_interrupt(&line->chan_list, &line->task, line->tty, irq);

Jan Kiszka

unread,
Apr 19, 2010, 6:00:03 PM4/19/10
to
Already defined in kernel.h. The official version assumes that 'n' is
power of two - which it is in our case.

Signed-off-by: Jan Kiszka <jan.k...@web.de>
---

arch/um/sys-x86_64/signal.c | 2 --
1 files changed, 0 insertions(+), 2 deletions(-)

diff --git a/arch/um/sys-x86_64/signal.c b/arch/um/sys-x86_64/signal.c
index 1a899a7..07797d1 100644
--- a/arch/um/sys-x86_64/signal.c
+++ b/arch/um/sys-x86_64/signal.c
@@ -165,8 +165,6 @@ struct rt_sigframe
struct _fpstate fpstate;
};

-#define round_down(m, n) (((m) / (n)) * (n))
-
int setup_signal_stack_si(unsigned long stack_top, int sig,
struct k_sigaction *ka, struct pt_regs * regs,
siginfo_t *info, sigset_t *set)

Jan Kiszka

unread,
Apr 19, 2010, 6:00:02 PM4/19/10
to
We can't pull in linux/sched.h, so just declare the struct.

Signed-off-by: Jan Kiszka <jan.k...@web.de>
---

arch/um/sys-i386/asm/elf.h | 2 ++
arch/um/sys-x86_64/asm/elf.h | 2 ++
2 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/arch/um/sys-i386/asm/elf.h b/arch/um/sys-i386/asm/elf.h
index e64cd41..a979a22 100644
--- a/arch/um/sys-i386/asm/elf.h
+++ b/arch/um/sys-i386/asm/elf.h
@@ -75,6 +75,8 @@ typedef struct user_i387_struct elf_fpregset_t;
pr_reg[16] = PT_REGS_SS(regs); \
} while (0);

+struct task_struct;
+
extern int elf_core_copy_fpregs(struct task_struct *t, elf_fpregset_t *fpu);

#define ELF_CORE_COPY_FPREGS(t, fpu) elf_core_copy_fpregs(t, fpu)
diff --git a/arch/um/sys-x86_64/asm/elf.h b/arch/um/sys-x86_64/asm/elf.h
index 49655c8..d760967 100644
--- a/arch/um/sys-x86_64/asm/elf.h
+++ b/arch/um/sys-x86_64/asm/elf.h
@@ -95,6 +95,8 @@ typedef struct user_i387_struct elf_fpregset_t;
(pr_reg)[25] = 0; \
(pr_reg)[26] = 0;

+struct task_struct;
+
extern int elf_core_copy_fpregs(struct task_struct *t, elf_fpregset_t *fpu);

#define ELF_CORE_COPY_FPREGS(t, fpu) elf_core_copy_fpregs(t, fpu)

Jan Kiszka

unread,
Apr 19, 2010, 6:00:02 PM4/19/10
to
The i386 subarch happens to pull in original NR_syscalls. Maybe we can
make that work for all host arch, but for now just avoid the clash by
using an all-upper-case name.

Signed-off-by: Jan Kiszka <jan.k...@web.de>
---

arch/um/kernel/skas/syscall.c | 4 ++--
1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/um/kernel/skas/syscall.c b/arch/um/kernel/skas/syscall.c
index 4e3b820..f5173e1 100644
--- a/arch/um/kernel/skas/syscall.c
+++ b/arch/um/kernel/skas/syscall.c
@@ -10,7 +10,7 @@
#include "sysdep/syscalls.h"

extern int syscall_table_size;
-#define NR_syscalls (syscall_table_size / sizeof(void *))
+#define NR_SYSCALLS (syscall_table_size / sizeof(void *))

void handle_syscall(struct uml_pt_regs *r)
{
@@ -30,7 +30,7 @@ void handle_syscall(struct uml_pt_regs *r)
* in case it's a compiler bug.
*/
syscall = UPT_SYSCALL_NR(r);
- if ((syscall >= NR_syscalls) || (syscall < 0))
+ if ((syscall >= NR_SYSCALLS) || (syscall < 0))
result = -ENOSYS;
else result = EXECUTE_SYSCALL(syscall, regs);

Amerigo Wang

unread,
Apr 20, 2010, 4:30:02 AM4/20/10
to
On Mon, Apr 19, 2010 at 11:53:04PM +0200, Jan Kiszka wrote:
>Signed-off-by: Jan Kiszka <jan.k...@web.de>


Acked-by: WANG Cong <xiyou.w...@gmail.com>

Amerigo Wang

unread,
Apr 20, 2010, 4:40:01 AM4/20/10
to
On Mon, Apr 19, 2010 at 11:53:05PM +0200, Jan Kiszka wrote:
>Already defined in kernel.h. The official version assumes that 'n' is
>power of two - which it is in our case.
>
>Signed-off-by: Jan Kiszka <jan.k...@web.de>
>---
> arch/um/sys-x86_64/signal.c | 2 --
> 1 files changed, 0 insertions(+), 2 deletions(-)
>
>diff --git a/arch/um/sys-x86_64/signal.c b/arch/um/sys-x86_64/signal.c
>index 1a899a7..07797d1 100644
>--- a/arch/um/sys-x86_64/signal.c
>+++ b/arch/um/sys-x86_64/signal.c
>@@ -165,8 +165,6 @@ struct rt_sigframe
> struct _fpstate fpstate;
> };
>
>-#define round_down(m, n) (((m) / (n)) * (n))
>-
> int setup_signal_stack_si(unsigned long stack_top, int sig,
> struct k_sigaction *ka, struct pt_regs * regs,
> siginfo_t *info, sigset_t *set)

Shouldn't this signal.c #include <linux/kernel.h>?

Thanks.

Amerigo Wang

unread,
Apr 20, 2010, 6:10:02 AM4/20/10
to
On Mon, Apr 19, 2010 at 11:53:06PM +0200, Jan Kiszka wrote:
>We can't pull in linux/sched.h, so just declare the struct.
>

Did you meet any build error? If yes, please include it.

Thanks.

Amerigo Wang

unread,
Apr 20, 2010, 6:10:02 AM4/20/10
to
On Mon, Apr 19, 2010 at 11:53:07PM +0200, Jan Kiszka wrote:
>The i386 subarch happens to pull in original NR_syscalls. Maybe we can
>make that work for all host arch, but for now just avoid the clash by
>using an all-upper-case name.
>

Where?

Amerigo Wang

unread,
Apr 20, 2010, 6:20:02 AM4/20/10
to
On Mon, Apr 19, 2010 at 11:53:08PM +0200, Jan Kiszka wrote:
>Remove duplicates and unused prototypes.
>
>Signed-off-by: Jan Kiszka <jan.k...@web.de>

Acked-by: WANG Cong <xiyou.w...@gmail.com>

Jeff Dike

unread,
Apr 20, 2010, 10:30:02 AM4/20/10
to
On Tue, Apr 20, 2010 at 04:33:58PM +0800, Amerigo Wang wrote:
> On Mon, Apr 19, 2010 at 11:53:05PM +0200, Jan Kiszka wrote:
> >Already defined in kernel.h. The official version assumes that 'n' is
> >power of two - which it is in our case.
> >
> >Signed-off-by: Jan Kiszka <jan.k...@web.de>
> >---
> > arch/um/sys-x86_64/signal.c | 2 --
> > 1 files changed, 0 insertions(+), 2 deletions(-)
> >
> >diff --git a/arch/um/sys-x86_64/signal.c b/arch/um/sys-x86_64/signal.c
> >index 1a899a7..07797d1 100644
> >--- a/arch/um/sys-x86_64/signal.c
> >+++ b/arch/um/sys-x86_64/signal.c
> >@@ -165,8 +165,6 @@ struct rt_sigframe
> > struct _fpstate fpstate;
> > };
> >
> >-#define round_down(m, n) (((m) / (n)) * (n))
> >-
> > int setup_signal_stack_si(unsigned long stack_top, int sig,
> > struct k_sigaction *ka, struct pt_regs * regs,
> > siginfo_t *info, sigset_t *set)
>
> Shouldn't this signal.c #include <linux/kernel.h>?

I agree - if this is going to depend on kernel.h, it should be
explicitly included.

Jeff

Jiri Kosina

unread,
Apr 20, 2010, 10:40:02 AM4/20/10
to
On Mon, 19 Apr 2010, Jan Kiszka wrote:

> Signed-off-by: Jan Kiszka <jan.k...@web.de>
> ---
> arch/um/drivers/line.c | 1 -
> 1 files changed, 0 insertions(+), 1 deletions(-)
>
> diff --git a/arch/um/drivers/line.c b/arch/um/drivers/line.c
> index 7a656bd..7f7338c 100644
> --- a/arch/um/drivers/line.c
> +++ b/arch/um/drivers/line.c
> @@ -19,7 +19,6 @@ static irqreturn_t line_interrupt(int irq, void *data)
> {
> struct chan *chan = data;
> struct line *line = chan->line;
> - struct tty_struct *tty;
>
> if (line)
> chan_interrupt(&line->chan_list, &line->task, line->tty, irq);

Applied, thanks.

--
Jiri Kosina
SUSE Labs, Novell Inc.

Jiri Kosina

unread,
Apr 20, 2010, 10:40:02 AM4/20/10
to
On Mon, 19 Apr 2010, Jan Kiszka wrote:

> Remove duplicates and unused prototypes.
>
> Signed-off-by: Jan Kiszka <jan.k...@web.de>

Applied, thanks.

--
Jiri Kosina
SUSE Labs, Novell Inc.

Jeff Dike

unread,
Apr 20, 2010, 10:40:02 AM4/20/10
to
On Tue, Apr 20, 2010 at 06:09:49PM +0800, Amerigo Wang wrote:
> On Mon, Apr 19, 2010 at 11:53:06PM +0200, Jan Kiszka wrote:
> >We can't pull in linux/sched.h, so just declare the struct.
> >
>
> Did you meet any build error? If yes, please include it.

What does this patch fix, aside from being a bit cleaner?

If it built before, without having a task_struct declaration, I think
that means that the elf_core_copy_fpregs was never used. The
task_struct * in the declaration would become a private task_struct,
known only to the declaration. If the implementation or callers have
the regular task_struct, it will be a different one, and the
prototypes will conflict due to the different types of the first
parameter.

Jeff

--
Work email - jdike at linux dot intel dot com

Jiri Kosina

unread,
Apr 20, 2010, 10:40:03 AM4/20/10
to
On Tue, 20 Apr 2010, Amerigo Wang wrote:

> On Mon, Apr 19, 2010 at 11:53:05PM +0200, Jan Kiszka wrote:
> >Already defined in kernel.h. The official version assumes that 'n' is
> >power of two - which it is in our case.
> >
> >Signed-off-by: Jan Kiszka <jan.k...@web.de>
> >---
> > arch/um/sys-x86_64/signal.c | 2 --
> > 1 files changed, 0 insertions(+), 2 deletions(-)
> >
> >diff --git a/arch/um/sys-x86_64/signal.c b/arch/um/sys-x86_64/signal.c
> >index 1a899a7..07797d1 100644
> >--- a/arch/um/sys-x86_64/signal.c
> >+++ b/arch/um/sys-x86_64/signal.c
> >@@ -165,8 +165,6 @@ struct rt_sigframe
> > struct _fpstate fpstate;
> > };
> >
> >-#define round_down(m, n) (((m) / (n)) * (n))
> >-
> > int setup_signal_stack_si(unsigned long stack_top, int sig,
> > struct k_sigaction *ka, struct pt_regs * regs,
> > siginfo_t *info, sigset_t *set)
>
> Shouldn't this signal.c #include <linux/kernel.h>?

Well, it gets included implicitly through uaccess.h -> sched.h ->
kernel.h.

Applied, thanks.

--
Jiri Kosina
SUSE Labs, Novell Inc.

Jiri Kosina

unread,
Apr 20, 2010, 11:50:02 AM4/20/10
to
On Tue, 20 Apr 2010, Jeff Dike wrote:

> > >Already defined in kernel.h. The official version assumes that 'n' is
> > >power of two - which it is in our case.
> > >
> > >Signed-off-by: Jan Kiszka <jan.k...@web.de>
> > >---
> > > arch/um/sys-x86_64/signal.c | 2 --
> > > 1 files changed, 0 insertions(+), 2 deletions(-)
> > >
> > >diff --git a/arch/um/sys-x86_64/signal.c b/arch/um/sys-x86_64/signal.c
> > >index 1a899a7..07797d1 100644
> > >--- a/arch/um/sys-x86_64/signal.c
> > >+++ b/arch/um/sys-x86_64/signal.c
> > >@@ -165,8 +165,6 @@ struct rt_sigframe
> > > struct _fpstate fpstate;
> > > };
> > >
> > >-#define round_down(m, n) (((m) / (n)) * (n))
> > >-
> > > int setup_signal_stack_si(unsigned long stack_top, int sig,
> > > struct k_sigaction *ka, struct pt_regs * regs,
> > > siginfo_t *info, sigset_t *set)
> >
> > Shouldn't this signal.c #include <linux/kernel.h>?
>
> I agree - if this is going to depend on kernel.h, it should be
> explicitly included.

Yup, I have already added that.

Thanks,

--
Jiri Kosina
SUSE Labs, Novell Inc.

Jeff Dike

unread,
Apr 20, 2010, 1:00:02 PM4/20/10
to
On Tue, Apr 20, 2010 at 04:35:52PM +0200, Jiri Kosina wrote:
> On Tue, 20 Apr 2010, Amerigo Wang wrote:
> > Shouldn't this signal.c #include <linux/kernel.h>?
>
> Well, it gets included implicitly through uaccess.h -> sched.h ->
> kernel.h.

You're depending on the internal details of uaccess.h and sched.h. If
either of them changed, then this would cause unexpected compile
failures here.

Better to explicitly include kernel.h.

Jeff

--
Work email - jdike at linux dot intel dot com

Jan Kiszka

unread,
Apr 20, 2010, 1:10:02 PM4/20/10
to
Jeff Dike wrote:
> On Tue, Apr 20, 2010 at 06:09:49PM +0800, Amerigo Wang wrote:
>> On Mon, Apr 19, 2010 at 11:53:06PM +0200, Jan Kiszka wrote:
>>> We can't pull in linux/sched.h, so just declare the struct.
>>>
>> Did you meet any build error? If yes, please include it.
>
> What does this patch fix, aside from being a bit cleaner?

CC arch/um/sys-i386/elfcore.o
In file included from /data/linux-2.6/include/linux/elf.h:8,
from /data/linux-2.6/arch/um/sys-i386/elfcore.c:2:
/data/linux-2.6/arch/um/sys-i386/asm/elf.h:78: warning: ‘struct task_struct’ declared inside parameter list
/data/linux-2.6/arch/um/sys-i386/asm/elf.h:78: warning: its scope is only this definition or declaration, which is probably not what you want

I guess not many people build against i386 hosts anymore, so this
remained widely unnoticed.

>
> If it built before, without having a task_struct declaration, I think
> that means that the elf_core_copy_fpregs was never used. The
> task_struct * in the declaration would become a private task_struct,
> known only to the declaration. If the implementation or callers have
> the regular task_struct, it will be a different one, and the
> prototypes will conflict due to the different types of the first
> parameter.

This is just a forward declaration (that many arch elf header include),
so no such problem exists.

BTW, to answer the other question in this thread: We have a circular
dependency that prevents including sched.h.

I can add all these information to some v2 of this patch if it is
required to get this merged. Please let me know.

Jan


signature.asc

Jiri Kosina

unread,
Apr 20, 2010, 1:20:01 PM4/20/10
to
On Tue, 20 Apr 2010, Jeff Dike wrote:

> > > Shouldn't this signal.c #include <linux/kernel.h>?
> >
> > Well, it gets included implicitly through uaccess.h -> sched.h ->
> > kernel.h.
>
> You're depending on the internal details of uaccess.h and sched.h. If
> either of them changed, then this would cause unexpected compile
> failures here.
>
> Better to explicitly include kernel.h.

Yeah, I have that already in my tree.

Thanks,

--
Jiri Kosina
SUSE Labs, Novell Inc.

Jan Kiszka

unread,
Apr 20, 2010, 1:20:01 PM4/20/10
to
Amerigo Wang wrote:
> On Mon, Apr 19, 2010 at 11:53:07PM +0200, Jan Kiszka wrote:
>> The i386 subarch happens to pull in original NR_syscalls. Maybe we can
>> make that work for all host arch, but for now just avoid the clash by
>> using an all-upper-case name.
>>
>
> Where?

Not sure if this answers your question:

CC arch/um/kernel/skas/syscall.o
/data/linux-2.6/arch/um/kernel/skas/syscall.c:13:1: warning:
"NR_syscalls" redefined
In file included from /data/linux-2.6/arch/x86/include/asm/unistd.h:3,
from
/data/linux-2.6/arch/um/sys-i386/shared/sysdep/syscalls.h:6,
from /data/linux-2.6/arch/um/kernel/skas/syscall.c:10:
/data/linux-2.6/arch/x86/include/asm/unistd_32.h:349:1: warning: this is
the location of the previous definition

Jan

signature.asc

Jiri Kosina

unread,
Apr 20, 2010, 7:50:01 PM4/20/10
to

I have updated the explanation in the changelog and applied the patch. If
anyone has any objections still, please let me know.

Thanks,

--
Jiri Kosina
SUSE Labs, Novell Inc.

Amerigo Wang

unread,
Apr 21, 2010, 2:10:02 AM4/21/10
to

Ah, sure. I misunderstood your purpose, please do include the warning
messages you are trying to fix in your patch description.


Thanks!

Amerigo Wang

unread,
Apr 21, 2010, 2:10:02 AM4/21/10
to
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
This is the right reason to do this. Ok then, thanks.

But it looks like x86_64 needs this too.


BTW, I don't think compile warning fixes are trivial enough to go
to tri...@kernel.org.

Jiri Kosina

unread,
Apr 21, 2010, 5:40:02 AM4/21/10
to
On Wed, 21 Apr 2010, Amerigo Wang wrote:

> > CC arch/um/sys-i386/elfcore.o
> >In file included from /data/linux-2.6/include/linux/elf.h:8,
> > from /data/linux-2.6/arch/um/sys-i386/elfcore.c:2:
> >/data/linux-2.6/arch/um/sys-i386/asm/elf.h:78: warning: ‘struct task_struct’ declared inside parameter list
> >/data/linux-2.6/arch/um/sys-i386/asm/elf.h:78: warning: its scope is only this definition or declaration, which is probably not what you want
> >
> >I guess not many people build against i386 hosts anymore, so this
> >remained widely unnoticed.
> >
> >>
> >> If it built before, without having a task_struct declaration, I think
> >> that means that the elf_core_copy_fpregs was never used. The
> >> task_struct * in the declaration would become a private task_struct,
> >> known only to the declaration. If the implementation or callers have
> >> the regular task_struct, it will be a different one, and the
> >> prototypes will conflict due to the different types of the first
> >> parameter.
> >
> >This is just a forward declaration (that many arch elf header include),
> >so no such problem exists.
> >
> >BTW, to answer the other question in this thread: We have a circular
> >dependency that prevents including sched.h.
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> This is the right reason to do this. Ok then, thanks.
>
> But it looks like x86_64 needs this too.
>
>
> BTW, I don't think compile warning fixes are trivial enough to go
> to tri...@kernel.org.

Why?

--
Jiri Kosina
SUSE Labs, Novell Inc.

Amerigo Wang

unread,
Apr 21, 2010, 6:50:02 AM4/21/10
to
On Wed, Apr 21, 2010 at 11:38:53AM +0200, Jiri Kosina wrote:
>On Wed, 21 Apr 2010, Amerigo Wang wrote:
>>
>> BTW, I don't think compile warning fixes are trivial enough to go
>> to tri...@kernel.org.
>
>Why?
>

Usually compile warnings fixes go into -mm, some coding style fixes
too.

Jiri Kosina

unread,
Apr 21, 2010, 6:50:02 AM4/21/10
to
On Wed, 21 Apr 2010, Amerigo Wang wrote:

> >> BTW, I don't think compile warning fixes are trivial enough to go
> >> to tri...@kernel.org.
> >
> >Why?
> >
> Usually compile warnings fixes go into -mm, some coding style fixes
> too.

Well, I personally don't care that much, submit your patches whereever you
wish.

The point of trivial tree is to take off load from other maintainers
(including Andrew) so that focusing on The Real Things is possible.

See relevant section of Documentation/SubmittingPatches.

But, as I said, I don't care that much. If you want to avoid trivial tree
for some reason, feel free to do so.

--
Jiri Kosina
SUSE Labs, Novell Inc.

Jiri Kosina

unread,
May 10, 2010, 5:40:03 PM5/10/10
to
On Wed, 21 Apr 2010, Amerigo Wang wrote:

> >> Where?
> >
> >Not sure if this answers your question:
> >
> > CC arch/um/kernel/skas/syscall.o
> >/data/linux-2.6/arch/um/kernel/skas/syscall.c:13:1: warning:
> >"NR_syscalls" redefined
> >In file included from /data/linux-2.6/arch/x86/include/asm/unistd.h:3,
> > from
> >/data/linux-2.6/arch/um/sys-i386/shared/sysdep/syscalls.h:6,
> > from /data/linux-2.6/arch/um/kernel/skas/syscall.c:10:
> >/data/linux-2.6/arch/x86/include/asm/unistd_32.h:349:1: warning: this is
> >the location of the previous definition
> >
>
> Ah, sure. I misunderstood your purpose, please do include the warning
> messages you are trying to fix in your patch description.

The patch doesn't seem to be present in linux-next as of today. I have
applied it to my queue.

Thanks,

--
Jiri Kosina
SUSE Labs, Novell Inc.

0 new messages