Re: [PATCH RFC 0/7] Allow multiple callbacks for hda_jack

[David Henningsson <david.henningsson@canonical.com>]

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



On 2014-09-15 14:11, Takashi Iwai wrote:
> At Mon, 15 Sep 2014 10:42:21 +0200,
> Takashi Iwai wrote:
>>
>> At Thu, 11 Sep 2014 17:14:00 +0200,
>> David Henningsson wrote:
>>>
>>>
>>>
>>> On 2014-09-11 16:19, Takashi Iwai wrote:
>>>> Hi,
>>>>
>>>> this is a series of patches I quickly cooked up after the discussion
>>>> in this morning: the support of multiple callbacks per jack.
>>>>
>>>> The series is applied on top of the previous fix patch (ALSA: hda -
>>>> Fix invalid pin powermap without jack detection).  It begins with
>>>> a couple of cleanups, then introduces the new hda_jack_callback
>>>> struct and the changes along with it, then ends with another
>>>> couple of cleanup patches based on the new infrastructure.
>>>>
>>>> I've tested only with a small set of devices, so far.
>>>
>>> In general I like this idea and I remember thinking along the same lines.
>>>
>>> I'm pondering whether we could use a more memory efficient layout for
>>> the callback list. Like allocating a snd_array on codec level and have
>>> indices to that list instead of pointers. Then the kernel would have
>>> less memory blocks to worry about. What do you think?
>>
>> I don't think the memory usage would be any problem in this case as
>> it's just a few numbers of small blocks.  The only question is which
>> is better manageable in the source code level.  Let's see...
>
> I tried hacking with snd_array, but this ended up more complexity in
> the code (either adding an extra stuff into struct hda_codec or
> obviously more overhead than the simple kmalloc).  So, I decided to
> keep the code as it was.
>
> If you find a better solution, let me know.  In anyway, I'll submit v2
> patches.

The attached version is a quick write-up of what I was thinking. 
(Untested, apply on top of patch 4/7.) Whether its better or worse is a 
matter of taste I suppose - the iterating over the callbacks is a little 
less compact, but OTOH the freeing is slightly simpler, and there are 
also less calls to malloc.

Btw, in your version, the current free in snd_hda_jack_tbl_clear is 
within CONFIG_SND_HDA_INPUT_JACK ifdef. Looks like a memory leak if 
CONFIG_SND_HDA_INPUT_JACK is not defined.


-- 
David Henningsson, Canonical Ltd.
https://launchpad.net/~diwic

[-- Attachment #2: jack.diff --]
[-- Type: text/x-patch, Size: 3954 bytes --]

diff --git a/sound/pci/hda/hda_codec.c b/sound/pci/hda/hda_codec.c
index 0aa2e1e..5a88776 100644
--- a/sound/pci/hda/hda_codec.c
+++ b/sound/pci/hda/hda_codec.c
@@ -1471,6 +1471,7 @@ int snd_hda_codec_new(struct hda_bus *bus,
 	snd_array_init(&codec->cvt_setups, sizeof(struct hda_cvt_setup), 8);
 	snd_array_init(&codec->spdif_out, sizeof(struct hda_spdif_out), 16);
 	snd_array_init(&codec->jacktbl, sizeof(struct hda_jack_tbl), 16);
+	snd_array_init(&codec->jack_cb_tbl, sizeof(struct hda_jack_callback), 16);
 	snd_array_init(&codec->verbs, sizeof(struct hda_verb *), 8);
 	INIT_LIST_HEAD(&codec->conn_list);
 
diff --git a/sound/pci/hda/hda_codec.h b/sound/pci/hda/hda_codec.h
index 9c8820f..746cb7c 100644
--- a/sound/pci/hda/hda_codec.h
+++ b/sound/pci/hda/hda_codec.h
@@ -390,6 +390,7 @@ struct hda_codec {
 
 	/* jack detection */
 	struct snd_array jacktbl;
+	struct snd_array jack_cb_tbl;
 	unsigned long jackpoll_interval; /* In jiffies. Zero means no poll, rely on unsol events */
 	struct delayed_work jackpoll_work;
 
diff --git a/sound/pci/hda/hda_jack.c b/sound/pci/hda/hda_jack.c
index cbaa5bf..9336f41 100644
--- a/sound/pci/hda/hda_jack.c
+++ b/sound/pci/hda/hda_jack.c
@@ -105,6 +105,7 @@ snd_hda_jack_tbl_new(struct hda_codec *codec, hda_nid_t nid)
 		return NULL;
 	jack->nid = nid;
 	jack->jack_dirty = 1;
+	jack->callback_idx = -1;
 	jack->tag = codec->jacktbl.used;
 	return jack;
 }
@@ -118,17 +119,13 @@ void snd_hda_jack_tbl_clear(struct hda_codec *codec)
 		struct hda_jack_tbl *jack = codec->jacktbl.list;
 		int i;
 		for (i = 0; i < codec->jacktbl.used; i++, jack++) {
-			struct hda_jack_callback *cb, *next;
 			if (jack->jack)
 				snd_device_free(codec->bus->card, jack->jack);
-			for (cb = jack->callback; cb; cb = next) {
-				next = cb->next;
-				kfree(cb);
-			}
 		}
 	}
 #endif
 	snd_array_free(&codec->jacktbl);
+	snd_array_free(&codec->jack_cb_tbl);
 }
 
 #define get_jack_plug_state(sense) !!(sense & AC_PINSENSE_PRESENCE)
@@ -237,13 +234,13 @@ snd_hda_jack_detect_enable_callback(struct hda_codec *codec, hda_nid_t nid,
 	if (!jack)
 		return ERR_PTR(-ENOMEM);
 	if (func) {
-		callback = kzalloc(sizeof(*callback), GFP_KERNEL);
+		callback = snd_array_new(&codec->jack_cb_tbl);
 		if (!callback)
 			return ERR_PTR(-ENOMEM);
 		callback->func = func;
 		callback->tbl = jack;
-		callback->next = jack->callback;
-		jack->callback = callback;
+		callback->next = jack->callback_idx;
+		jack->callback_idx = snd_array_index(&codec->jack_cb_tbl, callback);
 	}
 
 	if (jack->jack_detect)
@@ -519,16 +516,24 @@ EXPORT_SYMBOL_GPL(snd_hda_jack_add_kctls);
 static void call_jack_callback(struct hda_codec *codec,
 			       struct hda_jack_tbl *jack)
 {
+	int cb_idx = jack->callback_idx;
 	struct hda_jack_callback *cb;
 
-	for (cb = jack->callback; cb; cb = cb->next)
+	while (cb_idx != -1) {
+		cb = snd_array_elem(&codec->jack_cb_tbl, cb_idx);
 		cb->func(codec, cb);
+		cb_idx = cb->next;
+	}
 	if (jack->gated_jack) {
 		struct hda_jack_tbl *gated =
 			snd_hda_jack_tbl_get(codec, jack->gated_jack);
 		if (gated) {
-			for (cb = gated->callback; cb; cb = cb->next)
+			cb_idx = gated->callback_idx;
+			while (cb_idx != -1) {
+				cb = snd_array_elem(&codec->jack_cb_tbl, cb_idx);
 				cb->func(codec, cb);
+				cb_idx = cb->next;
+			}
 		}
 	}
 }
diff --git a/sound/pci/hda/hda_jack.h b/sound/pci/hda/hda_jack.h
index 2784cf5..c634f4e 100644
--- a/sound/pci/hda/hda_jack.h
+++ b/sound/pci/hda/hda_jack.h
@@ -22,13 +22,13 @@ struct hda_jack_callback {
 	struct hda_jack_tbl *tbl;
 	hda_jack_callback_fn func;
 	unsigned int private_data;	/* arbitrary data */
-	struct hda_jack_callback *next;
+	int next;
 };
 
 struct hda_jack_tbl {
 	hda_nid_t nid;
 	unsigned char tag;		/* unsol event tag */
-	struct hda_jack_callback *callback;
+	int callback_idx;
 	/* jack-detection stuff */
 	unsigned int pin_sense;		/* cached pin-sense value */
 	unsigned int jack_detect:1;	/* capable of jack-detection? */

[-- Attachment #3: Type: text/plain, Size: 0 bytes --]