LinuxLists.cc - [PATCH] platform/x86: drop unneeded assignment in host_control

2021-04-27 11:35:29

Subject: [PATCH] platform/x86: drop unneeded assignment in host_control_smi()

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 <[email protected]>
Signed-off-by: Yang Li <[email protected]>
---
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)
--
1.8.3.1

2021-04-27 17:11:04

by kernel test robot

[permalink] [raw]

Subject: Re: [PATCH] platform/x86: drop unneeded assignment in host_control_smi()

Hi Yang,

Thank you for the patch! Perhaps
[auto build test WARNING on linus/master]
# install x86_64 cross # apt-get install binutils-x86-6 # /> next-20210427]
wrong git tree, kindly drop us a note.
to use '--base' as documented in
ormat-patch]">https://git-scm.com/docs/git-format-patch]
y-ci/linux/commits/Yang-Li/platform-x86-drop-unneeded-assignment-in-host_control_smi/20210427-193333">https://github.com/0day-ci/linux/commits/Yang-Li/platform-x86-drop-unneeded-assignment-in-host_control_smi/20210427-193333
/pub/scm/linux/kernel/git/torvalds/linux.git">https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 4a0225c3d208cfa6e4550f2210ffd9114a952a81
(attached as .config)
href="https://github.com/llvm/llvm-project">https://github.com/llvm/llvm-project d7308da4a5aaded897a7e0c06e7e88d81fc64879)
busercontent.com/intel/lkp-tests/master/sbin/make.cross">https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
compiling tool for clang build
4-linux-gnu
day-ci/linux/commit/b211940b2feb481f64f80b8de9fe1c2e6a9f2b56">https://github.com/0day-ci/linux/commit/b211940b2feb481f64f80b8de9fe1c2e6a9f2b56
href="https://github.com/0day-ci/linux">https://github.com/0day-ci/linux
Yang-Li/platform-x86-drop-unneeded-assignment-in-host_control_smi/20210427-193333
80b8de9fe1c2e6a9f2b56
to linux build tree
COMPILER=clang make.cross W=1 ARCH=x86_64
following tag as appropriate
<[email protected]>
by >>):
warning: result of comparison of constant -1 with expression of type 'unsigned char' is always false [-Wtautological-constant-out-of-range-compare]
CMD_UNSUCCESSFUL) {
~~~~~~~~~~~~~~~~
/> Doug Warzecha 2005-09-06 355
Doug Warzecha 2005-09-06 356 /**
Doug Warzecha 2005-09-06 357 * host_control_smi: generate host control SMI
Doug Warzecha 2005-09-06 358 *
Doug Warzecha 2005-09-06 359 * Caller must set up the host control command in smi_data_buf.
Doug Warzecha 2005-09-06 360 */
Doug Warzecha 2005-09-06 361 static int host_control_smi(void)
Doug Warzecha 2005-09-06 362 {
Doug Warzecha 2005-09-06 363 struct apm_cmd *apm_cmd;
Doug Warzecha 2005-09-06 364 u8 *data;
Doug Warzecha 2005-09-06 365 unsigned long flags;
Doug Warzecha 2005-09-06 366 u32 num_ticks;
Doug Warzecha 2005-09-06 367 s8 cmd_status;
Doug Warzecha 2005-09-06 368 u8 index;
Doug Warzecha 2005-09-06 369
Doug Warzecha 2005-09-06 370 apm_cmd = (struct apm_cmd *)smi_data_buf;
Doug Warzecha 2005-09-06 371 apm_cmd->status = ESM_STATUS_CMD_UNSUCCESSFUL;
Doug Warzecha 2005-09-06 372
Doug Warzecha 2005-09-06 373 switch (host_control_smi_type) {
Doug Warzecha 2005-09-06 374 case HC_SMITYPE_TYPE1:
Doug Warzecha 2005-09-06 375 spin_lock_irqsave(&rtc_lock, flags);
Doug Warzecha 2005-09-06 376 /* write SMI data buffer physical address */
Doug Warzecha 2005-09-06 377 data = (u8 *)&smi_data_buf_phys_addr;
Doug Warzecha 2005-09-06 378 for (index = PE1300_CMOS_CMD_STRUCT_PTR;
Doug Warzecha 2005-09-06 379 index < (PE1300_CMOS_CMD_STRUCT_PTR + 4);
Doug Warzecha 2005-09-06 380 index++, data++) {
Doug Warzecha 2005-09-06 381 outb(index,
Doug Warzecha 2005-09-06 382 (CMOS_BASE_PORT + CMOS_PAGE2_INDEX_PORT_PIIX4));
Doug Warzecha 2005-09-06 383 outb(*data,
Doug Warzecha 2005-09-06 384 (CMOS_BASE_PORT + CMOS_PAGE2_DATA_PORT_PIIX4));
Doug Warzecha 2005-09-06 385 }
Doug Warzecha 2005-09-06 386
Doug Warzecha 2005-09-06 387 /* first set status to -1 as called by spec */
Doug Warzecha 2005-09-06 388 cmd_status = ESM_STATUS_CMD_UNSUCCESSFUL;
Doug Warzecha 2005-09-06 389 outb((u8) cmd_status, PCAT_APM_STATUS_PORT);
Doug Warzecha 2005-09-06 390
Doug Warzecha 2005-09-06 391 /* generate SMM call */
Doug Warzecha 2005-09-06 392 outb(ESM_APM_CMD, PCAT_APM_CONTROL_PORT);
Doug Warzecha 2005-09-06 393 spin_unlock_irqrestore(&rtc_lock, flags);
Doug Warzecha 2005-09-06 394
Doug Warzecha 2005-09-06 395 /* wait a few to see if it executed */
Doug Warzecha 2005-09-06 396 num_ticks = TIMEOUT_USEC_SHORT_SEMA_BLOCKING;
Yang Li 2021-04-27 397 while (inb(PCAT_APM_STATUS_PORT)
Doug Warzecha 2005-09-06 @398 == ESM_STATUS_CMD_UNSUCCESSFUL) {
Doug Warzecha 2005-09-06 399 num_ticks--;
Doug Warzecha 2005-09-06 400 if (num_ticks == EXPIRED_TIMER)
Doug Warzecha 2005-09-06 401 return -ETIME;
Doug Warzecha 2005-09-06 402 }
Doug Warzecha 2005-09-06 403 break;
Doug Warzecha 2005-09-06 404
Doug Warzecha 2005-09-06 405 case HC_SMITYPE_TYPE2:
Doug Warzecha 2005-09-06 406 case HC_SMITYPE_TYPE3:
Doug Warzecha 2005-09-06 407 spin_lock_irqsave(&rtc_lock, flags);
Doug Warzecha 2005-09-06 408 /* write SMI data buffer physical address */
Doug Warzecha 2005-09-06 409 data = (u8 *)&smi_data_buf_phys_addr;
Doug Warzecha 2005-09-06 410 for (index = PE1400_CMOS_CMD_STRUCT_PTR;
Doug Warzecha 2005-09-06 411 index < (PE1400_CMOS_CMD_STRUCT_PTR + 4);
Doug Warzecha 2005-09-06 412 index++, data++) {
Doug Warzecha 2005-09-06 413 outb(index, (CMOS_BASE_PORT + CMOS_PAGE1_INDEX_PORT));
Doug Warzecha 2005-09-06 414 outb(*data, (CMOS_BASE_PORT + CMOS_PAGE1_DATA_PORT));
Doug Warzecha 2005-09-06 415 }
Doug Warzecha 2005-09-06 416
Doug Warzecha 2005-09-06 417 /* generate SMM call */
Doug Warzecha 2005-09-06 418 if (host_control_smi_type == HC_SMITYPE_TYPE3)
Doug Warzecha 2005-09-06 419 outb(ESM_APM_CMD, PCAT_APM_CONTROL_PORT);
Doug Warzecha 2005-09-06 420 else
Doug Warzecha 2005-09-06 421 outb(ESM_APM_CMD, PE1400_APM_CONTROL_PORT);
Doug Warzecha 2005-09-06 422
Doug Warzecha 2005-09-06 423 /* restore RTC index pointer since it was written to above */
Doug Warzecha 2005-09-06 424 CMOS_READ(RTC_REG_C);
Doug Warzecha 2005-09-06 425 spin_unlock_irqrestore(&rtc_lock, flags);
Doug Warzecha 2005-09-06 426
Doug Warzecha 2005-09-06 427 /* read control port back to serialize write */
Doug Warzecha 2005-09-06 428 cmd_status = inb(PE1400_APM_CONTROL_PORT);
Doug Warzecha 2005-09-06 429
Doug Warzecha 2005-09-06 430 /* wait a few to see if it executed */
Doug Warzecha 2005-09-06 431 num_ticks = TIMEOUT_USEC_SHORT_SEMA_BLOCKING;
Doug Warzecha 2005-09-06 432 while (apm_cmd->status == ESM_STATUS_CMD_UNSUCCESSFUL) {
Doug Warzecha 2005-09-06 433 num_ticks--;
Doug Warzecha 2005-09-06 434 if (num_ticks == EXPIRED_TIMER)
Doug Warzecha 2005-09-06 435 return -ETIME;
Doug Warzecha 2005-09-06 436 }
Doug Warzecha 2005-09-06 437 break;
Doug Warzecha 2005-09-06 438
Doug Warzecha 2005-09-06 439 default:
Doug Warzecha 2005-09-06 440 dev_dbg(&dcdbas_pdev->dev, "%s: invalid SMI type %u\n",
Harvey Harrison 2008-04-29 441 __func__, host_control_smi_type);
Doug Warzecha 2005-09-06 442 return -ENOSYS;
Doug Warzecha 2005-09-06 443 }
Doug Warzecha 2005-09-06 444
Doug Warzecha 2005-09-06 445 return 0;
Doug Warzecha 2005-09-06 446 }
Doug Warzecha 2005-09-06 447
Corporation
y/list/kbuild-all@lists.01.org">https://lists.01.org/hyperkitty/list/[email protected]

Attachments:

(No filename) (12.25 kB)
.config.gz (33.11 kB)
Download all attachments

2021-05-11 11:12:16

by Hans de Goede

[permalink] [raw]

Subject: Re: [PATCH] platform/x86: drop unneeded assignment in host_control_smi()

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 <[email protected]>
> Signed-off-by: Yang Li <[email protected]>

Thank you for your patch, but as the "kernel test robot <[email protected]>"
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)
>