[PATCH] vgaswitcheroo: Fix error checking in vga_switcheroo_register_audio_client()

[PATCH] vgaswitcheroo: Fix error checking in vga_switcheroo_register_audio_client()

[Dan Carpenter]

The "id" variable is an enum and in this context it's treated as an
unsigned int so the error handling can never trigger.  The
->get_client_id() functions do not currently return any errors but
I imagine that if they did, then the intention was to return
VGA_SWITCHEROO_UNKNOWN_ID on error.  Let's check for both negatives
and UNKNOWN_ID so we'll catch it either way.

Reported-by: Su Hui <suhui@nfschina.com>
Closes: https://lore.kernel.org/all/20231026021056.850680-1-suhui@nfschina.com/
Fixes: 4aaf448fa975 ("vga_switcheroo: set audio client id according to bound GPU id")
Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
---
 drivers/gpu/vga/vga_switcheroo.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/vga/vga_switcheroo.c b/drivers/gpu/vga/vga_switcheroo.c
index 18f2c92beff8..216fb208eb31 100644
--- a/drivers/gpu/vga/vga_switcheroo.c
+++ b/drivers/gpu/vga/vga_switcheroo.c
@@ -375,7 +375,7 @@ int vga_switcheroo_register_audio_client(struct pci_dev *pdev,
 	mutex_lock(&vgasr_mutex);
 	if (vgasr_priv.active) {
 		id = vgasr_priv.handler->get_client_id(vga_dev);
-		if (id < 0) {
+		if ((int)id < 0 || id == VGA_SWITCHEROO_UNKNOWN_ID) {
 			mutex_unlock(&vgasr_mutex);
 			return -EINVAL;
 		}
-- 
2.47.2

Re: [PATCH] vgaswitcheroo: Fix error checking in vga_switcheroo_register_audio_client()

[Jani Nikula]

On Wed, 19 Feb 2025, Dan Carpenter <dan.carpenter@linaro.org> wrote:
> The "id" variable is an enum and in this context it's treated as an
> unsigned int so the error handling can never trigger.

When would that be true with GCC?

BR,
Jani.

> The
> ->get_client_id() functions do not currently return any errors but
> I imagine that if they did, then the intention was to return
> VGA_SWITCHEROO_UNKNOWN_ID on error.  Let's check for both negatives
> and UNKNOWN_ID so we'll catch it either way.
>
> Reported-by: Su Hui <suhui@nfschina.com>
> Closes: https://lore.kernel.org/all/20231026021056.850680-1-suhui@nfschina.com/
> Fixes: 4aaf448fa975 ("vga_switcheroo: set audio client id according to bound GPU id")
> Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
> ---
>  drivers/gpu/vga/vga_switcheroo.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/vga/vga_switcheroo.c b/drivers/gpu/vga/vga_switcheroo.c
> index 18f2c92beff8..216fb208eb31 100644
> --- a/drivers/gpu/vga/vga_switcheroo.c
> +++ b/drivers/gpu/vga/vga_switcheroo.c
> @@ -375,7 +375,7 @@ int vga_switcheroo_register_audio_client(struct pci_dev *pdev,
>  	mutex_lock(&vgasr_mutex);
>  	if (vgasr_priv.active) {
>  		id = vgasr_priv.handler->get_client_id(vga_dev);
> -		if (id < 0) {
> +		if ((int)id < 0 || id == VGA_SWITCHEROO_UNKNOWN_ID) {
>  			mutex_unlock(&vgasr_mutex);
>  			return -EINVAL;
>  		}

-- 
Jani Nikula, Intel

Re: [PATCH] vgaswitcheroo: Fix error checking in vga_switcheroo_register_audio_client()

[Dan Carpenter]

On Wed, Feb 19, 2025 at 05:17:56PM +0200, Jani Nikula wrote:
> On Wed, 19 Feb 2025, Dan Carpenter <dan.carpenter@linaro.org> wrote:
> > The "id" variable is an enum and in this context it's treated as an
> > unsigned int so the error handling can never trigger.
> 
> When would that be true with GCC?

The C standard give compilers a lot of flexibility with regards to enums.

But in terms of GCC/Clang then enums default to unsigned int, if you
declare one as negative then they become signed int.  If they don't fit
in int, then they become u64 etc.

enum u32_values {
	zero,
};

enum s32_values {
	minus_one = -1,
	zero,
};

enum u64_values {
	big = 0xfffffffffUL;
};

regards,
dan carpenter

Re: [PATCH] vgaswitcheroo: Fix error checking in vga_switcheroo_register_audio_client()

[Jani Nikula]

On Wed, 19 Feb 2025, Dan Carpenter <dan.carpenter@linaro.org> wrote:
> On Wed, Feb 19, 2025 at 05:17:56PM +0200, Jani Nikula wrote:
>> On Wed, 19 Feb 2025, Dan Carpenter <dan.carpenter@linaro.org> wrote:
>> > The "id" variable is an enum and in this context it's treated as an
>> > unsigned int so the error handling can never trigger.
>> 
>> When would that be true with GCC?
>
> The C standard give compilers a lot of flexibility with regards to enums.

This I did know.

> But in terms of GCC/Clang then enums default to unsigned int, if you
> declare one as negative then they become signed int.  If they don't fit
> in int, then they become u64 etc.

But somehow I'd failed to appreciate GCC/Clang actually do use unsigned
and signed on a case by case basis. I thought they defaulted to signed
int. TIL.

And I still consider myself a rather experienced C coder. There must be
something wrong with either C or me. Or possibly both.


Thanks,
Jani.

>
> enum u32_values {
> 	zero,
> };
>
> enum s32_values {
> 	minus_one = -1,
> 	zero,
> };
>
> enum u64_values {
> 	big = 0xfffffffffUL;
> };
>
> regards,
> dan carpenter
>

-- 
Jani Nikula, Intel

Re: [PATCH] vgaswitcheroo: Fix error checking in vga_switcheroo_register_audio_client()

[Jani Nikula]

On Wed, 19 Feb 2025, Dan Carpenter <dan.carpenter@linaro.org> wrote:
> The "id" variable is an enum and in this context it's treated as an
> unsigned int so the error handling can never trigger.  The
> ->get_client_id() functions do not currently return any errors but
> I imagine that if they did, then the intention was to return
> VGA_SWITCHEROO_UNKNOWN_ID on error.  Let's check for both negatives
> and UNKNOWN_ID so we'll catch it either way.
>
> Reported-by: Su Hui <suhui@nfschina.com>
> Closes: https://lore.kernel.org/all/20231026021056.850680-1-suhui@nfschina.com/
> Fixes: 4aaf448fa975 ("vga_switcheroo: set audio client id according to bound GPU id")
> Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
> ---
>  drivers/gpu/vga/vga_switcheroo.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/vga/vga_switcheroo.c b/drivers/gpu/vga/vga_switcheroo.c
> index 18f2c92beff8..216fb208eb31 100644
> --- a/drivers/gpu/vga/vga_switcheroo.c
> +++ b/drivers/gpu/vga/vga_switcheroo.c
> @@ -375,7 +375,7 @@ int vga_switcheroo_register_audio_client(struct pci_dev *pdev,
>  	mutex_lock(&vgasr_mutex);
>  	if (vgasr_priv.active) {
>  		id = vgasr_priv.handler->get_client_id(vga_dev);
> -		if (id < 0) {
> +		if ((int)id < 0 || id == VGA_SWITCHEROO_UNKNOWN_ID) {

Maybe we want to do something else here... see [1].

BR,
Jani.


[1] https://lore.kernel.org/r/CAHk-=wgg2A_iHNwf_JDjYJF=XHnKVGOjGp50FzVWniA2Z010bw@mail.gmail.com


>  			mutex_unlock(&vgasr_mutex);
>  			return -EINVAL;
>  		}

-- 
Jani Nikula, Intel

Re: [PATCH] vgaswitcheroo: Fix error checking in vga_switcheroo_register_audio_client()

[Dan Carpenter]

On Mon, Feb 24, 2025 at 03:14:33PM +0200, Jani Nikula wrote:
> On Wed, 19 Feb 2025, Dan Carpenter <dan.carpenter@linaro.org> wrote:
> > The "id" variable is an enum and in this context it's treated as an
> > unsigned int so the error handling can never trigger.  The
> > ->get_client_id() functions do not currently return any errors but
> > I imagine that if they did, then the intention was to return
> > VGA_SWITCHEROO_UNKNOWN_ID on error.  Let's check for both negatives
> > and UNKNOWN_ID so we'll catch it either way.
> >
> > Reported-by: Su Hui <suhui@nfschina.com>
> > Closes: https://lore.kernel.org/all/20231026021056.850680-1-suhui@nfschina.com/
> > Fixes: 4aaf448fa975 ("vga_switcheroo: set audio client id according to bound GPU id")
> > Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
> > ---
> >  drivers/gpu/vga/vga_switcheroo.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/vga/vga_switcheroo.c b/drivers/gpu/vga/vga_switcheroo.c
> > index 18f2c92beff8..216fb208eb31 100644
> > --- a/drivers/gpu/vga/vga_switcheroo.c
> > +++ b/drivers/gpu/vga/vga_switcheroo.c
> > @@ -375,7 +375,7 @@ int vga_switcheroo_register_audio_client(struct pci_dev *pdev,
> >  	mutex_lock(&vgasr_mutex);
> >  	if (vgasr_priv.active) {
> >  		id = vgasr_priv.handler->get_client_id(vga_dev);
> > -		if (id < 0) {
> > +		if ((int)id < 0 || id == VGA_SWITCHEROO_UNKNOWN_ID) {
> 
> Maybe we want to do something else here... see [1].
> 
> BR,
> Jani.
> 
> 
> [1] https://lore.kernel.org/r/CAHk-=wgg2A_iHNwf_JDjYJF=XHnKVGOjGp50FzVWniA2Z010bw@mail.gmail.com
> 

I feel like my patch is good enough...  I guess the concern here is that
GCC could change enums to something else.  I don't think that's likely as
it would break a lot of code.  The other option, which is a good option,
is to change the function signature for vgasr_priv.handler->get_client_id()
so that we do:

	ret = vgasr_priv.handler->get_client_id(vga_dev, &id);
	if (ret)
		return;

That's better code, honestly.  But I can't find motivation enough to do
the work.

regards,
dan carpenter

Re: [PATCH] vgaswitcheroo: Fix error checking in vga_switcheroo_register_audio_client()

[Jani Nikula]

On Mon, 24 Feb 2025, Dan Carpenter <dan.carpenter@linaro.org> wrote:
> On Mon, Feb 24, 2025 at 03:14:33PM +0200, Jani Nikula wrote:
>> On Wed, 19 Feb 2025, Dan Carpenter <dan.carpenter@linaro.org> wrote:
>> > The "id" variable is an enum and in this context it's treated as an
>> > unsigned int so the error handling can never trigger.  The
>> > ->get_client_id() functions do not currently return any errors but
>> > I imagine that if they did, then the intention was to return
>> > VGA_SWITCHEROO_UNKNOWN_ID on error.  Let's check for both negatives
>> > and UNKNOWN_ID so we'll catch it either way.
>> >
>> > Reported-by: Su Hui <suhui@nfschina.com>
>> > Closes: https://lore.kernel.org/all/20231026021056.850680-1-suhui@nfschina.com/
>> > Fixes: 4aaf448fa975 ("vga_switcheroo: set audio client id according to bound GPU id")
>> > Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
>> > ---
>> >  drivers/gpu/vga/vga_switcheroo.c | 2 +-
>> >  1 file changed, 1 insertion(+), 1 deletion(-)
>> >
>> > diff --git a/drivers/gpu/vga/vga_switcheroo.c b/drivers/gpu/vga/vga_switcheroo.c
>> > index 18f2c92beff8..216fb208eb31 100644
>> > --- a/drivers/gpu/vga/vga_switcheroo.c
>> > +++ b/drivers/gpu/vga/vga_switcheroo.c
>> > @@ -375,7 +375,7 @@ int vga_switcheroo_register_audio_client(struct pci_dev *pdev,
>> >  	mutex_lock(&vgasr_mutex);
>> >  	if (vgasr_priv.active) {
>> >  		id = vgasr_priv.handler->get_client_id(vga_dev);
>> > -		if (id < 0) {
>> > +		if ((int)id < 0 || id == VGA_SWITCHEROO_UNKNOWN_ID) {
>> 
>> Maybe we want to do something else here... see [1].
>> 
>> BR,
>> Jani.
>> 
>> 
>> [1] https://lore.kernel.org/r/CAHk-=wgg2A_iHNwf_JDjYJF=XHnKVGOjGp50FzVWniA2Z010bw@mail.gmail.com
>> 
>
> I feel like my patch is good enough...  I guess the concern here is that
> GCC could change enums to something else.  I don't think that's likely as
> it would break a lot of code.  The other option, which is a good option,
> is to change the function signature for vgasr_priv.handler->get_client_id()
> so that we do:
>
> 	ret = vgasr_priv.handler->get_client_id(vga_dev, &id);
> 	if (ret)
> 		return;
>
> That's better code, honestly.  But I can't find motivation enough to do
> the work.

The more I look at this, the more bonkers 4aaf448fa975 feels.

Anyway, I don't think ->get_client_id() hooks should return negative
error codes, and indeed none of them do. None of them return
VGA_SWITCHEROO_UNKNOWN_ID either, but that would be a valid return.

I suggest only checking for id == VGA_SWITCHEROO_UNKNOWN_ID. And doing
that in all the places that have that check, there are two more, but
they assign the return value to an int. So the int ret should be changed
to enum vga_switcheroo_unknown_id id I think.

Any chance of finding enough motivation to do that? ;)

BR,
Jani.



-- 
Jani Nikula, Intel

Re: [PATCH] vgaswitcheroo: Fix error checking in vga_switcheroo_register_audio_client()

[Takashi Iwai]

On Tue, 25 Feb 2025 10:10:29 +0100,
Jani Nikula wrote:
> 
> On Mon, 24 Feb 2025, Dan Carpenter <dan.carpenter@linaro.org> wrote:
> > On Mon, Feb 24, 2025 at 03:14:33PM +0200, Jani Nikula wrote:
> >> On Wed, 19 Feb 2025, Dan Carpenter <dan.carpenter@linaro.org> wrote:
> >> > The "id" variable is an enum and in this context it's treated as an
> >> > unsigned int so the error handling can never trigger.  The
> >> > ->get_client_id() functions do not currently return any errors but
> >> > I imagine that if they did, then the intention was to return
> >> > VGA_SWITCHEROO_UNKNOWN_ID on error.  Let's check for both negatives
> >> > and UNKNOWN_ID so we'll catch it either way.
> >> >
> >> > Reported-by: Su Hui <suhui@nfschina.com>
> >> > Closes: https://lore.kernel.org/all/20231026021056.850680-1-suhui@nfschina.com/
> >> > Fixes: 4aaf448fa975 ("vga_switcheroo: set audio client id according to bound GPU id")
> >> > Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
> >> > ---
> >> >  drivers/gpu/vga/vga_switcheroo.c | 2 +-
> >> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >> >
> >> > diff --git a/drivers/gpu/vga/vga_switcheroo.c b/drivers/gpu/vga/vga_switcheroo.c
> >> > index 18f2c92beff8..216fb208eb31 100644
> >> > --- a/drivers/gpu/vga/vga_switcheroo.c
> >> > +++ b/drivers/gpu/vga/vga_switcheroo.c
> >> > @@ -375,7 +375,7 @@ int vga_switcheroo_register_audio_client(struct pci_dev *pdev,
> >> >  	mutex_lock(&vgasr_mutex);
> >> >  	if (vgasr_priv.active) {
> >> >  		id = vgasr_priv.handler->get_client_id(vga_dev);
> >> > -		if (id < 0) {
> >> > +		if ((int)id < 0 || id == VGA_SWITCHEROO_UNKNOWN_ID) {
> >> 
> >> Maybe we want to do something else here... see [1].
> >> 
> >> BR,
> >> Jani.
> >> 
> >> 
> >> [1] https://lore.kernel.org/r/CAHk-=wgg2A_iHNwf_JDjYJF=XHnKVGOjGp50FzVWniA2Z010bw@mail.gmail.com
> >> 
> >
> > I feel like my patch is good enough...  I guess the concern here is that
> > GCC could change enums to something else.  I don't think that's likely as
> > it would break a lot of code.  The other option, which is a good option,
> > is to change the function signature for vgasr_priv.handler->get_client_id()
> > so that we do:
> >
> > 	ret = vgasr_priv.handler->get_client_id(vga_dev, &id);
> > 	if (ret)
> > 		return;
> >
> > That's better code, honestly.  But I can't find motivation enough to do
> > the work.
> 
> The more I look at this, the more bonkers 4aaf448fa975 feels.
> 
> Anyway, I don't think ->get_client_id() hooks should return negative
> error codes, and indeed none of them do. None of them return
> VGA_SWITCHEROO_UNKNOWN_ID either, but that would be a valid return.
> 
> I suggest only checking for id == VGA_SWITCHEROO_UNKNOWN_ID. And doing
> that in all the places that have that check, there are two more, but
> they assign the return value to an int. So the int ret should be changed
> to enum vga_switcheroo_unknown_id id I think.

+1, this sounds like the cleanest way.

> Any chance of finding enough motivation to do that? ;)

For whom? ;)


thanks,

Takashi

Re: [PATCH] vgaswitcheroo: Fix error checking in vga_switcheroo_register_audio_client()

[Dan Carpenter]

On Tue, Feb 25, 2025 at 11:10:29AM +0200, Jani Nikula wrote:
> On Mon, 24 Feb 2025, Dan Carpenter <dan.carpenter@linaro.org> wrote:
> > On Mon, Feb 24, 2025 at 03:14:33PM +0200, Jani Nikula wrote:
> >> On Wed, 19 Feb 2025, Dan Carpenter <dan.carpenter@linaro.org> wrote:
> >> > The "id" variable is an enum and in this context it's treated as an
> >> > unsigned int so the error handling can never trigger.  The
> >> > ->get_client_id() functions do not currently return any errors but
> >> > I imagine that if they did, then the intention was to return
> >> > VGA_SWITCHEROO_UNKNOWN_ID on error.  Let's check for both negatives
> >> > and UNKNOWN_ID so we'll catch it either way.
> >> >
> >> > Reported-by: Su Hui <suhui@nfschina.com>
> >> > Closes: https://lore.kernel.org/all/20231026021056.850680-1-suhui@nfschina.com/
> >> > Fixes: 4aaf448fa975 ("vga_switcheroo: set audio client id according to bound GPU id")
> >> > Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
> >> > ---
> >> >  drivers/gpu/vga/vga_switcheroo.c | 2 +-
> >> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >> >
> >> > diff --git a/drivers/gpu/vga/vga_switcheroo.c b/drivers/gpu/vga/vga_switcheroo.c
> >> > index 18f2c92beff8..216fb208eb31 100644
> >> > --- a/drivers/gpu/vga/vga_switcheroo.c
> >> > +++ b/drivers/gpu/vga/vga_switcheroo.c
> >> > @@ -375,7 +375,7 @@ int vga_switcheroo_register_audio_client(struct pci_dev *pdev,
> >> >  	mutex_lock(&vgasr_mutex);
> >> >  	if (vgasr_priv.active) {
> >> >  		id = vgasr_priv.handler->get_client_id(vga_dev);
> >> > -		if (id < 0) {
> >> > +		if ((int)id < 0 || id == VGA_SWITCHEROO_UNKNOWN_ID) {
> >> 
> >> Maybe we want to do something else here... see [1].
> >> 
> >> BR,
> >> Jani.
> >> 
> >> 
> >> [1] https://lore.kernel.org/r/CAHk-=wgg2A_iHNwf_JDjYJF=XHnKVGOjGp50FzVWniA2Z010bw@mail.gmail.com
> >> 
> >
> > I feel like my patch is good enough...  I guess the concern here is that
> > GCC could change enums to something else.  I don't think that's likely as
> > it would break a lot of code.  The other option, which is a good option,
> > is to change the function signature for vgasr_priv.handler->get_client_id()
> > so that we do:
> >
> > 	ret = vgasr_priv.handler->get_client_id(vga_dev, &id);
> > 	if (ret)
> > 		return;
> >
> > That's better code, honestly.  But I can't find motivation enough to do
> > the work.
> 
> The more I look at this, the more bonkers 4aaf448fa975 feels.
> 
> Anyway, I don't think ->get_client_id() hooks should return negative
> error codes, and indeed none of them do. None of them return
> VGA_SWITCHEROO_UNKNOWN_ID either, but that would be a valid return.
> 

Ugh...  Yeah.  The checks should all be the same obviously...


> I suggest only checking for id == VGA_SWITCHEROO_UNKNOWN_ID. And doing
> that in all the places that have that check, there are two more, but
> they assign the return value to an int. So the int ret should be changed
> to enum vga_switcheroo_unknown_id id I think.
> 

I mean the future is hard to predict but it's way more likely that
people will return negative error codes than that they'll return
VGA_SWITCHEROO_UNKNOWN_ID.  To be honest, I'd probably just apply
Su Hui's original patch at this point based on that's how that's what
all the callers essentially do.

https://lore.kernel.org/all/20231026021056.850680-1-suhui@nfschina.com/

Really returning VGA_SWITCHEROO_UNKNOWN_ID is a headache for the callers
because they have to test for it and then return -EINVAL.  It's
non-standard and more complicated than just checking for negative.

Changing it to return int is also slightly frustrating because everything
returns zero but that's more likely the best approach.

regards,
dan carpenter

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_atpx_handler.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_atpx_handler.c
index 3893e6fc2f03..494df3984c68 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_atpx_handler.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_atpx_handler.c
@@ -549,12 +549,14 @@ static int amdgpu_atpx_init(void)
  * look up whether we are the integrated or discrete GPU (all asics).
  * Returns the client id.
  */
-static enum vga_switcheroo_client_id amdgpu_atpx_get_client_id(struct pci_dev *pdev)
+static int amdgpu_atpx_get_client_id(struct pci_dev *pdev, enum vga_switcheroo_client_id *id)
 {
 	if (amdgpu_atpx_priv.dhandle == ACPI_HANDLE(&pdev->dev))
-		return VGA_SWITCHEROO_IGD;
+		*id = VGA_SWITCHEROO_IGD;
 	else
-		return VGA_SWITCHEROO_DIS;
+		*id = VGA_SWITCHEROO_DIS;
+
+	return 0;
 }
 
 static const struct vga_switcheroo_handler amdgpu_atpx_handler = {
diff --git a/drivers/gpu/drm/nouveau/nouveau_acpi.c b/drivers/gpu/drm/nouveau/nouveau_acpi.c
index 21b56cc7605c..72f3d02779ec 100644
--- a/drivers/gpu/drm/nouveau/nouveau_acpi.c
+++ b/drivers/gpu/drm/nouveau/nouveau_acpi.c
@@ -192,17 +192,22 @@ static int nouveau_dsm_power_state(enum vga_switcheroo_client_id id,
 	return nouveau_dsm_set_discrete_state(nouveau_dsm_priv.dhandle, state);
 }
 
-static enum vga_switcheroo_client_id nouveau_dsm_get_client_id(struct pci_dev *pdev)
+static int nouveau_dsm_get_client_id(struct pci_dev *pdev, enum vga_switcheroo_client_id *id)
 {
 	/* easy option one - intel vendor ID means Integrated */
-	if (pdev->vendor == PCI_VENDOR_ID_INTEL)
-		return VGA_SWITCHEROO_IGD;
+	if (pdev->vendor == PCI_VENDOR_ID_INTEL) {
+		*id = VGA_SWITCHEROO_IGD;
+		return 0;
+	}
 
 	/* is this device on Bus 0? - this may need improving */
-	if (pdev->bus->number == 0)
-		return VGA_SWITCHEROO_IGD;
+	if (pdev->bus->number == 0) {
+		*id = VGA_SWITCHEROO_IGD;
+		return 0;
+	}
 
-	return VGA_SWITCHEROO_DIS;
+	*id = VGA_SWITCHEROO_DIS;
+	return 0;
 }
 
 static const struct vga_switcheroo_handler nouveau_dsm_handler = {
diff --git a/drivers/gpu/drm/radeon/radeon_atpx_handler.c b/drivers/gpu/drm/radeon/radeon_atpx_handler.c
index f557535c1d7b..8dce19cd3d3a 100644
--- a/drivers/gpu/drm/radeon/radeon_atpx_handler.c
+++ b/drivers/gpu/drm/radeon/radeon_atpx_handler.c
@@ -529,12 +529,14 @@ static int radeon_atpx_init(void)
  * look up whether we are the integrated or discrete GPU (all asics).
  * Returns the client id.
  */
-static enum vga_switcheroo_client_id radeon_atpx_get_client_id(struct pci_dev *pdev)
+static int radeon_atpx_get_client_id(struct pci_dev *pdev, enum vga_switcheroo_client_id *id)
 {
 	if (radeon_atpx_priv.dhandle == ACPI_HANDLE(&pdev->dev))
-		return VGA_SWITCHEROO_IGD;
+		*id = VGA_SWITCHEROO_IGD;
 	else
-		return VGA_SWITCHEROO_DIS;
+		*id = VGA_SWITCHEROO_DIS;
+
+	return 0;
 }
 
 static const struct vga_switcheroo_handler radeon_atpx_handler = {
diff --git a/drivers/gpu/vga/vga_switcheroo.c b/drivers/gpu/vga/vga_switcheroo.c
index 18f2c92beff8..5ff222921d2f 100644
--- a/drivers/gpu/vga/vga_switcheroo.c
+++ b/drivers/gpu/vga/vga_switcheroo.c
@@ -187,6 +187,8 @@ static void vga_switcheroo_enable(void)
 {
 	int ret;
 	struct vga_switcheroo_client *client;
+	enum vga_switcheroo_client_id id;
+
 
 	/* call the handler to init */
 	if (vgasr_priv.handler->init)
@@ -197,11 +199,9 @@ static void vga_switcheroo_enable(void)
 		     client_id(client) != VGA_SWITCHEROO_UNKNOWN_ID)
 			continue;
 
-		ret = vgasr_priv.handler->get_client_id(client->pdev);
-		if (ret < 0)
+		ret = vgasr_priv.handler->get_client_id(client->pdev, &client->id);
+		if (ret)
 			return;
-
-		client->id = ret;
 	}
 
 	list_for_each_entry(client, &vgasr_priv.clients, list) {
@@ -209,13 +209,13 @@ static void vga_switcheroo_enable(void)
 		     client_id(client) != VGA_SWITCHEROO_UNKNOWN_ID)
 			continue;
 
-		ret = vgasr_priv.handler->get_client_id(client->vga_dev);
-		if (ret < 0)
+		ret = vgasr_priv.handler->get_client_id(client->vga_dev, &id);
+		if (ret)
 			return;
 
-		client->id = ret | ID_BIT_AUDIO;
+		client->id = id | ID_BIT_AUDIO;
 		if (client->ops->gpu_bound)
-			client->ops->gpu_bound(client->pdev, ret);
+			client->ops->gpu_bound(client->pdev, id);
 	}
 
 	vga_switcheroo_debugfs_init(&vgasr_priv);
@@ -364,6 +364,7 @@ int vga_switcheroo_register_audio_client(struct pci_dev *pdev,
 			struct pci_dev *vga_dev)
 {
 	enum vga_switcheroo_client_id id = VGA_SWITCHEROO_UNKNOWN_ID;
+	int ret;
 
 	/*
 	 * if vga_switcheroo has enabled, that mean two GPU clients and also
@@ -374,10 +375,10 @@ int vga_switcheroo_register_audio_client(struct pci_dev *pdev,
 	 */
 	mutex_lock(&vgasr_mutex);
 	if (vgasr_priv.active) {
-		id = vgasr_priv.handler->get_client_id(vga_dev);
-		if (id < 0) {
+		ret = vgasr_priv.handler->get_client_id(vga_dev, &id);
+		if (ret) {
 			mutex_unlock(&vgasr_mutex);
-			return -EINVAL;
+			return ret;
 		}
 		/* notify if GPU has been already bound */
 		if (ops->gpu_bound)
@@ -559,6 +560,7 @@ EXPORT_SYMBOL(vga_switcheroo_client_fb_set);
 int vga_switcheroo_lock_ddc(struct pci_dev *pdev)
 {
 	enum vga_switcheroo_client_id id;
+	int ret;
 
 	mutex_lock(&vgasr_priv.mux_hw_lock);
 	if (!vgasr_priv.handler || !vgasr_priv.handler->switch_ddc) {
@@ -566,7 +568,9 @@ int vga_switcheroo_lock_ddc(struct pci_dev *pdev)
 		return -ENODEV;
 	}
 
-	id = vgasr_priv.handler->get_client_id(pdev);
+	ret = vgasr_priv.handler->get_client_id(pdev, &id);
+	if (ret)
+		return ret;
 	vgasr_priv.old_ddc_owner = vgasr_priv.handler->switch_ddc(id);
 	return vgasr_priv.old_ddc_owner;
 }
@@ -597,11 +601,14 @@ int vga_switcheroo_unlock_ddc(struct pci_dev *pdev)
 		return -EINVAL;
 
 	if (vgasr_priv.old_ddc_owner >= 0) {
-		id = vgasr_priv.handler->get_client_id(pdev);
+		ret = vgasr_priv.handler->get_client_id(pdev, &id);
+		if (ret)
+			goto unlock;
 		if (vgasr_priv.old_ddc_owner != id)
 			ret = vgasr_priv.handler->switch_ddc(
 						     vgasr_priv.old_ddc_owner);
 	}
+unlock:
 	mutex_unlock(&vgasr_priv.mux_hw_lock);
 	return ret;
 }
diff --git a/drivers/platform/x86/apple-gmux.c b/drivers/platform/x86/apple-gmux.c
index 1417e230edbd..4f10e8c81a30 100644
--- a/drivers/platform/x86/apple-gmux.c
+++ b/drivers/platform/x86/apple-gmux.c
@@ -541,19 +541,21 @@ static int gmux_set_power_state(enum vga_switcheroo_client_id id,
 	return gmux_set_discrete_state(apple_gmux_data, state);
 }
 
-static enum vga_switcheroo_client_id gmux_get_client_id(struct pci_dev *pdev)
+static int gmux_get_client_id(struct pci_dev *pdev, enum vga_switcheroo_client_id *id)
 {
 	/*
 	 * Early Macbook Pros with switchable graphics use nvidia
 	 * integrated graphics. Hardcode that the 9400M is integrated.
 	 */
 	if (pdev->vendor == PCI_VENDOR_ID_INTEL)
-		return VGA_SWITCHEROO_IGD;
+		*id = VGA_SWITCHEROO_IGD;
 	else if (pdev->vendor == PCI_VENDOR_ID_NVIDIA &&
 		 pdev->device == 0x0863)
-		return VGA_SWITCHEROO_IGD;
+		*id = VGA_SWITCHEROO_IGD;
 	else
-		return VGA_SWITCHEROO_DIS;
+		*id = VGA_SWITCHEROO_DIS;
+
+	return 0;
 }
 
 static const struct vga_switcheroo_handler gmux_handler_no_ddc = {
diff --git a/include/linux/vga_switcheroo.h b/include/linux/vga_switcheroo.h
index 7e6ac0114d55..cd3167ba2d02 100644
--- a/include/linux/vga_switcheroo.h
+++ b/include/linux/vga_switcheroo.h
@@ -119,7 +119,7 @@ struct vga_switcheroo_handler {
 	int (*switch_ddc)(enum vga_switcheroo_client_id id);
 	int (*power_state)(enum vga_switcheroo_client_id id,
 			   enum vga_switcheroo_state state);
-	enum vga_switcheroo_client_id (*get_client_id)(struct pci_dev *pdev);
+	int (*get_client_id)(struct pci_dev *pdev, enum vga_switcheroo_client_id *id);
 };
 
 /**