| Message ID | 1439425221-30826-4-git-send-email-o-takashi@sakamocchi.jp (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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? > + 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. 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 >
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
On Aug 13 Takashi Sakamoto wrote: > On Aug 13 2015 15:31, Takashi Iwai wrote: > > On Thu, 13 Aug 2015 02:19:59 +0200, > > Takashi Sakamoto wrote: > >> --- a/sound/firewire/lib.c > >> +++ b/sound/firewire/lib.c [...] > >> +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; [...] > >> + /* 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); > >> +} The barrier is normally needed because: - "generation" is the IEEE 1394 bus generation, which is a counter that distinguishes periods between bus resets. - At a bus reset, nodes may assume different node IDs from before the reset. Hence whenever a node is addressed by means of node ID, the generation value must be given too. - The node_ID:generation tuple is accessed in the Linux firewire stack in a lockless manner¹: - firewire-core's bus reset handler writes them by set node_id; smp_wmb(); set generation - higher-level code reads them by get generation; smp_rmb(); get node_id This way highlevel always only uses current IDs in current generations or current IDs in outdated generations (the latter case being extremely rare and doing no harm because the link layer hardware will not emit this sort of requests to the bus), while on the other hand highlevel never uses outdated IDs in current generations (which would cause requests to be misdirected to a wrong node). So I am wondering, why would the barrier _not_ be needed here? ------------ ¹) This was the original author's choice in his preference over using a lock for this access. Another possibility for a lockless API would be to combine node_ID:generation into a single atomic variable; together they are 24 bits wide.
On Aug 15 Stefan Richter wrote: > - The node_ID:generation tuple is accessed in the Linux firewire stack in > a lockless manner¹: > ------------ > ¹) This was the original author's choice I meant the author of the 2.6.22+ firewire stack (Kristian Høgsberg).
On Sat, 15 Aug 2015 12:15:54 +0200, Stefan Richter wrote: > > On Aug 13 Takashi Sakamoto wrote: > > On Aug 13 2015 15:31, Takashi Iwai wrote: > > > On Thu, 13 Aug 2015 02:19:59 +0200, > > > Takashi Sakamoto wrote: > > >> --- a/sound/firewire/lib.c > > >> +++ b/sound/firewire/lib.c > [...] > > >> +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; > [...] > > >> + /* 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); > > >> +} > > The barrier is normally needed because: > - "generation" is the IEEE 1394 bus generation, which is a counter that > distinguishes periods between bus resets. > - At a bus reset, nodes may assume different node IDs from before the > reset. Hence whenever a node is addressed by means of node ID, the > generation value must be given too. > - The node_ID:generation tuple is accessed in the Linux firewire stack in > a lockless manner¹: > - firewire-core's bus reset handler writes them by > set node_id; smp_wmb(); set generation > - higher-level code reads them by > get generation; smp_rmb(); get node_id > This way highlevel always only uses current IDs in current generations > or current IDs in outdated generations (the latter case being > extremely rare and doing no harm because the link layer hardware will > not emit this sort of requests to the bus), while on the other hand > highlevel never uses outdated IDs in current generations (which would > cause requests to be misdirected to a wrong node). Thanks, that clarifies. But wouldn't it be more helpful to have some macro than open-coding at each place? Takashi
On Aug 16 Takashi Iwai wrote: > On Sat, 15 Aug 2015 12:15:54 +0200, > Stefan Richter wrote: > > > > On Aug 13 Takashi Sakamoto wrote: > > > On Aug 13 2015 15:31, Takashi Iwai wrote: > > > > On Thu, 13 Aug 2015 02:19:59 +0200, > > > > Takashi Sakamoto wrote: > > > >> + 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); > > > >> +} > > > > The barrier is normally needed because: [there is a dependency between node_ID and generation and] > > - The node_ID:generation tuple is accessed in the Linux firewire stack in > > a lockless manner: [...] > Thanks, that clarifies. But wouldn't it be more helpful to have some > macro than open-coding at each place? I agree that a cooked-up accessor via <linux/firewire.h> would be good, but I have no immediate idea how that accessor should look like. Looking at the various uses of fw_send_request [http://lxr.free-electrons.com/ident?i=fw_send_request], it's clear that this API between protocol drivers and firewire-core is quite low-level and flexible by nature. In particular, the different users of the API take node_id:generation from different sources.¹ As mentioned, another way would be to fuse node_id:generation into a single atomic datum throughout all firewire drivers. I.e. instead of ensuring the proper access order everytime, just access both data simultaneously everytime. Not having tried this yet, I am not sure which way would be more readable. ------------ ¹) Those differences between fw_send_request users are typically due to bus reset handling in highlevel drivers on top of firewire-core's bus reset handling. [firewire-core::bus reset handler] - analyze bus topology - write fw_device instances' node_id:generation tuple - notify protocol drivers [protocol_driver::bus reset handler] - perform protocol-specific reconnection procedure - write protocol_driver_device instance's node_id:generation tuple [protocol_driver::outbound IO] - read protocol_driver_device instance's node_id:generation tuple - send request Here the request is not only guaranteed to hit the topologically correct target, it is also guaranteed to hit only while in a protocol-specific connected state. (Requests with a stale generation won't go out to the bus.)
On Sun, 16 Aug 2015 14:15:09 +0200, Stefan Richter wrote: > > On Aug 16 Takashi Iwai wrote: > > On Sat, 15 Aug 2015 12:15:54 +0200, > > Stefan Richter wrote: > > > > > > On Aug 13 Takashi Sakamoto wrote: > > > > On Aug 13 2015 15:31, Takashi Iwai wrote: > > > > > On Thu, 13 Aug 2015 02:19:59 +0200, > > > > > Takashi Sakamoto wrote: > > > > >> + 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); > > > > >> +} > > > > > > The barrier is normally needed because: > [there is a dependency between node_ID and generation and] > > > - The node_ID:generation tuple is accessed in the Linux firewire stack in > > > a lockless manner: > [...] > > Thanks, that clarifies. But wouldn't it be more helpful to have some > > macro than open-coding at each place? > > I agree that a cooked-up accessor via <linux/firewire.h> would be good, > but I have no immediate idea how that accessor should look like. Looking > at the various uses of fw_send_request > [http://lxr.free-electrons.com/ident?i=fw_send_request], > it's clear that this API between protocol drivers and firewire-core is > quite low-level and flexible by nature. In particular, the different users > of the API take node_id:generation from different sources.¹ Argh, I somehow expected that the accesses are done in more or less in the same struct or device object. Then it'll be tricky, indeed. > As mentioned, another way would be to fuse node_id:generation into a single > atomic datum throughout all firewire drivers. I.e. instead of ensuring the > proper access order everytime, just access both data simultaneously > everytime. Not having tried this yet, I am not sure which way would be > more readable. Yeah, having a common tuple might things a bit easier to manage, I suppose. But I'm not familiar with the firewire code in general, so don't count this as a serious evaluation at all ;) thanks, Takashi
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 */ + 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); + + 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
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(+)