On Sun, 10 Apr 2022 11:13:55 +0200, Zheyu Ma wrote:

Hello,

I found a bug in echoaudio.c. When the driver fails at the function snd_echo_create(), it should release resources requested before, otherwise we will get the following warning:

[ 3.262866] remove_proc_entry: removing non-empty directory 'irq/21', leaking at least 'snd_indigodj' [ 3.263577] WARNING: CPU: 3 PID: 261 at fs/proc/generic.c:717 remove_proc_entry+0x389/0x3f0 [ 3.267098] RIP: 0010:remove_proc_entry+0x389/0x3f0 [ 3.269976] Call Trace: [ 3.269979] <TASK> [ 3.269988] unregister_irq_proc+0x14c/0x170 [ 3.269997] irq_free_descs+0x94/0xe0 [ 3.270004] mp_unmap_irq+0xb6/0x100 [ 3.270011] acpi_unregister_gsi_ioapic+0x27/0x40 [ 3.270017] acpi_pci_irq_disable+0x1d3/0x320 [ 3.270025] pci_disable_device+0x1ad/0x380 [ 3.270034] pcim_release+0x566/0x6d0 [ 3.270046] devres_release_all+0x1f1/0x2c0 [ 3.270057] really_probe+0xe0/0x920

Could you try the patch below?

thanks,

Takashi

--- --- a/sound/pci/echoaudio/echoaudio.c +++ b/sound/pci/echoaudio/echoaudio.c @@ -1906,9 +1906,6 @@ static int snd_echo_create(struct snd_card *card,

pci_write_config_byte(pci, PCI_LATENCY_TIMER, 0xC0);

- err = pcim_enable_device(pci); - if (err < 0) - return err; pci_set_master(pci);

/* Allocate chip if needed */ @@ -1987,6 +1984,10 @@ static int snd_echo_probe(struct pci_dev *pci, return -ENOENT; }

+ err = pcim_enable_device(pci); + if (err < 0) + return err; + i = 0; err = snd_devm_card_new(&pci->dev, index[dev], id[dev], THIS_MODULE, sizeof(*chip), &card);

On Mon, 11 Apr 2022 10:49:53 +0200, Zheyu Ma wrote:

On Mon, Apr 11, 2022 at 3:41 PM Takashi Iwai tiwai@suse.de wrote:

...

On Sun, 10 Apr 2022 11:13:55 +0200, Zheyu Ma wrote:

...

Hello,

I found a bug in echoaudio.c. When the driver fails at the function snd_echo_create(), it should release resources requested before, otherwise we will get the following warning:

[ 3.262866] remove_proc_entry: removing non-empty directory 'irq/21', leaking at least 'snd_indigodj' [ 3.263577] WARNING: CPU: 3 PID: 261 at fs/proc/generic.c:717 remove_proc_entry+0x389/0x3f0 [ 3.267098] RIP: 0010:remove_proc_entry+0x389/0x3f0 [ 3.269976] Call Trace: [ 3.269979] <TASK> [ 3.269988] unregister_irq_proc+0x14c/0x170 [ 3.269997] irq_free_descs+0x94/0xe0 [ 3.270004] mp_unmap_irq+0xb6/0x100 [ 3.270011] acpi_unregister_gsi_ioapic+0x27/0x40 [ 3.270017] acpi_pci_irq_disable+0x1d3/0x320 [ 3.270025] pci_disable_device+0x1ad/0x380 [ 3.270034] pcim_release+0x566/0x6d0 [ 3.270046] devres_release_all+0x1f1/0x2c0 [ 3.270057] really_probe+0xe0/0x920

Could you try the patch below?

The following patch works for me, the previous warning disappears, thank you. But I got another error, I have no idea about it.

OK, that's bad, it's basically the destructor order problem. Could you try the patch below instead of the previous one?

thanks,

Takashi

--- --- a/sound/pci/echoaudio/echoaudio.c +++ b/sound/pci/echoaudio/echoaudio.c @@ -1889,13 +1889,20 @@ static void snd_echo_free(struct snd_card *card) if (chip->comm_page) rest_in_peace(chip);

- if (chip->irq >= 0) - free_irq(chip->irq, chip); - /* release chip data */ free_firmware_cache(chip); }

+static void do_free_irq(void *p) +{ + struct echoaudio *chip = p; + + if (chip->irq >= 0) { + free_irq(chip->irq, chip); + chip->irq = -1; + } +} + /* <--snd_echo_probe() */ static int snd_echo_create(struct snd_card *card, struct pci_dev *pci) @@ -1945,8 +1952,9 @@ static int snd_echo_create(struct snd_card *card, card->sync_irq = chip->irq; dev_dbg(card->dev, "pci=%p irq=%d subdev=%04x Init hardware...\n", chip->pci, chip->irq, chip->pci->subsystem_device); - - card->private_free = snd_echo_free; + err = devm_add_action(card->dev, do_free_irq, chip); + if (err < 0) + return err;

/* Create the DSP comm page - this is the area of memory used for most of the communication with the DSP, which accesses it via bus mastering */ @@ -1957,6 +1965,7 @@ static int snd_echo_create(struct snd_card *card, return -ENOMEM; chip->comm_page_phys = chip->commpage_dma_buf->addr; chip->comm_page = (struct comm_page *)chip->commpage_dma_buf->area; + card->private_free = snd_echo_free;

err = init_hw(chip, chip->pci->device, chip->pci->subsystem_device); if (err >= 0)

On Mon, 11 Apr 2022 12:34:56 +0200, Zheyu Ma wrote:

On Mon, Apr 11, 2022 at 6:30 PM Takashi Iwai tiwai@suse.de wrote:

...

On Mon, 11 Apr 2022 11:15:41 +0200, Takashi Iwai wrote:

...

On Mon, 11 Apr 2022 11:06:09 +0200, Takashi Iwai wrote:

...

On Mon, 11 Apr 2022 10:49:53 +0200, Zheyu Ma wrote:

...

On Mon, Apr 11, 2022 at 3:41 PM Takashi Iwai tiwai@suse.de wrote:

...

On Sun, 10 Apr 2022 11:13:55 +0200, Zheyu Ma wrote: > > Hello, > > I found a bug in echoaudio.c. > When the driver fails at the function snd_echo_create(), it should > release resources requested before, otherwise we will get the > following warning: > > [ 3.262866] remove_proc_entry: removing non-empty directory > 'irq/21', leaking at least 'snd_indigodj' > [ 3.263577] WARNING: CPU: 3 PID: 261 at fs/proc/generic.c:717 > remove_proc_entry+0x389/0x3f0 > [ 3.267098] RIP: 0010:remove_proc_entry+0x389/0x3f0 > [ 3.269976] Call Trace: > [ 3.269979] <TASK> > [ 3.269988] unregister_irq_proc+0x14c/0x170 > [ 3.269997] irq_free_descs+0x94/0xe0 > [ 3.270004] mp_unmap_irq+0xb6/0x100 > [ 3.270011] acpi_unregister_gsi_ioapic+0x27/0x40 > [ 3.270017] acpi_pci_irq_disable+0x1d3/0x320 > [ 3.270025] pci_disable_device+0x1ad/0x380 > [ 3.270034] pcim_release+0x566/0x6d0 > [ 3.270046] devres_release_all+0x1f1/0x2c0 > [ 3.270057] really_probe+0xe0/0x920

Could you try the patch below?

The following patch works for me, the previous warning disappears, thank you. But I got another error, I have no idea about it.

OK, that's bad, it's basically the destructor order problem. Could you try the patch below instead of the previous one?

Sorry, the below one instead.

Err, scratch this one, too. It's a deeper problem than I thought. Will post a revised patch later.

Yeah, you are right, these two patches do not work, I got the same error message. Thanks for your effort.

Here we go, a revised patch. Basically it enforces the call of snd_card_free() at the error path during probe.

There are a bunch of similar patterns, and we need to address them all...

thanks,

Takashi

--- --- a/include/sound/core.h +++ b/include/sound/core.h @@ -279,6 +279,7 @@ int snd_card_new(struct device *parent, int idx, const char *xid, int snd_devm_card_new(struct device *parent, int idx, const char *xid, struct module *module, size_t extra_size, struct snd_card **card_ret); +int snd_devm_probe_or_release(struct device *dev, int ret);

int snd_card_disconnect(struct snd_card *card); void snd_card_disconnect_sync(struct snd_card *card); --- a/sound/core/init.c +++ b/sound/core/init.c @@ -235,6 +235,19 @@ int snd_devm_card_new(struct device *parent, int idx, const char *xid, } EXPORT_SYMBOL_GPL(snd_devm_card_new);

+int snd_devm_probe_or_release(struct device *dev, int ret) +{ + struct snd_card *card; + + if (!ret) + return 0; + card = devres_find(dev, __snd_card_release, NULL, NULL); + if (card) + snd_card_free(card); + return ret; +} +EXPORT_SYMBOL_GPL(snd_devm_probe_or_release); + static int snd_card_init(struct snd_card *card, struct device *parent, int idx, const char *xid, struct module *module, size_t extra_size) --- a/sound/pci/echoaudio/echoaudio.c +++ b/sound/pci/echoaudio/echoaudio.c @@ -1970,8 +1970,8 @@ static int snd_echo_create(struct snd_card *card, }

/* constructor */ -static int snd_echo_probe(struct pci_dev *pci, - const struct pci_device_id *pci_id) +static int __snd_echo_probe(struct pci_dev *pci, + const struct pci_device_id *pci_id) { static int dev; struct snd_card *card; @@ -2139,6 +2139,12 @@ static int snd_echo_probe(struct pci_dev *pci, return 0; }

+static int snd_echo_probe(struct pci_dev *pci, + const struct pci_device_id *pci_id) +{ + return snd_devm_probe_or_release(&pci->dev, + __snd_echo_probe(pci, pci_id)); +}

#if defined(CONFIG_PM_SLEEP)