Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1245103 > unrolled thread

Re: [PATCH 5/5] arm: boot: store ATAGs structure into DT "/chosen/linux,atags" entry

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH 5/5] arm: boot: store ATAGs structure into DT  "/chosen/linux,atags" entry Tony Lindgren <tony@atomide.com> - 2015-10-12 22:20 +0200
    Re: [PATCH 5/5] arm: boot: store ATAGs structure into DT "/chosen/linux,atags" entry Pali Rohár <pali.rohar@gmail.com> - 2015-10-12 22:30 +0200
      Re: [PATCH 5/5] arm: boot: store ATAGs structure into DT  "/chosen/linux,atags" entry Tony Lindgren <tony@atomide.com> - 2015-10-12 22:50 +0200
        Re: [PATCH 5/5] arm: boot: store ATAGs structure into DT  "/chosen/linux,atags" entry Pali Rohár <pali.rohar@gmail.com> - 2015-10-13 16:40 +0200

#1245103 — Re: [PATCH 5/5] arm: boot: store ATAGs structure into DT "/chosen/linux,atags" entry

* Tony Lindgren <tony@atomide.com> [150713 06:21]:
> * Pali Rohár <pali.rohar@gmail.com> [150707 05:00]:
> > On Tuesday 07 July 2015 12:32:13 Russell King - ARM Linux wrote:
> > > On Mon, Jul 06, 2015 at 10:26:13PM +0200, Pali Rohár wrote:
> > > > Legacy bootloaders can pass additional information for kernel or legacy
> > > > userspace applications. When booting DT kernel then ATAGs structure is not
> > > > more visible after running kernel uncompress code. This patch stores full
> > > > ATAGs structure into DT "/chosen/linux,atags" entry, so kernel can later
> > > > reuse it and export via /proc/atags to userspace.
> > > 
> > > I think you need to go through your commit messages and improve them,
> > > especially the ones with "TODO" in them.  As long as there's still things
> > > to be done, they're obviously not ready for merging.
> > > 
> > 
> > I know, in cover letter email I wrote that documentation is not ready...
> > I send patches for review and comments (like yours). I think it is still
> > better to send something and mark it as incomplete. It could prevent to
> > work on something which will be again rewritten...
> > 
> > > Moreover, exporting the ATAGS is questionable, even _if_ there are non-
> > > kexec programs making use of this.  The ATAGs have _never_ been exported
> > > to userspace when kexec disabled is the kernel - it was introduced for
> > > kexec, and has always had this:
> > > 
> > > config ATAGS_PROC
> > >         bool "Export atags in procfs"
> > >         depends on ATAGS && KEXEC
> > >         default y
> > > 
> > > Now, the fact that someone decided to start using it is pretty sad,
> > > because it means that if you disable KEXEC, userspace breaks.  That's
> > > not a kernel regression in any shape or form, because /proc/atags has
> > > never been there without KEXEC enabled.  That's a userspace bug, plain
> > > and simple.
> > > 
> > > Given that, I'm in two minds about whether to accept the last two
> > > patches which make this more than just "for KEXEC use to enable a KEXEC
> > > kernel to be booted."
> > > 
> > > Had it been provided without the KEXEC conditional, then I don't have
> > > a problem with these two patches.
> > > 
> > 
> > I understand it. Nokia originally invented their own entries in /proc/
> > which export needed ATAGs from kernel in human-readable form, but all
> > those entries were non-standard and specific for Nokia's kernels.
> > 
> > Do you have some other idea how to provide ATAGs information created
> > dynamically by legacy closed proprietary bootloader to userspace from DT
> > booted kernel?
> > 
> > Anyway, for supporting kexec (with passing ATAGs) it is needed to have
> > working /proc/atags file, right?
> 
> Yeah I think that since we already have it in /proc, we should just
> support it. And keep it behind CONFIG_KEXEC and CONFIG_ARM_APPENDED_DTB
> and hope we don't find other users for it.. Then reconsider the Kconfig
> dependencies if we do find other users.
>  
> > > It also sets a precedent: by adding this into DT, it is creating a new
> > > DT ABI as well, and we'll end up seeing dts files with an ATAG block
> > > patched into them.
> > > 
> > > Are the ATAGs at a fixed address on the N900?
> > 
> > Yes, in board-rx51.c is:
> > 
> > .atag_offset	= 0x100
> > 
> > and Nokia Bootloader (proprietary) store them to that address.
> >
> > > Can that be handled in
> > > some kind of legacy file for the N900 which calls save_atags() on it, so
> > > we don't end up introducing yet more stuff that we have to maintain into
> > > the distant future?  If not, what about copying a known working atag
> > > structure into a legacy file for the N900?
> >
> > I already asked question if it is possible to read ATAGs from DT booted
> > kernel. And somebody (do not remember who) wrote to ML, that it is not
> > possible and it can be done in that uncompress code.
> 
> I guess the other option would be to keep the raw ATAG area reserved,
> and only initialize /proc/atags from a board specific initcall.
> But I think that would complicate the already fragile uncompress
> relocation code even further?

Pali, any news on posting an updated series with the comments addressed
in this thread? It seems that we all pretty much agree what needs to
be done.

Regards,

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

[toc] | [next] | [standalone]

#1245110 — Re: [PATCH 5/5] arm: boot: store ATAGs structure into DT "/chosen/linux,atags" entry

[Multipart message — attachments visible in raw view] — view raw

On Monday 12 October 2015 22:16:40 Tony Lindgren wrote:
> * Tony Lindgren <tony@atomide.com> [150713 06:21]:
> > * Pali Rohár <pali.rohar@gmail.com> [150707 05:00]:
> > > On Tuesday 07 July 2015 12:32:13 Russell King - ARM Linux wrote:
> > > > On Mon, Jul 06, 2015 at 10:26:13PM +0200, Pali Rohár wrote:
> > > > > Legacy bootloaders can pass additional information for kernel
> > > > > or legacy userspace applications. When booting DT kernel
> > > > > then ATAGs structure is not more visible after running
> > > > > kernel uncompress code. This patch stores full ATAGs
> > > > > structure into DT "/chosen/linux,atags" entry, so kernel can
> > > > > later reuse it and export via /proc/atags to userspace.
> > > > 
> > > > I think you need to go through your commit messages and improve
> > > > them, especially the ones with "TODO" in them.  As long as
> > > > there's still things to be done, they're obviously not ready
> > > > for merging.
> > > 
> > > I know, in cover letter email I wrote that documentation is not
> > > ready... I send patches for review and comments (like yours). I
> > > think it is still better to send something and mark it as
> > > incomplete. It could prevent to work on something which will be
> > > again rewritten...
> > > 
> > > > Moreover, exporting the ATAGS is questionable, even _if_ there
> > > > are non- kexec programs making use of this.  The ATAGs have
> > > > _never_ been exported to userspace when kexec disabled is the
> > > > kernel - it was introduced for kexec, and has always had this:
> > > > 
> > > > config ATAGS_PROC
> > > > 
> > > >         bool "Export atags in procfs"
> > > >         depends on ATAGS && KEXEC
> > > >         default y
> > > > 
> > > > Now, the fact that someone decided to start using it is pretty
> > > > sad, because it means that if you disable KEXEC, userspace
> > > > breaks.  That's not a kernel regression in any shape or form,
> > > > because /proc/atags has never been there without KEXEC
> > > > enabled.  That's a userspace bug, plain and simple.
> > > > 
> > > > Given that, I'm in two minds about whether to accept the last
> > > > two patches which make this more than just "for KEXEC use to
> > > > enable a KEXEC kernel to be booted."
> > > > 
> > > > Had it been provided without the KEXEC conditional, then I
> > > > don't have a problem with these two patches.
> > > 
> > > I understand it. Nokia originally invented their own entries in
> > > /proc/ which export needed ATAGs from kernel in human-readable
> > > form, but all those entries were non-standard and specific for
> > > Nokia's kernels.
> > > 
> > > Do you have some other idea how to provide ATAGs information
> > > created dynamically by legacy closed proprietary bootloader to
> > > userspace from DT booted kernel?
> > > 
> > > Anyway, for supporting kexec (with passing ATAGs) it is needed to
> > > have working /proc/atags file, right?
> > 
> > Yeah I think that since we already have it in /proc, we should just
> > support it. And keep it behind CONFIG_KEXEC and
> > CONFIG_ARM_APPENDED_DTB and hope we don't find other users for
> > it.. Then reconsider the Kconfig dependencies if we do find other
> > users.
> > 
> > > > It also sets a precedent: by adding this into DT, it is
> > > > creating a new DT ABI as well, and we'll end up seeing dts
> > > > files with an ATAG block patched into them.
> > > > 
> > > > Are the ATAGs at a fixed address on the N900?
> > > 
> > > Yes, in board-rx51.c is:
> > > 
> > > .atag_offset	= 0x100
> > > 
> > > and Nokia Bootloader (proprietary) store them to that address.
> > > 
> > > > Can that be handled in
> > > > some kind of legacy file for the N900 which calls save_atags()
> > > > on it, so we don't end up introducing yet more stuff that we
> > > > have to maintain into the distant future?  If not, what about
> > > > copying a known working atag structure into a legacy file for
> > > > the N900?
> > > 
> > > I already asked question if it is possible to read ATAGs from DT
> > > booted kernel. And somebody (do not remember who) wrote to ML,
> > > that it is not possible and it can be done in that uncompress
> > > code.
> > 
> > I guess the other option would be to keep the raw ATAG area
> > reserved, and only initialize /proc/atags from a board specific
> > initcall. But I think that would complicate the already fragile
> > uncompress relocation code even further?
> 
> Pali, any news on posting an updated series with the comments
> addressed in this thread? It seems that we all pretty much agree
> what needs to be done.
> 
> Regards,
> 
> Tony

Tony, I'm not really sure what to do. Just wrap 4 and 5 patches into 
CONFIG_KEXEC? Or something more?

-- 
Pali Rohár
pali.rohar@gmail.com

[toc] | [prev] | [next] | [standalone]

#1245121

* Pali Rohár <pali.rohar@gmail.com> [151012 13:29]:
> On Monday 12 October 2015 22:16:40 Tony Lindgren wrote:
> > 
> > Pali, any news on posting an updated series with the comments
> > addressed in this thread? It seems that we all pretty much agree
> > what needs to be done.
> 
> Tony, I'm not really sure what to do. Just wrap 4 and 5 patches into 
> CONFIG_KEXEC? Or something more?

Well for most part your patches are fine, I think there were some
minor comments on the series.

For the CONFIG_KEXEC dependency, we should just keep the existing
behavior and keep /proc/atags behind CONFIG_KEXEC. That's all
I believe :)

Regards,

Tony


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

[toc] | [prev] | [next] | [standalone]

#1245787

On Monday 12 October 2015 13:45:09 Tony Lindgren wrote:
> * Pali Rohár <pali.rohar@gmail.com> [151012 13:29]:
> > On Monday 12 October 2015 22:16:40 Tony Lindgren wrote:
> > > 
> > > Pali, any news on posting an updated series with the comments
> > > addressed in this thread? It seems that we all pretty much agree
> > > what needs to be done.
> > 
> > Tony, I'm not really sure what to do. Just wrap 4 and 5 patches into 
> > CONFIG_KEXEC? Or something more?
> 
> Well for most part your patches are fine, I think there were some
> minor comments on the series.
> 
> For the CONFIG_KEXEC dependency, we should just keep the existing
> behavior and keep /proc/atags behind CONFIG_KEXEC. That's all
> I believe :)
> 
> Regards,
> 
> Tony
> 
> 

Ok. I will add CONFIG_KEXEC into atag patches.

And there is missing documentation for these two new DT properties
(marked as TODO in commit messages). Where to put them?

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web