2013-10-23 11:10:56

by Chen Gang

[permalink] [raw]
Subject: [PATCH] arc: kernel: add default extern variable 'screen_info' in "setup.c"

Add default 'screen_info' just like some of other architectures (e.g.
cris, score, sh, tile), or can not pass compiling.

The related error (with allmodconfig):

drivers/built-in.o: In function `vgacon_save_screen':
drivers/video/console/vgacon.c:1347: undefined reference to `screen_info'
drivers/video/console/vgacon.c:1348: undefined reference to `screen_info'
drivers/built-in.o: In function `vgacon_resize':
drivers/video/console/vgacon.c:1314: undefined reference to `screen_info'
drivers/video/console/vgacon.c:1315: undefined reference to `screen_info'
drivers/built-in.o: In function `vgacon_switch':
drivers/video/console/vgacon.c:820: undefined reference to `screen_info'
drivers/built-in.o:drivers/video/console/vgacon.c:840: more undefined references to `screen_info' follow


Signed-off-by: Chen Gang <[email protected]>
---
arch/arc/kernel/setup.c | 3 +++
1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/arch/arc/kernel/setup.c b/arch/arc/kernel/setup.c
index 2c68bc7e..07130f3 100644
--- a/arch/arc/kernel/setup.c
+++ b/arch/arc/kernel/setup.c
@@ -15,6 +15,7 @@
#include <linux/cpu.h>
#include <linux/of_fdt.h>
#include <linux/cache.h>
+#include <linux/screen_info.h>
#include <asm/sections.h>
#include <asm/arcregs.h>
#include <asm/tlb.h>
@@ -37,6 +38,8 @@ struct task_struct *_current_task[NR_CPUS]; /* For stack switching */

struct cpuinfo_arc cpuinfo_arc700[NR_CPUS];

+struct screen_info screen_info;
+

void read_arc_build_cfg_regs(void)
{
--
1.7.7.6


2013-10-23 13:48:10

by Vineet Gupta

[permalink] [raw]
Subject: RE: [PATCH] arc: kernel: add default extern variable 'screen_info' in "setup.c"

Apologies for top posting !

NAK.

ARC will never have VGA console. You need to add !ARC to relevant Kconfig. However that approach is frowned upon in general. The current way to doing such things is to define a new Kconfig item which relevant arches can select.

-Vineet
________________________________________
From: Chen Gang [[email protected]]
Sent: Wednesday, October 23, 2013 4:39 PM
To: [email protected]; Arnd Bergmann; [email protected]; Paul Gortmaker; James Hogan
Cc: [email protected]
Subject: [PATCH] arc: kernel: add default extern variable 'screen_info' in "setup.c"

Add default 'screen_info' just like some of other architectures (e.g.
cris, score, sh, tile), or can not pass compiling.

The related error (with allmodconfig):

drivers/built-in.o: In function `vgacon_save_screen':
drivers/video/console/vgacon.c:1347: undefined reference to `screen_info'
drivers/video/console/vgacon.c:1348: undefined reference to `screen_info'
drivers/built-in.o: In function `vgacon_resize':
drivers/video/console/vgacon.c:1314: undefined reference to `screen_info'
drivers/video/console/vgacon.c:1315: undefined reference to `screen_info'
drivers/built-in.o: In function `vgacon_switch':
drivers/video/console/vgacon.c:820: undefined reference to `screen_info'
drivers/built-in.o:drivers/video/console/vgacon.c:840: more undefined references to `screen_info' follow


Signed-off-by: Chen Gang <[email protected]>
---
arch/arc/kernel/setup.c | 3 +++
1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/arch/arc/kernel/setup.c b/arch/arc/kernel/setup.c
index 2c68bc7e..07130f3 100644
--- a/arch/arc/kernel/setup.c
+++ b/arch/arc/kernel/setup.c
@@ -15,6 +15,7 @@
#include <linux/cpu.h>
#include <linux/of_fdt.h>
#include <linux/cache.h>
+#include <linux/screen_info.h>
#include <asm/sections.h>
#include <asm/arcregs.h>
#include <asm/tlb.h>
@@ -37,6 +38,8 @@ struct task_struct *_current_task[NR_CPUS]; /* For stack switching */

struct cpuinfo_arc cpuinfo_arc700[NR_CPUS];

+struct screen_info screen_info;
+

void read_arc_build_cfg_regs(void)
{
--
1.7.7.6

2013-10-24 02:57:09

by Chen Gang

[permalink] [raw]
Subject: Re: [PATCH] arc: kernel: add default extern variable 'screen_info' in "setup.c"

On 10/23/2013 09:47 PM, Vineet Gupta wrote:
> Apologies for top posting !
>

Not at all.

> NAK.
>

OK, thanks. At least this patch is incorrect.


> ARC will never have VGA console. You need to add !ARC to relevant Kconfig. However that approach is frowned upon in general. The current way to doing such things is to define a new Kconfig item which relevant arches can select.

Hmm... it seems necessary to discuss about the 2 fixing ways (which
already had a long discussion in ARM64):

- add !ARC in related place, just like another (almost 30-40%) architectures done.

- add a new Kconfig item (e.g. HAVE_VGA_CONSOLE), and let the left (almost 50%) architectures which use VGA_CONSOLE to set it.

Catalin, Will, Geert (it seems also include you) prefer 2nd way, but I
prefer 1st, my reasons are below, please help check:

- 1st way need add some (10-20%) of architectures in one place, which belongs to local wide.
2nd way will let the left (almost 50%) architectures need add HAVE_VGA_CONSOLE in various place, which belongs to arch global wide.

- 1st way will let most (80-90%) of architectures don't care about VGA_CONSOLE (50% need it, 30-40% already mark it in the same place).
2nd way will let 50% architectures care about VGA_CONSOLE (although can let the left 10-20% architectures do nothing -- not care about it).

- 1st way is still easy, sustainable, and clear enough in local wide fixing (although maybe it is not the best, or clearest way).
2nd way is also easy, sustainable and clear enough (maybe the best, or clearest for 10-20% architectures), but it will let the fix in global wide

All together, if we can bear and sustainable, I always prefer to fix it
in local wide, not lead to arch global (especially 80-90% architectures
already followed 1st way).


BTW: for me, if less than 20% architectures need XXX, we can trigger
defining a new Kconfig item (e.g. HAVE_XXX), or just follow original
implementation.


Thanks.

>
> -Vineet
> ________________________________________
> From: Chen Gang [[email protected]]
> Sent: Wednesday, October 23, 2013 4:39 PM
> To: [email protected]; Arnd Bergmann; [email protected]; Paul Gortmaker; James Hogan
> Cc: [email protected]
> Subject: [PATCH] arc: kernel: add default extern variable 'screen_info' in "setup.c"
>
> Add default 'screen_info' just like some of other architectures (e.g.
> cris, score, sh, tile), or can not pass compiling.
>
> The related error (with allmodconfig):
>
> drivers/built-in.o: In function `vgacon_save_screen':
> drivers/video/console/vgacon.c:1347: undefined reference to `screen_info'
> drivers/video/console/vgacon.c:1348: undefined reference to `screen_info'
> drivers/built-in.o: In function `vgacon_resize':
> drivers/video/console/vgacon.c:1314: undefined reference to `screen_info'
> drivers/video/console/vgacon.c:1315: undefined reference to `screen_info'
> drivers/built-in.o: In function `vgacon_switch':
> drivers/video/console/vgacon.c:820: undefined reference to `screen_info'
> drivers/built-in.o:drivers/video/console/vgacon.c:840: more undefined references to `screen_info' follow
>
>
> Signed-off-by: Chen Gang <[email protected]>
> ---
> arch/arc/kernel/setup.c | 3 +++
> 1 files changed, 3 insertions(+), 0 deletions(-)
>
> diff --git a/arch/arc/kernel/setup.c b/arch/arc/kernel/setup.c
> index 2c68bc7e..07130f3 100644
> --- a/arch/arc/kernel/setup.c
> +++ b/arch/arc/kernel/setup.c
> @@ -15,6 +15,7 @@
> #include <linux/cpu.h>
> #include <linux/of_fdt.h>
> #include <linux/cache.h>
> +#include <linux/screen_info.h>
> #include <asm/sections.h>
> #include <asm/arcregs.h>
> #include <asm/tlb.h>
> @@ -37,6 +38,8 @@ struct task_struct *_current_task[NR_CPUS]; /* For stack switching */
>
> struct cpuinfo_arc cpuinfo_arc700[NR_CPUS];
>
> +struct screen_info screen_info;
> +
>
> void read_arc_build_cfg_regs(void)
> {
> --
> 1.7.7.6
>
>


--
Chen Gang

2013-10-24 08:30:42

by Geert Uytterhoeven

[permalink] [raw]
Subject: Re: [PATCH] arc: kernel: add default extern variable 'screen_info' in "setup.c"

On Thu, Oct 24, 2013 at 4:56 AM, Chen Gang <[email protected]> wrote:
>> NAK.
>
> OK, thanks. At least this patch is incorrect.
>
>
>> ARC will never have VGA console. You need to add !ARC to relevant Kconfig. However that approach is frowned upon in general. The current way to doing such things is to define a new Kconfig item which relevant arches can select.
>
> Hmm... it seems necessary to discuss about the 2 fixing ways (which
> already had a long discussion in ARM64):
>
> - add !ARC in related place, just like another (almost 30-40%) architectures done.
>
> - add a new Kconfig item (e.g. HAVE_VGA_CONSOLE), and let the left (almost 50%) architectures which use VGA_CONSOLE to set it.
>
> Catalin, Will, Geert (it seems also include you) prefer 2nd way, but I
> prefer 1st, my reasons are below, please help check:
>
> - 1st way need add some (10-20%) of architectures in one place, which belongs to local wide.
> 2nd way will let the left (almost 50%) architectures need add HAVE_VGA_CONSOLE in various place, which belongs to arch global wide.
>
> - 1st way will let most (80-90%) of architectures don't care about VGA_CONSOLE (50% need it, 30-40% already mark it in the same place).
> 2nd way will let 50% architectures care about VGA_CONSOLE (although can let the left 10-20% architectures do nothing -- not care about it).
>
> - 1st way is still easy, sustainable, and clear enough in local wide fixing (although maybe it is not the best, or clearest way).
> 2nd way is also easy, sustainable and clear enough (maybe the best, or clearest for 10-20% architectures), but it will let the fix in global wide
>
> All together, if we can bear and sustainable, I always prefer to fix it
> in local wide, not lead to arch global (especially 80-90% architectures
> already followed 1st way).
>
>
> BTW: for me, if less than 20% architectures need XXX, we can trigger
> defining a new Kconfig item (e.g. HAVE_XXX), or just follow original
> implementation.

Lot's of negative dependencies are missing for VGA_CONSOLE.
I think only alpha, some arm, ia64, some mips, some powerpc, and x86
may use it.

The main reason for introducing HAVE_VGA_CONSOLE is maintainability.
The question to ask is: does the logic have to change when adding a new
architecture?
- If yes, you're better off with the HAVE_xxx scheme.
- If no, use the negative dependencies in the common place.

VGA consoles are legacy, and not used on new systems. Witness the
list of architectures I gave above. All of them are (at least) 20 years old
(except for ia64, but that borrows from PC legacy).
So every time when adding a new architecture, you have to add
'&& !NEW_ARCH' to the VGA_CONSOLE dependency, which is in a
common place (drivers/.../Kconfig).
While with the HAVE_xxx scheme, you don't have to change anything.

Compare this to a shiny new feature that available on all new architectures,
but not on a few old one. There it makes sense to have the negative
dependencies on the old architectures, that will never have the new feature.
While new architectures have enabled it by default.

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds

2013-10-24 11:24:36

by Chen Gang

[permalink] [raw]
Subject: Re: [PATCH] arc: kernel: add default extern variable 'screen_info' in "setup.c"

On 10/24/2013 04:30 PM, Geert Uytterhoeven wrote:
> On Thu, Oct 24, 2013 at 4:56 AM, Chen Gang <[email protected]> wrote:
>>> NAK.
>>
>> OK, thanks. At least this patch is incorrect.
>>
>>
>>> ARC will never have VGA console. You need to add !ARC to relevant Kconfig. However that approach is frowned upon in general. The current way to doing such things is to define a new Kconfig item which relevant arches can select.
>>
>> Hmm... it seems necessary to discuss about the 2 fixing ways (which
>> already had a long discussion in ARM64):
>>
>> - add !ARC in related place, just like another (almost 30-40%) architectures done.
>>
>> - add a new Kconfig item (e.g. HAVE_VGA_CONSOLE), and let the left (almost 50%) architectures which use VGA_CONSOLE to set it.
>>
>> Catalin, Will, Geert (it seems also include you) prefer 2nd way, but I
>> prefer 1st, my reasons are below, please help check:
>>
>> - 1st way need add some (10-20%) of architectures in one place, which belongs to local wide.
>> 2nd way will let the left (almost 50%) architectures need add HAVE_VGA_CONSOLE in various place, which belongs to arch global wide.
>>
>> - 1st way will let most (80-90%) of architectures don't care about VGA_CONSOLE (50% need it, 30-40% already mark it in the same place).
>> 2nd way will let 50% architectures care about VGA_CONSOLE (although can let the left 10-20% architectures do nothing -- not care about it).
>>
>> - 1st way is still easy, sustainable, and clear enough in local wide fixing (although maybe it is not the best, or clearest way).
>> 2nd way is also easy, sustainable and clear enough (maybe the best, or clearest for 10-20% architectures), but it will let the fix in global wide
>>
>> All together, if we can bear and sustainable, I always prefer to fix it
>> in local wide, not lead to arch global (especially 80-90% architectures
>> already followed 1st way).
>>
>>
>> BTW: for me, if less than 20% architectures need XXX, we can trigger
>> defining a new Kconfig item (e.g. HAVE_XXX), or just follow original
>> implementation.
>
> Lot's of negative dependencies are missing for VGA_CONSOLE.
> I think only alpha, some arm, ia64, some mips, some powerpc, and x86
> may use it.
>

in "arch" sub-directory, contents 13/30 (43.3%) architectures contents
VGA_CONSOLE.

1 alpha/kernel/setup.c:137:struct screen_info screen_info = {
2 arm/kernel/setup.c:743:struct screen_info screen_info = {
3 cris/kernel/setup.c:28:struct screen_info screen_info;
4 ia64/kernel/setup.c:79:struct screen_info screen_info;
5 m32r/kernel/setup.c:55:struct screen_info screen_info = {
6 mips/kernel/setup.c:43:struct screen_info screen_info;
7 powerpc/kernel/setup-common.c:88:struct screen_info screen_info = {
8 score/kernel/setup.c:37:struct screen_info screen_info;
9 sh/kernel/setup.c:68:struct screen_info screen_info;
10 sparc/kernel/setup_64.c:66:struct screen_info screen_info = {
11 sparc/kernel/setup_32.c:53:struct screen_info screen_info = {
12 tile/kernel/setup.c:54:struct screen_info screen_info;
13 x86/kernel/setup.c:220:struct screen_info screen_info;
14 xtensa/kernel/setup.c:55:struct screen_info screen_info = { 0, 24, 0, 0, 0, 80, 0, 0, 0, 24, 1, 16};

It seems most of pc and servers support VGA_CONSOLE.


> The main reason for introducing HAVE_VGA_CONSOLE is maintainability.
> The question to ask is: does the logic have to change when adding a new
> architecture?
> - If yes, you're better off with the HAVE_xxx scheme.
> - If no, use the negative dependencies in the common place.
>

Yeah, it sounds reasonable to me, originally I really do not notice
about it, in my default memory, VGA_CONSOLE is commonly used, so not
care about this item (my most experience are under pc and server).

> VGA consoles are legacy, and not used on new systems. Witness the
> list of architectures I gave above. All of them are (at least) 20 years old
> (except for ia64, but that borrows from PC legacy).
> So every time when adding a new architecture, you have to add
> '&& !NEW_ARCH' to the VGA_CONSOLE dependency, which is in a
> common place (drivers/.../Kconfig).
> While with the HAVE_xxx scheme, you don't have to change anything.
>

Are you sure about it? (especially for a new server or pc comes).

Hmm... at least, I am not quite familiar about it, if really as what
you said (e.g. acked-by another members), I will support you, too.

If what you said is correct, could you help to send related patch for
it? if I send this kind of patches, it will be easily redirected to
"/dev/null".


> Compare this to a shiny new feature that available on all new architectures,
> but not on a few old one. There it makes sense to have the negative
> dependencies on the old architectures, that will never have the new feature.
> While new architectures have enabled it by default.
>

Excuse me, my English is not quite well, I do not quite understand your
meaning, but it seems not quite important for our discussing, so I just
skip it, now.

If you feel they are necessary contents, please help to repeat again.

thanks.


> Gr{oetje,eeting}s,
>
> Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
>
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
> -- Linus Torvalds
>
>


--
Chen Gang

2013-10-24 11:50:30

by Vineet Gupta

[permalink] [raw]
Subject: Re: [PATCH] arc: kernel: add default extern variable 'screen_info' in "setup.c"

On 10/24/2013 04:56 PM, Chen Gang wrote:
> If what you said is correct, could you help to send related patch for
> it? if I send this kind of patches, it will be easily redirected to
> "/dev/null".

Geert has explained the state of things quite nicely. Please send your patch to
introduce HAVE_xxx. I'm sure maintainers will respond to it as it is the correct
thing to do. But I don't want to add bloat to kernel for a random defconfig
failure which no one will really use for ARC.

If you think u can't/won't do it - let me know and I'll send the patches instead.

-Vineet

2013-10-24 11:51:40

by Geert Uytterhoeven

[permalink] [raw]
Subject: Re: [PATCH] arc: kernel: add default extern variable 'screen_info' in "setup.c"

Hi Chen,

On Thu, Oct 24, 2013 at 1:23 PM, Chen Gang <[email protected]> wrote:
>> Lot's of negative dependencies are missing for VGA_CONSOLE.
>> I think only alpha, some arm, ia64, some mips, some powerpc, and x86
>> may use it.
>>
>
> in "arch" sub-directory, contents 13/30 (43.3%) architectures contents
> VGA_CONSOLE.
>
> 1 alpha/kernel/setup.c:137:struct screen_info screen_info = {
> 2 arm/kernel/setup.c:743:struct screen_info screen_info = {
> 3 cris/kernel/setup.c:28:struct screen_info screen_info;
> 4 ia64/kernel/setup.c:79:struct screen_info screen_info;
> 5 m32r/kernel/setup.c:55:struct screen_info screen_info = {
> 6 mips/kernel/setup.c:43:struct screen_info screen_info;
> 7 powerpc/kernel/setup-common.c:88:struct screen_info screen_info = {
> 8 score/kernel/setup.c:37:struct screen_info screen_info;
> 9 sh/kernel/setup.c:68:struct screen_info screen_info;
> 10 sparc/kernel/setup_64.c:66:struct screen_info screen_info = {
> 11 sparc/kernel/setup_32.c:53:struct screen_info screen_info = {
> 12 tile/kernel/setup.c:54:struct screen_info screen_info;
> 13 x86/kernel/setup.c:220:struct screen_info screen_info;
> 14 xtensa/kernel/setup.c:55:struct screen_info screen_info = { 0, 24, 0, 0, 0, 80, 0, 0, 0, 24, 1, 16};
>
> It seems most of pc and servers support VGA_CONSOLE.

I'm quite sure many of these just provide a screen_info to make vgacon compile.

>> Compare this to a shiny new feature that available on all new architectures,
>> but not on a few old one. There it makes sense to have the negative
>> dependencies on the old architectures, that will never have the new feature.
>> While new architectures have enabled it by default.
>>
>
> Excuse me, my English is not quite well, I do not quite understand your
> meaning, but it seems not quite important for our discussing, so I just
> skip it, now.

Sorry, I should stop writing complex sentences.
For now, nevermind. There aren't that many real cases like this.

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds

2013-10-26 02:12:20

by Chen Gang

[permalink] [raw]
Subject: Re: [PATCH] arc: kernel: add default extern variable 'screen_info' in "setup.c"

On 10/24/2013 07:50 PM, Vineet Gupta wrote:
> On 10/24/2013 04:56 PM, Chen Gang wrote:
>> If what you said is correct, could you help to send related patch for
>> it? if I send this kind of patches, it will be easily redirected to
>> "/dev/null".
>
> Geert has explained the state of things quite nicely. Please send your patch to
> introduce HAVE_xxx. I'm sure maintainers will respond to it as it is the correct
> thing to do. But I don't want to add bloat to kernel for a random defconfig
> failure which no one will really use for ARC.
>
> If you think u can't/won't do it - let me know and I'll send the patches instead.
>

Please help send.

Thanks. :-)

> -Vineet
>
>


--
Chen Gang

2013-10-26 02:14:09

by Chen Gang

[permalink] [raw]
Subject: Re: [PATCH] arc: kernel: add default extern variable 'screen_info' in "setup.c"

On 10/24/2013 07:51 PM, Geert Uytterhoeven wrote:
> Hi Chen,
>
> On Thu, Oct 24, 2013 at 1:23 PM, Chen Gang <[email protected]> wrote:
>>> Lot's of negative dependencies are missing for VGA_CONSOLE.
>>> I think only alpha, some arm, ia64, some mips, some powerpc, and x86
>>> may use it.
>>>
>>
>> in "arch" sub-directory, contents 13/30 (43.3%) architectures contents
>> VGA_CONSOLE.
>>
>> 1 alpha/kernel/setup.c:137:struct screen_info screen_info = {
>> 2 arm/kernel/setup.c:743:struct screen_info screen_info = {
>> 3 cris/kernel/setup.c:28:struct screen_info screen_info;
>> 4 ia64/kernel/setup.c:79:struct screen_info screen_info;
>> 5 m32r/kernel/setup.c:55:struct screen_info screen_info = {
>> 6 mips/kernel/setup.c:43:struct screen_info screen_info;
>> 7 powerpc/kernel/setup-common.c:88:struct screen_info screen_info = {
>> 8 score/kernel/setup.c:37:struct screen_info screen_info;
>> 9 sh/kernel/setup.c:68:struct screen_info screen_info;
>> 10 sparc/kernel/setup_64.c:66:struct screen_info screen_info = {
>> 11 sparc/kernel/setup_32.c:53:struct screen_info screen_info = {
>> 12 tile/kernel/setup.c:54:struct screen_info screen_info;
>> 13 x86/kernel/setup.c:220:struct screen_info screen_info;
>> 14 xtensa/kernel/setup.c:55:struct screen_info screen_info = { 0, 24, 0, 0, 0, 80, 0, 0, 0, 24, 1, 16};
>>
>> It seems most of pc and servers support VGA_CONSOLE.
>
> I'm quite sure many of these just provide a screen_info to make vgacon compile.
>

Thank you for your confirmation.

>>> Compare this to a shiny new feature that available on all new architectures,
>>> but not on a few old one. There it makes sense to have the negative
>>> dependencies on the old architectures, that will never have the new feature.
>>> While new architectures have enabled it by default.
>>>
>>
>> Excuse me, my English is not quite well, I do not quite understand your
>> meaning, but it seems not quite important for our discussing, so I just
>> skip it, now.
>
> Sorry, I should stop writing complex sentences.
> For now, nevermind. There aren't that many real cases like this.
>

And I should be still improving my English. :-)

> Gr{oetje,eeting}s,
>
> Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
>
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
> -- Linus Torvalds
>
>

Thanks.
--
Chen Gang