[PATCH 0/1] hopefully fix null pointer dereference on i915 load

[PATCH 0/1] hopefully fix null pointer dereference on i915 load

[Jani Nikula]

Hi Mihai, could you test the following patch to see if it fixes the problem,
please?

BR,
Jani.


Jani Nikula (1):
  drm/i915: ensure i2c adapter is all set before adding it

 drivers/gpu/drm/i915/intel_i2c.c |    7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

-- 
1.7.9.5

[PATCH 1/1] drm/i915: ensure i2c adapter is all set before adding it

[Jani Nikula]

i2c_add_adapter() may do i2c transfers on the bus to detect supported
devices. Therefore the adapter needs to be all set before adding it. This
was not the case for the bit-banging fallback, resulting in an oops if the
device detection GMBUS transfers timed out. Fix the issue by calling
i2c_add_adapter() only after intel_gpio_setup().

LKML-Reference: <5021F00B.7000503@ionic.de>
Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/i915/intel_i2c.c |    7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_i2c.c b/drivers/gpu/drm/i915/intel_i2c.c
index 1991a44..a23ba84f 100644
--- a/drivers/gpu/drm/i915/intel_i2c.c
+++ b/drivers/gpu/drm/i915/intel_i2c.c
@@ -486,9 +486,6 @@ int intel_setup_gmbus(struct drm_device *dev)
 		bus->dev_priv = dev_priv;
 
 		bus->adapter.algo = &gmbus_algorithm;
-		ret = i2c_add_adapter(&bus->adapter);
-		if (ret)
-			goto err;
 
 		/* By default use a conservative clock rate */
 		bus->reg0 = port | GMBUS_RATE_100KHZ;
@@ -498,6 +495,10 @@ int intel_setup_gmbus(struct drm_device *dev)
 			bus->force_bit = true;
 
 		intel_gpio_setup(bus, port);
+
+		ret = i2c_add_adapter(&bus->adapter);
+		if (ret)
+			goto err;
 	}
 
 	intel_i2c_reset(dev_priv->dev);
-- 
1.7.9.5

Re: [PATCH 0/1] hopefully fix null pointer dereference on i915 load

[Mihai Moldovan]

[-- Attachment #1: Type: text/plain, Size: 3126 bytes --]

Hi Jani,

* On 13.08.2012 04:33 PM, Jani Nikula wrote:
> Hi Mihai, could you test the following patch to see if it fixes the problem,
> please?
>
> BR,
> Jani.
>
>
> Jani Nikula (1):
>   drm/i915: ensure i2c adapter is all set before adding it
>
>  drivers/gpu/drm/i915/intel_i2c.c |    7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
>

The reason sounds sane to me, but while looking through the code, I have seen a
few other problems, too.

To my understanding, we should use port for dev_priv->gmbus[], not the pin
mapping (which is only used for gmbus_ports[]).
Don't forget to add the +1 for pin -> port mapping to the error case.

Also, intel_gmbus_get_adapter is already accepting a port value (I made sure to
look at the calls in other files too), so don't map the port back to a pin.

Keep the same in mind for the intel_teardown_gmbus "destructor".

The current code adds the gmbus algorithm (gmbus_xfer) to gmbus port 0, which is
known as "disabled" and shouldn't be used (previously has_gpio was set to false
for those ports to not do any transfer on those ports.)

I may be wrong, could you review this and maybe add it to your patch?


diff --git a/drivers/gpu/drm/i915/intel_i2c.c b/drivers/gpu/drm/i915/intel_i2c.c
index 1991a44..b725993 100644
--- a/drivers/gpu/drm/i915/intel_i2c.c
+++ b/drivers/gpu/drm/i915/intel_i2c.c
@@ -472,8 +474,8 @@ int intel_setup_gmbus(struct drm_device *dev)
        mutex_init(&dev_priv->gmbus_mutex);

        for (i = 0; i < GMBUS_NUM_PORTS; i++) {
-               struct intel_gmbus *bus = &dev_priv->gmbus[i];
                u32 port = i + 1; /* +1 to map gmbus index to pin pair */
+               struct intel_gmbus *bus = &dev_priv->gmbus[port];

                bus->adapter.owner = THIS_MODULE;
                bus->adapter.class = I2C_CLASS_DDC;
@@ -506,7 +508,7 @@ int intel_setup_gmbus(struct drm_device *dev)

 err:
        while (--i) {
-               struct intel_gmbus *bus = &dev_priv->gmbus[i];
+               struct intel_gmbus *bus = &dev_priv->gmbus[i + 1];
                i2c_del_adapter(&bus->adapter);
        }
        return ret;
@@ -516,9 +518,8 @@ struct i2c_adapter *intel_gmbus_get_adapter(struct
drm_i915_private *dev_priv,
                                            unsigned port)
 {
        WARN_ON(!intel_gmbus_is_port_valid(port));
-       /* -1 to map pin pair to gmbus index */
        return (intel_gmbus_is_port_valid(port)) ?
-               &dev_priv->gmbus[port - 1].adapter : NULL;
+               &dev_priv->gmbus[port].adapter : NULL;
 }

 void intel_gmbus_set_speed(struct i2c_adapter *adapter, int speed)
@@ -543,8 +544,9 @@ void intel_teardown_gmbus(struct drm_device *dev)
        if (dev_priv->gmbus == NULL)
                return;

+        /* +1 to map gmbus index to pin pair */
        for (i = 0; i < GMBUS_NUM_PORTS; i++) {
-               struct intel_gmbus *bus = &dev_priv->gmbus[i];
+               struct intel_gmbus *bus = &dev_priv->gmbus[i + 1];
                i2c_del_adapter(&bus->adapter);
        }
 }




[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4506 bytes --]

Re: [PATCH 0/1] hopefully fix null pointer dereference on i915 load

[Daniel Vetter]

On Mon, Aug 13, 2012 at 05:03:24PM +0200, Mihai Moldovan wrote:
> Hi Jani,
> 
> * On 13.08.2012 04:33 PM, Jani Nikula wrote:
> > Hi Mihai, could you test the following patch to see if it fixes the problem,
> > please?
> >
> > BR,
> > Jani.
> >
> >
> > Jani Nikula (1):
> >   drm/i915: ensure i2c adapter is all set before adding it
> >
> >  drivers/gpu/drm/i915/intel_i2c.c |    7 ++++---
> >  1 file changed, 4 insertions(+), 3 deletions(-)
> >
> 
> The reason sounds sane to me, but while looking through the code, I have seen a
> few other problems, too.
> 
> To my understanding, we should use port for dev_priv->gmbus[], not the pin
> mapping (which is only used for gmbus_ports[]).
> Don't forget to add the +1 for pin -> port mapping to the error case.
> 
> Also, intel_gmbus_get_adapter is already accepting a port value (I made sure to
> look at the calls in other files too), so don't map the port back to a pin.
> 
> Keep the same in mind for the intel_teardown_gmbus "destructor".
> 
> The current code adds the gmbus algorithm (gmbus_xfer) to gmbus port 0, which is
> known as "disabled" and shouldn't be used (previously has_gpio was set to false
> for those ports to not do any transfer on those ports.)
> 
> I may be wrong, could you review this and maybe add it to your patch?

This seems to essentially undo

commit 2ed06c93a1fce057808894d73167aae03c76deaf
Author: Daniel Kurtz <djkurtz@chromium.org>
Date:   Wed Mar 28 02:36:15 2012 +0800

    drm/i915/intel_i2c: gmbus disabled and reserved ports are invalid

Note that port numbers start at 1, whereas the array is 0-index based. So
you patch here would blow up if you don't extend the dev_priv->gmbus
array.

Yours, Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

Re: [PATCH 0/1] hopefully fix null pointer dereference on i915 load

[Mihai Moldovan]

[-- Attachment #1: Type: text/plain, Size: 2508 bytes --]

* On 13.08.2012 05:09 PM, Daniel Vetter wrote:
> On Mon, Aug 13, 2012 at 05:03:24PM +0200, Mihai Moldovan wrote:
>> Hi Jani,
>>
>> The reason sounds sane to me, but while looking through the code, I have seen a
>> few other problems, too.
>>
>> To my understanding, we should use port for dev_priv->gmbus[], not the pin
>> mapping (which is only used for gmbus_ports[]).
>> Don't forget to add the +1 for pin -> port mapping to the error case.
>>
>> Also, intel_gmbus_get_adapter is already accepting a port value (I made sure to
>> look at the calls in other files too), so don't map the port back to a pin.
>>
>> Keep the same in mind for the intel_teardown_gmbus "destructor".
>>
>> The current code adds the gmbus algorithm (gmbus_xfer) to gmbus port 0, which is
>> known as "disabled" and shouldn't be used (previously has_gpio was set to false
>> for those ports to not do any transfer on those ports.)
>>
>> I may be wrong, could you review this and maybe add it to your patch?
> This seems to essentially undo
>
> commit 2ed06c93a1fce057808894d73167aae03c76deaf
> Author: Daniel Kurtz <djkurtz@chromium.org>
> Date:   Wed Mar 28 02:36:15 2012 +0800
>
>     drm/i915/intel_i2c: gmbus disabled and reserved ports are invalid
>
> Note that port numbers start at 1, whereas the array is 0-index based. So
> you patch here would blow up if you don't extend the dev_priv->gmbus
> array.

Uhm, no, quite on the contrary. gmbus starts at 0 (with idx 0 being labeled
"disabled" and idx ((GMBUS_NUM_PORTS == 6) + 1) being labeled "reserved", which
neither should be touched).
Thus, in effect, it starts with 1 and ends with 6, but the current code does not
take that into account, instead accessing elements from 0 onwards:

The code currently would access *dev_priv->gmbus[0] in the first iteration,
which is labeled as "disabled" and shouldn't be touched. Instead, we should do a
pin->port mapping and access *dev_priv->gmbus[1, 2, 3 ... 6] instead (with
*dev_priv->gmbus[7] left out, as it's marked as "reserved" and again shouldn't
be touched.)

However, accessing gmbus_ports[0] is fine, and we can then copy
gmbus_ports[0].name to  *dev_priv->gmbus[1]->adapter.name
                     ^ pin                          
                                                 ^ port

Blowing up seems impossible too, as GMBUS_NUM_PORTS is #defined as END_PORT -
BEGIN_PORT + 1 which will evaluate to 6 and be the last index used.

Best regards,


Mihai


[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4506 bytes --]

Re: [PATCH 0/1] hopefully fix null pointer dereference on i915 load

[Mihai Moldovan]

[-- Attachment #1: Type: text/plain, Size: 1450 bytes --]

Had another look at the code and would like to apologize for the confusion...

* On 13.08.2012 05:27 PM, Mihai Moldovan wrote:
> Uhm, no, quite on the contrary. gmbus starts at 0 (with idx 0 being labeled
> "disabled" and idx ((GMBUS_NUM_PORTS == 6) + 1) being labeled "reserved", which
> neither should be touched).

Wrong.
struct intel_gmbus gmbus[GMBUS_NUM_PORTS];
thus starting at 0 to GMBUS_NUM_PORTS-1, no more reserved or disabled ports. I
have totally overlooked the definition, sorry.

Ignore the rest of my comments and the patch, as they are based on false
assumptions (gmbus still containing the disabled and reserved ports.)

Instead, I'd like to ACK Jani's patch. The module can now be loaded fine,
there's no null ptr dereference anymore and only some gmbus warnings show up,
though this time only one message per port, so basically it's falling back to
bit banging on all gmbus ports as it should:

[   14.722454] i915 0000:00:02.0: setting latency timer to 64
[   14.796032] [drm] GMBUS [i915 gmbus ssc] timed out, falling back to bit
banging on pin 1
[   15.044039] [drm] GMBUS [i915 gmbus panel] timed out, falling back to bit
banging on pin 3
[   15.420067] [drm] GMBUS [i915 gmbus dpd] timed out, falling back to bit
banging on pin 6
[   15.548121] i915 0000:00:02.0: irq 55 for MSI/MSI-X
[   15.842123] [drm] Initialized i915 1.6.0 20080730 for 0000:00:02.0 on minor 0

Best regards,


Mihai


[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4506 bytes --]

Re: [PATCH 1/1] drm/i915: ensure i2c adapter is all set before adding it

[Daniel Vetter]

On Mon, Aug 13, 2012 at 05:33:02PM +0300, Jani Nikula wrote:
> i2c_add_adapter() may do i2c transfers on the bus to detect supported
> devices. Therefore the adapter needs to be all set before adding it. This
> was not the case for the bit-banging fallback, resulting in an oops if the
> device detection GMBUS transfers timed out. Fix the issue by calling
> i2c_add_adapter() only after intel_gpio_setup().
> 
> LKML-Reference: <5021F00B.7000503@ionic.de>
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>

Applied to -fixes with Mihai's tested-by added, thanks for the patch.
-Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

Re: [PATCH 0/1] hopefully fix null pointer dereference on i915 load

[Jani Nikula]

On Mon, 13 Aug 2012, Mihai Moldovan <ionic@ionic.de> wrote:
> Had another look at the code and would like to apologize for the confusion...

No worries Mihai, thanks for testing!

BR,
Jani.