[PATCH] input synaptics-rmi4: rmi_f01.c storage fix

[PATCH] input synaptics-rmi4: rmi_f01.c storage fix

[Christopher Heiny]

Correctly free driver related data when initialization fails.

Trivial: Clarify a diagnostic message.

Signed-off-by: Christopher Heiny <cheiny@synaptics.com>
Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Cc: Benjamin Tissoires <benjamin.tissoires@redhat.com>
Cc: Linux Walleij <linus.walleij@linaro.org>
Cc: David Herrmann <dh.herrmann@gmail.com>
Cc: Jiri Kosina <jkosina@suse.cz>

---

 drivers/input/rmi4/rmi_f01.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/input/rmi4/rmi_f01.c b/drivers/input/rmi4/rmi_f01.c
index 381ad60..e4a6df9 100644
--- a/drivers/input/rmi4/rmi_f01.c
+++ b/drivers/input/rmi4/rmi_f01.c
@@ -149,7 +149,7 @@ static int rmi_f01_alloc_memory(struct rmi_function *fn,
 
 	f01 = devm_kzalloc(&fn->dev, sizeof(struct f01_data), GFP_KERNEL);
 	if (!f01) {
-		dev_err(&fn->dev, "Failed to allocate fn_01_data.\n");
+		dev_err(&fn->dev, "Failed to allocate f01_data.\n");
 		return -ENOMEM;
 	}
 
@@ -158,6 +158,7 @@ static int rmi_f01_alloc_memory(struct rmi_function *fn,
 			GFP_KERNEL);
 	if (!f01->device_control.interrupt_enable) {
 		dev_err(&fn->dev, "Failed to allocate interrupt enable.\n");
+		devm_kfree(&fn->dev, f01);
 		return -ENOMEM;
 	}
 	fn->data = f01;
@@ -381,7 +382,8 @@ static int rmi_f01_initialize(struct rmi_function *fn)
 	return 0;
 
  error_exit:
-	kfree(data);
+	devm_kfree(&fn->dev, data->device_control.interrupt_enable);
+	devm_kfree(&fn->dev, data);
 	return error;
 }
 

Re: [PATCH] input synaptics-rmi4: rmi_f01.c storage fix

[Courtney Cavin]

On Wed, Feb 12, 2014 at 12:13:00AM +0100, Christopher Heiny wrote:
> Correctly free driver related data when initialization fails.
> 
> Trivial: Clarify a diagnostic message.
> 
> Signed-off-by: Christopher Heiny <cheiny@synaptics.com>
> Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> Cc: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> Cc: Linux Walleij <linus.walleij@linaro.org>
> Cc: David Herrmann <dh.herrmann@gmail.com>
> Cc: Jiri Kosina <jkosina@suse.cz>
> 
> ---
> 
>  drivers/input/rmi4/rmi_f01.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/input/rmi4/rmi_f01.c b/drivers/input/rmi4/rmi_f01.c
> index 381ad60..e4a6df9 100644
> --- a/drivers/input/rmi4/rmi_f01.c
> +++ b/drivers/input/rmi4/rmi_f01.c
> @@ -149,7 +149,7 @@ static int rmi_f01_alloc_memory(struct rmi_function *fn,
>  
>  	f01 = devm_kzalloc(&fn->dev, sizeof(struct f01_data), GFP_KERNEL);
>  	if (!f01) {
> -		dev_err(&fn->dev, "Failed to allocate fn_01_data.\n");
> +		dev_err(&fn->dev, "Failed to allocate f01_data.\n");

Just remove this printout, as it won't help any user in the case of OOM.
Additionally, there's already plenty of (more useful) information
printed out if kmalloc fails.

>  		return -ENOMEM;
>  	}
>  
> @@ -158,6 +158,7 @@ static int rmi_f01_alloc_memory(struct rmi_function *fn,
>  			GFP_KERNEL);
>  	if (!f01->device_control.interrupt_enable) {
>  		dev_err(&fn->dev, "Failed to allocate interrupt enable.\n");
> +		devm_kfree(&fn->dev, f01);

Unnecessary, devres_release_all() will release this, from:
	- really_probe() -> rmi_function_probe() -> rmi_f01_probe()
	- driver_probe_device()
	- __driver_attach()
	- driver_attach()
	- bus_add_driver()
	- driver_register()
	- __rmi_register_function_handler()

>  		return -ENOMEM;
>  	}
>  	fn->data = f01;
> @@ -381,7 +382,8 @@ static int rmi_f01_initialize(struct rmi_function *fn)
>  	return 0;
>  
>   error_exit:
> -	kfree(data);
> +	devm_kfree(&fn->dev, data->device_control.interrupt_enable);
> +	devm_kfree(&fn->dev, data);

Same as above for these two.

>  	return error;
>  }
>  

Generally devm_ release functions are unnecessary to call, as all
resources will get released on driver detach, whether abnormal or not.

-Courtney

Re: [PATCH] input synaptics-rmi4: rmi_f01.c storage fix

[Christopher Heiny]

On 02/11/2014 05:26 PM, Courtney Cavin wrote:
> On Wed, Feb 12, 2014 at 12:13:00AM +0100, Christopher Heiny wrote:
>> Correctly free driver related data when initialization fails.
>>
>> Trivial: Clarify a diagnostic message.
>>
>> Signed-off-by: Christopher Heiny <cheiny@synaptics.com>
>> Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
>> Cc: Benjamin Tissoires <benjamin.tissoires@redhat.com>
>> Cc: Linux Walleij <linus.walleij@linaro.org>
>> Cc: David Herrmann <dh.herrmann@gmail.com>
>> Cc: Jiri Kosina <jkosina@suse.cz>
>>
>> ---
>>
>>   drivers/input/rmi4/rmi_f01.c | 6 ++++--
>>   1 file changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/input/rmi4/rmi_f01.c b/drivers/input/rmi4/rmi_f01.c
>> index 381ad60..e4a6df9 100644
>> --- a/drivers/input/rmi4/rmi_f01.c
>> +++ b/drivers/input/rmi4/rmi_f01.c
>> @@ -149,7 +149,7 @@ static int rmi_f01_alloc_memory(struct rmi_function *fn,
>>
>>   	f01 = devm_kzalloc(&fn->dev, sizeof(struct f01_data), GFP_KERNEL);
>>   	if (!f01) {
>> -		dev_err(&fn->dev, "Failed to allocate fn_01_data.\n");
>> +		dev_err(&fn->dev, "Failed to allocate f01_data.\n");
>
> Just remove this printout, as it won't help any user in the case of OOM.
> Additionally, there's already plenty of (more useful) information
> printed out if kmalloc fails.

Good point.  There's similar messages in a number of places, so we'll do 
a single patch to clean them all up at once.

>
>>   		return -ENOMEM;
>>   	}
>>
>> @@ -158,6 +158,7 @@ static int rmi_f01_alloc_memory(struct rmi_function *fn,
>>   			GFP_KERNEL);
>>   	if (!f01->device_control.interrupt_enable) {
>>   		dev_err(&fn->dev, "Failed to allocate interrupt enable.\n");
>> +		devm_kfree(&fn->dev, f01);
>
> Unnecessary, devres_release_all() will release this, from:
> 	- really_probe() -> rmi_function_probe() -> rmi_f01_probe()
> 	- driver_probe_device()
> 	- __driver_attach()
> 	- driver_attach()
> 	- bus_add_driver()
> 	- driver_register()
> 	- __rmi_register_function_handler()

As mentioned before, we've received a lot of conflicting advice on 
devm_k*.  Thanks very much for the clarification - it makes more sense now.

>>   		return -ENOMEM;
>>   	}
>>   	fn->data = f01;
>> @@ -381,7 +382,8 @@ static int rmi_f01_initialize(struct rmi_function *fn)
>>   	return 0;
>>
>>    error_exit:
>> -	kfree(data);
>> +	devm_kfree(&fn->dev, data->device_control.interrupt_enable);
>> +	devm_kfree(&fn->dev, data);
>
> Same as above for these two.
>
>>   	return error;
>>   }
>>
>
> Generally devm_ release functions are unnecessary to call, as all
> resources will get released on driver detach, whether abnormal or not.
>
> -Courtney
>


-- 

Christopher Heiny
Senior Staff Firmware Engineer
Synaptics Incorporated

Re: [PATCH] input synaptics-rmi4: rmi_f01.c storage fix

[Dmitry Torokhov]

Hi Chris,

On Tue, Feb 11, 2014 at 03:13:00PM -0800, Christopher Heiny wrote:
> Correctly free driver related data when initialization fails.
> 
> Trivial: Clarify a diagnostic message.
> 
> Signed-off-by: Christopher Heiny <cheiny@synaptics.com>
> Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> Cc: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> Cc: Linux Walleij <linus.walleij@linaro.org>
> Cc: David Herrmann <dh.herrmann@gmail.com>
> Cc: Jiri Kosina <jkosina@suse.cz>
> 
> ---
> 
>  drivers/input/rmi4/rmi_f01.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/input/rmi4/rmi_f01.c b/drivers/input/rmi4/rmi_f01.c
> index 381ad60..e4a6df9 100644
> --- a/drivers/input/rmi4/rmi_f01.c
> +++ b/drivers/input/rmi4/rmi_f01.c
> @@ -149,7 +149,7 @@ static int rmi_f01_alloc_memory(struct rmi_function *fn,
>  
>  	f01 = devm_kzalloc(&fn->dev, sizeof(struct f01_data), GFP_KERNEL);
>  	if (!f01) {
> -		dev_err(&fn->dev, "Failed to allocate fn_01_data.\n");
> +		dev_err(&fn->dev, "Failed to allocate f01_data.\n");
>  		return -ENOMEM;
>  	}
>  
> @@ -158,6 +158,7 @@ static int rmi_f01_alloc_memory(struct rmi_function *fn,
>  			GFP_KERNEL);
>  	if (!f01->device_control.interrupt_enable) {
>  		dev_err(&fn->dev, "Failed to allocate interrupt enable.\n");
> +		devm_kfree(&fn->dev, f01);

As Courtney mentioned if you are calling devm_kfree() you are most
likely doing something wrong.

How about the patch below? Please check the XXX comment, I have some
concerns about lts vs doze_holdoff check mismatch in probe() and
config().

Thanks.

-- 
Dmitry

Input: synaptics-rmi4 - F01 initialization cleanup

From: Dmitry Torokhov <dmitry.torokhov@gmail.com>

- rename data to f01 where appropriate;
- switch to using rmi_read()/rmi_write() for single-byte data;
- allocate interrupt mask together with the main structure;
- do not kfree() memory allocated with devm;
- do not write config data in probe(), we have config() for that;
- drop unneeded rmi_f01_remove().

Reported-by: Courtney Cavin <courtney.cavin@sonymobile.com>
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/input/rmi4/rmi_f01.c |  397 ++++++++++++++++++------------------------
 1 file changed, 172 insertions(+), 225 deletions(-)

diff --git a/drivers/input/rmi4/rmi_f01.c b/drivers/input/rmi4/rmi_f01.c
index 381ad60..8f7840e 100644
--- a/drivers/input/rmi4/rmi_f01.c
+++ b/drivers/input/rmi4/rmi_f01.c
@@ -123,47 +123,21 @@ struct f01_device_control {
 
 struct f01_data {
 	struct f01_basic_properties properties;
-
 	struct f01_device_control device_control;
-	struct mutex control_mutex;
-
-	u8 device_status;
 
 	u16 interrupt_enable_addr;
 	u16 doze_interval_addr;
 	u16 wakeup_threshold_addr;
 	u16 doze_holdoff_addr;
-	int irq_count;
-	int num_of_irq_regs;
 
 #ifdef CONFIG_PM_SLEEP
 	bool suspended;
 	bool old_nosleep;
 #endif
-};
-
-static int rmi_f01_alloc_memory(struct rmi_function *fn,
-				int num_of_irq_regs)
-{
-	struct f01_data *f01;
 
-	f01 = devm_kzalloc(&fn->dev, sizeof(struct f01_data), GFP_KERNEL);
-	if (!f01) {
-		dev_err(&fn->dev, "Failed to allocate fn_01_data.\n");
-		return -ENOMEM;
-	}
-
-	f01->device_control.interrupt_enable = devm_kzalloc(&fn->dev,
-			sizeof(u8) * (num_of_irq_regs),
-			GFP_KERNEL);
-	if (!f01->device_control.interrupt_enable) {
-		dev_err(&fn->dev, "Failed to allocate interrupt enable.\n");
-		return -ENOMEM;
-	}
-	fn->data = f01;
-
-	return 0;
-}
+	unsigned int num_of_irq_regs;
+	u8 interrupt_enable[];
+};
 
 static int rmi_f01_read_properties(struct rmi_device *rmi_dev,
 				   u16 query_base_addr,
@@ -174,8 +148,9 @@ static int rmi_f01_read_properties(struct rmi_device *rmi_dev,
 
 	error = rmi_read_block(rmi_dev, query_base_addr,
 			       basic_query, sizeof(basic_query));
-	if (error < 0) {
-		dev_err(&rmi_dev->dev, "Failed to read device query registers.\n");
+	if (error) {
+		dev_err(&rmi_dev->dev,
+			"Failed to read device query registers: %d\n", error);
 		return error;
 	}
 
@@ -204,38 +179,51 @@ static int rmi_f01_read_properties(struct rmi_device *rmi_dev,
 	return 0;
 }
 
-static int rmi_f01_initialize(struct rmi_function *fn)
+static int rmi_f01_probe(struct rmi_function *fn)
 {
-	u8 temp;
-	int error;
-	u16 ctrl_base_addr;
 	struct rmi_device *rmi_dev = fn->rmi_dev;
 	struct rmi_driver_data *driver_data = dev_get_drvdata(&rmi_dev->dev);
-	struct f01_data *data = fn->data;
-	struct rmi_device_platform_data *pdata = to_rmi_platform_data(rmi_dev);
+	const struct rmi_device_platform_data *pdata =
+				to_rmi_platform_data(rmi_dev);
+	struct f01_data *f01;
+	size_t f01_size;
+	int error;
+	u16 ctrl_base_addr;
+	u8 device_status;
+	u8 temp;
+
+	f01_size = sizeof(struct f01_data) +
+				sizeof(u8) * driver_data->num_of_irq_regs;
+	f01 = devm_kzalloc(&fn->dev, f01_size, GFP_KERNEL);
+	if (!f01) {
+		dev_err(&fn->dev, "Failed to allocate fn01_data.\n");
+		return -ENOMEM;
+	}
 
-	mutex_init(&data->control_mutex);
+	f01->num_of_irq_regs = driver_data->num_of_irq_regs;
+	f01->device_control.interrupt_enable = f01->interrupt_enable;
 
 	/*
 	 * Set the configured bit and (optionally) other important stuff
 	 * in the device control register.
 	 */
 	ctrl_base_addr = fn->fd.control_base_addr;
-	error = rmi_read_block(rmi_dev, fn->fd.control_base_addr,
-			&data->device_control.ctrl0,
-			sizeof(data->device_control.ctrl0));
-	if (error < 0) {
-		dev_err(&fn->dev, "Failed to read F01 control.\n");
+
+	error = rmi_read(rmi_dev, fn->fd.control_base_addr,
+			 &f01->device_control.ctrl0);
+	if (error) {
+		dev_err(&fn->dev, "Failed to read F01 control: %d\n", error);
 		return error;
 	}
+
 	switch (pdata->power_management.nosleep) {
 	case RMI_F01_NOSLEEP_DEFAULT:
 		break;
 	case RMI_F01_NOSLEEP_OFF:
-		data->device_control.ctrl0 &= ~RMI_F01_CRTL0_NOSLEEP_BIT;
+		f01->device_control.ctrl0 &= ~RMI_F01_CRTL0_NOSLEEP_BIT;
 		break;
 	case RMI_F01_NOSLEEP_ON:
-		data->device_control.ctrl0 |= RMI_F01_CRTL0_NOSLEEP_BIT;
+		f01->device_control.ctrl0 |= RMI_F01_CRTL0_NOSLEEP_BIT;
 		break;
 	}
 
@@ -244,250 +232,213 @@ static int rmi_f01_initialize(struct rmi_function *fn)
 	 * reboot without power cycle.  If so, clear it so the sensor
 	 * is certain to function.
 	 */
-	if ((data->device_control.ctrl0 & RMI_F01_CTRL0_SLEEP_MODE_MASK) !=
+	if ((f01->device_control.ctrl0 & RMI_F01_CTRL0_SLEEP_MODE_MASK) !=
 			RMI_SLEEP_MODE_NORMAL) {
 		dev_warn(&fn->dev,
 			 "WARNING: Non-zero sleep mode found. Clearing...\n");
-		data->device_control.ctrl0 &= ~RMI_F01_CTRL0_SLEEP_MODE_MASK;
+		f01->device_control.ctrl0 &= ~RMI_F01_CTRL0_SLEEP_MODE_MASK;
 	}
 
-	data->device_control.ctrl0 |= RMI_F01_CRTL0_CONFIGURED_BIT;
+	f01->device_control.ctrl0 |= RMI_F01_CRTL0_CONFIGURED_BIT;
 
-	error = rmi_write_block(rmi_dev, fn->fd.control_base_addr,
-				&data->device_control.ctrl0,
-				sizeof(data->device_control.ctrl0));
-	if (error < 0) {
-		dev_err(&fn->dev, "Failed to write F01 control.\n");
+	error = rmi_write(rmi_dev, fn->fd.control_base_addr,
+			  f01->device_control.ctrl0);
+	if (error) {
+		dev_err(&fn->dev, "Failed to write F01 control: %d\n", error);
 		return error;
 	}
 
-	data->irq_count = driver_data->irq_count;
-	data->num_of_irq_regs = driver_data->num_of_irq_regs;
-	ctrl_base_addr += sizeof(u8);
+	f01->num_of_irq_regs = driver_data->num_of_irq_regs;
+	ctrl_base_addr += sizeof(f01->device_control.ctrl0);
 
-	data->interrupt_enable_addr = ctrl_base_addr;
+	f01->interrupt_enable_addr = ctrl_base_addr;
 	error = rmi_read_block(rmi_dev, ctrl_base_addr,
-				data->device_control.interrupt_enable,
-				sizeof(u8) * (data->num_of_irq_regs));
-	if (error < 0) {
+				f01->device_control.interrupt_enable,
+				sizeof(u8) * (f01->num_of_irq_regs));
+	if (error) {
 		dev_err(&fn->dev,
-			"Failed to read F01 control interrupt enable register.\n");
-		goto error_exit;
+			"Failed to read F01 control interrupt enable register: %d\n",
+			error);
+		return error;
 	}
 
-	ctrl_base_addr += data->num_of_irq_regs;
+	ctrl_base_addr += f01->num_of_irq_regs;
 
 	/* dummy read in order to clear irqs */
 	error = rmi_read(rmi_dev, fn->fd.data_base_addr + 1, &temp);
-	if (error < 0) {
+	if (error) {
 		dev_err(&fn->dev, "Failed to read Interrupt Status.\n");
 		return error;
 	}
 
 	error = rmi_f01_read_properties(rmi_dev, fn->fd.query_base_addr,
-					&data->properties);
-	if (error < 0) {
+					&f01->properties);
+	if (error) {
 		dev_err(&fn->dev, "Failed to read F01 properties.\n");
 		return error;
 	}
+
 	dev_info(&fn->dev, "found RMI device, manufacturer: %s, product: %s\n",
-		 data->properties.manufacturer_id == 1 ?
-							"Synaptics" : "unknown",
-		 data->properties.product_id);
+		 f01->properties.manufacturer_id == 1 ? "Synaptics" : "unknown",
+		 f01->properties.product_id);
 
 	/* read control register */
-	if (data->properties.has_adjustable_doze) {
-		data->doze_interval_addr = ctrl_base_addr;
+	if (f01->properties.has_adjustable_doze) {
+		f01->doze_interval_addr = ctrl_base_addr;
 		ctrl_base_addr++;
 
 		if (pdata->power_management.doze_interval) {
-			data->device_control.doze_interval =
+			f01->device_control.doze_interval =
 				pdata->power_management.doze_interval;
-			error = rmi_write(rmi_dev, data->doze_interval_addr,
-					data->device_control.doze_interval);
-			if (error < 0) {
-				dev_err(&fn->dev, "Failed to configure F01 doze interval register.\n");
-				goto error_exit;
-			}
 		} else {
-			error = rmi_read(rmi_dev, data->doze_interval_addr,
-					&data->device_control.doze_interval);
-			if (error < 0) {
-				dev_err(&fn->dev, "Failed to read F01 doze interval register.\n");
-				goto error_exit;
+			error = rmi_read(rmi_dev, f01->doze_interval_addr,
+					&f01->device_control.doze_interval);
+			if (error) {
+				dev_err(&fn->dev,
+					"Failed to read F01 doze interval register: %d\n",
+					error);
+				return error;
 			}
 		}
 
-		data->wakeup_threshold_addr = ctrl_base_addr;
+		f01->wakeup_threshold_addr = ctrl_base_addr;
 		ctrl_base_addr++;
 
 		if (pdata->power_management.wakeup_threshold) {
-			data->device_control.wakeup_threshold =
+			f01->device_control.wakeup_threshold =
 				pdata->power_management.wakeup_threshold;
-			error = rmi_write(rmi_dev, data->wakeup_threshold_addr,
-					data->device_control.wakeup_threshold);
-			if (error < 0) {
-				dev_err(&fn->dev, "Failed to configure F01 wakeup threshold register.\n");
-				goto error_exit;
-			}
 		} else {
-			error = rmi_read(rmi_dev, data->wakeup_threshold_addr,
-					&data->device_control.wakeup_threshold);
-			if (error < 0) {
-				dev_err(&fn->dev, "Failed to read F01 wakeup threshold register.\n");
-				goto error_exit;
+			error = rmi_read(rmi_dev, f01->wakeup_threshold_addr,
+					 &f01->device_control.wakeup_threshold);
+			if (error) {
+				dev_err(&fn->dev,
+					"Failed to read F01 wakeup threshold register: %d\n",
+					error);
+				return error;
 			}
 		}
 	}
 
-	if (data->properties.has_adjustable_doze_holdoff) {
-		data->doze_holdoff_addr = ctrl_base_addr;
+	if (f01->properties.has_adjustable_doze_holdoff) {
+		f01->doze_holdoff_addr = ctrl_base_addr;
 		ctrl_base_addr++;
 
 		if (pdata->power_management.doze_holdoff) {
-			data->device_control.doze_holdoff =
+			f01->device_control.doze_holdoff =
 				pdata->power_management.doze_holdoff;
-			error = rmi_write(rmi_dev, data->doze_holdoff_addr,
-					data->device_control.doze_holdoff);
-			if (error < 0) {
-				dev_err(&fn->dev, "Failed to configure F01 doze holdoff register.\n");
-				goto error_exit;
-			}
 		} else {
-			error = rmi_read(rmi_dev, data->doze_holdoff_addr,
-					&data->device_control.doze_holdoff);
-			if (error < 0) {
-				dev_err(&fn->dev, "Failed to read F01 doze holdoff register.\n");
-				goto error_exit;
+			error = rmi_read(rmi_dev, f01->doze_holdoff_addr,
+					 &f01->device_control.doze_holdoff);
+			if (error) {
+				dev_err(&fn->dev,
+					"Failed to read F01 doze holdoff register: %d\n",
+					error);
+				return error;
 			}
 		}
 	}
 
-	error = rmi_read_block(rmi_dev, fn->fd.data_base_addr,
-		&data->device_status, sizeof(data->device_status));
-	if (error < 0) {
-		dev_err(&fn->dev, "Failed to read device status.\n");
-		goto error_exit;
+	error = rmi_read(rmi_dev, fn->fd.data_base_addr, &device_status);
+	if (error) {
+		dev_err(&fn->dev,
+			"Failed to read device status: %d\n", error);
+		return error;
 	}
 
-	if (RMI_F01_STATUS_UNCONFIGURED(data->device_status)) {
+	if (RMI_F01_STATUS_UNCONFIGURED(device_status)) {
 		dev_err(&fn->dev,
 			"Device was reset during configuration process, status: %#02x!\n",
-			RMI_F01_STATUS_CODE(data->device_status));
-		error = -EINVAL;
-		goto error_exit;
+			RMI_F01_STATUS_CODE(device_status));
+		return -EINVAL;
 	}
 
-	return 0;
+	fn->data = f01;
 
- error_exit:
-	kfree(data);
-	return error;
+	return 0;
 }
 
 static int rmi_f01_config(struct rmi_function *fn)
 {
-	struct f01_data *data = fn->data;
-	int retval;
-
-	retval = rmi_write_block(fn->rmi_dev, fn->fd.control_base_addr,
-				 &data->device_control.ctrl0,
-				 sizeof(data->device_control.ctrl0));
-	if (retval < 0) {
-		dev_err(&fn->dev, "Failed to write device_control.reg.\n");
-		return retval;
-	}
+	struct f01_data *f01 = fn->data;
+	int error;
 
-	retval = rmi_write_block(fn->rmi_dev, data->interrupt_enable_addr,
-				 data->device_control.interrupt_enable,
-				 sizeof(u8) * data->num_of_irq_regs);
+	error = rmi_write(fn->rmi_dev, fn->fd.control_base_addr,
+			  f01->device_control.ctrl0);
+	if (error) {
+		dev_err(&fn->dev,
+			"Failed to write device_control reg: %d\n", error);
+		return error;
+	}
 
-	if (retval < 0) {
-		dev_err(&fn->dev, "Failed to write interrupt enable.\n");
-		return retval;
+	error = rmi_write_block(fn->rmi_dev, f01->interrupt_enable_addr,
+				f01->interrupt_enable,
+				sizeof(u8) * f01->num_of_irq_regs);
+	if (error) {
+		dev_err(&fn->dev,
+			"Failed to write interrupt enable: %d\n", error);
+		return error;
 	}
-	if (data->properties.has_lts) {
-		retval = rmi_write_block(fn->rmi_dev, data->doze_interval_addr,
-					 &data->device_control.doze_interval,
-					 sizeof(u8));
-		if (retval < 0) {
-			dev_err(&fn->dev, "Failed to write doze interval.\n");
-			return retval;
+
+	/* XXX: why we check has_lts here but has_adjustable_doze in probe? */
+	if (f01->properties.has_lts) {
+		error = rmi_write(fn->rmi_dev, f01->doze_interval_addr,
+				  f01->device_control.doze_interval);
+		if (error) {
+			dev_err(&fn->dev,
+				"Failed to write doze interval: %d\n", error);
+			return error;
 		}
 	}
 
-	if (data->properties.has_adjustable_doze) {
-		retval = rmi_write_block(fn->rmi_dev,
-					 data->wakeup_threshold_addr,
-					 &data->device_control.wakeup_threshold,
-					 sizeof(u8));
-		if (retval < 0) {
-			dev_err(&fn->dev, "Failed to write wakeup threshold.\n");
-			return retval;
+	if (f01->properties.has_adjustable_doze) {
+		error = rmi_write(fn->rmi_dev, f01->wakeup_threshold_addr,
+				  f01->device_control.wakeup_threshold);
+		if (error) {
+			dev_err(&fn->dev,
+				"Failed to write wakeup threshold: %d\n",
+				error);
+			return error;
 		}
 	}
 
-	if (data->properties.has_adjustable_doze_holdoff) {
-		retval = rmi_write_block(fn->rmi_dev, data->doze_holdoff_addr,
-					 &data->device_control.doze_holdoff,
-					 sizeof(u8));
-		if (retval < 0) {
-			dev_err(&fn->dev, "Failed to write doze holdoff.\n");
-			return retval;
+	if (f01->properties.has_adjustable_doze_holdoff) {
+		error = rmi_write(fn->rmi_dev, f01->doze_holdoff_addr,
+				  f01->device_control.doze_holdoff);
+		if (error) {
+			dev_err(&fn->dev,
+				"Failed to write doze holdoff: %d\n", error);
+			return error;
 		}
 	}
-	return 0;
-}
-
-static int rmi_f01_probe(struct rmi_function *fn)
-{
-	struct rmi_driver_data *driver_data =
-			dev_get_drvdata(&fn->rmi_dev->dev);
-	int error;
-
-	error = rmi_f01_alloc_memory(fn, driver_data->num_of_irq_regs);
-	if (error)
-		return error;
-
-	error = rmi_f01_initialize(fn);
-	if (error)
-		return error;
 
 	return 0;
 }
 
-static void rmi_f01_remove(struct rmi_function *fn)
-{
-	/* Placeholder for now. */
-}
-
 #ifdef CONFIG_PM_SLEEP
 static int rmi_f01_suspend(struct device *dev)
 {
 	struct rmi_function *fn = to_rmi_function(dev);
 	struct rmi_device *rmi_dev = fn->rmi_dev;
-	struct f01_data *data = fn->data;
+	struct f01_data *f01 = fn->data;
 	int error;
 
-	data->old_nosleep = data->device_control.ctrl0 &
-					RMI_F01_CRTL0_NOSLEEP_BIT;
-	data->device_control.ctrl0 &= ~RMI_F01_CRTL0_NOSLEEP_BIT;
+	f01->old_nosleep =
+		f01->device_control.ctrl0 & RMI_F01_CRTL0_NOSLEEP_BIT;
 
-	data->device_control.ctrl0 &= ~RMI_F01_CTRL0_SLEEP_MODE_MASK;
-	data->device_control.ctrl0 |= RMI_SLEEP_MODE_SENSOR_SLEEP;
+	f01->device_control.ctrl0 &= ~RMI_F01_CRTL0_NOSLEEP_BIT;
 
-	error = rmi_write_block(rmi_dev,
-				fn->fd.control_base_addr,
-				&data->device_control.ctrl0,
-				sizeof(data->device_control.ctrl0));
-	if (error < 0) {
-		dev_err(&fn->dev, "Failed to write sleep mode. Code: %d.\n",
-			error);
-		if (data->old_nosleep)
-			data->device_control.ctrl0 |=
-					RMI_F01_CRTL0_NOSLEEP_BIT;
-		data->device_control.ctrl0 &= ~RMI_F01_CTRL0_SLEEP_MODE_MASK;
-		data->device_control.ctrl0 |= RMI_SLEEP_MODE_NORMAL;
+	f01->device_control.ctrl0 &= ~RMI_F01_CTRL0_SLEEP_MODE_MASK;
+	f01->device_control.ctrl0 |= RMI_SLEEP_MODE_SENSOR_SLEEP;
+
+	error = rmi_write(rmi_dev, fn->fd.control_base_addr,
+			  f01->device_control.ctrl0);
+	if (error) {
+		dev_err(&fn->dev,
+			"Failed to write sleep mode: %d.\n", error);
+		if (f01->old_nosleep)
+			f01->device_control.ctrl0 |= RMI_F01_CRTL0_NOSLEEP_BIT;
+		f01->device_control.ctrl0 &= ~RMI_F01_CTRL0_SLEEP_MODE_MASK;
+		f01->device_control.ctrl0 |= RMI_SLEEP_MODE_NORMAL;
 		return error;
 	}
 
@@ -498,22 +449,20 @@ static int rmi_f01_resume(struct device *dev)
 {
 	struct rmi_function *fn = to_rmi_function(dev);
 	struct rmi_device *rmi_dev = fn->rmi_dev;
-	struct f01_data *data = fn->data;
+	struct f01_data *f01 = fn->data;
 	int error;
 
-	if (data->old_nosleep)
-		data->device_control.ctrl0 |= RMI_F01_CRTL0_NOSLEEP_BIT;
+	if (f01->old_nosleep)
+		f01->device_control.ctrl0 |= RMI_F01_CRTL0_NOSLEEP_BIT;
 
-	data->device_control.ctrl0 &= ~RMI_F01_CTRL0_SLEEP_MODE_MASK;
-	data->device_control.ctrl0 |= RMI_SLEEP_MODE_NORMAL;
+	f01->device_control.ctrl0 &= ~RMI_F01_CTRL0_SLEEP_MODE_MASK;
+	f01->device_control.ctrl0 |= RMI_SLEEP_MODE_NORMAL;
 
-	error = rmi_write_block(rmi_dev, fn->fd.control_base_addr,
-				&data->device_control.ctrl0,
-				sizeof(data->device_control.ctrl0));
-	if (error < 0) {
+	error = rmi_write(rmi_dev, fn->fd.control_base_addr,
+			  f01->device_control.ctrl0);
+	if (error) {
 		dev_err(&fn->dev,
-			"Failed to restore normal operation. Code: %d.\n",
-			error);
+			"Failed to restore normal operation: %d.\n", error);
 		return error;
 	}
 
@@ -527,22 +476,21 @@ static int rmi_f01_attention(struct rmi_function *fn,
 			     unsigned long *irq_bits)
 {
 	struct rmi_device *rmi_dev = fn->rmi_dev;
-	struct f01_data *data = fn->data;
-	int retval;
-
-	retval = rmi_read_block(rmi_dev, fn->fd.data_base_addr,
-		&data->device_status, sizeof(data->device_status));
-	if (retval < 0) {
-		dev_err(&fn->dev, "Failed to read device status, code: %d.\n",
-			retval);
-		return retval;
+	int error;
+	u8 device_status;
+
+	error = rmi_read(rmi_dev, fn->fd.data_base_addr, &device_status);
+	if (error) {
+		dev_err(&fn->dev,
+			"Failed to read device status: %d.\n", error);
+		return error;
 	}
 
-	if (RMI_F01_STATUS_UNCONFIGURED(data->device_status)) {
+	if (RMI_F01_STATUS_UNCONFIGURED(device_status)) {
 		dev_warn(&fn->dev, "Device reset detected.\n");
-		retval = rmi_dev->driver->reset_handler(rmi_dev);
-		if (retval < 0)
-			return retval;
+		error = rmi_dev->driver->reset_handler(rmi_dev);
+		if (error < 0)
+			return error;
 	}
 	return 0;
 }
@@ -559,7 +507,6 @@ static struct rmi_function_handler rmi_f01_handler = {
 	},
 	.func		= 0x01,
 	.probe		= rmi_f01_probe,
-	.remove		= rmi_f01_remove,
 	.config		= rmi_f01_config,
 	.attention	= rmi_f01_attention,
 };

Re: [PATCH] input synaptics-rmi4: rmi_f01.c storage fix

[Courtney Cavin]

On Wed, Feb 12, 2014 at 07:40:49AM +0100, Dmitry Torokhov wrote:
> Hi Chris,
> 
> On Tue, Feb 11, 2014 at 03:13:00PM -0800, Christopher Heiny wrote:
> > Correctly free driver related data when initialization fails.
> >
> > Trivial: Clarify a diagnostic message.
> >
> > Signed-off-by: Christopher Heiny <cheiny@synaptics.com>
> > Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> > Cc: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> > Cc: Linux Walleij <linus.walleij@linaro.org>
> > Cc: David Herrmann <dh.herrmann@gmail.com>
> > Cc: Jiri Kosina <jkosina@suse.cz>
> >
> > ---
> >
> >  drivers/input/rmi4/rmi_f01.c | 6 ++++--
> >  1 file changed, 4 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/input/rmi4/rmi_f01.c b/drivers/input/rmi4/rmi_f01.c
> > index 381ad60..e4a6df9 100644
> > --- a/drivers/input/rmi4/rmi_f01.c
> > +++ b/drivers/input/rmi4/rmi_f01.c
> > @@ -149,7 +149,7 @@ static int rmi_f01_alloc_memory(struct rmi_function *fn,
> >
> >       f01 = devm_kzalloc(&fn->dev, sizeof(struct f01_data), GFP_KERNEL);
> >       if (!f01) {
> > -             dev_err(&fn->dev, "Failed to allocate fn_01_data.\n");
> > +             dev_err(&fn->dev, "Failed to allocate f01_data.\n");
> >               return -ENOMEM;
> >       }
> >
> > @@ -158,6 +158,7 @@ static int rmi_f01_alloc_memory(struct rmi_function *fn,
> >                       GFP_KERNEL);
> >       if (!f01->device_control.interrupt_enable) {
> >               dev_err(&fn->dev, "Failed to allocate interrupt enable.\n");
> > +             devm_kfree(&fn->dev, f01);
> 
> As Courtney mentioned if you are calling devm_kfree() you are most
> likely doing something wrong.
> 
> How about the patch below? Please check the XXX comment, I have some
> concerns about lts vs doze_holdoff check mismatch in probe() and
> config().
> 
> Thanks.
> 
> --
> Dmitry
> 
> Input: synaptics-rmi4 - F01 initialization cleanup
> 
> From: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> 
> - rename data to f01 where appropriate;
> - switch to using rmi_read()/rmi_write() for single-byte data;
> - allocate interrupt mask together with the main structure;
> - do not kfree() memory allocated with devm;
> - do not write config data in probe(), we have config() for that;
> - drop unneeded rmi_f01_remove().

These seem like unrelated changes and make this patch hard to read, I
would prefer if we could separate these out.  Perhaps like so?
	[1] bug-fix
		- do not kfree() memory allocated with devm
	[2] simplify probe/remove logic
		- allocate interrupt mask together with the main structure
		- do not write config data in probe(), we have config() for that
		- drop unneeded rmi_f01_remove()
	[3] non-behavioral changes/cleanup
		- switch to using rmi_read()/rmi_write() for single-byte data
		- rename data to f01 where appropriate

Disregarding that, and the nitpick below, it looks good to me.

> 
> Reported-by: Courtney Cavin <courtney.cavin@sonymobile.com>
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> ---
>  drivers/input/rmi4/rmi_f01.c |  397 ++++++++++++++++++------------------------
>  1 file changed, 172 insertions(+), 225 deletions(-)
> 
> diff --git a/drivers/input/rmi4/rmi_f01.c b/drivers/input/rmi4/rmi_f01.c
> index 381ad60..8f7840e 100644
> --- a/drivers/input/rmi4/rmi_f01.c
> +++ b/drivers/input/rmi4/rmi_f01.c
[...] 
> -static int rmi_f01_initialize(struct rmi_function *fn)
> +static int rmi_f01_probe(struct rmi_function *fn)
>  {
> -       u8 temp;
> -       int error;
> -       u16 ctrl_base_addr;
>         struct rmi_device *rmi_dev = fn->rmi_dev;
>         struct rmi_driver_data *driver_data = dev_get_drvdata(&rmi_dev->dev);
> -       struct f01_data *data = fn->data;
> -       struct rmi_device_platform_data *pdata = to_rmi_platform_data(rmi_dev);
> +       const struct rmi_device_platform_data *pdata =
> +                               to_rmi_platform_data(rmi_dev);
> +       struct f01_data *f01;
> +       size_t f01_size;
> +       int error;
> +       u16 ctrl_base_addr;
> +       u8 device_status;
> +       u8 temp;
> +
> +       f01_size = sizeof(struct f01_data) +
> +                               sizeof(u8) * driver_data->num_of_irq_regs;
> +       f01 = devm_kzalloc(&fn->dev, f01_size, GFP_KERNEL);
> +       if (!f01) {
> +               dev_err(&fn->dev, "Failed to allocate fn01_data.\n");

Nitpick: Can we drop this printout in the process?  It's much less
useful than the error and backtrace coming from kmalloc on failure anyway.

> +               return -ENOMEM;
> +       }
[...]

> +       /* XXX: why we check has_lts here but has_adjustable_doze in probe? */

Hrm.  This register is poorly documented in the spec.  All of these bits
are reserved.  Chris, is there a newer version of the spec which
documents these bits?

-Courtney

Re: [PATCH] input synaptics-rmi4: rmi_f01.c storage fix

[Christopher Heiny]

On 02/11/2014 10:40 PM, Dmitry Torokhov wrote:
> Hi Chris,
>
> On Tue, Feb 11, 2014 at 03:13:00PM -0800, Christopher Heiny wrote:
>> >Correctly free driver related data when initialization fails.
>> >
>> >Trivial: Clarify a diagnostic message.
>> >
>> >Signed-off-by: Christopher Heiny<cheiny@synaptics.com>
>> >Cc: Dmitry Torokhov<dmitry.torokhov@gmail.com>
>> >Cc: Benjamin Tissoires<benjamin.tissoires@redhat.com>
>> >Cc: Linux Walleij<linus.walleij@linaro.org>
>> >Cc: David Herrmann<dh.herrmann@gmail.com>
>> >Cc: Jiri Kosina<jkosina@suse.cz>
>> >
>> >---
>> >
>> >  drivers/input/rmi4/rmi_f01.c | 6 ++++--
>> >  1 file changed, 4 insertions(+), 2 deletions(-)
>> >
>> >diff --git a/drivers/input/rmi4/rmi_f01.c b/drivers/input/rmi4/rmi_f01.c
>> >index 381ad60..e4a6df9 100644
>> >--- a/drivers/input/rmi4/rmi_f01.c
>> >+++ b/drivers/input/rmi4/rmi_f01.c
>> >@@ -149,7 +149,7 @@ static int rmi_f01_alloc_memory(struct rmi_function *fn,
>> >
>> >  	f01 = devm_kzalloc(&fn->dev, sizeof(struct f01_data), GFP_KERNEL);
>> >  	if (!f01) {
>> >-		dev_err(&fn->dev, "Failed to allocate fn_01_data.\n");
>> >+		dev_err(&fn->dev, "Failed to allocate f01_data.\n");
>> >  		return -ENOMEM;
>> >  	}
>> >
>> >@@ -158,6 +158,7 @@ static int rmi_f01_alloc_memory(struct rmi_function *fn,
>> >  			GFP_KERNEL);
>> >  	if (!f01->device_control.interrupt_enable) {
>> >  		dev_err(&fn->dev, "Failed to allocate interrupt enable.\n");
>> >+		devm_kfree(&fn->dev, f01);
> As Courtney mentioned if you are calling devm_kfree() you are most
> likely doing something wrong.
>
> How about the patch below? Please check the XXX comment, I have some
> concerns about lts vs doze_holdoff check mismatch in probe() and
> config().

Thanks for calling attention to the has_lts vs doze_holdoff check. 
There was actually a bug in two places relating to that.  It looks like 
a case of cut/paste dyslexia (or something like that).  Anyway, here's a 
version of your patch with those two bugs fixed.  I've tested this on 
Nexus 4.

					Chris


Input: synaptics-rmi4 - F01 initialization cleanup

From: Dmitry Torokhov <dmitry.torokhov@gmail.com>

- rename data to f01 where appropriate;
- switch to using rmi_read()/rmi_write() for single-byte data;
- allocate interrupt mask together with the main structure;
- do not kfree() memory allocated with devm;
- do not write config data in probe(), we have config() for that;
- drop unneeded rmi_f01_remove().
- correctly calculate control register address based on has_lts
- correctly write adjustable doze control registers

Reported-by: Courtney Cavin <courtney.cavin@sonymobile.com>
Reported-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Signed-off-by: Christopher Heiny <cheiny@synaptics.com>

---

  drivers/input/rmi4/rmi_f01.c | 400 
+++++++++++++++++++------------------------
  1 file changed, 173 insertions(+), 227 deletions(-)

diff --git a/drivers/input/rmi4/rmi_f01.c b/drivers/input/rmi4/rmi_f01.c
index 381ad60..9932548 100644
--- a/drivers/input/rmi4/rmi_f01.c
+++ b/drivers/input/rmi4/rmi_f01.c
@@ -123,47 +123,21 @@ struct f01_device_control {

  struct f01_data {
  	struct f01_basic_properties properties;
-
  	struct f01_device_control device_control;
-	struct mutex control_mutex;
-
-	u8 device_status;

  	u16 interrupt_enable_addr;
  	u16 doze_interval_addr;
  	u16 wakeup_threshold_addr;
  	u16 doze_holdoff_addr;
-	int irq_count;
-	int num_of_irq_regs;

  #ifdef CONFIG_PM_SLEEP
  	bool suspended;
  	bool old_nosleep;
  #endif
-};
-
-static int rmi_f01_alloc_memory(struct rmi_function *fn,
-				int num_of_irq_regs)
-{
-	struct f01_data *f01;
-
-	f01 = devm_kzalloc(&fn->dev, sizeof(struct f01_data), GFP_KERNEL);
-	if (!f01) {
-		dev_err(&fn->dev, "Failed to allocate fn_01_data.\n");
-		return -ENOMEM;
-	}
-
-	f01->device_control.interrupt_enable = devm_kzalloc(&fn->dev,
-			sizeof(u8) * (num_of_irq_regs),
-			GFP_KERNEL);
-	if (!f01->device_control.interrupt_enable) {
-		dev_err(&fn->dev, "Failed to allocate interrupt enable.\n");
-		return -ENOMEM;
-	}
-	fn->data = f01;

-	return 0;
-}
+	unsigned int num_of_irq_regs;
+	u8 interrupt_enable[];
+};

  static int rmi_f01_read_properties(struct rmi_device *rmi_dev,
  				   u16 query_base_addr,
@@ -174,8 +148,9 @@ static int rmi_f01_read_properties(struct rmi_device 
*rmi_dev,

  	error = rmi_read_block(rmi_dev, query_base_addr,
  			       basic_query, sizeof(basic_query));
-	if (error < 0) {
-		dev_err(&rmi_dev->dev, "Failed to read device query registers.\n");
+	if (error) {
+		dev_err(&rmi_dev->dev,
+			"Failed to read device query registers: %d\n", error);
  		return error;
  	}

@@ -204,38 +179,51 @@ static int rmi_f01_read_properties(struct 
rmi_device *rmi_dev,
  	return 0;
  }

-static int rmi_f01_initialize(struct rmi_function *fn)
+static int rmi_f01_probe(struct rmi_function *fn)
  {
-	u8 temp;
-	int error;
-	u16 ctrl_base_addr;
  	struct rmi_device *rmi_dev = fn->rmi_dev;
  	struct rmi_driver_data *driver_data = dev_get_drvdata(&rmi_dev->dev);
-	struct f01_data *data = fn->data;
-	struct rmi_device_platform_data *pdata = to_rmi_platform_data(rmi_dev);
+	const struct rmi_device_platform_data *pdata =
+				to_rmi_platform_data(rmi_dev);
+	struct f01_data *f01;
+	size_t f01_size;
+	int error;
+	u16 ctrl_base_addr;
+	u8 device_status;
+	u8 temp;

-	mutex_init(&data->control_mutex);
+	f01_size = sizeof(struct f01_data) +
+				sizeof(u8) * driver_data->num_of_irq_regs;
+	f01 = devm_kzalloc(&fn->dev, f01_size, GFP_KERNEL);
+	if (!f01) {
+		dev_err(&fn->dev, "Failed to allocate fn01_data.\n");
+		return -ENOMEM;
+	}
+
+	f01->num_of_irq_regs = driver_data->num_of_irq_regs;
+	f01->device_control.interrupt_enable = f01->interrupt_enable;

  	/*
  	 * Set the configured bit and (optionally) other important stuff
  	 * in the device control register.
  	 */
  	ctrl_base_addr = fn->fd.control_base_addr;
-	error = rmi_read_block(rmi_dev, fn->fd.control_base_addr,
-			&data->device_control.ctrl0,
-			sizeof(data->device_control.ctrl0));
-	if (error < 0) {
-		dev_err(&fn->dev, "Failed to read F01 control.\n");
+
+	error = rmi_read(rmi_dev, fn->fd.control_base_addr,
+			 &f01->device_control.ctrl0);
+	if (error) {
+		dev_err(&fn->dev, "Failed to read F01 control: %d\n", error);
  		return error;
  	}
+
  	switch (pdata->power_management.nosleep) {
  	case RMI_F01_NOSLEEP_DEFAULT:
  		break;
  	case RMI_F01_NOSLEEP_OFF:
-		data->device_control.ctrl0 &= ~RMI_F01_CRTL0_NOSLEEP_BIT;
+		f01->device_control.ctrl0 &= ~RMI_F01_CRTL0_NOSLEEP_BIT;
  		break;
  	case RMI_F01_NOSLEEP_ON:
-		data->device_control.ctrl0 |= RMI_F01_CRTL0_NOSLEEP_BIT;
+		f01->device_control.ctrl0 |= RMI_F01_CRTL0_NOSLEEP_BIT;
  		break;
  	}

@@ -244,250 +232,212 @@ static int rmi_f01_initialize(struct 
rmi_function *fn)
  	 * reboot without power cycle.  If so, clear it so the sensor
  	 * is certain to function.
  	 */
-	if ((data->device_control.ctrl0 & RMI_F01_CTRL0_SLEEP_MODE_MASK) !=
+	if ((f01->device_control.ctrl0 & RMI_F01_CTRL0_SLEEP_MODE_MASK) !=
  			RMI_SLEEP_MODE_NORMAL) {
  		dev_warn(&fn->dev,
  			 "WARNING: Non-zero sleep mode found. Clearing...\n");
-		data->device_control.ctrl0 &= ~RMI_F01_CTRL0_SLEEP_MODE_MASK;
+		f01->device_control.ctrl0 &= ~RMI_F01_CTRL0_SLEEP_MODE_MASK;
  	}

-	data->device_control.ctrl0 |= RMI_F01_CRTL0_CONFIGURED_BIT;
+	f01->device_control.ctrl0 |= RMI_F01_CRTL0_CONFIGURED_BIT;

-	error = rmi_write_block(rmi_dev, fn->fd.control_base_addr,
-				&data->device_control.ctrl0,
-				sizeof(data->device_control.ctrl0));
-	if (error < 0) {
-		dev_err(&fn->dev, "Failed to write F01 control.\n");
+	error = rmi_write(rmi_dev, fn->fd.control_base_addr,
+			  f01->device_control.ctrl0);
+	if (error) {
+		dev_err(&fn->dev, "Failed to write F01 control: %d\n", error);
  		return error;
  	}

-	data->irq_count = driver_data->irq_count;
-	data->num_of_irq_regs = driver_data->num_of_irq_regs;
-	ctrl_base_addr += sizeof(u8);
+	f01->num_of_irq_regs = driver_data->num_of_irq_regs;
+	ctrl_base_addr += sizeof(f01->device_control.ctrl0);

-	data->interrupt_enable_addr = ctrl_base_addr;
+	f01->interrupt_enable_addr = ctrl_base_addr;
  	error = rmi_read_block(rmi_dev, ctrl_base_addr,
-				data->device_control.interrupt_enable,
-				sizeof(u8) * (data->num_of_irq_regs));
-	if (error < 0) {
+				f01->device_control.interrupt_enable,
+				sizeof(u8) * (f01->num_of_irq_regs));
+	if (error) {
  		dev_err(&fn->dev,
-			"Failed to read F01 control interrupt enable register.\n");
-		goto error_exit;
+			"Failed to read F01 control interrupt enable register: %d\n",
+			error);
+		return error;
  	}

-	ctrl_base_addr += data->num_of_irq_regs;
+	ctrl_base_addr += f01->num_of_irq_regs;

  	/* dummy read in order to clear irqs */
  	error = rmi_read(rmi_dev, fn->fd.data_base_addr + 1, &temp);
-	if (error < 0) {
+	if (error) {
  		dev_err(&fn->dev, "Failed to read Interrupt Status.\n");
  		return error;
  	}

  	error = rmi_f01_read_properties(rmi_dev, fn->fd.query_base_addr,
-					&data->properties);
-	if (error < 0) {
+					&f01->properties);
+	if (error) {
  		dev_err(&fn->dev, "Failed to read F01 properties.\n");
  		return error;
  	}
+
  	dev_info(&fn->dev, "found RMI device, manufacturer: %s, product: %s\n",
-		 data->properties.manufacturer_id == 1 ?
-							"Synaptics" : "unknown",
-		 data->properties.product_id);
+		 f01->properties.manufacturer_id == 1 ? "Synaptics" : "unknown",
+		 f01->properties.product_id);

  	/* read control register */
-	if (data->properties.has_adjustable_doze) {
-		data->doze_interval_addr = ctrl_base_addr;
+	if (f01->properties.has_adjustable_doze) {
+		f01->doze_interval_addr = ctrl_base_addr;
  		ctrl_base_addr++;

  		if (pdata->power_management.doze_interval) {
-			data->device_control.doze_interval =
+			f01->device_control.doze_interval =
  				pdata->power_management.doze_interval;
-			error = rmi_write(rmi_dev, data->doze_interval_addr,
-					data->device_control.doze_interval);
-			if (error < 0) {
-				dev_err(&fn->dev, "Failed to configure F01 doze interval register.\n");
-				goto error_exit;
-			}
  		} else {
-			error = rmi_read(rmi_dev, data->doze_interval_addr,
-					&data->device_control.doze_interval);
-			if (error < 0) {
-				dev_err(&fn->dev, "Failed to read F01 doze interval register.\n");
-				goto error_exit;
+			error = rmi_read(rmi_dev, f01->doze_interval_addr,
+					&f01->device_control.doze_interval);
+			if (error) {
+				dev_err(&fn->dev,
+					"Failed to read F01 doze interval register: %d\n",
+					error);
+				return error;
  			}
  		}

-		data->wakeup_threshold_addr = ctrl_base_addr;
+		f01->wakeup_threshold_addr = ctrl_base_addr;
  		ctrl_base_addr++;

  		if (pdata->power_management.wakeup_threshold) {
-			data->device_control.wakeup_threshold =
+			f01->device_control.wakeup_threshold =
  				pdata->power_management.wakeup_threshold;
-			error = rmi_write(rmi_dev, data->wakeup_threshold_addr,
-					data->device_control.wakeup_threshold);
-			if (error < 0) {
-				dev_err(&fn->dev, "Failed to configure F01 wakeup threshold 
register.\n");
-				goto error_exit;
-			}
  		} else {
-			error = rmi_read(rmi_dev, data->wakeup_threshold_addr,
-					&data->device_control.wakeup_threshold);
-			if (error < 0) {
-				dev_err(&fn->dev, "Failed to read F01 wakeup threshold register.\n");
-				goto error_exit;
+			error = rmi_read(rmi_dev, f01->wakeup_threshold_addr,
+					 &f01->device_control.wakeup_threshold);
+			if (error) {
+				dev_err(&fn->dev,
+					"Failed to read F01 wakeup threshold register: %d\n",
+					error);
+				return error;
  			}
  		}
  	}

-	if (data->properties.has_adjustable_doze_holdoff) {
-		data->doze_holdoff_addr = ctrl_base_addr;
+	if (f01->properties.has_lts)
+		ctrl_base_addr++;
+
+	if (f01->properties.has_adjustable_doze_holdoff) {
+		f01->doze_holdoff_addr = ctrl_base_addr;
  		ctrl_base_addr++;

  		if (pdata->power_management.doze_holdoff) {
-			data->device_control.doze_holdoff =
+			f01->device_control.doze_holdoff =
  				pdata->power_management.doze_holdoff;
-			error = rmi_write(rmi_dev, data->doze_holdoff_addr,
-					data->device_control.doze_holdoff);
-			if (error < 0) {
-				dev_err(&fn->dev, "Failed to configure F01 doze holdoff register.\n");
-				goto error_exit;
-			}
  		} else {
-			error = rmi_read(rmi_dev, data->doze_holdoff_addr,
-					&data->device_control.doze_holdoff);
-			if (error < 0) {
-				dev_err(&fn->dev, "Failed to read F01 doze holdoff register.\n");
-				goto error_exit;
+			error = rmi_read(rmi_dev, f01->doze_holdoff_addr,
+					 &f01->device_control.doze_holdoff);
+			if (error) {
+				dev_err(&fn->dev,
+					"Failed to read F01 doze holdoff register: %d\n",
+					error);
+				return error;
  			}
  		}
  	}

-	error = rmi_read_block(rmi_dev, fn->fd.data_base_addr,
-		&data->device_status, sizeof(data->device_status));
-	if (error < 0) {
-		dev_err(&fn->dev, "Failed to read device status.\n");
-		goto error_exit;
+	error = rmi_read(rmi_dev, fn->fd.data_base_addr, &device_status);
+	if (error) {
+		dev_err(&fn->dev,
+			"Failed to read device status: %d\n", error);
+		return error;
  	}

-	if (RMI_F01_STATUS_UNCONFIGURED(data->device_status)) {
+	if (RMI_F01_STATUS_UNCONFIGURED(device_status)) {
  		dev_err(&fn->dev,
  			"Device was reset during configuration process, status: %#02x!\n",
-			RMI_F01_STATUS_CODE(data->device_status));
-		error = -EINVAL;
-		goto error_exit;
+			RMI_F01_STATUS_CODE(device_status));
+		return -EINVAL;
  	}

-	return 0;
+	fn->data = f01;

- error_exit:
-	kfree(data);
-	return error;
+	return 0;
  }

  static int rmi_f01_config(struct rmi_function *fn)
  {
-	struct f01_data *data = fn->data;
-	int retval;
-
-	retval = rmi_write_block(fn->rmi_dev, fn->fd.control_base_addr,
-				 &data->device_control.ctrl0,
-				 sizeof(data->device_control.ctrl0));
-	if (retval < 0) {
-		dev_err(&fn->dev, "Failed to write device_control.reg.\n");
-		return retval;
-	}
-
-	retval = rmi_write_block(fn->rmi_dev, data->interrupt_enable_addr,
-				 data->device_control.interrupt_enable,
-				 sizeof(u8) * data->num_of_irq_regs);
+	struct f01_data *f01 = fn->data;
+	int error;

-	if (retval < 0) {
-		dev_err(&fn->dev, "Failed to write interrupt enable.\n");
-		return retval;
+	error = rmi_write(fn->rmi_dev, fn->fd.control_base_addr,
+			  f01->device_control.ctrl0);
+	if (error) {
+		dev_err(&fn->dev,
+			"Failed to write device_control reg: %d\n", error);
+		return error;
  	}
-	if (data->properties.has_lts) {
-		retval = rmi_write_block(fn->rmi_dev, data->doze_interval_addr,
-					 &data->device_control.doze_interval,
-					 sizeof(u8));
-		if (retval < 0) {
-			dev_err(&fn->dev, "Failed to write doze interval.\n");
-			return retval;
-		}
+
+	error = rmi_write_block(fn->rmi_dev, f01->interrupt_enable_addr,
+				f01->interrupt_enable,
+				sizeof(u8) * f01->num_of_irq_regs);
+	if (error) {
+		dev_err(&fn->dev,
+			"Failed to write interrupt enable: %d\n", error);
+		return error;
  	}

-	if (data->properties.has_adjustable_doze) {
-		retval = rmi_write_block(fn->rmi_dev,
-					 data->wakeup_threshold_addr,
-					 &data->device_control.wakeup_threshold,
-					 sizeof(u8));
-		if (retval < 0) {
-			dev_err(&fn->dev, "Failed to write wakeup threshold.\n");
-			return retval;
+	if (f01->properties.has_adjustable_doze) {
+		error = rmi_write(fn->rmi_dev, f01->doze_interval_addr,
+				  f01->device_control.doze_interval);
+		if (error) {
+			dev_err(&fn->dev,
+				"Failed to write doze interval: %d\n", error);
+			return error;
+		}
+		error = rmi_write(fn->rmi_dev, f01->wakeup_threshold_addr,
+				  f01->device_control.wakeup_threshold);
+		if (error) {
+			dev_err(&fn->dev,
+				"Failed to write wakeup threshold: %d\n",
+				error);
+			return error;
  		}
  	}

-	if (data->properties.has_adjustable_doze_holdoff) {
-		retval = rmi_write_block(fn->rmi_dev, data->doze_holdoff_addr,
-					 &data->device_control.doze_holdoff,
-					 sizeof(u8));
-		if (retval < 0) {
-			dev_err(&fn->dev, "Failed to write doze holdoff.\n");
-			return retval;
+	if (f01->properties.has_adjustable_doze_holdoff) {
+		error = rmi_write(fn->rmi_dev, f01->doze_holdoff_addr,
+				  f01->device_control.doze_holdoff);
+		if (error) {
+			dev_err(&fn->dev,
+				"Failed to write doze holdoff: %d\n", error);
+			return error;
  		}
  	}
-	return 0;
-}
-
-static int rmi_f01_probe(struct rmi_function *fn)
-{
-	struct rmi_driver_data *driver_data =
-			dev_get_drvdata(&fn->rmi_dev->dev);
-	int error;
-
-	error = rmi_f01_alloc_memory(fn, driver_data->num_of_irq_regs);
-	if (error)
-		return error;
-
-	error = rmi_f01_initialize(fn);
-	if (error)
-		return error;

  	return 0;
  }

-static void rmi_f01_remove(struct rmi_function *fn)
-{
-	/* Placeholder for now. */
-}
-
  #ifdef CONFIG_PM_SLEEP
  static int rmi_f01_suspend(struct device *dev)
  {
  	struct rmi_function *fn = to_rmi_function(dev);
  	struct rmi_device *rmi_dev = fn->rmi_dev;
-	struct f01_data *data = fn->data;
+	struct f01_data *f01 = fn->data;
  	int error;

-	data->old_nosleep = data->device_control.ctrl0 &
-					RMI_F01_CRTL0_NOSLEEP_BIT;
-	data->device_control.ctrl0 &= ~RMI_F01_CRTL0_NOSLEEP_BIT;
+	f01->old_nosleep =
+		f01->device_control.ctrl0 & RMI_F01_CRTL0_NOSLEEP_BIT;

-	data->device_control.ctrl0 &= ~RMI_F01_CTRL0_SLEEP_MODE_MASK;
-	data->device_control.ctrl0 |= RMI_SLEEP_MODE_SENSOR_SLEEP;
+	f01->device_control.ctrl0 &= ~RMI_F01_CRTL0_NOSLEEP_BIT;

-	error = rmi_write_block(rmi_dev,
-				fn->fd.control_base_addr,
-				&data->device_control.ctrl0,
-				sizeof(data->device_control.ctrl0));
-	if (error < 0) {
-		dev_err(&fn->dev, "Failed to write sleep mode. Code: %d.\n",
-			error);
-		if (data->old_nosleep)
-			data->device_control.ctrl0 |=
-					RMI_F01_CRTL0_NOSLEEP_BIT;
-		data->device_control.ctrl0 &= ~RMI_F01_CTRL0_SLEEP_MODE_MASK;
-		data->device_control.ctrl0 |= RMI_SLEEP_MODE_NORMAL;
+	f01->device_control.ctrl0 &= ~RMI_F01_CTRL0_SLEEP_MODE_MASK;
+	f01->device_control.ctrl0 |= RMI_SLEEP_MODE_SENSOR_SLEEP;
+
+	error = rmi_write(rmi_dev, fn->fd.control_base_addr,
+			  f01->device_control.ctrl0);
+	if (error) {
+		dev_err(&fn->dev,
+			"Failed to write sleep mode: %d.\n", error);
+		if (f01->old_nosleep)
+			f01->device_control.ctrl0 |= RMI_F01_CRTL0_NOSLEEP_BIT;
+		f01->device_control.ctrl0 &= ~RMI_F01_CTRL0_SLEEP_MODE_MASK;
+		f01->device_control.ctrl0 |= RMI_SLEEP_MODE_NORMAL;
  		return error;
  	}

@@ -498,22 +448,20 @@ static int rmi_f01_resume(struct device *dev)
  {
  	struct rmi_function *fn = to_rmi_function(dev);
  	struct rmi_device *rmi_dev = fn->rmi_dev;
-	struct f01_data *data = fn->data;
+	struct f01_data *f01 = fn->data;
  	int error;

-	if (data->old_nosleep)
-		data->device_control.ctrl0 |= RMI_F01_CRTL0_NOSLEEP_BIT;
+	if (f01->old_nosleep)
+		f01->device_control.ctrl0 |= RMI_F01_CRTL0_NOSLEEP_BIT;

-	data->device_control.ctrl0 &= ~RMI_F01_CTRL0_SLEEP_MODE_MASK;
-	data->device_control.ctrl0 |= RMI_SLEEP_MODE_NORMAL;
+	f01->device_control.ctrl0 &= ~RMI_F01_CTRL0_SLEEP_MODE_MASK;
+	f01->device_control.ctrl0 |= RMI_SLEEP_MODE_NORMAL;

-	error = rmi_write_block(rmi_dev, fn->fd.control_base_addr,
-				&data->device_control.ctrl0,
-				sizeof(data->device_control.ctrl0));
-	if (error < 0) {
+	error = rmi_write(rmi_dev, fn->fd.control_base_addr,
+			  f01->device_control.ctrl0);
+	if (error) {
  		dev_err(&fn->dev,
-			"Failed to restore normal operation. Code: %d.\n",
-			error);
+			"Failed to restore normal operation: %d.\n", error);
  		return error;
  	}

@@ -527,22 +475,21 @@ static int rmi_f01_attention(struct rmi_function *fn,
  			     unsigned long *irq_bits)
  {
  	struct rmi_device *rmi_dev = fn->rmi_dev;
-	struct f01_data *data = fn->data;
-	int retval;
-
-	retval = rmi_read_block(rmi_dev, fn->fd.data_base_addr,
-		&data->device_status, sizeof(data->device_status));
-	if (retval < 0) {
-		dev_err(&fn->dev, "Failed to read device status, code: %d.\n",
-			retval);
-		return retval;
+	int error;
+	u8 device_status;
+
+	error = rmi_read(rmi_dev, fn->fd.data_base_addr, &device_status);
+	if (error) {
+		dev_err(&fn->dev,
+			"Failed to read device status: %d.\n", error);
+		return error;
  	}

-	if (RMI_F01_STATUS_UNCONFIGURED(data->device_status)) {
+	if (RMI_F01_STATUS_UNCONFIGURED(device_status)) {
  		dev_warn(&fn->dev, "Device reset detected.\n");
-		retval = rmi_dev->driver->reset_handler(rmi_dev);
-		if (retval < 0)
-			return retval;
+		error = rmi_dev->driver->reset_handler(rmi_dev);
+		if (error < 0)
+			return error;
  	}
  	return 0;
  }
@@ -559,7 +506,6 @@ static struct rmi_function_handler rmi_f01_handler = {
  	},
  	.func		= 0x01,
  	.probe		= rmi_f01_probe,
-	.remove		= rmi_f01_remove,
  	.config		= rmi_f01_config,
  	.attention	= rmi_f01_attention,
  };

Re: [PATCH] input synaptics-rmi4: rmi_f01.c storage fix

[Christopher Heiny]

On 02/12/2014 01:48 PM, Courtney Cavin wrote:
> On Wed, Feb 12, 2014 at 07:40:49AM +0100, Dmitry Torokhov wrote:
>> Hi Chris,
>>
>> On Tue, Feb 11, 2014 at 03:13:00PM -0800, Christopher Heiny wrote:
>>> Correctly free driver related data when initialization fails.
>>>
>>> Trivial: Clarify a diagnostic message.
>>>
>>> Signed-off-by: Christopher Heiny <cheiny@synaptics.com>
>>> Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
>>> Cc: Benjamin Tissoires <benjamin.tissoires@redhat.com>
>>> Cc: Linux Walleij <linus.walleij@linaro.org>
>>> Cc: David Herrmann <dh.herrmann@gmail.com>
>>> Cc: Jiri Kosina <jkosina@suse.cz>
>>>
>>> ---
>>>
>>>   drivers/input/rmi4/rmi_f01.c | 6 ++++--
>>>   1 file changed, 4 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/input/rmi4/rmi_f01.c b/drivers/input/rmi4/rmi_f01.c
>>> index 381ad60..e4a6df9 100644
>>> --- a/drivers/input/rmi4/rmi_f01.c
>>> +++ b/drivers/input/rmi4/rmi_f01.c
>>> @@ -149,7 +149,7 @@ static int rmi_f01_alloc_memory(struct rmi_function *fn,
>>>
>>>        f01 = devm_kzalloc(&fn->dev, sizeof(struct f01_data), GFP_KERNEL);
>>>        if (!f01) {
>>> -             dev_err(&fn->dev, "Failed to allocate fn_01_data.\n");
>>> +             dev_err(&fn->dev, "Failed to allocate f01_data.\n");
>>>                return -ENOMEM;
>>>        }
>>>
>>> @@ -158,6 +158,7 @@ static int rmi_f01_alloc_memory(struct rmi_function *fn,
>>>                        GFP_KERNEL);
>>>        if (!f01->device_control.interrupt_enable) {
>>>                dev_err(&fn->dev, "Failed to allocate interrupt enable.\n");
>>> +             devm_kfree(&fn->dev, f01);
>>
>> As Courtney mentioned if you are calling devm_kfree() you are most
>> likely doing something wrong.
>>
>> How about the patch below? Please check the XXX comment, I have some
>> concerns about lts vs doze_holdoff check mismatch in probe() and
>> config().
>>
>> Thanks.
>>
>> --
>> Dmitry
>>
>> Input: synaptics-rmi4 - F01 initialization cleanup
>>
>> From: Dmitry Torokhov <dmitry.torokhov@gmail.com>
>>
>> - rename data to f01 where appropriate;
>> - switch to using rmi_read()/rmi_write() for single-byte data;
>> - allocate interrupt mask together with the main structure;
>> - do not kfree() memory allocated with devm;
>> - do not write config data in probe(), we have config() for that;
>> - drop unneeded rmi_f01_remove().
>
> These seem like unrelated changes and make this patch hard to read, I
> would prefer if we could separate these out.  Perhaps like so?
> 	[1] bug-fix
> 		- do not kfree() memory allocated with devm
> 	[2] simplify probe/remove logic
> 		- allocate interrupt mask together with the main structure
> 		- do not write config data in probe(), we have config() for that
> 		- drop unneeded rmi_f01_remove()
> 	[3] non-behavioral changes/cleanup
> 		- switch to using rmi_read()/rmi_write() for single-byte data
> 		- rename data to f01 where appropriate
>
> Disregarding that, and the nitpick below, it looks good to me.

Hi,

This arrived a few seconds after I sent my reply.  Looks like mail is 
slow today.

I am OK to either split or lump the patch - Dmitry can make that call.

>
>>
>> Reported-by: Courtney Cavin <courtney.cavin@sonymobile.com>
>> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
>> ---
>>   drivers/input/rmi4/rmi_f01.c |  397 ++++++++++++++++++------------------------
>>   1 file changed, 172 insertions(+), 225 deletions(-)
>>
>> diff --git a/drivers/input/rmi4/rmi_f01.c b/drivers/input/rmi4/rmi_f01.c
>> index 381ad60..8f7840e 100644
>> --- a/drivers/input/rmi4/rmi_f01.c
>> +++ b/drivers/input/rmi4/rmi_f01.c
> [...]
>> -static int rmi_f01_initialize(struct rmi_function *fn)
>> +static int rmi_f01_probe(struct rmi_function *fn)
>>   {
>> -       u8 temp;
>> -       int error;
>> -       u16 ctrl_base_addr;
>>          struct rmi_device *rmi_dev = fn->rmi_dev;
>>          struct rmi_driver_data *driver_data = dev_get_drvdata(&rmi_dev->dev);
>> -       struct f01_data *data = fn->data;
>> -       struct rmi_device_platform_data *pdata = to_rmi_platform_data(rmi_dev);
>> +       const struct rmi_device_platform_data *pdata =
>> +                               to_rmi_platform_data(rmi_dev);
>> +       struct f01_data *f01;
>> +       size_t f01_size;
>> +       int error;
>> +       u16 ctrl_base_addr;
>> +       u8 device_status;
>> +       u8 temp;
>> +
>> +       f01_size = sizeof(struct f01_data) +
>> +                               sizeof(u8) * driver_data->num_of_irq_regs;
>> +       f01 = devm_kzalloc(&fn->dev, f01_size, GFP_KERNEL);
>> +       if (!f01) {
>> +               dev_err(&fn->dev, "Failed to allocate fn01_data.\n");
>
> Nitpick: Can we drop this printout in the process?  It's much less
> useful than the error and backtrace coming from kmalloc on failure anyway.

We print messages like that in a lot of places.  Based on your prior 
comments, I figured to do a blanket up date that removes all of those at 
once across the driver.  Would that be an OK solution?

>
>> +               return -ENOMEM;
>> +       }
> [...]
>
>> +       /* XXX: why we check has_lts here but has_adjustable_doze in probe? */
>
> Hrm.  This register is poorly documented in the spec.  All of these bits
> are reserved.  Chris, is there a newer version of the spec which
> documents these bits?

Unfortunately, no.  I've filed a bug on that.  In the meantime, I've 
found the following:

* It looks like there's a control register F01_RMI_Ctrl4 which is 
present if the has_lts bit is set, but is not used in any shipped LTS 
products.

* Both F01_RMI_Ctrl2 and F01_RMI_Ctrl3 (doze_interval and 
wakeup_threshold) are controlled by the has_adjustable_doze bit.

The patch I sent a bit ago includes fixes based on this info.

					Thanks,
						Chris

Re: [PATCH] input synaptics-rmi4: rmi_f01.c storage fix

[Dmitry Torokhov]

On Wed, Feb 12, 2014 at 01:48:19PM -0800, Courtney Cavin wrote:
> On Wed, Feb 12, 2014 at 07:40:49AM +0100, Dmitry Torokhov wrote:
> > Hi Chris,
> > 
> > On Tue, Feb 11, 2014 at 03:13:00PM -0800, Christopher Heiny wrote:
> > > Correctly free driver related data when initialization fails.
> > >
> > > Trivial: Clarify a diagnostic message.
> > >
> > > Signed-off-by: Christopher Heiny <cheiny@synaptics.com>
> > > Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> > > Cc: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> > > Cc: Linux Walleij <linus.walleij@linaro.org>
> > > Cc: David Herrmann <dh.herrmann@gmail.com>
> > > Cc: Jiri Kosina <jkosina@suse.cz>
> > >
> > > ---
> > >
> > >  drivers/input/rmi4/rmi_f01.c | 6 ++++--
> > >  1 file changed, 4 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/input/rmi4/rmi_f01.c b/drivers/input/rmi4/rmi_f01.c
> > > index 381ad60..e4a6df9 100644
> > > --- a/drivers/input/rmi4/rmi_f01.c
> > > +++ b/drivers/input/rmi4/rmi_f01.c
> > > @@ -149,7 +149,7 @@ static int rmi_f01_alloc_memory(struct rmi_function *fn,
> > >
> > >       f01 = devm_kzalloc(&fn->dev, sizeof(struct f01_data), GFP_KERNEL);
> > >       if (!f01) {
> > > -             dev_err(&fn->dev, "Failed to allocate fn_01_data.\n");
> > > +             dev_err(&fn->dev, "Failed to allocate f01_data.\n");
> > >               return -ENOMEM;
> > >       }
> > >
> > > @@ -158,6 +158,7 @@ static int rmi_f01_alloc_memory(struct rmi_function *fn,
> > >                       GFP_KERNEL);
> > >       if (!f01->device_control.interrupt_enable) {
> > >               dev_err(&fn->dev, "Failed to allocate interrupt enable.\n");
> > > +             devm_kfree(&fn->dev, f01);
> > 
> > As Courtney mentioned if you are calling devm_kfree() you are most
> > likely doing something wrong.
> > 
> > How about the patch below? Please check the XXX comment, I have some
> > concerns about lts vs doze_holdoff check mismatch in probe() and
> > config().
> > 
> > Thanks.
> > 
> > --
> > Dmitry
> > 
> > Input: synaptics-rmi4 - F01 initialization cleanup
> > 
> > From: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> > 
> > - rename data to f01 where appropriate;
> > - switch to using rmi_read()/rmi_write() for single-byte data;
> > - allocate interrupt mask together with the main structure;
> > - do not kfree() memory allocated with devm;
> > - do not write config data in probe(), we have config() for that;
> > - drop unneeded rmi_f01_remove().
> 
> These seem like unrelated changes and make this patch hard to read, I
> would prefer if we could separate these out.  Perhaps like so?
> 	[1] bug-fix
> 		- do not kfree() memory allocated with devm
> 	[2] simplify probe/remove logic
> 		- allocate interrupt mask together with the main structure
> 		- do not write config data in probe(), we have config() for that
> 		- drop unneeded rmi_f01_remove()
> 	[3] non-behavioral changes/cleanup
> 		- switch to using rmi_read()/rmi_write() for single-byte data
> 		- rename data to f01 where appropriate
> 
> Disregarding that, and the nitpick below, it looks good to me.

OK, I can do that...

> 
> > 
> > Reported-by: Courtney Cavin <courtney.cavin@sonymobile.com>
> > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> > ---
> >  drivers/input/rmi4/rmi_f01.c |  397 ++++++++++++++++++------------------------
> >  1 file changed, 172 insertions(+), 225 deletions(-)
> > 
> > diff --git a/drivers/input/rmi4/rmi_f01.c b/drivers/input/rmi4/rmi_f01.c
> > index 381ad60..8f7840e 100644
> > --- a/drivers/input/rmi4/rmi_f01.c
> > +++ b/drivers/input/rmi4/rmi_f01.c
> [...] 
> > -static int rmi_f01_initialize(struct rmi_function *fn)
> > +static int rmi_f01_probe(struct rmi_function *fn)
> >  {
> > -       u8 temp;
> > -       int error;
> > -       u16 ctrl_base_addr;
> >         struct rmi_device *rmi_dev = fn->rmi_dev;
> >         struct rmi_driver_data *driver_data = dev_get_drvdata(&rmi_dev->dev);
> > -       struct f01_data *data = fn->data;
> > -       struct rmi_device_platform_data *pdata = to_rmi_platform_data(rmi_dev);
> > +       const struct rmi_device_platform_data *pdata =
> > +                               to_rmi_platform_data(rmi_dev);
> > +       struct f01_data *f01;
> > +       size_t f01_size;
> > +       int error;
> > +       u16 ctrl_base_addr;
> > +       u8 device_status;
> > +       u8 temp;
> > +
> > +       f01_size = sizeof(struct f01_data) +
> > +                               sizeof(u8) * driver_data->num_of_irq_regs;
> > +       f01 = devm_kzalloc(&fn->dev, f01_size, GFP_KERNEL);
> > +       if (!f01) {
> > +               dev_err(&fn->dev, "Failed to allocate fn01_data.\n");
> 
> Nitpick: Can we drop this printout in the process?  It's much less
> useful than the error and backtrace coming from kmalloc on failure anyway.

Actually I like messages: who knows when we decided to change kmalloc
behavior and it also helps when there are several allocations in the
same function to know which one failed.

Thanks.

-- 
Dmitry

Re: [PATCH] input synaptics-rmi4: rmi_f01.c storage fix

[Courtney Cavin]

On Thu, Feb 13, 2014 at 12:21:38AM +0100, Christopher Heiny wrote:
> On 02/12/2014 01:48 PM, Courtney Cavin wrote:
> > On Wed, Feb 12, 2014 at 07:40:49AM +0100, Dmitry Torokhov wrote:
> >> Hi Chris,
> >>
> >> On Tue, Feb 11, 2014 at 03:13:00PM -0800, Christopher Heiny wrote:
> > [...]
> >> -static int rmi_f01_initialize(struct rmi_function *fn)
> >> +static int rmi_f01_probe(struct rmi_function *fn)
> >>   {
> >> -       u8 temp;
> >> -       int error;
> >> -       u16 ctrl_base_addr;
> >>          struct rmi_device *rmi_dev = fn->rmi_dev;
> >>          struct rmi_driver_data *driver_data = dev_get_drvdata(&rmi_dev->dev);
> >> -       struct f01_data *data = fn->data;
> >> -       struct rmi_device_platform_data *pdata = to_rmi_platform_data(rmi_dev);
> >> +       const struct rmi_device_platform_data *pdata =
> >> +                               to_rmi_platform_data(rmi_dev);
> >> +       struct f01_data *f01;
> >> +       size_t f01_size;
> >> +       int error;
> >> +       u16 ctrl_base_addr;
> >> +       u8 device_status;
> >> +       u8 temp;
> >> +
> >> +       f01_size = sizeof(struct f01_data) +
> >> +                               sizeof(u8) * driver_data->num_of_irq_regs;
> >> +       f01 = devm_kzalloc(&fn->dev, f01_size, GFP_KERNEL);
> >> +       if (!f01) {
> >> +               dev_err(&fn->dev, "Failed to allocate fn01_data.\n");
> >
> > Nitpick: Can we drop this printout in the process?  It's much less
> > useful than the error and backtrace coming from kmalloc on failure anyway.
> 
> We print messages like that in a lot of places.  Based on your prior 
> comments, I figured to do a blanket up date that removes all of those at 
> once across the driver.  Would that be an OK solution?

It's not really necessary to do a bulk cleanup of this, as it's not a
huge thing.  I just thought that since we were changing the text anyway, we
might as well get rid of the unnecessary stuff.  Dmitry's new email in this
thread settles the issue though, he prefers it, so it stays.

> >> +               return -ENOMEM;
> >> +       }
> > [...]
> >
> >> +       /* XXX: why we check has_lts here but has_adjustable_doze in probe? */
> >
> > Hrm.  This register is poorly documented in the spec.  All of these bits
> > are reserved.  Chris, is there a newer version of the spec which
> > documents these bits?
> 
> Unfortunately, no.  I've filed a bug on that.  In the meantime, I've 
> found the following:
> 
> * It looks like there's a control register F01_RMI_Ctrl4 which is 
> present if the has_lts bit is set, but is not used in any shipped LTS 
> products.
> 
> * Both F01_RMI_Ctrl2 and F01_RMI_Ctrl3 (doze_interval and 
> wakeup_threshold) are controlled by the has_adjustable_doze bit.
> 
> The patch I sent a bit ago includes fixes based on this info.

Ah, OK.  Thanks for the info!

-Courtney

Re: [PATCH] input synaptics-rmi4: rmi_f01.c storage fix

[Courtney Cavin]

On Thu, Feb 13, 2014 at 12:28:00AM +0100, Dmitry Torokhov wrote:
> On Wed, Feb 12, 2014 at 01:48:19PM -0800, Courtney Cavin wrote:
> > On Wed, Feb 12, 2014 at 07:40:49AM +0100, Dmitry Torokhov wrote:
> > > Hi Chris,
> > > 
> > > On Tue, Feb 11, 2014 at 03:13:00PM -0800, Christopher Heiny wrote:
> > > > Correctly free driver related data when initialization fails.
> > > >
> > > > Trivial: Clarify a diagnostic message.
> > > >
> > > > Signed-off-by: Christopher Heiny <cheiny@synaptics.com>
> > > > Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> > > > Cc: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> > > > Cc: Linux Walleij <linus.walleij@linaro.org>
> > > > Cc: David Herrmann <dh.herrmann@gmail.com>
> > > > Cc: Jiri Kosina <jkosina@suse.cz>
> > > >
> > > > ---
> > > >
> > > >  drivers/input/rmi4/rmi_f01.c | 6 ++++--
> > > >  1 file changed, 4 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/drivers/input/rmi4/rmi_f01.c b/drivers/input/rmi4/rmi_f01.c
> > > > index 381ad60..e4a6df9 100644
> > > > --- a/drivers/input/rmi4/rmi_f01.c
> > > > +++ b/drivers/input/rmi4/rmi_f01.c
> > > > @@ -149,7 +149,7 @@ static int rmi_f01_alloc_memory(struct rmi_function *fn,
> > > >
> > > >       f01 = devm_kzalloc(&fn->dev, sizeof(struct f01_data), GFP_KERNEL);
> > > >       if (!f01) {
> > > > -             dev_err(&fn->dev, "Failed to allocate fn_01_data.\n");
> > > > +             dev_err(&fn->dev, "Failed to allocate f01_data.\n");
> > > >               return -ENOMEM;
> > > >       }
> > > >
> > > > @@ -158,6 +158,7 @@ static int rmi_f01_alloc_memory(struct rmi_function *fn,
> > > >                       GFP_KERNEL);
> > > >       if (!f01->device_control.interrupt_enable) {
> > > >               dev_err(&fn->dev, "Failed to allocate interrupt enable.\n");
> > > > +             devm_kfree(&fn->dev, f01);
> > > 
> > > As Courtney mentioned if you are calling devm_kfree() you are most
> > > likely doing something wrong.
> > > 
> > > How about the patch below? Please check the XXX comment, I have some
> > > concerns about lts vs doze_holdoff check mismatch in probe() and
> > > config().
> > > 
> > > Thanks.
> > > 
> > > --
> > > Dmitry
> > > 
> > > Input: synaptics-rmi4 - F01 initialization cleanup
> > > 
> > > From: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> > > 
> > > - rename data to f01 where appropriate;
> > > - switch to using rmi_read()/rmi_write() for single-byte data;
> > > - allocate interrupt mask together with the main structure;
> > > - do not kfree() memory allocated with devm;
> > > - do not write config data in probe(), we have config() for that;
> > > - drop unneeded rmi_f01_remove().
> > 
> > These seem like unrelated changes and make this patch hard to read, I
> > would prefer if we could separate these out.  Perhaps like so?
> > 	[1] bug-fix
> > 		- do not kfree() memory allocated with devm
> > 	[2] simplify probe/remove logic
> > 		- allocate interrupt mask together with the main structure
> > 		- do not write config data in probe(), we have config() for that
> > 		- drop unneeded rmi_f01_remove()
> > 	[3] non-behavioral changes/cleanup
> > 		- switch to using rmi_read()/rmi_write() for single-byte data
> > 		- rename data to f01 where appropriate
> > 
> > Disregarding that, and the nitpick below, it looks good to me.
> 
> OK, I can do that...
> 
> > 
> > > 
> > > Reported-by: Courtney Cavin <courtney.cavin@sonymobile.com>
> > > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> > > ---
> > >  drivers/input/rmi4/rmi_f01.c |  397 ++++++++++++++++++------------------------
> > >  1 file changed, 172 insertions(+), 225 deletions(-)
> > > 
> > > diff --git a/drivers/input/rmi4/rmi_f01.c b/drivers/input/rmi4/rmi_f01.c
> > > index 381ad60..8f7840e 100644
> > > --- a/drivers/input/rmi4/rmi_f01.c
> > > +++ b/drivers/input/rmi4/rmi_f01.c
> > [...] 
> > > -static int rmi_f01_initialize(struct rmi_function *fn)
> > > +static int rmi_f01_probe(struct rmi_function *fn)
> > >  {
> > > -       u8 temp;
> > > -       int error;
> > > -       u16 ctrl_base_addr;
> > >         struct rmi_device *rmi_dev = fn->rmi_dev;
> > >         struct rmi_driver_data *driver_data = dev_get_drvdata(&rmi_dev->dev);
> > > -       struct f01_data *data = fn->data;
> > > -       struct rmi_device_platform_data *pdata = to_rmi_platform_data(rmi_dev);
> > > +       const struct rmi_device_platform_data *pdata =
> > > +                               to_rmi_platform_data(rmi_dev);
> > > +       struct f01_data *f01;
> > > +       size_t f01_size;
> > > +       int error;
> > > +       u16 ctrl_base_addr;
> > > +       u8 device_status;
> > > +       u8 temp;
> > > +
> > > +       f01_size = sizeof(struct f01_data) +
> > > +                               sizeof(u8) * driver_data->num_of_irq_regs;
> > > +       f01 = devm_kzalloc(&fn->dev, f01_size, GFP_KERNEL);
> > > +       if (!f01) {
> > > +               dev_err(&fn->dev, "Failed to allocate fn01_data.\n");
> > 
> > Nitpick: Can we drop this printout in the process?  It's much less
> > useful than the error and backtrace coming from kmalloc on failure anyway.
> 
> Actually I like messages: who knows when we decided to change kmalloc
> behavior and it also helps when there are several allocations in the
> same function to know which one failed.

Of course it's your choice, but I find addr2line does a pretty good job
here with the backtrace.  Maybe we'll get #define GFP_KERNEL __GFP_NOWARN,
but I think someone will go through with some cocci scripts at that point.

-Courtney