| Message ID | 20230726073127.2969387-1-tzungbi@kernel.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | platform/chrome: chromeos_acpi: print hex string for ACPI_TYPE_BUFFER | expand |
On Wed, Jul 26, 2023 at 03:31:27PM +0800, Tzung-Bi Shih wrote: > `element->buffer.pointer` should be binary blob. `%s` doesn't work perfect > for them. > > Print hex string for ACPI_TYPE_BUFFER. > > Signed-off-by: Tzung-Bi Shih <tzungbi@kernel.org> Curious about does this patch make sense to folks on the mailing list? > --- > drivers/platform/chrome/chromeos_acpi.c | 20 +++++++++++++++++++- > 1 file changed, 19 insertions(+), 1 deletion(-) > > diff --git a/drivers/platform/chrome/chromeos_acpi.c b/drivers/platform/chrome/chromeos_acpi.c > index 50d8a4d4352d..a60d344c54e4 100644 > --- a/drivers/platform/chrome/chromeos_acpi.c > +++ b/drivers/platform/chrome/chromeos_acpi.c > @@ -90,7 +90,25 @@ static int chromeos_acpi_handle_package(struct device *dev, union acpi_object *o > case ACPI_TYPE_STRING: > return sysfs_emit(buf, "%s\n", element->string.pointer); > case ACPI_TYPE_BUFFER: > - return sysfs_emit(buf, "%s\n", element->buffer.pointer); > + { > + int i, n; > + const int char_per_line = 16; > + > + for (i = 0, n = 0; i < element->buffer.length; ++i) { > + n += sysfs_emit_at(buf, n, " %2.2x", element->buffer.pointer[i]); > + > + if (((i + 1) % char_per_line) == 0) > + n += sysfs_emit_at(buf, n, "\n"); > + } > + n += sysfs_emit_at(buf, n, "\n"); > + > + if (n == PAGE_SIZE - 1) { > + dev_info(dev, "truncating sysfs content for %s\n", name); > + sysfs_emit_at(buf, n - 3, "..\n"); > + } > + > + return n; > + } > default: > dev_err(dev, "element type %d not supported\n", element->type); > return -EINVAL; > -- > 2.41.0.487.g6d72f3e995-goog >
On Tue, Aug 01, 2023 at 11:32:40AM +0800, Tzung-Bi Shih wrote: > On Wed, Jul 26, 2023 at 03:31:27PM +0800, Tzung-Bi Shih wrote: > > `element->buffer.pointer` should be binary blob. `%s` doesn't work perfect > > for them. > > > > Print hex string for ACPI_TYPE_BUFFER. > > > > Signed-off-by: Tzung-Bi Shih <tzungbi@kernel.org> > > Curious about does this patch make sense to folks on the mailing list? Since you asked.... (sorry if the questions are dumb; I'm not extremely familiar with this one) Are there any ABI concerns here? As in, did this perhaps work for some use cases that will be broken now by this change in format? If not, it would be nice to note your thought process on that. [1] Also, is it possible to use some standard formatting, like hex_dump_to_buffer()? Or, would it make more sense to make these into binary attributes (struct bin_attribute)? NB: the docs (Documentation/ABI/testing/sysfs-driver-chromeos-acpi) suggest that some attributes (like VDAT) are indeed binary, and so we should probably maintain or fix that. Brian [1] My guess: no real user space actually uses this driver yet, since ChromiumOS still has its own downstream variant on its active kernels?
On Tue, Aug 01, 2023 at 02:53:31PM -0700, Brian Norris wrote: > Are there any ABI concerns here? As in, did this perhaps work for some > use cases that will be broken now by this change in format? If not, it > would be nice to note your thought process on that. [1] There is an existing use case from userland program[2]. It works with downstream chromeos_acpi driver but not the upstream one. [2]: https://crrev.com/48a12071a43813e5bf0694f99e4024468ea900b6/host/arch/x86/lib/crossystem_arch.c#232 > Also, is it possible to use some standard formatting, like > hex_dump_to_buffer()? Ack, fix in v2[3]. [3]: https://patchwork.kernel.org/project/chrome-platform/patch/20230802095736.3079963-1-tzungbi@kernel.org/ > Or, would it make more sense to make these into binary attributes > (struct bin_attribute)? > > NB: the docs (Documentation/ABI/testing/sysfs-driver-chromeos-acpi) > suggest that some attributes (like VDAT) are indeed binary, and so we > should probably maintain or fix that. No. Unless we also want to change userland program in [2]. > [1] My guess: no real user space actually uses this driver yet, since > ChromiumOS still has its own downstream variant on its active kernels? Correct, AFAIK, there has no use cases for the upstream chromeos_acpi driver yet.
On Wed, Aug 02, 2023 at 06:06:30PM +0800, Tzung-Bi Shih wrote: > On Tue, Aug 01, 2023 at 02:53:31PM -0700, Brian Norris wrote: > > NB: the docs (Documentation/ABI/testing/sysfs-driver-chromeos-acpi) > > suggest that some attributes (like VDAT) are indeed binary, and so we > > should probably maintain or fix that. > > No. Unless we also want to change userland program in [2]. I mean, I think you need to do one of those two -- either keep this (maintain) as "binary", or update (fix) the doc to describe the hex dump instead. i.e., I think this piece of Documentation/ABI/testing/sysfs-driver-chromeos-acpi is now wrong: What: /sys/bus/platform/devices/GGL0001:*/VDAT Date: May 2022 KernelVersion: 5.19 Description: Returns the verified boot data block shared between the firmware verification step and the kernel verification step (binary). But, the userland program already doesn't know how to handle the upstream driver. So it'd be possible to change both at the same time. Anyway, hex-encoded is fine with me too; we just need to get the docs to match. Regards, Brian
On Wed, Aug 02, 2023 at 11:59:59AM -0700, Brian Norris wrote: > On Wed, Aug 02, 2023 at 06:06:30PM +0800, Tzung-Bi Shih wrote: > > On Tue, Aug 01, 2023 at 02:53:31PM -0700, Brian Norris wrote: > > > NB: the docs (Documentation/ABI/testing/sysfs-driver-chromeos-acpi) > > > suggest that some attributes (like VDAT) are indeed binary, and so we > > > should probably maintain or fix that. > > > > No. Unless we also want to change userland program in [2]. > > I mean, I think you need to do one of those two -- either keep this > (maintain) as "binary", or update (fix) the doc to describe the hex dump > instead. > > i.e., I think this piece of > Documentation/ABI/testing/sysfs-driver-chromeos-acpi is now wrong: > > What: /sys/bus/platform/devices/GGL0001:*/VDAT > Date: May 2022 > KernelVersion: 5.19 > Description: > Returns the verified boot data block shared between the > firmware verification step and the kernel verification step > (binary). > > But, the userland program already doesn't know how to handle the > upstream driver. So it'd be possible to change both at the same time. > > Anyway, hex-encoded is fine with me too; we just need to get the docs to > match. Ack, fix in v3 (https://patchwork.kernel.org/project/chrome-platform/patch/20230803011245.3773756-1-tzungbi@kernel.org/).
diff --git a/drivers/platform/chrome/chromeos_acpi.c b/drivers/platform/chrome/chromeos_acpi.c index 50d8a4d4352d..a60d344c54e4 100644 --- a/drivers/platform/chrome/chromeos_acpi.c +++ b/drivers/platform/chrome/chromeos_acpi.c @@ -90,7 +90,25 @@ static int chromeos_acpi_handle_package(struct device *dev, union acpi_object *o case ACPI_TYPE_STRING: return sysfs_emit(buf, "%s\n", element->string.pointer); case ACPI_TYPE_BUFFER: - return sysfs_emit(buf, "%s\n", element->buffer.pointer); + { + int i, n; + const int char_per_line = 16; + + for (i = 0, n = 0; i < element->buffer.length; ++i) { + n += sysfs_emit_at(buf, n, " %2.2x", element->buffer.pointer[i]); + + if (((i + 1) % char_per_line) == 0) + n += sysfs_emit_at(buf, n, "\n"); + } + n += sysfs_emit_at(buf, n, "\n"); + + if (n == PAGE_SIZE - 1) { + dev_info(dev, "truncating sysfs content for %s\n", name); + sysfs_emit_at(buf, n - 3, "..\n"); + } + + return n; + } default: dev_err(dev, "element type %d not supported\n", element->type); return -EINVAL;
`element->buffer.pointer` should be binary blob. `%s` doesn't work perfect for them. Print hex string for ACPI_TYPE_BUFFER. Signed-off-by: Tzung-Bi Shih <tzungbi@kernel.org> --- drivers/platform/chrome/chromeos_acpi.c | 20 +++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-)