Re: TDM bus support in Linux Kernel [PATCH]

[Oliver Neukum <oneukum@suse.de>]

On Wednesday 30 January 2013 12:37:25 Kurachkin Michail wrote:
> Hi Greg,
> 
> I followed your recommendations and created a diff using Linux 3.8-rc5 sources. Please review it and give your comments.

Part #3

+/*
+ * method "open" for character device
+ */
+static int slic_chr_open(struct inode *inode, struct file *file)
+{
+	struct slic_chr_dev *chr_dev;
+	struct si3226x_slic *slic;
+	struct si3226x_line *line;
+	struct tdm_device *tdm_dev;
+	struct tdm_voice_channel *ch;
+	int status = -ENXIO;
+
+	mutex_lock(&slic_chr_dev_lock);
+
+	list_for_each_entry(chr_dev, &slic_chr_dev_list, list) {
+		switch (chr_dev->type) {
+		case SLIC_CHR_DEV:
+			slic = dev_get_drvdata(chr_dev->dev);
+
+			if (slic->devt != inode->i_rdev)
+				continue;;
+
+			if (slic->file_opened) {
+				status = -EBUSY;
+				goto out;
+			}
+
+			slic->file_opened = 1;
+			status = 0;
+			break;
+
+		case LINE_CHR_DEV:
+			line = dev_get_drvdata(chr_dev->dev);
+			tdm_dev = line->tdm_dev;
+
+			if (line->devt != inode->i_rdev)
+				continue;
+
+			if (line->file_opened) {
+				status = -EBUSY;
+				goto out;
+			}
+
+			line->audio_buffer_size = tdm_get_voice_block_size(tdm_dev);
+
+			line->rx_buffer = kzalloc(line->audio_buffer_size, GFP_KERNEL);
+			if (!line->rx_buffer) {
+				status = -ENOMEM;
+				goto out;
+			}
+
+			line->tx_buffer = kzalloc(line->audio_buffer_size, GFP_KERNEL);
+			if (!line->tx_buffer) {
+				status = -ENOMEM;

memory leak of line->rx_buffer

+				goto out;
+			}
+
+			/* store pointer to transmit wait_queue_head_t for use in poll() */
+			ch = tdm_dev->ch;
+			line->tx_wait_queue = &ch->tx_queue;
+			line->rx_wait_queue = &ch->rx_queue;
+			line->file_opened = 1;
+			status = 0;
+
+			break;
+		}
+
+		file->private_data = chr_dev;
+		status = 0;
+		goto out;

a bit pointless

+	}
+
+out:
+	mutex_unlock(&slic_chr_dev_lock);
+	return status;
+}

+/*
+ * method "ioctl" for character device
+ */
+static long
+slic_chr_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
+{
+	struct slic_chr_dev *chr_dev = file->private_data;
+	struct si3226x_slic *slic;
+	struct si3226x_line *line;
+	int status = -ENXIO;
+
+	if (_IOC_TYPE(cmd) != SI3226X_IOC_MAGIC)
+		return -ENOTTY;
+
+	mutex_lock(&slic_chr_dev_lock);
+
+	if (!file->private_data) {
+		status = -EPERM;

-EPERM?

+		goto out;
+	}
+
+	switch(chr_dev->type) {
+	case SLIC_CHR_DEV:
+		slic = dev_get_drvdata(chr_dev->dev);
+		if (!slic->file_opened) {
+			status = -ENODEV;
+			goto out;
+		}
+
+		status = slic_control(slic, cmd, arg);
+		break;
+
+	case LINE_CHR_DEV:
+		line = dev_get_drvdata(chr_dev->dev);
+		if (!line->file_opened) {
+			status = -ENODEV;
+			goto out;
+		}
+
+		status = line_control(line, cmd, arg);
+		break;
+	}
+
+out:
+	mutex_unlock(&slic_chr_dev_lock);
+	return status;
+}


+/*
+ * method "read" for character device
+ */
+static ssize_t
+slic_chr_read(struct file *file, char *buff, size_t count, loff_t *offp)
+{
+	struct slic_chr_dev *chr_dev = file->private_data;
+	struct si3226x_line *line;
+	struct tdm_device *tdm_dev;
+	int rc;
+
+	if (!file->private_data)
+		return -EPERM;
+
+	if(chr_dev->type != LINE_CHR_DEV)
+		return -EPERM;
+
+	mutex_lock(&slic_chr_dev_lock);
+
+	line = dev_get_drvdata(chr_dev->dev);
+	tdm_dev = line->tdm_dev;
+
+	if (count != line->audio_buffer_size) {
+		rc = -ENODATA;
+		goto out;
+	}
+
+	rc = tdm_recv(tdm_dev, line->rx_buffer);
+	if (rc) {
+		rc = -EFAULT;

EFAULT is usually not what you return upon a read error.

+		goto out;
+	}
+
+	rc = copy_to_user(buff, line->rx_buffer, count);
+	if (rc) {
+		rc = -EFAULT;
+		goto out;
+	}
+
+	rc = count;
+out:
+	mutex_unlock(&slic_chr_dev_lock);
+	return rc;
+}