Re: [PATCH 03/25] ALSA: firewire-lib: add helper functions for asynchronous MIDI port

[Takashi Sakamoto <o-takashi@sakamocchi.jp>]

On Aug 13 2015 15:31, Takashi Iwai wrote:
> On Thu, 13 Aug 2015 02:19:59 +0200,
> Takashi Sakamoto wrote:
>>
>> Some devices receive MIDI messages via IEEE 1394 asynchronous transactions.
>> In this case, MIDI messages are transferred in fixed-length payload. It's
>> nice for firewire-lib module to have common helper functions.
>>
>> This commit implements this idea. Each driver adds
>> 'struct snd_fw_async_midi_port' in its data structure. In probing,
>> it should call snd_fw_async_midi_port_init() to initialize the
>> structure with some parameters such as target address, the length
>> of payload for a transaction and a pointer for callback function
>> to fill the payload buffer. When 'struct snd_rawmidi_ops.trigger()'
>> callback, it should call 'snd_fw_async_midi_port_run()' to start
>> transactions. Each driver should ensure that the lifetime of MIDI
>> substream continues till calling 'snd_fw_async_midi_port_finish()'.
>>
>> The helper functions support retries to transferring MIDI messages when
>> transmission errors occur. When transactions are successful, the helper
>> functions call 'snd_rawmidi_transmit_ack()' to consume MIDI bytes in the
>> buffer. Therefore, Each driver is expected to use
>> 'snd_rawmidi_transmit_peek()'.
>>
>> Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
>> ---
>>   sound/firewire/lib.c | 95 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>>   sound/firewire/lib.h | 46 +++++++++++++++++++++++++
>>   2 files changed, 141 insertions(+)
>>
>> diff --git a/sound/firewire/lib.c b/sound/firewire/lib.c
>> index 7409edb..f5b5526 100644
>> --- a/sound/firewire/lib.c
>> +++ b/sound/firewire/lib.c
>> @@ -9,6 +9,7 @@
>>   #include <linux/device.h>
>>   #include <linux/firewire.h>
>>   #include <linux/module.h>
>> +#include <linux/slab.h>
>>   #include "lib.h"
>>
>>   #define ERROR_RETRY_DELAY_MS	20
>> @@ -66,6 +67,100 @@ int snd_fw_transaction(struct fw_unit *unit, int tcode,
>>   }
>>   EXPORT_SYMBOL(snd_fw_transaction);
>>
>> +static void async_midi_port_callback(struct fw_card *card, int rcode,
>> +				     void *data, size_t length,
>> +				     void *callback_data)
>> +{
>> +	struct snd_fw_async_midi_port *port = callback_data;
>> +	struct snd_rawmidi_substream *substream = ACCESS_ONCE(port->substream);
>> +
>> +	if (rcode == RCODE_COMPLETE && substream != NULL)
>> +		snd_rawmidi_transmit_ack(substream, port->consume_bytes);
>> +}
>> +
>> +static void midi_port_tasklet(unsigned long data)
>> +{
>> +	struct snd_fw_async_midi_port *port =
>> +					(struct snd_fw_async_midi_port *)data;
>> +	struct snd_rawmidi_substream *substream = ACCESS_ONCE(port->substream);
>> +	int generation;
>> +	int type;
>> +
>> +	/* Nothing to do. */
>> +	if (substream == NULL || snd_rawmidi_transmit_empty(substream))
>> +		return;
>> +
>> +	/*
>> +	 * Fill the buffer. The callee must use snd_rawmidi_transmit_peek().
>> +	 * Later, snd_rawmidi_transmit_ack() may be called.
>> +	 */
>> +	memset(port->buf, 0, port->len);
>> +	port->consume_bytes = port->packetize(substream, port->buf);
>> +	if (port->consume_bytes <= 0) {
>> +		/* Do it in next chance, immediately. */
>> +		if (port->consume_bytes == 0)
>> +			tasklet_schedule(&port->tasklet);
>> +		return;
>> +	}
>> +
>> +	/* Calculate type of transaction. */
>> +	if (port->len == 4)
>> +		type = TCODE_WRITE_QUADLET_REQUEST;
>> +	else
>> +		type = TCODE_WRITE_BLOCK_REQUEST;
>> +
>> +	/* Start this transaction. */
>> +	generation = port->parent->generation;
>> +	smp_rmb(); /* node_id vs. generation */
>
> Why this barrier is needed?

Oops. I forgot to refine it after copying from the other place. Exactly, 
it's not need here.

>> +	fw_send_request(port->parent->card, &port->transaction, type,
>> +			port->parent->node_id, generation,
>> +			port->parent->max_speed, port->addr,
>> +			port->buf, port->len, async_midi_port_callback,
>> +			port);
>> +}
>> +
>> +/**
>> + * snd_fw_async_midi_port_init - initialize asynchronous MIDI port structure
>> + * @port: the asynchronous MIDI port to initialize
>> + * @unit: the target of the asynchronous transaction
>> + * @addr: the address to which transactions are transferred
>> + * @len: the length of transaction
>> + * @packetize: the callback function to fill given buffer, and returns the
>> + *	       number of consumed bytes for MIDI message.
>> + *
>> + */
>> +int snd_fw_async_midi_port_init(struct snd_fw_async_midi_port *port,
>> +		struct fw_unit *unit, __u64 addr, unsigned int len,
>> +		int (*packetize)(struct snd_rawmidi_substream *substream,
>> +				 __u8 *buf))
>> +{
>> +	port->len = DIV_ROUND_UP(len, 4) * 4;
>> +	port->buf = kzalloc(port->len, GFP_KERNEL);
>> +	if (port->buf == NULL)
>> +		return -ENOMEM;
>> +
>> +	port->parent = fw_parent_device(unit);
>> +	port->addr = addr;
>> +	port->packetize = packetize;
>> +
>> +	tasklet_init(&port->tasklet, midi_port_tasklet, (unsigned long)port);
>
> Well, tasklet is an interface that might be deprecated in future.
> Better to code with other interfaces if possible.
>
> But, tasklet seems heavily used in the current firewire code already,
> so it's fine to keep it in this patchset.  But bear it in mind that
> we may need conversions of tasklet usage sometime later.

Using the other kernel functionalities such as workqueue, itself, 
doesn't matter to me, because I've already post RFC with using workqueue 
instead of tasklet.

[alsa-devel] [PATCH 08/11] ALSA: digi00x: support MIDI ports for device 
control
http://mailman.alsa-project.org/pipermail/alsa-devel/2015-March/089147.html

ON the other hand, I'm interested in the discussion about the 
deprecation, even if 'might'. Could you show the resources about it?


Thanks

Takashi Sakamoto

> thanks,
>
> Takashi
>
>> +
>> +	return 0;
>> +}
>> +EXPORT_SYMBOL(snd_fw_async_midi_port_init);
>> +
>> +/**
>> + * snd_fw_async_midi_port_destroy - free asynchronous MIDI port structure
>> + * @port: the asynchronous MIDI port structure
>> + */
>> +void snd_fw_async_midi_port_destroy(struct snd_fw_async_midi_port *port)
>> +{
>> +	snd_fw_async_midi_port_finish(port);
>> +	tasklet_kill(&port->tasklet);
>> +	kfree(port->buf);
>> +}
>> +EXPORT_SYMBOL(snd_fw_async_midi_port_destroy);
>> +
>>   MODULE_DESCRIPTION("FireWire audio helper functions");
>>   MODULE_AUTHOR("Clemens Ladisch <clemens@ladisch.de>");
>>   MODULE_LICENSE("GPL v2");
>> diff --git a/sound/firewire/lib.h b/sound/firewire/lib.h
>> index 02cfabc..c662bf9 100644
>> --- a/sound/firewire/lib.h
>> +++ b/sound/firewire/lib.h
>> @@ -3,6 +3,8 @@
>>
>>   #include <linux/firewire-constants.h>
>>   #include <linux/types.h>
>> +#include <linux/sched.h>
>> +#include <sound/rawmidi.h>
>>
>>   struct fw_unit;
>>
>> @@ -20,4 +22,48 @@ static inline bool rcode_is_permanent_error(int rcode)
>>   	return rcode == RCODE_TYPE_ERROR || rcode == RCODE_ADDRESS_ERROR;
>>   }
>>
>> +struct snd_fw_async_midi_port {
>> +	struct fw_device *parent;
>> +	struct tasklet_struct tasklet;
>> +
>> +	__u64 addr;
>> +	struct fw_transaction transaction;
>> +
>> +	__u8 *buf;
>> +	unsigned int len;
>> +
>> +	struct snd_rawmidi_substream *substream;
>> +	int (*packetize)(struct snd_rawmidi_substream *substream, __u8 *buf);
>> +	unsigned int consume_bytes;
>> +};
>> +
>> +int snd_fw_async_midi_port_init(struct snd_fw_async_midi_port *port,
>> +		struct fw_unit *unit, __u64 addr, unsigned int len,
>> +		int (*packetize)(struct snd_rawmidi_substream *substream,
>> +				 __u8 *buf));
>> +void snd_fw_async_midi_port_destroy(struct snd_fw_async_midi_port *port);
>> +
>> +/**
>> + * snd_fw_async_midi_port_run - run transactions for the async MIDI port
>> + * @port: the asynchronous MIDI port
>> + * @substream: the MIDI substream
>> + */
>> +static inline void
>> +snd_fw_async_midi_port_run(struct snd_fw_async_midi_port *port,
>> +			   struct snd_rawmidi_substream *substream)
>> +{
>> +	port->substream = substream;
>> +	tasklet_schedule(&port->tasklet);
>> +}
>> +
>> +/**
>> + * snd_fw_async_midi_port_finish - finish the asynchronous MIDI port
>> + * @port: the asynchronous MIDI port
>> + */
>> +static inline void
>> +snd_fw_async_midi_port_finish(struct snd_fw_async_midi_port *port)
>> +{
>> +	port->substream = NULL;
>> +}
>> +
>>   #endif
>> --
>> 2.1.4