[PATCH 0/2] ASoC: Intel: avs: Fixes for avs_pci_probe()

[PATCH 0/2] ASoC: Intel: avs: Fixes for avs_pci_probe()

[Cezary Rojewski]

Two small changes that aim to improve code quality around error-path
area for the avs_pci_probe() function.

1) The synchronization-change is here to make sure both happy and error
paths are streamlines, no functional changes there.
2) The fix addresses null-ptr-deref and similar problems is
pcim_request_all_regions() fails.

Cezary Rojewski (2):
  ASoC: Intel: avs: Synchronize happy and error paths in PCI probe
  ASoC: Intel: avs: Fix uninitialized pointer error in probe()

 sound/soc/intel/avs/core.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

-- 
2.25.1

[PATCH 1/2] ASoC: Intel: avs: Synchronize happy and error paths in PCI probe

[Cezary Rojewski]

Error path in avs_pci_probe() does not match its happy path.

Reviewed-by: Amadeusz Sławiński <amadeuszx.slawinski@linux.intel.com>
Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com>
---
 sound/soc/intel/avs/core.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/sound/soc/intel/avs/core.c b/sound/soc/intel/avs/core.c
index 7af324753673..5ea42b5af26f 100644
--- a/sound/soc/intel/avs/core.c
+++ b/sound/soc/intel/avs/core.c
@@ -505,11 +505,11 @@ static int avs_pci_probe(struct pci_dev *pci, const struct pci_device_id *id)
 	return 0;
 
 err_i915_init:
-	pci_free_irq(pci, 0, adev);
-	pci_free_irq(pci, 0, bus);
-	pci_free_irq_vectors(pci);
-	pci_clear_master(pci);
 	pci_set_drvdata(pci, NULL);
+	pci_clear_master(pci);
+	pci_free_irq(pci, 0, adev);
+	pci_free_irq(pci, 0, bus);
+	pci_free_irq_vectors(pci);
 err_acquire_irq:
 	snd_hdac_bus_free_stream_pages(bus);
 	snd_hdac_ext_stream_free_all(bus);
-- 
2.25.1

[PATCH 2/2] ASoC: Intel: avs: Fix uninitialized pointer error in probe()

[Cezary Rojewski]

If pcim_request_all_regions() fails, error path operates on
uninitialized 'bus' pointer. Found out by Coverity static analyzer.

Reviewed-by: Amadeusz Sławiński <amadeuszx.slawinski@linux.intel.com>
Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com>
---
 sound/soc/intel/avs/core.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/sound/soc/intel/avs/core.c b/sound/soc/intel/avs/core.c
index 5ea42b5af26f..c40cdfc4ef96 100644
--- a/sound/soc/intel/avs/core.c
+++ b/sound/soc/intel/avs/core.c
@@ -445,6 +445,8 @@ static int avs_pci_probe(struct pci_dev *pci, const struct pci_device_id *id)
 	adev = devm_kzalloc(dev, sizeof(*adev), GFP_KERNEL);
 	if (!adev)
 		return -ENOMEM;
+	bus = &adev->base.core;
+
 	ret = avs_bus_init(adev, pci, id);
 	if (ret < 0) {
 		dev_err(dev, "failed to init avs bus: %d\n", ret);
@@ -455,7 +457,6 @@ static int avs_pci_probe(struct pci_dev *pci, const struct pci_device_id *id)
 	if (ret < 0)
 		return ret;
 
-	bus = &adev->base.core;
 	bus->addr = pci_resource_start(pci, 0);
 	bus->remap_addr = pci_ioremap_bar(pci, 0);
 	if (!bus->remap_addr) {
-- 
2.25.1

Re: [PATCH 1/2] ASoC: Intel: avs: Synchronize happy and error paths in PCI probe

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 654 bytes --]

On Tue, Jul 29, 2025 at 01:42:28PM +0200, Cezary Rojewski wrote:

>  err_i915_init:
> -	pci_free_irq(pci, 0, adev);
> -	pci_free_irq(pci, 0, bus);
> -	pci_free_irq_vectors(pci);
> -	pci_clear_master(pci);
>  	pci_set_drvdata(pci, NULL);
> +	pci_clear_master(pci);
> +	pci_free_irq(pci, 0, adev);
> +	pci_free_irq(pci, 0, bus);
> +	pci_free_irq_vectors(pci);

The ordering before clearing the driver data actually looks sensible
there, though looking at the interrupt handlers quickly they don't seem
to pay attention to driver data.  Really you could just delete the
pci_set_drvdata(), it should be redundant (and IIRC the driver core does
that anyway).

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 1/2] ASoC: Intel: avs: Synchronize happy and error paths in PCI probe

[Cezary Rojewski]

On 2025-07-29 1:47 PM, Mark Brown wrote:
> On Tue, Jul 29, 2025 at 01:42:28PM +0200, Cezary Rojewski wrote:
> 
>>   err_i915_init:
>> -	pci_free_irq(pci, 0, adev);
>> -	pci_free_irq(pci, 0, bus);
>> -	pci_free_irq_vectors(pci);
>> -	pci_clear_master(pci);
>>   	pci_set_drvdata(pci, NULL);
>> +	pci_clear_master(pci);
>> +	pci_free_irq(pci, 0, adev);
>> +	pci_free_irq(pci, 0, bus);
>> +	pci_free_irq_vectors(pci);
> 
> The ordering before clearing the driver data actually looks sensible
> there, though looking at the interrupt handlers quickly they don't seem
> to pay attention to driver data.  Really you could just delete the
> pci_set_drvdata(), it should be redundant (and IIRC the driver core does
> that anyway).

While I do agree that pci_set_drvdata(NULL) could be skipped, I feel 
better with having a true reverse order in the error path:

happy path:
	pci_alloc_irq_vectors();
	pci_request_irq(bus);
	pci_request_irq(adev);
	pci_set_master();


error path:
	pci_clear_master(pci);
	pci_free_irq(pci, 0, adev);
	pci_free_irq(pci, 0, bus);
	pci_free_irq_vectors(pci);

Is there anything wrong with the approach before I send v2?

Re: [PATCH 1/2] ASoC: Intel: avs: Synchronize happy and error paths in PCI probe

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 475 bytes --]

On Tue, Jul 29, 2025 at 02:49:18PM +0200, Cezary Rojewski wrote:

> happy path:
> 	pci_alloc_irq_vectors();
> 	pci_request_irq(bus);
> 	pci_request_irq(adev);
> 	pci_set_master();

> error path:
> 	pci_clear_master(pci);
> 	pci_free_irq(pci, 0, adev);
> 	pci_free_irq(pci, 0, bus);
> 	pci_free_irq_vectors(pci);

> Is there anything wrong with the approach before I send v2?

Unless there's some issue with having master disabled but interrupts
disabled that should be fine.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 1/2] ASoC: Intel: avs: Synchronize happy and error paths in PCI probe

[Cezary Rojewski]

On 2025-07-29 3:32 PM, Mark Brown wrote:
> On Tue, Jul 29, 2025 at 02:49:18PM +0200, Cezary Rojewski wrote:
> 
>> happy path:
>> 	pci_alloc_irq_vectors();
>> 	pci_request_irq(bus);
>> 	pci_request_irq(adev);
>> 	pci_set_master();
> 
>> error path:
>> 	pci_clear_master(pci);
>> 	pci_free_irq(pci, 0, adev);
>> 	pci_free_irq(pci, 0, bus);
>> 	pci_free_irq_vectors(pci);
> 
>> Is there anything wrong with the approach before I send v2?
> 
> Unless there's some issue with having master disabled but interrupts
> disabled that should be fine.

After your comments, I've investigated the drivers/ code and it seems 
it's more common to put pci_set_master() early in the probe() rather 
than at the end of it. That may trigger an update to avs/ and would 
render this very patch redundant.

Because of that, I've decided to drop this patch. Will re-iterate on the 
subject but don't want to block the followup patch.

Good review Mark!