Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [PATCH 1/1] Input: atmel_mxt_ts - allow specification of firmware
 file name
To:     Jiada Wang <jiada_wang@mentor.com>, nick@shmanahar.org,
        dmitry.torokhov@gmail.com
Cc:     linux-input@vger.kernel.org, linux-kernel@vger.kernel.org,
        erosca@de.adit-jv.com, Andrew_Gabbasov@mentor.com
References: <20200731075714.10608-1-jiada_wang@mentor.com>
From:   Dmitry Osipenko <digetx@gmail.com>
Message-ID: <e5da9e03-8156-39f6-c78b-0226c95690c6@gmail.com>
Date:   Sun, 9 Aug 2020 23:11:16 +0300
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101
 Thunderbird/68.10.0
MIME-Version: 1.0
In-Reply-To: <20200731075714.10608-1-jiada_wang@mentor.com>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 8bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

31.07.2020 10:57, Jiada Wang пишет:
> From: Nick Dyer <nick.dyer@itdev.co.uk>
> 
> On platforms which have multiple device instances using this driver, the
> firmware may be different on each device. This patch makes the user give
> the name of the firmware file when flashing.
> 
> This also prevents accidental triggering of the firmware load process.
> 
> Signed-off-by: Nick Dyer <nick.dyer@itdev.co.uk>
> Acked-by: Benson Leung <bleung@chromium.org>
> Acked-by: Yufeng Shen <miletus@chromium.org>
> (cherry picked from ndyer/linux/for-upstream commit 76ebb7cee971cb42dfb0a3a9224403b8b09abcf1)
> [gdavis: Forward port and fix conflicts.]
> Signed-off-by: George G. Davis <george_davis@mentor.com>
> Signed-off-by: Jiada Wang <jiada_wang@mentor.com>
> ---
>  drivers/input/touchscreen/atmel_mxt_ts.c | 42 ++++++++++++++++++++----
>  1 file changed, 36 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c
> index a2189739e30f..024dee7a3571 100644
> --- a/drivers/input/touchscreen/atmel_mxt_ts.c
> +++ b/drivers/input/touchscreen/atmel_mxt_ts.c
> @@ -30,8 +30,6 @@
>  #include <media/videobuf2-v4l2.h>
>  #include <media/videobuf2-vmalloc.h>
>  
> -/* Firmware files */
> -#define MXT_FW_NAME		"maxtouch.fw"
>  #define MXT_CFG_NAME		"maxtouch.cfg"
>  #define MXT_CFG_MAGIC		"OBP_RAW V1"
>  
> @@ -308,6 +306,7 @@ struct mxt_data {
>  	struct t7_config t7_cfg;
>  	struct mxt_dbg dbg;
>  	struct gpio_desc *reset_gpio;
> +	char *fw_name;

Hello, Jiada!

Have you decided to abandon the patch which allowed to specify firmware
name in a device tree?

>  	/* Cached parameters from object table */
>  	u16 T5_address;
> @@ -2766,7 +2765,7 @@ static int mxt_check_firmware_format(struct device *dev,
>  	return -EINVAL;
>  }
>  
> -static int mxt_load_fw(struct device *dev, const char *fn)
> +static int mxt_load_fw(struct device *dev)
>  {
>  	struct mxt_data *data = dev_get_drvdata(dev);
>  	const struct firmware *fw = NULL;
> @@ -2776,9 +2775,9 @@ static int mxt_load_fw(struct device *dev, const char *fn)
>  	unsigned int frame = 0;
>  	int ret;
>  
> -	ret = request_firmware(&fw, fn, dev);
> +	ret = request_firmware(&fw, data->fw_name, dev);
>  	if (ret) {
> -		dev_err(dev, "Unable to open firmware %s\n", fn);
> +		dev_err(dev, "Unable to open firmware %s\n", data->fw_name);
>  		return ret;
>  	}
>  
> @@ -2887,6 +2886,33 @@ static int mxt_load_fw(struct device *dev, const char *fn)
>  	return ret;
>  }
>  
> +static int mxt_update_file_name(struct device *dev, char **file_name,
> +				const char *buf, size_t count)
> +{
> +	char *file_name_tmp;
> +
> +	/* Simple sanity check */
> +	if (count > 64) {
> +		dev_warn(dev, "File name too long\n");
> +		return -EINVAL;
> +	}
> +
> +	file_name_tmp = krealloc(*file_name, count + 1, GFP_KERNEL);
> +	if (!file_name_tmp)
> +		return -ENOMEM;

The allocated string is never release, this is not good.

Wouldn't it be nicer to make data->fw_name a fixed-size string?

> +	*file_name = file_name_tmp;
> +	memcpy(*file_name, buf, count);
> +
> +	/* Echo into the sysfs entry may append newline at the end of buf */
> +	if (buf[count - 1] == '\n')
> +		(*file_name)[count - 1] = '\0';
> +	else
> +		(*file_name)[count] = '\0';

What about to use strscpy?

> +	return 0;
> +}
> +
>  static ssize_t mxt_update_fw_store(struct device *dev,
>  					struct device_attribute *attr,
>  					const char *buf, size_t count)
> @@ -2894,7 +2920,11 @@ static ssize_t mxt_update_fw_store(struct device *dev,
>  	struct mxt_data *data = dev_get_drvdata(dev);
>  	int error;
>  
> -	error = mxt_load_fw(dev, MXT_FW_NAME);
> +	error = mxt_update_file_name(dev, &data->fw_name, buf, count);
> +	if (error)
> +		return error;

This change breaks old behavior, I'm not sure that it's acceptable. The
default name should remain to be "maxtouch.fw", IMO.

> +	error = mxt_load_fw(dev);
>  	if (error) {
>  		dev_err(dev, "The firmware update failed(%d)\n", error);
>  		count = error;
>