| Message ID | 20170815211556.nnnnbtlvorgc3ijh@intel.com (mailing list archive) |
|---|---|
| State | RFC, archived |
| Headers | show |
"Luck, Tony" <tony.luck@intel.com> writes: > On Tue, Aug 15, 2017 at 11:22:06AM +0100, Punit Agrawal wrote: >> There is already a bert driver which prints the error record. Would it >> make sense to integrate the character device there instead of creating a >> new driver? > > Like this? The source code is smaller. But it doesn't offer the option to unload > the driver and unmap the BERT data region after you have retrieved the error record. Is there any benefit in being able to unload the driver in real world usage? It should be possible to convert the existing driver into a loadable module - thought that means re-printing the error records to the kernel log if the module is re-loaded. Not sure if that breaks any existing usecases. One thing I missed commenting on in the previous version - Have you thought of exposing the error records via /sys/firmware/acpi? The tables are already exposed there and as BERT is part of ACPI logically that's a better fit compared to a misc device. > > Either looks plausible to me (but I'm hardly a disinterested party). > > Votes? > > -Tony > > From cbeabf2d83fe91eebd960cd5cc1b61faaeed1441 Mon Sep 17 00:00:00 2001 > From: Tony Luck <tony.luck@intel.com> > Date: Tue, 15 Aug 2017 13:48:28 -0700 > Subject: [PATCH] ACPI: APEI: Extend BERT driver to provide a character device > to access data > > The BERT table simply provides the size and address of the error > record in BIOS reserved memory. Currently this driver decodes the > record to the console log. But other users of BERT may want to access > the full binary record. > > In an earlier age we might have used /dev/mem to retrieve this error > record, but many systems disable /dev/mem for security reasons. > > Extend this driver to provide read-only access to the data via a character > special device "/dev/bert-data". > > Cc: Len Brown <lenb@kernel.org> > Cc: Boris Petkov <bp@suse.de> > Cc: Tyler Baicar <tbaicar@codeaurora.org> > Cc: Punit Agrawal <punit.agrawal@arm.com> > Cc: linux-acpi@vger.kernel.org > Cc: linux-kernel@vger.kernel.org > Signed-off-by: Tony Luck <tony.luck@intel.com> > > v2: (suggested by Punit Agrawal) don't make a whole new driver, merge > this functionality into the existing BERT driver. > --- > drivers/acpi/apei/bert.c | 45 ++++++++++++++++++++++++++++++++++++++++++--- > 1 file changed, 42 insertions(+), 3 deletions(-) > > diff --git a/drivers/acpi/apei/bert.c b/drivers/acpi/apei/bert.c > index 12771fcf0417..9bc39b1bffde 100644 > --- a/drivers/acpi/apei/bert.c > +++ b/drivers/acpi/apei/bert.c > @@ -26,12 +26,16 @@ > #include <linux/init.h> > #include <linux/acpi.h> > #include <linux/io.h> > +#include <linux/miscdevice.h> > +#include <linux/uaccess.h> > > #include "apei-internal.h" > > #undef pr_fmt > #define pr_fmt(fmt) "BERT: " fmt > > +static __iomem void *bert_data; > +static unsigned int region_len; > static int bert_disable; > > static void __init bert_print_all(struct acpi_bert_region *region, > @@ -95,12 +99,45 @@ static int __init bert_check_table(struct acpi_table_bert *bert_tab) > return 0; > } > > +static int bert_chrdev_open(struct inode *inode, struct file *file) > +{ > + if (file->f_flags & (O_WRONLY | O_RDWR)) > + return -EPERM; > + inode->i_size = region_len; > + return 0; > +} > + > +static ssize_t bert_chrdev_read(struct file *filp, char __user *ubuf, > + size_t usize, loff_t *off) > +{ > + if (*off > region_len) > + return -EINVAL; > + if (*off + usize > region_len) > + usize = region_len - *off; > + if (copy_to_user(ubuf, bert_data + *off, usize)) > + return -EFAULT; > + *off += usize; > + return usize; > +} > + > +static const struct file_operations bert_chrdev_ops = { > + .open = bert_chrdev_open, > + .read = bert_chrdev_read, > + .llseek = default_llseek, > +}; > + > +static struct miscdevice bert_chrdev_device = { > + .minor = MISC_DYNAMIC_MINOR, > + .name = "bert-data", > + .fops = &bert_chrdev_ops, > + .mode = 0444, > +}; > + > static int __init bert_init(void) > { > - struct apei_resources bert_resources; > struct acpi_bert_region *boot_error_region; > + struct apei_resources bert_resources; > struct acpi_table_bert *bert_tab; > - unsigned int region_len; > acpi_status status; > int rc = 0; > > @@ -139,7 +176,9 @@ static int __init bert_init(void) > boot_error_region = ioremap_cache(bert_tab->address, region_len); > if (boot_error_region) { > bert_print_all(boot_error_region, region_len); > - iounmap(boot_error_region); > + bert_data = boot_error_region; > + if (misc_register(&bert_chrdev_device)) > + iounmap(boot_error_region); > } else { > rc = -ENOMEM; > } -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> One thing I missed commenting on in the previous version - > > Have you thought of exposing the error records via /sys/firmware/acpi? > The tables are already exposed there and as BERT is part of ACPI > logically that's a better fit compared to a misc device. That was my first thought :-) But I got stuck on how to name things. The BERT entry appears in /sys/firmware/acpi/tables/ ... but the code doesn't know anything special about "BERT", it just iterates over all the tables and makes them all appear. I thought about making it /sys/firmware/acpi/tables/BERT.data, but that seemed very ugly (this file isn't a "table", so why does it appear in the "tables" directory? So maybe /sys/firmware/acpi/table-data/BERT? Now the driver has to make another directory. Thoughts? -Tony -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
"Luck, Tony" <tony.luck@intel.com> writes: >> One thing I missed commenting on in the previous version - >> >> Have you thought of exposing the error records via /sys/firmware/acpi? >> The tables are already exposed there and as BERT is part of ACPI >> logically that's a better fit compared to a misc device. > > That was my first thought :-) > > But I got stuck on how to name things. The BERT entry appears in > /sys/firmware/acpi/tables/ ... but the code doesn't know anything special > about "BERT", it just iterates over all the tables and makes them all > appear. > > I thought about making it /sys/firmware/acpi/tables/BERT.data, but that > seemed very ugly (this file isn't a "table", so why does it appear in the > "tables" directory? So maybe /sys/firmware/acpi/table-data/BERT? Now > the driver has to make another directory. > > Thoughts? I agree that keeping the error record out of the tables directory makes sense as it's not an ACPI table. The best I could come up with was bert-data or bert-region in /sys/firmware/acpi/apei/. Though, I am OK with "table-data/BERT" as well. > > -Tony > -- > To unsubscribe from this list: send the line "unsubscribe linux-acpi" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/acpi/apei/bert.c b/drivers/acpi/apei/bert.c index 12771fcf0417..9bc39b1bffde 100644 --- a/drivers/acpi/apei/bert.c +++ b/drivers/acpi/apei/bert.c @@ -26,12 +26,16 @@ #include <linux/init.h> #include <linux/acpi.h> #include <linux/io.h> +#include <linux/miscdevice.h> +#include <linux/uaccess.h> #include "apei-internal.h" #undef pr_fmt #define pr_fmt(fmt) "BERT: " fmt +static __iomem void *bert_data; +static unsigned int region_len; static int bert_disable; static void __init bert_print_all(struct acpi_bert_region *region, @@ -95,12 +99,45 @@ static int __init bert_check_table(struct acpi_table_bert *bert_tab) return 0; } +static int bert_chrdev_open(struct inode *inode, struct file *file) +{ + if (file->f_flags & (O_WRONLY | O_RDWR)) + return -EPERM; + inode->i_size = region_len; + return 0; +} + +static ssize_t bert_chrdev_read(struct file *filp, char __user *ubuf, + size_t usize, loff_t *off) +{ + if (*off > region_len) + return -EINVAL; + if (*off + usize > region_len) + usize = region_len - *off; + if (copy_to_user(ubuf, bert_data + *off, usize)) + return -EFAULT; + *off += usize; + return usize; +} + +static const struct file_operations bert_chrdev_ops = { + .open = bert_chrdev_open, + .read = bert_chrdev_read, + .llseek = default_llseek, +}; + +static struct miscdevice bert_chrdev_device = { + .minor = MISC_DYNAMIC_MINOR, + .name = "bert-data", + .fops = &bert_chrdev_ops, + .mode = 0444, +}; + static int __init bert_init(void) { - struct apei_resources bert_resources; struct acpi_bert_region *boot_error_region; + struct apei_resources bert_resources; struct acpi_table_bert *bert_tab; - unsigned int region_len; acpi_status status; int rc = 0; @@ -139,7 +176,9 @@ static int __init bert_init(void) boot_error_region = ioremap_cache(bert_tab->address, region_len); if (boot_error_region) { bert_print_all(boot_error_region, region_len); - iounmap(boot_error_region); + bert_data = boot_error_region; + if (misc_register(&bert_chrdev_device)) + iounmap(boot_error_region); } else { rc = -ENOMEM; }