* Re: [PATCH] platform/x86: drop unneeded assignment in host_control_smi()
2021-04-27 11:31 [PATCH] platform/x86: drop unneeded assignment in host_control_smi() Yang Li
@ 2021-04-27 17:09 ` kernel test robot
2021-05-11 11:10 ` Hans de Goede
1 sibling, 0 replies; 3+ messages in thread
From: kernel test robot @ 2021-04-27 17:09 UTC (permalink / raw)
To: Yang Li, stuart.w.hayes
Cc: kbuild-all, clang-built-linux, hdegoede, mgross,
platform-driver-x86, linux-kernel, Yang Li
[-- Attachment #1: Type: text/plain, Size: 12403 bytes --]
Hi Yang,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on linus/master]
[also build test WARNING on v5.12 next-20210427]
[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]
url: https://github.com/0day-ci/linux/commits/Yang-Li/platform-x86-drop-unneeded-assignment-in-host_control_smi/20210427-193333
base: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 4a0225c3d208cfa6e4550f2210ffd9114a952a81
config: x86_64-randconfig-r022-20210427 (attached as .config)
compiler: clang version 13.0.0 (https://github.com/llvm/llvm-project d7308da4a5aaded897a7e0c06e7e88d81fc64879)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# install x86_64 cross compiling tool for clang build
# apt-get install binutils-x86-64-linux-gnu
# https://github.com/0day-ci/linux/commit/b211940b2feb481f64f80b8de9fe1c2e6a9f2b56
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Yang-Li/platform-x86-drop-unneeded-assignment-in-host_control_smi/20210427-193333
git checkout b211940b2feb481f64f80b8de9fe1c2e6a9f2b56
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 ARCH=x86_64
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
>> drivers/platform/x86/dell/dcdbas.c:398:10: warning: result of comparison of constant -1 with expression of type 'unsigned char' is always false [-Wtautological-constant-out-of-range-compare]
== ESM_STATUS_CMD_UNSUCCESSFUL) {
^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~
1 warning generated.
vim +398 drivers/platform/x86/dell/dcdbas.c
90563ec4129f14 drivers/firmware/dcdbas.c Doug Warzecha 2005-09-06 355
90563ec4129f14 drivers/firmware/dcdbas.c Doug Warzecha 2005-09-06 356 /**
90563ec4129f14 drivers/firmware/dcdbas.c Doug Warzecha 2005-09-06 357 * host_control_smi: generate host control SMI
90563ec4129f14 drivers/firmware/dcdbas.c Doug Warzecha 2005-09-06 358 *
90563ec4129f14 drivers/firmware/dcdbas.c Doug Warzecha 2005-09-06 359 * Caller must set up the host control command in smi_data_buf.
90563ec4129f14 drivers/firmware/dcdbas.c Doug Warzecha 2005-09-06 360 */
90563ec4129f14 drivers/firmware/dcdbas.c Doug Warzecha 2005-09-06 361 static int host_control_smi(void)
90563ec4129f14 drivers/firmware/dcdbas.c Doug Warzecha 2005-09-06 362 {
90563ec4129f14 drivers/firmware/dcdbas.c Doug Warzecha 2005-09-06 363 struct apm_cmd *apm_cmd;
90563ec4129f14 drivers/firmware/dcdbas.c Doug Warzecha 2005-09-06 364 u8 *data;
90563ec4129f14 drivers/firmware/dcdbas.c Doug Warzecha 2005-09-06 365 unsigned long flags;
90563ec4129f14 drivers/firmware/dcdbas.c Doug Warzecha 2005-09-06 366 u32 num_ticks;
90563ec4129f14 drivers/firmware/dcdbas.c Doug Warzecha 2005-09-06 367 s8 cmd_status;
90563ec4129f14 drivers/firmware/dcdbas.c Doug Warzecha 2005-09-06 368 u8 index;
90563ec4129f14 drivers/firmware/dcdbas.c Doug Warzecha 2005-09-06 369
90563ec4129f14 drivers/firmware/dcdbas.c Doug Warzecha 2005-09-06 370 apm_cmd = (struct apm_cmd *)smi_data_buf;
90563ec4129f14 drivers/firmware/dcdbas.c Doug Warzecha 2005-09-06 371 apm_cmd->status = ESM_STATUS_CMD_UNSUCCESSFUL;
90563ec4129f14 drivers/firmware/dcdbas.c Doug Warzecha 2005-09-06 372
90563ec4129f14 drivers/firmware/dcdbas.c Doug Warzecha 2005-09-06 373 switch (host_control_smi_type) {
90563ec4129f14 drivers/firmware/dcdbas.c Doug Warzecha 2005-09-06 374 case HC_SMITYPE_TYPE1:
90563ec4129f14 drivers/firmware/dcdbas.c Doug Warzecha 2005-09-06 375 spin_lock_irqsave(&rtc_lock, flags);
90563ec4129f14 drivers/firmware/dcdbas.c Doug Warzecha 2005-09-06 376 /* write SMI data buffer physical address */
90563ec4129f14 drivers/firmware/dcdbas.c Doug Warzecha 2005-09-06 377 data = (u8 *)&smi_data_buf_phys_addr;
90563ec4129f14 drivers/firmware/dcdbas.c Doug Warzecha 2005-09-06 378 for (index = PE1300_CMOS_CMD_STRUCT_PTR;
90563ec4129f14 drivers/firmware/dcdbas.c Doug Warzecha 2005-09-06 379 index < (PE1300_CMOS_CMD_STRUCT_PTR + 4);
90563ec4129f14 drivers/firmware/dcdbas.c Doug Warzecha 2005-09-06 380 index++, data++) {
90563ec4129f14 drivers/firmware/dcdbas.c Doug Warzecha 2005-09-06 381 outb(index,
90563ec4129f14 drivers/firmware/dcdbas.c Doug Warzecha 2005-09-06 382 (CMOS_BASE_PORT + CMOS_PAGE2_INDEX_PORT_PIIX4));
90563ec4129f14 drivers/firmware/dcdbas.c Doug Warzecha 2005-09-06 383 outb(*data,
90563ec4129f14 drivers/firmware/dcdbas.c Doug Warzecha 2005-09-06 384 (CMOS_BASE_PORT + CMOS_PAGE2_DATA_PORT_PIIX4));
90563ec4129f14 drivers/firmware/dcdbas.c Doug Warzecha 2005-09-06 385 }
90563ec4129f14 drivers/firmware/dcdbas.c Doug Warzecha 2005-09-06 386
90563ec4129f14 drivers/firmware/dcdbas.c Doug Warzecha 2005-09-06 387 /* first set status to -1 as called by spec */
90563ec4129f14 drivers/firmware/dcdbas.c Doug Warzecha 2005-09-06 388 cmd_status = ESM_STATUS_CMD_UNSUCCESSFUL;
90563ec4129f14 drivers/firmware/dcdbas.c Doug Warzecha 2005-09-06 389 outb((u8) cmd_status, PCAT_APM_STATUS_PORT);
90563ec4129f14 drivers/firmware/dcdbas.c Doug Warzecha 2005-09-06 390
90563ec4129f14 drivers/firmware/dcdbas.c Doug Warzecha 2005-09-06 391 /* generate SMM call */
90563ec4129f14 drivers/firmware/dcdbas.c Doug Warzecha 2005-09-06 392 outb(ESM_APM_CMD, PCAT_APM_CONTROL_PORT);
90563ec4129f14 drivers/firmware/dcdbas.c Doug Warzecha 2005-09-06 393 spin_unlock_irqrestore(&rtc_lock, flags);
90563ec4129f14 drivers/firmware/dcdbas.c Doug Warzecha 2005-09-06 394
90563ec4129f14 drivers/firmware/dcdbas.c Doug Warzecha 2005-09-06 395 /* wait a few to see if it executed */
90563ec4129f14 drivers/firmware/dcdbas.c Doug Warzecha 2005-09-06 396 num_ticks = TIMEOUT_USEC_SHORT_SEMA_BLOCKING;
b211940b2feb48 drivers/platform/x86/dell/dcdbas.c Yang Li 2021-04-27 397 while (inb(PCAT_APM_STATUS_PORT)
90563ec4129f14 drivers/firmware/dcdbas.c Doug Warzecha 2005-09-06 @398 == ESM_STATUS_CMD_UNSUCCESSFUL) {
90563ec4129f14 drivers/firmware/dcdbas.c Doug Warzecha 2005-09-06 399 num_ticks--;
90563ec4129f14 drivers/firmware/dcdbas.c Doug Warzecha 2005-09-06 400 if (num_ticks == EXPIRED_TIMER)
90563ec4129f14 drivers/firmware/dcdbas.c Doug Warzecha 2005-09-06 401 return -ETIME;
90563ec4129f14 drivers/firmware/dcdbas.c Doug Warzecha 2005-09-06 402 }
90563ec4129f14 drivers/firmware/dcdbas.c Doug Warzecha 2005-09-06 403 break;
90563ec4129f14 drivers/firmware/dcdbas.c Doug Warzecha 2005-09-06 404
90563ec4129f14 drivers/firmware/dcdbas.c Doug Warzecha 2005-09-06 405 case HC_SMITYPE_TYPE2:
90563ec4129f14 drivers/firmware/dcdbas.c Doug Warzecha 2005-09-06 406 case HC_SMITYPE_TYPE3:
90563ec4129f14 drivers/firmware/dcdbas.c Doug Warzecha 2005-09-06 407 spin_lock_irqsave(&rtc_lock, flags);
90563ec4129f14 drivers/firmware/dcdbas.c Doug Warzecha 2005-09-06 408 /* write SMI data buffer physical address */
90563ec4129f14 drivers/firmware/dcdbas.c Doug Warzecha 2005-09-06 409 data = (u8 *)&smi_data_buf_phys_addr;
90563ec4129f14 drivers/firmware/dcdbas.c Doug Warzecha 2005-09-06 410 for (index = PE1400_CMOS_CMD_STRUCT_PTR;
90563ec4129f14 drivers/firmware/dcdbas.c Doug Warzecha 2005-09-06 411 index < (PE1400_CMOS_CMD_STRUCT_PTR + 4);
90563ec4129f14 drivers/firmware/dcdbas.c Doug Warzecha 2005-09-06 412 index++, data++) {
90563ec4129f14 drivers/firmware/dcdbas.c Doug Warzecha 2005-09-06 413 outb(index, (CMOS_BASE_PORT + CMOS_PAGE1_INDEX_PORT));
90563ec4129f14 drivers/firmware/dcdbas.c Doug Warzecha 2005-09-06 414 outb(*data, (CMOS_BASE_PORT + CMOS_PAGE1_DATA_PORT));
90563ec4129f14 drivers/firmware/dcdbas.c Doug Warzecha 2005-09-06 415 }
90563ec4129f14 drivers/firmware/dcdbas.c Doug Warzecha 2005-09-06 416
90563ec4129f14 drivers/firmware/dcdbas.c Doug Warzecha 2005-09-06 417 /* generate SMM call */
90563ec4129f14 drivers/firmware/dcdbas.c Doug Warzecha 2005-09-06 418 if (host_control_smi_type == HC_SMITYPE_TYPE3)
90563ec4129f14 drivers/firmware/dcdbas.c Doug Warzecha 2005-09-06 419 outb(ESM_APM_CMD, PCAT_APM_CONTROL_PORT);
90563ec4129f14 drivers/firmware/dcdbas.c Doug Warzecha 2005-09-06 420 else
90563ec4129f14 drivers/firmware/dcdbas.c Doug Warzecha 2005-09-06 421 outb(ESM_APM_CMD, PE1400_APM_CONTROL_PORT);
90563ec4129f14 drivers/firmware/dcdbas.c Doug Warzecha 2005-09-06 422
90563ec4129f14 drivers/firmware/dcdbas.c Doug Warzecha 2005-09-06 423 /* restore RTC index pointer since it was written to above */
90563ec4129f14 drivers/firmware/dcdbas.c Doug Warzecha 2005-09-06 424 CMOS_READ(RTC_REG_C);
90563ec4129f14 drivers/firmware/dcdbas.c Doug Warzecha 2005-09-06 425 spin_unlock_irqrestore(&rtc_lock, flags);
90563ec4129f14 drivers/firmware/dcdbas.c Doug Warzecha 2005-09-06 426
90563ec4129f14 drivers/firmware/dcdbas.c Doug Warzecha 2005-09-06 427 /* read control port back to serialize write */
90563ec4129f14 drivers/firmware/dcdbas.c Doug Warzecha 2005-09-06 428 cmd_status = inb(PE1400_APM_CONTROL_PORT);
90563ec4129f14 drivers/firmware/dcdbas.c Doug Warzecha 2005-09-06 429
90563ec4129f14 drivers/firmware/dcdbas.c Doug Warzecha 2005-09-06 430 /* wait a few to see if it executed */
90563ec4129f14 drivers/firmware/dcdbas.c Doug Warzecha 2005-09-06 431 num_ticks = TIMEOUT_USEC_SHORT_SEMA_BLOCKING;
90563ec4129f14 drivers/firmware/dcdbas.c Doug Warzecha 2005-09-06 432 while (apm_cmd->status == ESM_STATUS_CMD_UNSUCCESSFUL) {
90563ec4129f14 drivers/firmware/dcdbas.c Doug Warzecha 2005-09-06 433 num_ticks--;
90563ec4129f14 drivers/firmware/dcdbas.c Doug Warzecha 2005-09-06 434 if (num_ticks == EXPIRED_TIMER)
90563ec4129f14 drivers/firmware/dcdbas.c Doug Warzecha 2005-09-06 435 return -ETIME;
90563ec4129f14 drivers/firmware/dcdbas.c Doug Warzecha 2005-09-06 436 }
90563ec4129f14 drivers/firmware/dcdbas.c Doug Warzecha 2005-09-06 437 break;
90563ec4129f14 drivers/firmware/dcdbas.c Doug Warzecha 2005-09-06 438
90563ec4129f14 drivers/firmware/dcdbas.c Doug Warzecha 2005-09-06 439 default:
90563ec4129f14 drivers/firmware/dcdbas.c Doug Warzecha 2005-09-06 440 dev_dbg(&dcdbas_pdev->dev, "%s: invalid SMI type %u\n",
eecd58536a9750 drivers/firmware/dcdbas.c Harvey Harrison 2008-04-29 441 __func__, host_control_smi_type);
90563ec4129f14 drivers/firmware/dcdbas.c Doug Warzecha 2005-09-06 442 return -ENOSYS;
90563ec4129f14 drivers/firmware/dcdbas.c Doug Warzecha 2005-09-06 443 }
90563ec4129f14 drivers/firmware/dcdbas.c Doug Warzecha 2005-09-06 444
90563ec4129f14 drivers/firmware/dcdbas.c Doug Warzecha 2005-09-06 445 return 0;
90563ec4129f14 drivers/firmware/dcdbas.c Doug Warzecha 2005-09-06 446 }
90563ec4129f14 drivers/firmware/dcdbas.c Doug Warzecha 2005-09-06 447
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 33906 bytes --]
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [PATCH] platform/x86: drop unneeded assignment in host_control_smi()
2021-04-27 11:31 [PATCH] platform/x86: drop unneeded assignment in host_control_smi() Yang Li
2021-04-27 17:09 ` kernel test robot
@ 2021-05-11 11:10 ` Hans de Goede
1 sibling, 0 replies; 3+ messages in thread
From: Hans de Goede @ 2021-05-11 11:10 UTC (permalink / raw)
To: Yang Li, stuart.w.hayes; +Cc: mgross, platform-driver-x86, linux-kernel
Hi Yang Li,
On 4/27/21 1:31 PM, Yang Li wrote:
> Making '==' operation with ESM_STATUS_CMD_UNSUCCESSFUL directly
> after calling the function inb() is more efficient, so assignment
> to 'cmd_status' is redundant.
>
> Eliminate the following clang_analyzer warning:
> drivers/platform/x86/dell/dcdbas.c:397:11: warning: Although the value
> stored to 'cmd_status' is used in the enclosing expression, the value is
> never actually read from 'cmd_status'
>
> No functional change.
>
> Reported-by: Abaci Robot <abaci@linux.alibaba.com>
> Signed-off-by: Yang Li <yang.lee@linux.alibaba.com>
Thank you for your patch, but as the "kernel test robot <lkp@intel.com>"
reported, this actually breaks the check in the while loop.
cmd_status is a s8 and ESM_STATUS_CMD_UNSUCCESSFUL is defined as -1.
By dropping the intermediate step of storing the inb() value into the
s8, we end up comparing the inb() unsigned result directly to -1 which
is never true.
A possible way to fix this (without reworking the rest of the code) would
be to either cast the inb() result to a s8, so that you end up with this:
while ((s8)inb(PCAT_APM_STATUS_PORT) == ESM_STATUS_CMD_UNSUCCESSFUL) {
Also while at it please change the condition to a single line as I did
above.
Thanks & Regards,
Hans
> ---
> drivers/platform/x86/dell/dcdbas.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/platform/x86/dell/dcdbas.c b/drivers/platform/x86/dell/dcdbas.c
> index d513a59..a9e8a88 100644
> --- a/drivers/platform/x86/dell/dcdbas.c
> +++ b/drivers/platform/x86/dell/dcdbas.c
> @@ -394,7 +394,7 @@ static int host_control_smi(void)
>
> /* wait a few to see if it executed */
> num_ticks = TIMEOUT_USEC_SHORT_SEMA_BLOCKING;
> - while ((cmd_status = inb(PCAT_APM_STATUS_PORT))
> + while (inb(PCAT_APM_STATUS_PORT)
> == ESM_STATUS_CMD_UNSUCCESSFUL) {
> num_ticks--;
> if (num_ticks == EXPIRED_TIMER)
>
^ permalink raw reply [flat|nested] 3+ messages in thread