Re: [LTP] [PATCH] device-drivers/acpi/ltp_acpi_cmds: Fix build errors

[Petr Vorel <pvorel@suse.cz>]

Hi Tiezhu,

> On 2025/7/17 下午10:09, Petr Vorel wrote:
> > Hi Tiezhu, all,

> ...

> > Due the above could you please take the approach Ricardo did in 82e38a1f24
> > ("block_dev: Convert to new API") - wrap with ifndef?

> > #ifndef DISK_NAME_LEN
> > # include <linux/genhd.h>
> > #endif

> > BTW I would personally use #ifndef HAVE_LINUX_BLKDEV_H than checking for
> > DISK_NAME_LEN as we already check for linux/blkdev.h in configure.ac, but that's
> > a minor detail.

> I think use "#ifndef DISK_NAME_LEN" is proper.

I'm sorry, I meant #ifdef HAVE_LINUX_GENHD_H (that is what is being checked in
configure.ac).

> Because both genhd.h and blkdev.h are exist before the kernel
> commit 322cbb50de71 ("block: remove genhd.h"), HAVE_LINUX_BLKDEV_H
> seems always define as 1 for the new and old kernel versions.

> But the definition DISK_NAME_LEN was moved from genhd.h into blkdev.h
> after that commit, because blkdev.h is included first, so we can check
> DISK_NAME_LEN, it should include genhd.h ifndef DISK_NAME_LEN.

Sure, this works now and it's good enough. The reason I would personally depend
on header based check #ifdef HAVE_LINUX_GENHD_H is that if kernel devs are ok to
remove whole header they can of course also rename or completely remove
DISK_NAME_LEN definition.

> > Yes we need to #if #else macros for acpi_bus_get_device() vs.
> > acpi_fetch_acpi_dev().

> Here is a draft change, I will post a formal v2 patch if you are OK.

> ----->8-----
> diff --git a/configure.ac b/configure.ac
> index 11e599a81..1f6e2b1b9 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -100,6 +100,7 @@ AC_SUBST(HAVE_FTS_H, $have_fts)
>  AC_CHECK_HEADERS(linux/vm_sockets.h, [], [], [#include <sys/socket.h>])

>  AC_CHECK_FUNCS_ONCE([ \
> +    acpi_bus_get_device \

I'm sorry this will not work. AC_CHECK_FUNCS_ONCE() works only for userspace
functions (and require AC_CHECK_HEADERS() with the userspace header to be passed
before).

But as I wrote before acpi_bus_get_device() and acpi_fetch_acpi_dev() is only in
kernel source drivers/acpi/scan.c (+ prototype is only in kernel only header
include/acpi/acpi_bus.h but it would not help).

We have AC_COMPILE_IFELSE(), which can be used for detecting static inline UAPI
functions, e.g. 

AC_COMPILE_IFELSE(
				  [AC_LANG_PROGRAM(
				   [[
					#include <linux/dccp.h>
					#include <stddef.h>
					]],
				   [[
					struct dccp_hdr *dh;
					(void)dccp_packet_hdr_len(dh->dccph_type);
					]]
				   )],
				   [AC_DEFINE([HAVE_DCCP_PACKET_HDR_LEN], [1], [Define if dccp_packet_hdr_len is available])],
				   []
				   )
But again, this is not the case of acpi_bus_get_device() nor
acpi_fetch_acpi_dev().

Can we get back to 5.18 based check?

Given the timeline I wrote above this should be sufficient:

#ifdef HAVE_LINUX_GENHD_H
=> acpi_fetch_acpi_dev()
#else
=> acpi_bus_get_device()
#endif

But probably the best way would be to check against 5.18:

#include <linux/version.h>
#if LINUX_VERSION_CODE < KERNEL_VERSION(5, 18, 0)
=> acpi_fetch_acpi_dev()
#else
=> acpi_bus_get_device()
#endif

>      cachestat \
>      clone3 \
>      close_range \
> diff --git a/testcases/kernel/device-drivers/acpi/ltp_acpi_cmds.c
> b/testcases/kernel/device-drivers/acpi/ltp_acpi_cmds.c
> index d12dd6b94..f68014732 100644
> --- a/testcases/kernel/device-drivers/acpi/ltp_acpi_cmds.c
> +++ b/testcases/kernel/device-drivers/acpi/ltp_acpi_cmds.c
> @@ -36,7 +36,9 @@
>  #include <linux/ioctl.h>
>  #include <linux/pm.h>
>  #include <linux/acpi.h>
> +#ifndef DISK_NAME_LEN
>  #include <linux/genhd.h>
> +#endif
>  #include <linux/dmi.h>
>  #include <linux/nls.h>

> @@ -123,14 +125,20 @@ static void get_crs_object(acpi_handle handle)

>  static void get_sysfs_path(acpi_handle handle)
>  {
> -       acpi_status status;
>         struct acpi_device *device;

>         kfree(sysfs_path);
>         sysfs_path = NULL;

> +#ifdef HAVE_ACPI_BUS_GET_DEVICE
> +       acpi_status status;
> +
>         status = acpi_bus_get_device(handle, &device);
>         if (ACPI_SUCCESS(status))
> +#else
> +       device = acpi_fetch_acpi_dev(handle);
> +       if (device)
> +#endif
>                 sysfs_path = kobject_get_path(&device->dev.kobj,
> GFP_KERNEL);
>  }

> @@ -398,9 +406,15 @@ static int acpi_test_bus(void)
>         if (acpi_failure(status, "acpi_get_handle"))
>                 return 1;

> +#ifdef HAVE_ACPI_BUS_GET_DEVICE
>         prk_alert("TEST -- acpi_bus_get_device");
>         status = acpi_bus_get_device(bus_handle, &device);
>         if (acpi_failure(status, "acpi_bus_get_device"))
> +#else
> +       prk_alert("TEST -- acpi_fetch_acpi_dev");
> +       device = acpi_fetch_acpi_dev(bus_handle);
> +       if (!device)
> +#endif
>                 return 1;

>         prk_alert("TEST -- acpi_bus_update_power ");

Please while you're at it remove trailing whitespace:
          prk_alert("TEST -- acpi_bus_update_power");

The rest LGTM.

Kind regards,
Petr

> Thanks,
> Tiezhu

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp