[PULL REQUEST] powerpc generic command line

[PULL REQUEST] powerpc generic command line

[Daniel Walker]

Here are the generic command line changes for powerpc. 

These changes have been in linux-next for two cycles, with few problems reported.
It's also been used at Cisco Systems, Inc. in production products for many many
years with no problems.

Please pull these changes.


The following changes since commit ccda4af0f4b92f7b4c308d3acc262f4a7e3affad:

  Linux 4.20-rc2 (2018-11-11 17:12:31 -0600)

are available in the git repository at:

  https://github.com/daniel-walker/cisco-linux.git for-powerpc

for you to fetch changes up to 5d4514a9c291ecf19b0626695161673d35e5d549:

  powerpc: convert config files to generic cmdline (2018-11-16 07:32:26 -0800)

----------------------------------------------------------------
Daniel Walker (3):
      add generic builtin command line
      powerpc: convert to generic builtin command line
      powerpc: convert config files to generic cmdline

 arch/powerpc/Kconfig                          | 23 +--------
 arch/powerpc/configs/44x/fsp2_defconfig       | 29 ++++++-----
 arch/powerpc/configs/44x/iss476-smp_defconfig | 24 ++++-----
 arch/powerpc/configs/44x/warp_defconfig       | 12 ++---
 arch/powerpc/configs/holly_defconfig          | 12 ++---
 arch/powerpc/configs/mvme5100_defconfig       | 25 +++++-----
 arch/powerpc/configs/skiroot_defconfig        | 48 +++++++++---------
 arch/powerpc/configs/storcenter_defconfig     | 15 +++---
 arch/powerpc/kernel/prom.c                    |  4 ++
 arch/powerpc/kernel/prom_init.c               |  8 +--
 arch/powerpc/kernel/prom_init_check.sh        |  2 +-
 include/linux/cmdline.h                       | 72 +++++++++++++++++++++++++++
 init/Kconfig                                  | 69 +++++++++++++++++++++++++
 13 files changed, 231 insertions(+), 112 deletions(-)
 create mode 100644 include/linux/cmdline.h

Re: [PULL REQUEST] powerpc generic command line

[Christophe Leroy]

Le 01/03/2019 à 20:44, Daniel Walker a écrit :
> Here are the generic command line changes for powerpc.
> 
> These changes have been in linux-next for two cycles, with few problems reported.
> It's also been used at Cisco Systems, Inc. in production products for many many
> years with no problems.
> 
> Please pull these changes.
> 
> 
> The following changes since commit ccda4af0f4b92f7b4c308d3acc262f4a7e3affad:
> 
>    Linux 4.20-rc2 (2018-11-11 17:12:31 -0600)
> 
> are available in the git repository at:
> 
>    https://github.com/daniel-walker/cisco-linux.git for-powerpc
> 
> for you to fetch changes up to 5d4514a9c291ecf19b0626695161673d35e5d549:
> 
>    powerpc: convert config files to generic cmdline (2018-11-16 07:32:26 -0800)
> 
> ----------------------------------------------------------------
> Daniel Walker (3):
>        add generic builtin command line
>        powerpc: convert to generic builtin command line
>        powerpc: convert config files to generic cmdline

Hello,

This series is in total contradiction with the work being done to add 
KASAN support to powerpc.

It also modifies the behaviour for powerpc.

Please do not apply this series as is, see my comments on the individual 
patchs for details.

Thanks
Christophe

> 
>   arch/powerpc/Kconfig                          | 23 +--------
>   arch/powerpc/configs/44x/fsp2_defconfig       | 29 ++++++-----
>   arch/powerpc/configs/44x/iss476-smp_defconfig | 24 ++++-----
>   arch/powerpc/configs/44x/warp_defconfig       | 12 ++---
>   arch/powerpc/configs/holly_defconfig          | 12 ++---
>   arch/powerpc/configs/mvme5100_defconfig       | 25 +++++-----
>   arch/powerpc/configs/skiroot_defconfig        | 48 +++++++++---------
>   arch/powerpc/configs/storcenter_defconfig     | 15 +++---
>   arch/powerpc/kernel/prom.c                    |  4 ++
>   arch/powerpc/kernel/prom_init.c               |  8 +--
>   arch/powerpc/kernel/prom_init_check.sh        |  2 +-
>   include/linux/cmdline.h                       | 72 +++++++++++++++++++++++++++
>   init/Kconfig                                  | 69 +++++++++++++++++++++++++
>   13 files changed, 231 insertions(+), 112 deletions(-)
>   create mode 100644 include/linux/cmdline.h
> 

Re: [PULL REQUEST] powerpc generic command line

[Daniel Walker]

On Mon, Mar 04, 2019 at 02:55:08PM +0100, Christophe Leroy wrote:
> 
> 
> Le 01/03/2019 à 20:44, Daniel Walker a écrit :
> > Here are the generic command line changes for powerpc.
> > 
> > These changes have been in linux-next for two cycles, with few problems reported.
> > It's also been used at Cisco Systems, Inc. in production products for many many
> > years with no problems.
> > 
> > Please pull these changes.
> > 
> > 
> > The following changes since commit ccda4af0f4b92f7b4c308d3acc262f4a7e3affad:
> > 
> >    Linux 4.20-rc2 (2018-11-11 17:12:31 -0600)
> > 
> > are available in the git repository at:
> > 
> >    https://github.com/daniel-walker/cisco-linux.git for-powerpc
> > 
> > for you to fetch changes up to 5d4514a9c291ecf19b0626695161673d35e5d549:
> > 
> >    powerpc: convert config files to generic cmdline (2018-11-16 07:32:26 -0800)
> > 
> > ----------------------------------------------------------------
> > Daniel Walker (3):
> >        add generic builtin command line
> >        powerpc: convert to generic builtin command line
> >        powerpc: convert config files to generic cmdline
> 
> Hello,
> 
> This series is in total contradiction with the work being done to add KASAN
> support to powerpc.
> 
> It also modifies the behaviour for powerpc.
> 
> Please do not apply this series as is, see my comments on the individual
> patchs for details.
> 

Not trying to offend you, but you comments seems overly alarmist. KASAN is a
debug feature, generally we don't write code around debug features (especially
ones which aren't merged yet). It would not be hard to correct our use of string
functions when your KASAN enablement is merged, I'm sure you could do it, but I would be happy
to do it also. The other comments you had I don't think rise to the level of
stopping a pull request. Our code is stabilized, so I'm not going to
re-design it at a late date like this.

I think the pull request is still valid.

Daniel

Re: [PULL REQUEST] powerpc generic command line

[Christophe Leroy]

Le 04/03/2019 à 17:57, Daniel Walker a écrit :
> On Mon, Mar 04, 2019 at 02:55:08PM +0100, Christophe Leroy wrote:
>>
>>
>> Le 01/03/2019 à 20:44, Daniel Walker a écrit :
>>> Here are the generic command line changes for powerpc.
>>>
>>> These changes have been in linux-next for two cycles, with few problems reported.
>>> It's also been used at Cisco Systems, Inc. in production products for many many
>>> years with no problems.
>>>
>>> Please pull these changes.
>>>
>>>
>>> The following changes since commit ccda4af0f4b92f7b4c308d3acc262f4a7e3affad:
>>>
>>>     Linux 4.20-rc2 (2018-11-11 17:12:31 -0600)
>>>
>>> are available in the git repository at:
>>>
>>>     https://github.com/daniel-walker/cisco-linux.git for-powerpc
>>>
>>> for you to fetch changes up to 5d4514a9c291ecf19b0626695161673d35e5d549:
>>>
>>>     powerpc: convert config files to generic cmdline (2018-11-16 07:32:26 -0800)
>>>
>>> ----------------------------------------------------------------
>>> Daniel Walker (3):
>>>         add generic builtin command line
>>>         powerpc: convert to generic builtin command line
>>>         powerpc: convert config files to generic cmdline
>>
>> Hello,
>>
>> This series is in total contradiction with the work being done to add KASAN
>> support to powerpc.
>>
>> It also modifies the behaviour for powerpc.
>>
>> Please do not apply this series as is, see my comments on the individual
>> patchs for details.
>>
> 
> Not trying to offend you, but you comments seems overly alarmist. KASAN is a
> debug feature, generally we don't write code around debug features (especially
> ones which aren't merged yet). It would not be hard to correct our use of string
> functions when your KASAN enablement is merged, I'm sure you could do it, but I would be happy
> to do it also. The other comments you had I don't think rise to the level of
> stopping a pull request. Our code is stabilized, so I'm not going to
> re-design it at a late date like this.
> 
> I think the pull request is still valid.
> 

Ok, lets the KASAN stuff aside. And I agree I misread the patch when I 
felt that it was changing the behaviour in prom_init.c

I don't want to criticise your work either, but your series seems 
overkill. Anyway, could you explain your approach and what is the 
benefit of your series compared to what is already existing ?

Can you also explain how your changes fit with the what is done is the 
function early_init_dt_scan_chosen_ppc() in drivers/of/fdt.c as your 
series doesn't modify it ? (extract below)



	/*
	 * CONFIG_CMDLINE is meant to be a default in case nothing else
	 * managed to set the command line, unless CONFIG_CMDLINE_FORCE
	 * is set in which case we override whatever was found earlier.
	 */
#ifdef CONFIG_CMDLINE
#if defined(CONFIG_CMDLINE_EXTEND)
	strlcat(data, " ", COMMAND_LINE_SIZE);
	strlcat(data, CONFIG_CMDLINE, COMMAND_LINE_SIZE);
#elif defined(CONFIG_CMDLINE_FORCE)
	strlcpy(data, CONFIG_CMDLINE, COMMAND_LINE_SIZE);
#else
	/* No arguments from boot loader, use kernel's  cmdl*/
	if (!((char *)data)[0])
		strlcpy(data, CONFIG_CMDLINE, COMMAND_LINE_SIZE);
#endif
#endif /* CONFIG_CMDLINE */




What I like in your series is that you make the CMDLINE config common to 
all arches. But my feeling is that the 40 or so lines of code in your 
cmdline.h is way complex compared to what it aims to do, ie replacing a 
few lines on code in two places. But I might not have the complete 
picture so feel free to tell all the details behind it. I see you 
already submitted part of your series to the ppc list in Novembre 
(https://patchwork.ozlabs.org/project/linuxppc-dev/list/?series=75078) 
unfortunatly the first patch of the series was not there, and it seems 
at that time your series has not generated any further discussion.

Thanks
Christophe

Re: [PULL REQUEST] powerpc generic command line

[Daniel Walker]

On Mon, Mar 04, 2019 at 06:29:12PM +0100, Christophe Leroy wrote:
> 
> 
> Le 04/03/2019 à 17:57, Daniel Walker a écrit :
> > On Mon, Mar 04, 2019 at 02:55:08PM +0100, Christophe Leroy wrote:
> > > 
> > > 
> > > Le 01/03/2019 à 20:44, Daniel Walker a écrit :
> > > > Here are the generic command line changes for powerpc.
> > > > 
> > > > These changes have been in linux-next for two cycles, with few problems reported.
> > > > It's also been used at Cisco Systems, Inc. in production products for many many
> > > > years with no problems.
> > > > 
> > > > Please pull these changes.
> > > > 
> > > > 
> > > > The following changes since commit ccda4af0f4b92f7b4c308d3acc262f4a7e3affad:
> > > > 
> > > >     Linux 4.20-rc2 (2018-11-11 17:12:31 -0600)
> > > > 
> > > > are available in the git repository at:
> > > > 
> > > >     https://github.com/daniel-walker/cisco-linux.git for-powerpc
> > > > 
> > > > for you to fetch changes up to 5d4514a9c291ecf19b0626695161673d35e5d549:
> > > > 
> > > >     powerpc: convert config files to generic cmdline (2018-11-16 07:32:26 -0800)
> > > > 
> > > > ----------------------------------------------------------------
> > > > Daniel Walker (3):
> > > >         add generic builtin command line
> > > >         powerpc: convert to generic builtin command line
> > > >         powerpc: convert config files to generic cmdline
> > > 
> > > Hello,
> > > 
> > > This series is in total contradiction with the work being done to add KASAN
> > > support to powerpc.
> > > 
> > > It also modifies the behaviour for powerpc.
> > > 
> > > Please do not apply this series as is, see my comments on the individual
> > > patchs for details.
> > > 
> > 
> > Not trying to offend you, but you comments seems overly alarmist. KASAN is a
> > debug feature, generally we don't write code around debug features (especially
> > ones which aren't merged yet). It would not be hard to correct our use of string
> > functions when your KASAN enablement is merged, I'm sure you could do it, but I would be happy
> > to do it also. The other comments you had I don't think rise to the level of
> > stopping a pull request. Our code is stabilized, so I'm not going to
> > re-design it at a late date like this.
> > 
> > I think the pull request is still valid.
> > 
> 
> Ok, lets the KASAN stuff aside. And I agree I misread the patch when I felt
> that it was changing the behaviour in prom_init.c
> 
> I don't want to criticise your work either, but your series seems overkill.
> Anyway, could you explain your approach and what is the benefit of your
> series compared to what is already existing ?

Current behavior varies across different architectures. Some platforms prepend
the command line with the boot loader arguments some platforms append it. Cisco
uses all these platforms. We desire a method to either append or prepend on a
number of architectures. It reduces code duplication across multiple
architectures. It allows a number of other command line related features to be
added in a single location instead of across all architectures. For example MIPS
has some additional features added related to the command line arguments, this
could be made generic and available to all architectures.

> 
> Can you also explain how your changes fit with the what is done is the
> function early_init_dt_scan_chosen_ppc() in drivers/of/fdt.c as your series
> doesn't modify it ? (extract below)
> 
> 
> 
> 	/*
> 	 * CONFIG_CMDLINE is meant to be a default in case nothing else
> 	 * managed to set the command line, unless CONFIG_CMDLINE_FORCE
> 	 * is set in which case we override whatever was found earlier.
> 	 */
> #ifdef CONFIG_CMDLINE
> #if defined(CONFIG_CMDLINE_EXTEND)
> 	strlcat(data, " ", COMMAND_LINE_SIZE);
> 	strlcat(data, CONFIG_CMDLINE, COMMAND_LINE_SIZE);
> #elif defined(CONFIG_CMDLINE_FORCE)
> 	strlcpy(data, CONFIG_CMDLINE, COMMAND_LINE_SIZE);
> #else
> 	/* No arguments from boot loader, use kernel's  cmdl*/
> 	if (!((char *)data)[0])
> 		strlcpy(data, CONFIG_CMDLINE, COMMAND_LINE_SIZE);
> #endif
> #endif /* CONFIG_CMDLINE */
 
Our code supersedes this code, and you can see the code is doing similar things.
Eventually this block will be replaced with a call into generic functions. We've
submitted changes to this code in the past,

https://lore.kernel.org/patchwork/patch/604997/

However, this code doesn't lend itself to piecemeal conversion to a generic
system. It's a generic system itself, but only for architectures/platforms with specific
characteristics. This is why we're doing it one at a time with PowerPC first.

> 
> 
> 
> What I like in your series is that you make the CMDLINE config common to all
> arches. But my feeling is that the 40 or so lines of code in your cmdline.h
> is way complex compared to what it aims to do, ie replacing a few lines on
> code in two places. But I might not have the complete picture so feel free
> to tell all the details behind it. I see you already submitted part of your
> series to the ppc list in Novembre
> (https://patchwork.ozlabs.org/project/linuxppc-dev/list/?series=75078)
> unfortunatly the first patch of the series was not there, and it seems at
> that time your series has not generated any further discussion.

Our code has been out there for years. I don't recall our first submission but
it was prior to last year.

Daniel

Re: [PULL REQUEST] powerpc generic command line

[Michael Ellerman]

Hi Daniel,

Daniel Walker <danielwa@cisco.com> writes:
> Here are the generic command line changes for powerpc. 
>
> These changes have been in linux-next for two cycles, with few problems reported.
> It's also been used at Cisco Systems, Inc. in production products for many many
> years with no problems.
>
> Please pull these changes.

Sorry I didn't reply to this earlier, have been busy with merge window
bugs and so on.

As I imagine you noticed, I didn't pull this. There are a few reasons.

Firstly you sent it a bit late, about a day before the 5.0 release, and
at 6am Saturday my time :) In future if you want me to merge something
please send a pull at least the ~Wednesday before the release.
 
Secondly I had no idea this code was even in linux-next. I'm not sure if
I was Cc'ed at some point when you added it, if so sorry I missed it,
but I get lots of email. If you're going to add changes to arch/powerpc
in your next tree I'd appreciate some notice, or preferably an explicit
ack.

The main reason I didn't merge it is that it's adding a bunch of code
outside of arch/powerpc, into files which I'm not the maintainer for,
and the patches doing so have no acks or reviews from anyone.

It's also adding a generic implementation with no indication that any
other arches are willing/able to use the generic implementation, which
begs the question whether it will actually used.

I appreciate it's hard to get these sort of cross architecture changes
into mainline, but I don't think this is the way to do it.

I'd suggest you post a patch series to linux-arch with the generic
changes and as many architecture conversions as you can manage, then get
some review/acks for the generic changes and chase arch maintainers for
some acks.

I realise you have posted the series before, it may require some
persistence. There were also quite a few comments from Christophe, so
replying to those would be a good place to start.

> The following changes since commit ccda4af0f4b92f7b4c308d3acc262f4a7e3affad:
>
>   Linux 4.20-rc2 (2018-11-11 17:12:31 -0600)
>
> are available in the git repository at:
>
>   https://github.com/daniel-walker/cisco-linux.git for-powerpc
>
> for you to fetch changes up to 5d4514a9c291ecf19b0626695161673d35e5d549:
>
>   powerpc: convert config files to generic cmdline (2018-11-16 07:32:26 -0800)
>
> ----------------------------------------------------------------
> Daniel Walker (3):
>       add generic builtin command line
>       powerpc: convert to generic builtin command line
>       powerpc: convert config files to generic cmdline
>
>  arch/powerpc/Kconfig                          | 23 +--------
>  arch/powerpc/configs/44x/fsp2_defconfig       | 29 ++++++-----
>  arch/powerpc/configs/44x/iss476-smp_defconfig | 24 ++++-----
>  arch/powerpc/configs/44x/warp_defconfig       | 12 ++---
>  arch/powerpc/configs/holly_defconfig          | 12 ++---
>  arch/powerpc/configs/mvme5100_defconfig       | 25 +++++-----
>  arch/powerpc/configs/skiroot_defconfig        | 48 +++++++++---------
>  arch/powerpc/configs/storcenter_defconfig     | 15 +++---

Also if you're updating defconfigs please don't include any unrelated
changes. Trimming the defconfigs can silently drop symbols and break
people's setups so needs to be done carefully.

It's safer to just sed the defconfig files directly, rather than running
savedefconfig on them.

cheers

>  arch/powerpc/kernel/prom.c                    |  4 ++
>  arch/powerpc/kernel/prom_init.c               |  8 +--
>  arch/powerpc/kernel/prom_init_check.sh        |  2 +-
>  include/linux/cmdline.h                       | 72 +++++++++++++++++++++++++++
>  init/Kconfig                                  | 69 +++++++++++++++++++++++++
>  13 files changed, 231 insertions(+), 112 deletions(-)
>  create mode 100644 include/linux/cmdline.h

Re: [PULL REQUEST] powerpc generic command line

[Daniel Walker]

On Tue, Mar 19, 2019 at 12:18:03PM +1100, Michael Ellerman wrote:
> Hi Daniel,
> 
> Daniel Walker <danielwa@cisco.com> writes:
> > Here are the generic command line changes for powerpc. 
> >
> > These changes have been in linux-next for two cycles, with few problems reported.
> > It's also been used at Cisco Systems, Inc. in production products for many many
> > years with no problems.
> >
> > Please pull these changes.
> 
> Sorry I didn't reply to this earlier, have been busy with merge window
> bugs and so on.
> 
> As I imagine you noticed, I didn't pull this. There are a few reasons.
> 
> Firstly you sent it a bit late, about a day before the 5.0 release, and
> at 6am Saturday my time :) In future if you want me to merge something
> please send a pull at least the ~Wednesday before the release.
  
Ok .. It was Friday morning my time.

> Secondly I had no idea this code was even in linux-next. I'm not sure if
> I was Cc'ed at some point when you added it, if so sorry I missed it,
> but I get lots of email. If you're going to add changes to arch/powerpc
> in your next tree I'd appreciate some notice, or preferably an explicit
> ack.
 
Can I have an ack now ? Since your looking at it. Do you think this has no use,
certainly Cisco has use for it. It's still in linux-next as of now.

> The main reason I didn't merge it is that it's adding a bunch of code
> outside of arch/powerpc, into files which I'm not the maintainer for,
> and the patches doing so have no acks or reviews from anyone.

With the exception of the Kconfig the header file is brand new, so I'm not sure
who would ack that. From a maintainer perspective I think you could add new
files without issues from other maintainers.

> It's also adding a generic implementation with no indication that any
> other arches are willing/able to use the generic implementation, which
> begs the question whether it will actually used.
 
It would have been used by powerpc ;) I've gotten feedback in the past from
Ralf Baechle who thought this was useful, however that was years ago when
this was first submitted and the code around this area in mips has changed and
it would require a fair amount of new work to function properly on mips.

Also , no other platforms need to use this. Powerpc could be the only user of
it. This isn't really a question of a new exciting implementation of
something. This is really simple, it's just consolidation across architectures.
The implementation is vanilla, non-exciting stuff.

> I appreciate it's hard to get these sort of cross architecture changes
> into mainline, but I don't think this is the way to do it.
> 
> I'd suggest you post a patch series to linux-arch with the generic
> changes and as many architecture conversions as you can manage, then get
> some review/acks for the generic changes and chase arch maintainers for
> some acks.
 
I didn't post to linux-arch , but the code has been around for years, submitted
multiple times with more architectures than powerpc. It was scaled down to just
powerpc to simplify it's submission.

It's really a simple set of changes, I don't think it needs as much thought as
other cross architecture changes.

> I realise you have posted the series before, it may require some
> persistence. There were also quite a few comments from Christophe, so
> replying to those would be a good place to start.
 
I've looked at his comments, but I think he was more worried about conflicts with
his debugging enablement, not something to stop a pull request.

> > The following changes since commit ccda4af0f4b92f7b4c308d3acc262f4a7e3affad:
> >
> >   Linux 4.20-rc2 (2018-11-11 17:12:31 -0600)
> >
> > are available in the git repository at:
> >
> >   https://github.com/daniel-walker/cisco-linux.git for-powerpc
> >
> > for you to fetch changes up to 5d4514a9c291ecf19b0626695161673d35e5d549:
> >
> >   powerpc: convert config files to generic cmdline (2018-11-16 07:32:26 -0800)
> >
> > ----------------------------------------------------------------
> > Daniel Walker (3):
> >       add generic builtin command line
> >       powerpc: convert to generic builtin command line
> >       powerpc: convert config files to generic cmdline
> >
> >  arch/powerpc/Kconfig                          | 23 +--------
> >  arch/powerpc/configs/44x/fsp2_defconfig       | 29 ++++++-----
> >  arch/powerpc/configs/44x/iss476-smp_defconfig | 24 ++++-----
> >  arch/powerpc/configs/44x/warp_defconfig       | 12 ++---
> >  arch/powerpc/configs/holly_defconfig          | 12 ++---
> >  arch/powerpc/configs/mvme5100_defconfig       | 25 +++++-----
> >  arch/powerpc/configs/skiroot_defconfig        | 48 +++++++++---------
> >  arch/powerpc/configs/storcenter_defconfig     | 15 +++---
> 
> Also if you're updating defconfigs please don't include any unrelated
> changes. Trimming the defconfigs can silently drop symbols and break
> people's setups so needs to be done carefully.
 
> It's safer to just sed the defconfig files directly, rather than running
> savedefconfig on them.
 
Ok.

Daniel

Re: [PULL REQUEST] powerpc generic command line

[Christophe Leroy]

Hi Daniel,

Le 19/03/2019 à 16:38, Daniel Walker a écrit :
> On Tue, Mar 19, 2019 at 12:18:03PM +1100, Michael Ellerman wrote:
>> Hi Daniel,
>>
>> Daniel Walker <danielwa@cisco.com> writes:
>>> Here are the generic command line changes for powerpc.
>>>
>>> These changes have been in linux-next for two cycles, with few problems reported.
>>> It's also been used at Cisco Systems, Inc. in production products for many many
>>> years with no problems.
>>>
>>> Please pull these changes.
>>
>> Sorry I didn't reply to this earlier, have been busy with merge window
>> bugs and so on.
>>
>> As I imagine you noticed, I didn't pull this. There are a few reasons.
>>
>> Firstly you sent it a bit late, about a day before the 5.0 release, and
>> at 6am Saturday my time :) In future if you want me to merge something
>> please send a pull at least the ~Wednesday before the release.
>    
> Ok .. It was Friday morning my time.
> 
>> Secondly I had no idea this code was even in linux-next. I'm not sure if
>> I was Cc'ed at some point when you added it, if so sorry I missed it,
>> but I get lots of email. If you're going to add changes to arch/powerpc
>> in your next tree I'd appreciate some notice, or preferably an explicit
>> ack.
>   
> Can I have an ack now ? Since your looking at it. Do you think this has no use,
> certainly Cisco has use for it. It's still in linux-next as of now.
> 
>> The main reason I didn't merge it is that it's adding a bunch of code
>> outside of arch/powerpc, into files which I'm not the maintainer for,
>> and the patches doing so have no acks or reviews from anyone.
> 
> With the exception of the Kconfig the header file is brand new, so I'm not sure
> who would ack that. From a maintainer perspective I think you could add new
> files without issues from other maintainers.
> 
>> It's also adding a generic implementation with no indication that any
>> other arches are willing/able to use the generic implementation, which
>> begs the question whether it will actually used.
>   
> It would have been used by powerpc ;) I've gotten feedback in the past from
> Ralf Baechle who thought this was useful, however that was years ago when
> this was first submitted and the code around this area in mips has changed and
> it would require a fair amount of new work to function properly on mips.
> 
> Also , no other platforms need to use this. Powerpc could be the only user of
> it. This isn't really a question of a new exciting implementation of
> something. This is really simple, it's just consolidation across architectures.
> The implementation is vanilla, non-exciting stuff.
> 
>> I appreciate it's hard to get these sort of cross architecture changes
>> into mainline, but I don't think this is the way to do it.
>>
>> I'd suggest you post a patch series to linux-arch with the generic
>> changes and as many architecture conversions as you can manage, then get
>> some review/acks for the generic changes and chase arch maintainers for
>> some acks.
>   
> I didn't post to linux-arch , but the code has been around for years, submitted
> multiple times with more architectures than powerpc. It was scaled down to just
> powerpc to simplify it's submission.
> 
> It's really a simple set of changes, I don't think it needs as much thought as
> other cross architecture changes.
> 
>> I realise you have posted the series before, it may require some
>> persistence. There were also quite a few comments from Christophe, so
>> replying to those would be a good place to start.
>   
> I've looked at his comments, but I think he was more worried about conflicts with
> his debugging enablement, not something to stop a pull request.

Well, that's what I started with, but at the end my main worry has been 
that you bring a non exciting set of complicated macros and code to 
replace simple code, and you break something out of generic OF code to a 
new brand new generic one, instead of updating the existing generic OF code.

I like the idea behind your series very much, but I don't like too much 
the way it is proposed to be implemented. If you give me one week or 
two, I will come with a lighter proposal that should achieve the same goal.

Christophe

> 
>>> The following changes since commit ccda4af0f4b92f7b4c308d3acc262f4a7e3affad:
>>>
>>>    Linux 4.20-rc2 (2018-11-11 17:12:31 -0600)
>>>
>>> are available in the git repository at:
>>>
>>>    https://github.com/daniel-walker/cisco-linux.git for-powerpc
>>>
>>> for you to fetch changes up to 5d4514a9c291ecf19b0626695161673d35e5d549:
>>>
>>>    powerpc: convert config files to generic cmdline (2018-11-16 07:32:26 -0800)
>>>
>>> ----------------------------------------------------------------
>>> Daniel Walker (3):
>>>        add generic builtin command line
>>>        powerpc: convert to generic builtin command line
>>>        powerpc: convert config files to generic cmdline
>>>
>>>   arch/powerpc/Kconfig                          | 23 +--------
>>>   arch/powerpc/configs/44x/fsp2_defconfig       | 29 ++++++-----
>>>   arch/powerpc/configs/44x/iss476-smp_defconfig | 24 ++++-----
>>>   arch/powerpc/configs/44x/warp_defconfig       | 12 ++---
>>>   arch/powerpc/configs/holly_defconfig          | 12 ++---
>>>   arch/powerpc/configs/mvme5100_defconfig       | 25 +++++-----
>>>   arch/powerpc/configs/skiroot_defconfig        | 48 +++++++++---------
>>>   arch/powerpc/configs/storcenter_defconfig     | 15 +++---
>>
>> Also if you're updating defconfigs please don't include any unrelated
>> changes. Trimming the defconfigs can silently drop symbols and break
>> people's setups so needs to be done carefully.
>   
>> It's safer to just sed the defconfig files directly, rather than running
>> savedefconfig on them.
>   
> Ok.
> 
> Daniel
> 

Re: [PULL REQUEST] powerpc generic command line

[Daniel Walker]

On Tue, Mar 19, 2019 at 06:42:35PM +0100, Christophe Leroy wrote:
> Well, that's what I started with, but at the end my main worry has been that
> you bring a non exciting set of complicated macros and code to replace
> simple code, and you break something out of generic OF code to a new brand
> new generic one, instead of updating the existing generic OF code.
 
Even if we update the generic OF code it only changes the powerpc changes slightly.
Because in arch/powerpc/kernel/prom_init.c there is a second version of the same
thing, which doesn't use OF.

We're not replacing simple macro's in powerpc with in-kind replacements, we're
adding a feature which we want. So yes our macros are more complicated, but in
the grand scheme of things they are very simple macros. If you think my stuff is
complicated, you haven't seen complicated.

I didn't see anyplace in your comments when you found code which would cause a
problem ? Did you find breakage which I missed?

> I like the idea behind your series very much, but I don't like too much the
> way it is proposed to be implemented. If you give me one week or two, I will
> come with a lighter proposal that should achieve the same goal.

It's fine with us, we just want the feature set. We'll continue with our version
tho, unless you decide to submit something.

I will incorporate your comments now, but immediately prior to a pull request I
couldn't add them.

Daniel