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

Submitted by Archit Taneja on April 19, 2016, 10:56 a.m.

Details

Message ID 1461063396-2285-2-git-send-email-architt@codeaurora.org
State New
Headers show
Series "drm/msm: Remove drm_driver load/unload ops" ( rev: 1 ) in DRI devel

Not browsing as part of any series.

Commit Message

Archit Taneja April 19, 2016, 10:56 a.m.
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(-)

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 Tue 19 Apr 03:56 PDT 2016, 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);
> +			}
>  		};
>  
>  		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;
>  }

The patch in itself looks good, but the bigger picture does not.

The ddc and hdp should be muxed to the hdmi block, so they should not
operated as gpios.

The mux seems more of a gpio so it should be made more explicit - i.e.
actually support muxing (if that's needed) rather than just setting hard
coded values.

And you should not gpio_request/free the gpio upon every usage, request
it during "probe time" and release it during "remove".

Regards,
Bjorn
On Apr 19, 2016 11:50, "Bjorn Andersson" <bjorn.andersson@linaro.org> wrote:
>
> On Tue 19 Apr 03:56 PDT 2016, 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);
> > +                     }
> >               };
> >
> >               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;
> >  }
>
> The patch in itself looks good, but the bigger picture does not.
>
> The ddc and hdp should be muxed to the hdmi block, so they should not
> operated as gpios.
>
> The mux seems more of a gpio so it should be made more explicit - i.e.
> actually support muxing (if that's needed) rather than just setting hard
> coded values.

Note that at least on some devices, hpd was unreliable without using a
combination of gpio and denounced hpd signal from HDMI block...

Not sure what sort of MUX it is but it seemed possible (and necessary) to
use both at same time..

Please be sure to test lots of devices and monitors if you are going to
change this ;-)

BR,
-R

> And you should not gpio_request/free the gpio upon every usage, request
> it during "probe time" and release it during "remove".
>
> Regards,
> Bjorn
> --
> To unsubscribe from this list: send the line "unsubscribe linux-arm-msm"
in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
On Tue 19 Apr 10:44 PDT 2016, Rob Clark wrote:

>    On Apr 19, 2016 11:50, "Bjorn Andersson" <bjorn.andersson@linaro.org>
>    wrote:
>    >
>    > On Tue 19 Apr 03:56 PDT 2016, Archit Taneja wrote:
>    >
[..]
>    >
>    > The patch in itself looks good, but the bigger picture does not.
>    >
>    > The ddc and hdp should be muxed to the hdmi block, so they should not
>    > operated as gpios.
>    >
>    > The mux seems more of a gpio so it should be made more explicit - i.e.
>    > actually support muxing (if that's needed) rather than just setting hard
>    > coded values.
> 
>    Note that at least on some devices, hpd was unreliable without using a
>    combination of gpio and denounced hpd signal from HDMI block...
> 

Right, I do think it makes sense to keep the detect-gpio.

>    Not sure what sort of MUX it is but it seemed possible (and necessary) to
>    use both at same time..
> 

The "liquid" devices seems to use it, but I haven't managed to figure
out for what.

>    Please be sure to test lots of devices and monitors if you are going to
>    change this ;-)
> 

Scary...


But I do believe that we should stop requesting/freeing the gpios every
time we try to enable/disable them and in the longer run we should drop
the ddc pins, make the hpd an explicit standalone gpiod handle and
figure out what the muxing is all about (and make that standalone as
well).


The patch solves the immediate symptom, but I think we should give it an
overhaul later. If nothing else just to not to carry the downstream
legacy of always gpio_requesting non-gpio pins.

Regards,
Bjorn
Hi Bjorn, Rob,

On 04/19/2016 11:14 PM, Rob Clark wrote:
>
> On Apr 19, 2016 11:50, "Bjorn Andersson" <bjorn.andersson@linaro.org
> <mailto:bjorn.andersson@linaro.org>> wrote:
>  >
>  > On Tue 19 Apr 03:56 PDT 2016, 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
> <mailto: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);
>  > > +                     }
>  > >               };
>  > >
>  > >               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;
>  > >  }
>  >
>  > The patch in itself looks good, but the bigger picture does not.
>  >
>  > The ddc and hdp should be muxed to the hdmi block, so they should not
>  > operated as gpios.

I agree on the DDC part. I am not sure why they are passed as GPIOs to
the driver. For boards that do this (i.e, 1fc6410), the pinctrl tlmm
node sets the GPIOs function as "hdmi". Does that mean the the
gpio_set_value() in the driver are simply ignored?

I'll test and drop these if things work fine without them.

>  >
>  > The mux seems more of a gpio so it should be made more explicit - i.e.
>  > actually support muxing (if that's needed) rather than just setting hard
>  > coded values.

I have to study the schematics a bit on what the MUX actually does.

>
> Note that at least on some devices, hpd was unreliable without using a
> combination of gpio and denounced hpd signal from HDMI block...

Hmm, one thing I don't get is that the pinctrl DT node for the HPD pin
explicitly assigns its function as "hdmi". If that's the case, how does
it even work as a gpio?

>
> Not sure what sort of MUX it is but it seemed possible (and necessary)
> to use both at same time..
>
> Please be sure to test lots of devices and monitors if you are going to
> change this ;-)

For sure!

>
> BR,
> -R
>
>  > And you should not gpio_request/free the gpio upon every usage, request
>  > it during "probe time" and release it during "remove".

The hpd_enable/disable functions actually get called only during probe
and remove. We don't enable/disable HPD sensing at other points in the
driver. So, I guess it should be okay for now.

Thanks,
Archit