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

[Mihai Moldovan <ionic@ionic.de>]

[-- 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 --]