null pointer dereference while loading i915

null pointer dereference while loading i915

[Mihai Moldovan]

[-- Attachment #1.1: Type: text/plain, Size: 1717 bytes --]

Hi Daniel, hi list

ever since version 3.2.0 (maybe even earlier, but 3.0.2 is still working fine),
my box is crashing when loading the i915 driver (mode-setting enabled.)

The current version I'm testing with is 3.5.0.

I was able to get the BUG output (please forgive any errors/flips in the output,
I have had to transcribe the messages from the screen/images), however, I'm not
able to find out what's wrong.

If I see it correctly, there's a null pointer dereference in a printk called
from inside gmbus_xfer. The only printk calls I can see in
drivers/gpu/drm/i915/intel_i2c.c gmbus_xfer() however are issued by the
DRM_DEBUG_KMS() and DRM_INFO() macros.
Neither call looks wrong to me, I even tried to swap adapter->name with
bus->adapter.name and make *sure* i < num is true, but haven't had any success.

I'd really like to see this bug fixed, as it's preventing me from updating the
kernel for over a year now.

Also, while 3.0.2 works, it *does* spew error/warning messages related to gmbus
and I've had corrupted VTs in the past (albeit after a long uptime with multiple
X restarting and DVI cable unplugging/reattaching events), so maybe there's a
lot more broken than "expected".

PCI-IDs:

00:02.0 VGA compatible controller [0300]: Intel Corporation 4 Series Chipset
Integrated Graphics Controller [8086:2e12] (rev 03) (prog-if 00 [VGA controller])
    Subsystem: Intel Corporation Device [8086:1003]
00:02.1 Display controller [0380]: Intel Corporation 4 Series Chipset Integrated
Graphics Controller [8086:2e13] (rev 03)
    Subsystem: Intel Corporation Device [8086:1003]

Messages are attached.

Any help is appreciated, thanks. :)

Best regards,


Mihai

[-- Attachment #1.2: i915_kernel_BUG_gmbus_nullptrderef.txt.bz2 --]
[-- Type: application/x-bzip2, Size: 1366 bytes --]

[-- Attachment #1.3: i915_3.0.2_warning_messages.txt.bz2 --]
[-- Type: application/x-bzip2, Size: 1141 bytes --]

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

Re: null pointer dereference while loading i915

[Daniel Vetter]

On Wed, Aug 8, 2012 at 6:50 AM, Mihai Moldovan <ionic@ionic.de> wrote:
> Hi Daniel, hi list
>
> ever since version 3.2.0 (maybe even earlier, but 3.0.2 is still working fine),
> my box is crashing when loading the i915 driver (mode-setting enabled.)
>
> The current version I'm testing with is 3.5.0.
>
> I was able to get the BUG output (please forgive any errors/flips in the output,
> I have had to transcribe the messages from the screen/images), however, I'm not
> able to find out what's wrong.
>
> If I see it correctly, there's a null pointer dereference in a printk called
> from inside gmbus_xfer. The only printk calls I can see in
> drivers/gpu/drm/i915/intel_i2c.c gmbus_xfer() however are issued by the
> DRM_DEBUG_KMS() and DRM_INFO() macros.
> Neither call looks wrong to me, I even tried to swap adapter->name with
> bus->adapter.name and make *sure* i < num is true, but haven't had any success.
>
> I'd really like to see this bug fixed, as it's preventing me from updating the
> kernel for over a year now.
>
> Also, while 3.0.2 works, it *does* spew error/warning messages related to gmbus
> and I've had corrupted VTs in the past (albeit after a long uptime with multiple
> X restarting and DVI cable unplugging/reattaching events), so maybe there's a
> lot more broken than "expected".

Hm, this is rather strange. gmbus should not be enable on 3.2 nor 3.0,
since exactly this issue might happen. We've re-enabled gmbus again on
3.5 after having fixed this bug. Are you sure that this is plain 3.2
you're running?

Yours, Daniel
-- 
Daniel Vetter
daniel.vetter@ffwll.ch - +41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: null pointer dereference while loading i915

[Mihai Moldovan]

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

* On 10.08.2012 12:10 PM, Daniel Vetter wrote:
> On Wed, Aug 8, 2012 at 6:50 AM, Mihai Moldovan <ionic@ionic.de> wrote:
>> Hi Daniel, hi list
>>
>> ever since version 3.2.0 (maybe even earlier, but 3.0.2 is still working fine),
>> my box is crashing when loading the i915 driver (mode-setting enabled.)
>>
>> The current version I'm testing with is 3.5.0.
>>
>> I was able to get the BUG output (please forgive any errors/flips in the output,
>> I have had to transcribe the messages from the screen/images), however, I'm not
>> able to find out what's wrong.
>>
>> If I see it correctly, there's a null pointer dereference in a printk called
>> from inside gmbus_xfer. The only printk calls I can see in
>> drivers/gpu/drm/i915/intel_i2c.c gmbus_xfer() however are issued by the
>> DRM_DEBUG_KMS() and DRM_INFO() macros.
>> Neither call looks wrong to me, I even tried to swap adapter->name with
>> bus->adapter.name and make *sure* i < num is true, but haven't had any success.
>>
>> I'd really like to see this bug fixed, as it's preventing me from updating the
>> kernel for over a year now.
>>
>> Also, while 3.0.2 works, it *does* spew error/warning messages related to gmbus
>> and I've had corrupted VTs in the past (albeit after a long uptime with multiple
>> X restarting and DVI cable unplugging/reattaching events), so maybe there's a
>> lot more broken than "expected".
>
> Hm, this is rather strange. gmbus should not be enable on 3.2 nor 3.0,
> since exactly this issue might happen. We've re-enabled gmbus again on
> 3.5 after having fixed this bug. Are you sure that this is plain 3.2
> you're running?

Sorry, I messed up the version numbers. Started bisecting yesterday and noticed,
that 3.0 up to 3.2 still work "fine" (see below), instead I've had another
problem with 3.2 (completely lockup after the kernel is running for a few
minutes, but I have no idea where this issue is coming from. Seems to be
happening with 3.2.0 only, so... *shrug*)

3.0.2           => working, gmbus warnings as posted.
3.1-09933/07170 => working, NO gmbus warnings, but render errors (see below)
3.2-rc2 to rc4  => working, NO gmbus warnings, but render errors (see below)
--- (stopped bisecting 3.0 to 3.2 as this was pointless) ---
--- (restarted bisecting with 3.2 to 3.5) ---
3.3.0-06109     => working, gmbus warnings just like with 3.0, render errors
(see below)
3.4.0-07487     => working, gmbus warnings, hang errors (see below)
...

I've done more steps, but have not yet finished bisecting, so stay tuned.
All those render errors look like that:

[drm] capturing error event; look for more information in
/debug/dri/0/i915_error_state
render error detected, EIR: 0x00000010
  IPEIR: 0x00000000
  IPEHR: 0x02000000
  INSTDONE: 0xffffffff
  INSTPS: 0x8001e025
  INSTDONE1: 0xbfbbffff
  ACTHD: 0x00a4203c
page table error
  PGTBL_ER: 0x00100000
[drm:i915_report_and_clear_eir] *ERROR* EIR stuck: 0x00000010, masking

I'll finish bisecting (and hope, that my guess was right, concerning the
varaiant I wasn't able to build) and will post the bisect log when done.

Meanwhile: at least for 3.0.2 and even older versions, gmbus must have been
enabled as I'm pretty sure I always saw those errors when booting (just
confirmed via logs for 3.0.0, 26.38.6, 2.6.39). Doesn't come up with 2.6.34,
2.6.36.1, 3.1-..., 3.2-... though.

Best regards,


Mihai



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

Re: null pointer dereference while loading i915

[Daniel Vetter]

On Fri, Aug 10, 2012 at 6:05 PM, Mihai Moldovan <ionic@ionic.de> wrote:
> * On 10.08.2012 12:10 PM, Daniel Vetter wrote:
>> On Wed, Aug 8, 2012 at 6:50 AM, Mihai Moldovan <ionic@ionic.de> wrote:
>>> Hi Daniel, hi list
>>>
>>> ever since version 3.2.0 (maybe even earlier, but 3.0.2 is still working fine),
>>> my box is crashing when loading the i915 driver (mode-setting enabled.)
>>>
>>> The current version I'm testing with is 3.5.0.
>>>
>>> I was able to get the BUG output (please forgive any errors/flips in the output,
>>> I have had to transcribe the messages from the screen/images), however, I'm not
>>> able to find out what's wrong.
>>>
>>> If I see it correctly, there's a null pointer dereference in a printk called
>>> from inside gmbus_xfer. The only printk calls I can see in
>>> drivers/gpu/drm/i915/intel_i2c.c gmbus_xfer() however are issued by the
>>> DRM_DEBUG_KMS() and DRM_INFO() macros.
>>> Neither call looks wrong to me, I even tried to swap adapter->name with
>>> bus->adapter.name and make *sure* i < num is true, but haven't had any success.
>>>
>>> I'd really like to see this bug fixed, as it's preventing me from updating the
>>> kernel for over a year now.
>>>
>>> Also, while 3.0.2 works, it *does* spew error/warning messages related to gmbus
>>> and I've had corrupted VTs in the past (albeit after a long uptime with multiple
>>> X restarting and DVI cable unplugging/reattaching events), so maybe there's a
>>> lot more broken than "expected".
>>
>> Hm, this is rather strange. gmbus should not be enable on 3.2 nor 3.0,
>> since exactly this issue might happen. We've re-enabled gmbus again on
>> 3.5 after having fixed this bug. Are you sure that this is plain 3.2
>> you're running?
>
> Sorry, I messed up the version numbers. Started bisecting yesterday and noticed,
> that 3.0 up to 3.2 still work "fine" (see below), instead I've had another
> problem with 3.2 (completely lockup after the kernel is running for a few
> minutes, but I have no idea where this issue is coming from. Seems to be
> happening with 3.2.0 only, so... *shrug*)
>
> 3.0.2           => working, gmbus warnings as posted.
> 3.1-09933/07170 => working, NO gmbus warnings, but render errors (see below)
> 3.2-rc2 to rc4  => working, NO gmbus warnings, but render errors (see below)
> --- (stopped bisecting 3.0 to 3.2 as this was pointless) ---
> --- (restarted bisecting with 3.2 to 3.5) ---
> 3.3.0-06109     => working, gmbus warnings just like with 3.0, render errors
> (see below)
> 3.4.0-07487     => working, gmbus warnings, hang errors (see below)
> ...
>
> I've done more steps, but have not yet finished bisecting, so stay tuned.
> All those render errors look like that:
>
> [drm] capturing error event; look for more information in
> /debug/dri/0/i915_error_state
> render error detected, EIR: 0x00000010
>   IPEIR: 0x00000000
>   IPEHR: 0x02000000
>   INSTDONE: 0xffffffff
>   INSTPS: 0x8001e025
>   INSTDONE1: 0xbfbbffff
>   ACTHD: 0x00a4203c
> page table error
>   PGTBL_ER: 0x00100000
> [drm:i915_report_and_clear_eir] *ERROR* EIR stuck: 0x00000010, masking
>
> I'll finish bisecting (and hope, that my guess was right, concerning the
> varaiant I wasn't able to build) and will post the bisect log when done.
>
> Meanwhile: at least for 3.0.2 and even older versions, gmbus must have been
> enabled as I'm pretty sure I always saw those errors when booting (just
> confirmed via logs for 3.0.0, 26.38.6, 2.6.39). Doesn't come up with 2.6.34,
> 2.6.36.1, 3.1-..., 3.2-... though.

Yeah, we've enabled gmbus a few times and then disabled it again due
to bugs. Also, the usual debug messsage says gmbus even when gmbus
isn't on ... yeah, slightly confusing, but that should be fixed, too.

For the gpu hang, please ensure that you're running the latest stable
release of everything (to avoid hunting down already known issues and
also because recent kernels dump more useful stuff), grab the entire
i915_error_state from debugfs and file a bug report with the usual
details at bugs.freedesktop.org against dri -> drm/intel.

Thanks,

Daniel
-- 
Daniel Vetter
daniel.vetter@ffwll.ch - +41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: null pointer dereference while loading i915

[Mihai Moldovan]

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

* On 10.08.2012 06:39 PM, Daniel Vetter wrote:
> On Fri, Aug 10, 2012 at 6:05 PM, Mihai Moldovan <ionic@ionic.de> wrote:
>> * On 10.08.2012 12:10 PM, Daniel Vetter wrote:
>>> On Wed, Aug 8, 2012 at 6:50 AM, Mihai Moldovan <ionic@ionic.de> wrote:
>>>> Hi Daniel, hi list
>>>>
>>>> ever since version 3.2.0 (maybe even earlier, but 3.0.2 is still working fine),
>>>> my box is crashing when loading the i915 driver (mode-setting enabled.)
>>>>
>>>> The current version I'm testing with is 3.5.0.
>>>>
>>>> I was able to get the BUG output (please forgive any errors/flips in the output,
>>>> I have had to transcribe the messages from the screen/images), however, I'm not
>>>> able to find out what's wrong.
>>>>
>>>> If I see it correctly, there's a null pointer dereference in a printk called
>>>> from inside gmbus_xfer. The only printk calls I can see in
>>>> drivers/gpu/drm/i915/intel_i2c.c gmbus_xfer() however are issued by the
>>>> DRM_DEBUG_KMS() and DRM_INFO() macros.
>>>> Neither call looks wrong to me, I even tried to swap adapter->name with
>>>> bus->adapter.name and make *sure* i < num is true, but haven't had any success.
>>>>
>>>> I'd really like to see this bug fixed, as it's preventing me from updating the
>>>> kernel for over a year now.
>>>>
>>>> Also, while 3.0.2 works, it *does* spew error/warning messages related to gmbus
>>>> and I've had corrupted VTs in the past (albeit after a long uptime with multiple
>>>> X restarting and DVI cable unplugging/reattaching events), so maybe there's a
>>>> lot more broken than "expected".
>>> Hm, this is rather strange. gmbus should not be enable on 3.2 nor 3.0,
>>> since exactly this issue might happen. We've re-enabled gmbus again on
>>> 3.5 after having fixed this bug. Are you sure that this is plain 3.2
>>> you're running?
>> Sorry, I messed up the version numbers. Started bisecting yesterday and noticed,
>> that 3.0 up to 3.2 still work "fine" (see below), instead I've had another
>> problem with 3.2 (completely lockup after the kernel is running for a few
>> minutes, but I have no idea where this issue is coming from. Seems to be
>> happening with 3.2.0 only, so... *shrug*)
>>
>> 3.0.2           => working, gmbus warnings as posted.
>> 3.1-09933/07170 => working, NO gmbus warnings, but render errors (see below)
>> 3.2-rc2 to rc4  => working, NO gmbus warnings, but render errors (see below)
>> --- (stopped bisecting 3.0 to 3.2 as this was pointless) ---
>> --- (restarted bisecting with 3.2 to 3.5) ---
>> 3.3.0-06109     => working, gmbus warnings just like with 3.0, render errors
>> (see below)
>> 3.4.0-07487     => working, gmbus warnings, hang errors (see below)
>> ...
>>
>> I've done more steps, but have not yet finished bisecting, so stay tuned.
>> All those render errors look like that:
>>
>> [drm] capturing error event; look for more information in
>> /debug/dri/0/i915_error_state
>> render error detected, EIR: 0x00000010
>>   IPEIR: 0x00000000
>>   IPEHR: 0x02000000
>>   INSTDONE: 0xffffffff
>>   INSTPS: 0x8001e025
>>   INSTDONE1: 0xbfbbffff
>>   ACTHD: 0x00a4203c
>> page table error
>>   PGTBL_ER: 0x00100000
>> [drm:i915_report_and_clear_eir] *ERROR* EIR stuck: 0x00000010, masking
>>
>> I'll finish bisecting (and hope, that my guess was right, concerning the
>> varaiant I wasn't able to build) and will post the bisect log when done.
>>
>> Meanwhile: at least for 3.0.2 and even older versions, gmbus must have been
>> enabled as I'm pretty sure I always saw those errors when booting (just
>> confirmed via logs for 3.0.0, 26.38.6, 2.6.39). Doesn't come up with 2.6.34,
>> 2.6.36.1, 3.1-..., 3.2-... though.
> Yeah, we've enabled gmbus a few times and then disabled it again due
> to bugs. Also, the usual debug messsage says gmbus even when gmbus
> isn't on ... yeah, slightly confusing, but that should be fixed, too.

Hm, OK.

Well, I'm done now.

bisect log:

git bisect start
# good: [805a6af8dba5dfdd35ec35dc52ec0122400b2610] Linux 3.2
git bisect good 805a6af8dba5dfdd35ec35dc52ec0122400b2610
# bad: [28a33cbc24e4256c143dce96c7d93bf423229f92] Linux 3.5
git bisect bad 28a33cbc24e4256c143dce96c7d93bf423229f92
# good: [49d99a2f9c4d033cc3965958a1397b1fad573dd3] Merge branch 'for-linus' of
git://oss.sgi.com/xfs/xfs
git bisect good 49d99a2f9c4d033cc3965958a1397b1fad573dd3
# good: [813a95e5b4fa936bbde10ef89188932745dcd7f4] Merge tag 'pinctrl' of
git://git.kernel.org/pub/scm/linux/kernel/git/arm/arm-soc
git bisect good 813a95e5b4fa936bbde10ef89188932745dcd7f4
# bad: [9978306e31a8f89bd81fbc4c49fd9aefb1d30d10] Merge branch 'for-linus' of
git://oss.sgi.com/xfs/xfs
git bisect bad 9978306e31a8f89bd81fbc4c49fd9aefb1d30d10
# good: [927ad551031798d4cba49766549600bbb33872d7] Merge tag
'ktest-v3.5-spelling' of
git://git.kernel.org/pub/scm/linux/kernel/git/rostedt/linux-ktest
git bisect good 927ad551031798d4cba49766549600bbb33872d7
# good: [2c01e7bc46f10e9190818437e564f7e0db875ae9] Merge branch 'for-linus' of
git://git.kernel.org/pub/scm/linux/kernel/git/dtor/input
git bisect good 2c01e7bc46f10e9190818437e564f7e0db875ae9
# bad: [5f54d29ee9dace1e2ef4e8c9873ad4dd7a06d11a] drm/nva3/pm: make pll->pll
mode work
git bisect bad 5f54d29ee9dace1e2ef4e8c9873ad4dd7a06d11a
# bad: [8b2e326dc7c5aa6952c88656d04d0d81fd85a6f8] drm/i915: Unconditionally
initialise the interrupt workers
git bisect bad 8b2e326dc7c5aa6952c88656d04d0d81fd85a6f8
# bad: [f637fde434c9e3687798730c7ddd367e93666013] drm/i915: inline
enable/disable_irq into ring->get/put_irq
git bisect bad f637fde434c9e3687798730c7ddd367e93666013
# bad: [23e3f9b37e7368ee8530ba99907508363feebc14] drm/i915: check for disabled
interrupts on ValleyView
git bisect bad 23e3f9b37e7368ee8530ba99907508363feebc14
# good: [8489731c9bd22c27ab17a2190cd7444604abf95f] drm/i915: move clflushing
into shmem_pread
git bisect good 8489731c9bd22c27ab17a2190cd7444604abf95f
# good: [3bd7d90938f1fe77de5991dc4b727843c4980b2a] drm/i915/intel_i2c: refactor
using intel_gmbus_get_adapter
git bisect good 3bd7d90938f1fe77de5991dc4b727843c4980b2a
# bad: [57f350b6722f9569f407872f6ead56e2d221d98a] drm/i915: add DPIO support
git bisect bad 57f350b6722f9569f407872f6ead56e2d221d98a
# bad: [93e537a10f2c8c0f2e74409b6cb473fc221758fa] drm/i915: split LVDS update
code out of i9xx_crtc_mode_set
git bisect bad 93e537a10f2c8c0f2e74409b6cb473fc221758fa
# bad: [f2c9677be3158c31ba19f527e2be0f7a519e19d1] drm/i915/intel_i2c: allocate
gmbus array as part of drm_i915_private
git bisect bad f2c9677be3158c31ba19f527e2be0f7a519e19d1
# bad: [2ed06c93a1fce057808894d73167aae03c76deaf] drm/i915/intel_i2c: gmbus
disabled and reserved ports are invalid
git bisect bad 2ed06c93a1fce057808894d73167aae03c76deaf


bisect result:

2ed06c93a1fce057808894d73167aae03c76deaf is the first bad commit
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
   
    There is no GMBUS "disabled" port 0, nor "reserved" port 7.
    For the other 6 ports there is a fixed 1:1 mapping between pin pairs and
    gmbus ports, which means every real gmbus port has a gpio pin.
   
    Given these realizations, clean up gmbus initialization.
   
    Tested on Sandybridge (gen 6, PCH == CougarPoint) hardware.
   
    Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>
    Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>

:040000 040000 cae5a0edaff96fc7f03eafaf8b2f1909032b0694
9244756995c6fe8b4e1e2ef5c916be1fe7ca4cbd M    drivers

I guess just reverting the commit on top of v3.5.0 won't work though.


> For the gpu hang, please ensure that you're running the latest stable
> release of everything (to avoid hunting down already known issues and
> also because recent kernels dump more useful stuff), grab the entire
> i915_error_state from debugfs and file a bug report with the usual
> details at bugs.freedesktop.org against dri -> drm/intel.

I should have a decently current userland (i.e., Intel Xorg module version 2.20.2).
Will do this once the kernel is working again. :)

Best regards,


Mihai


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

Re: null pointer dereference while loading i915

[Mihai Moldovan]

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

* On 10.08.2012 07:44 PM, Mihai Moldovan wrote:
> Hm, OK.
>
> Well, I'm done now.
>
> bisect log:
>
> git bisect start
> # good: [805a6af8dba5dfdd35ec35dc52ec0122400b2610] Linux 3.2
> git bisect good 805a6af8dba5dfdd35ec35dc52ec0122400b2610
> # bad: [28a33cbc24e4256c143dce96c7d93bf423229f92] Linux 3.5
> git bisect bad 28a33cbc24e4256c143dce96c7d93bf423229f92
> # good: [49d99a2f9c4d033cc3965958a1397b1fad573dd3] Merge branch 'for-linus' of
> git://oss.sgi.com/xfs/xfs
> git bisect good 49d99a2f9c4d033cc3965958a1397b1fad573dd3
> # good: [813a95e5b4fa936bbde10ef89188932745dcd7f4] Merge tag 'pinctrl' of
> git://git.kernel.org/pub/scm/linux/kernel/git/arm/arm-soc
> git bisect good 813a95e5b4fa936bbde10ef89188932745dcd7f4
> # bad: [9978306e31a8f89bd81fbc4c49fd9aefb1d30d10] Merge branch 'for-linus' of
> git://oss.sgi.com/xfs/xfs
> git bisect bad 9978306e31a8f89bd81fbc4c49fd9aefb1d30d10
> # good: [927ad551031798d4cba49766549600bbb33872d7] Merge tag
> 'ktest-v3.5-spelling' of
> git://git.kernel.org/pub/scm/linux/kernel/git/rostedt/linux-ktest
> git bisect good 927ad551031798d4cba49766549600bbb33872d7
> # good: [2c01e7bc46f10e9190818437e564f7e0db875ae9] Merge branch 'for-linus' of
> git://git.kernel.org/pub/scm/linux/kernel/git/dtor/input
> git bisect good 2c01e7bc46f10e9190818437e564f7e0db875ae9
> # bad: [5f54d29ee9dace1e2ef4e8c9873ad4dd7a06d11a] drm/nva3/pm: make pll->pll
> mode work
> git bisect bad 5f54d29ee9dace1e2ef4e8c9873ad4dd7a06d11a
> # bad: [8b2e326dc7c5aa6952c88656d04d0d81fd85a6f8] drm/i915: Unconditionally
> initialise the interrupt workers
> git bisect bad 8b2e326dc7c5aa6952c88656d04d0d81fd85a6f8
> # bad: [f637fde434c9e3687798730c7ddd367e93666013] drm/i915: inline
> enable/disable_irq into ring->get/put_irq
> git bisect bad f637fde434c9e3687798730c7ddd367e93666013
> # bad: [23e3f9b37e7368ee8530ba99907508363feebc14] drm/i915: check for disabled
> interrupts on ValleyView
> git bisect bad 23e3f9b37e7368ee8530ba99907508363feebc14
> # good: [8489731c9bd22c27ab17a2190cd7444604abf95f] drm/i915: move clflushing
> into shmem_pread
> git bisect good 8489731c9bd22c27ab17a2190cd7444604abf95f
> # good: [3bd7d90938f1fe77de5991dc4b727843c4980b2a] drm/i915/intel_i2c: refactor
> using intel_gmbus_get_adapter
> git bisect good 3bd7d90938f1fe77de5991dc4b727843c4980b2a
> # bad: [57f350b6722f9569f407872f6ead56e2d221d98a] drm/i915: add DPIO support
> git bisect bad 57f350b6722f9569f407872f6ead56e2d221d98a
> # bad: [93e537a10f2c8c0f2e74409b6cb473fc221758fa] drm/i915: split LVDS update
> code out of i9xx_crtc_mode_set
> git bisect bad 93e537a10f2c8c0f2e74409b6cb473fc221758fa
> # bad: [f2c9677be3158c31ba19f527e2be0f7a519e19d1] drm/i915/intel_i2c: allocate
> gmbus array as part of drm_i915_private
> git bisect bad f2c9677be3158c31ba19f527e2be0f7a519e19d1
> # bad: [2ed06c93a1fce057808894d73167aae03c76deaf] drm/i915/intel_i2c: gmbus
> disabled and reserved ports are invalid
> git bisect bad 2ed06c93a1fce057808894d73167aae03c76deaf

Just to be safe, I also tested git HEAD (3.6.0-rc1-00209-gf62bf17), no dice either.

Best regards,


Mihai


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

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

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