[v2,1/3] drm/msm/hdmi: Prevent gpio_free related kernel warnings

Submitted by Archit Taneja on April 25, 2016, 10:16 a.m.

Details

Message ID 1461579402-14301-2-git-send-email-architt@codeaurora.org
State New
Headers show

Not browsing as part of any series.

Patch hide | download patch | download mbox

diff --git a/drivers/gpu/drm/msm/hdmi/hdmi_connector.c b/drivers/gpu/drm/msm/hdmi/hdmi_connector.c
index 26129bf..ce86117 100644
--- a/drivers/gpu/drm/msm/hdmi/hdmi_connector.c
+++ b/drivers/gpu/drm/msm/hdmi/hdmi_connector.c
@@ -112,13 +112,16 @@  static int gpio_config(struct hdmi *hdmi, bool on)
 		for (i = 0; i < HDMI_MAX_NUM_GPIO; i++) {
 			struct hdmi_gpio_data gpio = config->gpios[i];
 
-			if (gpio.output) {
-				int value = gpio.value ? 0 : 1;
+			if (gpio.num != -1) {
+				if (gpio.output) {
+					int value = gpio.value ? 0 : 1;
 
-				gpio_set_value_cansleep(gpio.num, value);
-			}
+					gpio_set_value_cansleep(gpio.num,
+								value);
+				}
 
-			gpio_free(gpio.num);
+				gpio_free(gpio.num);
+			}
 		};
 
 		DBG("gpio off");
@@ -126,8 +129,10 @@  static int gpio_config(struct hdmi *hdmi, bool on)
 
 	return 0;
 err:
-	while (i--)
-		gpio_free(config->gpios[i].num);
+	while (i--) {
+		if (config->gpios[i].num != -1)
+			gpio_free(config->gpios[i].num);
+	}
 
 	return ret;
 }

Comments

On 2016-04-25 03:16, Archit Taneja wrote:
> Calling the legacy gpio_free on an invalid GPIO (a GPIO numbered -1)
> results in kernel warnings. This causes a lot of backtraces when
> we try to unload the drm/msm module.
> 
> Call gpio_free only on valid GPIOs.
> 
> Signed-off-by: Archit Taneja <architt@codeaurora.org>
> ---
>  drivers/gpu/drm/msm/hdmi/hdmi_connector.c | 19 ++++++++++++-------
>  1 file changed, 12 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/hdmi/hdmi_connector.c
> b/drivers/gpu/drm/msm/hdmi/hdmi_connector.c
> index 26129bf..ce86117 100644
> --- a/drivers/gpu/drm/msm/hdmi/hdmi_connector.c
> +++ b/drivers/gpu/drm/msm/hdmi/hdmi_connector.c
> @@ -112,13 +112,16 @@ static int gpio_config(struct hdmi *hdmi, bool 
> on)
>  		for (i = 0; i < HDMI_MAX_NUM_GPIO; i++) {
>  			struct hdmi_gpio_data gpio = config->gpios[i];
> 
> -			if (gpio.output) {
> -				int value = gpio.value ? 0 : 1;
> +			if (gpio.num != -1) {
> +				if (gpio.output) {
> +					int value = gpio.value ? 0 : 1;
> 
> -				gpio_set_value_cansleep(gpio.num, value);
> -			}
> +					gpio_set_value_cansleep(gpio.num,
> +								value);
> +				}
> 
> -			gpio_free(gpio.num);
> +				gpio_free(gpio.num);
> +			}

Can you do something like:

if (gpio.num == -1)
     continue;

instead? That would avoid the additional indentation (and increase 
readability).

Thomas
On 04/25/2016 11:18 PM, twp@codeaurora.org wrote:
> On 2016-04-25 03:16, Archit Taneja wrote:
>> Calling the legacy gpio_free on an invalid GPIO (a GPIO numbered -1)
>> results in kernel warnings. This causes a lot of backtraces when
>> we try to unload the drm/msm module.
>>
>> Call gpio_free only on valid GPIOs.
>>
>> Signed-off-by: Archit Taneja <architt@codeaurora.org>
>> ---
>>  drivers/gpu/drm/msm/hdmi/hdmi_connector.c | 19 ++++++++++++-------
>>  1 file changed, 12 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/msm/hdmi/hdmi_connector.c
>> b/drivers/gpu/drm/msm/hdmi/hdmi_connector.c
>> index 26129bf..ce86117 100644
>> --- a/drivers/gpu/drm/msm/hdmi/hdmi_connector.c
>> +++ b/drivers/gpu/drm/msm/hdmi/hdmi_connector.c
>> @@ -112,13 +112,16 @@ static int gpio_config(struct hdmi *hdmi, bool on)
>>          for (i = 0; i < HDMI_MAX_NUM_GPIO; i++) {
>>              struct hdmi_gpio_data gpio = config->gpios[i];
>>
>> -            if (gpio.output) {
>> -                int value = gpio.value ? 0 : 1;
>> +            if (gpio.num != -1) {
>> +                if (gpio.output) {
>> +                    int value = gpio.value ? 0 : 1;
>>
>> -                gpio_set_value_cansleep(gpio.num, value);
>> -            }
>> +                    gpio_set_value_cansleep(gpio.num,
>> +                                value);
>> +                }
>>
>> -            gpio_free(gpio.num);
>> +                gpio_free(gpio.num);
>> +            }
>
> Can you do something like:
>
> if (gpio.num == -1)
>      continue;
>
> instead? That would avoid the additional indentation (and increase
> readability).

Yes, that should make things cleaner. I'll make this change.

Thanks,
Archit