Bug 93675

Thanks for debugging this and finding a fix.

Be warned that the X server maintainers require that patches be submitted to 
xorg-devel mailing list for review before being applied:
http://wiki.x.org/wiki/Development/Documentation/SubmittingPatches/

Will I need to send the patch to the list myself, or is it automatic?

Sorry, it's not automatic.  You can either do it yourself or wait (possibly for 
years) for someone else to get around to it.   We're quite far backlogged.

(In reply to Kyle Guinn from comment #0)

Hi Kyle,

> Created attachment 120975 [details] [review]
> Check pointer before use
> 
> I received the following error when launching startx:
> 
> (EE) 
> (EE) Backtrace:
> (EE) 0: /usr/libexec/Xorg (xorg_backtrace+0x4a) [0x81fda6a]
> (EE) 1: /usr/libexec/Xorg (0x8048000+0x1b9e1e) [0x8201e1e]
> (EE) 2: linux-gate.so.1 (__kernel_rt_sigreturn+0x0) [0xb77adb50]
> (EE) 3: /usr/libexec/Xorg (xf86DestroyI2CDevRec+0x1b) [0x8115ecb]
> (EE) 4: /usr/lib/xorg/modules/drivers/openchrome_drv.so (0xb6dd8000+0x1ccd1)
> [0xb6df4cd1]
> (EE) 5: /usr/lib/xorg/modules/drivers/openchrome_drv.so (0xb6dd8000+0x1d1a0)
> [0xb6df51a0]
> (EE) 6: /usr/lib/xorg/modules/drivers/openchrome_drv.so (0xb6dd8000+0x1204d)
> [0xb6dea04d]
> (EE) 7: /usr/lib/xorg/modules/drivers/openchrome_drv.so (0xb6dd8000+0x155b7)
> [0xb6ded5b7]
> (EE) 8: /usr/libexec/Xorg (InitOutput+0xa8e) [0x80c639e]
> (EE) 9: /usr/libexec/Xorg (0x8048000+0x36393) [0x807e393]
> (EE) 10: /usr/libexec/Xorg (0x8048000+0x1e79e) [0x806679e]
> (EE) 11: /lib/libc.so.6 (__libc_start_main+0x107) [0xb7196287]
> (EE) 12: /usr/libexec/Xorg (0x8048000+0x1e7c4) [0x80667c4]
> (EE) 
> (EE) Segmentation fault at address 0x48
> (EE) 
> Fatal server error:
> (EE) Caught signal 11 (Segmentation fault). Server aborting
> (EE) 
> 
> I have traced it to xf86DestroyI2CDevRec.  This function is not checking
> that d->pI2CBus is valid before dereferencing it.  The latest openchrome
> from git is triggering this in via_dvi_init; it does a couple of probes to
> choose a value for pI2CBus, but if unsuccessful, calls xf86DestroyI2CDevRec
> without setting pI2CBus.
> 
> Patch is attached.

Which chipset are you using?
Is it P4M800 Pro, VN800, or CN700 chipset?
Does your system have a DVI connector (along with VT1632A TMDS transmitter for DVI functionality)?

Regards,

Kevin Brace

Hi Kyle,

I also recommend that you resubmit this bug report for xorg -> driver/openchrome at http://bugs.freedesktop.org.
I have known this bug for about a month, and I will like to fix it, but I still do not have Git repository access for OpenChrome.
That being said, I did request the repository access in the past hour or so.
I noticed this bug when I was modifying the code so that DVI can be used for systems other than P4M800 Pro, VN800, or CN700 chipset.

Regards,

Kevin Brace

You don't need to resubmit to move from Server/DDX/Xorg to Driver/openchrome,
just change the component in the menu, like this.

It is the CN700 chipset.  No DVI connector.

In particular, it's this board here:
http://www.jetwaycomputer.com/J7F4.html

(In reply to Kyle Guinn from comment #7)

Hi Kyle,

> It is the CN700 chipset.  No DVI connector.
> 
> In particular, it's this board here:
> http://www.jetwaycomputer.com/J7F4.html

I actually own that same board, but I have not really tested it with OpenChrome yet.
    There is a reason why you saw the crash you saw.
This is because the code will check "only for" P4M800 Pro, VN800, and CN700 chipsets when it goes into activating VT1632A external TMDS transmitter initialization code.

_____________________________________________________________________________
void
ViaOutputsDetect(ScrnInfoPtr pScrn)
{
    VIAPtr pVia = VIAPTR(pScrn);
    VIABIOSInfoPtr pBIOSInfo = pVia->pBIOSInfo;

    DEBUG(xf86DrvMsg(pScrn->scrnIndex, X_INFO, "ViaOutputsDetect\n"));

    pBIOSInfo->analog = NULL;

    /* LVDS */
    via_lvds_init(pScrn);

    /* VGA */
    via_analog_init(pScrn);

    /*
     * FIXME: xf86I2CProbeAddress(pVia->pI2CBus3, 0x40)
     * disables the panel on P4M900
     */
    /* TV encoder */
    if ((pVia->Chipset != VIA_P4M900) || (pVia->ActiveDevice & VIA_DEVICE_TV))
        via_tv_init(pScrn);

    if (pVia->Chipset == VIA_VM800) {
        via_dvi_init(pScrn);
    }

    if (pVia->ActiveDevice & VIA_DEVICE_DFP) {
        switch (pVia->Chipset) {
        case VIA_CX700:
        case VIA_VX800:
        case VIA_VX855:
        case VIA_VX900:
            via_dp_init(pScrn);
            break;
        }
    }
}
_____________________________________________________________________________

See the line that checks for VIA_VM800 (the PCI Device ID for P4M800 Pro, VN800, and CN700 integrated graphics).
If that line is true, then via_dvi_init function is called.
Here is the via_dvi_init function.
_____________________________________________________________________________
void
via_dvi_init(ScrnInfoPtr pScrn)
{
    VIAPtr pVia = VIAPTR(pScrn);
    xf86OutputPtr output = NULL;
    struct ViaVT1632PrivateData *private_data = NULL;
    I2CBusPtr pBus = NULL;
    I2CDevPtr pDev = NULL;

    if (!pVia->pI2CBus2 || !pVia->pI2CBus3) {
        return;
    }

    pDev = xf86CreateI2CDevRec();
    if (!pDev) {
        return;
    }

    pDev->DevName = "VT1632";
    pDev->SlaveAddr = 0x10;

    if (xf86I2CProbeAddress(pVia->pI2CBus3, pDev->SlaveAddr)) {
        pDev->pI2CBus = pVia->pI2CBus3;
    } else if (xf86I2CProbeAddress(pVia->pI2CBus2, pDev->SlaveAddr)) {
        pDev->pI2CBus = pVia->pI2CBus2;
    } else {
        xf86DestroyI2CDevRec(pDev, TRUE);
        return;
    }

    if (!xf86I2CDevInit(pDev)) {
        xf86DestroyI2CDevRec(pDev, TRUE);
        return;
    }

    if (!via_vt1632_probe(pScrn, pDev)) {
        xf86DestroyI2CDevRec(pDev, TRUE);
        return;
    }

    private_data = via_vt1632_init(pScrn, pDev);
    if (!private_data) {
        xf86DestroyI2CDevRec(pDev, TRUE);
        return;
    }

    output = xf86OutputCreate(pScrn, &via_dvi_funcs, "DVI-1");
    if (output) {
        output->driver_private = private_data;
        output->possible_crtcs = 0x2;
        output->possible_clones = 0;
        output->interlaceAllowed = FALSE;
        output->doubleScanAllowed = FALSE;
    }
}
_____________________________________________________________________________


If you take out xf86DestroyI2CDevRec API call after 2 calls to xf86I2CProbeAddress API call, the problem seems to go away (I am not sure if this is okay. If it is not, let me know.).

_____________________________________________________________________________

    if (xf86I2CProbeAddress(pVia->pI2CBus3, pDev->SlaveAddr)) {
        pDev->pI2CBus = pVia->pI2CBus3;
    } else if (xf86I2CProbeAddress(pVia->pI2CBus2, pDev->SlaveAddr)) {
        pDev->pI2CBus = pVia->pI2CBus2;
    } else {
//      xf86DestroyI2CDevRec(pDev, TRUE);
        return;
    }

_____________________________________________________________________________



This is the patch (fairly large patch) that added the VT1632A detection code in early 2015.
I think the problem with the code was that it was not really tested widely prior to being merged into the master branch

http://cgit.freedesktop.org/openchrome/xf86-video-openchrome/commit/?id=a8c2f04e2ef21e64f2e91dd6f3e237f80e8c80c6

The sole main developer seems to have disappeared from the project, and as a result, very little development activity has occurred in the last 12 months.
I am currently trying to assume the role of main developer in the near future, so that we can fix several bugs and do new commits.
    The reason why I know all of this is because I struggled to figure out why via_dvi_init function was crashing for a week in early January 2016.
Internally within OpenChrome, there is a large predefined display output table.
I was trying to reduce the use of this, but in the process, I was having issues with via_dvi_init function crashing.
VT1632A external TMDS transmitter is used by other VIA Technologies chipsets, so I did not like the way the current version of OpenChrome was limit the use with P4M800 Pro, VN800, and CN700 chipsets.
    Once I gain Git repository commit rights, I will definitely fix this bug in a new commit.
I hope the answers I gave you were satisfactory.

Regards,

Kevin Brace

Hi Kyle,

Just for your reference, P4M800 Pro, VN800, and CN700 integrated graphics all share the same PCI Device ID.

http://freedesktop.org/wiki/Openchrome/SupportedHardware/

This is why you saw the bug you saw (also see my previous post for more detailed analysis.).

Regards,

Kevin Brace

(In reply to Kevin Brace from comment #8)
> If you take out xf86DestroyI2CDevRec API call after 2 calls to
> xf86I2CProbeAddress API call, the problem seems to go away (I am not sure if
> this is okay. If it is not, let me know.).
> 
> _____________________________________________________________________________
> 
>     if (xf86I2CProbeAddress(pVia->pI2CBus3, pDev->SlaveAddr)) {
>         pDev->pI2CBus = pVia->pI2CBus3;
>     } else if (xf86I2CProbeAddress(pVia->pI2CBus2, pDev->SlaveAddr)) {
>         pDev->pI2CBus = pVia->pI2CBus2;
>     } else {
> //      xf86DestroyI2CDevRec(pDev, TRUE);
>         return;
>     }

Yes, that's the same offending line that I found.  Removing the xf86DestroyI2CDevRec call should fix it (haven't tried it myself) but it will result in a memory leak (Create without a matching Destroy).  That's why I decided to patch xf86DestroyI2CDevRec to handle this better.

(In reply to Kyle Guinn from comment #10)

Hi Kyle,

> > If you take out xf86DestroyI2CDevRec API call after 2 calls to
> > xf86I2CProbeAddress API call, the problem seems to go away (I am not sure if
> > this is okay. If it is not, let me know.).
> > 
> > _____________________________________________________________________________
> > 
> >     if (xf86I2CProbeAddress(pVia->pI2CBus3, pDev->SlaveAddr)) {
> >         pDev->pI2CBus = pVia->pI2CBus3;
> >     } else if (xf86I2CProbeAddress(pVia->pI2CBus2, pDev->SlaveAddr)) {
> >         pDev->pI2CBus = pVia->pI2CBus2;
> >     } else {
> > //      xf86DestroyI2CDevRec(pDev, TRUE);
> >         return;
> >     }
> 
> Yes, that's the same offending line that I found.  Removing the
> xf86DestroyI2CDevRec call should fix it (haven't tried it myself) but it
> will result in a memory leak (Create without a matching Destroy).  That's
> why I decided to patch xf86DestroyI2CDevRec to handle this better.

Do you have ideas on how this bug can be fixed without having to patch xorg (like what your patch does)?
Over at Bug 91966, I recently uploaded patches that will scan for the existence of VT1632A regardless whether or not it is actually hooked up.
As you saw from the source code snippets I showed, currently VT1632A is scanned only when P4M800 Pro and related chpsets are used. 
The patches I uploaded will let OpenChrome scan several I2C buses to figure out what is connected.
This became necessary because the bug reporter's system has a DVI-I connector (a DVI connector with DVI and VGA coming out, but shares the I2C bus between the two).
Unfortunately, he was having strange freezes with the second I2C bus (OpenChrome calls it I2C bus 2).
I am wondering if the instability the bug reporter reported is related to what you called "Create without a matching Destroy."

Regards,

Kevin Brace

(In reply to Kevin Brace from comment #11)
> Do you have ideas on how this bug can be fixed without having to patch xorg
> (like what your patch does)?

Yes, it should just be a matter of rearranging the code.  We have to do the Create only after one of the probes succeeded and we know which bus to use.  I'm attaching a patch of how it should look, but I have not yet tested it.


> Over at Bug 91966, I recently uploaded patches that will scan for the
> existence of VT1632A regardless whether or not it is actually hooked up.
> As you saw from the source code snippets I showed, currently VT1632A is
> scanned only when P4M800 Pro and related chpsets are used. 
> The patches I uploaded will let OpenChrome scan several I2C buses to figure
> out what is connected.
> This became necessary because the bug reporter's system has a DVI-I
> connector (a DVI connector with DVI and VGA coming out, but shares the I2C
> bus between the two).
> Unfortunately, he was having strange freezes with the second I2C bus
> (OpenChrome calls it I2C bus 2).
> I am wondering if the instability the bug reporter reported is related to
> what you called "Create without a matching Destroy."

Not likely, unless it's triggering the same segfault, but I wouldn't characterize a segfault as "freezing".  I haven't read through that bug yet.

The Create/Destroy pair is just a calloc/free pair.  If you omit the Destroy, then free isn't called and you have a memory leak.  By itself that shouldn't cause instability.  My guess is it's a completely separate issue and something's wrong with the scanning.

Created attachment 121481 [details] [review]
Move create after probe

Hi Kyle,

I looked at your patch, and made slight changes to it.
I tested the version of the patch I made changes to it, and the computer booted fine (Note: The computer I tested the patch does not have the VT1632A TMDS transmitter, in question.)
I am thinking of uploading the patch, but since you are the original author, how should I attribute the patch to you?
Should the patch look like this?

_______________________________________________________________
From: . . .
From: Kevin Brace <kevinbrace@gmx.com>
Date: Sat, 06 Feb 2016 00:24:01 -0600
Subject: [PATCH 1/1] Fixing via_dvi_init function

(Comments about the patch)

Signed-off-by: Kyle Guinn <Your e-mail address>
---
 src/via_outputs.c |   52 ++++++++++++++++++++++++++++++++++++++++++++--------
 1 file changed, X insertions(+), X deletions(-)

diff --git a/src/via_outputs.c b/src/via_outputs.c
. . .

_______________________________________________________________


If you do not like "Signed-off-by," let me know better words that note that you reported this bug originally, and wrote a patch to fix the bug.
Let me know what is the best way to handle this.

Regards,

Kevin Brace

(In reply to Kevin Brace from comment #14)
> Hi Kyle,
> 
> I looked at your patch, and made slight changes to it.
> I tested the version of the patch I made changes to it, and the computer
> booted fine (Note: The computer I tested the patch does not have the VT1632A
> TMDS transmitter, in question.)
> I am thinking of uploading the patch, but since you are the original author,
> how should I attribute the patch to you?

I did some quick reading and I think signed-off-by from both of us would make the most sense.  Can you attach your changes here?  I hate to sign off on something I haven't seen.

Created attachment 121584 [details] [review]
Fixing DVI detection code crash

The portion of the code responsible for detecting VT1632A external TMDS
transmitter for DVI was causing a crash if VT1632A was not detected on
I2C buses. This was happening due to xf86DestroyI2CDevRec being called
prior to the call to xf86I2CDevInit if VT1632A was not detected. Now the
code will probe I2C buses using xf86I2CProbeAddress to look for VT1632A,
and if VT1632A is not detected, it will stop trying to detect VT1632A
via I2C buses.

Signed-off-by: Kevin Brace <kevinbrace@gmx.com>

(In reply to Kyle Guinn from comment #15)
> 
> I did some quick reading and I think signed-off-by from both of us would
> make the most sense.  Can you attach your changes here?  I hate to sign off
> on something I haven't seen.

Hi Kyle,

I uploaded the patch.
I hope you are okay with it, but if there is something you do not like, let me know.
Obviously, "Signed-off-by: Kyle Guinn <Your e-mail address>" should go before my "Signed-off-by: . . ." line since you originally reported the bug and wrote the original patch.
I just made slight changes and wrote the comment section.
    Again, I tested the code, and at least it works with my laptop, but my laptop does not contain VT1632A.
That being said, I have a modified version of OpenChrome that will force the code to go look for VT1632A regardless of which chipset is in use.
That way, I did test the patch.

Regards,

Kevin Brace

Hi Kyle,

Just to let you know, I just obtained Git commit privilege for OpenChrome, and for the first time (ever), was able to push a patch into the OpenChrome repository.

https://cgit.freedesktop.org/openchrome/xf86-video-openchrome/commit/?id=35c6f0c1410247c4eba5fbc2e017561778c3c90d

What this means is that when you are ready, I can push the patch you originally wrote into the OpenChrome repository.

(In reply to Kevin Brace from comment #16)
> Created attachment 121584 [details] [review] [review]
> Fixing DVI detection code crash
> 
> The portion of the code responsible for detecting VT1632A external TMDS
> transmitter for DVI was causing a crash if VT1632A was not detected on
> I2C buses. This was happening due to xf86DestroyI2CDevRec being called
> prior to the call to xf86I2CDevInit if VT1632A was not detected. Now the
> code will probe I2C buses using xf86I2CProbeAddress to look for VT1632A,
> and if VT1632A is not detected, it will stop trying to detect VT1632A
> via I2C buses.
> 
> Signed-off-by: Kevin Brace <kevinbrace@gmx.com>

Looks good to me.  I tried it out here and it works.

Signed-off-by: Kyle Guinn <elyk03@gmail.com>

Fixed by commit 6964b889d77422ed16972b0ba849d9d5c6574daa.

https://cgit.freedesktop.org/openchrome/xf86-video-openchrome/commit/?id=6964b889d77422ed16972b0ba849d9d5c6574daa