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

[Jani Nikula <jani.nikula@linux.intel.com>]

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