mirror of https://lore.kernel.org/lkml/
 help / color / mirror / Atom feed
From: "Derek J. Clark" <derekjohn.clark@gmail.com>
To: Yufei CHENG <cd345al@gmail.com>, mpearson-lenovo@squebb.ca, i@rong.moe
Cc: hansg@kernel.org, ilpo.jarvinen@linux.intel.com,
	platform-driver-x86@vger.kernel.org,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2] platform/x86: lenovo: wmi-other: Fix uninitialized
Date: Sun, 26 Apr 2026 11:22:04 -0700	[thread overview]
Message-ID: <F095C80C-C233-4A50-ADC2-EC932563E1EC@gmail.com> (raw)
In-Reply-To: <20260426174332.9981-1-cd345al@gmail.com>

On April 26, 2026 10:43:32 AM PDT, Yufei CHENG <cd345al@gmail.com> wrote:
>Hi Rong,
>
>Thanks for your feedback, I've corrected the format and resend the patch.
>However I think I have to point out some other issues in this file.
>
>First, we should not complexify simple things. You use lwmi_om_fan_get_set() to
>combine get and set into a single function with a boolean flag to determine the
>action.
>This makes the control flow harder to follow and audit. Furthermore, this layer
>of abstraction is not applied constantly throughout the whole file. In some
>places, values are set through lwmi_om_fan_get_set() and in other places,
>they're called manually through lwmi_dev_evaluate_int()
>This inconsistency makes the abstraction pointless and is the direct cause of a
>second uninitialized bug
>
>In line 855, args.arg0 is set to attribute_id, but arg1 is never set. Though
>arg1 is omitted in read action, you're still passing stack garbage to the
>lwmi_dev_evaluate_int(). And I think it's better to zero it out if not used.

Hi Yufei,

This bug is already addressed in a patch series that has been submitted and is awaiting review from the subsystem maintainers. 

>Additionally, I doubt whether bypassing the minimum RPM divisor has any
>practical use at all, not to mention it's not guranteed to work and purely
>depends on whether EC accepts that sub-division value.
>
>I may do some refactor recently, and I'm happy to hear your suggestions.

Rong has a large refactor series queued up to simplify much of the driver already, pending the acceptance of the previously mentioned series. It would be best to avoid doing any additional large refactoring until that series has landed.

Thanks,
Derek

>Thanks,
>Yufei
>


  reply	other threads:[~2026-04-26 18:22 UTC|newest]

Thread overview: 9+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2026-04-26 14:59 [PATCH v2] platform/x86: lenovo: wmi-other: Fix uninitialized variable in lwmi_om_hwmon_write() Yufei CHENG
2026-04-26 16:07 ` Rong Zhang
2026-04-26 17:43   ` [PATCH v2] platform/x86: lenovo: wmi-other: Fix uninitialized Yufei CHENG
2026-04-26 18:22     ` Derek J. Clark [this message]
2026-04-26 18:38       ` no name
2026-04-26 18:47         ` Derek J. Clark
2026-04-27  1:14           ` no name
2026-04-26 16:50 ` [PATCH v3] platform/x86: lenovo: wmi-other: Fix uninitialized variable in lwmi_om_hwmon_write() Yufei CHENG
2026-04-30 13:54   ` Ilpo Järvinen

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=F095C80C-C233-4A50-ADC2-EC932563E1EC@gmail.com \
    --to=derekjohn.clark@gmail.com \
    --cc=cd345al@gmail.com \
    --cc=hansg@kernel.org \
    --cc=i@rong.moe \
    --cc=ilpo.jarvinen@linux.intel.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mpearson-lenovo@squebb.ca \
    --cc=platform-driver-x86@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox

all inboxes | Powered by JetHome®