This series is about unification on how we handle ACPI _UID when
it's known to be an integer-in-the-string.
The idea of merging either all via ACPI tree, or (which I prefer)
taking ACPI stuff for v6.1 while the rest may be picked up later
on by respective maintainers separately.
Partially compile-tested (x86-64).
Andy Shevchenko (8):
ACPI: utils: Add acpi_dev_uid_to_integer() helper to get _UID as
integer
ACPI: LPSS: Refactor _UID handling to use acpi_dev_uid_to_integer()
ACPI: x86: Refactor _UID handling to use acpi_dev_uid_to_integer()
i2c: amd-mp2-plat: Refactor _UID handling to use
acpi_dev_uid_to_integer()
i2c: mlxbf: Refactor _UID handling to use acpi_dev_uid_to_integer()
perf: qcom_l2_pmu: Refactor _UID handling to use
acpi_dev_uid_to_integer()
spi: pxa2xx: Refactor _UID handling to use acpi_dev_uid_to_integer()
efi/dev-path-parser: Refactor _UID handling to use
acpi_dev_uid_to_integer()
drivers/acpi/acpi_lpss.c | 15 ++++++------
drivers/acpi/utils.c | 24 ++++++++++++++++++
drivers/acpi/x86/utils.c | 14 ++++++++---
drivers/firmware/efi/dev-path-parser.c | 10 +++++---
drivers/i2c/busses/i2c-amd-mp2-plat.c | 27 +++++++-------------
drivers/i2c/busses/i2c-mlxbf.c | 19 +++++---------
drivers/perf/qcom_l2_pmu.c | 7 +++---
drivers/spi/spi-pxa2xx.c | 34 +++++++-------------------
include/acpi/acpi_bus.h | 1 +
include/linux/acpi.h | 5 ++++
10 files changed, 81 insertions(+), 75 deletions(-)
--
2.35.1
ACPI utils provide acpi_dev_uid_to_integer() helper to extract _UID as
an integer. Use it instead of custom approach.
Signed-off-by: Andy Shevchenko <[email protected]>
---
drivers/i2c/busses/i2c-mlxbf.c | 19 ++++++-------------
1 file changed, 6 insertions(+), 13 deletions(-)
diff --git a/drivers/i2c/busses/i2c-mlxbf.c b/drivers/i2c/busses/i2c-mlxbf.c
index 8716032f030a..4fd4b3799596 100644
--- a/drivers/i2c/busses/i2c-mlxbf.c
+++ b/drivers/i2c/busses/i2c-mlxbf.c
@@ -2230,34 +2230,27 @@ static int mlxbf_i2c_acpi_probe(struct device *dev, struct mlxbf_i2c_priv *priv)
{
const struct acpi_device_id *aid;
struct acpi_device *adev;
- unsigned long bus_id = 0;
- const char *uid;
+ u64 bus_id;
int ret;
if (acpi_disabled)
return -ENOENT;
- adev = ACPI_COMPANION(dev);
- if (!adev)
- return -ENXIO;
-
aid = acpi_match_device(mlxbf_i2c_acpi_ids, dev);
if (!aid)
return -ENODEV;
priv->chip = (struct mlxbf_i2c_chip_info *)aid->driver_data;
- uid = acpi_device_uid(adev);
- if (!uid || !(*uid)) {
+ ret = acpi_dev_uid_to_integer(adev, &bus_id);
+ if (ret) {
dev_err(dev, "Cannot retrieve UID\n");
- return -ENODEV;
+ return ret;
}
- ret = kstrtoul(uid, 0, &bus_id);
- if (!ret)
- priv->bus = bus_id;
+ priv->bus = bus_id;
- return ret;
+ return 0;
}
#else
static int mlxbf_i2c_acpi_probe(struct device *dev, struct mlxbf_i2c_priv *priv)
--
2.35.1
ACPI utils provide acpi_dev_uid_to_integer() helper to extract _UID as
an integer. Use it instead of custom approach.
Signed-off-by: Andy Shevchenko <[email protected]>
---
drivers/spi/spi-pxa2xx.c | 34 +++++++++-------------------------
1 file changed, 9 insertions(+), 25 deletions(-)
diff --git a/drivers/spi/spi-pxa2xx.c b/drivers/spi/spi-pxa2xx.c
index 4749dd598ec2..01a513d6c6b0 100644
--- a/drivers/spi/spi-pxa2xx.c
+++ b/drivers/spi/spi-pxa2xx.c
@@ -1321,30 +1321,6 @@ static void cleanup(struct spi_device *spi)
kfree(chip);
}
-#ifdef CONFIG_ACPI
-
-static int pxa2xx_spi_get_port_id(struct device *dev)
-{
- struct acpi_device *adev;
- unsigned int devid;
- int port_id = -1;
-
- adev = ACPI_COMPANION(dev);
- if (adev && adev->pnp.unique_id &&
- !kstrtouint(adev->pnp.unique_id, 0, &devid))
- port_id = devid;
- return port_id;
-}
-
-#else /* !CONFIG_ACPI */
-
-static int pxa2xx_spi_get_port_id(struct device *dev)
-{
- return -1;
-}
-
-#endif /* CONFIG_ACPI */
-
static bool pxa2xx_spi_idma_filter(struct dma_chan *chan, void *param)
{
return param == chan->device->dev;
@@ -1354,12 +1330,15 @@ static struct pxa2xx_spi_controller *
pxa2xx_spi_init_pdata(struct platform_device *pdev)
{
struct pxa2xx_spi_controller *pdata;
+ struct device *dev = &pdev->dev;
struct ssp_device *ssp;
struct resource *res;
struct device *parent = pdev->dev.parent;
u32 value = SSP_UNDEFINED;
enum pxa_ssp_type type;
const void *match;
+ int status;
+ u64 uid;
/* Always try to read property */
device_property_read_u32(&pdev->dev, "intel,spi-pxa2xx-type", &value);
@@ -1402,7 +1381,12 @@ pxa2xx_spi_init_pdata(struct platform_device *pdev)
ssp->type = type;
ssp->dev = &pdev->dev;
- ssp->port_id = pxa2xx_spi_get_port_id(&pdev->dev);
+
+ status = acpi_dev_uid_to_integer(ACPI_COMPANION(dev), &uid);
+ if (status)
+ ssp->port_id = -1;
+ else
+ ssp->port_id = uid;
pdata->is_slave = device_property_read_bool(&pdev->dev, "spi-slave");
pdata->num_chipselect = 1;
--
2.35.1
ACPI utils provide acpi_dev_uid_to_integer() helper to extract _UID as
an integer. Use it instead of custom approach.
Signed-off-by: Andy Shevchenko <[email protected]>
---
drivers/i2c/busses/i2c-amd-mp2-plat.c | 27 +++++++++------------------
1 file changed, 9 insertions(+), 18 deletions(-)
diff --git a/drivers/i2c/busses/i2c-amd-mp2-plat.c b/drivers/i2c/busses/i2c-amd-mp2-plat.c
index 84b7e6cbc67b..423fe0c8a471 100644
--- a/drivers/i2c/busses/i2c-amd-mp2-plat.c
+++ b/drivers/i2c/busses/i2c-amd-mp2-plat.c
@@ -244,14 +244,18 @@ static const struct i2c_adapter_quirks amd_i2c_dev_quirks = {
static int i2c_amd_probe(struct platform_device *pdev)
{
+ struct device *dev = &pdev->dev;
int ret;
struct amd_i2c_dev *i2c_dev;
- struct acpi_device *adev = ACPI_COMPANION(&pdev->dev);
struct amd_mp2_dev *mp2_dev;
- const char *uid;
+ u64 uid;
- if (!adev)
- return -ENODEV;
+ ret = acpi_dev_uid_to_integer(ACPI_COMPANION(dev), &uid);
+ if (ret)
+ return dev_err_probe(dev, ret, "missing UID/bus id!\n");
+ if (uid >= 2)
+ return dev_err_probe(dev, -EINVAL, "incorrect UID/bus id \"%llu\"!\n", uid);
+ dev_dbg(dev, "bus id is %llu\n", uid);
/* The ACPI namespace doesn't contain information about which MP2 PCI
* device an AMDI0011 ACPI device is related to, so assume that there's
@@ -266,6 +270,7 @@ static int i2c_amd_probe(struct platform_device *pdev)
if (!i2c_dev)
return -ENOMEM;
+ i2c_dev->common.bus_id = uid;
i2c_dev->common.mp2_dev = mp2_dev;
i2c_dev->pdev = pdev;
platform_set_drvdata(pdev, i2c_dev);
@@ -276,20 +281,6 @@ static int i2c_amd_probe(struct platform_device *pdev)
i2c_dev->common.resume = &i2c_amd_resume;
#endif
- uid = adev->pnp.unique_id;
- if (!uid) {
- dev_err(&pdev->dev, "missing UID/bus id!\n");
- return -EINVAL;
- } else if (strcmp(uid, "0") == 0) {
- i2c_dev->common.bus_id = 0;
- } else if (strcmp(uid, "1") == 0) {
- i2c_dev->common.bus_id = 1;
- } else {
- dev_err(&pdev->dev, "incorrect UID/bus id \"%s\"!\n", uid);
- return -EINVAL;
- }
- dev_dbg(&pdev->dev, "bus id is %u\n", i2c_dev->common.bus_id);
-
/* Register the adapter */
amd_mp2_pm_runtime_get(mp2_dev);
--
2.35.1
ACPI utils provide acpi_dev_uid_to_integer() helper to extract _UID as
an integer. Use it instead of custom approach.
Signed-off-by: Andy Shevchenko <[email protected]>
---
drivers/perf/qcom_l2_pmu.c | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/drivers/perf/qcom_l2_pmu.c b/drivers/perf/qcom_l2_pmu.c
index 30234c261b05..aa68e230e020 100644
--- a/drivers/perf/qcom_l2_pmu.c
+++ b/drivers/perf/qcom_l2_pmu.c
@@ -843,13 +843,14 @@ static int l2_cache_pmu_probe_cluster(struct device *dev, void *data)
struct acpi_device *adev = ACPI_COMPANION(dev);
struct l2cache_pmu *l2cache_pmu = data;
struct cluster_pmu *cluster;
- unsigned long fw_cluster_id;
+ u64 fw_cluster_id;
int err;
int irq;
- if (!adev || kstrtoul(adev->pnp.unique_id, 10, &fw_cluster_id) < 0) {
+ err = acpi_dev_uid_to_integer(adev, &fw_cluster_id);
+ if (err) {
dev_err(&pdev->dev, "unable to read ACPI uid\n");
- return -ENODEV;
+ return err;
}
cluster = devm_kzalloc(&pdev->dev, sizeof(*cluster), GFP_KERNEL);
--
2.35.1
Some users interpret _UID only as integer and for them it's easier to
have an integer representation of _UID. Add respective helper for that.
Signed-off-by: Andy Shevchenko <[email protected]>
---
drivers/acpi/utils.c | 24 ++++++++++++++++++++++++
include/acpi/acpi_bus.h | 1 +
include/linux/acpi.h | 5 +++++
3 files changed, 30 insertions(+)
diff --git a/drivers/acpi/utils.c b/drivers/acpi/utils.c
index 4acd6f7d1395..2ea14648a661 100644
--- a/drivers/acpi/utils.c
+++ b/drivers/acpi/utils.c
@@ -793,6 +793,30 @@ bool acpi_dev_hid_uid_match(struct acpi_device *adev,
}
EXPORT_SYMBOL(acpi_dev_hid_uid_match);
+/**
+ * acpi_dev_uid_to_integer - treat ACPI device _UID as integer
+ * @adev: ACPI device to get _UID from
+ * @integer: output buffer for integer
+ *
+ * Considers _UID as integer and converts it to @integer.
+ *
+ * Returns 0 on success, or negative error code otherwise.
+ */
+int acpi_dev_uid_to_integer(struct acpi_device *adev, u64 *integer)
+{
+ const char *uid;
+
+ if (!adev)
+ return -ENODEV;
+
+ uid = acpi_device_uid(adev);
+ if (!uid)
+ return -ENODATA;
+
+ return kstrtou64(uid, 0, integer);
+}
+EXPORT_SYMBOL(acpi_dev_uid_to_integer);
+
/**
* acpi_dev_found - Detect presence of a given ACPI device in the namespace.
* @hid: Hardware ID of the device.
diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h
index 42f76f2c2d49..1804d7a70918 100644
--- a/include/acpi/acpi_bus.h
+++ b/include/acpi/acpi_bus.h
@@ -739,6 +739,7 @@ static inline bool acpi_device_can_poweroff(struct acpi_device *adev)
}
bool acpi_dev_hid_uid_match(struct acpi_device *adev, const char *hid2, const char *uid2);
+int acpi_dev_uid_to_integer(struct acpi_device *adev, u64 *integer);
void acpi_dev_clear_dependencies(struct acpi_device *supplier);
bool acpi_dev_ready_for_enumeration(const struct acpi_device *device);
diff --git a/include/linux/acpi.h b/include/linux/acpi.h
index ed4aa395cc49..619b2b1e4fb4 100644
--- a/include/linux/acpi.h
+++ b/include/linux/acpi.h
@@ -799,6 +799,11 @@ acpi_dev_hid_uid_match(struct acpi_device *adev, const char *hid2, const char *u
return false;
}
+static inline int acpi_dev_uid_to_integer(struct acpi_device *adev, u64 *integer)
+{
+ return -ENODEV;
+}
+
static inline struct acpi_device *
acpi_dev_get_first_match_dev(const char *hid, const char *uid, s64 hrv)
{
--
2.35.1
On Wed, Sep 07, 2022 at 07:46:05PM +0300, Andy Shevchenko wrote:
> ACPI utils provide acpi_dev_uid_to_integer() helper to extract _UID as
> an integer. Use it instead of custom approach.
Acked-by: Mark Brown <[email protected]>
ACPI utils provide acpi_dev_uid_to_integer() helper to extract _UID as
an integer. Use it instead of custom approach.
Signed-off-by: Andy Shevchenko <[email protected]>
---
drivers/firmware/efi/dev-path-parser.c | 10 ++++++----
1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/drivers/firmware/efi/dev-path-parser.c b/drivers/firmware/efi/dev-path-parser.c
index eb9c65f97841..113b3ca1bd76 100644
--- a/drivers/firmware/efi/dev-path-parser.c
+++ b/drivers/firmware/efi/dev-path-parser.c
@@ -15,9 +15,11 @@
static long __init parse_acpi_path(const struct efi_dev_path *node,
struct device *parent, struct device **child)
{
- char hid[ACPI_ID_LEN], uid[11]; /* UINT_MAX + null byte */
struct acpi_device *adev;
struct device *phys_dev;
+ char hid[ACPI_ID_LEN];
+ long ret;
+ u64 uid;
if (node->header.length != 12)
return -EINVAL;
@@ -27,12 +29,12 @@ static long __init parse_acpi_path(const struct efi_dev_path *node,
'A' + ((node->acpi.hid >> 5) & 0x1f) - 1,
'A' + ((node->acpi.hid >> 0) & 0x1f) - 1,
node->acpi.hid >> 16);
- sprintf(uid, "%u", node->acpi.uid);
for_each_acpi_dev_match(adev, hid, NULL, -1) {
- if (adev->pnp.unique_id && !strcmp(adev->pnp.unique_id, uid))
+ ret = acpi_dev_uid_to_integer(adev, &uid);
+ if (ret == -ENODATA && node->acpi.uid == 0)
break;
- if (!adev->pnp.unique_id && node->acpi.uid == 0)
+ if (ret == 0 && node->acpi.uid == uid)
break;
}
if (!adev)
--
2.35.1
ACPI utils provide acpi_dev_uid_to_integer() helper to extract _UID as
an integer. Use it instead of custom approach.
Signed-off-by: Andy Shevchenko <[email protected]>
---
drivers/acpi/x86/utils.c | 14 ++++++++++----
1 file changed, 10 insertions(+), 4 deletions(-)
diff --git a/drivers/acpi/x86/utils.c b/drivers/acpi/x86/utils.c
index 664070fc8349..2764b4778ce7 100644
--- a/drivers/acpi/x86/utils.c
+++ b/drivers/acpi/x86/utils.c
@@ -351,11 +351,17 @@ int acpi_quirk_skip_serdev_enumeration(struct device *controller_parent, bool *s
struct acpi_device *adev = ACPI_COMPANION(controller_parent);
const struct dmi_system_id *dmi_id;
long quirks = 0;
+ u64 uid;
+ int ret;
*skip = false;
- /* !dev_is_platform() to not match on PNP enumerated debug UARTs */
- if (!adev || !adev->pnp.unique_id || !dev_is_platform(controller_parent))
+ ret = acpi_dev_uid_to_integer(adev, &uid);
+ if (ret)
+ return 0;
+
+ /* to not match on PNP enumerated debug UARTs */
+ if (!dev_is_platform(controller_parent))
return 0;
dmi_id = dmi_first_match(acpi_quirk_skip_dmi_ids);
@@ -363,10 +369,10 @@ int acpi_quirk_skip_serdev_enumeration(struct device *controller_parent, bool *s
quirks = (unsigned long)dmi_id->driver_data;
if (quirks & ACPI_QUIRK_UART1_TTY_UART2_SKIP) {
- if (!strcmp(adev->pnp.unique_id, "1"))
+ if (uid == 1)
return -ENODEV; /* Create tty cdev instead of serdev */
- if (!strcmp(adev->pnp.unique_id, "2"))
+ if (uid == 2)
*skip = true;
}
--
2.35.1
On Wed, 7 Sept 2022 at 18:57, Andy Shevchenko
<[email protected]> wrote:
>
> ACPI utils provide acpi_dev_uid_to_integer() helper to extract _UID as
> an integer. Use it instead of custom approach.
>
> Signed-off-by: Andy Shevchenko <[email protected]>
> ---
> drivers/firmware/efi/dev-path-parser.c | 10 ++++++----
> 1 file changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/firmware/efi/dev-path-parser.c b/drivers/firmware/efi/dev-path-parser.c
> index eb9c65f97841..113b3ca1bd76 100644
> --- a/drivers/firmware/efi/dev-path-parser.c
> +++ b/drivers/firmware/efi/dev-path-parser.c
> @@ -15,9 +15,11 @@
> static long __init parse_acpi_path(const struct efi_dev_path *node,
> struct device *parent, struct device **child)
> {
> - char hid[ACPI_ID_LEN], uid[11]; /* UINT_MAX + null byte */
> struct acpi_device *adev;
> struct device *phys_dev;
> + char hid[ACPI_ID_LEN];
> + long ret;
> + u64 uid;
>
> if (node->header.length != 12)
> return -EINVAL;
> @@ -27,12 +29,12 @@ static long __init parse_acpi_path(const struct efi_dev_path *node,
> 'A' + ((node->acpi.hid >> 5) & 0x1f) - 1,
> 'A' + ((node->acpi.hid >> 0) & 0x1f) - 1,
> node->acpi.hid >> 16);
> - sprintf(uid, "%u", node->acpi.uid);
>
> for_each_acpi_dev_match(adev, hid, NULL, -1) {
> - if (adev->pnp.unique_id && !strcmp(adev->pnp.unique_id, uid))
> + ret = acpi_dev_uid_to_integer(adev, &uid);
> + if (ret == -ENODATA && node->acpi.uid == 0)
> break;
> - if (!adev->pnp.unique_id && node->acpi.uid == 0)
> + if (ret == 0 && node->acpi.uid == uid)
Is it necessary to reorder the conditions here? I.e., why not
> + ret = acpi_dev_uid_to_integer(adev, &uid);
> + if (ret == 0 && node->acpi.uid == uid)
> break;
> + if (ret == -ENODATA && node->acpi.uid == 0)
> break;
?
With that fixed,
Reviewed-by: Ard Biesheuvel <[email protected]>
On Thu, Sep 08, 2022 at 11:29:22AM +0200, Hans de Goede wrote:
> On 9/7/22 18:46, Andy Shevchenko wrote:
...
> > + long ret;
>
> "long ret" should be "int ret" here since that is what acpi_dev_uid_to_integer()
> returns.
I put it long since the efi_get_device_by_path() uses long ret (for the sake of
consistency with the existing code), but I have no objections to move it to
int.
--
With Best Regards,
Andy Shevchenko
Hi,
On 9/7/22 18:45, Andy Shevchenko wrote:
> This series is about unification on how we handle ACPI _UID when
> it's known to be an integer-in-the-string.
>
> The idea of merging either all via ACPI tree, or (which I prefer)
> taking ACPI stuff for v6.1 while the rest may be picked up later
> on by respective maintainers separately.
>
> Partially compile-tested (x86-64).
>
> Andy Shevchenko (8):
> ACPI: utils: Add acpi_dev_uid_to_integer() helper to get _UID as
> integer
> ACPI: LPSS: Refactor _UID handling to use acpi_dev_uid_to_integer()
> ACPI: x86: Refactor _UID handling to use acpi_dev_uid_to_integer()
> i2c: amd-mp2-plat: Refactor _UID handling to use
> acpi_dev_uid_to_integer()
> i2c: mlxbf: Refactor _UID handling to use acpi_dev_uid_to_integer()
> perf: qcom_l2_pmu: Refactor _UID handling to use
> acpi_dev_uid_to_integer()
> spi: pxa2xx: Refactor _UID handling to use acpi_dev_uid_to_integer()
> efi/dev-path-parser: Refactor _UID handling to use
> acpi_dev_uid_to_integer()
>
> drivers/acpi/acpi_lpss.c | 15 ++++++------
> drivers/acpi/utils.c | 24 ++++++++++++++++++
> drivers/acpi/x86/utils.c | 14 ++++++++---
> drivers/firmware/efi/dev-path-parser.c | 10 +++++---
> drivers/i2c/busses/i2c-amd-mp2-plat.c | 27 +++++++-------------
> drivers/i2c/busses/i2c-mlxbf.c | 19 +++++---------
> drivers/perf/qcom_l2_pmu.c | 7 +++---
> drivers/spi/spi-pxa2xx.c | 34 +++++++-------------------
> include/acpi/acpi_bus.h | 1 +
> include/linux/acpi.h | 5 ++++
> 10 files changed, 81 insertions(+), 75 deletions(-)
>
Thanks, patches 1-7 look good to me:
Reviewed-by: Hans de Goede <[email protected]>
for patches 1-7.
I have one small remark for patch 8, which I will send in
a reply to patch 8.
Regards,
Hans
On Thu, Sep 08, 2022 at 11:28:48AM +0200, Hans de Goede wrote:
> On 9/7/22 18:45, Andy Shevchenko wrote:
> > This series is about unification on how we handle ACPI _UID when
> > it's known to be an integer-in-the-string.
> >
> > The idea of merging either all via ACPI tree, or (which I prefer)
> > taking ACPI stuff for v6.1 while the rest may be picked up later
> > on by respective maintainers separately.
> >
> > Partially compile-tested (x86-64).
> >
> > Andy Shevchenko (8):
> > ACPI: utils: Add acpi_dev_uid_to_integer() helper to get _UID as
> > integer
> > ACPI: LPSS: Refactor _UID handling to use acpi_dev_uid_to_integer()
> > ACPI: x86: Refactor _UID handling to use acpi_dev_uid_to_integer()
> > i2c: amd-mp2-plat: Refactor _UID handling to use
> > acpi_dev_uid_to_integer()
> > i2c: mlxbf: Refactor _UID handling to use acpi_dev_uid_to_integer()
> > perf: qcom_l2_pmu: Refactor _UID handling to use
> > acpi_dev_uid_to_integer()
> > spi: pxa2xx: Refactor _UID handling to use acpi_dev_uid_to_integer()
> > efi/dev-path-parser: Refactor _UID handling to use
> > acpi_dev_uid_to_integer()
> >
> > drivers/acpi/acpi_lpss.c | 15 ++++++------
> > drivers/acpi/utils.c | 24 ++++++++++++++++++
> > drivers/acpi/x86/utils.c | 14 ++++++++---
> > drivers/firmware/efi/dev-path-parser.c | 10 +++++---
> > drivers/i2c/busses/i2c-amd-mp2-plat.c | 27 +++++++-------------
> > drivers/i2c/busses/i2c-mlxbf.c | 19 +++++---------
> > drivers/perf/qcom_l2_pmu.c | 7 +++---
> > drivers/spi/spi-pxa2xx.c | 34 +++++++-------------------
> > include/acpi/acpi_bus.h | 1 +
> > include/linux/acpi.h | 5 ++++
> > 10 files changed, 81 insertions(+), 75 deletions(-)
> >
>
> Thanks, patches 1-7 look good to me:
>
> Reviewed-by: Hans de Goede <[email protected]>
>
> for patches 1-7.
>
> I have one small remark for patch 8, which I will send in
> a reply to patch 8.
Thanks for review!
--
With Best Regards,
Andy Shevchenko
Hi,
On 9/7/22 18:46, Andy Shevchenko wrote:
> ACPI utils provide acpi_dev_uid_to_integer() helper to extract _UID as
> an integer. Use it instead of custom approach.
>
> Signed-off-by: Andy Shevchenko <[email protected]>
> ---
> drivers/firmware/efi/dev-path-parser.c | 10 ++++++----
> 1 file changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/firmware/efi/dev-path-parser.c b/drivers/firmware/efi/dev-path-parser.c
> index eb9c65f97841..113b3ca1bd76 100644
> --- a/drivers/firmware/efi/dev-path-parser.c
> +++ b/drivers/firmware/efi/dev-path-parser.c
> @@ -15,9 +15,11 @@
> static long __init parse_acpi_path(const struct efi_dev_path *node,
> struct device *parent, struct device **child)
> {
> - char hid[ACPI_ID_LEN], uid[11]; /* UINT_MAX + null byte */
> struct acpi_device *adev;
> struct device *phys_dev;
> + char hid[ACPI_ID_LEN];
> + long ret;
"long ret" should be "int ret" here since that is what acpi_dev_uid_to_integer()
returns.
Regards,
Hans
> + u64 uid;
>
> if (node->header.length != 12)
> return -EINVAL;
> @@ -27,12 +29,12 @@ static long __init parse_acpi_path(const struct efi_dev_path *node,
> 'A' + ((node->acpi.hid >> 5) & 0x1f) - 1,
> 'A' + ((node->acpi.hid >> 0) & 0x1f) - 1,
> node->acpi.hid >> 16);
> - sprintf(uid, "%u", node->acpi.uid);
>
> for_each_acpi_dev_match(adev, hid, NULL, -1) {
> - if (adev->pnp.unique_id && !strcmp(adev->pnp.unique_id, uid))
> + ret = acpi_dev_uid_to_integer(adev, &uid);
> + if (ret == -ENODATA && node->acpi.uid == 0)
> break;
> - if (!adev->pnp.unique_id && node->acpi.uid == 0)
> + if (ret == 0 && node->acpi.uid == uid)
> break;
> }
> if (!adev)
On Thu, Sep 08, 2022 at 10:20:47AM +0200, Ard Biesheuvel wrote:
> On Wed, 7 Sept 2022 at 18:57, Andy Shevchenko
> <[email protected]> wrote:
...
> > for_each_acpi_dev_match(adev, hid, NULL, -1) {
> > - if (adev->pnp.unique_id && !strcmp(adev->pnp.unique_id, uid))
> > + ret = acpi_dev_uid_to_integer(adev, &uid);
> > + if (ret == -ENODATA && node->acpi.uid == 0)
> > break;
> > - if (!adev->pnp.unique_id && node->acpi.uid == 0)
> > + if (ret == 0 && node->acpi.uid == uid)
>
> Is it necessary to reorder the conditions here? I.e., why not
Code-wise there should be not much difference which does not affect the flow,
I think I moved it to be closer to the pattern "let's handle errors first",
but in this case I'm fine with your proposal.
> > + ret = acpi_dev_uid_to_integer(adev, &uid);
> > + if (ret == 0 && node->acpi.uid == uid)
> > break;
> > + if (ret == -ENODATA && node->acpi.uid == 0)
> > break;
>
> ?
>
> With that fixed,
>
> Reviewed-by: Ard Biesheuvel <[email protected]>
Thanks!
--
With Best Regards,
Andy Shevchenko