| Message ID | 20230802095736.3079963-1-tzungbi@kernel.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2] platform/chrome: chromeos_acpi: print hex string for ACPI_TYPE_BUFFER | expand |
On 8/2/23 2:57 PM, 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> Please add fixes tag and CC stable as well (stable@vger.kernel.org) in next revision. Fixes: 0a4cad9c11ad ("platform/chrome: Add ChromeOS ACPI device driver") > --- > Changes from v1[1]: > - Use hex_dump_to_buffer(). > - Rewrite the loop for catching edge cases for truncating. > > [1]: https://patchwork.kernel.org/project/chrome-platform/patch/20230726073127.2969387-1-tzungbi@kernel.org/ > > drivers/platform/chrome/chromeos_acpi.c | 31 ++++++++++++++++++++++++- > 1 file changed, 30 insertions(+), 1 deletion(-) > > diff --git a/drivers/platform/chrome/chromeos_acpi.c b/drivers/platform/chrome/chromeos_acpi.c > index 50d8a4d4352d..761506d307ad 100644 > --- a/drivers/platform/chrome/chromeos_acpi.c > +++ b/drivers/platform/chrome/chromeos_acpi.c > @@ -90,7 +90,36 @@ 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, r, at, room_left; > + const int byte_per_line = 16; > + > + at = 0; > + room_left = PAGE_SIZE - 1; > + for (i = 0; i < element->buffer.length && room_left; i += byte_per_line) { > + r = hex_dump_to_buffer(element->buffer.pointer + i, > + element->buffer.length - i, > + byte_per_line, 1, buf + at, room_left, > + false); > + if (r > room_left) > + goto truncating; > + at += r; > + room_left -= r; > + > + r = sysfs_emit_at(buf, at, "\n"); > + if (!r) > + goto truncating; > + at += r; > + room_left -= r; > + } > + > + buf[at] = 0; > + return at; > +truncating: > + dev_info(dev, "truncating sysfs content for %s\n", name); > + sysfs_emit_at(buf, PAGE_SIZE - 4, "..\n"); > + return PAGE_SIZE - 1; > + } > default: > dev_err(dev, "element type %d not supported\n", element->type); > return -EINVAL;
On Wed, Aug 2, 2023 at 2:58 AM Tzung-Bi Shih <tzungbi@kernel.org> 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> > --- > Changes from v1[1]: > - Use hex_dump_to_buffer(). > - Rewrite the loop for catching edge cases for truncating. > > [1]: https://patchwork.kernel.org/project/chrome-platform/patch/20230726073127.2969387-1-tzungbi@kernel.org/ > > drivers/platform/chrome/chromeos_acpi.c | 31 ++++++++++++++++++++++++- > 1 file changed, 30 insertions(+), 1 deletion(-) > > diff --git a/drivers/platform/chrome/chromeos_acpi.c b/drivers/platform/chrome/chromeos_acpi.c > index 50d8a4d4352d..761506d307ad 100644 > --- a/drivers/platform/chrome/chromeos_acpi.c > +++ b/drivers/platform/chrome/chromeos_acpi.c > @@ -90,7 +90,36 @@ 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, r, at, room_left; > + const int byte_per_line = 16; > + > + at = 0; > + room_left = PAGE_SIZE - 1; > + for (i = 0; i < element->buffer.length && room_left; i += byte_per_line) { > + r = hex_dump_to_buffer(element->buffer.pointer + i, > + element->buffer.length - i, > + byte_per_line, 1, buf + at, room_left, > + false); > + if (r > room_left) > + goto truncating; > + at += r; > + room_left -= r; > + > + r = sysfs_emit_at(buf, at, "\n"); > + if (!r) > + goto truncating; > + at += r; > + room_left -= r; > + } > + > + buf[at] = 0; > + return at; > +truncating: > + dev_info(dev, "truncating sysfs content for %s\n", name); I really dislike that because it will, if it happens, clog the log (the condition will be permanent). How about using dev_info_once() ? Thanks, Guenter > + sysfs_emit_at(buf, PAGE_SIZE - 4, "..\n"); > + return PAGE_SIZE - 1; > + } > default: > dev_err(dev, "element type %d not supported\n", element->type); > return -EINVAL; > -- > 2.41.0.585.gd2178a4bd4-goog >
On Wed, Aug 02, 2023 at 09:04:10AM -0700, Guenter Roeck wrote: > On Wed, Aug 2, 2023 at 2:58 AM Tzung-Bi Shih <tzungbi@kernel.org> wrote: > > +truncating: > > + dev_info(dev, "truncating sysfs content for %s\n", name); > > I really dislike that because it will, if it happens, clog the log > (the condition will be permanent). How about using dev_info_once() ? Fix in v3(https://patchwork.kernel.org/project/chrome-platform/patch/20230803011245.3773756-1-tzungbi@kernel.org/).
On Wed, Aug 02, 2023 at 08:15:31PM +0500, Muhammad Usama Anjum wrote: > On 8/2/23 2:57 PM, 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> > Please add fixes tag and CC stable as well (stable@vger.kernel.org) in next > revision. > > Fixes: 0a4cad9c11ad ("platform/chrome: Add ChromeOS ACPI device driver") I think a Fixes tag should be sufficient. Fix in v3 (https://patchwork.kernel.org/project/chrome-platform/patch/20230803011245.3773756-1-tzungbi@kernel.org/).
On 8/3/23 6:23 AM, Tzung-Bi Shih wrote: > On Wed, Aug 02, 2023 at 08:15:31PM +0500, Muhammad Usama Anjum wrote: >> On 8/2/23 2:57 PM, 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> >> Please add fixes tag and CC stable as well (stable@vger.kernel.org) in next >> revision. >> >> Fixes: 0a4cad9c11ad ("platform/chrome: Add ChromeOS ACPI device driver") > > I think a Fixes tag should be sufficient. Why do you think so? As this is a bug fix which exists in the driver since it was written, if we add fixes tag and probably cc stable as well, the patch will be applied to all LTS kernels which we really want. In this way, the bug would be fixed on all LTS kernels not just the latest kernel. > > Fix in v3 (https://patchwork.kernel.org/project/chrome-platform/patch/20230803011245.3773756-1-tzungbi@kernel.org/).
On Thu, Aug 03, 2023 at 09:17:23AM +0500, Muhammad Usama Anjum wrote: > On 8/3/23 6:23 AM, Tzung-Bi Shih wrote: > > On Wed, Aug 02, 2023 at 08:15:31PM +0500, Muhammad Usama Anjum wrote: > >> On 8/2/23 2:57 PM, 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> > >> Please add fixes tag and CC stable as well (stable@vger.kernel.org) in next > >> revision. > >> > >> Fixes: 0a4cad9c11ad ("platform/chrome: Add ChromeOS ACPI device driver") > > > > I think a Fixes tag should be sufficient. > Why do you think so? As this is a bug fix which exists in the driver since > it was written, if we add fixes tag and probably cc stable as well, the > patch will be applied to all LTS kernels which we really want. In this way, > the bug would be fixed on all LTS kernels not just the latest kernel. Without the patch, I observed some malfunctions of ChromiumOS userland program `crossystem`. There is no kernel crashes and the system is bootable. Thus, I think a Fixes tag should be sufficient.
On Mon, Aug 07, 2023 at 11:37:26AM +0800, Tzung-Bi Shih wrote: > On Thu, Aug 03, 2023 at 09:17:23AM +0500, Muhammad Usama Anjum wrote: > > On 8/3/23 6:23 AM, Tzung-Bi Shih wrote: > > > On Wed, Aug 02, 2023 at 08:15:31PM +0500, Muhammad Usama Anjum wrote: > > >> On 8/2/23 2:57 PM, 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> > > >> Please add fixes tag and CC stable as well (stable@vger.kernel.org) in next > > >> revision. > > >> > > >> Fixes: 0a4cad9c11ad ("platform/chrome: Add ChromeOS ACPI device driver") > > > > > > I think a Fixes tag should be sufficient. > > Why do you think so? As this is a bug fix which exists in the driver since > > it was written, if we add fixes tag and probably cc stable as well, the > > patch will be applied to all LTS kernels which we really want. In this way, > > the bug would be fixed on all LTS kernels not just the latest kernel. > > Without the patch, I observed some malfunctions of ChromiumOS userland program > `crossystem`. There is no kernel crashes and the system is bootable. Thus, I > think a Fixes tag should be sufficient. Let's cc stable as well.
diff --git a/drivers/platform/chrome/chromeos_acpi.c b/drivers/platform/chrome/chromeos_acpi.c index 50d8a4d4352d..761506d307ad 100644 --- a/drivers/platform/chrome/chromeos_acpi.c +++ b/drivers/platform/chrome/chromeos_acpi.c @@ -90,7 +90,36 @@ 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, r, at, room_left; + const int byte_per_line = 16; + + at = 0; + room_left = PAGE_SIZE - 1; + for (i = 0; i < element->buffer.length && room_left; i += byte_per_line) { + r = hex_dump_to_buffer(element->buffer.pointer + i, + element->buffer.length - i, + byte_per_line, 1, buf + at, room_left, + false); + if (r > room_left) + goto truncating; + at += r; + room_left -= r; + + r = sysfs_emit_at(buf, at, "\n"); + if (!r) + goto truncating; + at += r; + room_left -= r; + } + + buf[at] = 0; + return at; +truncating: + dev_info(dev, "truncating sysfs content for %s\n", name); + sysfs_emit_at(buf, PAGE_SIZE - 4, "..\n"); + return PAGE_SIZE - 1; + } 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> --- Changes from v1[1]: - Use hex_dump_to_buffer(). - Rewrite the loop for catching edge cases for truncating. [1]: https://patchwork.kernel.org/project/chrome-platform/patch/20230726073127.2969387-1-tzungbi@kernel.org/ drivers/platform/chrome/chromeos_acpi.c | 31 ++++++++++++++++++++++++- 1 file changed, 30 insertions(+), 1 deletion(-)