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

Groups > linux.kernel > #1298165 > unrolled thread

[PATCH] i2c-core: One function call less in acpi_i2c_space_handler() after error detection

Started bySF Markus Elfring <elfring@users.sourceforge.net>
First post2015-12-26 07:40 +0100
Last post2015-12-26 20:40 +0100
Articles 7 — 3 participants

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] i2c-core: One function call less in acpi_i2c_space_handler()  after error detection SF Markus Elfring <elfring@users.sourceforge.net> - 2015-12-26 07:40 +0100
    Re: [PATCH] i2c-core: One function call less in  acpi_i2c_space_handler() after error detection kbuild test robot <lkp@intel.com> - 2015-12-26 07:50 +0100
      [PATCH v2] i2c-core: One function call less in  acpi_i2c_space_handler() after error detection SF Markus Elfring <elfring@users.sourceforge.net> - 2015-12-26 08:10 +0100
        Re: [PATCH v2] i2c-core: One function call less in  acpi_i2c_space_handler() after error detection Wolfram Sang <wsa@the-dreams.de> - 2015-12-26 08:50 +0100
          Re: i2c-core: One function call less in acpi_i2c_space_handler()  after error detection SF Markus Elfring <elfring@users.sourceforge.net> - 2015-12-26 10:00 +0100
            Re: i2c-core: One function call less in acpi_i2c_space_handler()  after error detection Wolfram Sang <wsa@the-dreams.de> - 2015-12-26 19:50 +0100
              Re: i2c-core: One function call less in acpi_i2c_space_handler()  after error detection SF Markus Elfring <elfring@users.sourceforge.net> - 2015-12-26 20:40 +0100

#1298165 — [PATCH] i2c-core: One function call less in acpi_i2c_space_handler() after error detection

FromSF Markus Elfring <elfring@users.sourceforge.net>
Date2015-12-26 07:40 +0100
Subject[PATCH] i2c-core: One function call less in acpi_i2c_space_handler() after error detection
Message-ID<qJQGd-5Qn-1@gated-at.bofh.it>
From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sat, 26 Dec 2015 07:30:35 +0100

The kfree() function was called in one case by the
acpi_i2c_space_handler() function during error handling
even if the passed variable "client" contained a null pointer.

Implementation details could be improved by the adjustment of jump targets.

This issue was detected by using the Coccinelle software.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/i2c/i2c-core.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
index 7349b00..a24e06c 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -313,18 +313,18 @@ acpi_i2c_space_handler(u32 function, acpi_physical_address command,
 	client = kzalloc(sizeof(*client), GFP_KERNEL);
 	if (!client) {
 		ret = AE_NO_MEMORY;
-		goto err;
+		goto free_ares;
 	}
 
 	if (!value64 || ares->type != ACPI_RESOURCE_TYPE_SERIAL_BUS) {
 		ret = AE_BAD_PARAMETER;
-		goto err;
+		goto free_client;
 	}
 
 	sb = &ares->data.i2c_serial_bus;
 	if (sb->type != ACPI_RESOURCE_SERIAL_TYPE_I2C) {
 		ret = AE_BAD_PARAMETER;
-		goto err;
+		goto free_client;
 	}
 
 	client->adapter = adapter;
@@ -405,9 +405,9 @@ acpi_i2c_space_handler(u32 function, acpi_physical_address command,
 	}
 
 	gsb->status = status;
-
- err:
+free_client:
 	kfree(client);
+free_ares:
 	ACPI_FREE(ares);
 	return ret;
 }
-- 
2.6.3

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1298166 — Re: [PATCH] i2c-core: One function call less in acpi_i2c_space_handler() after error detection

Fromkbuild test robot <lkp@intel.com>
Date2015-12-26 07:50 +0100
SubjectRe: [PATCH] i2c-core: One function call less in acpi_i2c_space_handler() after error detection
Message-ID<qJQPT-5Ub-1@gated-at.bofh.it>
In reply to#1298165

[Multipart message — attachments visible in raw view] — view raw

[if your patch is applied to the wrong git tree, please drop us a note to help improving the system]
Hi Markus,

[auto build test ERROR on wsa/i2c/for-next]
[also build test ERROR on v4.4-rc6 next-20151223]

url:    https://github.com/0day-ci/linux/commits/SF-Markus-Elfring/i2c-core-One-function-call-less-in-acpi_i2c_space_handler-after-error-detection/20151226-143820
base:   https://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux i2c/for-next
config: x86_64-randconfig-x010-12251849 (attached as .config)
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

All errors (new ones prefixed by >>):

   drivers/i2c/i2c-core.c: In function 'acpi_i2c_space_handler':
>> drivers/i2c/i2c-core.c:404:3: error: label 'err' used but not defined
      goto err;
      ^

vim +/err +404 drivers/i2c/i2c-core.c

17f4a5c4 Wolfram Sang 2014-09-22  398  		}
17f4a5c4 Wolfram Sang 2014-09-22  399  		break;
17f4a5c4 Wolfram Sang 2014-09-22  400  
17f4a5c4 Wolfram Sang 2014-09-22  401  	default:
17f4a5c4 Wolfram Sang 2014-09-22  402  		pr_info("protocol(0x%02x) is not supported.\n", accessor_type);
17f4a5c4 Wolfram Sang 2014-09-22  403  		ret = AE_BAD_PARAMETER;
17f4a5c4 Wolfram Sang 2014-09-22 @404  		goto err;
17f4a5c4 Wolfram Sang 2014-09-22  405  	}
17f4a5c4 Wolfram Sang 2014-09-22  406  
17f4a5c4 Wolfram Sang 2014-09-22  407  	gsb->status = status;

:::::: The code at line 404 was first introduced by commit
:::::: 17f4a5c47f28de9ea59182f48d07f8c44ee5dcc9 i2c: move acpi code back into the core

:::::: TO: Wolfram Sang <wsa@the-dreams.de>
:::::: CC: Wolfram Sang <wsa@the-dreams.de>

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

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

#1298167 — [PATCH v2] i2c-core: One function call less in acpi_i2c_space_handler() after error detection

FromSF Markus Elfring <elfring@users.sourceforge.net>
Date2015-12-26 08:10 +0100
Subject[PATCH v2] i2c-core: One function call less in acpi_i2c_space_handler() after error detection
Message-ID<qJR9f-6fG-1@gated-at.bofh.it>
In reply to#1298166
From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sat, 26 Dec 2015 08:00:52 +0100

The kfree() function was called in one case by the
acpi_i2c_space_handler() function during error handling
even if the passed variable "client" contained a null pointer.

Implementation details could be improved by the adjustment of jump targets
according to the Linux coding style convention.

This issue was detected by using the Coccinelle software.

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

diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
index 7349b00..9996531 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -313,18 +313,18 @@ acpi_i2c_space_handler(u32 function, acpi_physical_address command,
 	client = kzalloc(sizeof(*client), GFP_KERNEL);
 	if (!client) {
 		ret = AE_NO_MEMORY;
-		goto err;
+		goto free_ares;
 	}
 
 	if (!value64 || ares->type != ACPI_RESOURCE_TYPE_SERIAL_BUS) {
 		ret = AE_BAD_PARAMETER;
-		goto err;
+		goto free_client;
 	}
 
 	sb = &ares->data.i2c_serial_bus;
 	if (sb->type != ACPI_RESOURCE_SERIAL_TYPE_I2C) {
 		ret = AE_BAD_PARAMETER;
-		goto err;
+		goto free_client;
 	}
 
 	client->adapter = adapter;
@@ -401,13 +401,13 @@ acpi_i2c_space_handler(u32 function, acpi_physical_address command,
 	default:
 		pr_info("protocol(0x%02x) is not supported.\n", accessor_type);
 		ret = AE_BAD_PARAMETER;
-		goto err;
+		goto free_client;
 	}
 
 	gsb->status = status;
-
- err:
+free_client:
 	kfree(client);
+free_ares:
 	ACPI_FREE(ares);
 	return ret;
 }
-- 
2.6.3

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1298169 — Re: [PATCH v2] i2c-core: One function call less in acpi_i2c_space_handler() after error detection

FromWolfram Sang <wsa@the-dreams.de>
Date2015-12-26 08:50 +0100
SubjectRe: [PATCH v2] i2c-core: One function call less in acpi_i2c_space_handler() after error detection
Message-ID<qJRLX-6t9-5@gated-at.bofh.it>
In reply to#1298167

[Multipart message — attachments visible in raw view] — view raw

> The kfree() function was called in one case by the
> acpi_i2c_space_handler() function during error handling
> even if the passed variable "client" contained a null pointer.

This is OK. kfree() is known to be NULL-tolerant and we rely on it in
various places to keep the code simpler.

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

#1298171 — Re: i2c-core: One function call less in acpi_i2c_space_handler() after error detection

FromSF Markus Elfring <elfring@users.sourceforge.net>
Date2015-12-26 10:00 +0100
SubjectRe: i2c-core: One function call less in acpi_i2c_space_handler() after error detection
Message-ID<qJSRI-77i-1@gated-at.bofh.it>
In reply to#1298169
>> The kfree() function was called in one case by the
>> acpi_i2c_space_handler() function during error handling
>> even if the passed variable "client" contained a null pointer.
> 
> This is OK. kfree() is known to be NULL-tolerant and we rely on it in
> various places to keep the code simpler.

I would appreciate if an unnecessary function call can be avoided here
so that the affected exception handling can become also a bit more efficient.

Regards,
Markus
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1298248 — Re: i2c-core: One function call less in acpi_i2c_space_handler() after error detection

FromWolfram Sang <wsa@the-dreams.de>
Date2015-12-26 19:50 +0100
SubjectRe: i2c-core: One function call less in acpi_i2c_space_handler() after error detection
Message-ID<qK24F-4vv-5@gated-at.bofh.it>
In reply to#1298171

[Multipart message — attachments visible in raw view] — view raw

On Sat, Dec 26, 2015 at 09:52:11AM +0100, SF Markus Elfring wrote:
> >> The kfree() function was called in one case by the
> >> acpi_i2c_space_handler() function during error handling
> >> even if the passed variable "client" contained a null pointer.
> > 
> > This is OK. kfree() is known to be NULL-tolerant and we rely on it in
> > various places to keep the code simpler.
> 
> I would appreciate if an unnecessary function call can be avoided here
> so that the affected exception handling can become also a bit more efficient.

Simpler code is easier to maintain. See your patch, you didn't get it
correctly at your first try. Also, this is not a hot path, so I see
it as a micro-optimization also adding complexity. I don't favor that.

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

#1298254 — Re: i2c-core: One function call less in acpi_i2c_space_handler() after error detection

FromSF Markus Elfring <elfring@users.sourceforge.net>
Date2015-12-26 20:40 +0100
SubjectRe: i2c-core: One function call less in acpi_i2c_space_handler() after error detection
Message-ID<qK2R4-50U-1@gated-at.bofh.it>
In reply to#1298248
>> I would appreciate if an unnecessary function call can be avoided here
>> so that the affected exception handling can become also a bit more efficient.
> 
> Simpler code is easier to maintain.

There are different opinions available around the desired simplicity.


> See your patch, you didn't get it correctly at your first try.

I wonder myself about the circumstances on how my incomplete update suggestion
did happen.


> Also, this is not a hot path,

I'm curious if approaches around better exception handling can eventually become
a "hot topic".


> so I see it as a micro-optimization

I can agree to this view for this function implementation.

 
> also adding complexity.

There are the usual software development trade-offs.


> I don't favor that.

Thanks for your constructive feedback.

Is an identifier like "free_client" a bit nicer (according to the Linux coding
style recommendations) than the short jump label "err" in the discussed use case?

Regards,
Markus
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web