Re: [PATCH] platform/x86: thinkpad_acpi: fixed warning and incorporated review comments

[Hans de Goede <hdegoede@redhat.com>]

Hi,Hi,

On 2/2/21 1:32 AM, Nitin Joshi wrote:
> The previous commit adding new sysfs for keyboard language has warning and
> few code correction has to be done as per new review comments.
> 
> Below changes has been addressed in this version:
>  - corrected warning. Many thanks to kernel test robot <lkp@intel.com> for
>    reporting and determining this warning.
>  - used sysfs_emit_at() API instead of strcat.
>  - sorted keyboard language array.
>  - removed unwanted space and corrected sentences.
> 
> Reported-by: kernel test robot <lkp@intel.com>
> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> Reviewed-by: Hans de Goede <hdegoede@redhat.com>

Erm, this patch is the result of some extra reviewing after I merged
the original patch, but the changes themselves have not been reviewed,
so these are wrong.

So I've dropped the 2 Reviewed-by tags.

In the future please only add Reviewed-by tags if you are:

1. Posting a new (revised) version of an existing patch
2. Where people have responded to a previous version with their Reviewed-by.
3. The changes in the new revision are small. Large changes invalidate
   previous reviews.

Thanks & Regards,

Hans


> Signed-off-by: Nitin Joshi <njoshi1@lenovo.com>
> ---
>  .../admin-guide/laptops/thinkpad-acpi.rst     | 15 ++++----
>  drivers/platform/x86/thinkpad_acpi.c          | 34 +++++++------------
>  2 files changed, 20 insertions(+), 29 deletions(-)
> 
> diff --git a/Documentation/admin-guide/laptops/thinkpad-acpi.rst b/Documentation/admin-guide/laptops/thinkpad-acpi.rst
> index b1188f05a99a..3b225ae47f1a 100644
> --- a/Documentation/admin-guide/laptops/thinkpad-acpi.rst
> +++ b/Documentation/admin-guide/laptops/thinkpad-acpi.rst
> @@ -1476,18 +1476,19 @@ sysfs: keyboard_lang
>  This feature is used to set keyboard language to ECFW using ASL interface.
>  Fewer thinkpads models like T580 , T590 , T15 Gen 1 etc.. has "=", "(',
>  ")" numeric keys, which are not displaying correctly, when keyboard language
> -is other than "english". This is because of default keyboard language in ECFW
> -is set as "english". Hence using this sysfs, user can set correct keyboard
> -language to ECFW and then these key's will work correctly .
> +is other than "english". This is because the default keyboard language in ECFW
> +is set as "english". Hence using this sysfs, user can set the correct keyboard
> +language to ECFW and then these key's will work correctly.
>  
>  Example of command to set keyboard language is mentioned below::
>  
>          echo jp > /sys/devices/platform/thinkpad_acpi/keyboard_lang
>  
> -Text corresponding to keyboard layout to be set in sysfs are : jp (Japan), be(Belgian),
> -cz(Czech), en(English), da(Danish), de(German), es(Spain) , et(Estonian),
> -fr(French) , fr-ch (French(Switzerland)), pl(Polish), sl(Slovenian), hu
> -(Hungarian), nl(Dutch), tr(Turkey), it(Italy), sv(Sweden), pt(portugese)
> +Text corresponding to keyboard layout to be set in sysfs are: be(Belgian),
> +cz(Czech), da(Danish), de(German), en(English), es(Spain), et(Estonian),
> +fr(French), fr-ch(French(Switzerland)), hu(Hungarian), it(Italy), jp (Japan),
> +nl(Dutch), nn(Norway), pl(Polish), pt(portugese), sl(Slovenian), sv(Sweden),
> +tr(Turkey)
>  
>  
>  Adaptive keyboard
> diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
> index 3cfc4a872c2d..a7f1b4ee5457 100644
> --- a/drivers/platform/x86/thinkpad_acpi.c
> +++ b/drivers/platform/x86/thinkpad_acpi.c
> @@ -9991,16 +9991,12 @@ struct keyboard_lang_data {
>  	int lang_code;
>  };
>  
> -/*
> - * When adding new entries to keyboard_lang_data, please check that
> - * the select_lang[] buffer in keyboard_lang_show() is still large enough.
> - */
> -struct keyboard_lang_data keyboard_lang_data[] = {
> -	{"en", 0},
> +static const struct keyboard_lang_data keyboard_lang_data[] = {
>  	{"be", 0x080c},
>  	{"cz", 0x0405},
>  	{"da", 0x0406},
>  	{"de", 0x0c07},
> +	{"en", 0x0000},
>  	{"es", 0x2c0a},
>  	{"et", 0x0425},
>  	{"fr", 0x040c},
> @@ -10056,9 +10052,7 @@ static ssize_t keyboard_lang_show(struct device *dev,
>  				struct device_attribute *attr,
>  				char *buf)
>  {
> -	int output, err, i;
> -	char select_lang[80] = "";
> -	char lang[8] = "";
> +	int output, err, i, len = 0;
>  
>  	err = get_keyboard_lang(&output);
>  	if (err)
> @@ -10066,19 +10060,18 @@ static ssize_t keyboard_lang_show(struct device *dev,
>  
>  	for (i = 0; i < ARRAY_SIZE(keyboard_lang_data); i++) {
>  		if (i)
> -			strcat(select_lang, " ");
> +			len += sysfs_emit_at(buf, len, "%s", " ");
>  
>  		if (output == keyboard_lang_data[i].lang_code) {
> -			strcat(lang, "[");
> -			strcat(lang, keyboard_lang_data[i].lang_str);
> -			strcat(lang, "]");
> -			strcat(select_lang, lang);
> +			len += sysfs_emit_at(buf, len, "%s%s%s", "[",
> +					keyboard_lang_data[i].lang_str, "]");

This can be simplified to:

			len += sysfs_emit_at(buf, len, "[%s]", keyboard_lang_data[i].lang_str);

I've applied the patch with this fixed up.

Thank you for your patch, I've applied this patch to my review-hans 
branch:
https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/log/?h=review-hans

Note it will show up in my review-hans branch once I've pushed my
local branch there, which might take a while.

Once I've run some tests on this branch the patches there will be
added to the platform-drivers-x86/for-next branch and eventually
will be included in the pdx86 pull-request to Linus for the next
merge-window.

Regards,

Hans




>  		} else {
> -			strcat(select_lang, keyboard_lang_data[i].lang_str);
> +			len += sysfs_emit_at(buf, len, "%s", keyboard_lang_data[i].lang_str);
>  		}
>  	}
> +	len += sysfs_emit_at(buf, len, "\n");
>  
> -	return sysfs_emit(buf, "%s\n", select_lang);
> +	return len;
>  }
>  
>  static ssize_t keyboard_lang_store(struct device *dev,
> @@ -10105,7 +10098,7 @@ static ssize_t keyboard_lang_store(struct device *dev,
>  		if (err)
>  			return err;
>  	} else {
> -		pr_err("Unknown Keyboard language. Ignoring\n");
> +		dev_err(&tpacpi_pdev->dev, "Unknown Keyboard language. Ignoring\n");
>  		return -EINVAL;
>  	}
>  
> @@ -10116,7 +10109,6 @@ static ssize_t keyboard_lang_store(struct device *dev,
>  
>  	return count;
>  }
> -
>  static DEVICE_ATTR_RW(keyboard_lang);
>  
>  static struct attribute *kbdlang_attributes[] = {
> @@ -10135,7 +10127,7 @@ static int tpacpi_kbdlang_init(struct ibm_init_struct *iibm)
>  	err = get_keyboard_lang(&output);
>  	/*
>  	 * If support isn't available (ENODEV) then don't return an error
> -	 * just don't create the sysfs group
> +	 * just don't create the sysfs group.
>  	 */
>  	if (err == -ENODEV)
>  		return 0;
> @@ -10144,9 +10136,7 @@ static int tpacpi_kbdlang_init(struct ibm_init_struct *iibm)
>  		return err;
>  
>  	/* Platform supports this feature - create the sysfs file */
> -	err = sysfs_create_group(&tpacpi_pdev->dev.kobj, &kbdlang_attr_group);
> -
> -	return err;
> +	return sysfs_create_group(&tpacpi_pdev->dev.kobj, &kbdlang_attr_group);
>  }
>  
>  static void kbdlang_exit(void)
>