[PATCH RFT] mach-s3c64xx:Fix error handling for certain calls to s3c_gpio_cfgpin_range in the file dev-audio.c

[PATCH RFT] mach-s3c64xx:Fix error handling for certain calls to s3c_gpio_cfgpin_range in the file dev-audio.c

[Nicholas Krause]

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

[PATCH RFT] mach-s3c64xx:Fix error handling for certain calls to s3c_gpio_cfgpin_range in the file dev-audio.c

[Russell King - ARM Linux]

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.

[PATCH RFT] mach-s3c64xx:Fix error handling for certain calls to s3c_gpio_cfgpin_range in the file dev-audio.c

[nick]

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."
>