|
|
|
Re: [please review patch] dsbr100: fix unplug oops | |
| [Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] | |
On Mon, Dec 8, 2008 at 8:35 PM, Alexey Klimov <klimov.linux@xxxxxxxxx> wrote:
> Hello, all
>
> Patch adds new macros - videodev_to_radio. Make it sense ?
> Mutex lock added, a lot of safety checks. "struct video_device videodev"
> is embedded in dsbr100_device structure that works up in addition many
> address of operands. Memcpy removed.
>
>
> Please, check if have free time..
>
> ---
> diff -r 6bab82c6096e linux/drivers/media/radio/dsbr100.c
> --- a/linux/drivers/media/radio/dsbr100.c Wed Dec 03 15:32:11 2008 -0200
> +++ b/linux/drivers/media/radio/dsbr100.c Tue Dec 09 04:23:43 2008 +0300
> @@ -146,6 +146,7 @@
> #define FREQ_MAX 108.0
> #define FREQ_MUL 16000
>
> +#define videodev_to_radio(d) container_of(d, struct dsbr100_device, videodev)
>
> static int usb_dsbr100_probe(struct usb_interface *intf,
> const struct usb_device_id *id);
> @@ -162,8 +163,9 @@
> /* Data for one (physical) device */
> struct dsbr100_device {
> struct usb_device *usbdev;
> - struct video_device *videodev;
> + struct video_device videodev;
> u8 *transfer_buffer;
> + struct mutex lock; /* buffer locking */
> int curfreq;
> int stereo;
> int users;
> @@ -198,6 +200,7 @@
> /* switch on radio */
> static int dsbr100_start(struct dsbr100_device *radio)
> {
> + mutex_lock(&radio->lock);
> if (usb_control_msg(radio->usbdev, usb_rcvctrlpipe(radio->usbdev, 0),
> USB_REQ_GET_STATUS,
> USB_TYPE_VENDOR | USB_RECIP_DEVICE | USB_DIR_IN,
> @@ -205,8 +208,12 @@
> usb_control_msg(radio->usbdev, usb_rcvctrlpipe(radio->usbdev, 0),
> DSB100_ONOFF,
> USB_TYPE_VENDOR | USB_RECIP_DEVICE | USB_DIR_IN,
> - 0x01, 0x00, radio->transfer_buffer, 8, 300) < 0)
> + 0x01, 0x00, radio->transfer_buffer, 8, 300) < 0) {
> + mutex_unlock(&radio->lock);
> return -1;
> + }
> +
> + mutex_unlock(&radio->lock);
> radio->muted=0;
The muted member should be set within the context of the lock.
> return (radio->transfer_buffer)[0];
> }
> @@ -215,6 +222,7 @@
> /* switch off radio */
> static int dsbr100_stop(struct dsbr100_device *radio)
> {
> + mutex_lock(&radio->lock);
> if (usb_control_msg(radio->usbdev, usb_rcvctrlpipe(radio->usbdev, 0),
> USB_REQ_GET_STATUS,
> USB_TYPE_VENDOR | USB_RECIP_DEVICE | USB_DIR_IN,
> @@ -222,8 +230,12 @@
> usb_control_msg(radio->usbdev, usb_rcvctrlpipe(radio->usbdev, 0),
> DSB100_ONOFF,
> USB_TYPE_VENDOR | USB_RECIP_DEVICE | USB_DIR_IN,
> - 0x00, 0x00, radio->transfer_buffer, 8, 300) < 0)
> + 0x00, 0x00, radio->transfer_buffer, 8, 300) < 0) {
> + mutex_unlock(&radio->lock);
> return -1;
> + }
> +
> + mutex_unlock(&radio->lock);
> radio->muted=1;
The muted member should be set within the context of the lock.
> return (radio->transfer_buffer)[0];
> }
> @@ -232,6 +244,7 @@
> static int dsbr100_setfreq(struct dsbr100_device *radio, int freq)
> {
> freq = (freq / 16 * 80) / 1000 + 856;
> + mutex_lock(&radio->lock);
> if (usb_control_msg(radio->usbdev, usb_rcvctrlpipe(radio->usbdev, 0),
> DSB100_TUNE,
> USB_TYPE_VENDOR | USB_RECIP_DEVICE | USB_DIR_IN,
> @@ -246,9 +259,12 @@
> USB_TYPE_VENDOR | USB_RECIP_DEVICE | USB_DIR_IN,
> 0x00, 0x24, radio->transfer_buffer, 8, 300) < 0) {
> radio->stereo = -1;
> + mutex_unlock(&radio->lock);
> return -1;
> }
> +
> radio->stereo = !((radio->transfer_buffer)[0] & 0x01);
> + mutex_unlock(&radio->lock);
> return (radio->transfer_buffer)[0];
> }
>
> @@ -256,6 +272,7 @@
> sees a stereo signal or not. Pity. */
> static void dsbr100_getstat(struct dsbr100_device *radio)
> {
> + mutex_lock(&radio->lock);
> if (usb_control_msg(radio->usbdev, usb_rcvctrlpipe(radio->usbdev, 0),
> USB_REQ_GET_STATUS,
> USB_TYPE_VENDOR | USB_RECIP_DEVICE | USB_DIR_IN,
> @@ -263,6 +280,7 @@
> radio->stereo = -1;
> else
> radio->stereo = !(radio->transfer_buffer[0] & 0x01);
> + mutex_unlock(&radio->lock);
> }
>
>
> @@ -277,16 +295,12 @@
> struct dsbr100_device *radio = usb_get_intfdata(intf);
>
> usb_set_intfdata (intf, NULL);
> - if (radio) {
> - video_unregister_device(radio->videodev);
> - radio->videodev = NULL;
> - if (radio->users) {
> - kfree(radio->transfer_buffer);
> - kfree(radio);
> - } else {
> - radio->removed = 1;
> - }
> - }
> +
> + mutex_lock(&radio->lock);
> + radio->removed = 1;
> + mutex_unlock(&radio->lock);
> +
> + video_unregister_device(&radio->videodev);
> }
>
>
> @@ -305,6 +319,10 @@
> struct v4l2_tuner *v)
> {
> struct dsbr100_device *radio = video_drvdata(file);
> +
> + /* safety check */
> + if (radio->removed)
> + return -EIO;
>
> if (v->index > 0)
> return -EINVAL;
> @@ -327,6 +345,12 @@
> static int vidioc_s_tuner(struct file *file, void *priv,
> struct v4l2_tuner *v)
> {
> + struct dsbr100_device *radio = video_drvdata(file);
> +
> + /* safety check */
> + if (radio->removed)
> + return -EIO;
> +
> if (v->index > 0)
> return -EINVAL;
>
> @@ -338,6 +362,10 @@
> {
> struct dsbr100_device *radio = video_drvdata(file);
>
> + /* safety check */
> + if (radio->removed)
> + return -EIO;
> +
> radio->curfreq = f->frequency;
> if (dsbr100_setfreq(radio, radio->curfreq) == -1)
> dev_warn(&radio->usbdev->dev, "Set frequency failed\n");
> @@ -348,6 +376,10 @@
> struct v4l2_frequency *f)
> {
> struct dsbr100_device *radio = video_drvdata(file);
> +
> + /* safety check */
> + if (radio->removed)
> + return -EIO;
>
> f->type = V4L2_TUNER_RADIO;
> f->frequency = radio->curfreq;
> @@ -373,6 +405,10 @@
> {
> struct dsbr100_device *radio = video_drvdata(file);
>
> + /* safety check */
> + if (radio->removed)
> + return -EIO;
> +
> switch (ctrl->id) {
> case V4L2_CID_AUDIO_MUTE:
> ctrl->value = radio->muted;
> @@ -385,6 +421,10 @@
> struct v4l2_control *ctrl)
> {
> struct dsbr100_device *radio = video_drvdata(file);
> +
> + /* safety check */
> + if (radio->removed)
> + return -EIO;
>
> switch (ctrl->id) {
> case V4L2_CID_AUDIO_MUTE:
> @@ -467,13 +507,19 @@
> static int usb_dsbr100_close(struct inode *inode, struct file *file)
> {
> struct dsbr100_device *radio = video_drvdata(file);
> + int retval;
>
> if (!radio)
> return -ENODEV;
> +
> radio->users = 0;
> - if (radio->removed) {
> - kfree(radio->transfer_buffer);
> - kfree(radio);
> + if (!radio->removed) {
> + retval = dsbr100_stop(radio);
> + if (retval == -1) {
> + dev_warn(&radio->usbdev->dev,
> + "dsbr100_stop failed\n");
> + }
> +
> }
> return 0;
> }
> @@ -508,6 +554,14 @@
> return 0;
> }
>
> +static void usb_dsbr100_video_device_release(struct video_device *videodev)
> +{
> + struct dsbr100_device *radio = videodev_to_radio(videodev);
> +
> + kfree(radio->transfer_buffer);
> + kfree(radio);
> +}
> +
> /* File system interface */
> static const struct file_operations usb_dsbr100_fops = {
> .owner = THIS_MODULE,
> @@ -536,11 +590,11 @@
> };
>
> /* V4L2 interface */
> -static struct video_device dsbr100_videodev_template = {
> +static struct video_device dsbr100_videodev_data = {
> .name = "D-Link DSB-R 100",
> .fops = &usb_dsbr100_fops,
> .ioctl_ops = &usb_dsbr100_ioctl_ops,
> - .release = video_device_release,
> + .release = usb_dsbr100_video_device_release,
> };
>
> /* check if the device is present and register with v4l and
> @@ -561,23 +615,17 @@
> kfree(radio);
> return -ENOMEM;
> }
> - radio->videodev = video_device_alloc();
>
> - if (!(radio->videodev)) {
> - kfree(radio->transfer_buffer);
> - kfree(radio);
> - return -ENOMEM;
> - }
> - memcpy(radio->videodev, &dsbr100_videodev_template,
> - sizeof(dsbr100_videodev_template));
> + mutex_init(&radio->lock);
> + radio->videodev = dsbr100_videodev_data;
> +
> radio->removed = 0;
> radio->users = 0;
> radio->usbdev = interface_to_usbdev(intf);
> radio->curfreq = FREQ_MIN * FREQ_MUL;
> - video_set_drvdata(radio->videodev, radio);
> - if (video_register_device(radio->videodev, VFL_TYPE_RADIO, radio_nr) < 0) {
> + video_set_drvdata(&radio->videodev, radio);
> + if (video_register_device(&radio->videodev, VFL_TYPE_RADIO, radio_nr) < 0) {
> dev_warn(&intf->dev, "Could not register video device\n");
> - video_device_release(radio->videodev);
> kfree(radio->transfer_buffer);
> kfree(radio);
> return -EIO;
>
>
> --
> Best regards, Klimov Alexey
>
>
--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@xxxxxxxxxx?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list
[Linux Media] [Older V4L] [Linux DVB] [Video Disk Recorder] [Linux Kernel] [Asterisk] [Photo] [DCCP] [Netdev] [Xorg] [Util Linux NG] [Xfree86] [Free Photo Albums] [Fedora Users] [Fedora Women] [ALSA Users] [ALSA Devel] [SSH] [DVB Maintainers] [Linux USB] [Yosemite Information]
|
|