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

[Pali RohÃr]

On Monday 12 October 2015 22:16:40 Tony Lindgren wrote:
> * Tony Lindgren <tony@xxxxxxxxxxx> [150713 06:21]:
> > * Pali RohÃr <pali.rohar@xxxxxxxxx> [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@xxxxxxxxx

Attachment: signature.asc
Description: This is a digitally signed message part.