* [PATCH 00/51] ALSA: More devres usages
@ 2021-07-13 14:24 Takashi Iwai
2021-07-13 14:24 ` [PATCH 01/51] ALSA: core: Add device-managed page allocator helper Takashi Iwai
` (9 more replies)
0 siblings, 10 replies; 15+ messages in thread
From: Takashi Iwai @ 2021-07-13 14:24 UTC (permalink / raw)
To: alsa-devel
Hi,
this is a resurrect of my early RFC patch with the fix and applying to
more drivers. The main purpose is merely the code cleanup over the
tree by replacing the resource allocations with devres helpers as much
as possible.
The only major change is the addition of devres-supported card object
handling snd_devm_card_new() and the page allocation helper
snd_devm_alloc_pages(). Those allow us to get rid of the remove
callback completely in many drivers. In the end, the series resulted
in a significant amount of code reduction.
This series covers only the drivers in pci, x86 and drivers
subdirectories. The patches for isa subdirectory will follow later.
Takashi
===
Takashi Iwai (51):
ALSA: core: Add device-managed page allocator helper
ALSA: core: Add managed card creation
ALSA: intel8x0: Allocate resources with device-managed APIs
ALSA: atiixp: Allocate resources with device-managed APIs
ALSA: hda: Allocate resources with device-managed APIs
ALSA: doc: Add device-managed resource section
ALSA: ad1889: Allocate resources with device-managed APIs
ALSA: als300: Allocate resources with device-managed APIs
ALSA: als4000: Allocate resources with device-managed APIs
ALSA: azt3328: Allocate resources with device-managed APIs
ALSA: bt87x: Allocate resources with device-managed APIs
ALSA: cmipci: Allocate resources with device-managed APIs
ALSA: cs4281: Allocate resources with device-managed APIs
ALSA: cs5530: Allocate resources with device-managed APIs
ALSA: ens137x: Allocate resources with device-managed APIs
ALSA: es1938: Allocate resources with device-managed APIs
ALSA: es1968: Allocate resources with device-managed APIs
ALSA: fm801: Allocate resources with device-managed APIs
ALSA: maestro3: Allocate resources with device-managed APIs
ALSA: rme32: Allocate resources with device-managed APIs
ALSA: rme96: Allocate resources with device-managed APIs
ALSA: sis7019: Allocate resources with device-managed APIs
ALSA: sonicvibes: Allocate resources with device-managed APIs
ALSA: via82xx: Allocate resources with device-managed APIs
ALSA: ali5451: Allocate resources with device-managed APIs
ALSA: au88x0: Allocate resources with device-managed APIs
ALSA: aw2: Allocate resources with device-managed APIs
ALSA: ca0106: Allocate resources with device-managed APIs
ALSA: cs46xx: Allocate resources with device-managed APIs
ALSA: cs5535audio: Allocate resources with device-managed APIs
ALSA: echoaudio: Allocate resources with device-managed APIs
ALSA: emu10k1: Allocate resources with device-managed APIs
ALSA: emu10k1x: Allocate resources with device-managed APIs
ALSA: ice1712: Allocate resources with device-managed APIs
ALSA: ice1724: Allocate resources with device-managed APIs
ALSA: korg1212: Allocate resources with device-managed APIs
ALSA: lola: Allocate resources with device-managed APIs
ALSA: oxygen: Allocate resources with device-managed APIs
ALSA: riptide: Allocate resources with device-managed APIs
ALSA: hdsp: Allocate resources with device-managed APIs
ALSA: hdspm: Allocate resources with device-managed APIs
ALSA: rme9652: Allocate resources with device-managed APIs
ALSA: trident: Allocate resources with device-managed APIs
ALSA: vx: Manage vx_core object with devres
ALSA: vx222: Allocate resources with device-managed APIs
ALSA: ymfpci: Allocate resources with device-managed APIs
ALSA: x86: Allocate resources with device-managed APIs
ALSA: virmidi: Allocate resources with device-managed APIs
ALSA: mtpav: Allocate resources with device-managed APIs
ALSA: serial-u16550: Allocate resources with device-managed APIs
ALSA: mpu401: Allocate resources with device-managed APIs
.../kernel-api/writing-an-alsa-driver.rst | 33 +++
include/sound/core.h | 5 +
include/sound/emu10k1.h | 6 +-
include/sound/memalloc.h | 4 +
sound/core/init.c | 101 ++++++++-
sound/core/memalloc.c | 46 ++++
sound/drivers/mpu401/mpu401.c | 33 +--
sound/drivers/mtpav.c | 30 +--
sound/drivers/serial-u16550.c | 57 +----
sound/drivers/virmidi.c | 21 +-
sound/drivers/vx/vx_core.c | 12 +-
sound/pci/ad1889.c | 144 +++---------
sound/pci/ali5451/ali5451.c | 88 ++------
sound/pci/als300.c | 79 ++-----
sound/pci/als4000.c | 59 ++---
sound/pci/atiixp.c | 92 ++------
sound/pci/atiixp_modem.c | 92 ++------
sound/pci/au88x0/au88x0.c | 134 +++--------
sound/pci/aw2/aw2-alsa.c | 102 ++-------
sound/pci/azt3328.c | 124 +++-------
sound/pci/bt87x.c | 97 ++------
sound/pci/ca0106/ca0106.h | 3 +-
sound/pci/ca0106/ca0106_main.c | 114 +++-------
sound/pci/cmipci.c | 104 +++------
sound/pci/cs4281.c | 112 ++--------
sound/pci/cs46xx/cs46xx.c | 51 ++---
sound/pci/cs46xx/cs46xx.h | 4 +-
sound/pci/cs46xx/cs46xx_lib.c | 109 ++-------
sound/pci/cs5530.c | 86 ++-----
sound/pci/cs5535audio/cs5535audio.c | 94 ++------
sound/pci/cs5535audio/cs5535audio_olpc.c | 7 +-
sound/pci/echoaudio/echoaudio.c | 168 ++++----------
sound/pci/echoaudio/echoaudio.h | 2 +-
sound/pci/emu10k1/emu10k1.c | 53 ++---
sound/pci/emu10k1/emu10k1_main.c | 102 +++------
sound/pci/emu10k1/emu10k1x.c | 128 +++--------
sound/pci/emu10k1/p16v.c | 22 +-
sound/pci/ens1370.c | 115 +++-------
sound/pci/es1938.c | 97 ++------
sound/pci/es1968.c | 112 ++--------
sound/pci/fm801.c | 103 ++-------
sound/pci/hda/hda_controller.h | 1 -
sound/pci/hda/hda_intel.c | 26 +--
sound/pci/ice1712/ice1712.c | 133 +++--------
sound/pci/ice1712/ice1724.c | 127 +++--------
sound/pci/intel8x0.c | 140 ++++--------
sound/pci/intel8x0m.c | 139 ++++--------
sound/pci/korg1212/korg1212.c | 211 +++++-------------
sound/pci/lola/lola.c | 127 +++--------
sound/pci/lola/lola.h | 5 +-
sound/pci/lola/lola_pcm.c | 20 +-
sound/pci/maestro3.c | 106 ++-------
sound/pci/oxygen/oxygen.c | 1 -
sound/pci/oxygen/oxygen.h | 1 -
sound/pci/oxygen/oxygen_lib.c | 66 ++----
sound/pci/oxygen/se6x.c | 1 -
sound/pci/oxygen/virtuoso.c | 1 -
sound/pci/riptide/riptide.c | 89 ++------
sound/pci/rme32.c | 49 +---
sound/pci/rme96.c | 57 ++---
sound/pci/rme9652/hdsp.c | 89 +++-----
sound/pci/rme9652/hdspm.c | 64 +-----
sound/pci/rme9652/rme9652.c | 85 ++-----
sound/pci/sis7019.c | 87 ++------
sound/pci/sonicvibes.c | 117 +++-------
sound/pci/trident/trident.c | 39 +---
sound/pci/trident/trident.h | 7 +-
sound/pci/trident/trident_main.c | 90 +++-----
sound/pci/trident/trident_memory.c | 8 +-
sound/pci/via82xx.c | 106 +++------
sound/pci/via82xx_modem.c | 86 ++-----
sound/pci/vx222/vx222.c | 69 +-----
sound/pci/ymfpci/ymfpci.c | 66 +++---
sound/pci/ymfpci/ymfpci.h | 8 +-
sound/pci/ymfpci/ymfpci_main.c | 149 ++++---------
sound/pcmcia/vx/vxpocket.c | 21 --
sound/x86/intel_hdmi_audio.c | 57 ++---
77 files changed, 1449 insertions(+), 3944 deletions(-)
--
2.26.2
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 01/51] ALSA: core: Add device-managed page allocator helper
2021-07-13 14:24 [PATCH 00/51] ALSA: More devres usages Takashi Iwai
@ 2021-07-13 14:24 ` Takashi Iwai
2021-07-13 14:24 ` [PATCH 02/51] ALSA: core: Add managed card creation Takashi Iwai
` (8 subsequent siblings)
9 siblings, 0 replies; 15+ messages in thread
From: Takashi Iwai @ 2021-07-13 14:24 UTC (permalink / raw)
To: alsa-devel
This is a preparation for allowing devres usages more widely in
various sound drivers. As a first step, this patch adds a new
allocator function, snd_devm_alloc_pages(), to manage the allocated
pages via devres, so that the pages will be automagically released as
device unbinding.
Unlike the old snd_dma_alloc_pages(), the new function returns
directly the snd_dma_buffer pointer. The caller needs NULL-check for
the allocation error appropriately.
Also, since a real device pointer is mandatory for devres,
SNDRV_DMA_TYPE_CONTINUOUS or SNDRV_DMA_TYPE_VMALLOC type can't be used
for this function.
Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
include/sound/memalloc.h | 4 ++++
sound/core/memalloc.c | 46 ++++++++++++++++++++++++++++++++++++++++
2 files changed, 50 insertions(+)
diff --git a/include/sound/memalloc.h b/include/sound/memalloc.h
index 44d87775b352..d22c9387b2ba 100644
--- a/include/sound/memalloc.h
+++ b/include/sound/memalloc.h
@@ -79,5 +79,9 @@ struct page *snd_sgbuf_get_page(struct snd_dma_buffer *dmab, size_t offset);
unsigned int snd_sgbuf_get_chunk_size(struct snd_dma_buffer *dmab,
unsigned int ofs, unsigned int size);
+/* device-managed memory allocator */
+struct snd_dma_buffer *snd_devm_alloc_pages(struct device *dev, int type,
+ size_t size);
+
#endif /* __SOUND_MEMALLOC_H */
diff --git a/sound/core/memalloc.c b/sound/core/memalloc.c
index 83b79edfa52d..3a78fdad1ab4 100644
--- a/sound/core/memalloc.c
+++ b/sound/core/memalloc.c
@@ -127,6 +127,52 @@ void snd_dma_free_pages(struct snd_dma_buffer *dmab)
}
EXPORT_SYMBOL(snd_dma_free_pages);
+/* called by devres */
+static void __snd_release_pages(struct device *dev, void *res)
+{
+ snd_dma_free_pages(res);
+}
+
+/**
+ * snd_devm_alloc_pages - allocate the buffer and manage with devres
+ * @dev: the device pointer
+ * @type: the DMA buffer type
+ * @size: the buffer size to allocate
+ *
+ * Allocate buffer pages depending on the given type and manage using devres.
+ * The pages will be released automatically at the device removal.
+ *
+ * Unlike snd_dma_alloc_pages(), this function requires the real device pointer,
+ * hence it can't work with SNDRV_DMA_TYPE_CONTINUOUS or
+ * SNDRV_DMA_TYPE_VMALLOC type.
+ *
+ * The function returns the snd_dma_buffer object at success, or NULL if failed.
+ */
+struct snd_dma_buffer *
+snd_devm_alloc_pages(struct device *dev, int type, size_t size)
+{
+ struct snd_dma_buffer *dmab;
+ int err;
+
+ if (WARN_ON(type == SNDRV_DMA_TYPE_CONTINUOUS ||
+ type == SNDRV_DMA_TYPE_VMALLOC))
+ return NULL;
+
+ dmab = devres_alloc(__snd_release_pages, sizeof(*dmab), GFP_KERNEL);
+ if (!dmab)
+ return NULL;
+
+ err = snd_dma_alloc_pages(type, dev, size, dmab);
+ if (err < 0) {
+ devres_free(dmab);
+ return NULL;
+ }
+
+ devres_add(dev, dmab);
+ return dmab;
+}
+EXPORT_SYMBOL_GPL(snd_devm_alloc_pages);
+
/**
* snd_dma_buffer_mmap - perform mmap of the given DMA buffer
* @dmab: buffer allocation information
--
2.26.2
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH 02/51] ALSA: core: Add managed card creation
2021-07-13 14:24 [PATCH 00/51] ALSA: More devres usages Takashi Iwai
2021-07-13 14:24 ` [PATCH 01/51] ALSA: core: Add device-managed page allocator helper Takashi Iwai
@ 2021-07-13 14:24 ` Takashi Iwai
2021-07-13 14:24 ` [PATCH 03/51] ALSA: intel8x0: Allocate resources with device-managed APIs Takashi Iwai
` (7 subsequent siblings)
9 siblings, 0 replies; 15+ messages in thread
From: Takashi Iwai @ 2021-07-13 14:24 UTC (permalink / raw)
To: alsa-devel
As a second step for preliminary to widen the devres usages among
sound drivers, this patch adds a new ALSA core API function,
snd_devm_card_new(), to create a snd_card object via devres.
When a card object is created by this new function, snd_card_free() is
called automatically and the card object resource gets released at the
device unbinding time.
However, the story isn't that simple. A caveat is that we have to
call snd_card_free() at the very first of the whole resource release
procedure, in order to assure that the all exposed devices on
user-space are deleted and sync with processes accessing those devices
before releasing resources.
For achieving it, snd_card_register() adds a new devres action to
trigger snd_card_free() automatically when the given card object is a
"managed" one. Since usually snd_card_register() is the last step of
the initialization, this should work in most cases.
With all these tricks, some drivers can get rid of the whole driver
remove callback code.
About a bit of implementation details: the patch adds two new flags to
snd_card object: managed and releasing. The former indicates that the
object was created via snd_devm_card_new(), and the latter is used for
avoiding the double-free of snd_card_free() calls. Both flags are
fairly internal and likely uninteresting to normal users.
Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
include/sound/core.h | 5 +++
sound/core/init.c | 101 +++++++++++++++++++++++++++++++++++++++----
2 files changed, 98 insertions(+), 8 deletions(-)
diff --git a/include/sound/core.h b/include/sound/core.h
index c4ade121727d..331195b51237 100644
--- a/include/sound/core.h
+++ b/include/sound/core.h
@@ -117,6 +117,8 @@ struct snd_card {
struct device card_dev; /* cardX object for sysfs */
const struct attribute_group *dev_groups[4]; /* assigned sysfs attr */
bool registered; /* card_dev is registered? */
+ bool managed; /* managed via devres */
+ bool releasing; /* during card free process */
int sync_irq; /* assigned irq, used for PCM sync */
wait_queue_head_t remove_sleep;
@@ -274,6 +276,9 @@ extern int (*snd_mixer_oss_notify_callback)(struct snd_card *card, int cmd);
int snd_card_new(struct device *parent, int idx, const char *xid,
struct module *module, int extra_size,
struct snd_card **card_ret);
+int snd_devm_card_new(struct device *parent, int idx, const char *xid,
+ struct module *module, int extra_size,
+ struct snd_card **card_ret);
int snd_card_disconnect(struct snd_card *card);
void snd_card_disconnect_sync(struct snd_card *card);
diff --git a/sound/core/init.c b/sound/core/init.c
index 1490568efdb0..7a507e8d24c1 100644
--- a/sound/core/init.c
+++ b/sound/core/init.c
@@ -134,6 +134,9 @@ void snd_device_initialize(struct device *dev, struct snd_card *card)
}
EXPORT_SYMBOL_GPL(snd_device_initialize);
+static int snd_card_init(struct snd_card *card, struct device *parent,
+ int idx, const char *xid, struct module *module,
+ int extra_size);
static int snd_card_do_free(struct snd_card *card);
static const struct attribute_group card_dev_attr_group;
@@ -163,9 +166,6 @@ int snd_card_new(struct device *parent, int idx, const char *xid,
{
struct snd_card *card;
int err;
-#ifdef CONFIG_SND_DEBUG
- char name[8];
-#endif
if (snd_BUG_ON(!card_ret))
return -EINVAL;
@@ -176,6 +176,76 @@ int snd_card_new(struct device *parent, int idx, const char *xid,
card = kzalloc(sizeof(*card) + extra_size, GFP_KERNEL);
if (!card)
return -ENOMEM;
+
+ err = snd_card_init(card, parent, idx, xid, module, extra_size);
+ if (err < 0) {
+ kfree(card);
+ return err;
+ }
+
+ *card_ret = card;
+ return 0;
+}
+EXPORT_SYMBOL(snd_card_new);
+
+static void __snd_card_release(struct device *dev, void *data)
+{
+ snd_card_free(data);
+}
+
+/**
+ * snd_devm_card_new - managed snd_card object creation
+ * @parent: the parent device object
+ * @idx: card index (address) [0 ... (SNDRV_CARDS-1)]
+ * @xid: card identification (ASCII string)
+ * @module: top level module for locking
+ * @extra_size: allocate this extra size after the main soundcard structure
+ * @card_ret: the pointer to store the created card instance
+ *
+ * This function works like snd_card_new() but manages the allocated resource
+ * via devres, i.e. you don't need to free explicitly.
+ *
+ * When a snd_card object is created with this function and registered via
+ * snd_card_register(), the very first devres action to call snd_card_free()
+ * is added automatically. In that way, the resource disconnection is assured
+ * at first, then released in the expected order.
+ */
+int snd_devm_card_new(struct device *parent, int idx, const char *xid,
+ struct module *module, int extra_size,
+ struct snd_card **card_ret)
+{
+ struct snd_card *card;
+ int err;
+
+ *card_ret = NULL;
+ if (extra_size < 0)
+ extra_size = 0;
+ card = devres_alloc(__snd_card_release, sizeof(*card) + extra_size,
+ GFP_KERNEL);
+ if (!card)
+ return -ENOMEM;
+ card->managed = true;
+ err = snd_card_init(card, parent, idx, xid, module, extra_size);
+ if (err < 0) {
+ devres_free(card);
+ return err;
+ }
+
+ devres_add(parent, card);
+ *card_ret = card;
+ return 0;
+}
+EXPORT_SYMBOL_GPL(snd_devm_card_new);
+
+static int snd_card_init(struct snd_card *card, struct device *parent,
+ int idx, const char *xid, struct module *module,
+ int extra_size)
+{
+ int err;
+#ifdef CONFIG_SND_DEBUG
+ char name[8];
+#endif
+
if (extra_size > 0)
card->private_data = (char *)card + sizeof(struct snd_card);
if (xid)
@@ -197,7 +267,6 @@ int snd_card_new(struct device *parent, int idx, const char *xid,
mutex_unlock(&snd_card_mutex);
dev_err(parent, "cannot find the slot for index %d (range 0-%i), error: %d\n",
idx, snd_ecards_limit - 1, err);
- kfree(card);
return err;
}
set_bit(idx, snd_cards_lock); /* lock it */
@@ -256,8 +325,6 @@ int snd_card_new(struct device *parent, int idx, const char *xid,
sprintf(name, "card%d", idx);
card->debugfs_root = debugfs_create_dir(name, sound_debugfs_root);
#endif
-
- *card_ret = card;
return 0;
__error_ctl:
@@ -266,7 +333,6 @@ int snd_card_new(struct device *parent, int idx, const char *xid,
put_device(&card->card_dev);
return err;
}
-EXPORT_SYMBOL(snd_card_new);
/**
* snd_card_ref - Get the card object from the index
@@ -481,6 +547,7 @@ EXPORT_SYMBOL_GPL(snd_card_disconnect_sync);
static int snd_card_do_free(struct snd_card *card)
{
+ card->releasing = true;
#if IS_ENABLED(CONFIG_SND_MIXER_OSS)
if (snd_mixer_oss_notify_callback)
snd_mixer_oss_notify_callback(card, SND_MIXER_OSS_NOTIFY_FREE);
@@ -498,7 +565,8 @@ static int snd_card_do_free(struct snd_card *card)
#endif
if (card->release_completion)
complete(card->release_completion);
- kfree(card);
+ if (!card->managed)
+ kfree(card);
return 0;
}
@@ -539,6 +607,9 @@ int snd_card_free(struct snd_card *card)
DECLARE_COMPLETION_ONSTACK(released);
int ret;
+ if (card->releasing)
+ return 0;
+
card->release_completion = &released;
ret = snd_card_free_when_closed(card);
if (ret)
@@ -745,6 +816,11 @@ int snd_card_add_dev_attr(struct snd_card *card,
}
EXPORT_SYMBOL_GPL(snd_card_add_dev_attr);
+static void trigger_card_free(void *data)
+{
+ snd_card_free(data);
+}
+
/**
* snd_card_register - register the soundcard
* @card: soundcard structure
@@ -768,6 +844,15 @@ int snd_card_register(struct snd_card *card)
if (err < 0)
return err;
card->registered = true;
+ } else {
+ if (card->managed)
+ devm_remove_action(card->dev, trigger_card_free, card);
+ }
+
+ if (card->managed) {
+ err = devm_add_action(card->dev, trigger_card_free, card);
+ if (err < 0)
+ return err;
}
err = snd_device_register_all(card);
--
2.26.2
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH 03/51] ALSA: intel8x0: Allocate resources with device-managed APIs
2021-07-13 14:24 [PATCH 00/51] ALSA: More devres usages Takashi Iwai
2021-07-13 14:24 ` [PATCH 01/51] ALSA: core: Add device-managed page allocator helper Takashi Iwai
2021-07-13 14:24 ` [PATCH 02/51] ALSA: core: Add managed card creation Takashi Iwai
@ 2021-07-13 14:24 ` Takashi Iwai
2021-07-13 14:24 ` [PATCH 04/51] ALSA: atiixp: " Takashi Iwai
` (6 subsequent siblings)
9 siblings, 0 replies; 15+ messages in thread
From: Takashi Iwai @ 2021-07-13 14:24 UTC (permalink / raw)
To: alsa-devel
This patch refactors the intel8x0 and intel8x0m driver codes using
devres and gets rid of the driver remove callback.
The conversion is fairly straightforward: each API call is replaced
with the device-managed API function, e.g. pci_enable_device() ->
pcim_enable_device(), and so on. The buffer descriptor list is
allocated with a new API, snd_devm_alloc_pages().
A slight code structure change is that the intel8x0 object is
allocated as a card's private_data instead of the own lowlevel
snd_device object. This simplifies the resource management.
And, the take-down procedure is triggered via card->private_free, and
it's registered at the end of the whole initialization, i.e. after the
all resources get properly managed.
The only not-devres-managed resource is the irq handler. Since we
need to release at suspend and re-acquire at resume (otherwise
something weird happens on some machines), this is still managed
manually. But the rest are all freed automatically.
The end result is a good amount of code reduction.
Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
sound/pci/intel8x0.c | 140 +++++++++++++-----------------------------
sound/pci/intel8x0m.c | 139 ++++++++++++-----------------------------
2 files changed, 81 insertions(+), 198 deletions(-)
diff --git a/sound/pci/intel8x0.c b/sound/pci/intel8x0.c
index 6a436a2ce26a..67cb65a53edd 100644
--- a/sound/pci/intel8x0.c
+++ b/sound/pci/intel8x0.c
@@ -378,7 +378,7 @@ struct intel8x0 {
spinlock_t reg_lock;
u32 bdbars_count;
- struct snd_dma_buffer bdbars;
+ struct snd_dma_buffer *bdbars;
u32 int_sta_reg; /* interrupt status register */
u32 int_sta_mask; /* interrupt status mask */
};
@@ -2528,8 +2528,9 @@ static int snd_intel8x0_chip_init(struct intel8x0 *chip, int probing)
return 0;
}
-static int snd_intel8x0_free(struct intel8x0 *chip)
+static void snd_intel8x0_free(struct snd_card *card)
{
+ struct intel8x0 *chip = card->private_data;
unsigned int i;
if (chip->irq < 0)
@@ -2552,16 +2553,6 @@ static int snd_intel8x0_free(struct intel8x0 *chip)
__hw_end:
if (chip->irq >= 0)
free_irq(chip->irq, chip);
- if (chip->bdbars.area)
- snd_dma_free_pages(&chip->bdbars);
- if (chip->addr)
- pci_iounmap(chip->pci, chip->addr);
- if (chip->bmaddr)
- pci_iounmap(chip->pci, chip->bmaddr);
- pci_release_regions(chip->pci);
- pci_disable_device(chip->pci);
- kfree(chip);
- return 0;
}
#ifdef CONFIG_PM_SLEEP
@@ -2831,12 +2822,6 @@ static void snd_intel8x0_proc_init(struct intel8x0 *chip)
snd_intel8x0_proc_read);
}
-static int snd_intel8x0_dev_free(struct snd_device *device)
-{
- struct intel8x0 *chip = device->device_data;
- return snd_intel8x0_free(chip);
-}
-
struct ich_reg_info {
unsigned int int_sta_mask;
unsigned int offset;
@@ -2880,19 +2865,15 @@ static int snd_intel8x0_inside_vm(struct pci_dev *pci)
return result;
}
-static int snd_intel8x0_create(struct snd_card *card,
- struct pci_dev *pci,
- unsigned long device_type,
- struct intel8x0 **r_intel8x0)
+static int snd_intel8x0_init(struct snd_card *card,
+ struct pci_dev *pci,
+ unsigned long device_type)
{
- struct intel8x0 *chip;
+ struct intel8x0 *chip = card->private_data;
int err;
unsigned int i;
unsigned int int_sta_masks;
struct ichdev *ichdev;
- static const struct snd_device_ops ops = {
- .dev_free = snd_intel8x0_dev_free,
- };
static const unsigned int bdbars[] = {
3, /* DEVICE_INTEL */
@@ -2925,17 +2906,10 @@ static int snd_intel8x0_create(struct snd_card *card,
};
const struct ich_reg_info *tbl;
- *r_intel8x0 = NULL;
-
- err = pci_enable_device(pci);
+ err = pcim_enable_device(pci);
if (err < 0)
return err;
- chip = kzalloc(sizeof(*chip), GFP_KERNEL);
- if (chip == NULL) {
- pci_disable_device(pci);
- return -ENOMEM;
- }
spin_lock_init(&chip->reg_lock);
chip->device_type = device_type;
chip->card = card;
@@ -2961,38 +2935,23 @@ static int snd_intel8x0_create(struct snd_card *card,
chip->fix_nocache = 1; /* enable workaround */
err = pci_request_regions(pci, card->shortname);
- if (err < 0) {
- kfree(chip);
- pci_disable_device(pci);
+ if (err < 0)
return err;
- }
if (device_type == DEVICE_ALI) {
/* ALI5455 has no ac97 region */
- chip->bmaddr = pci_iomap(pci, 0, 0);
- goto port_inited;
- }
-
- if (pci_resource_flags(pci, 2) & IORESOURCE_MEM) /* ICH4 and Nforce */
- chip->addr = pci_iomap(pci, 2, 0);
- else
- chip->addr = pci_iomap(pci, 0, 0);
- if (!chip->addr) {
- dev_err(card->dev, "AC'97 space ioremap problem\n");
- snd_intel8x0_free(chip);
- return -EIO;
+ chip->bmaddr = pcim_iomap(pci, 0, 0);
+ } else {
+ if (pci_resource_flags(pci, 2) & IORESOURCE_MEM) /* ICH4 and Nforce */
+ chip->addr = pcim_iomap(pci, 2, 0);
+ else
+ chip->addr = pcim_iomap(pci, 0, 0);
+ if (pci_resource_flags(pci, 3) & IORESOURCE_MEM) /* ICH4 */
+ chip->bmaddr = pcim_iomap(pci, 3, 0);
+ else
+ chip->bmaddr = pcim_iomap(pci, 1, 0);
}
- if (pci_resource_flags(pci, 3) & IORESOURCE_MEM) /* ICH4 */
- chip->bmaddr = pci_iomap(pci, 3, 0);
- else
- chip->bmaddr = pci_iomap(pci, 1, 0);
- port_inited:
- if (!chip->bmaddr) {
- dev_err(card->dev, "Controller space ioremap problem\n");
- snd_intel8x0_free(chip);
- return -EIO;
- }
chip->bdbars_count = bdbars[device_type];
/* initialize offsets */
@@ -3028,21 +2987,20 @@ static int snd_intel8x0_create(struct snd_card *card,
/* allocate buffer descriptor lists */
/* the start of each lists must be aligned to 8 bytes */
- if (snd_dma_alloc_pages(intel8x0_dma_type(chip), &pci->dev,
- chip->bdbars_count * sizeof(u32) * ICH_MAX_FRAGS * 2,
- &chip->bdbars) < 0) {
- snd_intel8x0_free(chip);
- dev_err(card->dev, "cannot allocate buffer descriptors\n");
+ chip->bdbars = snd_devm_alloc_pages(&pci->dev, intel8x0_dma_type(chip),
+ chip->bdbars_count * sizeof(u32) *
+ ICH_MAX_FRAGS * 2);
+ if (!chip->bdbars)
return -ENOMEM;
- }
+
/* tables must be aligned to 8 bytes here, but the kernel pages
are much bigger, so we don't care (on i386) */
int_sta_masks = 0;
for (i = 0; i < chip->bdbars_count; i++) {
ichdev = &chip->ichd[i];
- ichdev->bdbar = ((__le32 *)chip->bdbars.area) +
+ ichdev->bdbar = ((__le32 *)chip->bdbars->area) +
(i * ICH_MAX_FRAGS * 2);
- ichdev->bdbar_addr = chip->bdbars.addr +
+ ichdev->bdbar_addr = chip->bdbars->addr +
(i * sizeof(u32) * ICH_MAX_FRAGS * 2);
int_sta_masks |= ichdev->int_sta_mask;
}
@@ -3076,28 +3034,24 @@ static int snd_intel8x0_create(struct snd_card *card,
chip->codec_isr_bits |= chip->codec_bit[i];
err = snd_intel8x0_chip_init(chip, 1);
- if (err < 0) {
- snd_intel8x0_free(chip);
+ if (err < 0)
return err;
- }
/* request irq after initializaing int_sta_mask, etc */
+ /* NOTE: we don't use devm version here since it's released /
+ * re-acquired in PM callbacks.
+ * It's released explicitly in snd_intel8x0_free(), too.
+ */
if (request_irq(pci->irq, snd_intel8x0_interrupt,
IRQF_SHARED, KBUILD_MODNAME, chip)) {
dev_err(card->dev, "unable to grab IRQ %d\n", pci->irq);
- snd_intel8x0_free(chip);
return -EBUSY;
}
chip->irq = pci->irq;
card->sync_irq = chip->irq;
- err = snd_device_new(card, SNDRV_DEV_LOWLEVEL, chip, &ops);
- if (err < 0) {
- snd_intel8x0_free(chip);
- return err;
- }
+ card->private_free = snd_intel8x0_free;
- *r_intel8x0 = chip;
return 0;
}
@@ -3163,9 +3117,11 @@ static int snd_intel8x0_probe(struct pci_dev *pci,
int err;
struct shortname_table *name;
- err = snd_card_new(&pci->dev, index, id, THIS_MODULE, 0, &card);
+ err = snd_devm_card_new(&pci->dev, index, id, THIS_MODULE,
+ sizeof(*chip), &card);
if (err < 0)
return err;
+ chip = card->private_data;
if (spdif_aclink < 0)
spdif_aclink = check_default_spdif_aclink(pci);
@@ -3199,23 +3155,16 @@ static int snd_intel8x0_probe(struct pci_dev *pci,
buggy_irq = 0;
}
- err = snd_intel8x0_create(card, pci, pci_id->driver_data, &chip);
- if (err < 0) {
- snd_card_free(card);
+ err = snd_intel8x0_init(card, pci, pci_id->driver_data);
+ if (err < 0)
return err;
- }
- card->private_data = chip;
err = snd_intel8x0_mixer(chip, ac97_clock, ac97_quirk);
- if (err < 0) {
- snd_card_free(card);
+ if (err < 0)
return err;
- }
err = snd_intel8x0_pcm(chip);
- if (err < 0) {
- snd_card_free(card);
+ if (err < 0)
return err;
- }
snd_intel8x0_proc_init(chip);
@@ -3233,24 +3182,17 @@ static int snd_intel8x0_probe(struct pci_dev *pci,
}
err = snd_card_register(card);
- if (err < 0) {
- snd_card_free(card);
+ if (err < 0)
return err;
- }
+
pci_set_drvdata(pci, card);
return 0;
}
-static void snd_intel8x0_remove(struct pci_dev *pci)
-{
- snd_card_free(pci_get_drvdata(pci));
-}
-
static struct pci_driver intel8x0_driver = {
.name = KBUILD_MODNAME,
.id_table = snd_intel8x0_ids,
.probe = snd_intel8x0_probe,
- .remove = snd_intel8x0_remove,
.driver = {
.pm = INTEL8X0_PM_OPS,
},
diff --git a/sound/pci/intel8x0m.c b/sound/pci/intel8x0m.c
index b96fce6cbd83..7de3cb2f17b5 100644
--- a/sound/pci/intel8x0m.c
+++ b/sound/pci/intel8x0m.c
@@ -182,7 +182,7 @@ struct intel8x0m {
spinlock_t reg_lock;
- struct snd_dma_buffer bdbars;
+ struct snd_dma_buffer *bdbars;
u32 bdbars_count;
u32 int_sta_reg; /* interrupt status register */
u32 int_sta_mask; /* interrupt status mask */
@@ -947,8 +947,9 @@ static int snd_intel8x0m_chip_init(struct intel8x0m *chip, int probing)
return 0;
}
-static int snd_intel8x0m_free(struct intel8x0m *chip)
+static void snd_intel8x0m_free(struct snd_card *card)
{
+ struct intel8x0m *chip = card->private_data;
unsigned int i;
if (chip->irq < 0)
@@ -962,16 +963,6 @@ static int snd_intel8x0m_free(struct intel8x0m *chip)
__hw_end:
if (chip->irq >= 0)
free_irq(chip->irq, chip);
- if (chip->bdbars.area)
- snd_dma_free_pages(&chip->bdbars);
- if (chip->addr)
- pci_iounmap(chip->pci, chip->addr);
- if (chip->bmaddr)
- pci_iounmap(chip->pci, chip->bmaddr);
- pci_release_regions(chip->pci);
- pci_disable_device(chip->pci);
- kfree(chip);
- return 0;
}
#ifdef CONFIG_PM_SLEEP
@@ -1047,47 +1038,30 @@ static void snd_intel8x0m_proc_init(struct intel8x0m *chip)
snd_intel8x0m_proc_read);
}
-static int snd_intel8x0m_dev_free(struct snd_device *device)
-{
- struct intel8x0m *chip = device->device_data;
- return snd_intel8x0m_free(chip);
-}
-
struct ich_reg_info {
unsigned int int_sta_mask;
unsigned int offset;
};
-static int snd_intel8x0m_create(struct snd_card *card,
- struct pci_dev *pci,
- unsigned long device_type,
- struct intel8x0m **r_intel8x0m)
+static int snd_intel8x0m_init(struct snd_card *card,
+ struct pci_dev *pci,
+ unsigned long device_type)
{
- struct intel8x0m *chip;
+ struct intel8x0m *chip = card->private_data;
int err;
unsigned int i;
unsigned int int_sta_masks;
struct ichdev *ichdev;
- static const struct snd_device_ops ops = {
- .dev_free = snd_intel8x0m_dev_free,
- };
static const struct ich_reg_info intel_regs[2] = {
{ ICH_MIINT, 0 },
{ ICH_MOINT, 0x10 },
};
const struct ich_reg_info *tbl;
- *r_intel8x0m = NULL;
-
- err = pci_enable_device(pci);
+ err = pcim_enable_device(pci);
if (err < 0)
return err;
- chip = kzalloc(sizeof(*chip), GFP_KERNEL);
- if (chip == NULL) {
- pci_disable_device(pci);
- return -ENOMEM;
- }
spin_lock_init(&chip->reg_lock);
chip->device_type = device_type;
chip->card = card;
@@ -1095,37 +1069,21 @@ static int snd_intel8x0m_create(struct snd_card *card,
chip->irq = -1;
err = pci_request_regions(pci, card->shortname);
- if (err < 0) {
- kfree(chip);
- pci_disable_device(pci);
+ if (err < 0)
return err;
- }
if (device_type == DEVICE_ALI) {
/* ALI5455 has no ac97 region */
- chip->bmaddr = pci_iomap(pci, 0, 0);
- goto port_inited;
- }
-
- if (pci_resource_flags(pci, 2) & IORESOURCE_MEM) /* ICH4 and Nforce */
- chip->addr = pci_iomap(pci, 2, 0);
- else
- chip->addr = pci_iomap(pci, 0, 0);
- if (!chip->addr) {
- dev_err(card->dev, "AC'97 space ioremap problem\n");
- snd_intel8x0m_free(chip);
- return -EIO;
- }
- if (pci_resource_flags(pci, 3) & IORESOURCE_MEM) /* ICH4 */
- chip->bmaddr = pci_iomap(pci, 3, 0);
- else
- chip->bmaddr = pci_iomap(pci, 1, 0);
-
-port_inited:
- if (!chip->bmaddr) {
- dev_err(card->dev, "Controller space ioremap problem\n");
- snd_intel8x0m_free(chip);
- return -EIO;
+ chip->bmaddr = pcim_iomap(pci, 0, 0);
+ } else {
+ if (pci_resource_flags(pci, 2) & IORESOURCE_MEM) /* ICH4 and Nforce */
+ chip->addr = pcim_iomap(pci, 2, 0);
+ else
+ chip->addr = pcim_iomap(pci, 0, 0);
+ if (pci_resource_flags(pci, 3) & IORESOURCE_MEM) /* ICH4 */
+ chip->bmaddr = pcim_iomap(pci, 3, 0);
+ else
+ chip->bmaddr = pcim_iomap(pci, 1, 0);
}
/* initialize offsets */
@@ -1153,19 +1111,19 @@ static int snd_intel8x0m_create(struct snd_card *card,
/* allocate buffer descriptor lists */
/* the start of each lists must be aligned to 8 bytes */
- if (snd_dma_alloc_pages(SNDRV_DMA_TYPE_DEV, &pci->dev,
- chip->bdbars_count * sizeof(u32) * ICH_MAX_FRAGS * 2,
- &chip->bdbars) < 0) {
- snd_intel8x0m_free(chip);
+ chip->bdbars = snd_devm_alloc_pages(&pci->dev, SNDRV_DMA_TYPE_DEV,
+ chip->bdbars_count * sizeof(u32) *
+ ICH_MAX_FRAGS * 2);
+ if (!chip->bdbars)
return -ENOMEM;
- }
+
/* tables must be aligned to 8 bytes here, but the kernel pages
are much bigger, so we don't care (on i386) */
int_sta_masks = 0;
for (i = 0; i < chip->bdbars_count; i++) {
ichdev = &chip->ichd[i];
- ichdev->bdbar = ((__le32 *)chip->bdbars.area) + (i * ICH_MAX_FRAGS * 2);
- ichdev->bdbar_addr = chip->bdbars.addr + (i * sizeof(u32) * ICH_MAX_FRAGS * 2);
+ ichdev->bdbar = ((__le32 *)chip->bdbars->area) + (i * ICH_MAX_FRAGS * 2);
+ ichdev->bdbar_addr = chip->bdbars->addr + (i * sizeof(u32) * ICH_MAX_FRAGS * 2);
int_sta_masks |= ichdev->int_sta_mask;
}
chip->int_sta_reg = ICH_REG_GLOB_STA;
@@ -1174,27 +1132,23 @@ static int snd_intel8x0m_create(struct snd_card *card,
pci_set_master(pci);
err = snd_intel8x0m_chip_init(chip, 1);
- if (err < 0) {
- snd_intel8x0m_free(chip);
+ if (err < 0)
return err;
- }
+ /* NOTE: we don't use devm version here since it's released /
+ * re-acquired in PM callbacks.
+ * It's released explicitly in snd_intel8x0m_free(), too.
+ */
if (request_irq(pci->irq, snd_intel8x0m_interrupt, IRQF_SHARED,
KBUILD_MODNAME, chip)) {
dev_err(card->dev, "unable to grab IRQ %d\n", pci->irq);
- snd_intel8x0m_free(chip);
return -EBUSY;
}
chip->irq = pci->irq;
card->sync_irq = chip->irq;
- err = snd_device_new(card, SNDRV_DEV_LOWLEVEL, chip, &ops);
- if (err < 0) {
- snd_intel8x0m_free(chip);
- return err;
- }
+ card->private_free = snd_intel8x0m_free;
- *r_intel8x0m = chip;
return 0;
}
@@ -1232,9 +1186,11 @@ static int snd_intel8x0m_probe(struct pci_dev *pci,
int err;
struct shortname_table *name;
- err = snd_card_new(&pci->dev, index, id, THIS_MODULE, 0, &card);
+ err = snd_devm_card_new(&pci->dev, index, id, THIS_MODULE,
+ sizeof(*chip), &card);
if (err < 0)
return err;
+ chip = card->private_data;
strcpy(card->driver, "ICH-MODEM");
strcpy(card->shortname, "Intel ICH");
@@ -1246,23 +1202,16 @@ static int snd_intel8x0m_probe(struct pci_dev *pci,
}
strcat(card->shortname," Modem");
- err = snd_intel8x0m_create(card, pci, pci_id->driver_data, &chip);
- if (err < 0) {
- snd_card_free(card);
+ err = snd_intel8x0m_init(card, pci, pci_id->driver_data);
+ if (err < 0)
return err;
- }
- card->private_data = chip;
err = snd_intel8x0m_mixer(chip, ac97_clock);
- if (err < 0) {
- snd_card_free(card);
+ if (err < 0)
return err;
- }
err = snd_intel8x0m_pcm(chip);
- if (err < 0) {
- snd_card_free(card);
+ if (err < 0)
return err;
- }
snd_intel8x0m_proc_init(chip);
@@ -1270,24 +1219,16 @@ static int snd_intel8x0m_probe(struct pci_dev *pci,
card->shortname, chip->irq);
err = snd_card_register(card);
- if (err < 0) {
- snd_card_free(card);
+ if (err < 0)
return err;
- }
pci_set_drvdata(pci, card);
return 0;
}
-static void snd_intel8x0m_remove(struct pci_dev *pci)
-{
- snd_card_free(pci_get_drvdata(pci));
-}
-
static struct pci_driver intel8x0m_driver = {
.name = KBUILD_MODNAME,
.id_table = snd_intel8x0m_ids,
.probe = snd_intel8x0m_probe,
- .remove = snd_intel8x0m_remove,
.driver = {
.pm = INTEL8X0M_PM_OPS,
},
--
2.26.2
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH 04/51] ALSA: atiixp: Allocate resources with device-managed APIs
2021-07-13 14:24 [PATCH 00/51] ALSA: More devres usages Takashi Iwai
` (2 preceding siblings ...)
2021-07-13 14:24 ` [PATCH 03/51] ALSA: intel8x0: Allocate resources with device-managed APIs Takashi Iwai
@ 2021-07-13 14:24 ` Takashi Iwai
2021-07-13 14:24 ` [PATCH 05/51] ALSA: hda: " Takashi Iwai
` (5 subsequent siblings)
9 siblings, 0 replies; 15+ messages in thread
From: Takashi Iwai @ 2021-07-13 14:24 UTC (permalink / raw)
To: alsa-devel
Like the previous patch, this patch converts the resource allocations
with device-managed API calls, so that we can reduce resource-free
calls.
The atiixp drivers are simpler than intel8x0, and even the irq can be
allocated with devres.
The end result is a good amount of code reduction.
Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
sound/pci/atiixp.c | 92 +++++++++-------------------------------
sound/pci/atiixp_modem.c | 92 +++++++++-------------------------------
2 files changed, 40 insertions(+), 144 deletions(-)
diff --git a/sound/pci/atiixp.c b/sound/pci/atiixp.c
index 5f8aa35c4bea..b8e035d5930d 100644
--- a/sound/pci/atiixp.c
+++ b/sound/pci/atiixp.c
@@ -1530,87 +1530,44 @@ static void snd_atiixp_proc_init(struct atiixp *chip)
* destructor
*/
-static int snd_atiixp_free(struct atiixp *chip)
+static void snd_atiixp_free(struct snd_card *card)
{
- if (chip->irq < 0)
- goto __hw_end;
- snd_atiixp_chip_stop(chip);
-
- __hw_end:
- if (chip->irq >= 0)
- free_irq(chip->irq, chip);
- iounmap(chip->remap_addr);
- pci_release_regions(chip->pci);
- pci_disable_device(chip->pci);
- kfree(chip);
- return 0;
-}
-
-static int snd_atiixp_dev_free(struct snd_device *device)
-{
- struct atiixp *chip = device->device_data;
- return snd_atiixp_free(chip);
+ snd_atiixp_chip_stop(card->private_data);
}
/*
* constructor for chip instance
*/
-static int snd_atiixp_create(struct snd_card *card,
- struct pci_dev *pci,
- struct atiixp **r_chip)
+static int snd_atiixp_init(struct snd_card *card, struct pci_dev *pci)
{
- static const struct snd_device_ops ops = {
- .dev_free = snd_atiixp_dev_free,
- };
- struct atiixp *chip;
+ struct atiixp *chip = card->private_data;
int err;
- err = pci_enable_device(pci);
+ err = pcim_enable_device(pci);
if (err < 0)
return err;
- chip = kzalloc(sizeof(*chip), GFP_KERNEL);
- if (chip == NULL) {
- pci_disable_device(pci);
- return -ENOMEM;
- }
-
spin_lock_init(&chip->reg_lock);
mutex_init(&chip->open_mutex);
chip->card = card;
chip->pci = pci;
chip->irq = -1;
- err = pci_request_regions(pci, "ATI IXP AC97");
- if (err < 0) {
- pci_disable_device(pci);
- kfree(chip);
+ err = pcim_iomap_regions(pci, 1 << 0, "ATI IXP AC97");
+ if (err < 0)
return err;
- }
chip->addr = pci_resource_start(pci, 0);
- chip->remap_addr = pci_ioremap_bar(pci, 0);
- if (chip->remap_addr == NULL) {
- dev_err(card->dev, "AC'97 space ioremap problem\n");
- snd_atiixp_free(chip);
- return -EIO;
- }
+ chip->remap_addr = pcim_iomap_table(pci)[0];
- if (request_irq(pci->irq, snd_atiixp_interrupt, IRQF_SHARED,
- KBUILD_MODNAME, chip)) {
+ if (devm_request_irq(&pci->dev, pci->irq, snd_atiixp_interrupt,
+ IRQF_SHARED, KBUILD_MODNAME, chip)) {
dev_err(card->dev, "unable to grab IRQ %d\n", pci->irq);
- snd_atiixp_free(chip);
return -EBUSY;
}
chip->irq = pci->irq;
card->sync_irq = chip->irq;
+ card->private_free = snd_atiixp_free;
pci_set_master(pci);
- err = snd_device_new(card, SNDRV_DEV_LOWLEVEL, chip, &ops);
- if (err < 0) {
- snd_atiixp_free(chip);
- return err;
- }
-
- *r_chip = chip;
return 0;
}
@@ -1622,30 +1579,31 @@ static int snd_atiixp_probe(struct pci_dev *pci,
struct atiixp *chip;
int err;
- err = snd_card_new(&pci->dev, index, id, THIS_MODULE, 0, &card);
+ err = snd_devm_card_new(&pci->dev, index, id, THIS_MODULE,
+ sizeof(*chip), &card);
if (err < 0)
return err;
+ chip = card->private_data;
strcpy(card->driver, spdif_aclink ? "ATIIXP" : "ATIIXP-SPDMA");
strcpy(card->shortname, "ATI IXP");
- err = snd_atiixp_create(card, pci, &chip);
+ err = snd_atiixp_init(card, pci);
if (err < 0)
- goto __error;
- card->private_data = chip;
+ return err;
err = snd_atiixp_aclink_reset(chip);
if (err < 0)
- goto __error;
+ return err;
chip->spdif_over_aclink = spdif_aclink;
err = snd_atiixp_mixer_new(chip, ac97_clock, ac97_quirk);
if (err < 0)
- goto __error;
+ return err;
err = snd_atiixp_pcm_new(chip);
if (err < 0)
- goto __error;
+ return err;
snd_atiixp_proc_init(chip);
@@ -1659,26 +1617,16 @@ static int snd_atiixp_probe(struct pci_dev *pci,
err = snd_card_register(card);
if (err < 0)
- goto __error;
+ return err;
pci_set_drvdata(pci, card);
return 0;
-
- __error:
- snd_card_free(card);
- return err;
-}
-
-static void snd_atiixp_remove(struct pci_dev *pci)
-{
- snd_card_free(pci_get_drvdata(pci));
}
static struct pci_driver atiixp_driver = {
.name = KBUILD_MODNAME,
.id_table = snd_atiixp_ids,
.probe = snd_atiixp_probe,
- .remove = snd_atiixp_remove,
.driver = {
.pm = SND_ATIIXP_PM_OPS,
},
diff --git a/sound/pci/atiixp_modem.c b/sound/pci/atiixp_modem.c
index 9739c3a82777..178dce8ef1e9 100644
--- a/sound/pci/atiixp_modem.c
+++ b/sound/pci/atiixp_modem.c
@@ -1159,87 +1159,44 @@ static void snd_atiixp_proc_init(struct atiixp_modem *chip)
* destructor
*/
-static int snd_atiixp_free(struct atiixp_modem *chip)
+static void snd_atiixp_free(struct snd_card *card)
{
- if (chip->irq < 0)
- goto __hw_end;
- snd_atiixp_chip_stop(chip);
-
- __hw_end:
- if (chip->irq >= 0)
- free_irq(chip->irq, chip);
- iounmap(chip->remap_addr);
- pci_release_regions(chip->pci);
- pci_disable_device(chip->pci);
- kfree(chip);
- return 0;
-}
-
-static int snd_atiixp_dev_free(struct snd_device *device)
-{
- struct atiixp_modem *chip = device->device_data;
- return snd_atiixp_free(chip);
+ snd_atiixp_chip_stop(card->private_data);
}
/*
* constructor for chip instance
*/
-static int snd_atiixp_create(struct snd_card *card,
- struct pci_dev *pci,
- struct atiixp_modem **r_chip)
+static int snd_atiixp_init(struct snd_card *card, struct pci_dev *pci)
{
- static const struct snd_device_ops ops = {
- .dev_free = snd_atiixp_dev_free,
- };
- struct atiixp_modem *chip;
+ struct atiixp_modem *chip = card->private_data;
int err;
- err = pci_enable_device(pci);
+ err = pcim_enable_device(pci);
if (err < 0)
return err;
- chip = kzalloc(sizeof(*chip), GFP_KERNEL);
- if (chip == NULL) {
- pci_disable_device(pci);
- return -ENOMEM;
- }
-
spin_lock_init(&chip->reg_lock);
mutex_init(&chip->open_mutex);
chip->card = card;
chip->pci = pci;
chip->irq = -1;
- err = pci_request_regions(pci, "ATI IXP MC97");
- if (err < 0) {
- kfree(chip);
- pci_disable_device(pci);
+ err = pcim_iomap_regions(pci, 1 << 0, "ATI IXP MC97");
+ if (err < 0)
return err;
- }
chip->addr = pci_resource_start(pci, 0);
- chip->remap_addr = pci_ioremap_bar(pci, 0);
- if (chip->remap_addr == NULL) {
- dev_err(card->dev, "AC'97 space ioremap problem\n");
- snd_atiixp_free(chip);
- return -EIO;
- }
+ chip->remap_addr = pcim_iomap_table(pci)[0];
- if (request_irq(pci->irq, snd_atiixp_interrupt, IRQF_SHARED,
- KBUILD_MODNAME, chip)) {
+ if (devm_request_irq(&pci->dev, pci->irq, snd_atiixp_interrupt,
+ IRQF_SHARED, KBUILD_MODNAME, chip)) {
dev_err(card->dev, "unable to grab IRQ %d\n", pci->irq);
- snd_atiixp_free(chip);
return -EBUSY;
}
chip->irq = pci->irq;
card->sync_irq = chip->irq;
+ card->private_free = snd_atiixp_free;
pci_set_master(pci);
- err = snd_device_new(card, SNDRV_DEV_LOWLEVEL, chip, &ops);
- if (err < 0) {
- snd_atiixp_free(chip);
- return err;
- }
-
- *r_chip = chip;
return 0;
}
@@ -1251,28 +1208,29 @@ static int snd_atiixp_probe(struct pci_dev *pci,
struct atiixp_modem *chip;
int err;
- err = snd_card_new(&pci->dev, index, id, THIS_MODULE, 0, &card);
+ err = snd_devm_card_new(&pci->dev, index, id, THIS_MODULE,
+ sizeof(*chip), &card);
if (err < 0)
return err;
+ chip = card->private_data;
strcpy(card->driver, "ATIIXP-MODEM");
strcpy(card->shortname, "ATI IXP Modem");
- err = snd_atiixp_create(card, pci, &chip);
+ err = snd_atiixp_init(card, pci);
if (err < 0)
- goto __error;
- card->private_data = chip;
+ return err;
err = snd_atiixp_aclink_reset(chip);
if (err < 0)
- goto __error;
+ return err;
err = snd_atiixp_mixer_new(chip, ac97_clock);
if (err < 0)
- goto __error;
+ return err;
err = snd_atiixp_pcm_new(chip);
if (err < 0)
- goto __error;
+ return err;
snd_atiixp_proc_init(chip);
@@ -1283,26 +1241,16 @@ static int snd_atiixp_probe(struct pci_dev *pci,
err = snd_card_register(card);
if (err < 0)
- goto __error;
+ return err;
pci_set_drvdata(pci, card);
return 0;
-
- __error:
- snd_card_free(card);
- return err;
-}
-
-static void snd_atiixp_remove(struct pci_dev *pci)
-{
- snd_card_free(pci_get_drvdata(pci));
}
static struct pci_driver atiixp_modem_driver = {
.name = KBUILD_MODNAME,
.id_table = snd_atiixp_ids,
.probe = snd_atiixp_probe,
- .remove = snd_atiixp_remove,
.driver = {
.pm = SND_ATIIXP_PM_OPS,
},
--
2.26.2
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH 05/51] ALSA: hda: Allocate resources with device-managed APIs
2021-07-13 14:24 [PATCH 00/51] ALSA: More devres usages Takashi Iwai
` (3 preceding siblings ...)
2021-07-13 14:24 ` [PATCH 04/51] ALSA: atiixp: " Takashi Iwai
@ 2021-07-13 14:24 ` Takashi Iwai
2021-07-13 14:24 ` [PATCH 06/51] ALSA: doc: Add device-managed resource section Takashi Iwai
` (4 subsequent siblings)
9 siblings, 0 replies; 15+ messages in thread
From: Takashi Iwai @ 2021-07-13 14:24 UTC (permalink / raw)
To: alsa-devel
This patch is an attempt to slightly simplify the resource management
in HD-audio code, by using some device-managed APIs. Only a few
resources like PCI enablement and PCI resources managed via devres,
but most of the rest code dealing with HD-audio core stuff couldn't be
changed so much, hence the changes in this patch are pretty small in
the end. A special caveat is needed for the card object: we can't
move the card object release into devres, because the driver is
involved with the component stuff and its unregistiration doesn't work
well from devres release at all.
Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
sound/pci/hda/hda_controller.h | 1 -
sound/pci/hda/hda_intel.c | 26 +++++---------------------
2 files changed, 5 insertions(+), 22 deletions(-)
diff --git a/sound/pci/hda/hda_controller.h b/sound/pci/hda/hda_controller.h
index 68f9668788ea..e4b290de81de 100644
--- a/sound/pci/hda/hda_controller.h
+++ b/sound/pci/hda/hda_controller.h
@@ -141,7 +141,6 @@ struct azx {
unsigned int snoop:1;
unsigned int uc_buffer:1; /* non-cached pages for stream buffers */
unsigned int align_buffer_size:1;
- unsigned int region_requested:1;
unsigned int disabled:1; /* disabled by vga_switcheroo */
unsigned int pm_prepared:1;
diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
index 0322b289505e..4d64f2ce30dc 100644
--- a/sound/pci/hda/hda_intel.c
+++ b/sound/pci/hda/hda_intel.c
@@ -1367,18 +1367,11 @@ static void azx_free(struct azx *chip)
if (bus->irq >= 0)
free_irq(bus->irq, (void*)chip);
- if (chip->msi)
- pci_disable_msi(chip->pci);
- iounmap(bus->remap_addr);
azx_free_stream_pages(chip);
azx_free_streams(chip);
snd_hdac_bus_exit(bus);
- if (chip->region_requested)
- pci_release_regions(chip->pci);
-
- pci_disable_device(chip->pci);
#ifdef CONFIG_SND_HDA_PATCH_LOADER
release_firmware(chip->fw);
#endif
@@ -1767,15 +1760,13 @@ static int azx_create(struct snd_card *card, struct pci_dev *pci,
*rchip = NULL;
- err = pci_enable_device(pci);
+ err = pcim_enable_device(pci);
if (err < 0)
return err;
hda = devm_kzalloc(&pci->dev, sizeof(*hda), GFP_KERNEL);
- if (!hda) {
- pci_disable_device(pci);
+ if (!hda)
return -ENOMEM;
- }
chip = &hda->chip;
mutex_init(&chip->open_mutex);
@@ -1811,10 +1802,8 @@ static int azx_create(struct snd_card *card, struct pci_dev *pci,
chip->bdl_pos_adj = bdl_pos_adj[dev];
err = azx_bus_init(chip, model[dev]);
- if (err < 0) {
- pci_disable_device(pci);
+ if (err < 0)
return err;
- }
/* use the non-cached pages in non-snoop mode */
if (!azx_snoop(chip))
@@ -1860,17 +1849,12 @@ static int azx_first_init(struct azx *chip)
}
#endif
- err = pci_request_regions(pci, "ICH HD audio");
+ err = pcim_iomap_regions(pci, 1 << 0, "ICH HD audio");
if (err < 0)
return err;
- chip->region_requested = 1;
bus->addr = pci_resource_start(pci, 0);
- bus->remap_addr = pci_ioremap_bar(pci, 0);
- if (bus->remap_addr == NULL) {
- dev_err(card->dev, "ioremap error\n");
- return -ENXIO;
- }
+ bus->remap_addr = pcim_iomap_table(pci)[0];
if (chip->driver_type == AZX_DRIVER_SKL)
snd_hdac_bus_parse_capabilities(bus);
--
2.26.2
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH 06/51] ALSA: doc: Add device-managed resource section
2021-07-13 14:24 [PATCH 00/51] ALSA: More devres usages Takashi Iwai
` (4 preceding siblings ...)
2021-07-13 14:24 ` [PATCH 05/51] ALSA: hda: " Takashi Iwai
@ 2021-07-13 14:24 ` Takashi Iwai
2021-07-13 14:24 ` [PATCH 07/51] ALSA: ad1889: Allocate resources with device-managed APIs Takashi Iwai
` (3 subsequent siblings)
9 siblings, 0 replies; 15+ messages in thread
From: Takashi Iwai @ 2021-07-13 14:24 UTC (permalink / raw)
To: alsa-devel
Give brief explanations about the device-managed resources and the
newly introduced snd_devm_card_new() helper.
Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
.../kernel-api/writing-an-alsa-driver.rst | 33 +++++++++++++++++++
1 file changed, 33 insertions(+)
diff --git a/Documentation/sound/kernel-api/writing-an-alsa-driver.rst b/Documentation/sound/kernel-api/writing-an-alsa-driver.rst
index 01d59b8aea92..255b7d3bebd6 100644
--- a/Documentation/sound/kernel-api/writing-an-alsa-driver.rst
+++ b/Documentation/sound/kernel-api/writing-an-alsa-driver.rst
@@ -4172,6 +4172,39 @@ module license as GPL, etc., otherwise the system is shown as “tainted”.
MODULE_LICENSE("GPL");
+Device-Managed Resources
+========================
+
+In the examples above, all resources are allocated and released
+manually. But human beings are lazy in nature, especially developers
+are lazier. So there are some ways to automate the release part; it's
+the (device-)managed resources aka devres or devm family. For
+example, an object allocated via :c:func:`devm_kmalloc()` will be
+freed automatically at unbinding the device.
+
+ALSA core provides also the device-managed helper, namely,
+:c:func:`snd_devm_card_new()` for creating a card object.
+Call this functions instead of the normal :c:func:`snd_card_new()`,
+and you can forget the explicit :c:func:`snd_card_free()` call, as
+it's called automagically at error and removal paths.
+
+One caveat is that the call of :c:func:`snd_card_free()` would be put
+at the beginning of the call chain only after you call
+:c:func:`snd_card_register()`.
+
+Also, the ``private_free`` callback is always called at the card free,
+so be careful to put the hardware clean-up procedure in
+``private_free`` callback. It might be called even before you
+actually set up at an earlier error path. For avoiding such an
+invalid initialization, you can set ``private_free`` callback after
+:c:func:`snd_card_register()` call succeeds.
+
+Another thing to be remarked is that you should use device-managed
+helpers for each component as much as possible once when you manage
+the card in that way. Mixing up with the normal and the managed
+resources may screw up the release order.
+
+
How To Put Your Driver Into ALSA Tree
=====================================
--
2.26.2
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH 07/51] ALSA: ad1889: Allocate resources with device-managed APIs
2021-07-13 14:24 [PATCH 00/51] ALSA: More devres usages Takashi Iwai
` (5 preceding siblings ...)
2021-07-13 14:24 ` [PATCH 06/51] ALSA: doc: Add device-managed resource section Takashi Iwai
@ 2021-07-13 14:24 ` Takashi Iwai
2021-07-13 14:24 ` [PATCH 08/51] ALSA: als300: " Takashi Iwai
` (2 subsequent siblings)
9 siblings, 0 replies; 15+ messages in thread
From: Takashi Iwai @ 2021-07-13 14:24 UTC (permalink / raw)
To: alsa-devel
This patch converts the resource management in PCI ad1889 driver with
devres as a clean up. Each manual resource management is converted
with the corresponding devres helper, and the card object release is
managed now via card->private_free instead of a lowlevel snd_device.
Also, the unnecessary ac97 free callbacks are removed, too.
This should give no user-visible functional changes.
Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
sound/pci/ad1889.c | 144 ++++++++-------------------------------------
1 file changed, 25 insertions(+), 119 deletions(-)
diff --git a/sound/pci/ad1889.c b/sound/pci/ad1889.c
index 5c78951dd596..bba4dae8dcc7 100644
--- a/sound/pci/ad1889.c
+++ b/sound/pci/ad1889.c
@@ -740,20 +740,6 @@ snd_ad1889_ac97_xinit(struct snd_ad1889 *chip)
}
-static void
-snd_ad1889_ac97_bus_free(struct snd_ac97_bus *bus)
-{
- struct snd_ad1889 *chip = bus->private_data;
- chip->ac97_bus = NULL;
-}
-
-static void
-snd_ad1889_ac97_free(struct snd_ac97 *ac97)
-{
- struct snd_ad1889 *chip = ac97->private_data;
- chip->ac97 = NULL;
-}
-
static int
snd_ad1889_ac97_init(struct snd_ad1889 *chip, const char *quirk_override)
{
@@ -771,11 +757,8 @@ snd_ad1889_ac97_init(struct snd_ad1889 *chip, const char *quirk_override)
if (err < 0)
return err;
- chip->ac97_bus->private_free = snd_ad1889_ac97_bus_free;
-
memset(&ac97, 0, sizeof(ac97));
ac97.private_data = chip;
- ac97.private_free = snd_ad1889_ac97_free;
ac97.pci = chip->pci;
err = snd_ac97_mixer(chip->ac97_bus, &ac97, &chip->ac97);
@@ -787,11 +770,10 @@ snd_ad1889_ac97_init(struct snd_ad1889 *chip, const char *quirk_override)
return 0;
}
-static int
-snd_ad1889_free(struct snd_ad1889 *chip)
+static void
+snd_ad1889_free(struct snd_card *card)
{
- if (chip->irq < 0)
- goto skip_hw;
+ struct snd_ad1889 *chip = card->private_data;
spin_lock_irq(&chip->lock);
@@ -805,125 +787,60 @@ snd_ad1889_free(struct snd_ad1889 *chip)
ad1889_readl(chip, AD_DMA_DISR); /* flush, dammit! */
spin_unlock_irq(&chip->lock);
-
- if (chip->irq >= 0)
- free_irq(chip->irq, chip);
-
-skip_hw:
- iounmap(chip->iobase);
- pci_release_regions(chip->pci);
- pci_disable_device(chip->pci);
- kfree(chip);
- return 0;
-}
-
-static int
-snd_ad1889_dev_free(struct snd_device *device)
-{
- struct snd_ad1889 *chip = device->device_data;
- return snd_ad1889_free(chip);
-}
-
-static int
-snd_ad1889_init(struct snd_ad1889 *chip)
-{
- ad1889_writew(chip, AD_DS_CCS, AD_DS_CCS_CLKEN); /* turn on clock */
- ad1889_readw(chip, AD_DS_CCS); /* flush posted write */
-
- usleep_range(10000, 11000);
-
- /* enable Master and Target abort interrupts */
- ad1889_writel(chip, AD_DMA_DISR, AD_DMA_DISR_PMAE | AD_DMA_DISR_PTAE);
-
- return 0;
}
static int
-snd_ad1889_create(struct snd_card *card,
- struct pci_dev *pci,
- struct snd_ad1889 **rchip)
+snd_ad1889_create(struct snd_card *card, struct pci_dev *pci)
{
+ struct snd_ad1889 *chip = card->private_data;
int err;
- struct snd_ad1889 *chip;
- static const struct snd_device_ops ops = {
- .dev_free = snd_ad1889_dev_free,
- };
-
- *rchip = NULL;
-
- err = pci_enable_device(pci);
+ err = pcim_enable_device(pci);
if (err < 0)
return err;
/* check PCI availability (32bit DMA) */
if (dma_set_mask_and_coherent(&pci->dev, DMA_BIT_MASK(32))) {
dev_err(card->dev, "error setting 32-bit DMA mask.\n");
- pci_disable_device(pci);
return -ENXIO;
}
- /* allocate chip specific data with zero-filled memory */
- chip = kzalloc(sizeof(*chip), GFP_KERNEL);
- if (!chip) {
- pci_disable_device(pci);
- return -ENOMEM;
- }
-
chip->card = card;
- card->private_data = chip;
chip->pci = pci;
chip->irq = -1;
/* (1) PCI resource allocation */
- err = pci_request_regions(pci, card->driver);
+ err = pcim_iomap_regions(pci, 1 << 0, card->driver);
if (err < 0)
- goto free_and_ret;
+ return err;
chip->bar = pci_resource_start(pci, 0);
- chip->iobase = pci_ioremap_bar(pci, 0);
- if (chip->iobase == NULL) {
- dev_err(card->dev, "unable to reserve region.\n");
- err = -EBUSY;
- goto free_and_ret;
- }
+ chip->iobase = pcim_iomap_table(pci)[0];
pci_set_master(pci);
spin_lock_init(&chip->lock); /* only now can we call ad1889_free */
- if (request_irq(pci->irq, snd_ad1889_interrupt,
- IRQF_SHARED, KBUILD_MODNAME, chip)) {
+ if (devm_request_irq(&pci->dev, pci->irq, snd_ad1889_interrupt,
+ IRQF_SHARED, KBUILD_MODNAME, chip)) {
dev_err(card->dev, "cannot obtain IRQ %d\n", pci->irq);
- snd_ad1889_free(chip);
return -EBUSY;
}
chip->irq = pci->irq;
card->sync_irq = chip->irq;
+ card->private_free = snd_ad1889_free;
/* (2) initialization of the chip hardware */
- err = snd_ad1889_init(chip);
- if (err < 0) {
- snd_ad1889_free(chip);
- return err;
- }
+ ad1889_writew(chip, AD_DS_CCS, AD_DS_CCS_CLKEN); /* turn on clock */
+ ad1889_readw(chip, AD_DS_CCS); /* flush posted write */
- err = snd_device_new(card, SNDRV_DEV_LOWLEVEL, chip, &ops);
- if (err < 0) {
- snd_ad1889_free(chip);
- return err;
- }
+ usleep_range(10000, 11000);
- *rchip = chip;
+ /* enable Master and Target abort interrupts */
+ ad1889_writel(chip, AD_DMA_DISR, AD_DMA_DISR_PMAE | AD_DMA_DISR_PTAE);
return 0;
-
-free_and_ret:
- kfree(chip);
- pci_disable_device(pci);
-
- return err;
}
static int
@@ -944,19 +861,19 @@ snd_ad1889_probe(struct pci_dev *pci,
}
/* (2) */
- err = snd_card_new(&pci->dev, index[devno], id[devno], THIS_MODULE,
- 0, &card);
- /* XXX REVISIT: we can probably allocate chip in this call */
+ err = snd_devm_card_new(&pci->dev, index[devno], id[devno], THIS_MODULE,
+ sizeof(*chip), &card);
if (err < 0)
return err;
+ chip = card->private_data;
strcpy(card->driver, "AD1889");
strcpy(card->shortname, "Analog Devices AD1889");
/* (3) */
- err = snd_ad1889_create(card, pci, &chip);
+ err = snd_ad1889_create(card, pci);
if (err < 0)
- goto free_and_ret;
+ return err;
/* (4) */
sprintf(card->longname, "%s at 0x%lx irq %i",
@@ -966,11 +883,11 @@ snd_ad1889_probe(struct pci_dev *pci,
/* register AC97 mixer */
err = snd_ad1889_ac97_init(chip, ac97_quirk[devno]);
if (err < 0)
- goto free_and_ret;
+ return err;
err = snd_ad1889_pcm_init(chip, 0);
if (err < 0)
- goto free_and_ret;
+ return err;
/* register proc interface */
snd_ad1889_proc_init(chip);
@@ -978,23 +895,13 @@ snd_ad1889_probe(struct pci_dev *pci,
/* (6) */
err = snd_card_register(card);
if (err < 0)
- goto free_and_ret;
+ return err;
/* (7) */
pci_set_drvdata(pci, card);
devno++;
return 0;
-
-free_and_ret:
- snd_card_free(card);
- return err;
-}
-
-static void
-snd_ad1889_remove(struct pci_dev *pci)
-{
- snd_card_free(pci_get_drvdata(pci));
}
static const struct pci_device_id snd_ad1889_ids[] = {
@@ -1007,7 +914,6 @@ static struct pci_driver ad1889_pci_driver = {
.name = KBUILD_MODNAME,
.id_table = snd_ad1889_ids,
.probe = snd_ad1889_probe,
- .remove = snd_ad1889_remove,
};
module_pci_driver(ad1889_pci_driver);
--
2.26.2
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH 08/51] ALSA: als300: Allocate resources with device-managed APIs
2021-07-13 14:24 [PATCH 00/51] ALSA: More devres usages Takashi Iwai
` (6 preceding siblings ...)
2021-07-13 14:24 ` [PATCH 07/51] ALSA: ad1889: Allocate resources with device-managed APIs Takashi Iwai
@ 2021-07-13 14:24 ` Takashi Iwai
2021-07-13 14:24 ` [PATCH 09/51] ALSA: als4000: " Takashi Iwai
2021-07-13 14:27 ` [PATCH 00/51] ALSA: More devres usages Takashi Iwai
9 siblings, 0 replies; 15+ messages in thread
From: Takashi Iwai @ 2021-07-13 14:24 UTC (permalink / raw)
To: alsa-devel
This patch converts the resource management in PCI als300 driver with
devres as a clean up. Each manual resource management is converted
with the corresponding devres helper, and the card object release is
managed now via card->private_free instead of a lowlevel snd_device.
This should give no user-visible functional changes.
Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
sound/pci/als300.c | 79 ++++++++++------------------------------------
1 file changed, 17 insertions(+), 62 deletions(-)
diff --git a/sound/pci/als300.c b/sound/pci/als300.c
index 668008fc21f7..9c94072572a5 100644
--- a/sound/pci/als300.c
+++ b/sound/pci/als300.c
@@ -163,21 +163,11 @@ static void snd_als300_set_irq_flag(struct snd_als300 *chip, int cmd)
snd_als300_gcr_write(chip->port, MISC_CONTROL, tmp);
}
-static int snd_als300_free(struct snd_als300 *chip)
+static void snd_als300_free(struct snd_card *card)
{
- snd_als300_set_irq_flag(chip, IRQ_DISABLE);
- if (chip->irq >= 0)
- free_irq(chip->irq, chip);
- pci_release_regions(chip->pci);
- pci_disable_device(chip->pci);
- kfree(chip);
- return 0;
-}
+ struct snd_als300 *chip = card->private_data;
-static int snd_als300_dev_free(struct snd_device *device)
-{
- struct snd_als300 *chip = device->device_data;
- return snd_als300_free(chip);
+ snd_als300_set_irq_flag(chip, IRQ_DISABLE);
}
static irqreturn_t snd_als300_interrupt(int irq, void *dev_id)
@@ -248,11 +238,6 @@ static irqreturn_t snd_als300plus_interrupt(int irq, void *dev_id)
return IRQ_HANDLED;
}
-static void snd_als300_remove(struct pci_dev *pci)
-{
- snd_card_free(pci_get_drvdata(pci));
-}
-
static unsigned short snd_als300_ac97_read(struct snd_ac97 *ac97,
unsigned short reg)
{
@@ -610,35 +595,22 @@ static void snd_als300_init(struct snd_als300 *chip)
}
static int snd_als300_create(struct snd_card *card,
- struct pci_dev *pci, int chip_type,
- struct snd_als300 **rchip)
+ struct pci_dev *pci, int chip_type)
{
- struct snd_als300 *chip;
+ struct snd_als300 *chip = card->private_data;
void *irq_handler;
int err;
- static const struct snd_device_ops ops = {
- .dev_free = snd_als300_dev_free,
- };
- *rchip = NULL;
-
- err = pci_enable_device(pci);
+ err = pcim_enable_device(pci);
if (err < 0)
return err;
if (dma_set_mask_and_coherent(&pci->dev, DMA_BIT_MASK(28))) {
dev_err(card->dev, "error setting 28bit DMA mask\n");
- pci_disable_device(pci);
return -ENXIO;
}
pci_set_master(pci);
- chip = kzalloc(sizeof(*chip), GFP_KERNEL);
- if (chip == NULL) {
- pci_disable_device(pci);
- return -ENOMEM;
- }
-
chip->card = card;
chip->pci = pci;
chip->irq = -1;
@@ -646,11 +618,9 @@ static int snd_als300_create(struct snd_card *card,
spin_lock_init(&chip->reg_lock);
err = pci_request_regions(pci, "ALS300");
- if (err < 0) {
- kfree(chip);
- pci_disable_device(pci);
+ if (err < 0)
return err;
- }
+
chip->port = pci_resource_start(pci, 0);
if (chip->chip_type == DEVICE_ALS300_PLUS)
@@ -658,38 +628,29 @@ static int snd_als300_create(struct snd_card *card,
else
irq_handler = snd_als300_interrupt;
- if (request_irq(pci->irq, irq_handler, IRQF_SHARED,
- KBUILD_MODNAME, chip)) {
+ if (devm_request_irq(&pci->dev, pci->irq, irq_handler, IRQF_SHARED,
+ KBUILD_MODNAME, chip)) {
dev_err(card->dev, "unable to grab IRQ %d\n", pci->irq);
- snd_als300_free(chip);
return -EBUSY;
}
chip->irq = pci->irq;
card->sync_irq = chip->irq;
+ card->private_free = snd_als300_free;
snd_als300_init(chip);
err = snd_als300_ac97(chip);
if (err < 0) {
dev_err(card->dev, "Could not create ac97\n");
- snd_als300_free(chip);
return err;
}
err = snd_als300_new_pcm(chip);
if (err < 0) {
dev_err(card->dev, "Could not create PCM\n");
- snd_als300_free(chip);
- return err;
- }
-
- err = snd_device_new(card, SNDRV_DEV_LOWLEVEL, chip, &ops);
- if (err < 0) {
- snd_als300_free(chip);
return err;
}
- *rchip = chip;
return 0;
}
@@ -737,20 +698,16 @@ static int snd_als300_probe(struct pci_dev *pci,
return -ENOENT;
}
- err = snd_card_new(&pci->dev, index[dev], id[dev], THIS_MODULE,
- 0, &card);
-
+ err = snd_devm_card_new(&pci->dev, index[dev], id[dev], THIS_MODULE,
+ sizeof(*chip), &card);
if (err < 0)
return err;
chip_type = pci_id->driver_data;
- err = snd_als300_create(card, pci, chip_type, &chip);
- if (err < 0) {
- snd_card_free(card);
+ err = snd_als300_create(card, pci, chip_type);
+ if (err < 0)
return err;
- }
- card->private_data = chip;
strcpy(card->driver, "ALS300");
if (chip->chip_type == DEVICE_ALS300_PLUS)
@@ -764,10 +721,9 @@ static int snd_als300_probe(struct pci_dev *pci,
card->shortname, chip->port, chip->irq);
err = snd_card_register(card);
- if (err < 0) {
- snd_card_free(card);
+ if (err < 0)
return err;
- }
+
pci_set_drvdata(pci, card);
dev++;
return 0;
@@ -777,7 +733,6 @@ static struct pci_driver als300_driver = {
.name = KBUILD_MODNAME,
.id_table = snd_als300_ids,
.probe = snd_als300_probe,
- .remove = snd_als300_remove,
.driver = {
.pm = SND_ALS300_PM_OPS,
},
--
2.26.2
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH 09/51] ALSA: als4000: Allocate resources with device-managed APIs
2021-07-13 14:24 [PATCH 00/51] ALSA: More devres usages Takashi Iwai
` (7 preceding siblings ...)
2021-07-13 14:24 ` [PATCH 08/51] ALSA: als300: " Takashi Iwai
@ 2021-07-13 14:24 ` Takashi Iwai
2021-07-13 14:27 ` [PATCH 00/51] ALSA: More devres usages Takashi Iwai
9 siblings, 0 replies; 15+ messages in thread
From: Takashi Iwai @ 2021-07-13 14:24 UTC (permalink / raw)
To: alsa-devel
This patch converts the resource management in PCI als4000 driver with
devres as a clean up. Each manual resource management is converted
with the corresponding devres helper, and the card object release is
managed now via card->private_free instead of a lowlevel snd_device.
This should give no user-visible functional changes.
Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
sound/pci/als4000.c | 59 +++++++++++++--------------------------------
1 file changed, 17 insertions(+), 42 deletions(-)
diff --git a/sound/pci/als4000.c b/sound/pci/als4000.c
index 509f317ee682..535eccd124be 100644
--- a/sound/pci/als4000.c
+++ b/sound/pci/als4000.c
@@ -746,13 +746,15 @@ static int snd_als4000_create_gameport(struct snd_card_als4000 *acard, int dev)
if (joystick_port[dev] == 1) { /* auto-detect */
for (io_port = 0x200; io_port <= 0x218; io_port += 8) {
- r = request_region(io_port, 8, "ALS4000 gameport");
+ r = devm_request_region(&acard->pci->dev, io_port, 8,
+ "ALS4000 gameport");
if (r)
break;
}
} else {
io_port = joystick_port[dev];
- r = request_region(io_port, 8, "ALS4000 gameport");
+ r = devm_request_region(&acard->pci->dev, io_port, 8,
+ "ALS4000 gameport");
}
if (!r) {
@@ -763,7 +765,6 @@ static int snd_als4000_create_gameport(struct snd_card_als4000 *acard, int dev)
acard->gameport = gp = gameport_allocate_port();
if (!gp) {
dev_err(&acard->pci->dev, "cannot allocate memory for gameport\n");
- release_and_free_resource(r);
return -ENOMEM;
}
@@ -771,7 +772,6 @@ static int snd_als4000_create_gameport(struct snd_card_als4000 *acard, int dev)
gameport_set_phys(gp, "pci%s/gameport0", pci_name(acard->pci));
gameport_set_dev_parent(gp, &acard->pci->dev);
gp->io = io_port;
- gameport_set_port_data(gp, r);
/* Enable legacy joystick port */
snd_als4000_set_addr(acard->iobase, 0, 0, 0, 1);
@@ -784,15 +784,11 @@ static int snd_als4000_create_gameport(struct snd_card_als4000 *acard, int dev)
static void snd_als4000_free_gameport(struct snd_card_als4000 *acard)
{
if (acard->gameport) {
- struct resource *r = gameport_get_port_data(acard->gameport);
-
gameport_unregister_port(acard->gameport);
acard->gameport = NULL;
/* disable joystick */
snd_als4000_set_addr(acard->iobase, 0, 0, 0, 0);
-
- release_and_free_resource(r);
}
}
#else
@@ -808,8 +804,6 @@ static void snd_card_als4000_free( struct snd_card *card )
snd_als4k_gcr_write_addr(acard->iobase, ALS4K_GCR8C_MISC_CTRL, 0);
/* free resources */
snd_als4000_free_gameport(acard);
- pci_release_regions(acard->pci);
- pci_disable_device(acard->pci);
}
static int snd_card_als4000_probe(struct pci_dev *pci,
@@ -832,36 +826,30 @@ static int snd_card_als4000_probe(struct pci_dev *pci,
}
/* enable PCI device */
- err = pci_enable_device(pci);
+ err = pcim_enable_device(pci);
if (err < 0)
return err;
/* check, if we can restrict PCI DMA transfers to 24 bits */
if (dma_set_mask_and_coherent(&pci->dev, DMA_BIT_MASK(24))) {
dev_err(&pci->dev, "architecture does not support 24bit PCI busmaster DMA\n");
- pci_disable_device(pci);
return -ENXIO;
}
err = pci_request_regions(pci, "ALS4000");
- if (err < 0) {
- pci_disable_device(pci);
+ if (err < 0)
return err;
- }
iobase = pci_resource_start(pci, 0);
pci_read_config_word(pci, PCI_COMMAND, &word);
pci_write_config_word(pci, PCI_COMMAND, word | PCI_COMMAND_IO);
pci_set_master(pci);
- err = snd_card_new(&pci->dev, index[dev], id[dev], THIS_MODULE,
- sizeof(*acard) /* private_data: acard */,
- &card);
- if (err < 0) {
- pci_release_regions(pci);
- pci_disable_device(pci);
+ err = snd_devm_card_new(&pci->dev, index[dev], id[dev], THIS_MODULE,
+ sizeof(*acard) /* private_data: acard */,
+ &card);
+ if (err < 0)
return err;
- }
acard = card->private_data;
acard->pci = pci;
@@ -881,7 +869,7 @@ static int snd_card_als4000_probe(struct pci_dev *pci,
SB_HW_ALS4000,
&chip);
if (err < 0)
- goto out_err;
+ return err;
acard->chip = chip;
chip->pci = pci;
@@ -902,7 +890,7 @@ static int snd_card_als4000_probe(struct pci_dev *pci,
if (err < 0) {
dev_err(&pci->dev, "no MPU-401 device at 0x%lx?\n",
iobase + ALS4K_IOB_30_MIDI_DATA);
- goto out_err;
+ return err;
}
/* FIXME: ALS4000 has interesting MPU401 configuration features
* at ALS4K_CR1A_MPU401_UART_MODE_CONTROL
@@ -912,11 +900,11 @@ static int snd_card_als4000_probe(struct pci_dev *pci,
err = snd_als4000_pcm(chip, 0);
if (err < 0)
- goto out_err;
+ return err;
err = snd_sbmixer_new(chip);
if (err < 0)
- goto out_err;
+ return err;
if (snd_opl3_create(card,
iobase + ALS4K_IOB_10_ADLIB_ADDR0,
@@ -928,30 +916,18 @@ static int snd_card_als4000_probe(struct pci_dev *pci,
} else {
err = snd_opl3_hwdep_new(opl3, 0, 1, NULL);
if (err < 0)
- goto out_err;
+ return err;
}
snd_als4000_create_gameport(acard, dev);
err = snd_card_register(card);
if (err < 0)
- goto out_err;
+ return err;
pci_set_drvdata(pci, card);
dev++;
- err = 0;
- goto out;
-
-out_err:
- snd_card_free(card);
-
-out:
- return err;
-}
-
-static void snd_card_als4000_remove(struct pci_dev *pci)
-{
- snd_card_free(pci_get_drvdata(pci));
+ return 0;
}
#ifdef CONFIG_PM_SLEEP
@@ -996,7 +972,6 @@ static struct pci_driver als4000_driver = {
.name = KBUILD_MODNAME,
.id_table = snd_als4000_ids,
.probe = snd_card_als4000_probe,
- .remove = snd_card_als4000_remove,
.driver = {
.pm = SND_ALS4000_PM_OPS,
},
--
2.26.2
^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH 00/51] ALSA: More devres usages
2021-07-13 14:24 [PATCH 00/51] ALSA: More devres usages Takashi Iwai
` (8 preceding siblings ...)
2021-07-13 14:24 ` [PATCH 09/51] ALSA: als4000: " Takashi Iwai
@ 2021-07-13 14:27 ` Takashi Iwai
9 siblings, 0 replies; 15+ messages in thread
From: Takashi Iwai @ 2021-07-13 14:27 UTC (permalink / raw)
To: alsa-devel
On Tue, 13 Jul 2021 16:24:36 +0200,
Takashi Iwai wrote:
>
> Hi,
>
> this is a resurrect of my early RFC patch with the fix and applying to
> more drivers. The main purpose is merely the code cleanup over the
> tree by replacing the resource allocations with devres helpers as much
> as possible.
>
> The only major change is the addition of devres-supported card object
> handling snd_devm_card_new() and the page allocation helper
> snd_devm_alloc_pages(). Those allow us to get rid of the remove
> callback completely in many drivers. In the end, the series resulted
> in a significant amount of code reduction.
>
> This series covers only the drivers in pci, x86 and drivers
> subdirectories. The patches for isa subdirectory will follow later.
>
>
> Takashi
>
> ===
>
> Takashi Iwai (51):
> ALSA: core: Add device-managed page allocator helper
> ALSA: core: Add managed card creation
> ALSA: intel8x0: Allocate resources with device-managed APIs
> ALSA: atiixp: Allocate resources with device-managed APIs
> ALSA: hda: Allocate resources with device-managed APIs
> ALSA: doc: Add device-managed resource section
> ALSA: ad1889: Allocate resources with device-managed APIs
> ALSA: als300: Allocate resources with device-managed APIs
> ALSA: als4000: Allocate resources with device-managed APIs
> ALSA: azt3328: Allocate resources with device-managed APIs
> ALSA: bt87x: Allocate resources with device-managed APIs
> ALSA: cmipci: Allocate resources with device-managed APIs
> ALSA: cs4281: Allocate resources with device-managed APIs
> ALSA: cs5530: Allocate resources with device-managed APIs
> ALSA: ens137x: Allocate resources with device-managed APIs
> ALSA: es1938: Allocate resources with device-managed APIs
> ALSA: es1968: Allocate resources with device-managed APIs
> ALSA: fm801: Allocate resources with device-managed APIs
> ALSA: maestro3: Allocate resources with device-managed APIs
> ALSA: rme32: Allocate resources with device-managed APIs
> ALSA: rme96: Allocate resources with device-managed APIs
> ALSA: sis7019: Allocate resources with device-managed APIs
> ALSA: sonicvibes: Allocate resources with device-managed APIs
> ALSA: via82xx: Allocate resources with device-managed APIs
> ALSA: ali5451: Allocate resources with device-managed APIs
> ALSA: au88x0: Allocate resources with device-managed APIs
> ALSA: aw2: Allocate resources with device-managed APIs
> ALSA: ca0106: Allocate resources with device-managed APIs
> ALSA: cs46xx: Allocate resources with device-managed APIs
> ALSA: cs5535audio: Allocate resources with device-managed APIs
> ALSA: echoaudio: Allocate resources with device-managed APIs
> ALSA: emu10k1: Allocate resources with device-managed APIs
> ALSA: emu10k1x: Allocate resources with device-managed APIs
> ALSA: ice1712: Allocate resources with device-managed APIs
> ALSA: ice1724: Allocate resources with device-managed APIs
> ALSA: korg1212: Allocate resources with device-managed APIs
> ALSA: lola: Allocate resources with device-managed APIs
> ALSA: oxygen: Allocate resources with device-managed APIs
> ALSA: riptide: Allocate resources with device-managed APIs
> ALSA: hdsp: Allocate resources with device-managed APIs
> ALSA: hdspm: Allocate resources with device-managed APIs
> ALSA: rme9652: Allocate resources with device-managed APIs
> ALSA: trident: Allocate resources with device-managed APIs
> ALSA: vx: Manage vx_core object with devres
> ALSA: vx222: Allocate resources with device-managed APIs
> ALSA: ymfpci: Allocate resources with device-managed APIs
> ALSA: x86: Allocate resources with device-managed APIs
> ALSA: virmidi: Allocate resources with device-managed APIs
> ALSA: mtpav: Allocate resources with device-managed APIs
> ALSA: serial-u16550: Allocate resources with device-managed APIs
> ALSA: mpu401: Allocate resources with device-managed APIs
>
> .../kernel-api/writing-an-alsa-driver.rst | 33 +++
> include/sound/core.h | 5 +
> include/sound/emu10k1.h | 6 +-
> include/sound/memalloc.h | 4 +
> sound/core/init.c | 101 ++++++++-
> sound/core/memalloc.c | 46 ++++
> sound/drivers/mpu401/mpu401.c | 33 +--
> sound/drivers/mtpav.c | 30 +--
> sound/drivers/serial-u16550.c | 57 +----
> sound/drivers/virmidi.c | 21 +-
> sound/drivers/vx/vx_core.c | 12 +-
> sound/pci/ad1889.c | 144 +++---------
> sound/pci/ali5451/ali5451.c | 88 ++------
> sound/pci/als300.c | 79 ++-----
> sound/pci/als4000.c | 59 ++---
> sound/pci/atiixp.c | 92 ++------
> sound/pci/atiixp_modem.c | 92 ++------
> sound/pci/au88x0/au88x0.c | 134 +++--------
> sound/pci/aw2/aw2-alsa.c | 102 ++-------
> sound/pci/azt3328.c | 124 +++-------
> sound/pci/bt87x.c | 97 ++------
> sound/pci/ca0106/ca0106.h | 3 +-
> sound/pci/ca0106/ca0106_main.c | 114 +++-------
> sound/pci/cmipci.c | 104 +++------
> sound/pci/cs4281.c | 112 ++--------
> sound/pci/cs46xx/cs46xx.c | 51 ++---
> sound/pci/cs46xx/cs46xx.h | 4 +-
> sound/pci/cs46xx/cs46xx_lib.c | 109 ++-------
> sound/pci/cs5530.c | 86 ++-----
> sound/pci/cs5535audio/cs5535audio.c | 94 ++------
> sound/pci/cs5535audio/cs5535audio_olpc.c | 7 +-
> sound/pci/echoaudio/echoaudio.c | 168 ++++----------
> sound/pci/echoaudio/echoaudio.h | 2 +-
> sound/pci/emu10k1/emu10k1.c | 53 ++---
> sound/pci/emu10k1/emu10k1_main.c | 102 +++------
> sound/pci/emu10k1/emu10k1x.c | 128 +++--------
> sound/pci/emu10k1/p16v.c | 22 +-
> sound/pci/ens1370.c | 115 +++-------
> sound/pci/es1938.c | 97 ++------
> sound/pci/es1968.c | 112 ++--------
> sound/pci/fm801.c | 103 ++-------
> sound/pci/hda/hda_controller.h | 1 -
> sound/pci/hda/hda_intel.c | 26 +--
> sound/pci/ice1712/ice1712.c | 133 +++--------
> sound/pci/ice1712/ice1724.c | 127 +++--------
> sound/pci/intel8x0.c | 140 ++++--------
> sound/pci/intel8x0m.c | 139 ++++--------
> sound/pci/korg1212/korg1212.c | 211 +++++-------------
> sound/pci/lola/lola.c | 127 +++--------
> sound/pci/lola/lola.h | 5 +-
> sound/pci/lola/lola_pcm.c | 20 +-
> sound/pci/maestro3.c | 106 ++-------
> sound/pci/oxygen/oxygen.c | 1 -
> sound/pci/oxygen/oxygen.h | 1 -
> sound/pci/oxygen/oxygen_lib.c | 66 ++----
> sound/pci/oxygen/se6x.c | 1 -
> sound/pci/oxygen/virtuoso.c | 1 -
> sound/pci/riptide/riptide.c | 89 ++------
> sound/pci/rme32.c | 49 +---
> sound/pci/rme96.c | 57 ++---
> sound/pci/rme9652/hdsp.c | 89 +++-----
> sound/pci/rme9652/hdspm.c | 64 +-----
> sound/pci/rme9652/rme9652.c | 85 ++-----
> sound/pci/sis7019.c | 87 ++------
> sound/pci/sonicvibes.c | 117 +++-------
> sound/pci/trident/trident.c | 39 +---
> sound/pci/trident/trident.h | 7 +-
> sound/pci/trident/trident_main.c | 90 +++-----
> sound/pci/trident/trident_memory.c | 8 +-
> sound/pci/via82xx.c | 106 +++------
> sound/pci/via82xx_modem.c | 86 ++-----
> sound/pci/vx222/vx222.c | 69 +-----
> sound/pci/ymfpci/ymfpci.c | 66 +++---
> sound/pci/ymfpci/ymfpci.h | 8 +-
> sound/pci/ymfpci/ymfpci_main.c | 149 ++++---------
> sound/pcmcia/vx/vxpocket.c | 21 --
> sound/x86/intel_hdmi_audio.c | 57 ++---
> 77 files changed, 1449 insertions(+), 3944 deletions(-)
Oops, the patch submission was truncated mistakenly, so scratch this
thread.
Will resend the whole series. Sorry for inconvenience.
Takashi
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 02/51] ALSA: core: Add managed card creation
2021-07-13 16:05 ` Takashi Iwai
@ 2021-07-13 16:15 ` Takashi Iwai
0 siblings, 0 replies; 15+ messages in thread
From: Takashi Iwai @ 2021-07-13 16:15 UTC (permalink / raw)
To: Amadeusz SX2awiX4ski; +Cc: alsa-devel
On Tue, 13 Jul 2021 18:05:34 +0200,
Takashi Iwai wrote:
>
> On Tue, 13 Jul 2021 17:23:02 +0200,
> Amadeusz SX2awiX4ski wrote:
> >
> > On 7/13/2021 4:28 PM, Takashi Iwai wrote:
> >
> > > /**
> > > * snd_card_ref - Get the card object from the index
> > > @@ -481,6 +547,7 @@ EXPORT_SYMBOL_GPL(snd_card_disconnect_sync);
> > > static int snd_card_do_free(struct snd_card *card)
> > > {
> > > + card->releasing = true;
> > > #if IS_ENABLED(CONFIG_SND_MIXER_OSS)
> > > if (snd_mixer_oss_notify_callback)
> > > snd_mixer_oss_notify_callback(card, SND_MIXER_OSS_NOTIFY_FREE);
> > > @@ -498,7 +565,8 @@ static int snd_card_do_free(struct snd_card *card)
> > > #endif
> > > if (card->release_completion)
> > > complete(card->release_completion);
> > > - kfree(card);
> > > + if (!card->managed)
> > > + kfree(card);
> > > return 0;
> > > }
> > > @@ -539,6 +607,9 @@ int snd_card_free(struct snd_card *card)
> > > DECLARE_COMPLETION_ONSTACK(released);
> > > int ret;
> > > + if (card->releasing)
> > > + return 0;
> > > +
> >
> > "card->releasing" use feels bit racy to me... something like below
> > would break it?
> >
> > thread1 thread2
> > snd_card_free()
> > if(card->releasing) == false
> > thread1 goes sleep
> > snd_card_do_free()
> > card->releasing = true
> > run until the end
> > thread1 resume
> > continues with trying to release
>
> It's a destructor and can't be called in parallel.
That is, what the code above cares is the case where snd_card_free()
is called explicitly even if the card is created with devres. So the
check of card->releasing could be __snd_card_release() instead in
snd_card_free(), too.
Takashi
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 02/51] ALSA: core: Add managed card creation
2021-07-13 15:23 ` Amadeusz Sławiński
@ 2021-07-13 16:05 ` Takashi Iwai
2021-07-13 16:15 ` Takashi Iwai
0 siblings, 1 reply; 15+ messages in thread
From: Takashi Iwai @ 2021-07-13 16:05 UTC (permalink / raw)
To: Amadeusz SX2awiX4ski; +Cc: alsa-devel
On Tue, 13 Jul 2021 17:23:02 +0200,
Amadeusz SX2awiX4ski wrote:
>
> On 7/13/2021 4:28 PM, Takashi Iwai wrote:
>
> ...
>
> > +
> > +/**
> > + * snd_devm_card_new - managed snd_card object creation
> > + * @parent: the parent device object
> > + * @idx: card index (address) [0 ... (SNDRV_CARDS-1)]
> > + * @xid: card identification (ASCII string)
> > + * @module: top level module for locking
> > + * @extra_size: allocate this extra size after the main soundcard structure
> > + * @card_ret: the pointer to store the created card instance
> > + *
> > + * This function works like snd_card_new() but manages the allocated resource
> > + * via devres, i.e. you don't need to free explicitly.
> > + *
> > + * When a snd_card object is created with this function and registered via
> > + * snd_card_register(), the very first devres action to call snd_card_free()
> > + * is added automatically. In that way, the resource disconnection is assured
> > + * at first, then released in the expected order.
> > + */
> > +int snd_devm_card_new(struct device *parent, int idx, const char *xid,
> > + struct module *module, int extra_size,
> > + struct snd_card **card_ret)
> > +{
> > + struct snd_card *card;
> > + int err;
> > +
> > + *card_ret = NULL;
> > + if (extra_size < 0)
> > + extra_size = 0;
> Maybe just make extra_size unsigned or even better size_t?
OK, that would fit for a new function, indeed.
Will modify in v2.
> > /**
> > * snd_card_ref - Get the card object from the index
> > @@ -481,6 +547,7 @@ EXPORT_SYMBOL_GPL(snd_card_disconnect_sync);
> > static int snd_card_do_free(struct snd_card *card)
> > {
> > + card->releasing = true;
> > #if IS_ENABLED(CONFIG_SND_MIXER_OSS)
> > if (snd_mixer_oss_notify_callback)
> > snd_mixer_oss_notify_callback(card, SND_MIXER_OSS_NOTIFY_FREE);
> > @@ -498,7 +565,8 @@ static int snd_card_do_free(struct snd_card *card)
> > #endif
> > if (card->release_completion)
> > complete(card->release_completion);
> > - kfree(card);
> > + if (!card->managed)
> > + kfree(card);
> > return 0;
> > }
> > @@ -539,6 +607,9 @@ int snd_card_free(struct snd_card *card)
> > DECLARE_COMPLETION_ONSTACK(released);
> > int ret;
> > + if (card->releasing)
> > + return 0;
> > +
>
> "card->releasing" use feels bit racy to me... something like below
> would break it?
>
> thread1 thread2
> snd_card_free()
> if(card->releasing) == false
> thread1 goes sleep
> snd_card_do_free()
> card->releasing = true
> run until the end
> thread1 resume
> continues with trying to release
It's a destructor and can't be called in parallel.
> > card->release_completion = &released;
> > ret = snd_card_free_when_closed(card);
> > if (ret)
> > @@ -745,6 +816,11 @@ int snd_card_add_dev_attr(struct snd_card *card,
> > }
> > EXPORT_SYMBOL_GPL(snd_card_add_dev_attr);
> > +static void trigger_card_free(void *data)
> > +{
> > + snd_card_free(data);
> > +}
> > +
> > /**
> > * snd_card_register - register the soundcard
> > * @card: soundcard structure
> > @@ -768,6 +844,15 @@ int snd_card_register(struct snd_card *card)
> > if (err < 0)
> > return err;
> > card->registered = true;
> > + } else {
> > + if (card->managed)
> > + devm_remove_action(card->dev, trigger_card_free, card);
>
> Not sure I understand, we are in _register function, so why do we
> remove action?
snd_card_register() can be called multiple times for re-registering
the newly added components (e.g. usb-audio driver does it). In that
case, we have to move this trigger_card_free action at the head again.
thanks,
Takashi
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 02/51] ALSA: core: Add managed card creation
2021-07-13 14:28 ` [PATCH 02/51] ALSA: core: Add managed card creation Takashi Iwai
@ 2021-07-13 15:23 ` Amadeusz Sławiński
2021-07-13 16:05 ` Takashi Iwai
0 siblings, 1 reply; 15+ messages in thread
From: Amadeusz Sławiński @ 2021-07-13 15:23 UTC (permalink / raw)
To: Takashi Iwai, alsa-devel
On 7/13/2021 4:28 PM, Takashi Iwai wrote:
...
> +
> +/**
> + * snd_devm_card_new - managed snd_card object creation
> + * @parent: the parent device object
> + * @idx: card index (address) [0 ... (SNDRV_CARDS-1)]
> + * @xid: card identification (ASCII string)
> + * @module: top level module for locking
> + * @extra_size: allocate this extra size after the main soundcard structure
> + * @card_ret: the pointer to store the created card instance
> + *
> + * This function works like snd_card_new() but manages the allocated resource
> + * via devres, i.e. you don't need to free explicitly.
> + *
> + * When a snd_card object is created with this function and registered via
> + * snd_card_register(), the very first devres action to call snd_card_free()
> + * is added automatically. In that way, the resource disconnection is assured
> + * at first, then released in the expected order.
> + */
> +int snd_devm_card_new(struct device *parent, int idx, const char *xid,
> + struct module *module, int extra_size,
> + struct snd_card **card_ret)
> +{
> + struct snd_card *card;
> + int err;
> +
> + *card_ret = NULL;
> + if (extra_size < 0)
> + extra_size = 0;
Maybe just make extra_size unsigned or even better size_t?
...
>
> /**
> * snd_card_ref - Get the card object from the index
> @@ -481,6 +547,7 @@ EXPORT_SYMBOL_GPL(snd_card_disconnect_sync);
>
> static int snd_card_do_free(struct snd_card *card)
> {
> + card->releasing = true;
> #if IS_ENABLED(CONFIG_SND_MIXER_OSS)
> if (snd_mixer_oss_notify_callback)
> snd_mixer_oss_notify_callback(card, SND_MIXER_OSS_NOTIFY_FREE);
> @@ -498,7 +565,8 @@ static int snd_card_do_free(struct snd_card *card)
> #endif
> if (card->release_completion)
> complete(card->release_completion);
> - kfree(card);
> + if (!card->managed)
> + kfree(card);
> return 0;
> }
>
> @@ -539,6 +607,9 @@ int snd_card_free(struct snd_card *card)
> DECLARE_COMPLETION_ONSTACK(released);
> int ret;
>
> + if (card->releasing)
> + return 0;
> +
"card->releasing" use feels bit racy to me... something like below would
break it?
thread1 thread2
snd_card_free()
if(card->releasing) == false
thread1 goes sleep
snd_card_do_free()
card->releasing = true
run until the end
thread1 resume
continues with trying to release
> card->release_completion = &released;
> ret = snd_card_free_when_closed(card);
> if (ret)
> @@ -745,6 +816,11 @@ int snd_card_add_dev_attr(struct snd_card *card,
> }
> EXPORT_SYMBOL_GPL(snd_card_add_dev_attr);
>
> +static void trigger_card_free(void *data)
> +{
> + snd_card_free(data);
> +}
> +
> /**
> * snd_card_register - register the soundcard
> * @card: soundcard structure
> @@ -768,6 +844,15 @@ int snd_card_register(struct snd_card *card)
> if (err < 0)
> return err;
> card->registered = true;
> + } else {
> + if (card->managed)
> + devm_remove_action(card->dev, trigger_card_free, card);
Not sure I understand, we are in _register function, so why do we remove
action?
> + }
> +
> + if (card->managed) {
> + err = devm_add_action(card->dev, trigger_card_free, card);
> + if (err < 0)
> + return err;
> }
>
> err = snd_device_register_all(card);
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 02/51] ALSA: core: Add managed card creation
2021-07-13 14:28 [PATCH resent " Takashi Iwai
@ 2021-07-13 14:28 ` Takashi Iwai
2021-07-13 15:23 ` Amadeusz Sławiński
0 siblings, 1 reply; 15+ messages in thread
From: Takashi Iwai @ 2021-07-13 14:28 UTC (permalink / raw)
To: alsa-devel
As a second step for preliminary to widen the devres usages among
sound drivers, this patch adds a new ALSA core API function,
snd_devm_card_new(), to create a snd_card object via devres.
When a card object is created by this new function, snd_card_free() is
called automatically and the card object resource gets released at the
device unbinding time.
However, the story isn't that simple. A caveat is that we have to
call snd_card_free() at the very first of the whole resource release
procedure, in order to assure that the all exposed devices on
user-space are deleted and sync with processes accessing those devices
before releasing resources.
For achieving it, snd_card_register() adds a new devres action to
trigger snd_card_free() automatically when the given card object is a
"managed" one. Since usually snd_card_register() is the last step of
the initialization, this should work in most cases.
With all these tricks, some drivers can get rid of the whole driver
remove callback code.
About a bit of implementation details: the patch adds two new flags to
snd_card object: managed and releasing. The former indicates that the
object was created via snd_devm_card_new(), and the latter is used for
avoiding the double-free of snd_card_free() calls. Both flags are
fairly internal and likely uninteresting to normal users.
Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
include/sound/core.h | 5 +++
sound/core/init.c | 101 +++++++++++++++++++++++++++++++++++++++----
2 files changed, 98 insertions(+), 8 deletions(-)
diff --git a/include/sound/core.h b/include/sound/core.h
index c4ade121727d..331195b51237 100644
--- a/include/sound/core.h
+++ b/include/sound/core.h
@@ -117,6 +117,8 @@ struct snd_card {
struct device card_dev; /* cardX object for sysfs */
const struct attribute_group *dev_groups[4]; /* assigned sysfs attr */
bool registered; /* card_dev is registered? */
+ bool managed; /* managed via devres */
+ bool releasing; /* during card free process */
int sync_irq; /* assigned irq, used for PCM sync */
wait_queue_head_t remove_sleep;
@@ -274,6 +276,9 @@ extern int (*snd_mixer_oss_notify_callback)(struct snd_card *card, int cmd);
int snd_card_new(struct device *parent, int idx, const char *xid,
struct module *module, int extra_size,
struct snd_card **card_ret);
+int snd_devm_card_new(struct device *parent, int idx, const char *xid,
+ struct module *module, int extra_size,
+ struct snd_card **card_ret);
int snd_card_disconnect(struct snd_card *card);
void snd_card_disconnect_sync(struct snd_card *card);
diff --git a/sound/core/init.c b/sound/core/init.c
index 1490568efdb0..7a507e8d24c1 100644
--- a/sound/core/init.c
+++ b/sound/core/init.c
@@ -134,6 +134,9 @@ void snd_device_initialize(struct device *dev, struct snd_card *card)
}
EXPORT_SYMBOL_GPL(snd_device_initialize);
+static int snd_card_init(struct snd_card *card, struct device *parent,
+ int idx, const char *xid, struct module *module,
+ int extra_size);
static int snd_card_do_free(struct snd_card *card);
static const struct attribute_group card_dev_attr_group;
@@ -163,9 +166,6 @@ int snd_card_new(struct device *parent, int idx, const char *xid,
{
struct snd_card *card;
int err;
-#ifdef CONFIG_SND_DEBUG
- char name[8];
-#endif
if (snd_BUG_ON(!card_ret))
return -EINVAL;
@@ -176,6 +176,76 @@ int snd_card_new(struct device *parent, int idx, const char *xid,
card = kzalloc(sizeof(*card) + extra_size, GFP_KERNEL);
if (!card)
return -ENOMEM;
+
+ err = snd_card_init(card, parent, idx, xid, module, extra_size);
+ if (err < 0) {
+ kfree(card);
+ return err;
+ }
+
+ *card_ret = card;
+ return 0;
+}
+EXPORT_SYMBOL(snd_card_new);
+
+static void __snd_card_release(struct device *dev, void *data)
+{
+ snd_card_free(data);
+}
+
+/**
+ * snd_devm_card_new - managed snd_card object creation
+ * @parent: the parent device object
+ * @idx: card index (address) [0 ... (SNDRV_CARDS-1)]
+ * @xid: card identification (ASCII string)
+ * @module: top level module for locking
+ * @extra_size: allocate this extra size after the main soundcard structure
+ * @card_ret: the pointer to store the created card instance
+ *
+ * This function works like snd_card_new() but manages the allocated resource
+ * via devres, i.e. you don't need to free explicitly.
+ *
+ * When a snd_card object is created with this function and registered via
+ * snd_card_register(), the very first devres action to call snd_card_free()
+ * is added automatically. In that way, the resource disconnection is assured
+ * at first, then released in the expected order.
+ */
+int snd_devm_card_new(struct device *parent, int idx, const char *xid,
+ struct module *module, int extra_size,
+ struct snd_card **card_ret)
+{
+ struct snd_card *card;
+ int err;
+
+ *card_ret = NULL;
+ if (extra_size < 0)
+ extra_size = 0;
+ card = devres_alloc(__snd_card_release, sizeof(*card) + extra_size,
+ GFP_KERNEL);
+ if (!card)
+ return -ENOMEM;
+ card->managed = true;
+ err = snd_card_init(card, parent, idx, xid, module, extra_size);
+ if (err < 0) {
+ devres_free(card);
+ return err;
+ }
+
+ devres_add(parent, card);
+ *card_ret = card;
+ return 0;
+}
+EXPORT_SYMBOL_GPL(snd_devm_card_new);
+
+static int snd_card_init(struct snd_card *card, struct device *parent,
+ int idx, const char *xid, struct module *module,
+ int extra_size)
+{
+ int err;
+#ifdef CONFIG_SND_DEBUG
+ char name[8];
+#endif
+
if (extra_size > 0)
card->private_data = (char *)card + sizeof(struct snd_card);
if (xid)
@@ -197,7 +267,6 @@ int snd_card_new(struct device *parent, int idx, const char *xid,
mutex_unlock(&snd_card_mutex);
dev_err(parent, "cannot find the slot for index %d (range 0-%i), error: %d\n",
idx, snd_ecards_limit - 1, err);
- kfree(card);
return err;
}
set_bit(idx, snd_cards_lock); /* lock it */
@@ -256,8 +325,6 @@ int snd_card_new(struct device *parent, int idx, const char *xid,
sprintf(name, "card%d", idx);
card->debugfs_root = debugfs_create_dir(name, sound_debugfs_root);
#endif
-
- *card_ret = card;
return 0;
__error_ctl:
@@ -266,7 +333,6 @@ int snd_card_new(struct device *parent, int idx, const char *xid,
put_device(&card->card_dev);
return err;
}
-EXPORT_SYMBOL(snd_card_new);
/**
* snd_card_ref - Get the card object from the index
@@ -481,6 +547,7 @@ EXPORT_SYMBOL_GPL(snd_card_disconnect_sync);
static int snd_card_do_free(struct snd_card *card)
{
+ card->releasing = true;
#if IS_ENABLED(CONFIG_SND_MIXER_OSS)
if (snd_mixer_oss_notify_callback)
snd_mixer_oss_notify_callback(card, SND_MIXER_OSS_NOTIFY_FREE);
@@ -498,7 +565,8 @@ static int snd_card_do_free(struct snd_card *card)
#endif
if (card->release_completion)
complete(card->release_completion);
- kfree(card);
+ if (!card->managed)
+ kfree(card);
return 0;
}
@@ -539,6 +607,9 @@ int snd_card_free(struct snd_card *card)
DECLARE_COMPLETION_ONSTACK(released);
int ret;
+ if (card->releasing)
+ return 0;
+
card->release_completion = &released;
ret = snd_card_free_when_closed(card);
if (ret)
@@ -745,6 +816,11 @@ int snd_card_add_dev_attr(struct snd_card *card,
}
EXPORT_SYMBOL_GPL(snd_card_add_dev_attr);
+static void trigger_card_free(void *data)
+{
+ snd_card_free(data);
+}
+
/**
* snd_card_register - register the soundcard
* @card: soundcard structure
@@ -768,6 +844,15 @@ int snd_card_register(struct snd_card *card)
if (err < 0)
return err;
card->registered = true;
+ } else {
+ if (card->managed)
+ devm_remove_action(card->dev, trigger_card_free, card);
+ }
+
+ if (card->managed) {
+ err = devm_add_action(card->dev, trigger_card_free, card);
+ if (err < 0)
+ return err;
}
err = snd_device_register_all(card);
--
2.26.2
^ permalink raw reply related [flat|nested] 15+ messages in thread
end of thread, other threads:[~2021-07-13 16:16 UTC | newest]
Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-07-13 14:24 [PATCH 00/51] ALSA: More devres usages Takashi Iwai
2021-07-13 14:24 ` [PATCH 01/51] ALSA: core: Add device-managed page allocator helper Takashi Iwai
2021-07-13 14:24 ` [PATCH 02/51] ALSA: core: Add managed card creation Takashi Iwai
2021-07-13 14:24 ` [PATCH 03/51] ALSA: intel8x0: Allocate resources with device-managed APIs Takashi Iwai
2021-07-13 14:24 ` [PATCH 04/51] ALSA: atiixp: " Takashi Iwai
2021-07-13 14:24 ` [PATCH 05/51] ALSA: hda: " Takashi Iwai
2021-07-13 14:24 ` [PATCH 06/51] ALSA: doc: Add device-managed resource section Takashi Iwai
2021-07-13 14:24 ` [PATCH 07/51] ALSA: ad1889: Allocate resources with device-managed APIs Takashi Iwai
2021-07-13 14:24 ` [PATCH 08/51] ALSA: als300: " Takashi Iwai
2021-07-13 14:24 ` [PATCH 09/51] ALSA: als4000: " Takashi Iwai
2021-07-13 14:27 ` [PATCH 00/51] ALSA: More devres usages Takashi Iwai
2021-07-13 14:28 [PATCH resent " Takashi Iwai
2021-07-13 14:28 ` [PATCH 02/51] ALSA: core: Add managed card creation Takashi Iwai
2021-07-13 15:23 ` Amadeusz Sławiński
2021-07-13 16:05 ` Takashi Iwai
2021-07-13 16:15 ` Takashi Iwai
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.