* [PATCH RFT] mach-s3c64xx:Fix error handling for certain calls to s3c_gpio_cfgpin_range in the file dev-audio.c
@ 2015-09-17 3:00 ` Nicholas Krause
0 siblings, 0 replies; 6+ messages in thread
From: Nicholas Krause @ 2015-09-17 3:00 UTC (permalink / raw)
To: kgene; +Cc: k.kozlowski, linux-samsung-soc, linux, linux-kernel,
linux-arm-kernel
This fixes error handling for calls to the function
s3c_gpio_cfgpin_range in the file dev-audio.c that
assume calls to this particular function always run
successfully to properly check now if these calls
fail by returning a error code and if so return it
directly as otherwise we fail to signal that the
pins have failed to changed their configuration to
future function users of the gpio configuration
of this device, therefore making them incorrectly
access a misconfigured gpio subsystem on the board
using this gpio driver.
Signed-off-by: Nicholas Krause <xerofoify@gmail.com>
---
arch/arm/mach-s3c64xx/dev-audio.c | 12 +++++++-----
1 file changed, 7 insertions(+), 5 deletions(-)
diff --git a/arch/arm/mach-s3c64xx/dev-audio.c b/arch/arm/mach-s3c64xx/dev-audio.c
index ff780a8..81fabdb 100644
--- a/arch/arm/mach-s3c64xx/dev-audio.c
+++ b/arch/arm/mach-s3c64xx/dev-audio.c
@@ -27,6 +27,7 @@
static int s3c64xx_i2s_cfg_gpio(struct platform_device *pdev)
{
unsigned int base;
+ int ret;
switch (pdev->id) {
case 0:
@@ -47,9 +48,9 @@ static int s3c64xx_i2s_cfg_gpio(struct platform_device *pdev)
return -EINVAL;
}
- s3c_gpio_cfgpin_range(base, 5, S3C_GPIO_SFN(3));
-
- return 0;
+ ret = s3c_gpio_cfgpin_range(base, 5, S3C_GPIO_SFN(3));
+
+ return ret;
}
static struct resource s3c64xx_iis0_resource[] = {
@@ -122,6 +123,7 @@ EXPORT_SYMBOL(s3c64xx_device_iisv4);
static int s3c64xx_pcm_cfg_gpio(struct platform_device *pdev)
{
unsigned int base;
+ int ret;
switch (pdev->id) {
case 0:
@@ -136,8 +138,8 @@ static int s3c64xx_pcm_cfg_gpio(struct platform_device *pdev)
return -EINVAL;
}
- s3c_gpio_cfgpin_range(base, 5, S3C_GPIO_SFN(2));
- return 0;
+ ret = s3c_gpio_cfgpin_range(base, 5, S3C_GPIO_SFN(2));
+ return ret;
}
static struct resource s3c64xx_pcm0_resource[] = {
--
2.1.4
^ permalink raw reply related [flat|nested] 6+ messages in thread* [PATCH RFT] mach-s3c64xx:Fix error handling for certain calls to s3c_gpio_cfgpin_range in the file dev-audio.c
@ 2015-09-17 3:00 ` Nicholas Krause
0 siblings, 0 replies; 6+ messages in thread
From: Nicholas Krause @ 2015-09-17 3:00 UTC (permalink / raw)
To: linux-arm-kernel
This fixes error handling for calls to the function
s3c_gpio_cfgpin_range in the file dev-audio.c that
assume calls to this particular function always run
successfully to properly check now if these calls
fail by returning a error code and if so return it
directly as otherwise we fail to signal that the
pins have failed to changed their configuration to
future function users of the gpio configuration
of this device, therefore making them incorrectly
access a misconfigured gpio subsystem on the board
using this gpio driver.
Signed-off-by: Nicholas Krause <xerofoify@gmail.com>
---
arch/arm/mach-s3c64xx/dev-audio.c | 12 +++++++-----
1 file changed, 7 insertions(+), 5 deletions(-)
diff --git a/arch/arm/mach-s3c64xx/dev-audio.c b/arch/arm/mach-s3c64xx/dev-audio.c
index ff780a8..81fabdb 100644
--- a/arch/arm/mach-s3c64xx/dev-audio.c
+++ b/arch/arm/mach-s3c64xx/dev-audio.c
@@ -27,6 +27,7 @@
static int s3c64xx_i2s_cfg_gpio(struct platform_device *pdev)
{
unsigned int base;
+ int ret;
switch (pdev->id) {
case 0:
@@ -47,9 +48,9 @@ static int s3c64xx_i2s_cfg_gpio(struct platform_device *pdev)
return -EINVAL;
}
- s3c_gpio_cfgpin_range(base, 5, S3C_GPIO_SFN(3));
-
- return 0;
+ ret = s3c_gpio_cfgpin_range(base, 5, S3C_GPIO_SFN(3));
+
+ return ret;
}
static struct resource s3c64xx_iis0_resource[] = {
@@ -122,6 +123,7 @@ EXPORT_SYMBOL(s3c64xx_device_iisv4);
static int s3c64xx_pcm_cfg_gpio(struct platform_device *pdev)
{
unsigned int base;
+ int ret;
switch (pdev->id) {
case 0:
@@ -136,8 +138,8 @@ static int s3c64xx_pcm_cfg_gpio(struct platform_device *pdev)
return -EINVAL;
}
- s3c_gpio_cfgpin_range(base, 5, S3C_GPIO_SFN(2));
- return 0;
+ ret = s3c_gpio_cfgpin_range(base, 5, S3C_GPIO_SFN(2));
+ return ret;
}
static struct resource s3c64xx_pcm0_resource[] = {
--
2.1.4
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH RFT] mach-s3c64xx:Fix error handling for certain calls to s3c_gpio_cfgpin_range in the file dev-audio.c
2015-09-17 3:00 ` Nicholas Krause
@ 2015-09-17 10:23 ` Russell King - ARM Linux
-1 siblings, 0 replies; 6+ messages in thread
From: Russell King - ARM Linux @ 2015-09-17 10:23 UTC (permalink / raw)
To: Nicholas Krause
Cc: kgene, k.kozlowski, linux-arm-kernel, linux-samsung-soc,
linux-kernel
On Wed, Sep 16, 2015 at 11:00:35PM -0400, Nicholas Krause wrote:
> diff --git a/arch/arm/mach-s3c64xx/dev-audio.c b/arch/arm/mach-s3c64xx/dev-audio.c
> index ff780a8..81fabdb 100644
> --- a/arch/arm/mach-s3c64xx/dev-audio.c
> +++ b/arch/arm/mach-s3c64xx/dev-audio.c
> @@ -27,6 +27,7 @@
> static int s3c64xx_i2s_cfg_gpio(struct platform_device *pdev)
> {
> unsigned int base;
> + int ret;
>
> switch (pdev->id) {
> case 0:
> @@ -47,9 +48,9 @@ static int s3c64xx_i2s_cfg_gpio(struct platform_device *pdev)
> return -EINVAL;
> }
>
> - s3c_gpio_cfgpin_range(base, 5, S3C_GPIO_SFN(3));
> -
> - return 0;
> + ret = s3c_gpio_cfgpin_range(base, 5, S3C_GPIO_SFN(3));
> +
> + return ret;
Let's look at the code:
case 2:
s3c_gpio_cfgpin(S3C64XX_GPC(4), S3C_GPIO_SFN(5));
s3c_gpio_cfgpin(S3C64XX_GPC(5), S3C_GPIO_SFN(5));
s3c_gpio_cfgpin(S3C64XX_GPC(7), S3C_GPIO_SFN(5));
s3c_gpio_cfgpin_range(S3C64XX_GPH(6), 4, S3C_GPIO_SFN(5));
return 0;
default:
printk(KERN_DEBUG "Invalid I2S Controller number: %d\n",
pdev->id);
return -EINVAL;
}
s3c_gpio_cfgpin_range(base, 5, S3C_GPIO_SFN(3));
return 0;
and you're changing it to:
case 2:
s3c_gpio_cfgpin(S3C64XX_GPC(4), S3C_GPIO_SFN(5));
s3c_gpio_cfgpin(S3C64XX_GPC(5), S3C_GPIO_SFN(5));
s3c_gpio_cfgpin(S3C64XX_GPC(7), S3C_GPIO_SFN(5));
s3c_gpio_cfgpin_range(S3C64XX_GPH(6), 4, S3C_GPIO_SFN(5));
return 0;
default:
printk(KERN_DEBUG "Invalid I2S Controller number: %d\n",
pdev->id);
return -EINVAL;
}
ret = s3c_gpio_cfgpin_range(base, 5, S3C_GPIO_SFN(3));
return ret;
What about all the previous calls to s3c_gpio_cfgpin_range() and
s3c_gpio_cfgpin() ? Can't they fail as well?
However, _maybe_ the original authors idea was "I don't care if these
calls fail, it's safer to continue if they do" and your changes actually
result in breakage.
Maybe the better solution would be to add WARN_ON() around each of these.
There's a lot of questions here, none of them are a trivial case of "lets
generate a patch to just do some random change to the code and hope it's
right."
--
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
^ permalink raw reply [flat|nested] 6+ messages in thread* [PATCH RFT] mach-s3c64xx:Fix error handling for certain calls to s3c_gpio_cfgpin_range in the file dev-audio.c
@ 2015-09-17 10:23 ` Russell King - ARM Linux
0 siblings, 0 replies; 6+ messages in thread
From: Russell King - ARM Linux @ 2015-09-17 10:23 UTC (permalink / raw)
To: linux-arm-kernel
On Wed, Sep 16, 2015 at 11:00:35PM -0400, Nicholas Krause wrote:
> diff --git a/arch/arm/mach-s3c64xx/dev-audio.c b/arch/arm/mach-s3c64xx/dev-audio.c
> index ff780a8..81fabdb 100644
> --- a/arch/arm/mach-s3c64xx/dev-audio.c
> +++ b/arch/arm/mach-s3c64xx/dev-audio.c
> @@ -27,6 +27,7 @@
> static int s3c64xx_i2s_cfg_gpio(struct platform_device *pdev)
> {
> unsigned int base;
> + int ret;
>
> switch (pdev->id) {
> case 0:
> @@ -47,9 +48,9 @@ static int s3c64xx_i2s_cfg_gpio(struct platform_device *pdev)
> return -EINVAL;
> }
>
> - s3c_gpio_cfgpin_range(base, 5, S3C_GPIO_SFN(3));
> -
> - return 0;
> + ret = s3c_gpio_cfgpin_range(base, 5, S3C_GPIO_SFN(3));
> +
> + return ret;
Let's look at the code:
case 2:
s3c_gpio_cfgpin(S3C64XX_GPC(4), S3C_GPIO_SFN(5));
s3c_gpio_cfgpin(S3C64XX_GPC(5), S3C_GPIO_SFN(5));
s3c_gpio_cfgpin(S3C64XX_GPC(7), S3C_GPIO_SFN(5));
s3c_gpio_cfgpin_range(S3C64XX_GPH(6), 4, S3C_GPIO_SFN(5));
return 0;
default:
printk(KERN_DEBUG "Invalid I2S Controller number: %d\n",
pdev->id);
return -EINVAL;
}
s3c_gpio_cfgpin_range(base, 5, S3C_GPIO_SFN(3));
return 0;
and you're changing it to:
case 2:
s3c_gpio_cfgpin(S3C64XX_GPC(4), S3C_GPIO_SFN(5));
s3c_gpio_cfgpin(S3C64XX_GPC(5), S3C_GPIO_SFN(5));
s3c_gpio_cfgpin(S3C64XX_GPC(7), S3C_GPIO_SFN(5));
s3c_gpio_cfgpin_range(S3C64XX_GPH(6), 4, S3C_GPIO_SFN(5));
return 0;
default:
printk(KERN_DEBUG "Invalid I2S Controller number: %d\n",
pdev->id);
return -EINVAL;
}
ret = s3c_gpio_cfgpin_range(base, 5, S3C_GPIO_SFN(3));
return ret;
What about all the previous calls to s3c_gpio_cfgpin_range() and
s3c_gpio_cfgpin() ? Can't they fail as well?
However, _maybe_ the original authors idea was "I don't care if these
calls fail, it's safer to continue if they do" and your changes actually
result in breakage.
Maybe the better solution would be to add WARN_ON() around each of these.
There's a lot of questions here, none of them are a trivial case of "lets
generate a patch to just do some random change to the code and hope it's
right."
--
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH RFT] mach-s3c64xx:Fix error handling for certain calls to s3c_gpio_cfgpin_range in the file dev-audio.c
2015-09-17 10:23 ` Russell King - ARM Linux
@ 2015-09-17 10:52 ` nick
-1 siblings, 0 replies; 6+ messages in thread
From: nick @ 2015-09-17 10:52 UTC (permalink / raw)
To: Russell King - ARM Linux
Cc: linux-samsung-soc, k.kozlowski, kgene, linux-kernel,
linux-arm-kernel
On 2015-09-17 06:23 AM, Russell King - ARM Linux wrote:
> On Wed, Sep 16, 2015 at 11:00:35PM -0400, Nicholas Krause wrote:
>> diff --git a/arch/arm/mach-s3c64xx/dev-audio.c b/arch/arm/mach-s3c64xx/dev-audio.c
>> index ff780a8..81fabdb 100644
>> --- a/arch/arm/mach-s3c64xx/dev-audio.c
>> +++ b/arch/arm/mach-s3c64xx/dev-audio.c
>> @@ -27,6 +27,7 @@
>> static int s3c64xx_i2s_cfg_gpio(struct platform_device *pdev)
>> {
>> unsigned int base;
>> + int ret;
>>
>> switch (pdev->id) {
>> case 0:
>> @@ -47,9 +48,9 @@ static int s3c64xx_i2s_cfg_gpio(struct platform_device *pdev)
>> return -EINVAL;
>> }
>>
>> - s3c_gpio_cfgpin_range(base, 5, S3C_GPIO_SFN(3));
>> -
>> - return 0;
>> + ret = s3c_gpio_cfgpin_range(base, 5, S3C_GPIO_SFN(3));
>> +
>> + return ret;
>
> Let's look at the code:
>
> case 2:
> s3c_gpio_cfgpin(S3C64XX_GPC(4), S3C_GPIO_SFN(5));
> s3c_gpio_cfgpin(S3C64XX_GPC(5), S3C_GPIO_SFN(5));
> s3c_gpio_cfgpin(S3C64XX_GPC(7), S3C_GPIO_SFN(5));
> s3c_gpio_cfgpin_range(S3C64XX_GPH(6), 4, S3C_GPIO_SFN(5));
> return 0;
> default:
> printk(KERN_DEBUG "Invalid I2S Controller number: %d\n",
> pdev->id);
> return -EINVAL;
> }
>
> s3c_gpio_cfgpin_range(base, 5, S3C_GPIO_SFN(3));
>
> return 0;
>
> and you're changing it to:
>
> case 2:
> s3c_gpio_cfgpin(S3C64XX_GPC(4), S3C_GPIO_SFN(5));
> s3c_gpio_cfgpin(S3C64XX_GPC(5), S3C_GPIO_SFN(5));
> s3c_gpio_cfgpin(S3C64XX_GPC(7), S3C_GPIO_SFN(5));
> s3c_gpio_cfgpin_range(S3C64XX_GPH(6), 4, S3C_GPIO_SFN(5));
> return 0;
> default:
> printk(KERN_DEBUG "Invalid I2S Controller number: %d\n",
> pdev->id);
> return -EINVAL;
> }
>
> ret = s3c_gpio_cfgpin_range(base, 5, S3C_GPIO_SFN(3));
>
> return ret;
>
> What about all the previous calls to s3c_gpio_cfgpin_range() and
> s3c_gpio_cfgpin() ? Can't they fail as well?
>
> However, _maybe_ the original authors idea was "I don't care if these
> calls fail, it's safer to continue if they do" and your changes actually
> result in breakage.
>
I considered that maybe I was a little too hasty when reading this function
definition and came to the wrong conclusion when writing this patch :(.
> Maybe the better solution would be to add WARN_ON() around each of these.
>
WARN_ON around these is better as they cannot fail or the gpio configuration
for this board will not function properly making part of this board no longer
usable until a reboot or hard restart.
Nick
> There's a lot of questions here, none of them are a trivial case of "lets
> generate a patch to just do some random change to the code and hope it's
> right."
>
^ permalink raw reply [flat|nested] 6+ messages in thread* [PATCH RFT] mach-s3c64xx:Fix error handling for certain calls to s3c_gpio_cfgpin_range in the file dev-audio.c
@ 2015-09-17 10:52 ` nick
0 siblings, 0 replies; 6+ messages in thread
From: nick @ 2015-09-17 10:52 UTC (permalink / raw)
To: linux-arm-kernel
On 2015-09-17 06:23 AM, Russell King - ARM Linux wrote:
> On Wed, Sep 16, 2015 at 11:00:35PM -0400, Nicholas Krause wrote:
>> diff --git a/arch/arm/mach-s3c64xx/dev-audio.c b/arch/arm/mach-s3c64xx/dev-audio.c
>> index ff780a8..81fabdb 100644
>> --- a/arch/arm/mach-s3c64xx/dev-audio.c
>> +++ b/arch/arm/mach-s3c64xx/dev-audio.c
>> @@ -27,6 +27,7 @@
>> static int s3c64xx_i2s_cfg_gpio(struct platform_device *pdev)
>> {
>> unsigned int base;
>> + int ret;
>>
>> switch (pdev->id) {
>> case 0:
>> @@ -47,9 +48,9 @@ static int s3c64xx_i2s_cfg_gpio(struct platform_device *pdev)
>> return -EINVAL;
>> }
>>
>> - s3c_gpio_cfgpin_range(base, 5, S3C_GPIO_SFN(3));
>> -
>> - return 0;
>> + ret = s3c_gpio_cfgpin_range(base, 5, S3C_GPIO_SFN(3));
>> +
>> + return ret;
>
> Let's look at the code:
>
> case 2:
> s3c_gpio_cfgpin(S3C64XX_GPC(4), S3C_GPIO_SFN(5));
> s3c_gpio_cfgpin(S3C64XX_GPC(5), S3C_GPIO_SFN(5));
> s3c_gpio_cfgpin(S3C64XX_GPC(7), S3C_GPIO_SFN(5));
> s3c_gpio_cfgpin_range(S3C64XX_GPH(6), 4, S3C_GPIO_SFN(5));
> return 0;
> default:
> printk(KERN_DEBUG "Invalid I2S Controller number: %d\n",
> pdev->id);
> return -EINVAL;
> }
>
> s3c_gpio_cfgpin_range(base, 5, S3C_GPIO_SFN(3));
>
> return 0;
>
> and you're changing it to:
>
> case 2:
> s3c_gpio_cfgpin(S3C64XX_GPC(4), S3C_GPIO_SFN(5));
> s3c_gpio_cfgpin(S3C64XX_GPC(5), S3C_GPIO_SFN(5));
> s3c_gpio_cfgpin(S3C64XX_GPC(7), S3C_GPIO_SFN(5));
> s3c_gpio_cfgpin_range(S3C64XX_GPH(6), 4, S3C_GPIO_SFN(5));
> return 0;
> default:
> printk(KERN_DEBUG "Invalid I2S Controller number: %d\n",
> pdev->id);
> return -EINVAL;
> }
>
> ret = s3c_gpio_cfgpin_range(base, 5, S3C_GPIO_SFN(3));
>
> return ret;
>
> What about all the previous calls to s3c_gpio_cfgpin_range() and
> s3c_gpio_cfgpin() ? Can't they fail as well?
>
> However, _maybe_ the original authors idea was "I don't care if these
> calls fail, it's safer to continue if they do" and your changes actually
> result in breakage.
>
I considered that maybe I was a little too hasty when reading this function
definition and came to the wrong conclusion when writing this patch :(.
> Maybe the better solution would be to add WARN_ON() around each of these.
>
WARN_ON around these is better as they cannot fail or the gpio configuration
for this board will not function properly making part of this board no longer
usable until a reboot or hard restart.
Nick
> There's a lot of questions here, none of them are a trivial case of "lets
> generate a patch to just do some random change to the code and hope it's
> right."
>
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2015-09-17 10:52 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-09-17 3:00 [PATCH RFT] mach-s3c64xx:Fix error handling for certain calls to s3c_gpio_cfgpin_range in the file dev-audio.c Nicholas Krause
2015-09-17 3:00 ` Nicholas Krause
2015-09-17 10:23 ` Russell King - ARM Linux
2015-09-17 10:23 ` Russell King - ARM Linux
2015-09-17 10:52 ` nick
2015-09-17 10:52 ` nick
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.