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

Mihai Moldovan
2012-08-13T15:03:24Z

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