| 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
| ||
|
Message-Id: <201303180858.47329.hverkuil@xs4all.nl>
Date: Mon, 18 Mar 2013 08:58:47 +0100
From: Hans Verkuil <hverkuil@...all.nl>
To: Jon Arne Jørgensen <jonarne@...arne.no>
Cc: linux-media@...r.kernel.org, linux-kernel@...r.kernel.org,
elezegarcia@...il.com
Subject: Re: [RFC V1 2/8] smi2021: Add smi2021_main.c
Hi Jon Arne,
Here is my review of this driver.
Regards,
Hans
On Thu March 14 2013 15:06:58 Jon Arne Jørgensen wrote:
> This is the core of the smi2021 module.
> It will register the module with the kernel, and register the
> usb probe function.
>
> Signed-off-by: Jon Arne Jørgensen <jonarne@...arne.no>
> ---
> drivers/media/usb/smi2021/smi2021_main.c | 339 +++++++++++++++++++++++++++++++
> 1 file changed, 339 insertions(+)
> create mode 100644 drivers/media/usb/smi2021/smi2021_main.c
>
> diff --git a/drivers/media/usb/smi2021/smi2021_main.c b/drivers/media/usb/smi2021/smi2021_main.c
> new file mode 100644
> index 0000000..cc600e7
> --- /dev/null
> +++ b/drivers/media/usb/smi2021/smi2021_main.c
> @@ -0,0 +1,339 @@
> +/*******************************************************************************
> + * smi2021_main.c *
> + * *
> + * USB Driver for SMI2021 - EasyCAP *
> + * USB ID 1c88:003c *
> + * *
> + * *****************************************************************************
> + *
> + * Copyright 2011-2013 Jon Arne Jørgensen
> + * <jonjon.arnearne--a.t--gmail.com>
> + *
> + * Copyright 2011, 2012 Tony Brown, Michal Demin, Jeffry Johnston
> + *
> + * This file is part of SMI2021
> + * http://code.google.com/p/easycap-somagic-linux/
> + *
> + * This program is free software: you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation, either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, see <http://www.gnu.org/licenses/>.
> + *
> + * This driver is heavily influensed by the STK1160 driver.
> + * Copyright (C) 2012 Ezequiel Garcia
> + * <elezegarcia--a.t--gmail.com>
> + *
> + */
> +
> +#include "smi2021.h"
> +
> +#define VENDOR_ID 0x1c88
> +
> +static unsigned int imput;
> +module_param(imput, int, 0644);
> +MODULE_PARM_DESC(input, "Set default input");
> +
> +MODULE_LICENSE("GPL");
> +MODULE_AUTHOR("Jon Arne Jørgensen <jonjon.arnearne--a.t--gmail.com>");
> +MODULE_DESCRIPTION("SMI2021 - EasyCap");
> +MODULE_VERSION(SMI2021_DRIVER_VERSION);
> +
> +
> +struct usb_device_id smi2021_usb_device_id_table[] = {
Should be static const
> + { USB_DEVICE(VENDOR_ID, 0x003c) },
> + { USB_DEVICE(VENDOR_ID, 0x003d) },
> + { USB_DEVICE(VENDOR_ID, 0x003e) },
> + { USB_DEVICE(VENDOR_ID, 0x003f) },
> + { }
> +};
> +
> +MODULE_DEVICE_TABLE(usb, smi2021_usb_device_id_table);
> +
> +static unsigned short saa7113_addrs[] = {
> + 0x4a,
> + I2C_CLIENT_END
> +};
Don't use this. See below for more info.
> +
> +/******************************************************************************/
> +/* */
> +/* Write to saa7113 */
> +/* */
> +/******************************************************************************/
> +
> +inline int transfer_usb_ctrl(struct smi2021_dev *dev, u8 *data, int len)
> +{
> + return usb_control_msg(dev->udev, usb_sndctrlpipe(dev->udev, 0x00),
> + 0x01, USB_DIR_OUT | USB_TYPE_VENDOR | USB_RECIP_DEVICE,
> + 0x0b, 0x00,
> + data, len, 1000);
> +}
> +
> +int smi2021_write_reg(struct smi2021_dev *dev, u8 addr, u16 reg, u8 val)
> +{
> + int rc;
> + u8 snd_data[8];
> +
> + memset(snd_data, 0x00, 8);
> +
> + snd_data[SMI2021_CTRL_HEAD] = 0x0b;
> + snd_data[SMI2021_CTRL_ADDR] = addr;
> + snd_data[SMI2021_CTRL_DATA_SIZE] = 0x01;
> +
> + if (addr) {
> + /* This is I2C data for the saa7113 chip */
> + snd_data[SMI2021_CTRL_BM_DATA_TYPE] = 0xc0;
> + snd_data[SMI2021_CTRL_BM_DATA_OFFSET] = 0x01;
> +
> + snd_data[SMI2021_CTRL_I2C_REG] = reg;
> + snd_data[SMI2021_CTRL_I2C_VAL] = val;
> + } else {
> + /* This is register settings for the smi2021 chip */
> + snd_data[SMI2021_CTRL_BM_DATA_OFFSET] = 0x82;
> +
> + snd_data[SMI2021_CTRL_REG_HI] = __cpu_to_be16(reg) >> 8;
> + snd_data[SMI2021_CTRL_REG_LO] = __cpu_to_be16(reg);
This looks really weird. On a little endian system REG_HI is now initialized
with the LSB, not MSB and the other way around on a big endian system.
The _cpu_to_be16 certainly doesn't belong here.
> +
> + }
> +
> + rc = transfer_usb_ctrl(dev, snd_data, 8);
> + if (rc < 0) {
> + smi2021_warn("write failed on register 0x%x, errno: %d\n",
> + reg, rc);
> + return rc;
> + }
> +
> + return 0;
> +}
> +
> +int smi2021_read_reg(struct smi2021_dev *dev, u8 addr, u16 reg, u8 *val)
> +{
> + int rc;
> + u8 rcv_data[13];
> + u8 snd_data[8];
> + memset(rcv_data, 0x00, 13);
> + memset(snd_data, 0x00, 8);
> +
> + snd_data[SMI2021_CTRL_HEAD] = 0x0b;
> + snd_data[SMI2021_CTRL_ADDR] = addr;
> + snd_data[SMI2021_CTRL_BM_DATA_TYPE] = 0x84;
> + snd_data[SMI2021_CTRL_DATA_SIZE] = 0x01;
> + snd_data[SMI2021_CTRL_I2C_REG] = reg;
> +
> + *val = 0;
> +
> + rc = transfer_usb_ctrl(dev, snd_data, 8);
> + if (rc < 0) {
> + smi2021_warn(
> + "1st pass failing to read reg 0x%x, usb-errno: %d\n",
> + reg, rc);
> + return rc;
> + }
> +
> + snd_data[SMI2021_CTRL_BM_DATA_TYPE] = 0xa0;
> + rc = transfer_usb_ctrl(dev, snd_data, 8);
> + if (rc < 0) {
> + smi2021_warn(
> + "2nd pass failing to read reg 0x%x, usb-errno: %d\n",
> + reg, rc);
> + return rc;
> + }
> +
> + rc = usb_control_msg(dev->udev,
> + usb_rcvctrlpipe(dev->udev, 0x80), 0x01,
> + USB_DIR_IN | USB_TYPE_VENDOR | USB_RECIP_DEVICE,
> + 0x0b, 0x00, rcv_data, 13, 1000);
> + if (rc < 0) {
> + smi2021_warn("Failed to read reg 0x%x, usb-errno: %d\n",
> + reg, rc);
> + return rc;
> + }
> +
> + *val = rcv_data[SMI2021_CTRL_I2C_RCV_VAL];
> + return 0;
> +}
> +
> +static void smi2021_reset_device(struct smi2021_dev *dev)
> +{
> + smi2021_write_reg(dev, 0, 0x3a, 0x80);
> + smi2021_write_reg(dev, 0, 0x3b, 0x80);
> + smi2021_write_reg(dev, 0, 0x3b, 0x00);
> +}
> +
> +static void release_v4l2_dev(struct v4l2_device *v4l2_dev)
> +{
> + struct smi2021_dev *dev = container_of(v4l2_dev, struct smi2021_dev,
> + v4l2_dev);
> + smi2021_dbg("Releasing all resources\n");
> +
> + smi2021_i2c_unregister(dev);
> +
> + v4l2_ctrl_handler_free(&dev->ctrl_handler);
> + v4l2_device_unregister(&dev->v4l2_dev);
> + kfree(dev);
> +}
> +
> +#define hb_mult(w_max_packet_size) (1 + (((w_max_packet_size) >> 11) & 0x03))
> +
> +static int smi2021_scan_usb(struct usb_interface *intf, struct usb_device *udev)
> +{
> + int i, e, ifnum, sizedescr, size;
> + const struct usb_endpoint_descriptor *desc;
> + ifnum = intf->altsetting[0].desc.bInterfaceNumber;
> +
> + for (i = 0; i < intf->num_altsetting; i++) {
> + for (e = 0; e < intf->altsetting[i].desc.bNumEndpoints; e++) {
> + desc = &intf->altsetting[i].endpoint[e].desc;
> + sizedescr = le16_to_cpu(desc->wMaxPacketSize);
> + size = sizedescr & 0x7ff;
> +
> + if (udev->speed == USB_SPEED_HIGH)
> + size = size * hb_mult(sizedescr);
> + }
> + }
> + return 0;
> +}
> +
> +/******************************************************************************/
> +/* */
> +/* DEVICE - PROBE & DISCONNECT */
> +/* */
> +/******************************************************************************/
> +static int smi2021_usb_probe(struct usb_interface *intf,
> + const struct usb_device_id *devid)
> +{
> + int rc = 0;
> + struct usb_device *udev = interface_to_usbdev(intf);
> + struct smi2021_dev *dev;
> +
> + if (udev == (struct usb_device *)NULL) {
> + smi2021_err("device is NULL\n");
> + return -EFAULT;
> + }
> +
> + smi2021_scan_usb(intf, udev);
> +
> + dev = kzalloc(sizeof(struct smi2021_dev), GFP_KERNEL);
> + if (dev == NULL)
> + return -ENOMEM;
> +
> + dev->udev = udev;
> + dev->dev = &intf->dev;
> + usb_set_intfdata(intf, dev);
> +
> + /* Initialize videobuf2 stuff */
> + rc = smi2021_vb2_setup(dev);
> + if (rc < 0)
> + goto free_err;
> +
> + spin_lock_init(&dev->buf_lock);
> + mutex_init(&dev->v4l2_lock);
> + mutex_init(&dev->vb_queue_lock);
> +
> + rc = v4l2_ctrl_handler_init(&dev->ctrl_handler, 0);
> + if (rc) {
> + smi2021_err("v4l2_ctrl_handler_init failed with: %d\n", rc);
> + goto free_err;
> + }
> +
> + dev->v4l2_dev.release = release_v4l2_dev;
> + dev->v4l2_dev.ctrl_handler = &dev->ctrl_handler;
> + rc = v4l2_device_register(dev->dev, &dev->v4l2_dev);
> + if (rc) {
> + smi2021_err("v4l2_device_register failed with %d\n", rc);
> + goto free_ctrl;
> + }
> +
> + smi2021_reset_device(dev);
> +
> + rc = smi2021_i2c_register(dev);
> + if (rc < 0)
> + goto unreg_v4l2;
> +
> + dev->sd_saa7113 = v4l2_i2c_new_subdev(&dev->v4l2_dev, &dev->i2c_adap,
> + "saa7113", 0, saa7113_addrs);
Fill in the addr argument directly and use NULL for probe_addrs. Probing is
not something new drivers should do.
See also my comments in the i2c.c review.
> +
> + smi2021_dbg("Driver version %s successfully loaded\n",
> + SMI2021_DRIVER_VERSION);
> +
> + v4l2_device_call_all(&dev->v4l2_dev, 0, core, reset, 0);
> + v4l2_device_call_all(&dev->v4l2_dev, 0, video, s_stream, 0);
> +
> + rc = smi2021_snd_register(dev);
> + if (rc < 0)
> + goto unreg_i2c;
> +
> + rc = smi2021_video_register(dev);
> + if (rc < 0)
> + goto unreg_snd;
> +
> + return 0;
> +
> +unreg_snd:
> + smi2021_snd_unregister(dev);
> +unreg_i2c:
> + smi2021_i2c_unregister(dev);
> +unreg_v4l2:
> + v4l2_device_unregister(&dev->v4l2_dev);
> +free_ctrl:
> + v4l2_ctrl_handler_free(&dev->ctrl_handler);
> +free_err:
> + kfree(dev);
> +
> + return rc;
> +}
> +
> +static void smi2021_usb_disconnect(struct usb_interface *intf)
> +{
> + struct smi2021_dev *dev = usb_get_intfdata(intf);
> +
> + smi2021_dbg("Going for release!\n");
> +
> + usb_set_intfdata(intf, NULL);
> +
> + mutex_lock(&dev->vb_queue_lock);
> + mutex_lock(&dev->v4l2_lock);
> +
> + smi2021_uninit_isoc(dev);
> + smi2021_clear_queue(dev);
> +
> + video_unregister_device(&dev->vdev);
> + v4l2_device_disconnect(&dev->v4l2_dev);
> +
> + /* This way current users can detect device is gone */
> + dev->udev = NULL;
> +
> + mutex_unlock(&dev->v4l2_lock);
> + mutex_unlock(&dev->vb_queue_lock);
> +
> + smi2021_snd_unregister(dev);
> +
> + /*
> + * This calls release_v4l2_dev if it's the last reference.
> + * Otherwise, the release is postponed until there are no users left.
> + */
> + v4l2_device_put(&dev->v4l2_dev);
> +}
> +
> +/******************************************************************************/
> +/* */
> +/* MODULE - INIT & EXIT */
> +/* */
> +/******************************************************************************/
> +
> +struct usb_driver smi2021_usb_driver = {
> + .name = "smi2021",
> + .id_table = smi2021_usb_device_id_table,
> + .probe = smi2021_usb_probe,
> + .disconnect = smi2021_usb_disconnect
> +};
> +
> +module_usb_driver(smi2021_usb_driver);
> +
>
Regards,
Hans
--
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/
Powered by blists - more mailing lists