linux-kernel - Re: [alsa-devel] [PATCH 2.6.34-rc5] HID: Prodikeys PC-MIDI HID Driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Thu, 06 May 2010 08:30:22 +0200
From:	Clemens Ladisch <clemens@...isch.de>
To:	"dhprince.devel@...oo" <dhprince.devel@...oo.co.uk>
CC:	linux-input@...r.kernel.org, alsa-devel@...a-project.org,
	linux-kernel@...r.kernel.org
Subject: Re: [alsa-devel] [PATCH 2.6.34-rc5] HID: Prodikeys PC-MIDI HID Driver

dhprince.devel@...oo wrote:
> A specialised HID driver for the CreativeLabs Prodikeys PC-MIDI USB 
> Keyboard.
> ...

I cannot comment on the input stuff, but the sound stuff looks good
overall.

> +What:        /sys/bus/hid/drivers/prodikeys/.../channel
> +Date:        April 2010
> +KernelVersion:    2.6.34
> +Contact:    Don Prince <dhprince.devel@...oo.co.uk>
> +Descripts/checkpatch.plscription:

Huh?

> +What:        /sys/bus/hid/drivers/prodikeys/.../sustain
> +Description:
> +        Allows control (via software) the sustain duration of a
> +        note held by the pc-midi driver.

Why is this feature needed?  Does the device report key releases
incorrectly?

These three sysfs controls could also be implemented as mixer controls.
This would allow them to be automatically saved and restored when the
computer is shut down.

> +    { HID_USB_DEVICE(USB_VENDOR_ID_CREATIVELABS, 
> USB_DEVICE_ID_PRODIKEYS_PCMIDI) },

Your mailer wrapped long lines and killed the tabs.
Please see <Documentation/email-clients.txt>.

And your code looks as if it does not use eight-column tabs for
indention.

> +static void pcmidi_send_note(struct pcmidi_snd *pm,
> ...
> +    res = snd_rawmidi_receive(pm->in_substream, buffer, 3);

This return value is never used.

> +                    if (!atomic_read(&pms->in_use)) {
> +                        pms->status = status;
> +                        pms->note = note;
> +                        pms->velocity = velocity;
> +                        atomic_set(&pms->in_use, 1);

If you're using the atomic variable to protect against some concurrently
executing code, then you have a race condition in the time interval
between the read and the set.

> +static void pcmidi_out_tasklet(unsigned long data)
> ...
> +    while (1) {
> +        /* just read them and drop them */
> +        u8 b;
> +        if (snd_rawmidi_transmit(pm->out_substream, &b, 1) != 1) {
> +            pm->out_active = 0;
> +            break;
> +        }

This isn't quite how output ports are supposed to be implemented.  :-)

I'd guess you want to remove the OUTPUT and DUPLEX flags and to drop all
output-related functions.

> +static void pcmidi_in_trigger(struct snd_rawmidi_substream *substream, 
> int up)
> ...
> +    if (up)
> +        set_bit(substream->number, &pm->in_triggered);
> +    else
> +        clear_bit(substream->number, &pm->in_triggered);

You have only one substream, a boolean variable would suffice.

> +int pcmidi_snd_initialise(struct pcmidi_snd *pm)
> ...
> +    int out_ports = 1;
> +    int in_ports = 1;

These variables are not variable and therefore not needed.

> +    snd_component_add(card, "MIDI");

This function is intended for sound cards that are composed of several
components, and the component name is usually a chip name.  I think
you don't need to call this function at all.

> +    err = snd_card_register(card);
> ...
> +    /* create sysfs variables */
> +    err = device_create_file(&pm->pk->hdev->dev,
> +                 sysfs_device_attr_channel);
> ...
> +    spin_lock_init(&pm->rawmidi_in_lock);
> +
> +    init_sustain_timers(pm);

snd_card_register() makes all sound interfaces available to userspace.
Initializations for any data that might be accessed from userspace must
be done before that call.

Regards,
Clemens
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/