Re: [PATCH v2] MIDI driver for Behringer BCD2000 USB device

[Clemens Ladisch <clemens@ladisch.de>]

Mario Kicherer wrote:
> On 06.02.2014 10:21, Clemens Ladisch wrote:
>>> +config SND_USB_BCD2000
>>> +    tristate "Behringer BCD2000 MIDI driver"
>>> +    select SND_HWDEP
>>
>> Why HWDEP?
>
> I admit, I copied this from the other modules (caiaq I believe). After
> grepping the documentation, I'm not sure if I need this. Should I remove
> it?

Yes; you aren't using any snd_hwdep* functions.

>> There is no need to have "usb" in the module name if that is the only
>> bus this device works with.
>
> Okay, after looking at the other modules I thought that it is common to
> use the "snd-usb" prefix.

The USB audio class driver uses "snd-usb-audio" because "snd-audio" would
be completely useless as a name.  Other drivers then blindly copied this.

> Should I remove it? I find it useful, e.g., to recognize it in a "lsmod".

"snd-bcd2000" would not be recognizable?

>>> -obj-$(CONFIG_SND) += misc/ usx2y/ caiaq/ 6fire/ hiface/
>>> +obj-$(CONFIG_SND) += misc/ usx2y/ caiaq/ 6fire/ hiface/ bcd2000/
>>
>> If there is only a single source file, just put it into misc/.
>> If you plan to have multiple source files, then this file should have
>> "_midi" in its name.
>
> Okay, I thought that the code is short enough to put everything in one
> file but I think with the audio part it would be best to split the file.
> I can move it to misc/ or do you think an own subdirectory is okay to
> avoid moving it in later versions?

It doesn't make much sense to put it into a directory that you know you
will move it out of later.

>> ALSA does not require full MIDI commands, and handles running status.
>
> Oh, okay. So, should I just pass everything through to
> snd_rawmidi_receive() ?

Yes.

>>> +static void bcd2000_midi_output_done(struct urb *urb)
>>> +{
>>> ...
>>> +    if (!bcd2k->midi_out_substream)
>>> +        return;
>>> +
>>> +    bcd2000_midi_send(bcd2k, bcd2k->midi_out_substream);
>>
>> The midi_out_substream field gets modified asynchronously by the trigger
>> callback; this might happen between these two statements.
>>
>> Assuming that setting a pointer is atomic, use ACCESS_ONCE.
>
> You mean like this:
>
> struct snd_rawmidi_substream * midi_out_substream;
> midi_out_substream = ACCESS_ONCE(bcd2k->midi_out_substream);
> if (!midi_out_substream)
>     return;
> bcd2000_midi_send(bcd2k, midi_out_substream);

Yes.

The code that modifies this field also needs ACCESS_ONCE.

>>> +static void bcd2000_init_midi(struct bcd2000 *bcd2k)
>>> +{
>>> +    bcd2k->midi_in_urb = usb_alloc_urb(0, GFP_KERNEL);
>>> +    bcd2k->midi_out_urb = usb_alloc_urb(0, GFP_KERNEL);
>>
>> Error checking.
>
> Okay, I guess I should close everything I opened in this module before
> or would a "return" suffice?

Typically, in case of an error, a function is responsible to revert
everything allocated in this function, i.e.:

static int bcd2000_init_midi(struct bcd2000 *bcd2k)
{
	bcd2k->midi_in_urb = usb_alloc_urb(0, GFP_KERNEL);
	if (!bcd2k->midi_in_urb)
		return -ENOMEM;
	bcd2k->midi_out_urb = usb_alloc_urb(0, GFP_KERNEL);
	if (!bcd2k->midi_out_urb) {
		usb_free_urb(bcd2k->midi_in_urb);
		return -ENOMEM;
	}
	...


Regards,
Clemens