diff mbox series

ALSA: hda/i915: Fix one too many pci_dev_put()

Message ID 20220416064418.2364582-1-lucas.demarchi@intel.com
State Accepted
Commit 00fd7cfad0548b6b7234c93370076f9b9c2e39f8
Headers show
Series ALSA: hda/i915: Fix one too many pci_dev_put() | expand

Commit Message

Lucas De Marchi April 16, 2022, 6:44 a.m. UTC
pci_get_class() will already unref the pci device passed as argument.
So if it's unconditionally unref'ed, even if the loop is not stopped,
there will be one too many unref for each device not matched.

Cc: Kai Vehmanen <kai.vehmanen@linux.intel.com>
Cc: Takashi Iwai <tiwai@suse.de>
Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/5701
Fixes: 0dc2696a4623 ("ALSA: hda/i915 - skip acomp init if no matching display")
Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
---
 sound/hda/hdac_i915.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

Comments

Lucas De Marchi April 18, 2022, 4:50 a.m. UTC | #1
On Sun, Apr 17, 2022 at 01:13:49PM +0300, Kai Vehmanen wrote:
>Hi,
>
>On Fri, 15 Apr 2022, Lucas De Marchi wrote:
>
>> pci_get_class() will already unref the pci device passed as argument.
>> So if it's unconditionally unref'ed, even if the loop is not stopped,
>
>thanks Lucas. And yes indeed, overlooked that pci_get_class()
>will decrement the from device is specified.
>
>> --- a/sound/hda/hdac_i915.c
>> +++ b/sound/hda/hdac_i915.c
>> @@ -127,11 +127,10 @@ static int i915_gfx_present(struct pci_dev *hdac_pci)
>>  		display_dev = pci_get_class(class, display_dev);
>>
>>  		if (display_dev && display_dev->vendor == PCI_VENDOR_ID_INTEL &&
>> -		    connectivity_check(display_dev, hdac_pci))
>> +		    connectivity_check(display_dev, hdac_pci)) {
>> +			pci_dev_put(display_dev);
>>  			match = true;
>> -
>> -		pci_dev_put(display_dev);
>> -
>> +		}
>
>Reviewed-by: Kai Vehmanen <kai.vehmanen@linux.intel.com>

I applied this to our topic/core-for-CI branch to unblock CI on
DG2. Ultimately the target for this is the sound tree though.

thanks
Lucas De Marchi


>
>Br, Kai
Takashi Iwai April 19, 2022, 5:54 a.m. UTC | #2
On Mon, 18 Apr 2022 06:50:32 +0200,
Lucas De Marchi wrote:
> 
> On Sun, Apr 17, 2022 at 01:13:49PM +0300, Kai Vehmanen wrote:
> >Hi,
> >
> >On Fri, 15 Apr 2022, Lucas De Marchi wrote:
> >
> >> pci_get_class() will already unref the pci device passed as argument.
> >> So if it's unconditionally unref'ed, even if the loop is not stopped,
> >
> >thanks Lucas. And yes indeed, overlooked that pci_get_class()
> >will decrement the from device is specified.
> >
> >> --- a/sound/hda/hdac_i915.c
> >> +++ b/sound/hda/hdac_i915.c
> >> @@ -127,11 +127,10 @@ static int i915_gfx_present(struct pci_dev *hdac_pci)
> >>  		display_dev = pci_get_class(class, display_dev);
> >>
> >>  		if (display_dev && display_dev->vendor == PCI_VENDOR_ID_INTEL &&
> >> -		    connectivity_check(display_dev, hdac_pci))
> >> +		    connectivity_check(display_dev, hdac_pci)) {
> >> +			pci_dev_put(display_dev);
> >>  			match = true;
> >> -
> >> -		pci_dev_put(display_dev);
> >> -
> >> +		}
> >
> >Reviewed-by: Kai Vehmanen <kai.vehmanen@linux.intel.com>
> 
> I applied this to our topic/core-for-CI branch to unblock CI on
> DG2. Ultimately the target for this is the sound tree though.

The patch looks good, feel free to submit it.


thanks,

Takashi
Lucas De Marchi April 19, 2022, 6:26 a.m. UTC | #3
On Tue, Apr 19, 2022 at 07:54:30AM +0200, Takashi Iwai wrote:
>On Mon, 18 Apr 2022 06:50:32 +0200,
>Lucas De Marchi wrote:
>>
>> On Sun, Apr 17, 2022 at 01:13:49PM +0300, Kai Vehmanen wrote:
>> >Hi,
>> >
>> >On Fri, 15 Apr 2022, Lucas De Marchi wrote:
>> >
>> >> pci_get_class() will already unref the pci device passed as argument.
>> >> So if it's unconditionally unref'ed, even if the loop is not stopped,
>> >
>> >thanks Lucas. And yes indeed, overlooked that pci_get_class()
>> >will decrement the from device is specified.
>> >
>> >> --- a/sound/hda/hdac_i915.c
>> >> +++ b/sound/hda/hdac_i915.c
>> >> @@ -127,11 +127,10 @@ static int i915_gfx_present(struct pci_dev *hdac_pci)
>> >>  		display_dev = pci_get_class(class, display_dev);
>> >>
>> >>  		if (display_dev && display_dev->vendor == PCI_VENDOR_ID_INTEL &&
>> >> -		    connectivity_check(display_dev, hdac_pci))
>> >> +		    connectivity_check(display_dev, hdac_pci)) {
>> >> +			pci_dev_put(display_dev);
>> >>  			match = true;
>> >> -
>> >> -		pci_dev_put(display_dev);
>> >> -
>> >> +		}
>> >
>> >Reviewed-by: Kai Vehmanen <kai.vehmanen@linux.intel.com>
>>
>> I applied this to our topic/core-for-CI branch to unblock CI on
>> DG2. Ultimately the target for this is the sound tree though.
>
>The patch looks good, feel free to submit it.

not sure if I was clear. This patch is already targeting the sound tree:
it should apply cleanly.

topic/core-for-CI in the drm subsystem is a "temporary branch" while we
don't backmerge with the sound changes.

Lucas De Marchi

>
>
>thanks,
>
>Takashi
Takashi Iwai April 19, 2022, 6:40 a.m. UTC | #4
On Tue, 19 Apr 2022 08:26:06 +0200,
Lucas De Marchi wrote:
> 
> On Tue, Apr 19, 2022 at 07:54:30AM +0200, Takashi Iwai wrote:
> >On Mon, 18 Apr 2022 06:50:32 +0200,
> >Lucas De Marchi wrote:
> >>
> >> On Sun, Apr 17, 2022 at 01:13:49PM +0300, Kai Vehmanen wrote:
> >> >Hi,
> >> >
> >> >On Fri, 15 Apr 2022, Lucas De Marchi wrote:
> >> >
> >> >> pci_get_class() will already unref the pci device passed as argument.
> >> >> So if it's unconditionally unref'ed, even if the loop is not stopped,
> >> >
> >> >thanks Lucas. And yes indeed, overlooked that pci_get_class()
> >> >will decrement the from device is specified.
> >> >
> >> >> --- a/sound/hda/hdac_i915.c
> >> >> +++ b/sound/hda/hdac_i915.c
> >> >> @@ -127,11 +127,10 @@ static int i915_gfx_present(struct pci_dev *hdac_pci)
> >> >>  		display_dev = pci_get_class(class, display_dev);
> >> >>
> >> >>  		if (display_dev && display_dev->vendor == PCI_VENDOR_ID_INTEL &&
> >> >> -		    connectivity_check(display_dev, hdac_pci))
> >> >> +		    connectivity_check(display_dev, hdac_pci)) {
> >> >> +			pci_dev_put(display_dev);
> >> >>  			match = true;
> >> >> -
> >> >> -		pci_dev_put(display_dev);
> >> >> -
> >> >> +		}
> >> >
> >> >Reviewed-by: Kai Vehmanen <kai.vehmanen@linux.intel.com>
> >>
> >> I applied this to our topic/core-for-CI branch to unblock CI on
> >> DG2. Ultimately the target for this is the sound tree though.
> >
> >The patch looks good, feel free to submit it.
> 
> not sure if I was clear. This patch is already targeting the sound tree:
> it should apply cleanly.

The original patch hasn't reached to me (we've had the mail server
problem in the last weekend, and that might be the reason).

Could you resubmit?


thanks,

Takashi
Takashi Iwai April 19, 2022, 6:56 a.m. UTC | #5
On Tue, 19 Apr 2022 08:40:01 +0200,
Takashi Iwai wrote:
> 
> On Tue, 19 Apr 2022 08:26:06 +0200,
> Lucas De Marchi wrote:
> > 
> > On Tue, Apr 19, 2022 at 07:54:30AM +0200, Takashi Iwai wrote:
> > >On Mon, 18 Apr 2022 06:50:32 +0200,
> > >Lucas De Marchi wrote:
> > >>
> > >> On Sun, Apr 17, 2022 at 01:13:49PM +0300, Kai Vehmanen wrote:
> > >> >Hi,
> > >> >
> > >> >On Fri, 15 Apr 2022, Lucas De Marchi wrote:
> > >> >
> > >> >> pci_get_class() will already unref the pci device passed as argument.
> > >> >> So if it's unconditionally unref'ed, even if the loop is not stopped,
> > >> >
> > >> >thanks Lucas. And yes indeed, overlooked that pci_get_class()
> > >> >will decrement the from device is specified.
> > >> >
> > >> >> --- a/sound/hda/hdac_i915.c
> > >> >> +++ b/sound/hda/hdac_i915.c
> > >> >> @@ -127,11 +127,10 @@ static int i915_gfx_present(struct pci_dev *hdac_pci)
> > >> >>  		display_dev = pci_get_class(class, display_dev);
> > >> >>
> > >> >>  		if (display_dev && display_dev->vendor == PCI_VENDOR_ID_INTEL &&
> > >> >> -		    connectivity_check(display_dev, hdac_pci))
> > >> >> +		    connectivity_check(display_dev, hdac_pci)) {
> > >> >> +			pci_dev_put(display_dev);
> > >> >>  			match = true;
> > >> >> -
> > >> >> -		pci_dev_put(display_dev);
> > >> >> -
> > >> >> +		}
> > >> >
> > >> >Reviewed-by: Kai Vehmanen <kai.vehmanen@linux.intel.com>
> > >>
> > >> I applied this to our topic/core-for-CI branch to unblock CI on
> > >> DG2. Ultimately the target for this is the sound tree though.
> > >
> > >The patch looks good, feel free to submit it.
> > 
> > not sure if I was clear. This patch is already targeting the sound tree:
> > it should apply cleanly.
> 
> The original patch hasn't reached to me (we've had the mail server
> problem in the last weekend, and that might be the reason).
> 
> Could you resubmit?

Never mind, I could find it in lore.
  https://lore.kernel.org/all/20220416064418.2364582-1-lucas.demarchi@intel.com/


Takashi
Takashi Iwai April 19, 2022, 7:01 a.m. UTC | #6
On Sat, 16 Apr 2022 08:44:18 +0200,
Lucas De Marchi wrote:
> 
> pci_get_class() will already unref the pci device passed as argument.
> So if it's unconditionally unref'ed, even if the loop is not stopped,
> there will be one too many unref for each device not matched.
> 
> Cc: Kai Vehmanen <kai.vehmanen@linux.intel.com>
> Cc: Takashi Iwai <tiwai@suse.de>
> Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/5701
> Fixes: 0dc2696a4623 ("ALSA: hda/i915 - skip acomp init if no matching display")
> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>

Thanks, applied now.
But the Fixes commit id was wrong.  I corrected to the right upstream
one, c9db8a30d9f0.


Takashi
Lucas De Marchi April 19, 2022, 3:05 p.m. UTC | #7
On Tue, Apr 19, 2022 at 09:01:34AM +0200, Takashi Iwai wrote:
>On Sat, 16 Apr 2022 08:44:18 +0200,
>Lucas De Marchi wrote:
>>
>> pci_get_class() will already unref the pci device passed as argument.
>> So if it's unconditionally unref'ed, even if the loop is not stopped,
>> there will be one too many unref for each device not matched.
>>
>> Cc: Kai Vehmanen <kai.vehmanen@linux.intel.com>
>> Cc: Takashi Iwai <tiwai@suse.de>
>> Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/5701
>> Fixes: 0dc2696a4623 ("ALSA: hda/i915 - skip acomp init if no matching display")
>> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
>
>Thanks, applied now.
>But the Fixes commit id was wrong.  I corrected to the right upstream
>one, c9db8a30d9f0.

indeed, I messed that up, using the hash from our local tree.

thanks
Lucas De Marchi
diff mbox series

Patch

diff --git a/sound/hda/hdac_i915.c b/sound/hda/hdac_i915.c
index 1ce2cea137db..12dad5e9c45d 100644
--- a/sound/hda/hdac_i915.c
+++ b/sound/hda/hdac_i915.c
@@ -127,11 +127,10 @@  static int i915_gfx_present(struct pci_dev *hdac_pci)
 		display_dev = pci_get_class(class, display_dev);
 
 		if (display_dev && display_dev->vendor == PCI_VENDOR_ID_INTEL &&
-		    connectivity_check(display_dev, hdac_pci))
+		    connectivity_check(display_dev, hdac_pci)) {
+			pci_dev_put(display_dev);
 			match = true;
-
-		pci_dev_put(display_dev);
-
+		}
 	} while (!match && display_dev);
 
 	return match;