Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1488637 > unrolled thread

[PATCH 0/4] GPU-DRM-TILCDC: Fine-tuning for two function implementations

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH 0/4] GPU-DRM-TILCDC: Fine-tuning for two function  implementations SF Markus Elfring <elfring@users.sourceforge.net> - 2016-09-22 10:40 +0200
    [PATCH 2/4] GPU-DRM-TILCDC: Return directly after a failed  kfree_table_init() in tilcdc_convert_slave_node() SF Markus Elfring <elfring@users.sourceforge.net> - 2016-09-22 10:40 +0200
      Re: [PATCH 2/4] GPU-DRM-TILCDC: Return directly after a failed  kfree_table_init() in tilcdc_convert_slave_node() Dan Carpenter <dan.carpenter@oracle.com> - 2016-09-22 13:00 +0200
      Re: [PATCH 2/4] GPU-DRM-TILCDC: Return directly after a failed  kfree_table_init() in tilcdc_convert_slave_node() Jyri Sarha <jsarha@ti.com> - 2016-09-22 19:00 +0200
        Re: GPU-DRM-TILCDC: Return directly after a failed kfree_table_init()  in tilcdc_convert_slave_node() SF Markus Elfring <elfring@users.sourceforge.net> - 2016-09-22 20:20 +0200
    [PATCH 3/4] GPU-DRM-TILCDC: Less function calls in  tilcdc_convert_slave_node() after error detection SF Markus Elfring <elfring@users.sourceforge.net> - 2016-09-22 10:40 +0200
      Re: [PATCH 3/4] GPU-DRM-TILCDC: Less function calls in  tilcdc_convert_slave_node() after error detection Jyri Sarha <jsarha@ti.com> - 2016-09-22 19:10 +0200
        Re: GPU-DRM-TILCDC: Less function calls in  tilcdc_convert_slave_node() after error detection SF Markus Elfring <elfring@users.sourceforge.net> - 2016-09-22 20:40 +0200
          Re: GPU-DRM-TILCDC: Less function calls in  tilcdc_convert_slave_node() after error detection Jyri Sarha <jsarha@ti.com> - 2016-09-22 22:30 +0200
            Re: GPU-DRM-TILCDC: Less function calls in  tilcdc_convert_slave_node() after error detection SF Markus Elfring <elfring@users.sourceforge.net> - 2016-09-23 09:40 +0200
              Re: GPU-DRM-TILCDC: Less function calls in  tilcdc_convert_slave_node() after error detection Jyri Sarha <jsarha@ti.com> - 2016-09-23 12:40 +0200
                Re: GPU-DRM-TILCDC: Less function calls in  tilcdc_convert_slave_node() after error detection SF Markus Elfring <elfring@users.sourceforge.net> - 2016-09-23 13:00 +0200
          Re: GPU-DRM-TILCDC: Less function calls in tilcdc_convert_slave_node()  after error detection Rob Clark <robdclark@gmail.com> - 2016-09-23 13:00 +0200
            Re: GPU-DRM-TILCDC: Less function calls in  tilcdc_convert_slave_node() after error detection SF Markus Elfring <elfring@users.sourceforge.net> - 2016-09-23 13:30 +0200
              Re: GPU-DRM-TILCDC: Less function calls in tilcdc_convert_slave_node()  after error detection Rob Clark <robdclark@gmail.com> - 2016-09-23 13:40 +0200
                Re: GPU-DRM-TILCDC: Less function calls in  tilcdc_convert_slave_node() after error detection SF Markus Elfring <elfring@users.sourceforge.net> - 2016-09-23 14:20 +0200
                  Re: GPU-DRM-TILCDC: Less function calls in tilcdc_convert_slave_node()  after error detection Rob Clark <robdclark@gmail.com> - 2016-09-23 15:10 +0200
    [PATCH 4/4] GPU-DRM-TILCDC: Delete unnecessary variable  initialisations in tilcdc_convert_slave_node() SF Markus Elfring <elfring@users.sourceforge.net> - 2016-09-22 10:40 +0200

#1488637 — [PATCH 0/4] GPU-DRM-TILCDC: Fine-tuning for two function implementations

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Thu, 22 Sep 2016 10:25:43 +0200

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

Markus Elfring (4):
  Use kmalloc_array()
  Return directly after a failed kfree_table_init() in tilcdc_convert_slave_node()
  Less function calls in tilcdc_convert_slave_node() after error detection
  Delete unnecessary variable initialisations

 drivers/gpu/drm/tilcdc/tilcdc_slave_compat.c | 36 +++++++++++++++-------------
 1 file changed, 20 insertions(+), 16 deletions(-)

-- 
2.10.0

[toc] | [next] | [standalone]

#1488643 — [PATCH 2/4] GPU-DRM-TILCDC: Return directly after a failed kfree_table_init() in tilcdc_convert_slave_node()

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Thu, 22 Sep 2016 09:29:23 +0200

Return directly after a memory allocation failed in this function
at the beginning.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/gpu/drm/tilcdc/tilcdc_slave_compat.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/tilcdc/tilcdc_slave_compat.c b/drivers/gpu/drm/tilcdc/tilcdc_slave_compat.c
index 8faa28f..6204405 100644
--- a/drivers/gpu/drm/tilcdc/tilcdc_slave_compat.c
+++ b/drivers/gpu/drm/tilcdc/tilcdc_slave_compat.c
@@ -206,7 +206,7 @@ void __init tilcdc_convert_slave_node(void)
 	int ret;
 
 	if (kfree_table_init(&kft))
-		goto out;
+		return;
 
 	lcdc = of_find_matching_node(NULL, tilcdc_of_match);
 	slave = of_find_matching_node(NULL, tilcdc_slave_of_match);
-- 
2.10.0

[toc] | [prev] | [next] | [standalone]

#1488757 — Re: [PATCH 2/4] GPU-DRM-TILCDC: Return directly after a failed kfree_table_init() in tilcdc_convert_slave_node()

This one is actually a bug fix...  But finding bug fixes in this series
is like looking for kernels of edible corn in piles of monkey poop.

Also, classic "One Err" bug.

regards,
dan carpenter

[toc] | [prev] | [next] | [standalone]

#1489089 — Re: [PATCH 2/4] GPU-DRM-TILCDC: Return directly after a failed kfree_table_init() in tilcdc_convert_slave_node()

On 09/22/16 11:32, SF Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Thu, 22 Sep 2016 09:29:23 +0200
> 
> Return directly after a memory allocation failed in this function
> at the beginning.
> 
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>  drivers/gpu/drm/tilcdc/tilcdc_slave_compat.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/tilcdc/tilcdc_slave_compat.c b/drivers/gpu/drm/tilcdc/tilcdc_slave_compat.c
> index 8faa28f..6204405 100644
> --- a/drivers/gpu/drm/tilcdc/tilcdc_slave_compat.c
> +++ b/drivers/gpu/drm/tilcdc/tilcdc_slave_compat.c
> @@ -206,7 +206,7 @@ void __init tilcdc_convert_slave_node(void)
>  	int ret;
>  
>  	if (kfree_table_init(&kft))
> -		goto out;
> +		return;
>  
>  	lcdc = of_find_matching_node(NULL, tilcdc_of_match);
>  	slave = of_find_matching_node(NULL, tilcdc_slave_of_match);
> 

Thanks,
This is a real bug. I'll pick this up, but with your permission I change
the commit subject to follow the current convention.

Best regards,
Jyri

[toc] | [prev] | [next] | [standalone]

#1489278 — Re: GPU-DRM-TILCDC: Return directly after a failed kfree_table_init() in tilcdc_convert_slave_node()

> I'll pick this up, but with your permission I change
> the commit subject to follow the current convention.

I guess that I can accept the integration of this update suggestion
also with an adjusted prefix in the commit title.   ;-)

Regards,
Markus

[toc] | [prev] | [next] | [standalone]

#1488647 — [PATCH 3/4] GPU-DRM-TILCDC: Less function calls in tilcdc_convert_slave_node() after error detection

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Thu, 22 Sep 2016 10:06:50 +0200

The of_node_put() function was called in some cases
by the tilcdc_convert_slave_node() function during error handling
even if the passed variable contained a null pointer.

* Adjust jump targets according to the Linux coding style convention.

* Split a condition check for resource detection failures so that
  each pointer from these function calls will be checked immediately.

  See also background information:
  Topic "CWE-754: Improper check for unusual or exceptional conditions"
  Link: https://cwe.mitre.org/data/definitions/754.html

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/gpu/drm/tilcdc/tilcdc_slave_compat.c | 28 +++++++++++++++++-----------
 1 file changed, 17 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/tilcdc/tilcdc_slave_compat.c b/drivers/gpu/drm/tilcdc/tilcdc_slave_compat.c
index 6204405..6ee5865 100644
--- a/drivers/gpu/drm/tilcdc/tilcdc_slave_compat.c
+++ b/drivers/gpu/drm/tilcdc/tilcdc_slave_compat.c
@@ -209,25 +209,27 @@ void __init tilcdc_convert_slave_node(void)
 		return;
 
 	lcdc = of_find_matching_node(NULL, tilcdc_of_match);
-	slave = of_find_matching_node(NULL, tilcdc_slave_of_match);
+	if (!of_device_is_available(lcdc))
+		goto free_table;
 
-	if (!slave || !of_device_is_available(lcdc))
-		goto out;
+	slave = of_find_matching_node(NULL, tilcdc_slave_of_match);
+	if (!slave)
+		goto put_node_lcdc;
 
 	i2c = of_parse_phandle(slave, "i2c", 0);
 	if (!i2c) {
 		pr_err("%s: Can't find i2c node trough phandle\n", __func__);
-		goto out;
+		goto put_node_slave;
 	}
 
 	overlay = tilcdc_get_overlay(&kft);
 	if (!overlay)
-		goto out;
+		goto put_node_i2c;
 
 	encoder = of_find_matching_node(overlay, tilcdc_tda998x_of_match);
 	if (!encoder) {
 		pr_err("%s: Failed to find tda998x node\n", __func__);
-		goto out;
+		goto put_node_i2c;
 	}
 
 	tilcdc_copy_props(slave, encoder, tilcdc_slave_props, &kft);
@@ -238,10 +240,10 @@ void __init tilcdc_convert_slave_node(void)
 			continue;
 		if (!strncmp("i2c", (char *)prop->value, prop->length))
 			if (tilcdc_prop_str_update(prop, i2c->full_name, &kft))
-				goto out;
+				goto put_node_fragment;
 		if (!strncmp("lcdc", (char *)prop->value, prop->length))
 			if (tilcdc_prop_str_update(prop, lcdc->full_name, &kft))
-				goto out;
+				goto put_node_fragment;
 	}
 
 	tilcdc_node_disable(slave);
@@ -252,12 +254,16 @@ void __init tilcdc_convert_slave_node(void)
 	else
 		pr_info("%s: ti,tilcdc,slave node successfully converted\n",
 			__func__);
-out:
-	kfree_table_free(&kft);
+ put_node_fragment:
+	of_node_put(fragment);
+ put_node_i2c:
 	of_node_put(i2c);
+ put_node_slave:
 	of_node_put(slave);
+ put_node_lcdc:
 	of_node_put(lcdc);
-	of_node_put(fragment);
+ free_table:
+	kfree_table_free(&kft);
 }
 
 int __init tilcdc_slave_compat_init(void)
-- 
2.10.0

[toc] | [prev] | [next] | [standalone]

#1489095 — Re: [PATCH 3/4] GPU-DRM-TILCDC: Less function calls in tilcdc_convert_slave_node() after error detection

On 09/22/16 11:33, SF Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Thu, 22 Sep 2016 10:06:50 +0200
> 
> The of_node_put() function was called in some cases
> by the tilcdc_convert_slave_node() function during error handling
> even if the passed variable contained a null pointer.
> 
> * Adjust jump targets according to the Linux coding style convention.
> 
> * Split a condition check for resource detection failures so that
>   each pointer from these function calls will be checked immediately.
> 
>   See also background information:
>   Topic "CWE-754: Improper check for unusual or exceptional conditions"
>   Link: https://cwe.mitre.org/data/definitions/754.html
> 

I don't really agree with this patch. There is no harm in calling
of_node_put() with NULL as an argument and because of that there is no
point in making the function more complex and harder to maintain.

Best regards,
Jyri

> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>  drivers/gpu/drm/tilcdc/tilcdc_slave_compat.c | 28 +++++++++++++++++-----------
>  1 file changed, 17 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/gpu/drm/tilcdc/tilcdc_slave_compat.c b/drivers/gpu/drm/tilcdc/tilcdc_slave_compat.c
> index 6204405..6ee5865 100644
> --- a/drivers/gpu/drm/tilcdc/tilcdc_slave_compat.c
> +++ b/drivers/gpu/drm/tilcdc/tilcdc_slave_compat.c
> @@ -209,25 +209,27 @@ void __init tilcdc_convert_slave_node(void)
>  		return;
>  
>  	lcdc = of_find_matching_node(NULL, tilcdc_of_match);
> -	slave = of_find_matching_node(NULL, tilcdc_slave_of_match);
> +	if (!of_device_is_available(lcdc))
> +		goto free_table;
>  
> -	if (!slave || !of_device_is_available(lcdc))
> -		goto out;
> +	slave = of_find_matching_node(NULL, tilcdc_slave_of_match);
> +	if (!slave)
> +		goto put_node_lcdc;
>  
>  	i2c = of_parse_phandle(slave, "i2c", 0);
>  	if (!i2c) {
>  		pr_err("%s: Can't find i2c node trough phandle\n", __func__);
> -		goto out;
> +		goto put_node_slave;
>  	}
>  
>  	overlay = tilcdc_get_overlay(&kft);
>  	if (!overlay)
> -		goto out;
> +		goto put_node_i2c;
>  
>  	encoder = of_find_matching_node(overlay, tilcdc_tda998x_of_match);
>  	if (!encoder) {
>  		pr_err("%s: Failed to find tda998x node\n", __func__);
> -		goto out;
> +		goto put_node_i2c;
>  	}
>  
>  	tilcdc_copy_props(slave, encoder, tilcdc_slave_props, &kft);
> @@ -238,10 +240,10 @@ void __init tilcdc_convert_slave_node(void)
>  			continue;
>  		if (!strncmp("i2c", (char *)prop->value, prop->length))
>  			if (tilcdc_prop_str_update(prop, i2c->full_name, &kft))
> -				goto out;
> +				goto put_node_fragment;
>  		if (!strncmp("lcdc", (char *)prop->value, prop->length))
>  			if (tilcdc_prop_str_update(prop, lcdc->full_name, &kft))
> -				goto out;
> +				goto put_node_fragment;
>  	}
>  
>  	tilcdc_node_disable(slave);
> @@ -252,12 +254,16 @@ void __init tilcdc_convert_slave_node(void)
>  	else
>  		pr_info("%s: ti,tilcdc,slave node successfully converted\n",
>  			__func__);
> -out:
> -	kfree_table_free(&kft);
> + put_node_fragment:
> +	of_node_put(fragment);
> + put_node_i2c:
>  	of_node_put(i2c);
> + put_node_slave:
>  	of_node_put(slave);
> + put_node_lcdc:
>  	of_node_put(lcdc);
> -	of_node_put(fragment);
> + free_table:
> +	kfree_table_free(&kft);
>  }
>  
>  int __init tilcdc_slave_compat_init(void)
> 

[toc] | [prev] | [next] | [standalone]

#1489332 — Re: GPU-DRM-TILCDC: Less function calls in tilcdc_convert_slave_node() after error detection

>> The of_node_put() function was called in some cases
>> by the tilcdc_convert_slave_node() function during error handling
>> even if the passed variable contained a null pointer.
>>
>> * Adjust jump targets according to the Linux coding style convention.
>>
>> * Split a condition check for resource detection failures so that
>>   each pointer from these function calls will be checked immediately.
>>
>>   See also background information:
>>   Topic "CWE-754: Improper check for unusual or exceptional conditions"
>>   Link: https://cwe.mitre.org/data/definitions/754.html
>>
> 
> I don't really agree with this patch.

This kind of feedback can be fine at first glance.


> There is no harm in calling of_node_put() with NULL as an argument

The cost of additional function calls will be eventually not noticed
just because they belong to an exception handling implementation so far.


> and because of that there is no point in making the function more complex

There is inherent software complexity involved.


> and harder to maintain.

How do you think about to discuss this aspect a bit more?


I suggest to reconsider this design detail if it is really acceptable
for the safe implementation of such a software module.

* How much will it matter in general that one function call was performed
  in this use case without checking its return value immediately?

* Should it usually be determined quicker if a required resource
  could be acquired before trying the next allocation?

Regards,
Markus

[toc] | [prev] | [next] | [standalone]

#1489520 — Re: GPU-DRM-TILCDC: Less function calls in tilcdc_convert_slave_node() after error detection

On 09/22/16 21:38, SF Markus Elfring wrote:
>>> The of_node_put() function was called in some cases
>>> by the tilcdc_convert_slave_node() function during error handling
>>> even if the passed variable contained a null pointer.
>>>
>>> * Adjust jump targets according to the Linux coding style convention.
>>>
>>> * Split a condition check for resource detection failures so that
>>>   each pointer from these function calls will be checked immediately.
>>>
>>>   See also background information:
>>>   Topic "CWE-754: Improper check for unusual or exceptional conditions"
>>>   Link: https://cwe.mitre.org/data/definitions/754.html
>>>
>>
>> I don't really agree with this patch.
> 
> This kind of feedback can be fine at first glance.
> 
> 
>> There is no harm in calling of_node_put() with NULL as an argument
> 
> The cost of additional function calls will be eventually not noticed
> just because they belong to an exception handling implementation so far.
> 
> 
>> and because of that there is no point in making the function more complex
> 
> There is inherent software complexity involved.
> 

I think the "if (node)" in the of_node_put() is there on purpose,
because it potentially saves the caller one extra if()-statement and
keeps the caller code simpler.

> 
>> and harder to maintain.
> 
> How do you think about to discuss this aspect a bit more?
> 

Keeping the goto labels in right order needs precision and can lead to
subtle errors. Sometimes there is no way to avoid that, but here there is.

Best regards,
Jyri

[toc] | [prev] | [next] | [standalone]

#1489810 — Re: GPU-DRM-TILCDC: Less function calls in tilcdc_convert_slave_node() after error detection

> I think the "if (node)" in the of_node_put() is there on purpose,

Yes, of course.

Does such an implementation detail correspond to a general software design pattern?


> because it potentially saves the caller one extra if()-statement

This can occasionally happen.


> and keeps the caller code simpler.

A special view on software simplicity can also lead to questionable intermediate
function implementation, can't it?


> Keeping the goto labels in right order needs precision

I can agree to this view.


> and can lead to subtle errors.

The management of jump labels is just another software development challenge
as usual, isn't it?


> Sometimes there is no way to avoid that,

How do you think about to clarify the constraints which you imagine a bit more?


> but here there is.

I disagree to this conclusion.

Would you like to care a bit more for efficiency and software correctness
around the discussed exception handling?

Regards,
Markus

[toc] | [prev] | [next] | [standalone]

#1489937 — Re: GPU-DRM-TILCDC: Less function calls in tilcdc_convert_slave_node() after error detection

On 09/23/16 10:36, SF Markus Elfring wrote:
>> I think the "if (node)" in the of_node_put() is there on purpose,
> 
> Yes, of course.
> 
> Does such an implementation detail correspond to a general software design pattern?
> 

Yes it does. For instance standard malloc()/free() implementation [1].

> 
>> because it potentially saves the caller one extra if()-statement
> 
> This can occasionally happen.
> 
> 
>> and keeps the caller code simpler.
> 
> A special view on software simplicity can also lead to questionable intermediate
> function implementation, can't it?
> 

I don't really follow. But in any case I do not see anything
questionable in the current tilcdc_convert_slave_node() implementation.

> 
>> Keeping the goto labels in right order needs precision
> 
> I can agree to this view.
> 
> 
>> and can lead to subtle errors.
> 
> The management of jump labels is just another software development challenge
> as usual, isn't it?
> 

Yes. But usually it pays of to avoid complexity when possible.

> 
>> Sometimes there is no way to avoid that,
> 
> How do you think about to clarify the constraints which you imagine a bit more?
> 

If the the of_node_put() behaviour would not be specified with null
pointer as parameter, there would be such a constraint.

I am beginning to have a feeling that this discussion is not going anywhere.

> 
>> but here there is.
> 
> I disagree to this conclusion.
> 
> Would you like to care a bit more for efficiency and software correctness
> around the discussed exception handling?
> 

No, I would not. I think we have reached the bottom of this discussion.
For the moment I have more important tasks to do.

Best regards,
Jyri

[1] http://pubs.opengroup.org/onlinepubs/9699919799/functions/free.html

[toc] | [prev] | [next] | [standalone]

#1489943 — Re: GPU-DRM-TILCDC: Less function calls in tilcdc_convert_slave_node() after error detection

>> A special view on software simplicity can also lead to questionable intermediate
>> function implementation, can't it?
> 
> I don't really follow. But in any case I do not see anything
> questionable in the current tilcdc_convert_slave_node() implementation.

I identified update candidates there like the following.

1. Delayed checking for null pointers
…
	if (!slave || !of_device_is_available(lcdc))
…

2. Usage of a single jump label for (too many?) cases
…
		goto out;
…

   Can the corresponding exception handling become also a bit more efficient?


>> Would you like to care a bit more for efficiency and software correctness
>> around the discussed exception handling?
> 
> No, I would not.

Thanks for this information.

I hope that the software situation can also be improved around
this design aspect somehow.


> For the moment I have more important tasks to do.

I know also that various open issues are competing for your software
development attention as usual.

Regards,
Markus

[toc] | [prev] | [next] | [standalone]

#1489945 — Re: GPU-DRM-TILCDC: Less function calls in tilcdc_convert_slave_node() after error detection

On Thu, Sep 22, 2016 at 2:38 PM, SF Markus Elfring
<elfring@users.sourceforge.net> wrote:
>>> The of_node_put() function was called in some cases
>>> by the tilcdc_convert_slave_node() function during error handling
>>> even if the passed variable contained a null pointer.
>>>
>>> * Adjust jump targets according to the Linux coding style convention.
>>>
>>> * Split a condition check for resource detection failures so that
>>>   each pointer from these function calls will be checked immediately.
>>>
>>>   See also background information:
>>>   Topic "CWE-754: Improper check for unusual or exceptional conditions"
>>>   Link: https://cwe.mitre.org/data/definitions/754.html
>>>
>>
>> I don't really agree with this patch.
>
> This kind of feedback can be fine at first glance.
>
>
>> There is no harm in calling of_node_put() with NULL as an argument
>
> The cost of additional function calls will be eventually not noticed
> just because they belong to an exception handling implementation so far.

iirc, there are Coccinelle rules that find code with unnecessary null
checks and removes them..

Although you probably made this complex enough that cocinelle would
not find it.  That is not a complement.  One should not make error
handling/cleanup more complex than needed.

BR,
-R

>
>> and because of that there is no point in making the function more complex
>
> There is inherent software complexity involved.
>
>
>> and harder to maintain.
>
> How do you think about to discuss this aspect a bit more?
>
>
> I suggest to reconsider this design detail if it is really acceptable
> for the safe implementation of such a software module.
>
> * How much will it matter in general that one function call was performed
>   in this use case without checking its return value immediately?
>
> * Should it usually be determined quicker if a required resource
>   could be acquired before trying the next allocation?
>
> Regards,
> Markus
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

[toc] | [prev] | [next] | [standalone]

#1489956 — Re: GPU-DRM-TILCDC: Less function calls in tilcdc_convert_slave_node() after error detection

> iirc, there are Coccinelle rules that find code with unnecessary null
> checks and removes them.

This kind of software change is not needed here.

I find that a corresponding return value check happens one function call
too late.


> Although you probably made this complex enough that cocinelle would
> not find it.  That is not a complement.

I imagine that scripts for the semantic patch language can find more
source code places where questionable disjunctions are used so far.
Would you dare to split any more condition checks?


> One should not make error handling/cleanup more complex than needed.

I see a need to improve not only correctness there but also a bit of
software efficiency.

Regards,
Markus

[toc] | [prev] | [next] | [standalone]

#1489965 — Re: GPU-DRM-TILCDC: Less function calls in tilcdc_convert_slave_node() after error detection

On Fri, Sep 23, 2016 at 7:19 AM, SF Markus Elfring
<elfring@users.sourceforge.net> wrote:
>> iirc, there are Coccinelle rules that find code with unnecessary null
>> checks and removes them.
>
> This kind of software change is not needed here.
>
> I find that a corresponding return value check happens one function call
> too late.
>

I think you misunderstand my point.. which is that additional
conditional checks wrapping a function call to something that already
checks for null should be removed.. not introduced.

>
>> Although you probably made this complex enough that cocinelle would
>> not find it.  That is not a complement.
>
> I imagine that scripts for the semantic patch language can find more
> source code places where questionable disjunctions are used so far.
> Would you dare to split any more condition checks?
>
>
>> One should not make error handling/cleanup more complex than needed.
>
> I see a need to improve not only correctness there but also a bit of
> software efficiency.

If you can measure any performance difference and present some results
(esp. considering that this is something that just happens when the
driver is loaded), then we'll talk.  Until then, please don't send
this sort of patch.  Thank you.

BR,
-R

> Regards,
> Markus

[toc] | [prev] | [next] | [standalone]

#1489994 — Re: GPU-DRM-TILCDC: Less function calls in tilcdc_convert_slave_node() after error detection

>> I see a need to improve not only correctness there but also a bit of
>> software efficiency.
> 
> If you can measure any performance difference and present some results
> (esp. considering that this is something that just happens when the
> driver is loaded), then we'll talk.

Are you really interested to increase your software development attention
for a quicker exception handling implementation in this function?


> Until then, please don't send this sort of patch.  Thank you.

Will benchmark statistics change such a change rejection ever?

Regards,
Markus

[toc] | [prev] | [next] | [standalone]

#1490050 — Re: GPU-DRM-TILCDC: Less function calls in tilcdc_convert_slave_node() after error detection

On Fri, Sep 23, 2016 at 8:17 AM, SF Markus Elfring
<elfring@users.sourceforge.net> wrote:
>>> I see a need to improve not only correctness there but also a bit of
>>> software efficiency.
>>
>> If you can measure any performance difference and present some results
>> (esp. considering that this is something that just happens when the
>> driver is loaded), then we'll talk.
>
> Are you really interested to increase your software development attention
> for a quicker exception handling implementation in this function?
>

No one is interested in making error handling more complex for a
non-existent benefit ;-)

>
>> Until then, please don't send this sort of patch.  Thank you.
>
> Will benchmark statistics change such a change rejection ever?

If you could demonstrate a real benefit to additional complexity for
something that actually matters (ie. not something like this that runs
once at bootup) I would care.

I don't recommend that you waste your time, since there is
approximately nothing in modesetting path that happens with a high
enough frequency to benefit from such a micro-optimization.
Especially not initialization code that runs once at boot up.

Fixing actual bugs is useful and valuable.  Premature "optimization"
at the expense of extra complexity is very much not useful.

BR,
-R

> Regards,
> Markus

[toc] | [prev] | [next] | [standalone]

#1488648 — [PATCH 4/4] GPU-DRM-TILCDC: Delete unnecessary variable initialisations in tilcdc_convert_slave_node()

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Thu, 22 Sep 2016 10:15:36 +0200

Four local variables will be set to an appropriate pointer a bit later.
Thus omit the explicit initialisation which became unnecessary with
a previous update step.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/gpu/drm/tilcdc/tilcdc_slave_compat.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/tilcdc/tilcdc_slave_compat.c b/drivers/gpu/drm/tilcdc/tilcdc_slave_compat.c
index 6ee5865..ae90728 100644
--- a/drivers/gpu/drm/tilcdc/tilcdc_slave_compat.c
+++ b/drivers/gpu/drm/tilcdc/tilcdc_slave_compat.c
@@ -196,8 +196,7 @@ static const char * const tilcdc_slave_props[] __initconst = {
 
 void __init tilcdc_convert_slave_node(void)
 {
-	struct device_node *slave = NULL, *lcdc = NULL;
-	struct device_node *i2c = NULL, *fragment = NULL;
+	struct device_node *slave, *lcdc, *i2c, *fragment;
 	struct device_node *overlay, *encoder;
 	struct property *prop;
 	/* For all memory needed for the overlay tree. This memory can
-- 
2.10.0

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web