[PATCH] iio: qcom-spmi-vadc: One check less in vadc_measure_ref_points() after error detection

[PATCH] iio: qcom-spmi-vadc: One check less in vadc_measure_ref_points() after error detection

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sat, 26 Dec 2015 13:53:15 +0100

This issue was detected by using the Coccinelle software.

Move the jump label directly before the desired log statement
so that the variable "ret" does not need to be checked once more
after it was determined that a function call failed.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/iio/adc/qcom-spmi-vadc.c | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/drivers/iio/adc/qcom-spmi-vadc.c b/drivers/iio/adc/qcom-spmi-vadc.c
index c2babe5..391eefa 100644
--- a/drivers/iio/adc/qcom-spmi-vadc.c
+++ b/drivers/iio/adc/qcom-spmi-vadc.c
@@ -424,7 +424,7 @@ static int vadc_measure_ref_points(struct vadc_priv *vadc)
 	prop = vadc_get_channel(vadc, VADC_REF_1250MV);
 	ret = vadc_do_conversion(vadc, prop, &read_1);
 	if (ret)
-		goto err;
+		goto report_failure;
 
 	/* Try with buffered 625mV channel first */
 	prop = vadc_get_channel(vadc, VADC_SPARE1);
@@ -433,11 +433,11 @@ static int vadc_measure_ref_points(struct vadc_priv *vadc)
 
 	ret = vadc_do_conversion(vadc, prop, &read_2);
 	if (ret)
-		goto err;
+		goto report_failure;
 
 	if (read_1 == read_2) {
 		ret = -EINVAL;
-		goto err;
+		goto report_failure;
 	}
 
 	vadc->graph[VADC_CALIB_ABSOLUTE].dy = read_1 - read_2;
@@ -447,23 +447,24 @@ static int vadc_measure_ref_points(struct vadc_priv *vadc)
 	prop = vadc_get_channel(vadc, VADC_VDD_VADC);
 	ret = vadc_do_conversion(vadc, prop, &read_1);
 	if (ret)
-		goto err;
+		goto report_failure;
 
 	prop = vadc_get_channel(vadc, VADC_GND_REF);
 	ret = vadc_do_conversion(vadc, prop, &read_2);
 	if (ret)
-		goto err;
+		goto report_failure;
 
 	if (read_1 == read_2) {
 		ret = -EINVAL;
-		goto err;
+		goto report_failure;
 	}
 
 	vadc->graph[VADC_CALIB_RATIOMETRIC].dy = read_1 - read_2;
 	vadc->graph[VADC_CALIB_RATIOMETRIC].gnd = read_2;
-err:
-	if (ret)
+	if (ret) {
+report_failure:
 		dev_err(vadc->dev, "measure reference points failed\n");
+	}
 
 	return ret;
 }
-- 
2.6.3

Re: [PATCH] iio: qcom-spmi-vadc: One check less in vadc_measure_ref_points() after error detection

[Jonathan Cameron]

On 26/12/15 13:04, SF Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Sat, 26 Dec 2015 13:53:15 +0100
> 
> This issue was detected by using the Coccinelle software.
> 
> Move the jump label directly before the desired log statement
> so that the variable "ret" does not need to be checked once more
> after it was determined that a function call failed.
> 
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
If we are going to change this, I would prefer to see more useful
local error messages and direct returns rather than jumping to
a very generic message at the end.

I'm also less than keen on jumping into conditionals as I find
it slightly less readable. 

We might technically be 'simplifying' the code, but in this case
the gain is very minor for a fair bit of code churn...

Thanks,

Jonathan
> ---
>  drivers/iio/adc/qcom-spmi-vadc.c | 17 +++++++++--------
>  1 file changed, 9 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/iio/adc/qcom-spmi-vadc.c b/drivers/iio/adc/qcom-spmi-vadc.c
> index c2babe5..391eefa 100644
> --- a/drivers/iio/adc/qcom-spmi-vadc.c
> +++ b/drivers/iio/adc/qcom-spmi-vadc.c
> @@ -424,7 +424,7 @@ static int vadc_measure_ref_points(struct vadc_priv *vadc)
>  	prop = vadc_get_channel(vadc, VADC_REF_1250MV);
>  	ret = vadc_do_conversion(vadc, prop, &read_1);
>  	if (ret)
> -		goto err;
> +		goto report_failure;
In this first case we have already had a report that a conversion failed.
I suppose adding that it was during reference point measurement 'might'
be useful additional information.  I'm not really convinced of it does
however... Hence I'd drop reporting it entirely in this function.
>  
>  	/* Try with buffered 625mV channel first */
>  	prop = vadc_get_channel(vadc, VADC_SPARE1);
> @@ -433,11 +433,11 @@ static int vadc_measure_ref_points(struct vadc_priv *vadc)
>  
>  	ret = vadc_do_conversion(vadc, prop, &read_2);
>  	if (ret)
> -		goto err;
> +		goto report_failure;
>  
>  	if (read_1 == read_2) {
>  		ret = -EINVAL;
I think this one indicates we can't actually read anything at all
for some reason...  It's the only form of error we won't have effectively
already reported so is worthy of some sort of debug message...
> -		goto err;
> +		goto report_failure;
>  	}
>  
>  	vadc->graph[VADC_CALIB_ABSOLUTE].dy = read_1 - read_2;
> @@ -447,23 +447,24 @@ static int vadc_measure_ref_points(struct vadc_priv *vadc)
>  	prop = vadc_get_channel(vadc, VADC_VDD_VADC);
>  	ret = vadc_do_conversion(vadc, prop, &read_1);
>  	if (ret)
> -		goto err;
> +		goto report_failure;
>  
>  	prop = vadc_get_channel(vadc, VADC_GND_REF);
>  	ret = vadc_do_conversion(vadc, prop, &read_2);
>  	if (ret)
> -		goto err;
> +		goto report_failure;
>  
>  	if (read_1 == read_2) {
>  		ret = -EINVAL;
> -		goto err;
> +		goto report_failure;
>  	}
>  
>  	vadc->graph[VADC_CALIB_RATIOMETRIC].dy = read_1 - read_2;
>  	vadc->graph[VADC_CALIB_RATIOMETRIC].gnd = read_2;
> -err:
> -	if (ret)
> +	if (ret) {
> +report_failure:
>  		dev_err(vadc->dev, "measure reference points failed\n");
> +	}
>  
>  	return ret;
>  }
> 

[PATCH 0/7] iio: Fine-tuning for several function implementations

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sat, 24 Sep 2016 08:10:08 +0200

Some update suggestions were taken into account
from static source code analysis.

Markus Elfring (7):
  Use kmalloc_array() in iio_scan_mask_set()
  Rename a jump label in iio_buffer_store_watermark()
  Rename a jump label in iio_buffer_store_enable()
  Rename a jump label in iio_buffer_write_length()
  Rename a jump label in iio_scan_el_ts_store()
  Rename a jump label in iio_scan_el_store()
  Adjust checks for null pointers in six functions

 drivers/iio/industrialio-buffer.c | 55 ++++++++++++++++++---------------------
 1 file changed, 26 insertions(+), 29 deletions(-)

-- 
2.10.0

[PATCH 1/7] iio: Use kmalloc_array() in iio_scan_mask_set()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Fri, 23 Sep 2016 22:30:32 +0200

A multiplication for the size determination of a memory allocation
indicated that an array data structure should be processed.
Thus use the corresponding function "kmalloc_array".

This issue was detected by using the Coccinelle software.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/iio/industrialio-buffer.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
index 49bf9c5..7a4d9499 100644
--- a/drivers/iio/industrialio-buffer.c
+++ b/drivers/iio/industrialio-buffer.c
@@ -307,10 +307,9 @@ static int iio_scan_mask_set(struct iio_dev *indio_dev,
 	const unsigned long *mask;
 	unsigned long *trialmask;
 
-	trialmask = kmalloc(sizeof(*trialmask)*
-			    BITS_TO_LONGS(indio_dev->masklength),
-			    GFP_KERNEL);
-
+	trialmask = kmalloc_array(BITS_TO_LONGS(indio_dev->masklength),
+				  sizeof(*trialmask),
+				  GFP_KERNEL);
 	if (trialmask == NULL)
 		return -ENOMEM;
 	if (!indio_dev->masklength) {
-- 
2.10.0

Re: [PATCH 1/7] iio: Use kmalloc_array() in iio_scan_mask_set()

[Jonathan Cameron]

On 24/09/16 07:24, SF Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Fri, 23 Sep 2016 22:30:32 +0200
> 
> A multiplication for the size determination of a memory allocation
> indicated that an array data structure should be processed.
> Thus use the corresponding function "kmalloc_array".
> 
> This issue was detected by using the Coccinelle software.
> 
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
Applied to the togreg branch of iio.git - initially pushed out
as testing for the autobuilders to play with it.

Thanks,

Jonathan
> ---
>  drivers/iio/industrialio-buffer.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
> index 49bf9c5..7a4d9499 100644
> --- a/drivers/iio/industrialio-buffer.c
> +++ b/drivers/iio/industrialio-buffer.c
> @@ -307,10 +307,9 @@ static int iio_scan_mask_set(struct iio_dev *indio_dev,
>  	const unsigned long *mask;
>  	unsigned long *trialmask;
>  
> -	trialmask = kmalloc(sizeof(*trialmask)*
> -			    BITS_TO_LONGS(indio_dev->masklength),
> -			    GFP_KERNEL);
> -
> +	trialmask = kmalloc_array(BITS_TO_LONGS(indio_dev->masklength),
> +				  sizeof(*trialmask),
> +				  GFP_KERNEL);
>  	if (trialmask == NULL)
>  		return -ENOMEM;
>  	if (!indio_dev->masklength) {
> 

Re: [PATCH 1/7] iio: Use kmalloc_array() in iio_scan_mask_set()

[SF Markus Elfring]

> Applied to the togreg branch of iio.git - initially pushed out
> as testing for the autobuilders to play with it.

Thanks for your positive response.


Do you see any need to improve the software situation around
the macro "BITS_TO_LONGS" further?

Do you fiddle with a programming interface like "bitmap_alloc" occasionally?

Regards,
Markus

Re: [PATCH 1/7] iio: Use kmalloc_array() in iio_scan_mask_set()

[Jonathan Cameron]

On 24/09/16 17:18, SF Markus Elfring wrote:
>> Applied to the togreg branch of iio.git - initially pushed out
>> as testing for the autobuilders to play with it.
> 
> Thanks for your positive response.
> 
> 
> Do you see any need to improve the software situation around
> the macro "BITS_TO_LONGS" further?
> 
> Do you fiddle with a programming interface like "bitmap_alloc" occasionally?
It might be clearer than having an array allocation as we do after this
patch. Obviously the representation is an array, but it is a little ugly
as it stands!
> 
> Regards,
> Markus
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

[PATCH 2/7] iio: Rename a jump label in iio_buffer_store_watermark()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sat, 24 Sep 2016 06:54:49 +0200

Adjust jump labels according to the current Linux coding style convention.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/iio/industrialio-buffer.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
index 7a4d9499..a865af8 100644
--- a/drivers/iio/industrialio-buffer.c
+++ b/drivers/iio/industrialio-buffer.c
@@ -1028,16 +1028,16 @@ static ssize_t iio_buffer_store_watermark(struct device *dev,
 
 	if (val > buffer->length) {
 		ret = -EINVAL;
-		goto out;
+		goto unlock;
 	}
 
 	if (iio_buffer_is_active(indio_dev->buffer)) {
 		ret = -EBUSY;
-		goto out;
+		goto unlock;
 	}
 
 	buffer->watermark = val;
-out:
+unlock:
 	mutex_unlock(&indio_dev->mlock);
 
 	return ret ? ret : len;
-- 
2.10.0

Re: [PATCH 2/7] iio: Rename a jump label in iio_buffer_store_watermark()

[Jonathan Cameron]

On 24/09/16 07:25, SF Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Sat, 24 Sep 2016 06:54:49 +0200
> 
> Adjust jump labels according to the current Linux coding style convention.
> 
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
I'm not necessarily against this change which does perhaps clarify the code
ever so slightly, but I am interested to know where 
'current Linux coding style convention' comes from?

Jonathan
> ---
>  drivers/iio/industrialio-buffer.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
> index 7a4d9499..a865af8 100644
> --- a/drivers/iio/industrialio-buffer.c
> +++ b/drivers/iio/industrialio-buffer.c
> @@ -1028,16 +1028,16 @@ static ssize_t iio_buffer_store_watermark(struct device *dev,
>  
>  	if (val > buffer->length) {
>  		ret = -EINVAL;
> -		goto out;
> +		goto unlock;
>  	}
>  
>  	if (iio_buffer_is_active(indio_dev->buffer)) {
>  		ret = -EBUSY;
> -		goto out;
> +		goto unlock;
>  	}
>  
>  	buffer->watermark = val;
> -out:
> +unlock:
>  	mutex_unlock(&indio_dev->mlock);
>  
>  	return ret ? ret : len;
> 

Re: iio: Rename a jump label in iio_buffer_store_watermark()

[SF Markus Elfring]

> I'm not necessarily against this change which does perhaps clarify the code
> ever so slightly,

Thanks for another bit of positive feedback.


> but I am interested to know where 'current Linux coding style convention' comes from?

How often do you check the status of a document like "CodingStyle" for example?   ;-)

How do you think about information from a commit like
"docs: Remove space-before-label guidance from CodingStyle" (on 2016-09-21)?
https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/commit/Documentation/CodingStyle?id=79c70c304b0b443429b2a0019518532c5162817a

Regards,
Markus

Re: iio: Rename a jump label in iio_buffer_store_watermark()

[Jonathan Cameron]

On 24/09/16 20:21, SF Markus Elfring wrote:
>> I'm not necessarily against this change which does perhaps clarify the code
>> ever so slightly,
> 
> Thanks for another bit of positive feedback.
> 
> 
>> but I am interested to know where 'current Linux coding style convention' comes from?
> 
> How often do you check the status of a document like "CodingStyle" for example?   ;-)
Every time I see a patch doing a style change I don't remember being there last time
I looked ;)

I'm not seeing this one in there.  Which tool is spitting it out? Or is
the test yours?
> 
> How do you think about information from a commit like
> "docs: Remove space-before-label guidance from CodingStyle" (on 2016-09-21)?
> https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/commit/Documentation/CodingStyle?id=79c70c304b0b443429b2a0019518532c5162817a
Yeah, I saw the discussion / flame war that resulted in that revert...
> 
> Regards,
> Markus
> 

Re: iio: Rename a jump label in iio_buffer_store_watermark()

[SF Markus Elfring]

> I'm not seeing this one in there.

How much do you care about selection of appropriate identifiers in source files?


> Which tool is spitting it out?

Are you looking for any special tool?


> Or is the test yours?

Which test do you mean?


> Yeah, I saw the discussion / flame war that resulted in that revert...

Would you like to share any more constructive ideas for this software evolution?

Regards,
Markus

Re: iio: Rename a jump label in iio_buffer_store_watermark()

[Jonathan Cameron]

On 25/09/16 14:00, SF Markus Elfring wrote:
>> I'm not seeing this one in there.
> 
> How much do you care about selection of appropriate identifiers in source files?
It's not an inappropriate identifier as it stands. The point is
that it could be better.
> 
> 
>> Which tool is spitting it out?
> 
> Are you looking for any special tool?
I was wondering how you identified these particular
issues as I wanted to know the logic behind the test.
> 
> 
>> Or is the test yours?
> 
> Which test do you mean?
Whatever you used to find these jump labels (the patch series
description suggested it was a static checker).
> 
> 
>> Yeah, I saw the discussion / flame war that resulted in that revert...
> 
> Would you like to share any more constructive ideas for this software evolution?
Not a lot of time at the moment, sorry.

Jonathan
> 
> Regards,
> Markus
> 

Re: iio: Rename a jump label in iio_buffer_store_watermark()

[SF Markus Elfring]

> It's not an inappropriate identifier as it stands. The point is
> that it could be better.

Thanks for your interest in clarifying further improvement possibilities.


>>> Which tool is spitting it out?
>>
>> Are you looking for any special tool?
> I was wondering how you identified these particular
> issues as I wanted to know the logic behind the test.

Do you get any related ideas from information in a message like
"Source code review around jump label usage" (from 2015-12-11)?

https://lkml.org/lkml/2015/12/11/378
https://lkml.kernel.org/r/<566ABCD9.1060404@users.sourceforge.net>


>> Which test do you mean?
> Whatever you used to find these jump labels

There is a general possibility that dedicated scripts for the semantic
patch language can also adjust jump labels as I suggested it for five
functions in this software.


> (the patch series description suggested it was a static checker).

The corresponding five patches are just a result of a source code review
by the means of a current text editor wit extra support for programming.

How do you think about to improve the capabilities of tools for advanced
static source code analysis any further?

Regards,
Markus

Re: iio: Rename a jump label in iio_buffer_store_watermark()

[Jonathan Cameron]

On 25/09/16 16:17, SF Markus Elfring wrote:
>> It's not an inappropriate identifier as it stands. The point is
>> that it could be better.
> 
> Thanks for your interest in clarifying further improvement possibilities.
> 
> 
>>>> Which tool is spitting it out?
>>>
>>> Are you looking for any special tool?
>> I was wondering how you identified these particular
>> issues as I wanted to know the logic behind the test.
> 
> Do you get any related ideas from information in a message like
> "Source code review around jump label usage" (from 2015-12-11)?
> 
> https://lkml.org/lkml/2015/12/11/378
> https://lkml.kernel.org/r/<566ABCD9.1060404@users.sourceforge.net>
> 
> 
>>> Which test do you mean?
>> Whatever you used to find these jump labels
> 
> There is a general possibility that dedicated scripts for the semantic
> patch language can also adjust jump labels as I suggested it for five
> functions in this software.
> 
> 
>> (the patch series description suggested it was a static checker).
> 
> The corresponding five patches are just a result of a source code review
> by the means of a current text editor wit extra support for programming.
To my mind there is a divide between the cost of making changes like this
as a result of initial review and that of doing it on existing code.

I don't think this one is worth while for existing code.
> 
> How do you think about to improve the capabilities of tools for advanced
> static source code analysis any further?
> 
> Regards,
> Markus
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: iio: Rename a jump label in iio_buffer_store_watermark()

[SF Markus Elfring]

> To my mind there is a divide between the cost of making changes like this
> as a result of initial review and that of doing it on existing code.

I can follow this view.


> I don't think this one is worth while for existing code.

How do you think about once more to increase the usage of the jump label "unlock"
also for this software module?

Regards,
Markus

[PATCH 3/7] iio: Rename a jump label in iio_buffer_store_enable()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sat, 24 Sep 2016 07:11:32 +0200

Adjust a jump label according to the current Linux coding style convention.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/iio/industrialio-buffer.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
index a865af8..6509f0f 100644
--- a/drivers/iio/industrialio-buffer.c
+++ b/drivers/iio/industrialio-buffer.c
@@ -982,7 +982,7 @@ static ssize_t iio_buffer_store_enable(struct device *dev,
 	inlist = iio_buffer_is_active(indio_dev->buffer);
 	/* Already in desired state */
 	if (inlist == requested_state)
-		goto done;
+		goto unlock;
 
 	if (requested_state)
 		ret = __iio_update_buffers(indio_dev,
@@ -990,8 +990,7 @@ static ssize_t iio_buffer_store_enable(struct device *dev,
 	else
 		ret = __iio_update_buffers(indio_dev,
 					 NULL, indio_dev->buffer);
-
-done:
+unlock:
 	mutex_unlock(&indio_dev->mlock);
 	return (ret < 0) ? ret : len;
 }
-- 
2.10.0

[PATCH 4/7] iio: Rename a jump label in iio_buffer_write_length()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sat, 24 Sep 2016 07:17:44 +0200

Adjust a jump label according to the current Linux coding style convention.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/iio/industrialio-buffer.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
index 6509f0f..376101f 100644
--- a/drivers/iio/industrialio-buffer.c
+++ b/drivers/iio/industrialio-buffer.c
@@ -505,10 +505,10 @@ static ssize_t iio_buffer_write_length(struct device *dev,
 		ret = 0;
 	}
 	if (ret)
-		goto out;
+		goto unlock;
 	if (buffer->length && buffer->length < buffer->watermark)
 		buffer->watermark = buffer->length;
-out:
+unlock:
 	mutex_unlock(&indio_dev->mlock);
 
 	return ret ? ret : len;
-- 
2.10.0

[PATCH 5/7] iio: Rename a jump label in iio_scan_el_ts_store()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sat, 24 Sep 2016 07:27:26 +0200

Adjust a jump label according to the current Linux coding style convention.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/iio/industrialio-buffer.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
index 376101f..4b8f313 100644
--- a/drivers/iio/industrialio-buffer.c
+++ b/drivers/iio/industrialio-buffer.c
@@ -409,10 +409,10 @@ static ssize_t iio_scan_el_ts_store(struct device *dev,
 	mutex_lock(&indio_dev->mlock);
 	if (iio_buffer_is_active(indio_dev->buffer)) {
 		ret = -EBUSY;
-		goto error_ret;
+		goto unlock;
 	}
 	indio_dev->buffer->scan_timestamp = state;
-error_ret:
+unlock:
 	mutex_unlock(&indio_dev->mlock);
 
 	return ret ? ret : len;
-- 
2.10.0

[PATCH 6/7] iio: Rename a jump label in iio_scan_el_store()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sat, 24 Sep 2016 07:40:59 +0200

Adjust jump labels according to the current Linux coding style convention.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/iio/industrialio-buffer.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
index 4b8f313..57e201a 100644
--- a/drivers/iio/industrialio-buffer.c
+++ b/drivers/iio/industrialio-buffer.c
@@ -363,22 +363,21 @@ static ssize_t iio_scan_el_store(struct device *dev,
 	mutex_lock(&indio_dev->mlock);
 	if (iio_buffer_is_active(indio_dev->buffer)) {
 		ret = -EBUSY;
-		goto error_ret;
+		goto unlock;
 	}
 	ret = iio_scan_mask_query(indio_dev, buffer, this_attr->address);
 	if (ret < 0)
-		goto error_ret;
+		goto unlock;
 	if (!state && ret) {
 		ret = iio_scan_mask_clear(buffer, this_attr->address);
 		if (ret)
-			goto error_ret;
+			goto unlock;
 	} else if (state && !ret) {
 		ret = iio_scan_mask_set(indio_dev, buffer, this_attr->address);
 		if (ret)
-			goto error_ret;
+			goto unlock;
 	}
-
-error_ret:
+unlock:
 	mutex_unlock(&indio_dev->mlock);
 
 	return ret < 0 ? ret : len;
-- 
2.10.0

[PATCH 7/7] iio: Adjust checks for null pointers in six functions

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sat, 24 Sep 2016 08:00:07 +0200
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

The script "checkpatch.pl" can point information out like the following.

Comparison to NULL could be written !…

Thus fix the affected source code places.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/iio/industrialio-buffer.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
index 57e201a..6893639 100644
--- a/drivers/iio/industrialio-buffer.c
+++ b/drivers/iio/industrialio-buffer.c
@@ -310,7 +310,7 @@ static int iio_scan_mask_set(struct iio_dev *indio_dev,
 	trialmask = kmalloc_array(BITS_TO_LONGS(indio_dev->masklength),
 				  sizeof(*trialmask),
 				  GFP_KERNEL);
-	if (trialmask == NULL)
+	if (!trialmask)
 		return -ENOMEM;
 	if (!indio_dev->masklength) {
 		WARN(1, "Trying to set scanmask prior to registering buffer\n");
@@ -711,7 +711,7 @@ static int iio_verify_update(struct iio_dev *indio_dev,
 	/* What scan mask do we actually have? */
 	compound_mask = kcalloc(BITS_TO_LONGS(indio_dev->masklength),
 				sizeof(long), GFP_KERNEL);
-	if (compound_mask == NULL)
+	if (!compound_mask)
 		return -ENOMEM;
 
 	scan_timestamp = false;
@@ -736,7 +736,7 @@ static int iio_verify_update(struct iio_dev *indio_dev,
 				    compound_mask,
 				    strict_scanmask);
 		kfree(compound_mask);
-		if (scan_mask == NULL)
+		if (!scan_mask)
 			return -EINVAL;
 	} else {
 	    scan_mask = compound_mask;
@@ -940,7 +940,7 @@ int iio_update_buffers(struct iio_dev *indio_dev,
 		goto out_unlock;
 	}
 
-	if (indio_dev->info == NULL) {
+	if (!indio_dev->info) {
 		ret = -ENODEV;
 		goto out_unlock;
 	}
@@ -1130,11 +1130,11 @@ int iio_buffer_alloc_sysfs_and_mask(struct iio_dev *indio_dev)
 				indio_dev->scan_index_timestamp =
 					channels[i].scan_index;
 		}
-		if (indio_dev->masklength && buffer->scan_mask == NULL) {
+		if (indio_dev->masklength && !buffer->scan_mask) {
 			buffer->scan_mask = kcalloc(BITS_TO_LONGS(indio_dev->masklength),
 						    sizeof(*buffer->scan_mask),
 						    GFP_KERNEL);
-			if (buffer->scan_mask == NULL) {
+			if (!buffer->scan_mask) {
 				ret = -ENOMEM;
 				goto error_cleanup_dynamic;
 			}
@@ -1146,7 +1146,7 @@ int iio_buffer_alloc_sysfs_and_mask(struct iio_dev *indio_dev)
 	buffer->scan_el_group.attrs = kcalloc(attrcount + 1,
 					      sizeof(buffer->scan_el_group.attrs[0]),
 					      GFP_KERNEL);
-	if (buffer->scan_el_group.attrs == NULL) {
+	if (!buffer->scan_el_group.attrs) {
 		ret = -ENOMEM;
 		goto error_free_scan_mask;
 	}
@@ -1291,7 +1291,7 @@ static int iio_buffer_add_demux(struct iio_buffer *buffer,
 		(*p)->length += length;
 	} else {
 		*p = kmalloc(sizeof(**p), GFP_KERNEL);
-		if (*p == NULL)
+		if (!*p)
 			return -ENOMEM;
 		(*p)->from = in_loc;
 		(*p)->to = out_loc;
@@ -1356,7 +1356,7 @@ static int iio_buffer_update_demux(struct iio_dev *indio_dev,
 		in_loc += length;
 	}
 	buffer->demux_bounce = kzalloc(out_loc, GFP_KERNEL);
-	if (buffer->demux_bounce == NULL) {
+	if (!buffer->demux_bounce) {
 		ret = -ENOMEM;
 		goto error_clear_mux_table;
 	}
-- 
2.10.0

Re: [PATCH 7/7] iio: Adjust checks for null pointers in six functions

[Jonathan Cameron]

On 24/09/16 07:31, SF Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Sat, 24 Sep 2016 08:00:07 +0200
> MIME-Version: 1.0
> Content-Type: text/plain; charset=UTF-8
> Content-Transfer-Encoding: 8bit
> 
> The script "checkpatch.pl" can point information out like the following.
> 
> Comparison to NULL could be written !…
> 
> Thus fix the affected source code places.
> 
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
This is a more worthwhile change in my mind to the jump label changes.
Would you mind resending with it not based on top of those?

Thanks,

Jonathan
> ---
>  drivers/iio/industrialio-buffer.c | 18 +++++++++---------
>  1 file changed, 9 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
> index 57e201a..6893639 100644
> --- a/drivers/iio/industrialio-buffer.c
> +++ b/drivers/iio/industrialio-buffer.c
> @@ -310,7 +310,7 @@ static int iio_scan_mask_set(struct iio_dev *indio_dev,
>  	trialmask = kmalloc_array(BITS_TO_LONGS(indio_dev->masklength),
>  				  sizeof(*trialmask),
>  				  GFP_KERNEL);
> -	if (trialmask == NULL)
> +	if (!trialmask)
>  		return -ENOMEM;
>  	if (!indio_dev->masklength) {
>  		WARN(1, "Trying to set scanmask prior to registering buffer\n");
> @@ -711,7 +711,7 @@ static int iio_verify_update(struct iio_dev *indio_dev,
>  	/* What scan mask do we actually have? */
>  	compound_mask = kcalloc(BITS_TO_LONGS(indio_dev->masklength),
>  				sizeof(long), GFP_KERNEL);
> -	if (compound_mask == NULL)
> +	if (!compound_mask)
>  		return -ENOMEM;
>  
>  	scan_timestamp = false;
> @@ -736,7 +736,7 @@ static int iio_verify_update(struct iio_dev *indio_dev,
>  				    compound_mask,
>  				    strict_scanmask);
>  		kfree(compound_mask);
> -		if (scan_mask == NULL)
> +		if (!scan_mask)
>  			return -EINVAL;
>  	} else {
>  	    scan_mask = compound_mask;
> @@ -940,7 +940,7 @@ int iio_update_buffers(struct iio_dev *indio_dev,
>  		goto out_unlock;
>  	}
>  
> -	if (indio_dev->info == NULL) {
> +	if (!indio_dev->info) {
>  		ret = -ENODEV;
>  		goto out_unlock;
>  	}
> @@ -1130,11 +1130,11 @@ int iio_buffer_alloc_sysfs_and_mask(struct iio_dev *indio_dev)
>  				indio_dev->scan_index_timestamp =
>  					channels[i].scan_index;
>  		}
> -		if (indio_dev->masklength && buffer->scan_mask == NULL) {
> +		if (indio_dev->masklength && !buffer->scan_mask) {
>  			buffer->scan_mask = kcalloc(BITS_TO_LONGS(indio_dev->masklength),
>  						    sizeof(*buffer->scan_mask),
>  						    GFP_KERNEL);
> -			if (buffer->scan_mask == NULL) {
> +			if (!buffer->scan_mask) {
>  				ret = -ENOMEM;
>  				goto error_cleanup_dynamic;
>  			}
> @@ -1146,7 +1146,7 @@ int iio_buffer_alloc_sysfs_and_mask(struct iio_dev *indio_dev)
>  	buffer->scan_el_group.attrs = kcalloc(attrcount + 1,
>  					      sizeof(buffer->scan_el_group.attrs[0]),
>  					      GFP_KERNEL);
> -	if (buffer->scan_el_group.attrs == NULL) {
> +	if (!buffer->scan_el_group.attrs) {
>  		ret = -ENOMEM;
>  		goto error_free_scan_mask;
>  	}
> @@ -1291,7 +1291,7 @@ static int iio_buffer_add_demux(struct iio_buffer *buffer,
>  		(*p)->length += length;
>  	} else {
>  		*p = kmalloc(sizeof(**p), GFP_KERNEL);
> -		if (*p == NULL)
> +		if (!*p)
>  			return -ENOMEM;
>  		(*p)->from = in_loc;
>  		(*p)->to = out_loc;
> @@ -1356,7 +1356,7 @@ static int iio_buffer_update_demux(struct iio_dev *indio_dev,
>  		in_loc += length;
>  	}
>  	buffer->demux_bounce = kzalloc(out_loc, GFP_KERNEL);
> -	if (buffer->demux_bounce == NULL) {
> +	if (!buffer->demux_bounce) {
>  		ret = -ENOMEM;
>  		goto error_clear_mux_table;
>  	}
> 

Re: iio: Adjust checks for null pointers in six functions

[SF Markus Elfring]

> This is a more worthwhile change in my mind to the jump label changes.

Thanks for another bit of positive feedback for this refactoring.


> Would you mind resending with it not based on top of those?

Is a resend really needed?

Does the tool "Git" let you also apply this update step before
the other suggestions?

Would you dare a reordering of items according to your change preferences?

Regards,
Markus

Re: iio: Adjust checks for null pointers in six functions

[Jonathan Cameron]

On 25/09/16 15:44, SF Markus Elfring wrote:
>> This is a more worthwhile change in my mind to the jump label changes.
> 
> Thanks for another bit of positive feedback for this refactoring.
> 
> 
>> Would you mind resending with it not based on top of those?
> 
> Is a resend really needed?
It's a question of time.  Maintainers regularly push back stuff
they could fix up themselves purely to save their time.

Also, I'm afraid I don't care enough to put the time in for this
one.  I care enough to review / apply the patch though.
> 
> Does the tool "Git" let you also apply this update step before
> the other suggestions?
It rarely copes with this sort of reordering as it doesn't have
the semantic knowledge of what matters in the other patches.
> 
> Would you dare a reordering of items according to your change preferences?
> 
> Regards,
> Markus
> 

Re: iio: Adjust checks for null pointers in six functions

[SF Markus Elfring]

> Also, I'm afraid I don't care enough to put the time in for this one.

It's a pity.


>> Does the tool "Git" let you also apply this update step before
>> the other suggestions?
> It rarely copes with this sort of reordering as it doesn't have
> the semantic knowledge of what matters in the other patches.

I assume that the involved software developers got a specific impression
about the discussed patch series for this software module so that a few
relevant dependencies became clearer eventually.

I proposed some update steps where I chose a specific patch granularity
once more. So I imagine that a bit of variation in the patch order
could be supported already.

Will it be possible to apply also this one a bit later just because
the patch hunk contexts might be still valid in the (near) future?

Regards,
Markus

Re: iio: Adjust checks for null pointers in six functions

[Al Viro]

On Sun, Sep 25, 2016 at 07:45:40PM +0200, SF Markus Elfring wrote:

> I assume that the involved software developers got a specific impression
> about the discussed patch series for this software module so that a few
> relevant dependencies became clearer eventually.
> 
> I proposed some update steps where I chose a specific patch granularity
> once more. So I imagine that a bit of variation in the patch order
> could be supported already.
> 
> Will it be possible to apply also this one a bit later just because
> the patch hunk contexts might be still valid in the (near) future?

I wonder if you realize that your postings _reek_ with the Strong Programme
intellectual offal...  Are you, by any chance, some kind of sociology grad?
You can't study development purely on the process level, ignoring the actual
usefulness of contributions and the fact that there are objective criteria
of their worth.

Re: iio: Adjust checks for null pointers in six functions

[SF Markus Elfring]

> Are you, by any chance, some kind of sociology grad?

Not yet. ;-)   - It looks also unlikely at the moment that my life will evolve
in such direction.


> You can't study development purely on the process level,

I find that I am not studying this topic. Am I practically contributing
small improvements to free software (including Linux) for years?


> ignoring the actual usefulness of contributions

I guess their usefulness is varying over time.


> and the fact that there are objective criteria of their worth.

Are there any source code quality goals left over which would be nice
to achieve also for this software module?

Regards,
Markus