| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAMEXYWfXFFyzQ65tGsBjYkecFCnB568531KEXoV5+Q1TksR0Zw@mail.gmail.com>
Date: Wed, 24 Dec 2025 11:44:29 +0000
From: Benjamin Philip <benjamin.philip495@...il.com>
To: Mark Pearson <mpearson-lenovo@...ebb.ca>,
"platform-driver-x86@...r.kernel.org" <platform-driver-x86@...r.kernel.org>, linux-kernel@...r.kernel.org
Cc: "Derek J . Clark" <derekjohn.clark@...il.com>, Hans de Goede <hansg@...nel.org>,
Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
Subject: Re: [PATCH 2/5] platform/x86: think-lmi: Remove unnecessary parens
"Mark Pearson" <mpearson-lenovo@...ebb.ca> writes:
> On Tue, Dec 23, 2025, at 2:23 PM, Benjamin Philip wrote:
>> This commit removes any unnecessary parentheses as flagged by
>> checkpatch in the following check:
>>
>> CHECK: Unnecessary parentheses around '...'
>>
>
> Shouldn't you mention that you also corrected pieces of code alignment here too?
>
I must have included them in by accident, since they share the same hunk
as a parens fix. Maybe splitting the hunk and then moving the alignment
changes to the last patch would be better?
>> Signed-off-by: Benjamin Philip <benjamin.philip495@...il.com>
>> ---
>> drivers/platform/x86/lenovo/think-lmi.c | 22 +++++++++++-----------
>> 1 file changed, 11 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/platform/x86/lenovo/think-lmi.c
>> b/drivers/platform/x86/lenovo/think-lmi.c
>> index 540b472b1bf3..1fac8986d077 100644
>> --- a/drivers/platform/x86/lenovo/think-lmi.c
>> +++ b/drivers/platform/x86/lenovo/think-lmi.c
>> @@ -440,7 +440,7 @@ static ssize_t current_password_store(struct kobject *kobj,
>>
>> pwdlen = strlen(buf);
>> /* pwdlen == 0 is allowed to clear the password */
>> - if (pwdlen && ((pwdlen < setting->minlen) || (pwdlen > setting->maxlen)))
>> + if (pwdlen && (pwdlen < setting->minlen || pwdlen > setting->maxlen))
>> return -EINVAL;
>>
>> strscpy(setting->password, buf, setting->maxlen);
>> @@ -476,7 +476,7 @@ static ssize_t new_password_store(struct kobject *kobj,
>>
>> pwdlen = strlen(new_pwd);
>> /* pwdlen == 0 is allowed to clear the password */
>> - if (pwdlen && ((pwdlen < setting->minlen) || (pwdlen > setting->maxlen))) {
>> + if (pwdlen && (pwdlen < setting->minlen || pwdlen > setting->maxlen)) {
>> ret = -EINVAL;
>> goto out;
>> }
>> @@ -859,7 +859,7 @@ static ssize_t certificate_store(struct kobject *kobj,
>> signature = setting->signature;
>> } else { /* Cert install */
>> /* Check if SMC and SVC already installed */
>> - if ((setting == tlmi_priv.pwd_system) &&
>> tlmi_priv.pwd_admin->cert_installed) {
>> + if (setting == tlmi_priv.pwd_system && tlmi_priv.pwd_admin->cert_installed) {
>> /* This gets treated as a cert update */
>> install_mode = TLMI_CERT_UPDATE;
>> signature = tlmi_priv.pwd_admin->signature;
>> @@ -881,7 +881,7 @@ static ssize_t certificate_store(struct kobject *kobj,
>> } else {
>> /* This is a fresh install */
>> /* To set admin cert, a password must be enabled */
>> - if ((setting == tlmi_priv.pwd_admin) &&
>> + if (setting == tlmi_priv.pwd_admin &&
>> (!setting->pwd_enabled || !setting->password[0])) {
>> kfree(new_cert);
>> return -EACCES;
>> @@ -965,13 +965,13 @@ static ssize_t save_signature_store(struct kobject *kobj,
>> static struct kobj_attribute auth_save_signature = __ATTR_WO(save_signature);
>>
>> static umode_t auth_attr_is_visible(struct kobject *kobj,
>> - struct attribute *attr, int n)
>> + struct attribute *attr, int n)
>> {
>> struct tlmi_pwd_setting *setting = to_tlmi_pwd_setting(kobj);
>>
>> /* We only want to display level and index settings on HDD/NVMe */
>> if (attr == &auth_index.attr || attr == &auth_level.attr) {
>> - if ((setting == tlmi_priv.pwd_hdd) || (setting == tlmi_priv.pwd_nvme))
>> + if (setting == tlmi_priv.pwd_hdd || setting == tlmi_priv.pwd_nvme)
>> return attr->mode;
>> return 0;
>> }
>> @@ -985,8 +985,8 @@ static umode_t auth_attr_is_visible(struct kobject *kobj,
>> if (tlmi_priv.certificate_support) {
>> if (setting == tlmi_priv.pwd_admin)
>> return attr->mode;
>> - if ((tlmi_priv.pwdcfg.core.password_mode >= TLMI_PWDCFG_MODE_MULTICERT) &&
>> - (setting == tlmi_priv.pwd_system))
>> + if (tlmi_priv.pwdcfg.core.password_mode >= TLMI_PWDCFG_MODE_MULTICERT &&
>> + setting == tlmi_priv.pwd_system)
>> return attr->mode;
>> }
>> return 0;
>> @@ -1216,13 +1216,13 @@ static struct kobj_attribute attr_current_val
>> = __ATTR_RW_MODE(current_value, 06
>>
>> static struct kobj_attribute attr_type = __ATTR_RO(type);
>>
>> -static umode_t attr_is_visible(struct kobject *kobj,
>> - struct attribute *attr, int n)
>> +static umode_t attr_is_visible(struct kobject *kobj, struct attribute *attr,
>> + int n)
>> {
>> struct tlmi_attr_setting *setting = to_tlmi_attr_setting(kobj);
>>
>> /* We don't want to display possible_values attributes if not available */
>> - if ((attr == &attr_possible_values.attr) && (!setting->possible_values))
>> + if (attr == &attr_possible_values.attr && !setting->possible_values)
>> return 0;
>>
>> return attr->mode;
>> --
>> 2.52.0
>
> I prefer the brackets as they were - for me it's clearer and removing them adds little value.
> But the changes are fine, and I guess it's preferred overall by kernel community, so shrug.
>
I find the previous style clearer too, and I agree that this change
comes down to pedantry and offer no real readability improvements.
Nevertheless, I think there's value in enforcing consistency and the
popular preference.
Note to Mark: Resent with the Cc: list included. Ignore the private
copy. I'm still ironing out my notmuch-emacs config :)
-- bp
> Reviewed-by: Mark Pearson <mpearson-lenovo@...ebb.ca>
Powered by blists - more mailing lists