diff mbox

[RFC] dice: simplifying address registration

Message ID 553A3FC5.60809@sakamocchi.jp (mailing list archive)
State New, archived
Headers show

Commit Message

Takashi Sakamoto April 24, 2015, 1:06 p.m. UTC 
Clemens,

Would I request your comments about an attached patch?

Currently Dice driver keeps address ranges for each unit instance. But
there's a way to reuse the same address range. I think this idea can
simplify driver probe processing and save resources of host controller.


Regards

Takashi Sakamoto

Comments

Clemens Ladisch April 25, 2015, 8:49 a.m. UTC | #1 
Takashi Sakamoto wrote:
> Currently Dice driver keeps address ranges for each unit instance. But
> there's a way to reuse the same address range.

This requires an additional check for the packet's source node, which is
essentially a duplicate of the core's address lookup code.

> I think this idea can simplify driver probe processing

Yes, but that code is just moved to the module load processing.

> and save resources of host controller.

When using multiple DICE devices, this saves four bytes of FireWire
address space per card, out of the total address space of 2^48 bytes.
Or what other resources do you mean?


Regards,
Clemens
Takashi Sakamoto April 25, 2015, 12:25 p.m. UTC | #2 
Clemens,

Thanks for your comment.

On Apr 25 2015 17:49, Clemens Ladisch wrote:
> Takashi Sakamoto wrote:
>> Currently Dice driver keeps address ranges for each unit instance. But
>> there's a way to reuse the same address range.
> 
> This requires an additional check for the packet's source node, which is
> essentially a duplicate of the core's address lookup code.
>
>> I think this idea can simplify driver probe processing
> 
> Yes, but that code is just moved to the module load processing.

From probe processing, thus address allocation is just one time, not
each time IEEE 1394 unit is probed.

>> and save resources of host controller.
> 
> When using multiple DICE devices, this saves four bytes of FireWire
> address space per card, out of the total address space of 2^48 bytes.
> Or what other resources do you mean?

Actually, 2^48 bytes are really huge. But there're some rude devices to
transfer transaction to a certain address (i.e. Fireworks). For me, it's
basically better idea to keep allocated address range as small as possible.

...But seems to be not so practical. OK. I dropped this patch.


Thanks

Takashi Sakamoto
diff mbox

Patch

From 93c676c1f2a7b7a78b4de2877e2dfcf97e9e2e94 Mon Sep 17 00:00:00 2001
From: Takashi Sakamoto <o-takashi@sakamocchi.jp>
Date: Fri, 24 Apr 2015 11:25:13 +0900
Subject: [PATCH] ALSA: dice: use the same address space for Dice transaction

Current Dice driver implementation keeps address space for each
device instance. This is a waste of resources in host controller
and a bit complicity in device probing.

This commit applies the same address spaces for each device
instance. The range of address is kept in module initialization
process. When probing devices, the range is registered to
device's register by read transaction, then the device instance
is kept in module local list. When receiving transactions,
the local list is seeked to pick up the instance.

Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
---
 sound/firewire/dice/dice-transaction.c | 128 +++++++++++++++++++++------------
 sound/firewire/dice/dice.c             |  13 +++-
 sound/firewire/dice/dice.h             |   8 ++-
 3 files changed, 96 insertions(+), 53 deletions(-)

diff --git a/sound/firewire/dice/dice-transaction.c b/sound/firewire/dice/dice-transaction.c
index aee74618..0c41193 100644
--- a/sound/firewire/dice/dice-transaction.c
+++ b/sound/firewire/dice/dice-transaction.c
@@ -11,6 +11,12 @@ 
 
 #define NOTIFICATION_TIMEOUT_MS	100
 
+static DEFINE_SPINLOCK(instances_lock);
+static struct snd_dice *instances[SNDRV_CARDS] = SNDRV_DEFAULT_PTR;
+
+static struct fw_address_handler notification_handler;
+static int register_notification_address(struct snd_dice *dice, bool retry);
+
 static u64 get_subaddr(struct snd_dice *dice, enum snd_dice_addr_type type,
 		       u64 offset)
 {
@@ -114,7 +120,7 @@  retry:
 			goto end;
 		}
 
-		err = snd_dice_transaction_reinit(dice);
+		err = register_notification_address(dice, false);
 		if (err < 0)
 			goto end;
 
@@ -204,17 +210,39 @@  static void dice_notification(struct fw_card *card, struct fw_request *request,
 			      int generation, unsigned long long offset,
 			      void *data, size_t length, void *callback_data)
 {
-	struct snd_dice *dice = callback_data;
+	struct snd_dice *dice;
+	struct fw_device *device;
 	u32 bits;
 	unsigned long flags;
+	unsigned int i;
+	int rcode;
 
 	if (tcode != TCODE_WRITE_QUADLET_REQUEST) {
-		fw_send_response(card, request, RCODE_TYPE_ERROR);
-		return;
+		rcode = RCODE_TYPE_ERROR;
+		goto end;
 	}
 	if ((offset & 3) != 0) {
-		fw_send_response(card, request, RCODE_ADDRESS_ERROR);
-		return;
+		rcode = RCODE_ADDRESS_ERROR;
+		goto end;
+	}
+
+	spin_lock_irq(&instances_lock);
+	for (i = 0; i < SNDRV_CARDS; i++) {
+		dice = instances[i];
+		if (dice == NULL)
+			continue;
+		device = fw_parent_device(dice->unit);
+		if (device->card != card ||
+		    device->generation != generation)
+			continue;
+		smp_rmb();	/* node_id vs. generation */
+		if (device->node_id != source)
+			continue;
+		break;
+	}
+	if (i == SNDRV_CARDS) {
+		rcode = RCODE_TYPE_ERROR;
+		goto end;
 	}
 
 	bits = be32_to_cpup(data);
@@ -223,11 +251,15 @@  static void dice_notification(struct fw_card *card, struct fw_request *request,
 	dice->notification_bits |= bits;
 	spin_unlock_irqrestore(&dice->lock, flags);
 
-	fw_send_response(card, request, RCODE_COMPLETE);
-
 	if (bits & NOTIFY_CLOCK_ACCEPTED)
 		complete(&dice->clock_accepted);
 	wake_up(&dice->hwdep_wait);
+
+	spin_unlock_irq(&instances_lock);
+
+	rcode = RCODE_COMPLETE;
+end:
+	fw_send_response(card, request, rcode);
 }
 
 static int register_notification_address(struct snd_dice *dice, bool retry)
@@ -247,7 +279,7 @@  static int register_notification_address(struct snd_dice *dice, bool retry)
 		buffer[0] = cpu_to_be64(OWNER_NO_OWNER);
 		buffer[1] = cpu_to_be64(
 			((u64)device->card->node_id << OWNER_NODE_SHIFT) |
-			dice->notification_handler.offset);
+			notification_handler.offset);
 
 		dice->owner_generation = device->generation;
 		smp_rmb(); /* node_id vs. generation */
@@ -284,18 +316,28 @@  static int register_notification_address(struct snd_dice *dice, bool retry)
 	return err;
 }
 
-static void unregister_notification_address(struct snd_dice *dice)
+void snd_dice_transaction_unregister(struct snd_dice *dice)
 {
 	struct fw_device *device = fw_parent_device(dice->unit);
 	__be64 *buffer;
+	unsigned int i;
+
+	/* Remove from local list. */
+	spin_lock_irq(&instances_lock);
+	for (i = 0; i < SNDRV_CARDS; i++) {
+		if (instances[i] != dice)
+			continue;
+		instances[i] = NULL;
+	}
+	spin_unlock_irq(&instances_lock);
 
 	buffer = kmalloc(2 * 8, GFP_KERNEL);
 	if (buffer == NULL)
 		return;
 
 	buffer[0] = cpu_to_be64(
-		((u64)device->card->node_id << OWNER_NODE_SHIFT) |
-		dice->notification_handler.offset);
+			((u64)device->card->node_id << OWNER_NODE_SHIFT) |
+			notification_handler.offset);
 	buffer[1] = cpu_to_be64(OWNER_NO_OWNER);
 	snd_fw_transaction(dice->unit, TCODE_LOCK_COMPARE_SWAP,
 			   get_subaddr(dice, SND_DICE_ADDR_TYPE_GLOBAL,
@@ -308,33 +350,15 @@  static void unregister_notification_address(struct snd_dice *dice)
 	dice->owner_generation = -1;
 }
 
-void snd_dice_transaction_destroy(struct snd_dice *dice)
-{
-	struct fw_address_handler *handler = &dice->notification_handler;
-
-	if (handler->callback_data == NULL)
-		return;
-
-	unregister_notification_address(dice);
-
-	fw_core_remove_address_handler(handler);
-	handler->callback_data = NULL;
-}
-
-int snd_dice_transaction_reinit(struct snd_dice *dice)
+int snd_dice_transaction_reregister(struct snd_dice *dice)
 {
-	struct fw_address_handler *handler = &dice->notification_handler;
-
-	if (handler->callback_data == NULL)
-		return -EINVAL;
-
 	return register_notification_address(dice, false);
 }
 
-int snd_dice_transaction_init(struct snd_dice *dice)
+int snd_dice_transaction_register(struct snd_dice *dice)
 {
-	struct fw_address_handler *handler = &dice->notification_handler;
 	__be32 *pointers;
+	unsigned int i;
 	int err;
 
 	/* Use the same way which dice_interface_check() does. */
@@ -349,23 +373,10 @@  int snd_dice_transaction_init(struct snd_dice *dice)
 	if (err < 0)
 		goto end;
 
-	/* Allocation callback in address space over host controller */
-	handler->length = 4;
-	handler->address_callback = dice_notification;
-	handler->callback_data = dice;
-	err = fw_core_add_address_handler(handler, &fw_high_memory_region);
-	if (err < 0) {
-		handler->callback_data = NULL;
-		goto end;
-	}
-
 	/* Register the address space */
 	err = register_notification_address(dice, true);
-	if (err < 0) {
-		fw_core_remove_address_handler(handler);
-		handler->callback_data = NULL;
+	if (err < 0)
 		goto end;
-	}
 
 	dice->global_offset = be32_to_cpu(pointers[0]) * 4;
 	dice->tx_offset = be32_to_cpu(pointers[2]) * 4;
@@ -376,7 +387,30 @@  int snd_dice_transaction_init(struct snd_dice *dice)
 	/* Set up later. */
 	if (be32_to_cpu(pointers[1]) * 4 >= GLOBAL_CLOCK_CAPABILITIES + 4)
 		dice->clock_caps = 1;
+
+	/* Insert into local list. */
+	spin_lock_irq(&instances_lock);
+	for (i = 0; i < SNDRV_CARDS; i++) {
+		if (instances[i] != NULL)
+			continue;
+		instances[i] = dice;
+		break;
+	}
+	spin_unlock_irq(&instances_lock);
 end:
 	kfree(pointers);
 	return err;
 }
+
+int snd_dice_transaction_init(void)
+{
+	notification_handler.length = 4;
+	notification_handler.address_callback = dice_notification;
+	return fw_core_add_address_handler(&notification_handler,
+					   &fw_high_memory_region);
+}
+
+void snd_dice_transaction_destroy(void)
+{
+	fw_core_remove_address_handler(&notification_handler);
+}
diff --git a/sound/firewire/dice/dice.c b/sound/firewire/dice/dice.c
index 70a111d..251afc6 100644
--- a/sound/firewire/dice/dice.c
+++ b/sound/firewire/dice/dice.c
@@ -237,7 +237,7 @@  static void dice_card_free(struct snd_card *card)
 	struct snd_dice *dice = card->private_data;
 
 	snd_dice_stream_destroy_duplex(dice);
-	snd_dice_transaction_destroy(dice);
+	snd_dice_transaction_unregister(dice);
 	fw_unit_put(dice->unit);
 
 	mutex_destroy(&dice->mutex);
@@ -268,7 +268,7 @@  static int dice_probe(struct fw_unit *unit, const struct ieee1394_device_id *id)
 	init_completion(&dice->clock_accepted);
 	init_waitqueue_head(&dice->hwdep_wait);
 
-	err = snd_dice_transaction_init(dice);
+	err = snd_dice_transaction_register(dice);
 	if (err < 0)
 		goto error;
 
@@ -323,7 +323,7 @@  static void dice_bus_reset(struct fw_unit *unit)
 	struct snd_dice *dice = dev_get_drvdata(&unit->device);
 
 	/* The handler address register becomes initialized. */
-	snd_dice_transaction_reinit(dice);
+	snd_dice_transaction_reregister(dice);
 
 	mutex_lock(&dice->mutex);
 	snd_dice_stream_update_duplex(dice);
@@ -355,11 +355,18 @@  static struct fw_driver dice_driver = {
 
 static int __init alsa_dice_init(void)
 {
+	int err;
+
+	err = snd_dice_transaction_init();
+	if (err < 0)
+		return err;
+
 	return driver_register(&dice_driver.driver);
 }
 
 static void __exit alsa_dice_exit(void)
 {
+	snd_dice_transaction_destroy();
 	driver_unregister(&dice_driver.driver);
 }
 
diff --git a/sound/firewire/dice/dice.h b/sound/firewire/dice/dice.h
index d8b721c..85d5fcb 100644
--- a/sound/firewire/dice/dice.h
+++ b/sound/firewire/dice/dice.h
@@ -162,9 +162,11 @@  int snd_dice_transaction_set_rate(struct snd_dice *dice, unsigned int rate);
 int snd_dice_transaction_get_rate(struct snd_dice *dice, unsigned int *rate);
 int snd_dice_transaction_set_enable(struct snd_dice *dice);
 void snd_dice_transaction_clear_enable(struct snd_dice *dice);
-int snd_dice_transaction_init(struct snd_dice *dice);
-int snd_dice_transaction_reinit(struct snd_dice *dice);
-void snd_dice_transaction_destroy(struct snd_dice *dice);
+void snd_dice_transaction_unregister(struct snd_dice *dice);
+int snd_dice_transaction_reregister(struct snd_dice *dice);
+int snd_dice_transaction_register(struct snd_dice *dice);
+int snd_dice_transaction_init(void);
+void snd_dice_transaction_destroy(void);
 
 #define SND_DICE_RATES_COUNT	7
 extern const unsigned int snd_dice_rates[SND_DICE_RATES_COUNT];
-- 
2.1.4