[PATCH 0/1] apple-gmux: Assign apple_gmux_data before registering

[PATCH 0/1] apple-gmux: Assign apple_gmux_data before registering

[Lukas Wunner]

Hi Darren,

the following patch is a useful fix for apple-gmux by Matthew Garrett
which is well over a year old but unfortunately never got merged.

The commit message makes it sound as if the fix is only needed for
reprobing (in case apple-gmux registers after the DRM drivers).
I'm not yet sure if we'll use reprobing or deferred initialization,
however the patch is needed even if we go with deferred initialization
as it fixes a race condition that is triggered by invoking a handler
callback between the call to vga_switcheroo_register_handler() and the
assignment of apple_gmux_data.

The patch has seen extensive testing and is actively used by myself
and others on various MacBook Pro models.

Could you take a look at the patch and (barring any objections) ack it?

Thanks,

Lukas


Matthew Garrett (1):
  apple-gmux: Assign apple_gmux_data before registering

 drivers/platform/x86/apple-gmux.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

-- 
1.8.5.2 (Apple Git-48)

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 1/1] apple-gmux: Assign apple_gmux_data before registering

[Lukas Wunner]

From: Matthew Garrett <matthew.garrett@nebula.com>

Registering the handler after both GPUs will trigger a DDC switch for
connector reprobing. This will oops if apple_gmux_data hasn't already
been assigned. Reorder the code to do that.

Tested-by: Pierre Moreau <pierre.morrow@free.fr>
    [MBP  5,3 2009  nvidia MCP79 + G96        pre-retina  15"]
Tested-by: Paul Hordiienko <pvt.gord@gmail.com>
    [MBP  6,2 2010  intel ILK + nvidia GT216  pre-retina  15"]
Tested-by: Lukas Wunner <lukas@wunner.de>
    [MBP  9,1 2012  intel IVB + nvidia GK107  pre-retina  15"]
Tested-by: William Brown <william@blackhats.net.au>
    [MBP  8,2 2011  intel SNB + amd turks     pre-retina  15"]
Tested-by: Bruno Bierbaumer <bruno@bierbaumer.net>
    [MBP 11,3 2013  intel HSW + nvidia GK107  retina      15"]

Signed-off-by: Matthew Garrett <matthew.garrett@nebula.com>
Reviewed-by: Lukas Wunner <lukas@wunner.de>
Signed-off-by: Lukas Wunner <lukas@wunner.de>
---
 drivers/platform/x86/apple-gmux.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/platform/x86/apple-gmux.c b/drivers/platform/x86/apple-gmux.c
index 976efeb..aa58d41 100644
--- a/drivers/platform/x86/apple-gmux.c
+++ b/drivers/platform/x86/apple-gmux.c
@@ -588,18 +588,20 @@ static int gmux_probe(struct pnp_dev *pnp, const struct pnp_device_id *id)
 		gmux_data->gpe = -1;
 	}
 
+	apple_gmux_data = gmux_data;
+	init_completion(&gmux_data->powerchange_done);
+	gmux_enable_interrupts(gmux_data);
+
 	if (vga_switcheroo_register_handler(&gmux_handler)) {
 		ret = -ENODEV;
 		goto err_register_handler;
 	}
 
-	init_completion(&gmux_data->powerchange_done);
-	apple_gmux_data = gmux_data;
-	gmux_enable_interrupts(gmux_data);
-
 	return 0;
 
 err_register_handler:
+	gmux_disable_interrupts(gmux_data);
+	apple_gmux_data = NULL;
 	if (gmux_data->gpe >= 0)
 		acpi_disable_gpe(NULL, gmux_data->gpe);
 err_enable_gpe:
-- 
1.8.5.2 (Apple Git-48)

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/1] apple-gmux: Assign apple_gmux_data before registering

[Darren Hart]

On Mon, Nov 16, 2015 at 09:38:40PM +0100, Lukas Wunner wrote:
> From: Matthew Garrett <matthew.garrett@nebula.com>
> 
> Registering the handler after both GPUs will trigger a DDC switch for
> connector reprobing. This will oops if apple_gmux_data hasn't already
> been assigned. Reorder the code to do that.
> 
> [Lukas: More generally, this commit fixes a race condition that
> is triggered by invoking a handler callback between the call to
> vga_switcheroo_register_handler() and the assignment of
> apple_gmux_data.]
> 
> Tested-by: Pierre Moreau <pierre.morrow@free.fr>
>     [MBP  5,3 2009  nvidia MCP79 + G96        pre-retina  15"]
> Tested-by: Paul Hordiienko <pvt.gord@gmail.com>
>     [MBP  6,2 2010  intel ILK + nvidia GT216  pre-retina  15"]
> Tested-by: Lukas Wunner <lukas@wunner.de>
>     [MBP  9,1 2012  intel IVB + nvidia GK107  pre-retina  15"]
> Tested-by: William Brown <william@blackhats.net.au>
>     [MBP  8,2 2011  intel SNB + amd turks     pre-retina  15"]
> Tested-by: Bruno Bierbaumer <bruno@bierbaumer.net>
>     [MBP 11,3 2013  intel HSW + nvidia GK107  retina      15"]
> 
> Signed-off-by: Matthew Garrett <matthew.garrett@nebula.com>
> Reviewed-by: Lukas Wunner <lukas@wunner.de>
> Signed-off-by: Lukas Wunner <lukas@wunner.de>

My apologies for the delay. Thank you for the testing data and submitting.

I have queued this to testing. Pending success on 0-day, it will land in
linux-next shortly (tomorrow most likely) where I hope it will receive
additional testing.

-- 
Darren Hart
Intel Open Source Technology Center
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 0/1] apple-gmux: Assign apple_gmux_data before registering

[Darren Hart]

Hi Lukas,

Please send the patch to the list and maintainers reported by
get_maintainer.pl (specifically the platform-driver-x86 list and my
infradead ID) so we can have the review on list.

Thanks,

On 11/9/15 11:28 AM, Lukas Wunner wrote:
> Hi Darren,
> 
> the following patch is a useful fix for apple-gmux by Matthew Garrett
> which is well over a year old but unfortunately never got merged.
> 
> The commit message makes it sound as if the fix is only needed for
> reprobing (in case apple-gmux registers after the DRM drivers).
> I'm not yet sure if we'll use reprobing or deferred initialization,
> however the patch is needed even if we go with deferred initialization
> as it fixes a race condition that is triggered by invoking a handler
> callback between the call to vga_switcheroo_register_handler() and the
> assignment of apple_gmux_data.
> 
> The patch has seen extensive testing and is actively used by myself
> and others on various MacBook Pro models.
> 
> Could you take a look at the patch and (barring any objections) ack it?
> 
> Thanks,
> 
> Lukas
> 
> 
> Matthew Garrett (1):
>   apple-gmux: Assign apple_gmux_data before registering
> 
>  drivers/platform/x86/apple-gmux.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 

-- 
Darren Hart
Intel Open Source Technology Center
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 1/1] apple-gmux: Assign apple_gmux_data before registering

[Lukas Wunner]

From: Matthew Garrett <matthew.garrett@nebula.com>

Registering the handler after both GPUs will trigger a DDC switch for
connector reprobing. This will oops if apple_gmux_data hasn't already
been assigned. Reorder the code to do that.

[Lukas: More generally, this commit fixes a race condition that
is triggered by invoking a handler callback between the call to
vga_switcheroo_register_handler() and the assignment of
apple_gmux_data.]

Tested-by: Pierre Moreau <pierre.morrow@free.fr>
    [MBP  5,3 2009  nvidia MCP79 + G96        pre-retina  15"]
Tested-by: Paul Hordiienko <pvt.gord@gmail.com>
    [MBP  6,2 2010  intel ILK + nvidia GT216  pre-retina  15"]
Tested-by: Lukas Wunner <lukas@wunner.de>
    [MBP  9,1 2012  intel IVB + nvidia GK107  pre-retina  15"]
Tested-by: William Brown <william@blackhats.net.au>
    [MBP  8,2 2011  intel SNB + amd turks     pre-retina  15"]
Tested-by: Bruno Bierbaumer <bruno@bierbaumer.net>
    [MBP 11,3 2013  intel HSW + nvidia GK107  retina      15"]

Signed-off-by: Matthew Garrett <matthew.garrett@nebula.com>
Reviewed-by: Lukas Wunner <lukas@wunner.de>
Signed-off-by: Lukas Wunner <lukas@wunner.de>
---
 drivers/platform/x86/apple-gmux.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/platform/x86/apple-gmux.c b/drivers/platform/x86/apple-gmux.c
index 976efeb..aa58d41 100644
--- a/drivers/platform/x86/apple-gmux.c
+++ b/drivers/platform/x86/apple-gmux.c
@@ -588,18 +588,20 @@ static int gmux_probe(struct pnp_dev *pnp, const struct pnp_device_id *id)
 		gmux_data->gpe = -1;
 	}
 
+	apple_gmux_data = gmux_data;
+	init_completion(&gmux_data->powerchange_done);
+	gmux_enable_interrupts(gmux_data);
+
 	if (vga_switcheroo_register_handler(&gmux_handler)) {
 		ret = -ENODEV;
 		goto err_register_handler;
 	}
 
-	init_completion(&gmux_data->powerchange_done);
-	apple_gmux_data = gmux_data;
-	gmux_enable_interrupts(gmux_data);
-
 	return 0;
 
 err_register_handler:
+	gmux_disable_interrupts(gmux_data);
+	apple_gmux_data = NULL;
 	if (gmux_data->gpe >= 0)
 		acpi_disable_gpe(NULL, gmux_data->gpe);
 err_enable_gpe:
-- 
1.8.5.2 (Apple Git-48)

[PATCH 0/1] apple-gmux: Assign apple_gmux_data before registering

[Lukas Wunner]

Hi Darren,

as requested, here's a resend of this patch with cc: platform-driver-x86.

The patch is a useful fix for apple-gmux by Matthew Garrett. It is well
over a year old but unfortunately never got merged.

The commit message makes it sound as if the fix is only needed for
reprobing (in case apple-gmux registers after the DRM drivers).
I'm not yet sure if we'll use reprobing or deferred initialization,
however the patch is needed even if we go with deferred initialization
as it fixes a race condition that is triggered by invoking a handler
callback between the call to vga_switcheroo_register_handler() and the
assignment of apple_gmux_data. I've amended the commit message to make
that clearer.

The patch has seen extensive testing and is actively used by myself
and others on various MacBook Pro models.

Could you take a look at the patch and (barring any objections) ack it?

Thanks,

Lukas


Matthew Garrett (1):
  apple-gmux: Assign apple_gmux_data before registering

 drivers/platform/x86/apple-gmux.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

-- 
1.8.5.2 (Apple Git-48)