Re: [RESEND PATCHv5 09/11] modem_shm: Character device for SHMchannel access.

[Alan Cox]

On Tue, 31 Jan 2012 09:48:44 +0100
Sjur Brændeland <sjur.brandeland@xxxxxxxxxxxxxx> wrote:

> Add a character device implementation for the SHM stream channels.
> The character device provides asynchronous IO and ring-buffer handling.
> The device copies data directly from the Shared Memory area into
> user-land buffers.

What is the use case for this - it seems a little odd that it's not using
the tty layer so won't work with all the normal modem apps as anyone
would expect and want ?


> +static unsigned int shmchr_chrpoll(struct file *filp, poll_table *waittab)
> +{
> +	struct shmchr_char_dev *dev = filp->private_data;
> +	unsigned int mask = 0;
> +
> +	if (dev == NULL) {
> +		mdev_dbg(dev, "private_data not set!\n");
> +		return -EBADFD;
> +	}

How can this occur. If it can't occur why check ? BUG_ON() would
certainly be better to as you'd get a trace and it would get captured not
silently ignored and problems never detected.

An if.. dbg sequence to end users is basically "silently pretend we
didn't break and hope", which isn't ideal at all.


> +
> +	/* I want to be alone on dev (except status and queue). */
> +	if (mutex_lock_interruptible(&dev->mutex)) {
> +		mdev_dbg(dev, "mutex_lock_interruptible got signalled\n");
> +		mask |= POLLERR;
> +		goto out_unlocked;

That's very odd behaviour for poll() and may confuse apps. Can the mutex
ever be held for a long time ?

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