Re: [PATCH 0/1] hopefully fix null pointer dereference on i915 load
On Mon, 13 Aug 2012, Mihai Moldovan wrote: > Had another look at the code and would like to apologize for the confusion... No worries Mihai, thanks for testing! BR, Jani. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 0/1] hopefully fix null pointer dereference on i915 load
On Mon, 13 Aug 2012, Mihai Moldovan io...@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. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 0/1] hopefully fix null pointer dereference on i915 load
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 :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 :00:02.0: irq 55 for MSI/MSI-X [ 15.842123] [drm] Initialized i915 1.6.0 20080730 for :00:02.0 on minor 0 Best regards, Mihai smime.p7s Description: S/MIME Cryptographic Signature
Re: [PATCH 0/1] hopefully fix null pointer dereference on i915 load
* 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 > 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 smime.p7s Description: S/MIME Cryptographic Signature
Re: [PATCH 0/1] hopefully fix null pointer dereference on i915 load
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 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: dan...@ffwll.ch Mobile: +41 (0)79 365 57 48 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 0/1] hopefully fix null pointer dereference on i915 load
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(_priv->gmbus_mutex);
for (i = 0; i < GMBUS_NUM_PORTS; i++) {
- struct intel_gmbus *bus = _priv->gmbus[i];
u32 port = i + 1; /* +1 to map gmbus index to pin pair */
+ struct intel_gmbus *bus = _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 = _priv->gmbus[i];
+ struct intel_gmbus *bus = _priv->gmbus[i + 1];
i2c_del_adapter(>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)) ?
- _priv->gmbus[port - 1].adapter : NULL;
+ _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 = _priv->gmbus[i];
+ struct intel_gmbus *bus = _priv->gmbus[i + 1];
i2c_del_adapter(>adapter);
}
}
smime.p7s
Description: S/MIME Cryptographic Signature
[PATCH 0/1] hopefully fix null pointer dereference on i915 load
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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH 0/1] hopefully fix null pointer dereference on i915 load
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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 0/1] hopefully fix null pointer dereference on i915 load
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);
}
}
smime.p7s
Description: S/MIME Cryptographic Signature
Re: [PATCH 0/1] hopefully fix null pointer dereference on i915 load
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 djku...@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: dan...@ffwll.ch Mobile: +41 (0)79 365 57 48 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 0/1] hopefully fix null pointer dereference on i915 load
* 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 djku...@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 smime.p7s Description: S/MIME Cryptographic Signature
Re: [PATCH 0/1] hopefully fix null pointer dereference on i915 load
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 :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 :00:02.0: irq 55 for MSI/MSI-X [ 15.842123] [drm] Initialized i915 1.6.0 20080730 for :00:02.0 on minor 0 Best regards, Mihai smime.p7s Description: S/MIME Cryptographic Signature
