diff mbox series

[v2,2/2] ACPI: PM: Use max() for clearer D3 state selection in acpi_dev_pm_get_state

Message ID 20240626130941.1527127-3-prabhakar.pujeri@gmail.com (mailing list archive)
State Changes Requested, archived
Headers show
Series ACPI: Replace Ternary Operations with min()/max() Macros | expand

Commit Message

Prabhakar Pujeri June 26, 2024, 1:09 p.m. UTC 
Signed-off-by: Prabhakar Pujeri <prabhakar.pujeri@gmail.com>
---
 drivers/acpi/device_pm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

kernel test robot June 27, 2024, 12:18 p.m. UTC | #1 
Hi Prabhakar,

kernel test robot noticed the following build errors:

[auto build test ERROR on rafael-pm/linux-next]
[also build test ERROR on rafael-pm/bleeding-edge linus/master v6.10-rc5 next-20240626]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Prabhakar-Pujeri/ACPI-CPPC-Replace-ternary-operator-with-max-in-cppc_find_dmi_mhz/20240627-032938
base:   https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git linux-next
patch link:    https://lore.kernel.org/r/20240626130941.1527127-3-prabhakar.pujeri%40gmail.com
patch subject: [PATCH v2 2/2] ACPI: PM: Use max() for clearer D3 state selection in acpi_dev_pm_get_state
config: x86_64-rhel-8.3-rust (https://download.01.org/0day-ci/archive/20240627/202406271915.8djHC3jQ-lkp@intel.com/config)
compiler: clang version 18.1.5 (https://github.com/llvm/llvm-project 617a15a9eac96088ae5e9134248d8236e34b91b1)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240627/202406271915.8djHC3jQ-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202406271915.8djHC3jQ-lkp@intel.com/

All errors (new ones prefixed by >>):

>> drivers/acpi/device_pm.c:763:12: error: static assertion failed due to requirement '__builtin_choose_expr((sizeof(int) == sizeof (*(8 ? ((void *)((long)((((unsigned long long)(-1)) < (unsigned long long)1)) * 0L)) : (int *)8))), (((unsigned long long)(-1)) < (unsigned long long)1), 0) == __builtin_choose_expr((sizeof(int) == sizeof (*(8 ? ((void *)((long)((((int)(-1)) < (int)1)) * 0L)) : (int *)8))), (((int)(-1)) < (int)1), 0) || __builtin_choose_expr((sizeof(int) == sizeof (*(8 ? ((void *)((long)((((unsigned long long)(-1)) < (unsigned long long)1)) * 0L)) : (int *)8))), (((unsigned long long)(-1)) < (unsigned long long)1), 0) == __builtin_choose_expr((sizeof(int) == sizeof (*(8 ? ((void *)((long)((((int)(-1)) < (int)1)) * 0L)) : (int *)8))), (((int)(-1)) < (int)1), 0) || (__builtin_choose_expr((sizeof(int) == sizeof (*(8 ? ((void *)((long)(ret) * 0L)) : (int *)8))) && __builtin_choose_expr((sizeof(int) == sizeof (*(8 ? ((void *)((long)((((unsigned long long)(-1)) < (unsigned long long)1)) * 0L)) : (int *)8))), (((unsigned long long)(-1)) < (unsigned long long)1), 0), ret, -1) >= 0) || (__builtin_choose_expr((sizeof(int) == sizeof (*(8 ? ((void *)((long)(d_min) * 0L)) : (int *)8))) && __builtin_choose_expr((sizeof(int) == sizeof (*(8 ? ((void *)((long)((((int)(-1)) < (int)1)) * 0L)) : (int *)8))), (((int)(-1)) < (int)1), 0), d_min, -1) >= 0)': max(ret, d_min) signedness error, fix types or consider umax() before max_t()
     763 |                         d_max = max(ret, d_min);
         |                                 ^~~~~~~~~~~~~~~
   include/linux/minmax.h:92:19: note: expanded from macro 'max'
      92 | #define max(x, y)       __careful_cmp(max, x, y)
         |                         ^~~~~~~~~~~~~~~~~~~~~~~~
   include/linux/minmax.h:58:3: note: expanded from macro '__careful_cmp'
      58 |                 __cmp_once(op, x, y, __UNIQUE_ID(__x), __UNIQUE_ID(__y)))
         |                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   include/linux/minmax.h:51:16: note: expanded from macro '__cmp_once'
      51 |         static_assert(__types_ok(x, y),                 \
         |         ~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
      52 |                 #op "(" #x ", " #y ") signedness error, fix types or consider u" #op "() before " #op "_t()"); \
         |                 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   note: (skipping 1 expansions in backtrace; use -fmacro-backtrace-limit=0 to see all)
   include/linux/minmax.h:31:2: note: expanded from macro '__is_signed'
      31 |         __builtin_choose_expr(__is_constexpr(is_signed_type(typeof(x))),        \
         |         ^
   include/linux/build_bug.h:77:50: note: expanded from macro 'static_assert'
      77 | #define static_assert(expr, ...) __static_assert(expr, ##__VA_ARGS__, #expr)
         |                                  ~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~
   include/linux/build_bug.h:78:56: note: expanded from macro '__static_assert'
      78 | #define __static_assert(expr, msg, ...) _Static_assert(expr, msg)
         |                                                        ^~~~
   1 error generated.


vim +763 drivers/acpi/device_pm.c

   646	
   647	/**
   648	 * acpi_dev_pm_get_state - Get preferred power state of ACPI device.
   649	 * @dev: Device whose preferred target power state to return.
   650	 * @adev: ACPI device node corresponding to @dev.
   651	 * @target_state: System state to match the resultant device state.
   652	 * @d_min_p: Location to store the highest power state available to the device.
   653	 * @d_max_p: Location to store the lowest power state available to the device.
   654	 *
   655	 * Find the lowest power (highest number) and highest power (lowest number) ACPI
   656	 * device power states that the device can be in while the system is in the
   657	 * state represented by @target_state.  Store the integer numbers representing
   658	 * those stats in the memory locations pointed to by @d_max_p and @d_min_p,
   659	 * respectively.
   660	 *
   661	 * Callers must ensure that @dev and @adev are valid pointers and that @adev
   662	 * actually corresponds to @dev before using this function.
   663	 *
   664	 * Returns 0 on success or -ENODATA when one of the ACPI methods fails or
   665	 * returns a value that doesn't make sense.  The memory locations pointed to by
   666	 * @d_max_p and @d_min_p are only modified on success.
   667	 */
   668	static int acpi_dev_pm_get_state(struct device *dev, struct acpi_device *adev,
   669					 u32 target_state, int *d_min_p, int *d_max_p)
   670	{
   671		char method[] = { '_', 'S', '0' + target_state, 'D', '\0' };
   672		acpi_handle handle = adev->handle;
   673		unsigned long long ret;
   674		int d_min, d_max;
   675		bool wakeup = false;
   676		bool has_sxd = false;
   677		acpi_status status;
   678	
   679		/*
   680		 * If the system state is S0, the lowest power state the device can be
   681		 * in is D3cold, unless the device has _S0W and is supposed to signal
   682		 * wakeup, in which case the return value of _S0W has to be used as the
   683		 * lowest power state available to the device.
   684		 */
   685		d_min = ACPI_STATE_D0;
   686		d_max = ACPI_STATE_D3_COLD;
   687	
   688		/*
   689		 * If present, _SxD methods return the minimum D-state (highest power
   690		 * state) we can use for the corresponding S-states.  Otherwise, the
   691		 * minimum D-state is D0 (ACPI 3.x).
   692		 */
   693		if (target_state > ACPI_STATE_S0) {
   694			/*
   695			 * We rely on acpi_evaluate_integer() not clobbering the integer
   696			 * provided if AE_NOT_FOUND is returned.
   697			 */
   698			ret = d_min;
   699			status = acpi_evaluate_integer(handle, method, NULL, &ret);
   700			if ((ACPI_FAILURE(status) && status != AE_NOT_FOUND)
   701			    || ret > ACPI_STATE_D3_COLD)
   702				return -ENODATA;
   703	
   704			/*
   705			 * We need to handle legacy systems where D3hot and D3cold are
   706			 * the same and 3 is returned in both cases, so fall back to
   707			 * D3cold if D3hot is not a valid state.
   708			 */
   709			if (!adev->power.states[ret].flags.valid) {
   710				if (ret == ACPI_STATE_D3_HOT)
   711					ret = ACPI_STATE_D3_COLD;
   712				else
   713					return -ENODATA;
   714			}
   715	
   716			if (status == AE_OK)
   717				has_sxd = true;
   718	
   719			d_min = ret;
   720			wakeup = device_may_wakeup(dev) && adev->wakeup.flags.valid
   721				&& adev->wakeup.sleep_state >= target_state;
   722		} else if (device_may_wakeup(dev) && dev->power.wakeirq) {
   723			/*
   724			 * The ACPI subsystem doesn't manage the wake bit for IRQs
   725			 * defined with ExclusiveAndWake and SharedAndWake. Instead we
   726			 * expect them to be managed via the PM subsystem. Drivers
   727			 * should call dev_pm_set_wake_irq to register an IRQ as a wake
   728			 * source.
   729			 *
   730			 * If a device has a wake IRQ attached we need to check the
   731			 * _S0W method to get the correct wake D-state. Otherwise we
   732			 * end up putting the device into D3Cold which will more than
   733			 * likely disable wake functionality.
   734			 */
   735			wakeup = true;
   736		} else {
   737			/* ACPI GPE is specified in _PRW. */
   738			wakeup = adev->wakeup.flags.valid;
   739		}
   740	
   741		/*
   742		 * If _PRW says we can wake up the system from the target sleep state,
   743		 * the D-state returned by _SxD is sufficient for that (we assume a
   744		 * wakeup-aware driver if wake is set).  Still, if _SxW exists
   745		 * (ACPI 3.x), it should return the maximum (lowest power) D-state that
   746		 * can wake the system.  _S0W may be valid, too.
   747		 */
   748		if (wakeup) {
   749			method[3] = 'W';
   750			status = acpi_evaluate_integer(handle, method, NULL, &ret);
   751			if (status == AE_NOT_FOUND) {
   752				/* No _SxW. In this case, the ACPI spec says that we
   753				 * must not go into any power state deeper than the
   754				 * value returned from _SxD.
   755				 */
   756				if (has_sxd && target_state > ACPI_STATE_S0)
   757					d_max = d_min;
   758			} else if (ACPI_SUCCESS(status) && ret <= ACPI_STATE_D3_COLD) {
   759				/* Fall back to D3cold if ret is not a valid state. */
   760				if (!adev->power.states[ret].flags.valid)
   761					ret = ACPI_STATE_D3_COLD;
   762	
 > 763				d_max = max(ret, d_min);
   764			} else {
   765				return -ENODATA;
   766			}
   767		}
   768	
   769		if (d_min_p)
   770			*d_min_p = d_min;
   771	
   772		if (d_max_p)
   773			*d_max_p = d_max;
   774	
   775		return 0;
   776	}
   777
Rafael J. Wysocki July 1, 2024, 5:19 p.m. UTC | #2 
On Wed, Jun 26, 2024 at 3:14 PM Prabhakar Pujeri
<prabhakar.pujeri@gmail.com> wrote:
>
> Signed-off-by: Prabhakar Pujeri <prabhakar.pujeri@gmail.com>
> ---
>  drivers/acpi/device_pm.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/acpi/device_pm.c b/drivers/acpi/device_pm.c
> index 3b4d048c4941..a90ae059fb60 100644
> --- a/drivers/acpi/device_pm.c
> +++ b/drivers/acpi/device_pm.c
> @@ -760,7 +760,7 @@ static int acpi_dev_pm_get_state(struct device *dev, struct acpi_device *adev,
>                         if (!adev->power.states[ret].flags.valid)
>                                 ret = ACPI_STATE_D3_COLD;
>
> -                       d_max = ret > d_min ? ret : d_min;
> +                       d_max = max(ret, d_min);

You need to use max_t() here.

>                 } else {
>                         return -ENODATA;
>                 }
> --
diff mbox series

Patch

diff --git a/drivers/acpi/device_pm.c b/drivers/acpi/device_pm.c
index 3b4d048c4941..a90ae059fb60 100644
--- a/drivers/acpi/device_pm.c
+++ b/drivers/acpi/device_pm.c
@@ -760,7 +760,7 @@  static int acpi_dev_pm_get_state(struct device *dev, struct acpi_device *adev,
 			if (!adev->power.states[ret].flags.valid)
 				ret = ACPI_STATE_D3_COLD;
 
-			d_max = ret > d_min ? ret : d_min;
+			d_max = max(ret, d_min);
 		} else {
 			return -ENODATA;
 		}