| Message ID | 20200319175737.10166-1-frederic.danis@collabora.com |
|---|---|
| Headers | show |
| Series | Add command to display or save Linux PStore dumps | expand |
Dear Fr?d?ric, In message <20200319175737.10166-1-frederic.danis at collabora.com> you wrote: > This serie of patches adds a new pstore command allowing to display or save > ramoops logs (oops, panic, console, ftrace and user) generated by a previous > kernel crash. > PStore parameters can be set in U-Boot configuration file, or at run-time > using "pstore set" command. For kernel using Device Tree, the parameters are > dynamically added to Device Tree. > Records size should be the same as the ones used by kernel, and should be a > power of 2. I wonder if we are reinventing the wheel here again? There is this feature in U-Boot which is called "shared log buffer"; a couple of years ago this was fully functional at least on Power and ARM architectures, but it was rarely used and probably has not been tested for years. A;so, the necessary tiny patch to have it supported in Linux as well never made it upstream (don't remember why, likely lack of time/interest). The functionality we had then was the following: - A memory area war reserved in U-Boot (typically at the upper end of memory) as a buffer that was shared between U-Boot and Linux. The format was as used for the kernel log buffer. - Upon boot, U-Boot would not re-initialize an existing log buffer, but keep it's content. That means, you could read and display the log buffer of the linux kernel that was running before the reset. After kernel crashes, pretty often this contained information that the kernel could not even print to the serial console any more. - In U-Boot, you could append log entries to that buffer. For example, this was used to record the results of the Power On Self Test (POST) routines (another feature that only few people still remember). - When booting Linux, the kernel syslog mechanism was used to extract the information from the log buffer in the usual way. The interesting fact here was that the Linux kernel was able to extract and save the kernel panic messages etc. from the crash before, plus any messages logged by U-Boot. To me this sounds very much like what you are adding here (plus a few features more). Does it make sense to unify such code? Added Heiko to Cc:, as he is currently working on fixes to get shared logbuffer working again for another project. Best regards, Wolfgang Denk
On 3/19/20 9:37 PM, Wolfgang Denk wrote: > Dear Fr?d?ric, > > In message <20200319175737.10166-1-frederic.danis at collabora.com> you wrote: >> This serie of patches adds a new pstore command allowing to display or save >> ramoops logs (oops, panic, console, ftrace and user) generated by a previous >> kernel crash. >> PStore parameters can be set in U-Boot configuration file, or at run-time >> using "pstore set" command. For kernel using Device Tree, the parameters are >> dynamically added to Device Tree. >> Records size should be the same as the ones used by kernel, and should be a >> power of 2. > > I wonder if we are reinventing the wheel here again? > > There is this feature in U-Boot which is called "shared log buffer"; > a couple of years ago this was fully functional at least on Power > and ARM architectures, but it was rarely used and probably has not > been tested for years. A;so, the necessary tiny patch to have it > supported in Linux as well never made it upstream (don't remember > why, likely lack of time/interest). > > The functionality we had then was the following: > > - A memory area war reserved in U-Boot (typically at the upper end > of memory) as a buffer that was shared between U-Boot and Linux. > The format was as used for the kernel log buffer. > - Upon boot, U-Boot would not re-initialize an existing log buffer, > but keep it's content. That means, you could read and display the > log buffer of the linux kernel that was running before the reset. > After kernel crashes, pretty often this contained information that > the kernel could not even print to the serial console any more. > - In U-Boot, you could append log entries to that buffer. For > example, this was used to record the results of the Power On Self > Test (POST) routines (another feature that only few people still > remember). > - When booting Linux, the kernel syslog mechanism was used to > extract the information from the log buffer in the usual way. > > The interesting fact here was that the Linux kernel was able to > extract and save the kernel panic messages etc. from the crash > before, plus any messages logged by U-Boot. > > > To me this sounds very much like what you are adding here (plus a > few features more). Does it make sense to unify such code? It seems you are relating to https://lore.kernel.org/lkml/844oyrqvvb.fsf at sauna.l.org/t/ ramoops in Linux is exactly doing what was suggested in 2009. You can find the Documentation/admin-guide/ramoops.rst git grep -GHrn 'shared log' finds nothing in U-Boot. So if any part of the old implementation in U-Boot exists, could you, please, point us to the coding. If the original design never made it into Linux and there is an established Linux interface since 2011, I would plead to eliminate any remaining non-compliant coding from U-Boot should it exist. Best regards Heinrich > > > > Added Heiko to Cc:, as he is currently working on fixes to get > shared logbuffer working again for another project. > > Best regards, > > Wolfgang Denk >
Dear Heinrich, In message <a1b7a877-e950-b09a-a0f6-2c9c9cb7e1fb at gmx.de> you wrote: > > > To me this sounds very much like what you are adding here (plus a > > few features more). Does it make sense to unify such code? > > It seems you are relating to > https://lore.kernel.org/lkml/844oyrqvvb.fsf at sauna.l.org/t/ No, I'm not. I was talking of my own code from many, many years ago. > ramoops in Linux is exactly doing what was suggested in 2009. You can > find the Documentation/admin-guide/ramoops.rst We had this in U-Boot long before that time. It was a key requirement when we added POST support in 2002. > git grep -GHrn 'shared log' finds nothing in U-Boot. So if any part of > the old implementation in U-Boot exists, could you, please, point us to > the coding. The shared log buffer support was added by commit 56f94be3ef63: commit 56f94be3ef63732384063e110277ed89701b6471 (tag: LABEL_2002_11_05_1735) Author: Wolfgang Denk <wdenk> Date: Tue Nov 5 16:35:14 2002 +0000 * Add support for log buffer which can be passed to Linux kernel's syslog mechanism; used especially for POST results. * Patch by Klaus Heydeck, 31 Oct 2002: Add initial support for kup4k board The code was mainly in common/cmd_log.c, but this got heahily rewritten and is now in cmd/log.c ; apparently this got lost (like the original copyright, sic!) when Simon modified / rewrote this driver. For history, try: git log --follow -- common/cmd_log.c > If the original design never made it into Linux and there is an > established Linux interface since 2011, I would plead to eliminate any > remaining non-compliant coding from U-Boot should it exist. I understand what Linus has is one-way, only focussing on crash dump, i. e. it does not allow to pass information from U-Boot to Linux? Also, my understanding is that the changes needed in Linux are pretty small. Maybe Heiko can comment on that... Best regards, Wolfgang Denk
Hello Wolfgang, Heinrich, Am 20.03.2020 um 09:08 schrieb Wolfgang Denk: > Dear Heinrich, > > In message <a1b7a877-e950-b09a-a0f6-2c9c9cb7e1fb at gmx.de> you wrote: >> >>> To me this sounds very much like what you are adding here (plus a >>> few features more). Does it make sense to unify such code? >> >> It seems you are relating to >> https://lore.kernel.org/lkml/844oyrqvvb.fsf at sauna.l.org/t/ > > No, I'm not. I was talking of my own code from many, many years > ago. > >> ramoops in Linux is exactly doing what was suggested in 2009. You can >> find the Documentation/admin-guide/ramoops.rst > > We had this in U-Boot long before that time. It was a key > requirement when we added POST support in 2002. > >> git grep -GHrn 'shared log' finds nothing in U-Boot. So if any part of >> the old implementation in U-Boot exists, could you, please, point us to >> the coding. > > The shared log buffer support was added by commit 56f94be3ef63: > > commit 56f94be3ef63732384063e110277ed89701b6471 (tag: LABEL_2002_11_05_1735) > Author: Wolfgang Denk <wdenk> > Date: Tue Nov 5 16:35:14 2002 +0000 > > * Add support for log buffer which can be passed to Linux kernel's > syslog mechanism; used especially for POST results. > > * Patch by Klaus Heydeck, 31 Oct 2002: > Add initial support for kup4k board > > The code was mainly in common/cmd_log.c, but this got heahily > rewritten and is now in cmd/log.c ; apparently this got lost (like > the original copyright, sic!) when Simon modified / rewrote this > driver. > > For history, try: git log --follow -- common/cmd_log.c > > >> If the original design never made it into Linux and there is an >> established Linux interface since 2011, I would plead to eliminate any >> remaining non-compliant coding from U-Boot should it exist. > > I understand what Linus has is one-way, only focussing on crash > dump, i. e. it does not allow to pass information from U-Boot to Linux? > > Also, my understanding is that the changes needed in Linux are > pretty small. > > Maybe Heiko can comment on that... Since 2002 the logbuffer support in linux changed, so the old patch does not work anymore. First there is now the struct "printk_log" [1] Unfortunately the size of it is dependend on linux configuration. Each log entry is prepended from such a struct, just missing the possibility to detect if entry is valid (as linux was the only user of it, this was not needed). There is already a possibilty to set a log buffer length through kernel commandline parameter "log_buf_len" [2]. If this is passed, kernel already uses a new area (currently memblock_alloced) for log buffer storage! So added now the following to linux logbuffer handling: Pass the persistent logbuffer address and length through a kernel commandline parameter and use it instead memblock_alloc() Add a magic field in [1] which I use to detect if there are valid entries and where is the end of logbuffer entries in the new logbuffer area (Not that easy as there was may a wrap around, but also this is detectable) -> start and end in the new log area is now known. Now copy the new logbuffer messages from __log_buf with the already existing functions in linux kernel to the end of the new logbuffer area ... and you have old linux bootlogs and the new ones after a reboot. aditionally changes in U-Boot: - enable PRAM to reserve RAM at the end of RAM for logbuffer so U-Boot does not use this memory space. Already in U-Boot. - add common/log_buffer.c log backend, which uses the functions from kernel for analysing/writing content of the logbuffer area. "log rec" U-Boot command appends now also U-Boot log messages to the logbuffer area. - add "log show" command, with which you can show current content of logbuffer area And now U-Boot log messages also shown in linux bootlog ... bye Heiko [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/kernel/printk/printk.c#n368 [2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/kernel/printk/printk.c#n1114
Hi Wolfgang, Heinrich, Heiko, On 20/03/2020 09:08, Wolfgang Denk wrote: > Dear Heinrich, > > In message<a1b7a877-e950-b09a-a0f6-2c9c9cb7e1fb at gmx.de> you wrote: >>> To me this sounds very much like what you are adding here (plus a >>> few features more). Does it make sense to unify such code? >> It seems you are relating to >> https://lore.kernel.org/lkml/844oyrqvvb.fsf at sauna.l.org/t/ > No, I'm not. I was talking of my own code from many, many years > ago. > >> ramoops in Linux is exactly doing what was suggested in 2009. You can >> find the Documentation/admin-guide/ramoops.rst > We had this in U-Boot long before that time. It was a key > requirement when we added POST support in 2002. > >> git grep -GHrn 'shared log' finds nothing in U-Boot. So if any part of >> the old implementation in U-Boot exists, could you, please, point us to >> the coding. > The shared log buffer support was added by commit 56f94be3ef63: > > commit 56f94be3ef63732384063e110277ed89701b6471 (tag: LABEL_2002_11_05_1735) > Author: Wolfgang Denk <wdenk> > Date: Tue Nov 5 16:35:14 2002 +0000 > > * Add support for log buffer which can be passed to Linux kernel's > syslog mechanism; used especially for POST results. > > * Patch by Klaus Heydeck, 31 Oct 2002: > Add initial support for kup4k board > > The code was mainly in common/cmd_log.c, but this got heahily > rewritten and is now in cmd/log.c ; apparently this got lost (like > the original copyright, sic!) when Simon modified / rewrote this > driver. > > For history, try: git log --follow -- common/cmd_log.c > > >> If the original design never made it into Linux and there is an >> established Linux interface since 2011, I would plead to eliminate any >> remaining non-compliant coding from U-Boot should it exist. > I understand what Linus has is one-way, only focussing on crash > dump, i. e. it does not allow to pass information from U-Boot to Linux? Yes, currently it is not possible to pass information from U-Boot to Linux. But, PStore does not only store crash dump, but also console, ftrace and user space logs. It may be possible to add a "bootloader" storage space to PStore. Best regards, Fr?d?ric Danis
Dear Fr?d?ric, In message <03674974-2a2c-3804-ce02-4f3d38ff01d9 at collabora.com> you wrote: > > > I understand what Linus has is one-way, only focussing on crash > > dump, i. e. it does not allow to pass information from U-Boot to Linux? > > Yes, currently it is not possible to pass information from U-Boot to Linux. > But, PStore does not only store crash dump, but also console, ftrace and > user space logs. > > It may be possible to add a "bootloader" storage space to PStore. This does not make much sense to me. We should not try to push any possible functionality into U-Boot just because we can. U-Boot is a boot loader, and should only contain necessary functionality and things, that are impossible or difficult to solve in other ways. I would much rather see the opposite direction working (again): being able to pass information from U-Boot to Linux kernel and further into Linux user land in a standard way. Best regards, Wolfgang Denk
Dear Wolfgang, On 20/03/2020 11:35, Wolfgang Denk wrote: > Dear Fr?d?ric, > > In message <03674974-2a2c-3804-ce02-4f3d38ff01d9 at collabora.com> you wrote: >>> I understand what Linus has is one-way, only focussing on crash >>> dump, i. e. it does not allow to pass information from U-Boot to Linux? >> Yes, currently it is not possible to pass information from U-Boot to Linux. >> But, PStore does not only store crash dump, but also console, ftrace and >> user space logs. >> >> It may be possible to add a "bootloader" storage space to PStore. > This does not make much sense to me. We should not try to push any > possible functionality into U-Boot just because we can. Yes, I understand it. But, when debugging kernel crashes occurring either early in boot or hard crashes later, it can be helpful to be able to retrieve logs using the U-Boot session. > U-Boot is a boot loader, and should only contain necessary > functionality and things, that are impossible or difficult to solve > in other ways. > > > I would much rather see the opposite direction working (again): > being able to pass information from U-Boot to Linux kernel and > further into Linux user land in a standard way. > > Best regards, > > Wolfgang Denk > Best regards, Fr?d?ric Danis
Dear Fr?d?ric, In message <9b0b5776-cb47-dfb2-ab77-36c89dc64359 at collabora.com> you wrote: > > But, when debugging kernel crashes occurring either early in boot or > hard crashes later, it can be helpful to be able to retrieve logs using > the U-Boot session. Full agreement, and we had such a feature working and in mainline already 17+ years ago. It's sad that people rather reinvent the wheel instead of using existing functionality (or reviving it, if it has been broken over time). Best regards, Wolfgang Denk
Hi, On Fri, 20 Mar 2020 at 05:30, Wolfgang Denk <wd at denx.de> wrote: > > Dear Fr?d?ric, > > In message <9b0b5776-cb47-dfb2-ab77-36c89dc64359 at collabora.com> you wrote: > > > > But, when debugging kernel crashes occurring either early in boot or > > hard crashes later, it can be helpful to be able to retrieve logs using > > the U-Boot session. > > Full agreement, and we had such a feature working and in mainline > already 17+ years ago. > > It's sad that people rather reinvent the wheel instead of using > existing functionality (or reviving it, if it has been broken over > time). Indeed. Let's make sure there are tests added to this new feature (which looks extremely useful to me), so it stays working. Regards, Simon
On Mon, Mar 23, 2020 at 09:37:12AM -0600, Simon Glass wrote: > Hi, > > On Fri, 20 Mar 2020 at 05:30, Wolfgang Denk <wd at denx.de> wrote: > > > > Dear Fr?d?ric, > > > > In message <9b0b5776-cb47-dfb2-ab77-36c89dc64359 at collabora.com> you wrote: > > > > > > But, when debugging kernel crashes occurring either early in boot or > > > hard crashes later, it can be helpful to be able to retrieve logs using > > > the U-Boot session. > > > > Full agreement, and we had such a feature working and in mainline > > already 17+ years ago. > > > > It's sad that people rather reinvent the wheel instead of using > > existing functionality (or reviving it, if it has been broken over > > time). > > Indeed. > > Let's make sure there are tests added to this new feature (which looks > extremely useful to me), so it stays working. I'll just chime in to say that I have now reached the point in my career where I've both been told "you're re-inventing the wheel (again)" and now seen the wheel be re-re...-re-invented.