[PATCH] h8300/kernel/setup.c: add "linux/initrd.h" to pass compiling

[PATCH] h8300/kernel/setup.c: add "linux/initrd.h" to pass compiling

[Chen Gang]

Need add "linux/initrd.h" to pass compiling.

The related error (allmodconfig for h8300):

  arch/h8300/kernel/setup.c: In function 'setup_arch':
  arch/h8300/kernel/setup.c:103:3: error: 'initrd_start' undeclared (first use in this function)
     initrd_start = memory_start;
     ^
  arch/h8300/kernel/setup.c:103:3: note: each undeclared identifier is reported only once for each function it appears in
  arch/h8300/kernel/setup.c:104:3: error: 'initrd_end' undeclared (first use in this function)
     initrd_end = memory_start += be32_to_cpu(((unsigned long *) (memory_start))[2]);
     ^

Signed-off-by: Chen Gang <gang.chen@asianux.com>
---
 arch/h8300/kernel/setup.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/arch/h8300/kernel/setup.c b/arch/h8300/kernel/setup.c
index d0b1607..684e734 100644
--- a/arch/h8300/kernel/setup.c
+++ b/arch/h8300/kernel/setup.c
@@ -47,6 +47,9 @@
 #include <asm/regs267x.h>
 #endif
 
+#if defined(CONFIG_BLK_DEV_INITRD)
+#include <linux/initrd.h>
+#endif
 #define STUBSIZE 0xc000
 
 unsigned long rom_length;
-- 
1.7.7.6

Re: [PATCH] h8300/kernel/setup.c: add "linux/initrd.h" to pass compiling

[Geert Uytterhoeven]

On Mon, Aug 26, 2013 at 12:31 PM, Chen Gang <gang.chen@asianux.com> wrote:
> --- a/arch/h8300/kernel/setup.c
> +++ b/arch/h8300/kernel/setup.c
> @@ -47,6 +47,9 @@
>  #include <asm/regs267x.h>
>  #endif
>
> +#if defined(CONFIG_BLK_DEV_INITRD)

Why have you added the #ifdef?

> +#include <linux/initrd.h>
> +#endif
>  #define STUBSIZE 0xc000

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

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

Re: [PATCH] h8300/kernel/setup.c: add "linux/initrd.h" to pass compiling

[Chen Gang]

On 08/26/2013 07:00 PM, Geert Uytterhoeven wrote:
> On Mon, Aug 26, 2013 at 12:31 PM, Chen Gang <gang.chen@asianux.com> wrote:
>> --- a/arch/h8300/kernel/setup.c
>> +++ b/arch/h8300/kernel/setup.c
>> @@ -47,6 +47,9 @@
>>  #include <asm/regs267x.h>
>>  #endif
>>
>> +#if defined(CONFIG_BLK_DEV_INITRD)
> 
> Why have you added the #ifdef?
> 

The related code is below (maybe we need add additional related
comments in the patch for it ?).

in arch/h8300/kernel/setup.c

 94 void __init setup_arch(char **cmdline_p)
 95 {
 96         int bootmap_size;
 97 
 98         memory_start = (unsigned long) &_ramstart;
 99 
100         /* allow for ROMFS on the end of the kernel */
101         if (memcmp((void *)memory_start, "-rom1fs-", 8) == 0) {
102 #if defined(CONFIG_BLK_DEV_INITRD)
103                 initrd_start = memory_start;
104                 initrd_end = memory_start += be32_to_cpu(((unsigned long *) (memory_start))[2]);
105 #else
106                 memory_start += be32_to_cpu(((unsigned long *) memory_start)[2]);
107 #endif
108         }


>> +#include <linux/initrd.h>
>> +#endif
>>  #define STUBSIZE 0xc000
> 
> Gr{oetje,eeting}s,
> 
>                         Geert
> 
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
> 
> 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

Re: [PATCH] h8300/kernel/setup.c: add "linux/initrd.h" to pass compiling

[Geert Uytterhoeven]

On Mon, Aug 26, 2013 at 1:06 PM, Chen Gang <gang.chen@asianux.com> wrote:
> On 08/26/2013 07:00 PM, Geert Uytterhoeven wrote:
>> On Mon, Aug 26, 2013 at 12:31 PM, Chen Gang <gang.chen@asianux.com> wrote:
>>> --- a/arch/h8300/kernel/setup.c
>>> +++ b/arch/h8300/kernel/setup.c
>>> @@ -47,6 +47,9 @@
>>>  #include <asm/regs267x.h>
>>>  #endif
>>>
>>> +#if defined(CONFIG_BLK_DEV_INITRD)
>>
>> Why have you added the #ifdef?
>>
>
> The related code is below (maybe we need add additional related
> comments in the patch for it ?).
>
> in arch/h8300/kernel/setup.c
>
>  94 void __init setup_arch(char **cmdline_p)
>  95 {
>  96         int bootmap_size;
>  97
>  98         memory_start = (unsigned long) &_ramstart;
>  99
> 100         /* allow for ROMFS on the end of the kernel */
> 101         if (memcmp((void *)memory_start, "-rom1fs-", 8) == 0) {
> 102 #if defined(CONFIG_BLK_DEV_INITRD)
> 103                 initrd_start = memory_start;
> 104                 initrd_end = memory_start += be32_to_cpu(((unsigned long *) (memory_start))[2]);
> 105 #else
> 106                 memory_start += be32_to_cpu(((unsigned long *) memory_start)[2]);
> 107 #endif
> 108         }

Sure, it's used conditionally. But it doesn't harm to always include it.
That means less #ifdefs in the code.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

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

Re: [PATCH] h8300/kernel/setup.c: add "linux/initrd.h" to pass compiling

[Chen Gang]

On 08/26/2013 07:08 PM, Geert Uytterhoeven wrote:
> On Mon, Aug 26, 2013 at 1:06 PM, Chen Gang <gang.chen@asianux.com> wrote:
>> On 08/26/2013 07:00 PM, Geert Uytterhoeven wrote:
>>> On Mon, Aug 26, 2013 at 12:31 PM, Chen Gang <gang.chen@asianux.com> wrote:
>>>> --- a/arch/h8300/kernel/setup.c
>>>> +++ b/arch/h8300/kernel/setup.c
>>>> @@ -47,6 +47,9 @@
>>>>  #include <asm/regs267x.h>
>>>>  #endif
>>>>
>>>> +#if defined(CONFIG_BLK_DEV_INITRD)
>>>
>>> Why have you added the #ifdef?
>>>
>>
>> The related code is below (maybe we need add additional related
>> comments in the patch for it ?).
>>
>> in arch/h8300/kernel/setup.c
>>
>>  94 void __init setup_arch(char **cmdline_p)
>>  95 {
>>  96         int bootmap_size;
>>  97
>>  98         memory_start = (unsigned long) &_ramstart;
>>  99
>> 100         /* allow for ROMFS on the end of the kernel */
>> 101         if (memcmp((void *)memory_start, "-rom1fs-", 8) == 0) {
>> 102 #if defined(CONFIG_BLK_DEV_INITRD)
>> 103                 initrd_start = memory_start;
>> 104                 initrd_end = memory_start += be32_to_cpu(((unsigned long *) (memory_start))[2]);
>> 105 #else
>> 106                 memory_start += be32_to_cpu(((unsigned long *) memory_start)[2]);
>> 107 #endif
>> 108         }
> 
> Sure, it's used conditionally. But it doesn't harm to always include it.
> That means less #ifdefs in the code.
> 

Hmm... I feel, add "#ifdefs" can make the code more clearer (consistent
with the "#ifdefs" 'for initrd_start' and 'end').

For C code readers, more code doesn't mean more complex, if it can make
things clearer after add some more lines (and be sure of no negative
effect with performance), normally I prefer to add some more lines.

And this file has already had an area for all "#ifdefs include", I just
add it in this area, so at least, it can mach this file well, is it ?

;-)


Thanks.

> Gr{oetje,eeting}s,
> 
>                         Geert
> 
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
> 
> 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

Re: [PATCH] h8300/kernel/setup.c: add "linux/initrd.h" to pass compiling

[Guenter Roeck]

On Mon, Aug 26, 2013 at 07:19:38PM +0800, Chen Gang wrote:
> On 08/26/2013 07:08 PM, Geert Uytterhoeven wrote:
> > On Mon, Aug 26, 2013 at 1:06 PM, Chen Gang <gang.chen@asianux.com> wrote:
> >> On 08/26/2013 07:00 PM, Geert Uytterhoeven wrote:
> >>> On Mon, Aug 26, 2013 at 12:31 PM, Chen Gang <gang.chen@asianux.com> wrote:
> >>>> --- a/arch/h8300/kernel/setup.c
> >>>> +++ b/arch/h8300/kernel/setup.c
> >>>> @@ -47,6 +47,9 @@
> >>>>  #include <asm/regs267x.h>
> >>>>  #endif
> >>>>
> >>>> +#if defined(CONFIG_BLK_DEV_INITRD)
> >>>
> >>> Why have you added the #ifdef?
> >>>
> >>
> >> The related code is below (maybe we need add additional related
> >> comments in the patch for it ?).
> >>
> >> in arch/h8300/kernel/setup.c
> >>
> >>  94 void __init setup_arch(char **cmdline_p)
> >>  95 {
> >>  96         int bootmap_size;
> >>  97
> >>  98         memory_start = (unsigned long) &_ramstart;
> >>  99
> >> 100         /* allow for ROMFS on the end of the kernel */
> >> 101         if (memcmp((void *)memory_start, "-rom1fs-", 8) == 0) {
> >> 102 #if defined(CONFIG_BLK_DEV_INITRD)
> >> 103                 initrd_start = memory_start;
> >> 104                 initrd_end = memory_start += be32_to_cpu(((unsigned long *) (memory_start))[2]);
> >> 105 #else
> >> 106                 memory_start += be32_to_cpu(((unsigned long *) memory_start)[2]);
> >> 107 #endif
> >> 108         }
> > 
> > Sure, it's used conditionally. But it doesn't harm to always include it.
> > That means less #ifdefs in the code.
> > 
> 
> Hmm... I feel, add "#ifdefs" can make the code more clearer (consistent
> with the "#ifdefs" 'for initrd_start' and 'end').
> 
The goal in the Linux kernel is to reduce the amount of ifdefs in the code
by moving conditional code as much as possible into header files. That actually
has a practical advantage - it ensures that all code is compiled.

You may agree or disagree with this approach, but you should follow it and not
add new ifdefs when you write kernel code, much less unnecessary ones.
If anything, you might try to remove existing ifdefs while you are at it.

> For C code readers, more code doesn't mean more complex, if it can make
> things clearer after add some more lines (and be sure of no negative
> effect with performance), normally I prefer to add some more lines.
> 
The Linux kernel community tends to think otherwise. For my part I don't
see how ifdefs, and much more so unecessary ones, make anything clearer.
I would argue you'll end up not seeing the forest because of all the trees.

> And this file has already had an area for all "#ifdefs include", I just
> add it in this area, so at least, it can mach this file well, is it ?
> 
Your argument is along the line of "the code is bad anyway, so it is ok
to make it worse". Not really a good argument if you want to get your code
accepted.

I would suggest to read Documentation/SubmittingPatches, section 2.2,
before you continue with your line of argument.

Guenter

Re: [PATCH] h8300/kernel/setup.c: add "linux/initrd.h" to pass compiling

[Chen Gang]

On 08/27/2013 06:12 AM, Guenter Roeck wrote:
> On Mon, Aug 26, 2013 at 07:19:38PM +0800, Chen Gang wrote:
>> On 08/26/2013 07:08 PM, Geert Uytterhoeven wrote:
>>> On Mon, Aug 26, 2013 at 1:06 PM, Chen Gang <gang.chen@asianux.com> wrote:
>>>> On 08/26/2013 07:00 PM, Geert Uytterhoeven wrote:
>>>>> On Mon, Aug 26, 2013 at 12:31 PM, Chen Gang <gang.chen@asianux.com> wrote:
>>>>>> --- a/arch/h8300/kernel/setup.c
>>>>>> +++ b/arch/h8300/kernel/setup.c
>>>>>> @@ -47,6 +47,9 @@
>>>>>>  #include <asm/regs267x.h>
>>>>>>  #endif
>>>>>>
>>>>>> +#if defined(CONFIG_BLK_DEV_INITRD)
>>>>>
>>>>> Why have you added the #ifdef?
>>>>>
>>>>
>>>> The related code is below (maybe we need add additional related
>>>> comments in the patch for it ?).
>>>>
>>>> in arch/h8300/kernel/setup.c
>>>>
>>>>  94 void __init setup_arch(char **cmdline_p)
>>>>  95 {
>>>>  96         int bootmap_size;
>>>>  97
>>>>  98         memory_start = (unsigned long) &_ramstart;
>>>>  99
>>>> 100         /* allow for ROMFS on the end of the kernel */
>>>> 101         if (memcmp((void *)memory_start, "-rom1fs-", 8) == 0) {
>>>> 102 #if defined(CONFIG_BLK_DEV_INITRD)
>>>> 103                 initrd_start = memory_start;
>>>> 104                 initrd_end = memory_start += be32_to_cpu(((unsigned long *) (memory_start))[2]);
>>>> 105 #else
>>>> 106                 memory_start += be32_to_cpu(((unsigned long *) memory_start)[2]);
>>>> 107 #endif
>>>> 108         }
>>>
>>> Sure, it's used conditionally. But it doesn't harm to always include it.
>>> That means less #ifdefs in the code.
>>>
>>
>> Hmm... I feel, add "#ifdefs" can make the code more clearer (consistent
>> with the "#ifdefs" 'for initrd_start' and 'end').
>>
> The goal in the Linux kernel is to reduce the amount of ifdefs in the code
> by moving conditional code as much as possible into header files. That actually
> has a practical advantage - it ensures that all code is compiled.
> 

Yeah, it should be.

> You may agree or disagree with this approach, but you should follow it and not
> add new ifdefs when you write kernel code, much less unnecessary ones.
> If anything, you might try to remove existing ifdefs while you are at it.
> 

Of cause, I agree.

And do you guess: "I do not agree with it" ?


>> For C code readers, more code doesn't mean more complex, if it can make
>> things clearer after add some more lines (and be sure of no negative
>> effect with performance), normally I prefer to add some more lines.
>>
> The Linux kernel community tends to think otherwise. For my part I don't
> see how ifdefs, and much more so unecessary ones, make anything clearer.
> I would argue you'll end up not seeing the forest because of all the trees.
> 

Hmm... it seems the 'clearer' depends on personal feelings (and 'clear'
does not mean 'code style'). 
 
For my feeling, when the code is more match the 'real world' and more
consistent with each other, it will be more clearer (maybe it is in
'bad code style').

In fact, 'forest' is not conflict with "all the trees", when we feel we
need discuss about it, it means both of them need improving.

  for satisfy "all the trees": need fix the issue.
  for satisfy "forest": need beautify code.


>> And this file has already had an area for all "#ifdefs include", I just
>> add it in this area, so at least, it can mach this file well, is it ?
>>
> Your argument is along the line of "the code is bad anyway, so it is ok
> to make it worse". Not really a good argument if you want to get your code
> accepted.
> 

So we need divide our current discussion contents into 2 patches.

  1st patch is for issue fix. it should follow the original 'coding styles' no matter whether it is good or bad.
    (it is just current patch).

  2nd patch is for 'beautify code', it should use 'correct' coding styles.
    (if possible I will send 2nd patch for it, it is not a bad idea to me if it can be applied). ;-)


> I would suggest to read Documentation/SubmittingPatches, section 2.2,
> before you continue with your line of argument.
> 

Yeah, I have already read it 6 months ago.

Hmm... but it is not bad idea for every members to read it once more
(including you and me).


> Guenter
> 
> 

Thanks.
-- 
Chen Gang

Re: [PATCH] h8300/kernel/setup.c: add "linux/initrd.h" to pass compiling

[Chen Gang]

On 08/26/2013 06:31 PM, Chen Gang wrote:
> Need add "linux/initrd.h" to pass compiling.
> 
> The related error (allmodconfig for h8300):
> 
>   arch/h8300/kernel/setup.c: In function 'setup_arch':
>   arch/h8300/kernel/setup.c:103:3: error: 'initrd_start' undeclared (first use in this function)
>      initrd_start = memory_start;
>      ^
>   arch/h8300/kernel/setup.c:103:3: note: each undeclared identifier is reported only once for each function it appears in
>   arch/h8300/kernel/setup.c:104:3: error: 'initrd_end' undeclared (first use in this function)
>      initrd_end = memory_start += be32_to_cpu(((unsigned long *) (memory_start))[2]);
>      ^
> 
> Signed-off-by: Chen Gang <gang.chen@asianux.com>
> ---
>  arch/h8300/kernel/setup.c |    3 +++
>  1 files changed, 3 insertions(+), 0 deletions(-)
> 
> diff --git a/arch/h8300/kernel/setup.c b/arch/h8300/kernel/setup.c
> index d0b1607..684e734 100644
> --- a/arch/h8300/kernel/setup.c
> +++ b/arch/h8300/kernel/setup.c
> @@ -47,6 +47,9 @@
>  #include <asm/regs267x.h>
>  #endif
>  
> +#if defined(CONFIG_BLK_DEV_INITRD)
> +#include <linux/initrd.h>
> +#endif
>  #define STUBSIZE 0xc000
>  

Oh, it need an empty line after "#endif" to mach "current coding style".

I will send patch v2 for it.

Thanks.

>  unsigned long rom_length;
> 


-- 
Chen Gang

[PATCH v2] h8300/kernel/setup.c: add "linux/initrd.h" to pass compiling

[Chen Gang]

The related error (allmodconfig for h8300):

  arch/h8300/kernel/setup.c: In function 'setup_arch':
  arch/h8300/kernel/setup.c:103:3: error: 'initrd_start' undeclared (first use in this function)
     initrd_start = memory_start;
     ^
  arch/h8300/kernel/setup.c:103:3: note: each undeclared identifier is reported only once for each function it appears in
  arch/h8300/kernel/setup.c:104:3: error: 'initrd_end' undeclared (first use in this function)
     initrd_end = memory_start += be32_to_cpu(((unsigned long *) (memory_start))[2]);
     ^

Signed-off-by: Chen Gang <gang.chen@asianux.com>
---
 arch/h8300/kernel/setup.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/arch/h8300/kernel/setup.c b/arch/h8300/kernel/setup.c
index d0b1607..85639a1 100644
--- a/arch/h8300/kernel/setup.c
+++ b/arch/h8300/kernel/setup.c
@@ -47,6 +47,10 @@
 #include <asm/regs267x.h>
 #endif
 
+#if defined(CONFIG_BLK_DEV_INITRD)
+#include <linux/initrd.h>
+#endif
+
 #define STUBSIZE 0xc000
 
 unsigned long rom_length;
-- 
1.7.7.6

Re: [PATCH v2] h8300/kernel/setup.c: add "linux/initrd.h" to pass compiling

[Guenter Roeck]

On 08/29/2013 08:59 PM, Chen Gang wrote:
> The related error (allmodconfig for h8300):
>
>    arch/h8300/kernel/setup.c: In function 'setup_arch':
>    arch/h8300/kernel/setup.c:103:3: error: 'initrd_start' undeclared (first use in this function)
>       initrd_start = memory_start;
>       ^
>    arch/h8300/kernel/setup.c:103:3: note: each undeclared identifier is reported only once for each function it appears in
>    arch/h8300/kernel/setup.c:104:3: error: 'initrd_end' undeclared (first use in this function)
>       initrd_end = memory_start += be32_to_cpu(((unsigned long *) (memory_start))[2]);
>       ^
>
> Signed-off-by: Chen Gang <gang.chen@asianux.com>
> ---
>   arch/h8300/kernel/setup.c |    4 ++++
>   1 files changed, 4 insertions(+), 0 deletions(-)
>
> diff --git a/arch/h8300/kernel/setup.c b/arch/h8300/kernel/setup.c
> index d0b1607..85639a1 100644
> --- a/arch/h8300/kernel/setup.c
> +++ b/arch/h8300/kernel/setup.c
> @@ -47,6 +47,10 @@
>   #include <asm/regs267x.h>
>   #endif
>
> +#if defined(CONFIG_BLK_DEV_INITRD)
> +#include <linux/initrd.h>
> +#endif
> +

Is the #ifdef/#endif really needed ? If not you should drop it.

Thanks,
Guenter

Re: [PATCH v2] h8300/kernel/setup.c: add "linux/initrd.h" to pass compiling

[Chen Gang]

On 08/30/2013 12:32 PM, Guenter Roeck wrote:
> On 08/29/2013 08:59 PM, Chen Gang wrote:
>> The related error (allmodconfig for h8300):
>>
>>    arch/h8300/kernel/setup.c: In function 'setup_arch':
>>    arch/h8300/kernel/setup.c:103:3: error: 'initrd_start' undeclared
>> (first use in this function)
>>       initrd_start = memory_start;
>>       ^
>>    arch/h8300/kernel/setup.c:103:3: note: each undeclared identifier
>> is reported only once for each function it appears in
>>    arch/h8300/kernel/setup.c:104:3: error: 'initrd_end' undeclared
>> (first use in this function)
>>       initrd_end = memory_start += be32_to_cpu(((unsigned long *)
>> (memory_start))[2]);
>>       ^
>>
>> Signed-off-by: Chen Gang <gang.chen@asianux.com>
>> ---
>>   arch/h8300/kernel/setup.c |    4 ++++
>>   1 files changed, 4 insertions(+), 0 deletions(-)
>>
>> diff --git a/arch/h8300/kernel/setup.c b/arch/h8300/kernel/setup.c
>> index d0b1607..85639a1 100644
>> --- a/arch/h8300/kernel/setup.c
>> +++ b/arch/h8300/kernel/setup.c
>> @@ -47,6 +47,10 @@
>>   #include <asm/regs267x.h>
>>   #endif
>>
>> +#if defined(CONFIG_BLK_DEV_INITRD)
>> +#include <linux/initrd.h>
>> +#endif
>> +
> 
> Is the #ifdef/#endif really needed ? If not you should drop it.
> 

"linux/initrd.h" is needed by 'initrd_start' and 'initrd_end' when
BLK_DEV_INITRD enabled.

'memory_start' is defined within this file, and also only one place may
use "linux/initrd.h" within this file.

So if BLK_DEV_INITRD disabled, do not need "linux/initrd.h" either.


Please reference the code in "arch/h8300/kernel/setup.c" below.

102 void __init setup_arch(char **cmdline_p)
103 {
104         int bootmap_size;
105 
106         memory_start = (unsigned long) &_ramstart;
107 
108         /* allow for ROMFS on the end of the kernel */
109         if (memcmp((void *)memory_start, "-rom1fs-", 8) == 0) {
110 #if defined(CONFIG_BLK_DEV_INITRD)
111                 initrd_start = memory_start;
112                 initrd_end = memory_start += be32_to_cpu(((unsigned long *) (memory_start))[2]);
113 #else
114                 memory_start += be32_to_cpu(((unsigned long *) memory_start)[2]);
115 #endif
116         }



> Thanks,
> Guenter
> 
> 
> 

Thanks.
-- 
Chen Gang

Re: [PATCH v2] h8300/kernel/setup.c: add "linux/initrd.h" to pass compiling

[Guenter Roeck]

On 08/29/2013 10:49 PM, Chen Gang wrote:
> On 08/30/2013 12:32 PM, Guenter Roeck wrote:
>> On 08/29/2013 08:59 PM, Chen Gang wrote:
>>> The related error (allmodconfig for h8300):
>>>
>>>     arch/h8300/kernel/setup.c: In function 'setup_arch':
>>>     arch/h8300/kernel/setup.c:103:3: error: 'initrd_start' undeclared
>>> (first use in this function)
>>>        initrd_start = memory_start;
>>>        ^
>>>     arch/h8300/kernel/setup.c:103:3: note: each undeclared identifier
>>> is reported only once for each function it appears in
>>>     arch/h8300/kernel/setup.c:104:3: error: 'initrd_end' undeclared
>>> (first use in this function)
>>>        initrd_end = memory_start += be32_to_cpu(((unsigned long *)
>>> (memory_start))[2]);
>>>        ^
>>>
>>> Signed-off-by: Chen Gang <gang.chen@asianux.com>
>>> ---
>>>    arch/h8300/kernel/setup.c |    4 ++++
>>>    1 files changed, 4 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/arch/h8300/kernel/setup.c b/arch/h8300/kernel/setup.c
>>> index d0b1607..85639a1 100644
>>> --- a/arch/h8300/kernel/setup.c
>>> +++ b/arch/h8300/kernel/setup.c
>>> @@ -47,6 +47,10 @@
>>>    #include <asm/regs267x.h>
>>>    #endif
>>>
>>> +#if defined(CONFIG_BLK_DEV_INITRD)
>>> +#include <linux/initrd.h>
>>> +#endif
>>> +
>>
>> Is the #ifdef/#endif really needed ? If not you should drop it.
>>
>
> "linux/initrd.h" is needed by 'initrd_start' and 'initrd_end' when
> BLK_DEV_INITRD enabled.
>
> 'memory_start' is defined within this file, and also only one place may
> use "linux/initrd.h" within this file.
>
> So if BLK_DEV_INITRD disabled, do not need "linux/initrd.h" either.
>

I didn't ask if the include is needed, I asked if the ifdef is needed.

The goal is to reduce the number of ifdefs in the code. We should not
include new ones if not necessary. That there are already other
(possibly unnecessary) ifdefs in the code doesn't mean we should add new ones.

Guenter

Re: [PATCH v2] h8300/kernel/setup.c: add "linux/initrd.h" to pass compiling

[Chen Gang]

On 08/30/2013 07:12 PM, Guenter Roeck wrote:
> On 08/29/2013 10:49 PM, Chen Gang wrote:
>> On 08/30/2013 12:32 PM, Guenter Roeck wrote:
>>> On 08/29/2013 08:59 PM, Chen Gang wrote:
>>>> The related error (allmodconfig for h8300):
>>>>
>>>>     arch/h8300/kernel/setup.c: In function 'setup_arch':
>>>>     arch/h8300/kernel/setup.c:103:3: error: 'initrd_start' undeclared
>>>> (first use in this function)
>>>>        initrd_start = memory_start;
>>>>        ^
>>>>     arch/h8300/kernel/setup.c:103:3: note: each undeclared identifier
>>>> is reported only once for each function it appears in
>>>>     arch/h8300/kernel/setup.c:104:3: error: 'initrd_end' undeclared
>>>> (first use in this function)
>>>>        initrd_end = memory_start += be32_to_cpu(((unsigned long *)
>>>> (memory_start))[2]);
>>>>        ^
>>>>
>>>> Signed-off-by: Chen Gang <gang.chen@asianux.com>
>>>> ---
>>>>    arch/h8300/kernel/setup.c |    4 ++++
>>>>    1 files changed, 4 insertions(+), 0 deletions(-)
>>>>
>>>> diff --git a/arch/h8300/kernel/setup.c b/arch/h8300/kernel/setup.c
>>>> index d0b1607..85639a1 100644
>>>> --- a/arch/h8300/kernel/setup.c
>>>> +++ b/arch/h8300/kernel/setup.c
>>>> @@ -47,6 +47,10 @@
>>>>    #include <asm/regs267x.h>
>>>>    #endif
>>>>
>>>> +#if defined(CONFIG_BLK_DEV_INITRD)
>>>> +#include <linux/initrd.h>
>>>> +#endif
>>>> +
>>>
>>> Is the #ifdef/#endif really needed ? If not you should drop it.
>>>
>>
>> "linux/initrd.h" is needed by 'initrd_start' and 'initrd_end' when
>> BLK_DEV_INITRD enabled.
>>
>> 'memory_start' is defined within this file, and also only one place may
>> use "linux/initrd.h" within this file.
>>
>> So if BLK_DEV_INITRD disabled, do not need "linux/initrd.h" either.
>>
> 
> I didn't ask if the include is needed, I asked if the ifdef is needed.
> 

Adding "ifdef" can mach original/local code style (no matter whether the
code style is good or not),


> The goal is to reduce the number of ifdefs in the code. We should not
> include new ones if not necessary. That there are already other
> (possibly unnecessary) ifdefs in the code doesn't mean we should add new
> ones.
> 

Yeah, if necessary, we need send additional patch to try to reduce all
of "ifdef" within this ".c" file.

In my opinion, we'd better to avoid 2 or more coding styles in one ".c"
file.


> Guenter
> 

Thanks.
-- 
Chen Gang

Re: [PATCH v2] h8300/kernel/setup.c: add "linux/initrd.h" to pass compiling

[Guenter Roeck]

On 08/29/2013 08:59 PM, Chen Gang wrote:
> The related error (allmodconfig for h8300):
>
>    arch/h8300/kernel/setup.c: In function 'setup_arch':
>    arch/h8300/kernel/setup.c:103:3: error: 'initrd_start' undeclared (first use in this function)
>       initrd_start = memory_start;
>       ^
>    arch/h8300/kernel/setup.c:103:3: note: each undeclared identifier is reported only once for each function it appears in
>    arch/h8300/kernel/setup.c:104:3: error: 'initrd_end' undeclared (first use in this function)
>       initrd_end = memory_start += be32_to_cpu(((unsigned long *) (memory_start))[2]);
>       ^
>
> Signed-off-by: Chen Gang <gang.chen@asianux.com>
> ---

Maybe an odd question, but is there a way to actually compile the h8300 target
in the first place ? The cross compiler on kernel.org doesn't work, nor does
the one available through Ubuntu.

Guenter

Re: [PATCH v2] h8300/kernel/setup.c: add "linux/initrd.h" to pass compiling

[Chen Gang]

On 08/30/2013 12:53 PM, Guenter Roeck wrote:
> On 08/29/2013 08:59 PM, Chen Gang wrote:
>> The related error (allmodconfig for h8300):
>>
>>    arch/h8300/kernel/setup.c: In function 'setup_arch':
>>    arch/h8300/kernel/setup.c:103:3: error: 'initrd_start' undeclared
>> (first use in this function)
>>       initrd_start = memory_start;
>>       ^
>>    arch/h8300/kernel/setup.c:103:3: note: each undeclared identifier
>> is reported only once for each function it appears in
>>    arch/h8300/kernel/setup.c:104:3: error: 'initrd_end' undeclared
>> (first use in this function)
>>       initrd_end = memory_start += be32_to_cpu(((unsigned long *)
>> (memory_start))[2]);
>>       ^
>>
>> Signed-off-by: Chen Gang <gang.chen@asianux.com>
>> ---
> 
> Maybe an odd question, but is there a way to actually compile the h8300
> target

Firstly, at least for me, I don't think it is an odd question.  :-)

For the tool-chain:

  I compiled the cross-compiler from the gcc and binutils source code.
  GCC has too many bugs to compile kernel with -Os (normal make will fail).
  If without -Os (no optimization), it can work correctly which is enough for my goal: "let h8300 pass allmodconfig".  ;-)

>From building with allmodconfig for h8300:

  I can find more chances to provide contributes (both for h8300 and for others).
  I can learn more in kernel wide.
  I can familiar the gcc and binutils step by step (especially to familiar with their issues).

Next:

  I will communicate/work with the gcc guys for the gcc issues which found by building kernel.

  :-)



> in the first place ? The cross compiler on kernel.org doesn't work, nor
> does
> the one available through Ubuntu.
> 
> Guenter
> 

For binutils, no release under Ubuntu, and the Fedora17's is incorrect
(can not use), but the binutils-2.22 from gnu is OK.

For gcc, no release under Ubuntu, for Fedora17's, gcc-4.9, gcc-4.8,
gcc-4.7.4, and gcc-4.4.7 all have bugs for compiling kernel(their bugs
are different too).

It is really not easy to fix these bugs (gcc guys have too many issues
to fix), even if really find the root cause, it is still difficult to
fix (fix one bug is very easy to cause another more issues).



I almost have no compiler related experience, but I plan to dive into
 -- Nothing is impossible, if God bless.


Thanks.
-- 
Chen Gang

Re: [PATCH v2] h8300/kernel/setup.c: add "linux/initrd.h" to pass compiling

[Guenter Roeck]

On 08/29/2013 11:34 PM, Chen Gang wrote:
> On 08/30/2013 12:53 PM, Guenter Roeck wrote:
>> On 08/29/2013 08:59 PM, Chen Gang wrote:
>>> The related error (allmodconfig for h8300):
>>>
>>>     arch/h8300/kernel/setup.c: In function 'setup_arch':
>>>     arch/h8300/kernel/setup.c:103:3: error: 'initrd_start' undeclared
>>> (first use in this function)
>>>        initrd_start = memory_start;
>>>        ^
>>>     arch/h8300/kernel/setup.c:103:3: note: each undeclared identifier
>>> is reported only once for each function it appears in
>>>     arch/h8300/kernel/setup.c:104:3: error: 'initrd_end' undeclared
>>> (first use in this function)
>>>        initrd_end = memory_start += be32_to_cpu(((unsigned long *)
>>> (memory_start))[2]);
>>>        ^
>>>
>>> Signed-off-by: Chen Gang <gang.chen@asianux.com>
>>> ---
>>
>> Maybe an odd question, but is there a way to actually compile the h8300
>> target
>
> Firstly, at least for me, I don't think it is an odd question.  :-)
>
> For the tool-chain:
>
>    I compiled the cross-compiler from the gcc and binutils source code.
>    GCC has too many bugs to compile kernel with -Os (normal make will fail).
>    If without -Os (no optimization), it can work correctly which is enough for my goal: "let h8300 pass allmodconfig".  ;-)
>
>>From building with allmodconfig for h8300:
>
>    I can find more chances to provide contributes (both for h8300 and for others).
>    I can learn more in kernel wide.
>    I can familiar the gcc and binutils step by step (especially to familiar with their issues).
>
> Next:
>
>    I will communicate/work with the gcc guys for the gcc issues which found by building kernel.
>
>    :-)
>
>
>
>> in the first place ? The cross compiler on kernel.org doesn't work, nor
>> does
>> the one available through Ubuntu.
>>
>> Guenter
>>
>
> For binutils, no release under Ubuntu, and the Fedora17's is incorrect
> (can not use), but the binutils-2.22 from gnu is OK.
>
> For gcc, no release under Ubuntu, for Fedora17's, gcc-4.9, gcc-4.8,
> gcc-4.7.4, and gcc-4.4.7 all have bugs for compiling kernel(their bugs
> are different too).
>
> It is really not easy to fix these bugs (gcc guys have too many issues
> to fix), even if really find the root cause, it is still difficult to
> fix (fix one bug is very easy to cause another more issues).
>

I have to wonder ... is this all worth it ? It almost looks like no one
is using this architecture anymore. Do you have target hardware to test
any of your changes ?

Thanks,
Guenter

Re: [PATCH v2] h8300/kernel/setup.c: add "linux/initrd.h" to pass compiling

[richard -rw- weinberger]

On Fri, Aug 30, 2013 at 1:18 PM, Guenter Roeck <linux@roeck-us.net> wrote:
> On 08/29/2013 11:34 PM, Chen Gang wrote:
>>
>> On 08/30/2013 12:53 PM, Guenter Roeck wrote:
>>>
>>> On 08/29/2013 08:59 PM, Chen Gang wrote:
>>>>
>>>> The related error (allmodconfig for h8300):
>>>>
>>>>     arch/h8300/kernel/setup.c: In function 'setup_arch':
>>>>     arch/h8300/kernel/setup.c:103:3: error: 'initrd_start' undeclared
>>>> (first use in this function)
>>>>        initrd_start = memory_start;
>>>>        ^
>>>>     arch/h8300/kernel/setup.c:103:3: note: each undeclared identifier
>>>> is reported only once for each function it appears in
>>>>     arch/h8300/kernel/setup.c:104:3: error: 'initrd_end' undeclared
>>>> (first use in this function)
>>>>        initrd_end = memory_start += be32_to_cpu(((unsigned long *)
>>>> (memory_start))[2]);
>>>>        ^
>>>>
>>>> Signed-off-by: Chen Gang <gang.chen@asianux.com>
>>>> ---
>>>
>>>
>>> Maybe an odd question, but is there a way to actually compile the h8300
>>> target
>>
>>
>> Firstly, at least for me, I don't think it is an odd question.  :-)
>>
>> For the tool-chain:
>>
>>    I compiled the cross-compiler from the gcc and binutils source code.
>>    GCC has too many bugs to compile kernel with -Os (normal make will
>> fail).
>>    If without -Os (no optimization), it can work correctly which is enough
>> for my goal: "let h8300 pass allmodconfig".  ;-)
>>
>>> From building with allmodconfig for h8300:
>>
>>
>>    I can find more chances to provide contributes (both for h8300 and for
>> others).
>>    I can learn more in kernel wide.
>>    I can familiar the gcc and binutils step by step (especially to
>> familiar with their issues).
>>
>> Next:
>>
>>    I will communicate/work with the gcc guys for the gcc issues which
>> found by building kernel.
>>
>>    :-)
>>
>>
>>
>>> in the first place ? The cross compiler on kernel.org doesn't work, nor
>>> does
>>> the one available through Ubuntu.
>>>
>>> Guenter
>>>
>>
>> For binutils, no release under Ubuntu, and the Fedora17's is incorrect
>> (can not use), but the binutils-2.22 from gnu is OK.
>>
>> For gcc, no release under Ubuntu, for Fedora17's, gcc-4.9, gcc-4.8,
>> gcc-4.7.4, and gcc-4.4.7 all have bugs for compiling kernel(their bugs
>> are different too).
>>
>> It is really not easy to fix these bugs (gcc guys have too many issues
>> to fix), even if really find the root cause, it is still difficult to
>> fix (fix one bug is very easy to cause another more issues).
>>
>
> I have to wonder ... is this all worth it ? It almost looks like no one
> is using this architecture anymore. Do you have target hardware to test
> any of your changes ?

Chen has to achieve his 10 patches/month quota. :-\

-- 
Thanks,
//richard

Re: [PATCH v2] h8300/kernel/setup.c: add "linux/initrd.h" to pass compiling

[Guenter Roeck]

On 08/30/2013 04:44 AM, richard -rw- weinberger wrote:
> On Fri, Aug 30, 2013 at 1:18 PM, Guenter Roeck <linux@roeck-us.net> wrote:
>> On 08/29/2013 11:34 PM, Chen Gang wrote:
>>>
>>> On 08/30/2013 12:53 PM, Guenter Roeck wrote:
>>>>
>>>> On 08/29/2013 08:59 PM, Chen Gang wrote:
>>>>>
>>>>> The related error (allmodconfig for h8300):
>>>>>
>>>>>      arch/h8300/kernel/setup.c: In function 'setup_arch':
>>>>>      arch/h8300/kernel/setup.c:103:3: error: 'initrd_start' undeclared
>>>>> (first use in this function)
>>>>>         initrd_start = memory_start;
>>>>>         ^
>>>>>      arch/h8300/kernel/setup.c:103:3: note: each undeclared identifier
>>>>> is reported only once for each function it appears in
>>>>>      arch/h8300/kernel/setup.c:104:3: error: 'initrd_end' undeclared
>>>>> (first use in this function)
>>>>>         initrd_end = memory_start += be32_to_cpu(((unsigned long *)
>>>>> (memory_start))[2]);
>>>>>         ^
>>>>>
>>>>> Signed-off-by: Chen Gang <gang.chen@asianux.com>
>>>>> ---
>>>>
>>>>
>>>> Maybe an odd question, but is there a way to actually compile the h8300
>>>> target
>>>
>>>
>>> Firstly, at least for me, I don't think it is an odd question.  :-)
>>>
>>> For the tool-chain:
>>>
>>>     I compiled the cross-compiler from the gcc and binutils source code.
>>>     GCC has too many bugs to compile kernel with -Os (normal make will
>>> fail).
>>>     If without -Os (no optimization), it can work correctly which is enough
>>> for my goal: "let h8300 pass allmodconfig".  ;-)
>>>
>>>>  From building with allmodconfig for h8300:
>>>
>>>
>>>     I can find more chances to provide contributes (both for h8300 and for
>>> others).
>>>     I can learn more in kernel wide.
>>>     I can familiar the gcc and binutils step by step (especially to
>>> familiar with their issues).
>>>
>>> Next:
>>>
>>>     I will communicate/work with the gcc guys for the gcc issues which
>>> found by building kernel.
>>>
>>>     :-)
>>>
>>>
>>>
>>>> in the first place ? The cross compiler on kernel.org doesn't work, nor
>>>> does
>>>> the one available through Ubuntu.
>>>>
>>>> Guenter
>>>>
>>>
>>> For binutils, no release under Ubuntu, and the Fedora17's is incorrect
>>> (can not use), but the binutils-2.22 from gnu is OK.
>>>
>>> For gcc, no release under Ubuntu, for Fedora17's, gcc-4.9, gcc-4.8,
>>> gcc-4.7.4, and gcc-4.4.7 all have bugs for compiling kernel(their bugs
>>> are different too).
>>>
>>> It is really not easy to fix these bugs (gcc guys have too many issues
>>> to fix), even if really find the root cause, it is still difficult to
>>> fix (fix one bug is very easy to cause another more issues).
>>>
>>
>> I have to wonder ... is this all worth it ? It almost looks like no one
>> is using this architecture anymore. Do you have target hardware to test
>> any of your changes ?
>
> Chen has to achieve his 10 patches/month quota. :-\
>

I don't mind that, but there might be more valuable targets to achieve that goal
than a potentially dead architecture.

A more useful change may be to remove the code from the kernel if there is no plan
to fix it (for real, I mean).

Guenter

Re: [PATCH v2] h8300/kernel/setup.c: add "linux/initrd.h" to pass compiling

[Chen Gang]

On 08/30/2013 08:20 PM, Guenter Roeck wrote:
> On 08/30/2013 04:44 AM, richard -rw- weinberger wrote:
>> On Fri, Aug 30, 2013 at 1:18 PM, Guenter Roeck <linux@roeck-us.net>
>> wrote:
>>> On 08/29/2013 11:34 PM, Chen Gang wrote:
>>>>
>>>> On 08/30/2013 12:53 PM, Guenter Roeck wrote:
>>>>>
>>>>> On 08/29/2013 08:59 PM, Chen Gang wrote:
>>>>>>
>>>>>> The related error (allmodconfig for h8300):
>>>>>>
>>>>>>      arch/h8300/kernel/setup.c: In function 'setup_arch':
>>>>>>      arch/h8300/kernel/setup.c:103:3: error: 'initrd_start'
>>>>>> undeclared
>>>>>> (first use in this function)
>>>>>>         initrd_start = memory_start;
>>>>>>         ^
>>>>>>      arch/h8300/kernel/setup.c:103:3: note: each undeclared
>>>>>> identifier
>>>>>> is reported only once for each function it appears in
>>>>>>      arch/h8300/kernel/setup.c:104:3: error: 'initrd_end' undeclared
>>>>>> (first use in this function)
>>>>>>         initrd_end = memory_start += be32_to_cpu(((unsigned long *)
>>>>>> (memory_start))[2]);
>>>>>>         ^
>>>>>>
>>>>>> Signed-off-by: Chen Gang <gang.chen@asianux.com>
>>>>>> ---
>>>>>
>>>>>
>>>>> Maybe an odd question, but is there a way to actually compile the
>>>>> h8300
>>>>> target
>>>>
>>>>
>>>> Firstly, at least for me, I don't think it is an odd question.  :-)
>>>>
>>>> For the tool-chain:
>>>>
>>>>     I compiled the cross-compiler from the gcc and binutils source
>>>> code.
>>>>     GCC has too many bugs to compile kernel with -Os (normal make will
>>>> fail).
>>>>     If without -Os (no optimization), it can work correctly which is
>>>> enough
>>>> for my goal: "let h8300 pass allmodconfig".  ;-)
>>>>
>>>>>  From building with allmodconfig for h8300:
>>>>
>>>>
>>>>     I can find more chances to provide contributes (both for h8300
>>>> and for
>>>> others).
>>>>     I can learn more in kernel wide.
>>>>     I can familiar the gcc and binutils step by step (especially to
>>>> familiar with their issues).
>>>>
>>>> Next:
>>>>
>>>>     I will communicate/work with the gcc guys for the gcc issues which
>>>> found by building kernel.
>>>>
>>>>     :-)
>>>>
>>>>
>>>>
>>>>> in the first place ? The cross compiler on kernel.org doesn't work,
>>>>> nor
>>>>> does
>>>>> the one available through Ubuntu.
>>>>>
>>>>> Guenter
>>>>>
>>>>
>>>> For binutils, no release under Ubuntu, and the Fedora17's is incorrect
>>>> (can not use), but the binutils-2.22 from gnu is OK.
>>>>
>>>> For gcc, no release under Ubuntu, for Fedora17's, gcc-4.9, gcc-4.8,
>>>> gcc-4.7.4, and gcc-4.4.7 all have bugs for compiling kernel(their bugs
>>>> are different too).
>>>>
>>>> It is really not easy to fix these bugs (gcc guys have too many issues
>>>> to fix), even if really find the root cause, it is still difficult to
>>>> fix (fix one bug is very easy to cause another more issues).
>>>>
>>>
>>> I have to wonder ... is this all worth it ? It almost looks like no one
>>> is using this architecture anymore. Do you have target hardware to test
>>> any of your changes ?
>>

No, I have no related hardwares.

Hmm... but I still think it is worth to do:

  1. until now, we do not get confirmation that "h8300 is useless, need remove it from kernel".

  2. it is an architecture which may cause more issues (no MMU, but has IOMAP; support ISA, but not support PCI; has gpio, but not use gpio lib, ...).
     so can have more chances to fine another related sub-systems' issues.

  3. It is worth to scan all architectures one by one, and let all of them pass compiling with allmodconfig, h8300 is part of them.
       

>> Chen has to achieve his 10 patches/month quota. :-\
>>

Yeah, it is a basic requrement to me (which I made by myself, it is not
my company's requirement).

It is a protection goal:

  if I finish them or more, does not mean I am well done (more patches may not mean more contributes).
  if I can not finish them, that means I should improve myself in time.

> 
> I don't mind that, but there might be more valuable targets to achieve
> that goal
> than a potentially dead architecture.
> 

Yeah, in fact, I feel each architecture can archieve that goal (e.g.
with srandconfig, or find issues only by reading source code).

And now I am scanning all architectures by cross-compiling with
allmodconfig, h8300 is just one of them.

:-)


> A more useful change may be to remove the code from the kernel if there
> is no plan
> to fix it (for real, I mean).
> 

Hmm... if we are sure about it, that sounds reasonable to me (but
excuse me, I can not sure about it).

Welcome any other members' suggestions or completions.


> Guenter
> 
> 
> 


Thanks.
-- 
Chen Gang

[PATCH trivial] block/ioctl.c: let code match 'kernel code style'

[Chen Gang]

For 'switch case', remove redundancy '\t' (also can let related lines
within 80 columns) and remove redundancy empty lines, just like other
'switch case' which match 'kernel code style' within the file.

Let blkpg_ioctl() within 80 columns. Let 2nd line of blkdev_ioctl() and
__blkdev_driver_ioctl() align 1st line parameter's start position, just
like blk_ioctl_discard() and blk_ioctl_zeroout() within the file.

For is_unrecognized_ioctl(), can shrink the 'return' statement into one
line (so can save 2 lines), it still matches 'kernel code style' and it
is no conflict with others within the file.


Signed-off-by: Chen Gang <gang.chen@asianux.com>
---
 block/ioctl.c |  216 ++++++++++++++++++++++++++++-----------------------------
 1 files changed, 105 insertions(+), 111 deletions(-)

diff --git a/block/ioctl.c b/block/ioctl.c
index a31d91d..4a0be2d 100644
--- a/block/ioctl.c
+++ b/block/ioctl.c
@@ -9,7 +9,8 @@
 #include <linux/blktrace_api.h>
 #include <asm/uaccess.h>
 
-static int blkpg_ioctl(struct block_device *bdev, struct blkpg_ioctl_arg __user *arg)
+static int blkpg_ioctl(struct block_device *bdev,
+		       struct blkpg_ioctl_arg __user *arg)
 {
 	struct block_device *bdevp;
 	struct gendisk *disk;
@@ -32,121 +33,119 @@ static int blkpg_ioctl(struct block_device *bdev, struct blkpg_ioctl_arg __user
 	partno = p.pno;
 	if (partno <= 0)
 		return -EINVAL;
-	switch (a.op) {
-		case BLKPG_ADD_PARTITION:
-			start = p.start >> 9;
-			length = p.length >> 9;
-			/* check for fit in a hd_struct */
-			if (sizeof(sector_t) == sizeof(long) &&
-			    sizeof(long long) > sizeof(long)) {
-				long pstart = start, plength = length;
-				if (pstart != start || plength != length
-				    || pstart < 0 || plength < 0 || partno > 65535)
-					return -EINVAL;
-			}
-
-			mutex_lock(&bdev->bd_mutex);
-
-			/* overlap? */
-			disk_part_iter_init(&piter, disk,
-					    DISK_PITER_INCL_EMPTY);
-			while ((part = disk_part_iter_next(&piter))) {
-				if (!(start + length <= part->start_sect ||
-				      start >= part->start_sect + part->nr_sects)) {
-					disk_part_iter_exit(&piter);
-					mutex_unlock(&bdev->bd_mutex);
-					return -EBUSY;
-				}
-			}
-			disk_part_iter_exit(&piter);
 
-			/* all seems OK */
-			part = add_partition(disk, partno, start, length,
-					     ADDPART_FLAG_NONE, NULL);
-			mutex_unlock(&bdev->bd_mutex);
-			return IS_ERR(part) ? PTR_ERR(part) : 0;
-		case BLKPG_DEL_PARTITION:
-			part = disk_get_part(disk, partno);
-			if (!part)
-				return -ENXIO;
+	switch (a.op) {
+	case BLKPG_ADD_PARTITION:
+		start = p.start >> 9;
+		length = p.length >> 9;
+		/* check for fit in a hd_struct */
+		if (sizeof(sector_t) == sizeof(long) &&
+		    sizeof(long long) > sizeof(long)) {
+			long pstart = start, plength = length;
+			if (pstart != start || plength != length
+			    || pstart < 0 || plength < 0 || partno > 65535)
+				return -EINVAL;
+		}
 
-			bdevp = bdget(part_devt(part));
-			disk_put_part(part);
-			if (!bdevp)
-				return -ENOMEM;
+		mutex_lock(&bdev->bd_mutex);
 
-			mutex_lock(&bdevp->bd_mutex);
-			if (bdevp->bd_openers) {
-				mutex_unlock(&bdevp->bd_mutex);
-				bdput(bdevp);
+		/* overlap? */
+		disk_part_iter_init(&piter, disk, DISK_PITER_INCL_EMPTY);
+		while ((part = disk_part_iter_next(&piter))) {
+			if (!(start + length <= part->start_sect ||
+			      start >= part->start_sect + part->nr_sects)) {
+				disk_part_iter_exit(&piter);
+				mutex_unlock(&bdev->bd_mutex);
 				return -EBUSY;
 			}
-			/* all seems OK */
-			fsync_bdev(bdevp);
-			invalidate_bdev(bdevp);
-
-			mutex_lock_nested(&bdev->bd_mutex, 1);
-			delete_partition(disk, partno);
-			mutex_unlock(&bdev->bd_mutex);
+		}
+		disk_part_iter_exit(&piter);
+
+		/* all seems OK */
+		part = add_partition(disk, partno, start, length,
+				     ADDPART_FLAG_NONE, NULL);
+		mutex_unlock(&bdev->bd_mutex);
+		return IS_ERR(part) ? PTR_ERR(part) : 0;
+	case BLKPG_DEL_PARTITION:
+		part = disk_get_part(disk, partno);
+		if (!part)
+			return -ENXIO;
+
+		bdevp = bdget(part_devt(part));
+		disk_put_part(part);
+		if (!bdevp)
+			return -ENOMEM;
+
+		mutex_lock(&bdevp->bd_mutex);
+		if (bdevp->bd_openers) {
 			mutex_unlock(&bdevp->bd_mutex);
 			bdput(bdevp);
-
-			return 0;
-		case BLKPG_RESIZE_PARTITION:
-			start = p.start >> 9;
-			/* new length of partition in bytes */
-			length = p.length >> 9;
-			/* check for fit in a hd_struct */
-			if (sizeof(sector_t) == sizeof(long) &&
-			    sizeof(long long) > sizeof(long)) {
-				long pstart = start, plength = length;
-				if (pstart != start || plength != length
-				    || pstart < 0 || plength < 0)
-					return -EINVAL;
-			}
-			part = disk_get_part(disk, partno);
-			if (!part)
-				return -ENXIO;
-			bdevp = bdget(part_devt(part));
-			if (!bdevp) {
-				disk_put_part(part);
-				return -ENOMEM;
-			}
-			mutex_lock(&bdevp->bd_mutex);
-			mutex_lock_nested(&bdev->bd_mutex, 1);
-			if (start != part->start_sect) {
-				mutex_unlock(&bdevp->bd_mutex);
-				mutex_unlock(&bdev->bd_mutex);
-				bdput(bdevp);
-				disk_put_part(part);
+			return -EBUSY;
+		}
+		/* all seems OK */
+		fsync_bdev(bdevp);
+		invalidate_bdev(bdevp);
+
+		mutex_lock_nested(&bdev->bd_mutex, 1);
+		delete_partition(disk, partno);
+		mutex_unlock(&bdev->bd_mutex);
+		mutex_unlock(&bdevp->bd_mutex);
+		bdput(bdevp);
+		return 0;
+	case BLKPG_RESIZE_PARTITION:
+		start = p.start >> 9;
+		/* new length of partition in bytes */
+		length = p.length >> 9;
+		/* check for fit in a hd_struct */
+		if (sizeof(sector_t) == sizeof(long) &&
+		    sizeof(long long) > sizeof(long)) {
+			long pstart = start, plength = length;
+			if (pstart != start || plength != length
+			    || pstart < 0 || plength < 0)
 				return -EINVAL;
-			}
-			/* overlap? */
-			disk_part_iter_init(&piter, disk,
-					    DISK_PITER_INCL_EMPTY);
-			while ((lpart = disk_part_iter_next(&piter))) {
-				if (lpart->partno != partno &&
-				   !(start + length <= lpart->start_sect ||
-				   start >= lpart->start_sect + lpart->nr_sects)
-				   ) {
-					disk_part_iter_exit(&piter);
-					mutex_unlock(&bdevp->bd_mutex);
-					mutex_unlock(&bdev->bd_mutex);
-					bdput(bdevp);
-					disk_put_part(part);
-					return -EBUSY;
-				}
-			}
-			disk_part_iter_exit(&piter);
-			part_nr_sects_write(part, (sector_t)length);
-			i_size_write(bdevp->bd_inode, p.length);
+		}
+		part = disk_get_part(disk, partno);
+		if (!part)
+			return -ENXIO;
+		bdevp = bdget(part_devt(part));
+		if (!bdevp) {
+			disk_put_part(part);
+			return -ENOMEM;
+		}
+		mutex_lock(&bdevp->bd_mutex);
+		mutex_lock_nested(&bdev->bd_mutex, 1);
+		if (start != part->start_sect) {
 			mutex_unlock(&bdevp->bd_mutex);
 			mutex_unlock(&bdev->bd_mutex);
 			bdput(bdevp);
 			disk_put_part(part);
-			return 0;
-		default:
 			return -EINVAL;
+		}
+		/* overlap? */
+		disk_part_iter_init(&piter, disk, DISK_PITER_INCL_EMPTY);
+		while ((lpart = disk_part_iter_next(&piter))) {
+			if (lpart->partno != partno &&
+			   !(start + length <= lpart->start_sect ||
+			   start >= lpart->start_sect + lpart->nr_sects)
+			   ) {
+				disk_part_iter_exit(&piter);
+				mutex_unlock(&bdevp->bd_mutex);
+				mutex_unlock(&bdev->bd_mutex);
+				bdput(bdevp);
+				disk_put_part(part);
+				return -EBUSY;
+			}
+		}
+		disk_part_iter_exit(&piter);
+		part_nr_sects_write(part, (sector_t)length);
+		i_size_write(bdevp->bd_inode, p.length);
+		mutex_unlock(&bdevp->bd_mutex);
+		mutex_unlock(&bdev->bd_mutex);
+		bdput(bdevp);
+		disk_put_part(part);
+		return 0;
+	default:
+		return -EINVAL;
 	}
 }
 
@@ -232,7 +231,7 @@ static int put_u64(unsigned long arg, u64 val)
 }
 
 int __blkdev_driver_ioctl(struct block_device *bdev, fmode_t mode,
-			unsigned cmd, unsigned long arg)
+			  unsigned cmd, unsigned long arg)
 {
 	struct gendisk *disk = bdev->bd_disk;
 
@@ -263,16 +262,14 @@ EXPORT_SYMBOL_GPL(__blkdev_driver_ioctl);
  */
 static inline int is_unrecognized_ioctl(int ret)
 {
-	return	ret == -EINVAL ||
-		ret == -ENOTTY ||
-		ret == -ENOIOCTLCMD;
+	return	ret == -EINVAL || ret == -ENOTTY || ret == -ENOIOCTLCMD;
 }
 
 /*
  * always keep this in sync with compat_blkdev_ioctl()
  */
 int blkdev_ioctl(struct block_device *bdev, fmode_t mode, unsigned cmd,
-			unsigned long arg)
+		 unsigned long arg)
 {
 	struct gendisk *disk = bdev->bd_disk;
 	struct backing_dev_info *bdi;
@@ -291,7 +288,6 @@ int blkdev_ioctl(struct block_device *bdev, fmode_t mode, unsigned cmd,
 		fsync_bdev(bdev);
 		invalidate_bdev(bdev);
 		return 0;
-
 	case BLKROSET:
 		ret = __blkdev_driver_ioctl(bdev, mode, cmd, arg);
 		if (!is_unrecognized_ioctl(ret))
@@ -302,7 +298,6 @@ int blkdev_ioctl(struct block_device *bdev, fmode_t mode, unsigned cmd,
 			return -EFAULT;
 		set_device_ro(bdev, n);
 		return 0;
-
 	case BLKDISCARD:
 	case BLKSECDISCARD: {
 		uint64_t range[2];
@@ -327,7 +322,6 @@ int blkdev_ioctl(struct block_device *bdev, fmode_t mode, unsigned cmd,
 
 		return blk_ioctl_zeroout(bdev, range[0], range[1]);
 	}
-
 	case HDIO_GETGEO: {
 		struct hd_geometry geo;
 
-- 
1.7.7.6

Re: [PATCH trivial] block/ioctl.c: let code match 'kernel code style'

[Li Zefan]

Please don't. Pure colding style cleanup is discouraged.

You're not going to run checkpatch.pl on the whole kernel tree and fix
all the complaints, are you?

On 2013/9/3 16:29, Chen Gang wrote:
> For 'switch case', remove redundancy '\t' (also can let related lines
> within 80 columns) and remove redundancy empty lines, just like other
> 'switch case' which match 'kernel code style' within the file.
> 
> Let blkpg_ioctl() within 80 columns. Let 2nd line of blkdev_ioctl() and
> __blkdev_driver_ioctl() align 1st line parameter's start position, just
> like blk_ioctl_discard() and blk_ioctl_zeroout() within the file.
> 
> For is_unrecognized_ioctl(), can shrink the 'return' statement into one
> line (so can save 2 lines), it still matches 'kernel code style' and it
> is no conflict with others within the file.
> 
> 
> Signed-off-by: Chen Gang <gang.chen@asianux.com>
> ---
>  block/ioctl.c |  216 ++++++++++++++++++++++++++++-----------------------------
>  1 files changed, 105 insertions(+), 111 deletions(-)

Re: [PATCH trivial] block/ioctl.c: let code match 'kernel code style'

[Chen Gang]

On 09/03/2013 04:55 PM, Li Zefan wrote:
> Please don't. Pure colding style cleanup is discouraged.
> 

Pardon?

Do you mean "coding style", not "colding style"? (or it is my
misunderstanding?).


> You're not going to run checkpatch.pl on the whole kernel tree and fix
> all the complaints, are you?
> 

I am not going to, it seems that may be the 'job' of Joe and
trivial@kernel.org?


BTW: it seems you did not redirect my mails to "/dev/null".  ;-)

Thanks.

> On 2013/9/3 16:29, Chen Gang wrote:
>> For 'switch case', remove redundancy '\t' (also can let related lines
>> within 80 columns) and remove redundancy empty lines, just like other
>> 'switch case' which match 'kernel code style' within the file.
>>
>> Let blkpg_ioctl() within 80 columns. Let 2nd line of blkdev_ioctl() and
>> __blkdev_driver_ioctl() align 1st line parameter's start position, just
>> like blk_ioctl_discard() and blk_ioctl_zeroout() within the file.
>>
>> For is_unrecognized_ioctl(), can shrink the 'return' statement into one
>> line (so can save 2 lines), it still matches 'kernel code style' and it
>> is no conflict with others within the file.
>>
>>
>> Signed-off-by: Chen Gang <gang.chen@asianux.com>
>> ---
>>  block/ioctl.c |  216 ++++++++++++++++++++++++++++-----------------------------
>>  1 files changed, 105 insertions(+), 111 deletions(-)
> 
> 
> 


-- 
Chen Gang

Re: [PATCH trivial] block/ioctl.c: let code match 'kernel code style'

[Jiri Kosina]

On Tue, 3 Sep 2013, Chen Gang wrote:

> Do you mean "coding style", not "colding style"? (or it is my
> misunderstanding?).
> 
> 
> > You're not going to run checkpatch.pl on the whole kernel tree and fix
> > all the complaints, are you?
> > 
> 
> I am not going to, it seems that may be the 'job' of Joe and
> trivial@kernel.org?

FWIW, I am not generally accepting pure code reformatting patches.

-- 
Jiri Kosina
SUSE Labs

Re: [PATCH trivial] block/ioctl.c: let code match 'kernel code style'

[Chen Gang]

On 09/03/2013 05:27 PM, Jiri Kosina wrote:
> On Tue, 3 Sep 2013, Chen Gang wrote:
> 
>> Do you mean "coding style", not "colding style"? (or it is my
>> misunderstanding?).
>>
>>
>>> You're not going to run checkpatch.pl on the whole kernel tree and fix
>>> all the complaints, are you?
>>>
>>
>> I am not going to, it seems that may be the 'job' of Joe and
>> trivial@kernel.org?
> 
> FWIW, I am not generally accepting pure code reformatting patches.
> 

Yes, it is really not quite efficient to only accept pure code
reformatting patches (it must contain another 'things').
;-)


Thanks.
-- 
Chen Gang

Re: [PATCH trivial] block/ioctl.c: let code match 'kernel code style'

[Chen Gang]

It seems necessary to explain my goal (why I intended to send this patch).

1. support the 'work' of Joe and trivial@kernel.org.

   in my opinion, 'trivial'  means none-urgent, but it also means important.
     (if both none-urgent and none-important, why need it?)

   so I intend to send 1 or 2 trivial patches per month to express my opinion and support their 'work'.

2. I have planned to provide some contributes to block sub-system in 2nd half of 2013.

   I have mentioned about it in my some original discussions (if necessary, I will provide the proof).
   but at least now, I have done nothing for block sub system.
   it is time to do something for it.

3. avoid the misunderstanding of the other related discussion in kernel mailing list.

   the discussion is about whether need "#ifdef" for a patch.
   maybe some members (e.g. Guenter) misunderstand me: "I do not care about the code style".
     (it is only my 'guess', may he does not misunderstand, and this patch wants to make it clear).


BTW:

  at least for me, I don't think "more patches is more contributions".
  but it is true that I am trying to send more patches and get more discussions in kernel mailing list.
  and I am (should be) also trying to avoid send waste mails to kernel mailing list.


Welcome any additional suggestions or completions.

Thanks.

On 09/03/2013 05:06 PM, Chen Gang wrote:
> On 09/03/2013 04:55 PM, Li Zefan wrote:
>> Please don't. Pure colding style cleanup is discouraged.
>>
> 
> Pardon?
> 
> Do you mean "coding style", not "colding style"? (or it is my
> misunderstanding?).
> 
> 
>> You're not going to run checkpatch.pl on the whole kernel tree and fix
>> all the complaints, are you?
>>
> 
> I am not going to, it seems that may be the 'job' of Joe and
> trivial@kernel.org?
> 
> 
> BTW: it seems you did not redirect my mails to "/dev/null".  ;-)
> 
> Thanks.
> 
>> On 2013/9/3 16:29, Chen Gang wrote:
>>> For 'switch case', remove redundancy '\t' (also can let related lines
>>> within 80 columns) and remove redundancy empty lines, just like other
>>> 'switch case' which match 'kernel code style' within the file.
>>>
>>> Let blkpg_ioctl() within 80 columns. Let 2nd line of blkdev_ioctl() and
>>> __blkdev_driver_ioctl() align 1st line parameter's start position, just
>>> like blk_ioctl_discard() and blk_ioctl_zeroout() within the file.
>>>
>>> For is_unrecognized_ioctl(), can shrink the 'return' statement into one
>>> line (so can save 2 lines), it still matches 'kernel code style' and it
>>> is no conflict with others within the file.
>>>
>>>
>>> Signed-off-by: Chen Gang <gang.chen@asianux.com>
>>> ---
>>>  block/ioctl.c |  216 ++++++++++++++++++++++++++++-----------------------------
>>>  1 files changed, 105 insertions(+), 111 deletions(-)
>>
>>
>>
> 
> 


-- 
Chen Gang