[PATCH 1/3] Input: ili210x - use kvmalloc() to allocate buffer for firmware update

[PATCH 1/3] Input: ili210x - use kvmalloc() to allocate buffer for firmware update

[Dmitry Torokhov]

Allocating a contiguous buffer of 64K may fail is memory is sufficiently
fragmented, and may cause OOM kill of an unrelated process. However we
do not need to have contiguous memory. We also do not need to zero
out the buffer since it will be overwritten with firmware data.

Switch to using kvmalloc() instead of kzalloc().

Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/input/touchscreen/ili210x.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/input/touchscreen/ili210x.c b/drivers/input/touchscreen/ili210x.c
index 79bdb2b10949..f3c3ad70244f 100644
--- a/drivers/input/touchscreen/ili210x.c
+++ b/drivers/input/touchscreen/ili210x.c
@@ -597,7 +597,7 @@ static int ili251x_firmware_to_buffer(const struct firmware *fw,
 	 * once, copy them all into this buffer at the right locations, and then
 	 * do all operations on this linear buffer.
 	 */
-	fw_buf = kzalloc(SZ_64K, GFP_KERNEL);
+	fw_buf = kvmalloc(SZ_64K, GFP_KERNEL);
 	if (!fw_buf)
 		return -ENOMEM;
 
@@ -627,7 +627,7 @@ static int ili251x_firmware_to_buffer(const struct firmware *fw,
 	return 0;
 
 err_big:
-	kfree(fw_buf);
+	kvfree(fw_buf);
 	return error;
 }
 
@@ -870,7 +870,7 @@ static ssize_t ili210x_firmware_update_store(struct device *dev,
 	ili210x_hardware_reset(priv->reset_gpio);
 	dev_dbg(dev, "Firmware update ended, error=%i\n", error);
 	enable_irq(client->irq);
-	kfree(fwbuf);
+	kvfree(fwbuf);
 	return error;
 }
 
-- 
2.45.2.505.gda0bf45e8d-goog

[PATCH 2/3] Input: ili210x - switch to using cleanup functions in firmware code

[Dmitry Torokhov]

Start using __free() attributes to simplify the code and error handling.

Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/input/touchscreen/ili210x.c | 123 ++++++++++++++--------------
 1 file changed, 63 insertions(+), 60 deletions(-)

diff --git a/drivers/input/touchscreen/ili210x.c b/drivers/input/touchscreen/ili210x.c
index f3c3ad70244f..55f3852c8dae 100644
--- a/drivers/input/touchscreen/ili210x.c
+++ b/drivers/input/touchscreen/ili210x.c
@@ -582,14 +582,12 @@ static ssize_t ili210x_calibrate(struct device *dev,
 }
 static DEVICE_ATTR(calibrate, S_IWUSR, NULL, ili210x_calibrate);
 
-static int ili251x_firmware_to_buffer(const struct firmware *fw,
-				      u8 **buf, u16 *ac_end, u16 *df_end)
+static const u8 *ili251x_firmware_to_buffer(const struct firmware *fw,
+					    u16 *ac_end, u16 *df_end)
 {
 	const struct ihex_binrec *rec;
 	u32 fw_addr, fw_last_addr = 0;
 	u16 fw_len;
-	u8 *fw_buf;
-	int error;
 
 	/*
 	 * The firmware ihex blob can never be bigger than 64 kiB, so make this
@@ -597,9 +595,9 @@ static int ili251x_firmware_to_buffer(const struct firmware *fw,
 	 * once, copy them all into this buffer at the right locations, and then
 	 * do all operations on this linear buffer.
 	 */
-	fw_buf = kvmalloc(SZ_64K, GFP_KERNEL);
+	u8* fw_buf __free(kvfree) = kvmalloc(SZ_64K, GFP_KERNEL);
 	if (!fw_buf)
-		return -ENOMEM;
+		return ERR_PTR(-ENOMEM);
 
 	rec = (const struct ihex_binrec *)fw->data;
 	while (rec) {
@@ -607,10 +605,8 @@ static int ili251x_firmware_to_buffer(const struct firmware *fw,
 		fw_len = be16_to_cpu(rec->len);
 
 		/* The last 32 Byte firmware block can be 0xffe0 */
-		if (fw_addr + fw_len > SZ_64K || fw_addr > SZ_64K - 32) {
-			error = -EFBIG;
-			goto err_big;
-		}
+		if (fw_addr + fw_len > SZ_64K || fw_addr > SZ_64K - 32)
+			return ERR_PTR(-EFBIG);
 
 		/* Find the last address before DF start address, that is AC end */
 		if (fw_addr == 0xf000)
@@ -623,12 +619,8 @@ static int ili251x_firmware_to_buffer(const struct firmware *fw,
 
 	/* DF end address is the last address in the firmware blob */
 	*df_end = fw_addr + fw_len;
-	*buf = fw_buf;
-	return 0;
 
-err_big:
-	kvfree(fw_buf);
-	return error;
+	return_ptr(fw_buf);
 }
 
 /* Switch mode between Application and BootLoader */
@@ -691,7 +683,7 @@ static int ili251x_firmware_busy(struct i2c_client *client)
 	return 0;
 }
 
-static int ili251x_firmware_write_to_ic(struct device *dev, u8 *fwbuf,
+static int ili251x_firmware_write_to_ic(struct device *dev, const u8 *fwbuf,
 					u16 start, u16 end, u8 dataflash)
 {
 	struct i2c_client *client = to_i2c_client(dev);
@@ -776,47 +768,17 @@ static void ili210x_hardware_reset(struct gpio_desc *reset_gpio)
 	msleep(300);
 }
 
-static ssize_t ili210x_firmware_update_store(struct device *dev,
-					     struct device_attribute *attr,
-					     const char *buf, size_t count)
+static int ili210x_do_firmware_update(struct ili210x *priv,
+				      const u8 *fwbuf, u16 ac_end, u16 df_end)
 {
-	struct i2c_client *client = to_i2c_client(dev);
-	struct ili210x *priv = i2c_get_clientdata(client);
-	const char *fwname = ILI251X_FW_FILENAME;
-	const struct firmware *fw;
-	u16 ac_end, df_end;
-	u8 *fwbuf;
+	struct i2c_client *client = priv->client;
+	struct device *dev = &client->dev;
 	int error;
 	int i;
 
-	error = request_ihex_firmware(&fw, fwname, dev);
-	if (error) {
-		dev_err(dev, "Failed to request firmware %s, error=%d\n",
-			fwname, error);
-		return error;
-	}
-
-	error = ili251x_firmware_to_buffer(fw, &fwbuf, &ac_end, &df_end);
-	release_firmware(fw);
-	if (error)
-		return error;
-
-	/*
-	 * Disable touchscreen IRQ, so that we would not get spurious touch
-	 * interrupt during firmware update, and so that the IRQ handler won't
-	 * trigger and interfere with the firmware update. There is no bit in
-	 * the touch controller to disable the IRQs during update, so we have
-	 * to do it this way here.
-	 */
-	disable_irq(client->irq);
-
-	dev_dbg(dev, "Firmware update started, firmware=%s\n", fwname);
-
-	ili210x_hardware_reset(priv->reset_gpio);
-
 	error = ili251x_firmware_reset(client);
 	if (error)
-		goto exit;
+		return error;
 
 	/* This may not succeed on first try, so re-try a few times. */
 	for (i = 0; i < 5; i++) {
@@ -826,7 +788,7 @@ static ssize_t ili210x_firmware_update_store(struct device *dev,
 	}
 
 	if (error)
-		goto exit;
+		return error;
 
 	dev_dbg(dev, "IC is now in BootLoader mode\n");
 
@@ -835,7 +797,7 @@ static ssize_t ili210x_firmware_update_store(struct device *dev,
 	error = ili251x_firmware_write_to_ic(dev, fwbuf, 0xf000, df_end, 1);
 	if (error) {
 		dev_err(dev, "DF firmware update failed, error=%d\n", error);
-		goto exit;
+		return error;
 	}
 
 	dev_dbg(dev, "DataFlash firmware written\n");
@@ -843,7 +805,7 @@ static ssize_t ili210x_firmware_update_store(struct device *dev,
 	error = ili251x_firmware_write_to_ic(dev, fwbuf, 0x2000, ac_end, 0);
 	if (error) {
 		dev_err(dev, "AC firmware update failed, error=%d\n", error);
-		goto exit;
+		return error;
 	}
 
 	dev_dbg(dev, "Application firmware written\n");
@@ -856,22 +818,63 @@ static ssize_t ili210x_firmware_update_store(struct device *dev,
 	}
 
 	if (error)
-		goto exit;
+		return error;
 
 	dev_dbg(dev, "IC is now in Application mode\n");
 
 	error = ili251x_firmware_update_cached_state(dev);
 	if (error)
-		goto exit;
+		return error;
 
-	error = count;
+	return 0;
+}
+
+static ssize_t ili210x_firmware_update_store(struct device *dev,
+					     struct device_attribute *attr,
+					     const char *buf, size_t count)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	struct ili210x *priv = i2c_get_clientdata(client);
+	const char *fwname = ILI251X_FW_FILENAME;
+	u16 ac_end, df_end;
+	int error;
+
+	const struct firmware *fw __free(firmware) = NULL;
+	error = request_ihex_firmware(&fw, fwname, dev);
+	if (error) {
+		dev_err(dev, "Failed to request firmware %s, error=%d\n",
+			fwname, error);
+		return error;
+	}
+
+	const u8* fwbuf __free(kvfree) =
+			ili251x_firmware_to_buffer(fw, &ac_end, &df_end);
+	error = PTR_ERR_OR_ZERO(fwbuf);
+	if (error)
+		return error;
+
+	/*
+	 * Disable touchscreen IRQ, so that we would not get spurious touch
+	 * interrupt during firmware update, and so that the IRQ handler won't
+	 * trigger and interfere with the firmware update. There is no bit in
+	 * the touch controller to disable the IRQs during update, so we have
+	 * to do it this way here.
+	 */
+	disable_irq(client->irq);
+
+	dev_dbg(dev, "Firmware update started, firmware=%s\n", fwname);
+
+	ili210x_hardware_reset(priv->reset_gpio);
+
+	error = ili210x_do_firmware_update(priv, fwbuf, ac_end, df_end);
 
-exit:
 	ili210x_hardware_reset(priv->reset_gpio);
+
 	dev_dbg(dev, "Firmware update ended, error=%i\n", error);
+
 	enable_irq(client->irq);
-	kvfree(fwbuf);
-	return error;
+
+	return error ?: count;
 }
 
 static DEVICE_ATTR(firmware_update, 0200, NULL, ili210x_firmware_update_store);
-- 
2.45.2.505.gda0bf45e8d-goog

[PATCH 3/3] Input: ili210x - use guard notation when disabling and reenabling IRQ

[Dmitry Torokhov]

This makes the code more compact and error handling more robust.

Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/input/touchscreen/ili210x.c | 16 +++++++---------
 1 file changed, 7 insertions(+), 9 deletions(-)

diff --git a/drivers/input/touchscreen/ili210x.c b/drivers/input/touchscreen/ili210x.c
index 55f3852c8dae..4573844c3395 100644
--- a/drivers/input/touchscreen/ili210x.c
+++ b/drivers/input/touchscreen/ili210x.c
@@ -860,19 +860,17 @@ static ssize_t ili210x_firmware_update_store(struct device *dev,
 	 * the touch controller to disable the IRQs during update, so we have
 	 * to do it this way here.
 	 */
-	disable_irq(client->irq);
+	scoped_guard(disable_irq, &client->irq) {
+		dev_dbg(dev, "Firmware update started, firmware=%s\n", fwname);
 
-	dev_dbg(dev, "Firmware update started, firmware=%s\n", fwname);
+		ili210x_hardware_reset(priv->reset_gpio);
 
-	ili210x_hardware_reset(priv->reset_gpio);
+		error = ili210x_do_firmware_update(priv, fwbuf, ac_end, df_end);
 
-	error = ili210x_do_firmware_update(priv, fwbuf, ac_end, df_end);
+		ili210x_hardware_reset(priv->reset_gpio);
 
-	ili210x_hardware_reset(priv->reset_gpio);
-
-	dev_dbg(dev, "Firmware update ended, error=%i\n", error);
-
-	enable_irq(client->irq);
+		dev_dbg(dev, "Firmware update ended, error=%i\n", error);
+	}
 
 	return error ?: count;
 }
-- 
2.45.2.505.gda0bf45e8d-goog

Re: [PATCH 1/3] Input: ili210x - use kvmalloc() to allocate buffer for firmware update

[Markus Elfring]

> Allocating a contiguous buffer of 64K may fail is memory is sufficiently
…

                                                 if?


I find that cover letters can be helpful for presented patch series.

Regards,
Markus

Re: [PATCH 3/3] Input: ili210x - use guard notation when disabling and reenabling IRQ

[Markus Elfring]

> This makes the code more compact and error handling more robust.

Please improve the change description with an imperative wording.
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.10-rc2#n94

Regards,
Markus