Re: [PATCH v3 12/29] dm: core: Add basic ACPI support
Hi Andy, On Wed, 8 Apr 2020 at 11:02, Andy Shevchenko wrote: > > On Tue, Apr 07, 2020 at 08:57:24PM -0600, Simon Glass wrote: > > On Fri, 3 Apr 2020 at 05:35, Andy Shevchenko > > wrote: > > > On Mon, Mar 30, 2020 at 05:12:48PM -0600, Simon Glass wrote: > > ... > > > > > + strncpy(out_name, name, ACPI_NAME_MAX); > > > > > > memcpy()? > > > > strcpy() seems better since it doesn't rely on the correct string in name. > > Definitely not. It has no boundary checks. > > > > > + out_name[ACPI_NAME_LEN] = '\0'; > > > > > > I dunno if compiler is clever enough to catch this and avoid any warnings. > > > > I don't see any warnings. > > strncpy() fine then. > > > > Also it seems above should also have _LEN, and not _MAX. > > > > Do you mean for the strncpy()? I could do that but I still want to set > > the terminator to nul so that the string is definitely terminated. > > Terminator can be not present. > I should be > > strncpy(out_name, name, ACPI_NAME_LEN); > out_name[ACPI_NAME_LEN] = '\0'; Why? The code I have seems to work just fine? Regards, Simon
Re: [PATCH v3 12/29] dm: core: Add basic ACPI support
On Tue, Apr 07, 2020 at 08:57:24PM -0600, Simon Glass wrote: > On Fri, 3 Apr 2020 at 05:35, Andy Shevchenko > wrote: > > On Mon, Mar 30, 2020 at 05:12:48PM -0600, Simon Glass wrote: ... > > > + strncpy(out_name, name, ACPI_NAME_MAX); > > > > memcpy()? > > strcpy() seems better since it doesn't rely on the correct string in name. Definitely not. It has no boundary checks. > > > + out_name[ACPI_NAME_LEN] = '\0'; > > > > I dunno if compiler is clever enough to catch this and avoid any warnings. > > I don't see any warnings. strncpy() fine then. > > Also it seems above should also have _LEN, and not _MAX. > > Do you mean for the strncpy()? I could do that but I still want to set > the terminator to nul so that the string is definitely terminated. Terminator can be not present. I should be strncpy(out_name, name, ACPI_NAME_LEN); out_name[ACPI_NAME_LEN] = '\0'; -- With Best Regards, Andy Shevchenko
Re: [PATCH v3 12/29] dm: core: Add basic ACPI support
Hi Andy,
On Fri, 3 Apr 2020 at 05:35, Andy Shevchenko
wrote:
>
> On Mon, Mar 30, 2020 at 05:12:48PM -0600, Simon Glass wrote:
> > ACPI (Advanced Configuration and Power Interface) is a standard for
> > specifying information about a platform. It is a little like device
> > tree but the bindings are part of the specification and it supports an
> > interpreted bytecode language.
> >
> > Driver model does not use ACPI for U-Boot's configuration, but it is
> > convenient to have it support generation of ACPI tables for passing to
> > Linux, etc.
> >
> > As a starting point, add an optional set of ACPI operations to each
> > device. Initially only a single operation is available, to obtain the
> > ACPI name for the device. More operations are added later.
> >
> > Enable ACPI for sandbox to ensure build coverage and so that we can add
> > tests.
>
>
> ...
>
> > +int acpi_copy_name(char *out_name, const char *name)
> > +{
>
> > + strncpy(out_name, name, ACPI_NAME_MAX);
>
> memcpy()?
strcpy() seems better since it doesn't rely on the correct string in name.
> > + out_name[ACPI_NAME_LEN] = '\0';
>
> I dunno if compiler is clever enough to catch this and avoid any warnings.
I don't see any warnings.
> Also it seems above should also have _LEN, and not _MAX.
Do you mean for the strncpy()? I could do that but I still want to set
the terminator to nul so that the string is definitely terminated.
Regards,
Simon
Re: [PATCH v3 12/29] dm: core: Add basic ACPI support
On Mon, Mar 30, 2020 at 05:12:48PM -0600, Simon Glass wrote:
> ACPI (Advanced Configuration and Power Interface) is a standard for
> specifying information about a platform. It is a little like device
> tree but the bindings are part of the specification and it supports an
> interpreted bytecode language.
>
> Driver model does not use ACPI for U-Boot's configuration, but it is
> convenient to have it support generation of ACPI tables for passing to
> Linux, etc.
>
> As a starting point, add an optional set of ACPI operations to each
> device. Initially only a single operation is available, to obtain the
> ACPI name for the device. More operations are added later.
>
> Enable ACPI for sandbox to ensure build coverage and so that we can add
> tests.
...
> +int acpi_copy_name(char *out_name, const char *name)
> +{
> + strncpy(out_name, name, ACPI_NAME_MAX);
memcpy()?
> + out_name[ACPI_NAME_LEN] = '\0';
I dunno if compiler is clever enough to catch this and avoid any warnings.
Also it seems above should also have _LEN, and not _MAX.
> + return 0;
> +}
--
With Best Regards,
Andy Shevchenko
Antwort: [PATCH v3 12/29] dm: core: Add basic ACPI support
Hi Simon, -"Simon Glass" schrieb: - >Betreff: [PATCH v3 12/29] dm: core: Add basic ACPI support > >ACPI (Advanced Configuration and Power Interface) is a standard for >specifying information about a platform. It is a little like device >tree but the bindings are part of the specification and it supports >an >interpreted bytecode language. > >Driver model does not use ACPI for U-Boot's configuration, but it is >convenient to have it support generation of ACPI tables for passing >to >Linux, etc. > >As a starting point, add an optional set of ACPI operations to each >device. Initially only a single operation is available, to obtain the >ACPI name for the device. More operations are added later. > >Enable ACPI for sandbox to ensure build coverage and so that we can >add >tests. > >Reviewed-by: Bin Meng >Signed-off-by: Simon Glass >--- > >Changes in v3: >- Compute ACPI_NAME_MAX from ACPI_NAME_LEN >- Drop 'Intel' from 'Intel ACPI' >- Rename acpi_return_name() to acpi_copy_name() >- Reword commit message to drop the bit about ACPI being complicated >- Use strncpy() instead of strcpy() in acpi_copy_name() > >Changes in v2: >- Move LOGC_ACPI definition to this patch > > drivers/core/Kconfig | 9 ++ > drivers/core/Makefile | 1 + > drivers/core/acpi.c | 33 +++ > include/dm/acpi.h | 73 >+++ > include/dm/device.h | 5 +++ > include/log.h | 2 ++ > 6 files changed, 123 insertions(+) > create mode 100644 drivers/core/acpi.c > create mode 100644 include/dm/acpi.h > Reviewed-by: Wolfgang Wallner
[PATCH v3 12/29] dm: core: Add basic ACPI support
ACPI (Advanced Configuration and Power Interface) is a standard for
specifying information about a platform. It is a little like device
tree but the bindings are part of the specification and it supports an
interpreted bytecode language.
Driver model does not use ACPI for U-Boot's configuration, but it is
convenient to have it support generation of ACPI tables for passing to
Linux, etc.
As a starting point, add an optional set of ACPI operations to each
device. Initially only a single operation is available, to obtain the
ACPI name for the device. More operations are added later.
Enable ACPI for sandbox to ensure build coverage and so that we can add
tests.
Reviewed-by: Bin Meng
Signed-off-by: Simon Glass
---
Changes in v3:
- Compute ACPI_NAME_MAX from ACPI_NAME_LEN
- Drop 'Intel' from 'Intel ACPI'
- Rename acpi_return_name() to acpi_copy_name()
- Reword commit message to drop the bit about ACPI being complicated
- Use strncpy() instead of strcpy() in acpi_copy_name()
Changes in v2:
- Move LOGC_ACPI definition to this patch
drivers/core/Kconfig | 9 ++
drivers/core/Makefile | 1 +
drivers/core/acpi.c | 33 +++
include/dm/acpi.h | 73 +++
include/dm/device.h | 5 +++
include/log.h | 2 ++
6 files changed, 123 insertions(+)
create mode 100644 drivers/core/acpi.c
create mode 100644 include/dm/acpi.h
diff --git a/drivers/core/Kconfig b/drivers/core/Kconfig
index 3b95b5387b9..a3b03993423 100644
--- a/drivers/core/Kconfig
+++ b/drivers/core/Kconfig
@@ -261,4 +261,13 @@ config DM_DEV_READ_INLINE
bool
default y if !OF_LIVE
+config ACPIGEN
+ bool "Support ACPI table generation in driver model"
+ default y if SANDBOX || GENERATE_ACPI_TABLE
+ help
+ This option enables generation of ACPI tables using driver-model
+ devices. It adds a new operation struct to each driver, to support
+ things like generating device-specific tables and returning the ACPI
+ name of a device.
+
endmenu
diff --git a/drivers/core/Makefile b/drivers/core/Makefile
index bce7467da1d..c707026a3a0 100644
--- a/drivers/core/Makefile
+++ b/drivers/core/Makefile
@@ -3,6 +3,7 @@
# Copyright (c) 2013 Google, Inc
obj-y += device.o fdtaddr.o lists.o root.o uclass.o util.o
+obj-$(CONFIG_$(SPL_TPL_)ACPIGEN) += acpi.o
obj-$(CONFIG_DEVRES) += devres.o
obj-$(CONFIG_$(SPL_)DM_DEVICE_REMOVE) += device-remove.o
obj-$(CONFIG_$(SPL_)SIMPLE_BUS)+= simple-bus.o
diff --git a/drivers/core/acpi.c b/drivers/core/acpi.c
new file mode 100644
index 000..0e64c21bf5b
--- /dev/null
+++ b/drivers/core/acpi.c
@@ -0,0 +1,33 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Core driver model support for ACPI table generation
+ *
+ * Copyright 2019 Google LLC
+ * Written by Simon Glass
+ */
+
+#define LOG_CATEOGRY LOGC_ACPI
+
+#include
+#include
+#include
+#include
+
+int acpi_copy_name(char *out_name, const char *name)
+{
+ strncpy(out_name, name, ACPI_NAME_MAX);
+ out_name[ACPI_NAME_LEN] = '\0';
+
+ return 0;
+}
+
+int acpi_get_name(const struct udevice *dev, char *out_name)
+{
+ struct acpi_ops *aops;
+
+ aops = device_get_acpi_ops(dev);
+ if (aops && aops->get_name)
+ return aops->get_name(dev, out_name);
+
+ return -ENOSYS;
+}
diff --git a/include/dm/acpi.h b/include/dm/acpi.h
new file mode 100644
index 000..c13d0c8448a
--- /dev/null
+++ b/include/dm/acpi.h
@@ -0,0 +1,73 @@
+/* SPDX-License-Identifier: GPL-2.0+ */
+/*
+ * Core ACPI (Advanced Configuration and Power Interface) support
+ *
+ * Copyright 2019 Google LLC
+ * Written by Simon Glass
+ */
+
+#ifndef __DM_ACPI_H__
+#define __DM_ACPI_H__
+
+/* Allow operations to be optional for ACPI */
+#if CONFIG_IS_ENABLED(ACPIGEN)
+#define acpi_ops_ptr(_ptr) .acpi_ops = _ptr,
+#else
+#define acpi_ops_ptr(_ptr)
+#endif
+
+/* Length of an ACPI name string, excluding nul terminator */
+#define ACPI_NAME_LEN 4
+
+/* Length of an ACPI name string including nul terminator */
+#define ACPI_NAME_MAX (ACPI_NAME_LEN + 1)
+
+/**
+ * struct acpi_ops - ACPI operations supported by driver model
+ */
+struct acpi_ops {
+ /**
+* get_name() - Obtain the ACPI name of a device
+*
+* @dev: Device to check
+* @out_name: Place to put the name, must hold at least ACPI_NAME_MAX
+* bytes
+* @return 0 if OK, -ENOENT if no name is available, other -ve value on
+* other error
+*/
+ int (*get_name)(const struct udevice *dev, char *out_name);
+};
+
+#define device_get_acpi_ops(dev) ((dev)->driver->acpi_ops)
+
+/**
+ * acpi_get_name() - Obtain the ACPI name of a device
+ *
+ * @dev: Device to check
+ * @out_name: Place to put the name, must hold at least ACPI_NAME_MAX
+ * bytes
+ * @return 0 if OK, -ENOENT if no name is available, other -ve value on
+ * other error
+ */
+int acpi_get_name(const s
