2021-03-20 22:25:40

by Tong Zhang

[permalink] [raw]
Subject: [PATCH v2 0/3] ALSA: hdsp and hdspm, don't disable device if not enabled

This series fixes issues in hdsp and hdspm. The drivers in question want
to disable a device that is not enabled on error path.

v2: add fix to rme9652

Tong Zhang (3):
ALSA: hdsp: don't disable if not enabled
ALSA: hdspm: don't disable if not enabled
ALSA: rme9652: don't disable if not enabled

sound/pci/rme9652/hdsp.c | 10 ++++++----
sound/pci/rme9652/hdspm.c | 10 ++++++----
sound/pci/rme9652/rme9652.c | 10 ++++++----
3 files changed, 18 insertions(+), 12 deletions(-)

--
2.25.1


2021-03-20 22:25:40

by Tong Zhang

[permalink] [raw]
Subject: [PATCH v2 1/3] ALSA: hdsp: don't disable if not enabled

hdsp wants to disable a not enabled pci device, which makes kernel
throw a warning. Make sure the device is enabled before calling disable.

[ 1.758292] snd_hdsp 0000:00:03.0: disabling already-disabled device
[ 1.758327] WARNING: CPU: 0 PID: 180 at drivers/pci/pci.c:2146 pci_disable_device+0x91/0xb0
[ 1.766985] Call Trace:
[ 1.767121] snd_hdsp_card_free+0x94/0xf0 [snd_hdsp]
[ 1.767388] release_card_device+0x4b/0x80 [snd]
[ 1.767639] device_release+0x3b/0xa0
[ 1.767838] kobject_put+0x94/0x1b0
[ 1.768027] put_device+0x13/0x20
[ 1.768207] snd_card_free+0x61/0x90 [snd]
[ 1.768430] snd_hdsp_probe+0x524/0x5e0 [snd_hdsp]

Signed-off-by: Tong Zhang <[email protected]>
---
sound/pci/rme9652/hdsp.c | 10 ++++++----
1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/sound/pci/rme9652/hdsp.c b/sound/pci/rme9652/hdsp.c
index 4cf879c42dc4..d9879a5bd60e 100644
--- a/sound/pci/rme9652/hdsp.c
+++ b/sound/pci/rme9652/hdsp.c
@@ -5285,8 +5285,10 @@ static int snd_hdsp_create(struct snd_card *card,

pci_set_master(hdsp->pci);

- if ((err = pci_request_regions(pci, "hdsp")) < 0)
+ if ((err = pci_request_regions(pci, "hdsp")) < 0) {
+ pci_disable_device(pci);
return err;
+ }
hdsp->port = pci_resource_start(pci, 0);
if ((hdsp->iobase = ioremap(hdsp->port, HDSP_IO_EXTENT)) == NULL) {
dev_err(hdsp->card->dev, "unable to remap region 0x%lx-0x%lx\n",
@@ -5387,10 +5389,10 @@ static int snd_hdsp_free(struct hdsp *hdsp)
vfree(hdsp->fw_uploaded);
iounmap(hdsp->iobase);

- if (hdsp->port)
+ if (hdsp->port) {
pci_release_regions(hdsp->pci);
-
- pci_disable_device(hdsp->pci);
+ pci_disable_device(hdsp->pci);
+ }
return 0;
}

--
2.25.1

2021-03-20 22:27:43

by Tong Zhang

[permalink] [raw]
Subject: [PATCH v2 2/3] ALSA: hdspm: don't disable if not enabled

hdspm wants to disable a not enabled pci device, which makes kernel
throw a warning. Make sure the device is enabled before calling disable.

[ 1.786391] snd_hdspm 0000:00:03.0: disabling already-disabled device
[ 1.786400] WARNING: CPU: 0 PID: 182 at drivers/pci/pci.c:2146 pci_disable_device+0x91/0xb0
[ 1.795181] Call Trace:
[ 1.795320] snd_hdspm_card_free+0x58/0xa0 [snd_hdspm]
[ 1.795595] release_card_device+0x4b/0x80 [snd]
[ 1.795860] device_release+0x3b/0xa0
[ 1.796072] kobject_put+0x94/0x1b0
[ 1.796260] put_device+0x13/0x20
[ 1.796438] snd_card_free+0x61/0x90 [snd]
[ 1.796659] snd_hdspm_probe+0x97b/0x1440 [snd_hdspm]

Signed-off-by: Tong Zhang <[email protected]>
---
sound/pci/rme9652/hdspm.c | 10 ++++++----
1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/sound/pci/rme9652/hdspm.c b/sound/pci/rme9652/hdspm.c
index 8d900c132f0f..af3898c88bba 100644
--- a/sound/pci/rme9652/hdspm.c
+++ b/sound/pci/rme9652/hdspm.c
@@ -6582,8 +6582,10 @@ static int snd_hdspm_create(struct snd_card *card,
pci_set_master(hdspm->pci);

err = pci_request_regions(pci, "hdspm");
- if (err < 0)
+ if (err < 0) {
+ pci_disable_device(pci);
return err;
+ }

hdspm->port = pci_resource_start(pci, 0);
io_extent = pci_resource_len(pci, 0);
@@ -6880,10 +6882,10 @@ static int snd_hdspm_free(struct hdspm * hdspm)
kfree(hdspm->mixer);
iounmap(hdspm->iobase);

- if (hdspm->port)
+ if (hdspm->port) {
pci_release_regions(hdspm->pci);
-
- pci_disable_device(hdspm->pci);
+ pci_disable_device(hdspm->pci);
+ }
return 0;
}

--
2.25.1

2021-03-20 22:28:43

by Tong Zhang

[permalink] [raw]
Subject: [PATCH v2 3/3] ALSA: rme9652: don't disable if not enabled

rme9652 wants to disable a not enabled pci device, which makes kernel
throw a warning. Make sure the device is enabled before calling disable.

[ 1.751595] snd_rme9652 0000:00:03.0: disabling already-disabled device
[ 1.751605] WARNING: CPU: 0 PID: 174 at drivers/pci/pci.c:2146 pci_disable_device+0x91/0xb0
[ 1.759968] Call Trace:
[ 1.760145] snd_rme9652_card_free+0x76/0xa0 [snd_rme9652]
[ 1.760434] release_card_device+0x4b/0x80 [snd]
[ 1.760679] device_release+0x3b/0xa0
[ 1.760874] kobject_put+0x94/0x1b0
[ 1.761059] put_device+0x13/0x20
[ 1.761235] snd_card_free+0x61/0x90 [snd]
[ 1.761454] snd_rme9652_probe+0x3be/0x700 [snd_rme9652]

Signed-off-by: Tong Zhang <[email protected]>
---
sound/pci/rme9652/rme9652.c | 10 ++++++----
1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/sound/pci/rme9652/rme9652.c b/sound/pci/rme9652/rme9652.c
index 4df992e846f2..f9c9b8a80797 100644
--- a/sound/pci/rme9652/rme9652.c
+++ b/sound/pci/rme9652/rme9652.c
@@ -1728,10 +1728,10 @@ static int snd_rme9652_free(struct snd_rme9652 *rme9652)
if (rme9652->irq >= 0)
free_irq(rme9652->irq, (void *)rme9652);
iounmap(rme9652->iobase);
- if (rme9652->port)
+ if (rme9652->port) {
pci_release_regions(rme9652->pci);
-
- pci_disable_device(rme9652->pci);
+ pci_disable_device(rme9652->pci);
+ }
return 0;
}

@@ -2454,8 +2454,10 @@ static int snd_rme9652_create(struct snd_card *card,

spin_lock_init(&rme9652->lock);

- if ((err = pci_request_regions(pci, "rme9652")) < 0)
+ if ((err = pci_request_regions(pci, "rme9652")) < 0) {
+ pci_disable_device(pci);
return err;
+ }
rme9652->port = pci_resource_start(pci, 0);
rme9652->iobase = ioremap(rme9652->port, RME9652_IO_EXTENT);
if (rme9652->iobase == NULL) {
--
2.25.1

2021-03-21 08:31:39

by Takashi Iwai

[permalink] [raw]
Subject: Re: [PATCH v2 0/3] ALSA: hdsp and hdspm, don't disable device if not enabled

On Sat, 20 Mar 2021 23:23:33 +0100,
Tong Zhang wrote:
>
> This series fixes issues in hdsp and hdspm. The drivers in question want
> to disable a device that is not enabled on error path.
>
> v2: add fix to rme9652
>
> Tong Zhang (3):
> ALSA: hdsp: don't disable if not enabled
> ALSA: hdspm: don't disable if not enabled
> ALSA: rme9652: don't disable if not enabled

Thanks for the patches.

IMO, a safer way for this is to add pci_is_enabled() check in *_free()
functions around the call of pci_disable_device(). The point is that
*_free() is the sole destructor function that manages all stuff, hence
it's better to do all there. And, of course, it'll be less changes.

Care to resend v3 patches with that?


thanks,

Takashi

2021-03-21 16:20:59

by Tong Zhang

[permalink] [raw]
Subject: [PATCH v3 2/3] ALSA: hdspm: don't disable if not enabled

hdspm wants to disable a not enabled pci device, which makes kernel
throw a warning. Make sure the device is enabled before calling disable.

[ 1.786391] snd_hdspm 0000:00:03.0: disabling already-disabled device
[ 1.786400] WARNING: CPU: 0 PID: 182 at drivers/pci/pci.c:2146 pci_disable_device+0x91/0xb0
[ 1.795181] Call Trace:
[ 1.795320] snd_hdspm_card_free+0x58/0xa0 [snd_hdspm]
[ 1.795595] release_card_device+0x4b/0x80 [snd]
[ 1.795860] device_release+0x3b/0xa0
[ 1.796072] kobject_put+0x94/0x1b0
[ 1.796260] put_device+0x13/0x20
[ 1.796438] snd_card_free+0x61/0x90 [snd]
[ 1.796659] snd_hdspm_probe+0x97b/0x1440 [snd_hdspm]

Suggested-by: Takashi Iwai <[email protected]>
Signed-off-by: Tong Zhang <[email protected]>
---
sound/pci/rme9652/hdspm.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/sound/pci/rme9652/hdspm.c b/sound/pci/rme9652/hdspm.c
index 8d900c132f0f..00cbf81ab2a6 100644
--- a/sound/pci/rme9652/hdspm.c
+++ b/sound/pci/rme9652/hdspm.c
@@ -6883,7 +6883,8 @@ static int snd_hdspm_free(struct hdspm * hdspm)
if (hdspm->port)
pci_release_regions(hdspm->pci);

- pci_disable_device(hdspm->pci);
+ if (pci_is_enabled(hdspm->pci))
+ pci_disable_device(hdspm->pci);
return 0;
}

--
2.25.1

2021-03-21 16:21:08

by Tong Zhang

[permalink] [raw]
Subject: [PATCH v3 1/3] ALSA: hdsp: don't disable if not enabled

hdsp wants to disable a not enabled pci device, which makes kernel
throw a warning. Make sure the device is enabled before calling disable.

[ 1.758292] snd_hdsp 0000:00:03.0: disabling already-disabled device
[ 1.758327] WARNING: CPU: 0 PID: 180 at drivers/pci/pci.c:2146 pci_disable_device+0x91/0xb0
[ 1.766985] Call Trace:
[ 1.767121] snd_hdsp_card_free+0x94/0xf0 [snd_hdsp]
[ 1.767388] release_card_device+0x4b/0x80 [snd]
[ 1.767639] device_release+0x3b/0xa0
[ 1.767838] kobject_put+0x94/0x1b0
[ 1.768027] put_device+0x13/0x20
[ 1.768207] snd_card_free+0x61/0x90 [snd]
[ 1.768430] snd_hdsp_probe+0x524/0x5e0 [snd_hdsp]

Suggested-by: Takashi Iwai <[email protected]>
Signed-off-by: Tong Zhang <[email protected]>
---
sound/pci/rme9652/hdsp.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/sound/pci/rme9652/hdsp.c b/sound/pci/rme9652/hdsp.c
index 4cf879c42dc4..720297cbdf87 100644
--- a/sound/pci/rme9652/hdsp.c
+++ b/sound/pci/rme9652/hdsp.c
@@ -5390,7 +5390,8 @@ static int snd_hdsp_free(struct hdsp *hdsp)
if (hdsp->port)
pci_release_regions(hdsp->pci);

- pci_disable_device(hdsp->pci);
+ if (pci_is_enabled(hdsp->pci))
+ pci_disable_device(hdsp->pci);
return 0;
}

--
2.25.1

2021-03-21 16:21:52

by Tong Zhang

[permalink] [raw]
Subject: [PATCH v3 3/3] ALSA: rme9652: don't disable if not enabled

rme9652 wants to disable a not enabled pci device, which makes kernel
throw a warning. Make sure the device is enabled before calling disable.

[ 1.751595] snd_rme9652 0000:00:03.0: disabling already-disabled device
[ 1.751605] WARNING: CPU: 0 PID: 174 at drivers/pci/pci.c:2146 pci_disable_device+0x91/0xb0
[ 1.759968] Call Trace:
[ 1.760145] snd_rme9652_card_free+0x76/0xa0 [snd_rme9652]
[ 1.760434] release_card_device+0x4b/0x80 [snd]
[ 1.760679] device_release+0x3b/0xa0
[ 1.760874] kobject_put+0x94/0x1b0
[ 1.761059] put_device+0x13/0x20
[ 1.761235] snd_card_free+0x61/0x90 [snd]
[ 1.761454] snd_rme9652_probe+0x3be/0x700 [snd_rme9652]

Suggested-by: Takashi Iwai <[email protected]>
Signed-off-by: Tong Zhang <[email protected]>
---
sound/pci/rme9652/rme9652.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/sound/pci/rme9652/rme9652.c b/sound/pci/rme9652/rme9652.c
index 4df992e846f2..f407a95fc81f 100644
--- a/sound/pci/rme9652/rme9652.c
+++ b/sound/pci/rme9652/rme9652.c
@@ -1731,7 +1731,8 @@ static int snd_rme9652_free(struct snd_rme9652 *rme9652)
if (rme9652->port)
pci_release_regions(rme9652->pci);

- pci_disable_device(rme9652->pci);
+ if (pci_is_enabled(rme9652->pci))
+ pci_disable_device(rme9652->pci);
return 0;
}

--
2.25.1

2021-03-21 16:22:36

by Tong Zhang

[permalink] [raw]
Subject: Re: [PATCH v2 0/3] ALSA: hdsp and hdspm, don't disable device if not enabled

On Sun, Mar 21, 2021 at 4:16 AM Takashi Iwai <[email protected]> wrote:
>
> On Sat, 20 Mar 2021 23:23:33 +0100,
> Tong Zhang wrote:
> >
> > This series fixes issues in hdsp and hdspm. The drivers in question want
> > to disable a device that is not enabled on error path.
> >
> > v2: add fix to rme9652
> >
> > Tong Zhang (3):
> > ALSA: hdsp: don't disable if not enabled
> > ALSA: hdspm: don't disable if not enabled
> > ALSA: rme9652: don't disable if not enabled
>
> Thanks for the patches.
>
> IMO, a safer way for this is to add pci_is_enabled() check in *_free()
> functions around the call of pci_disable_device(). The point is that
> *_free() is the sole destructor function that manages all stuff, hence
> it's better to do all there. And, of course, it'll be less changes.
>
> Care to resend v3 patches with that?
>
>
> thanks,
>
> Takashi

Thanks Takashi.
I made some changes and sent them as v3.
- Tong

2021-03-21 17:10:05

by Tong Zhang

[permalink] [raw]
Subject: [PATCH v3 0/3] ALSA: rme9652 don't disable device if not enabled

This series fixes issues in hdsp and hdspm. The drivers in question want
to disable a device that is not enabled on error path.

v2: add fix to rme9652
v3: change checks to pci_is_enabled()

Tong Zhang (3):
ALSA: hdsp: don't disable if not enabled
ALSA: hdspm: don't disable if not enabled
ALSA: rme9652: don't disable if not enabled

sound/pci/rme9652/hdsp.c | 3 ++-
sound/pci/rme9652/hdspm.c | 3 ++-
sound/pci/rme9652/rme9652.c | 3 ++-
3 files changed, 6 insertions(+), 3 deletions(-)

--
2.25.1

2021-03-22 11:25:10

by Takashi Iwai

[permalink] [raw]
Subject: Re: [PATCH v3 0/3] ALSA: rme9652 don't disable device if not enabled

On Sun, 21 Mar 2021 16:38:37 +0100,
Tong Zhang wrote:
>
> This series fixes issues in hdsp and hdspm. The drivers in question want
> to disable a device that is not enabled on error path.
>
> v2: add fix to rme9652
> v3: change checks to pci_is_enabled()
>
> Tong Zhang (3):
> ALSA: hdsp: don't disable if not enabled
> ALSA: hdspm: don't disable if not enabled
> ALSA: rme9652: don't disable if not enabled

Now applied all three patches. Thanks.


Takashi