Re: [RFCv2 PATCH 06/10] radio_ms800: use video_drvdata instead of filp->private_data
Why does this matter? From my understanding, v4l2_fh is meant to be embed it in a device specific structure. If it is the first member in the device specific structure, then the pointer to v4l2_fh is the same as the one to the device specific structure. Otherwise, a simple container_of can be used to calculate the appropriate address of the container. In either case, the pointer calculation method is always going to be faster than executing a call, and de-referencing multiple pointers to retrieve the same information. filp->private data is there for a reason, why not use it as intended and avoid the additional overhead added by this patch? Regards, David Ellingsworth -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFCv2 PATCH 07/10] radio-mr800: remove autopm support.
>From my understanding, auto power management is for automatically suspending and resuming a driver whenever it is idle. Obviously this is a bad for this type of driver as it would turn off the radio whenever it was idle. It is not necessary to remove suspend/resume support in order to drop auto power management from this driver. In fact doing so would be a mistake in my opinion. The current suspend/resume cycle ensures the radio if off during suspend, and restores it's last state during resume. These changes would leave the radio in it's current state, consuming power if it were on, while the system is suspended. This is a drastic deviation from the current behavior and would most likely not be appreciated by users that expect the device to go off during suspend and back on after resume. I NACK this change due to the complete removal of suspend/resume support. Regards, David Ellingsworth -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Volunteers needed: BKL removal: replace .ioctl by .unlocked_ioctl
On Sat, Dec 18, 2010 at 6:31 AM, Hans Verkuil wrote: > Hi all, > > Now that the BKL patch series has been merged in 2.6.37 it is time to work > on replacing .ioctl by .unlocked_ioctl in all v4l drivers. > > I've made an inventory of all drivers that still use .ioctl and I am looking > for volunteers to tackle one or more drivers. > > I have CCed this email to the maintainers of the various drivers (if I know > who it is) in the hope that we can get this conversion done as quickly as > possible. > > If I have added your name to a driver, then please confirm if you are able to > work on it or not. If you can't work on it, but you know someone else, then > let me know as well. > > There is also a list of drivers where I do not know who can do the conversion. > If you can tackle one or more of those, please respond. Unfortunately, those > are among the hardest to convert :-( > > It would be great if we can tackle most of these drivers for 2.6.38. I think > we should finish all drivers for 2.6.39 at the latest. > > There are two ways of doing the conversion: one is to do all the locking > within > the driver, the other is to use core-assisted locking. How to do the > core-assisted > locking is described in Documentation/video4linux/v4l2-framework.txt, but I'll > repeat the relevant part here: > > v4l2_file_operations and locking > > > You can set a pointer to a mutex_lock in struct video_device. Usually this > will be either a top-level mutex or a mutex per device node. If you want > finer-grained locking then you have to set it to NULL and do you own locking. > > If a lock is specified then all file operations will be serialized on that > lock. If you use videobuf then you must pass the same lock to the videobuf > queue initialize function: if videobuf has to wait for a frame to arrive, then > it will temporarily unlock the lock and relock it afterwards. If your driver > also waits in the code, then you should do the same to allow other processes > to access the device node while the first process is waiting for something. > > The implementation of a hotplug disconnect should also take the lock before > calling v4l2_device_disconnect. > > > Driver list: > > saa7146 (Hans Verkuil) > mem2mem_testdev (Pawel Osciak or Marek Szyprowski) > cx23885 (Steve Toth) > cx18-alsa (Andy Walls) > omap24xxcam (Sakari Ailus or David Cohen) > au0828 (Janne Grunau) > cpia2 (Andy Walls or Hans Verkuil) > cx231xx (Mauro Carvalho Chehab) > davinci (Muralidharan Karicheri) > saa6588 (Hans Verkuil) > pvrusb2 (Mike Isely) > usbvision (Hans Verkuil) > s5p-fimc (Sylwester Nawrocki) > fsl-viu (Anatolij Gustschin) > tlg2300 (Mauro Carvalho Chehab) > zr364xx (Hans de Goede) > soc_camera (Guennadi Liakhovetski) > usbvideo/vicam (Hans de Goede) > s2255drv (Pete Eberlein) > bttv (Mauro Carvalho Chehab) > stk-webcam (Hans de Goede) > se401 (Hans de Goede) > si4713-i2c (Hans Verkuil) > dsbr100 (Hans Verkuil) There are patches for dsbr100 here: http://desource.dyndns.org/~atog/gitweb/?p=linux-media.git These patches don't use the new core-assisted locking as they were written back in MAY, but they DO remove the BKL from the driver. The patches have been compile tested only, as I do not have access to a device for this driver. > > Staging driver list: > > go7007 (Pete Eberlein) > tm6000 (Mauro Carvalho Chehab) > (stradis/cpia: will be removed in 2.6.38, so no need to do anything) > > Unassigned drivers: > > saa7134 > em28xx > cx88 > solo6x10 (staging driver) > > Regards, > > Hans > Regards, David Ellingsworth -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFCv2 PATCH 07/15] dsbr100: convert to unlocked_ioctl.
On Thu, Nov 18, 2010 at 10:29 AM, Hans Verkuil wrote: > >> On Thu, Nov 18, 2010 at 9:46 AM, Hans Verkuil wrote: >>> >>>> This driver has quite a few locking issues that would only be made >>>> worse by your patch. A much better patch for this can be found here: >>>> >>>> http://desource.dyndns.org/~atog/gitweb?p=linux-media.git;a=commitdiff;h=9c5d8ebb602e9af46902c5f3d4d4cc80227d3f7c >>> >>> Much too big for 2.6.37. I'll just drop this patch from my patch series. >>> Instead it will rely on the new lock in v4l2_device (BKL replacement) >>> that >>> serializes all ioctls. For 2.6.38 we can convert it to core assisted >>> locking which is much easier. >>> >> >> I don't see how this patch is really any bigger than some of the >> others in your series. Sure there are a lot of deletions, but the >> number of additions is quite small. There are much worse ways all the >> locking issues in this driver could have been corrected. This patch >> manages to do just that while reducing the overall size of the driver >> at the same time. > > Basically there are two reasons why I'm not including this in my patch > series: 1) you disagreed with my patch and 2) I disagree with your patch. > > In this case the patch for 2.6.37 should either be small and trivial, or > we should postpone it to 2.6.38 and use proper core-assisted locking > (which makes the most sense for this driver in my opinion). And I really > dislike those BUG_ONs you've added, to be honest. Sorry about that... The reality is that the BUG_ONs should never be hit. I added those because I don't have the hardware needed to test my changes. So if I missed something, the issue would be very apparent to whoever was doing the testing. If someone would actually test this series, the BUG_ONs could be removed. Maybe a WARN_ON would have been better, but to me accessing the device struct without holding the lock is a bug. The only valid case where it would be okay to do that would be from within the probe function before the device is registered. > > Anyway, feel free to post your own patch for this driver. I've no problems > with that at all. It is clear that this driver takes more time to get a > proper patch for that makes everyone happy, and I really need to make the > git pull request for 2.6.37 tomorrow as we don't want to do this too late > in the 2.6.37 cycle. > I understand, but if you or anyone else had actually bothered to review this patch when it was submitted back in May then it might have made its way into the kernel by now. At the time the patch was written, core assisted locking wasn't even implemented. Granted the ioctl added to correct the locking might cause a cache miss, but this is acceptable for such a simple driver. This is a good first step towards core assisted locking. Regards, David Ellingsworth -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFCv2 PATCH 07/15] dsbr100: convert to unlocked_ioctl.
On Thu, Nov 18, 2010 at 9:46 AM, Hans Verkuil wrote: > >> This driver has quite a few locking issues that would only be made >> worse by your patch. A much better patch for this can be found here: >> >> http://desource.dyndns.org/~atog/gitweb?p=linux-media.git;a=commitdiff;h=9c5d8ebb602e9af46902c5f3d4d4cc80227d3f7c > > Much too big for 2.6.37. I'll just drop this patch from my patch series. > Instead it will rely on the new lock in v4l2_device (BKL replacement) that > serializes all ioctls. For 2.6.38 we can convert it to core assisted > locking which is much easier. > I don't see how this patch is really any bigger than some of the others in your series. Sure there are a lot of deletions, but the number of additions is quite small. There are much worse ways all the locking issues in this driver could have been corrected. This patch manages to do just that while reducing the overall size of the driver at the same time. Regards, David Ellingsworth -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFCv2 PATCH 07/15] dsbr100: convert to unlocked_ioctl.
This driver has quite a few locking issues that would only be made worse by your patch. A much better patch for this can be found here: http://desource.dyndns.org/~atog/gitweb?p=linux-media.git;a=commitdiff;h=9c5d8ebb602e9af46902c5f3d4d4cc80227d3f7c Regards, David Ellingsworth -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH 0/8] V4L BKL removal: first round
On Tue, Nov 16, 2010 at 4:42 PM, Hans Verkuil wrote: > On Tuesday, November 16, 2010 22:32:57 David Ellingsworth wrote: >> Hans, >> >> I've had some patches pending for a while now that affect the dsbr100 >> driver. The patches can be seen here: >> http://desource.dyndns.org/~atog/gitweb/?p=linux-media.git in the >> dsbr100 branch. The first patch in the series fixes locking issues >> throughout the driver and converts it to use the unlocked ioctl. The >> series is a bit old, so it doesn't make use of the v4l2 core assisted >> locking; but that is trivial to implement after this patch. > > Would it be a problem for you if for 2.6.37 I just replace .ioctl by > .unlocked_ioctl? And do the full conversion for 2.6.38? That way the > 2.6.37 patches remain small. > If you look at the first patch in that series, you'll see that the conversion isn't that simple. There are a lot of places in that driver that should have been protected by a lock that weren't. At the moment, I think the BKL protects these areas from racing so just replacing the .ioctl with the .unlocked_ioctl here isn't a good solution for this driver. Applying the patch I've provided will however remove the BKL and the resolve all the locking issues as well. Regards, David Ellingsworth -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH 0/8] V4L BKL removal: first round
Hans, I've had some patches pending for a while now that affect the dsbr100 driver. The patches can be seen here: http://desource.dyndns.org/~atog/gitweb/?p=linux-media.git in the dsbr100 branch. The first patch in the series fixes locking issues throughout the driver and converts it to use the unlocked ioctl. The series is a bit old, so it doesn't make use of the v4l2 core assisted locking; but that is trivial to implement after this patch. Regards, David Ellingsworth -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH] radio-mr800: locking fixes
> Any conclusion about the locking fixes patch? > I don't like it, but it at least fixes the problem. You can add my ack. Acked-by: David Ellingsworth -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH] radio-mr800: locking fixes
On Mon, Oct 18, 2010 at 9:20 AM, Hans Verkuil wrote: > >> On Sun, Oct 17, 2010 at 8:52 AM, Hans Verkuil wrote: >>> On Sunday, October 17, 2010 14:26:18 Hans Verkuil wrote: >>>> - serialize the suspend and resume functions using the global lock. >>>> - do not call usb_autopm_put_interface after a disconnect. >>>> - fix a race when disconnecting the device. >>> >>> Regarding autosuspend: something seems to work since the >>> power/runtime_status >>> attribute goes from 'suspended' to 'active' whenever the radio handle is >>> open. >>> But the suspend and resume functions are never called. I can't figure >>> out >>> why not. I don't see anything strange. >>> >>> The whole autopm stuff is highly suspect anyway on a device like this >>> since >>> it is perfectly reasonable to just set a frequency and exit. The audio >>> is >>> just going to the line-in anyway. In other words: not having the device >>> node >>> open does not mean that the device is idle and can be suspended. >>> >>> My proposal would be to rip out the whole autosuspend business from this >>> driver. I've no idea why it is here at all. >>> >>> Regards, >>> >>> Hans >> >> Hans, I highly agree with that analysis. The original author put that >> code in. But like you, I'm not sure if it was ever really valid. Since >> I didn't have anything to test with, I left it untouched. >> >> Regards, >> >> David Ellingsworth >> >> > > OK, then I'll make a new patch that just rips out autosuspend support. I thought about this a little more. I think this driver could benefit from auto-suspend, but it's current implementation is indeed flawed. The calls to usb_autopm_put/get_interface could occur whenever the device is muted/unmuted respectively after the device has been initialized. Thus, the suspend should not happen while the device is in use per se, but could occur after it's been muted. Regards, David Ellingsworth -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH] radio-mr800: locking fixes
On Mon, Oct 18, 2010 at 10:35 AM, David Ellingsworth wrote: > On Mon, Oct 18, 2010 at 10:18 AM, Hans Verkuil wrote: >> >>> On Mon, Oct 18, 2010 at 9:38 AM, Hans Verkuil wrote: >>>> >>>>> On Sun, Oct 17, 2010 at 8:26 AM, Hans Verkuil >>>>> wrote: >>>>>> - serialize the suspend and resume functions using the global lock. >>>>>> - do not call usb_autopm_put_interface after a disconnect. >>>>>> - fix a race when disconnecting the device. >>>>>> >>>>>> Reported-by: David Ellingsworth >>>>>> Signed-off-by: Hans Verkuil >>>>>> --- >>>>>> drivers/media/radio/radio-mr800.c | 17 ++--- >>>>>> 1 files changed, 14 insertions(+), 3 deletions(-) >>>>>> >>>>>> diff --git a/drivers/media/radio/radio-mr800.c >>>>>> b/drivers/media/radio/radio-mr800.c >>>>>> index 2f56b26..b540e80 100644 >>>>>> --- a/drivers/media/radio/radio-mr800.c >>>>>> +++ b/drivers/media/radio/radio-mr800.c >>>>>> @@ -284,9 +284,13 @@ static void usb_amradio_disconnect(struct >>>>>> usb_interface *intf) >>>>>> struct amradio_device *radio = >>>>>> to_amradio_dev(usb_get_intfdata(intf)); >>>>>> >>>>>> mutex_lock(&radio->lock); >>>>>> + /* increase the device node's refcount */ >>>>>> + get_device(&radio->videodev.dev); >>>>>> v4l2_device_disconnect(&radio->v4l2_dev); >>>>>> - mutex_unlock(&radio->lock); >>>>>> video_unregister_device(&radio->videodev); >>>>>> + mutex_unlock(&radio->lock); >>>>>> + /* decrease the device node's refcount, allowing it to be >>>>>> released */ >>>>>> + put_device(&radio->videodev.dev); >>>>>> } >>>>> >>>>> Hans, I understand the use of get/put_device here.. but can you >>>>> explain to me what issue you are trying to solve? >>>>> video_unregister_device does not have to be synchronized with anything >>>>> else. Thus, it is perfectly safe to call video_unregister_device while >>>>> not holding the device lock. Your prior implementation here was >>>>> correct. >>>> >>>> This the original sequence: >>>> >>>> mutex_lock(&radio->lock); >>>> v4l2_device_disconnect(&radio->v4l2_dev); >>>> mutex_unlock(&radio->lock); >>>> video_unregister_device(&radio->videodev); >>>> >>>> The problem with this is that userspace can call open or ioctl after the >>>> unlock and before the device node is marked unregistered by >>>> video_unregister_device. >>>> >>>> Once you disconnect you want to block all access (except the release >>>> call). >>>> What my patch does is to move the video_unregister_device call inside >>>> the >>>> lock, but then I have to guard against the release being called before >>>> the >>>> unlock by increasing the refcount. >>>> >>>> I have ideas to improve on this as this gets hairy when you have >>>> multiple >>>> device nodes, but I wait with that until the next kernel cycle. >>>> >>>> Regards, >>>> >>>> Hans >>>> >>> >>> I think you're trying to solve a problem that doesn't exist. >>> To be a little more specific we have the following: >>> >>> 1. video_register_device - increments device refcount >>> 2. video_unregister_device - decrements device refcount >>> 3. v4l2_open - increments device refcount >>> 4. v4l2_release - decrements device refcount >>> >>> Keeping this in mind, the release callback of video_device is called >>> only when the device count reaches 0. >>> >>> So under normal operation we have: >>> >>> 1. video_register_device -> device refcount incremented to 1 >>> 2. v4l2_open -> device refcount incremented to 2 >>> 3. v4l2_release -> device refcount decremented to 1 >>> 4. disconnect callback: video_unregister_device -> device refcount >>> decremented to 0 & release ca
Re: [RFC PATCH] radio-mr800: locking fixes
On Mon, Oct 18, 2010 at 10:18 AM, Hans Verkuil wrote: > >> On Mon, Oct 18, 2010 at 9:38 AM, Hans Verkuil wrote: >>> >>>> On Sun, Oct 17, 2010 at 8:26 AM, Hans Verkuil >>>> wrote: >>>>> - serialize the suspend and resume functions using the global lock. >>>>> - do not call usb_autopm_put_interface after a disconnect. >>>>> - fix a race when disconnecting the device. >>>>> >>>>> Reported-by: David Ellingsworth >>>>> Signed-off-by: Hans Verkuil >>>>> --- >>>>> drivers/media/radio/radio-mr800.c | 17 ++--- >>>>> 1 files changed, 14 insertions(+), 3 deletions(-) >>>>> >>>>> diff --git a/drivers/media/radio/radio-mr800.c >>>>> b/drivers/media/radio/radio-mr800.c >>>>> index 2f56b26..b540e80 100644 >>>>> --- a/drivers/media/radio/radio-mr800.c >>>>> +++ b/drivers/media/radio/radio-mr800.c >>>>> @@ -284,9 +284,13 @@ static void usb_amradio_disconnect(struct >>>>> usb_interface *intf) >>>>> struct amradio_device *radio = >>>>> to_amradio_dev(usb_get_intfdata(intf)); >>>>> >>>>> mutex_lock(&radio->lock); >>>>> + /* increase the device node's refcount */ >>>>> + get_device(&radio->videodev.dev); >>>>> v4l2_device_disconnect(&radio->v4l2_dev); >>>>> - mutex_unlock(&radio->lock); >>>>> video_unregister_device(&radio->videodev); >>>>> + mutex_unlock(&radio->lock); >>>>> + /* decrease the device node's refcount, allowing it to be >>>>> released */ >>>>> + put_device(&radio->videodev.dev); >>>>> } >>>> >>>> Hans, I understand the use of get/put_device here.. but can you >>>> explain to me what issue you are trying to solve? >>>> video_unregister_device does not have to be synchronized with anything >>>> else. Thus, it is perfectly safe to call video_unregister_device while >>>> not holding the device lock. Your prior implementation here was >>>> correct. >>> >>> This the original sequence: >>> >>> mutex_lock(&radio->lock); >>> v4l2_device_disconnect(&radio->v4l2_dev); >>> mutex_unlock(&radio->lock); >>> video_unregister_device(&radio->videodev); >>> >>> The problem with this is that userspace can call open or ioctl after the >>> unlock and before the device node is marked unregistered by >>> video_unregister_device. >>> >>> Once you disconnect you want to block all access (except the release >>> call). >>> What my patch does is to move the video_unregister_device call inside >>> the >>> lock, but then I have to guard against the release being called before >>> the >>> unlock by increasing the refcount. >>> >>> I have ideas to improve on this as this gets hairy when you have >>> multiple >>> device nodes, but I wait with that until the next kernel cycle. >>> >>> Regards, >>> >>> Hans >>> >> >> I think you're trying to solve a problem that doesn't exist. >> To be a little more specific we have the following: >> >> 1. video_register_device - increments device refcount >> 2. video_unregister_device - decrements device refcount >> 3. v4l2_open - increments device refcount >> 4. v4l2_release - decrements device refcount >> >> Keeping this in mind, the release callback of video_device is called >> only when the device count reaches 0. >> >> So under normal operation we have: >> >> 1. video_register_device -> device refcount incremented to 1 >> 2. v4l2_open -> device refcount incremented to 2 >> 3. v4l2_release -> device refcount decremented to 1 >> 4. disconnect callback: video_unregister_device -> device refcount >> decremented to 0 & release callback called. >> >> If the user disconnects the device while it's open we have the following: >> >> 1. video_register_device -> device refcount incremented to 1 >> 2. v4l2_open -> device refcount incremented to 2 >> 3. disconnect callback: video_unregister_device -> device refcount >> decremented to 1 >> 4. v4l2_release -> device refcount decremented to 0
Re: [RFC PATCH] radio-mr800: locking fixes
On Mon, Oct 18, 2010 at 9:38 AM, Hans Verkuil wrote: > >> On Sun, Oct 17, 2010 at 8:26 AM, Hans Verkuil wrote: >>> - serialize the suspend and resume functions using the global lock. >>> - do not call usb_autopm_put_interface after a disconnect. >>> - fix a race when disconnecting the device. >>> >>> Reported-by: David Ellingsworth >>> Signed-off-by: Hans Verkuil >>> --- >>> drivers/media/radio/radio-mr800.c | 17 ++--- >>> 1 files changed, 14 insertions(+), 3 deletions(-) >>> >>> diff --git a/drivers/media/radio/radio-mr800.c >>> b/drivers/media/radio/radio-mr800.c >>> index 2f56b26..b540e80 100644 >>> --- a/drivers/media/radio/radio-mr800.c >>> +++ b/drivers/media/radio/radio-mr800.c >>> @@ -284,9 +284,13 @@ static void usb_amradio_disconnect(struct >>> usb_interface *intf) >>> struct amradio_device *radio = >>> to_amradio_dev(usb_get_intfdata(intf)); >>> >>> mutex_lock(&radio->lock); >>> + /* increase the device node's refcount */ >>> + get_device(&radio->videodev.dev); >>> v4l2_device_disconnect(&radio->v4l2_dev); >>> - mutex_unlock(&radio->lock); >>> video_unregister_device(&radio->videodev); >>> + mutex_unlock(&radio->lock); >>> + /* decrease the device node's refcount, allowing it to be >>> released */ >>> + put_device(&radio->videodev.dev); >>> } >> >> Hans, I understand the use of get/put_device here.. but can you >> explain to me what issue you are trying to solve? >> video_unregister_device does not have to be synchronized with anything >> else. Thus, it is perfectly safe to call video_unregister_device while >> not holding the device lock. Your prior implementation here was >> correct. > > This the original sequence: > > mutex_lock(&radio->lock); > v4l2_device_disconnect(&radio->v4l2_dev); > mutex_unlock(&radio->lock); > video_unregister_device(&radio->videodev); > > The problem with this is that userspace can call open or ioctl after the > unlock and before the device node is marked unregistered by > video_unregister_device. > > Once you disconnect you want to block all access (except the release call). > What my patch does is to move the video_unregister_device call inside the > lock, but then I have to guard against the release being called before the > unlock by increasing the refcount. > > I have ideas to improve on this as this gets hairy when you have multiple > device nodes, but I wait with that until the next kernel cycle. > > Regards, > > Hans > I think you're trying to solve a problem that doesn't exist. To be a little more specific we have the following: 1. video_register_device - increments device refcount 2. video_unregister_device - decrements device refcount 3. v4l2_open - increments device refcount 4. v4l2_release - decrements device refcount Keeping this in mind, the release callback of video_device is called only when the device count reaches 0. So under normal operation we have: 1. video_register_device -> device refcount incremented to 1 2. v4l2_open -> device refcount incremented to 2 3. v4l2_release -> device refcount decremented to 1 4. disconnect callback: video_unregister_device -> device refcount decremented to 0 & release callback called. If the user disconnects the device while it's open we have the following: 1. video_register_device -> device refcount incremented to 1 2. v4l2_open -> device refcount incremented to 2 3. disconnect callback: video_unregister_device -> device refcount decremented to 1 4. v4l2_release -> device refcount decremented to 0 & release callback called. In the above case, once video_unregister_device has been called, calls to open no longer will work. However, the user holding the currently open file handle can still call ioctl and other callbacks, but those should be met with an -EIO, forcing them to close the open handle. The original code did this by using the usb device as an indicator to see if the device was still connected, as this functionality was not in v4l2_core. On the other hand, v4l2_core could do this for us, just by checking if the device is still registered. As you can see from the above, there are no race conditions here. Regards, David Ellingsworth -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH] radio-mr800: locking fixes
On Sun, Oct 17, 2010 at 8:26 AM, Hans Verkuil wrote: > - serialize the suspend and resume functions using the global lock. > - do not call usb_autopm_put_interface after a disconnect. > - fix a race when disconnecting the device. > > Reported-by: David Ellingsworth > Signed-off-by: Hans Verkuil > --- > drivers/media/radio/radio-mr800.c | 17 ++--- > 1 files changed, 14 insertions(+), 3 deletions(-) > > diff --git a/drivers/media/radio/radio-mr800.c > b/drivers/media/radio/radio-mr800.c > index 2f56b26..b540e80 100644 > --- a/drivers/media/radio/radio-mr800.c > +++ b/drivers/media/radio/radio-mr800.c > @@ -284,9 +284,13 @@ static void usb_amradio_disconnect(struct usb_interface > *intf) > struct amradio_device *radio = to_amradio_dev(usb_get_intfdata(intf)); > > mutex_lock(&radio->lock); > + /* increase the device node's refcount */ > + get_device(&radio->videodev.dev); > v4l2_device_disconnect(&radio->v4l2_dev); > - mutex_unlock(&radio->lock); > video_unregister_device(&radio->videodev); > + mutex_unlock(&radio->lock); > + /* decrease the device node's refcount, allowing it to be released */ > + put_device(&radio->videodev.dev); > } Hans, I understand the use of get/put_device here.. but can you explain to me what issue you are trying to solve? video_unregister_device does not have to be synchronized with anything else. Thus, it is perfectly safe to call video_unregister_device while not holding the device lock. Your prior implementation here was correct. Regards, David Ellingsworth -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH] radio-mr800: locking fixes
On Sun, Oct 17, 2010 at 8:52 AM, Hans Verkuil wrote: > On Sunday, October 17, 2010 14:26:18 Hans Verkuil wrote: >> - serialize the suspend and resume functions using the global lock. >> - do not call usb_autopm_put_interface after a disconnect. >> - fix a race when disconnecting the device. > > Regarding autosuspend: something seems to work since the power/runtime_status > attribute goes from 'suspended' to 'active' whenever the radio handle is open. > But the suspend and resume functions are never called. I can't figure out > why not. I don't see anything strange. > > The whole autopm stuff is highly suspect anyway on a device like this since > it is perfectly reasonable to just set a frequency and exit. The audio is > just going to the line-in anyway. In other words: not having the device node > open does not mean that the device is idle and can be suspended. > > My proposal would be to rip out the whole autosuspend business from this > driver. I've no idea why it is here at all. > > Regards, > > Hans Hans, I highly agree with that analysis. The original author put that code in. But like you, I'm not sure if it was ever really valid. Since I didn't have anything to test with, I left it untouched. Regards, David Ellingsworth -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [GIT PATCHES FOR 2.6.37] Move V4L2 locking into the core framework
On Mon, Oct 11, 2010 at 2:05 PM, David Ellingsworth wrote: > On Mon, Oct 11, 2010 at 11:40 AM, Hans Verkuil wrote: >> On Sunday, October 10, 2010 19:33:48 David Ellingsworth wrote: >>> Hans, >>> >>> On Sun, Sep 26, 2010 at 8:25 AM, Hans Verkuil wrote: >>> > Hi Mauro, >>> > >>> > These are the locking patches. It's based on my previous test tree, but >>> > with >>> > more testing with em28xx and radio-mr800 and some small tweaks relating to >>> > disconnect handling and video_is_registered(). >>> > >>> > I also removed the unused get_unmapped_area file op and I am now blocking >>> > any further (unlocked_)ioctl calls after the device node is unregistered. >>> > The only things an application can do legally after a disconnect is >>> > unmap() >>> > and close(). >>> > >>> > This patch series also contains a small em28xx fix that I found while >>> > testing >>> > the em28xx BKL removal patch. >>> > >>> > Regards, >>> > >>> > Hans >>> > >>> > The following changes since commit >>> > dace3857de7a16b83ae7d4e13c94de8e4b267d2a: >>> > Hans Verkuil (1): >>> > V4L/DVB: tvaudio: remove obsolete tda8425 initialization >>> > >>> > are available in the git repository at: >>> > >>> > ssh://linuxtv.org/git/hverkuil/v4l-dvb.git bkl >>> > >>> > Hans Verkuil (10): >>> > v4l2-dev: after a disconnect any ioctl call will be blocked. >>> > v4l2-dev: remove get_unmapped_area >>> > v4l2: add core serialization lock. >>> > videobuf: prepare to make locking optional in videobuf >>> > videobuf: add ext_lock argument to the queue init functions >>> > videobuf: add queue argument to videobuf_waiton() >>> > vivi: remove BKL. >>> > em28xx: remove BKL. >>> > em28xx: the default std was not passed on to the subdevs >>> > radio-mr800: remove BKL >>> >>> Did you even test these patches? >> >> Yes, I did test. And it works for me. But you are correct in that it >> shouldn't >> work since the struct will indeed be freed. I'll fix this and post a patch. >> >> I'm not sure why it works fine when I test it. >> >> There is a problem as well with unlocking before unregistering the device in >> that it leaves a race condition where another app can open the device again >> before it is registered. I have to think about that some more. > > Actually, no this problem did not exist. The previous version of the > driver cleared the USB device member to indicate that the device had > been disconnected. During open, if the USB device member was null, it > aborted with -EIO. If there's a race there now, it's only because you > introduced it. > > One thing I noticed while looking at this driver is that there's a > call to usb_autopm_put_interface in usb_amradio_close. I'm not sure if > it's a problem or not, but is it valid to call that after the device > has been disconnected? I only ask, because it wasn't called in > previous versions if the driver was disconnected before all handles to > the device were closed. If it's incorrect to call it within this > context, then this introduces another bug as well. It seems logical > that for every get there should be a put, but I don't know in this > case. Just came across this in power-management.txt kernel documentation: 343 Drivers need not be concerned about balancing changes to the usage 344 counter; the USB core will undo any remaining "get"s when a driver 345 is unbound from its interface. As a corollary, drivers must not call 346 any of the usb_autopm_* functions after their diconnect() routine has 347 returned. According to this documentation, the usb_autopm_put_interface call in usb_amradio_close should not occur if the device has been disconnected. The code you removed, used to prevent this special case. Regards, David Ellingsworth -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [GIT PATCHES FOR 2.6.37] Move V4L2 locking into the core framework
Hans, I noticed a couple more issues with this series. In your changes to radio-mr800, you removed the lock from usb_amradio_suspend and usb_amradio_resume without implementing resume/suspend support in the v4l2 core. Without resume/suspend support in the v4l2 core, the locking within usb_amradio_suspend and usb_amradio_resume must remain to prevent races between other open/close/ioctl/read/mmap/etc and the resume/suspend cycle. Please revert the changes you made to these two functions. Regards, David Ellingsworth -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [GIT PATCHES FOR 2.6.37] Fix locking order in radio-mr800
On Mon, Oct 11, 2010 at 11:41 AM, Hans Verkuil wrote: > The following changes since commit 9147e3dbca0712a5435cd2ea7c48d39344f904eb: > Mauro Carvalho Chehab (1): > V4L/DVB: cx231xx: use core-assisted lock > > are available in the git repository at: > > ssh://linuxtv.org/git/hverkuil/v4l-dvb.git mr800 > > Hans Verkuil (1): > radio-mr800: fix locking order > Acked-By: David Ellingsworth -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [GIT PATCHES FOR 2.6.37] Move V4L2 locking into the core framework
On Mon, Oct 11, 2010 at 11:40 AM, Hans Verkuil wrote: > On Sunday, October 10, 2010 19:33:48 David Ellingsworth wrote: >> Hans, >> >> On Sun, Sep 26, 2010 at 8:25 AM, Hans Verkuil wrote: >> > Hi Mauro, >> > >> > These are the locking patches. It's based on my previous test tree, but >> > with >> > more testing with em28xx and radio-mr800 and some small tweaks relating to >> > disconnect handling and video_is_registered(). >> > >> > I also removed the unused get_unmapped_area file op and I am now blocking >> > any further (unlocked_)ioctl calls after the device node is unregistered. >> > The only things an application can do legally after a disconnect is unmap() >> > and close(). >> > >> > This patch series also contains a small em28xx fix that I found while >> > testing >> > the em28xx BKL removal patch. >> > >> > Regards, >> > >> > Hans >> > >> > The following changes since commit >> > dace3857de7a16b83ae7d4e13c94de8e4b267d2a: >> > Hans Verkuil (1): >> > V4L/DVB: tvaudio: remove obsolete tda8425 initialization >> > >> > are available in the git repository at: >> > >> > ssh://linuxtv.org/git/hverkuil/v4l-dvb.git bkl >> > >> > Hans Verkuil (10): >> > v4l2-dev: after a disconnect any ioctl call will be blocked. >> > v4l2-dev: remove get_unmapped_area >> > v4l2: add core serialization lock. >> > videobuf: prepare to make locking optional in videobuf >> > videobuf: add ext_lock argument to the queue init functions >> > videobuf: add queue argument to videobuf_waiton() >> > vivi: remove BKL. >> > em28xx: remove BKL. >> > em28xx: the default std was not passed on to the subdevs >> > radio-mr800: remove BKL >> >> Did you even test these patches? > > Yes, I did test. And it works for me. But you are correct in that it shouldn't > work since the struct will indeed be freed. I'll fix this and post a patch. > > I'm not sure why it works fine when I test it. > > There is a problem as well with unlocking before unregistering the device in > that it leaves a race condition where another app can open the device again > before it is registered. I have to think about that some more. Actually, no this problem did not exist. The previous version of the driver cleared the USB device member to indicate that the device had been disconnected. During open, if the USB device member was null, it aborted with -EIO. If there's a race there now, it's only because you introduced it. One thing I noticed while looking at this driver is that there's a call to usb_autopm_put_interface in usb_amradio_close. I'm not sure if it's a problem or not, but is it valid to call that after the device has been disconnected? I only ask, because it wasn't called in previous versions if the driver was disconnected before all handles to the device were closed. If it's incorrect to call it within this context, then this introduces another bug as well. It seems logical that for every get there should be a put, but I don't know in this case. > >> >> Mauro, you should be ashamed for accepting a series that obviously has >> issues. > > Hardly obvious, and definitely not his fault. > This comment was more general, since Mauro admitted having to make changes to your series to get it to compile under i386 architectures. Regards, David Ellingsworth -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [GIT PATCHES FOR 2.6.37] Move V4L2 locking into the core framework
On Sun, Oct 10, 2010 at 1:33 PM, David Ellingsworth wrote: > Hans, > > On Sun, Sep 26, 2010 at 8:25 AM, Hans Verkuil wrote: >> Hi Mauro, >> >> These are the locking patches. It's based on my previous test tree, but with >> more testing with em28xx and radio-mr800 and some small tweaks relating to >> disconnect handling and video_is_registered(). >> >> I also removed the unused get_unmapped_area file op and I am now blocking >> any further (unlocked_)ioctl calls after the device node is unregistered. >> The only things an application can do legally after a disconnect is unmap() >> and close(). >> >> This patch series also contains a small em28xx fix that I found while testing >> the em28xx BKL removal patch. >> >> Regards, >> >> Hans >> >> The following changes since commit dace3857de7a16b83ae7d4e13c94de8e4b267d2a: >> Hans Verkuil (1): >> V4L/DVB: tvaudio: remove obsolete tda8425 initialization >> >> are available in the git repository at: >> >> ssh://linuxtv.org/git/hverkuil/v4l-dvb.git bkl >> >> Hans Verkuil (10): >> v4l2-dev: after a disconnect any ioctl call will be blocked. >> v4l2-dev: remove get_unmapped_area >> v4l2: add core serialization lock. >> videobuf: prepare to make locking optional in videobuf >> videobuf: add ext_lock argument to the queue init functions >> videobuf: add queue argument to videobuf_waiton() >> vivi: remove BKL. >> em28xx: remove BKL. >> em28xx: the default std was not passed on to the subdevs >> radio-mr800: remove BKL > > Did you even test these patches? The last one in the series clearly > breaks radio-mr800 and the commit message does not describe the > changes made. radio-mr800 has been BKL independent for quite some > time. Hans, you of all people should know that calling > video_unregister_device could result in the driver specific structure > being freed. The mutex must therefore be unlocked _before_ calling > video_unregister_device. Otherwise you're passing freed memory to > mutex_unlock in usb_amradio_disconnect. > To reiterate, the video_device struct is part of the amradio_device struct. When video_device_unregister is called, it can cause the release callback of the video_device struct to be called. In this case, that results in usb_amradio_video_device_release being called which frees the amradio_device struct. Therefore any use of the amradio_device struct after calling video_device_unregister is a potential use after free error. In this particular case you are trying to access the amradio_device.lock member which has potentially been freed already. Regards, David Ellingsworth -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [GIT PATCHES FOR 2.6.37] Move V4L2 locking into the core framework
Hans, On Sun, Sep 26, 2010 at 8:25 AM, Hans Verkuil wrote: > Hi Mauro, > > These are the locking patches. It's based on my previous test tree, but with > more testing with em28xx and radio-mr800 and some small tweaks relating to > disconnect handling and video_is_registered(). > > I also removed the unused get_unmapped_area file op and I am now blocking > any further (unlocked_)ioctl calls after the device node is unregistered. > The only things an application can do legally after a disconnect is unmap() > and close(). > > This patch series also contains a small em28xx fix that I found while testing > the em28xx BKL removal patch. > > Regards, > > Hans > > The following changes since commit dace3857de7a16b83ae7d4e13c94de8e4b267d2a: > Hans Verkuil (1): > V4L/DVB: tvaudio: remove obsolete tda8425 initialization > > are available in the git repository at: > > ssh://linuxtv.org/git/hverkuil/v4l-dvb.git bkl > > Hans Verkuil (10): > v4l2-dev: after a disconnect any ioctl call will be blocked. > v4l2-dev: remove get_unmapped_area > v4l2: add core serialization lock. > videobuf: prepare to make locking optional in videobuf > videobuf: add ext_lock argument to the queue init functions > videobuf: add queue argument to videobuf_waiton() > vivi: remove BKL. > em28xx: remove BKL. > em28xx: the default std was not passed on to the subdevs > radio-mr800: remove BKL Did you even test these patches? The last one in the series clearly breaks radio-mr800 and the commit message does not describe the changes made. radio-mr800 has been BKL independent for quite some time. Hans, you of all people should know that calling video_unregister_device could result in the driver specific structure being freed. The mutex must therefore be unlocked _before_ calling video_unregister_device. Otherwise you're passing freed memory to mutex_unlock in usb_amradio_disconnect. If each patch had been properly posted to the list, others might have caught issues like this earlier. Posting a link to a repository is no substitute for this process. Mauro, you should be ashamed for accepting a series that obviously has issues. Regards, David Ellingsworth -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH/RFC v2 0/8] dsbr100: driver cleanup and fixes
> I will also check your patches soon. I have this old hardware at home. > The sooner the better. These patches have been waiting for review since May. I'd rather not have to rebase them and resend them a third time. Regards, David Ellingsworth -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH -hg] Warn user that driver is backported and might not work as expected
> --- a/v4l/scripts/make_kconfig.pl Sun Jun 27 17:17:06 2010 -0300 > +++ b/v4l/scripts/make_kconfig.pl Fri Sep 17 11:49:02 2010 -0300 > @@ -671,4 +671,13 @@ > > EOF2 > } > +print << "EOF3"; > +WARNING: This is the V4L/DVB backport tree, with experimental drivers > + backported to run on legacy kernels from the development tree at: > + http://git.linuxtv.org/media-tree.git. > + It is generally safe to use it for testing a new driver or > + feature, but its usage on production environments is risky. > + Don't use it at production. You've being warned. The last line should read: "Don't use it in production. You've been warned." > +EOF3 > + sleep 5; > } > -- Regards, David Ellingsworth -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: ibmcam (xrilink_cit) and konica webcam driver porting to gspca update
On Mon, Sep 13, 2010 at 2:02 PM, David Ellingsworth wrote: > On Sun, Sep 5, 2010 at 4:58 AM, Hans de Goede wrote: >> Hi, >> >> On 08/31/2010 11:43 PM, David Ellingsworth wrote: >>> >>> Hans, >>> >>> I haven't had any success with this driver as of yet. My camera is >>> shown here: http://www.amazon.com/IBM-Net-Camera-Pro-camera/dp/B0009MH25U >>> The part number listed on the bottom is 22P5086. It's also labeled as >>> being an IBM Net Camera Pro. >> >> Ah ok, so you have the same one as I have, that model was never supported >> by the old ibmcam driver, so I take it you never had it working with the >> old ibmcam driver ? >> >>> When I plug the camera in, it is detected >>> >>> by the driver but it does not seem to function in this mode. Every >>> attempt to obtain video from it using qv4l2 results in a black or >>> green image. >>> >>> If I use the ibm_netcam_pro module option >> >> Given that is the same camera as I have using the ibm_netcam_pro module >> option is definitely the right thing to do. >> >> I noticed in your lsusb -v output that you're doing this from within vmware? > > Correct I was using vmware workstation's usb pass through to test the camera. > >> >> I think that is the cause of things not working. This camera will not >> even work when connected through a real hub, let alone through a >> virtual one. The only way this camera works for me is when it is >> connected to a usb port directly on the motherboard, running Linux >> directly on the hardware, can you please try that ? > > Unfortunately, I'm unable to test with real hardware at the moment. My > laptop, which has Linux installed on it is currently out of commission > until I can find time to repair it's power adapter. Once I get it > fixed, I'll try to retest and we'll go from there. > I just tested this driver with my camera using real hardware and it works wonderfully. Thanks for all the hard work. The only thing left to do with this camera is to reverse engineer the compressed video stream in order to improve the frame rate at higher resolutions. Regards, David Ellingsworth -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH/RFC v2 0/8] dsbr100: driver cleanup and fixes
Alexey, Can you review/test this patch series? Patches 2/8, 3/8, and 5/8 are bug fixes the rest are mainly cleanups. Patch 2/8 should fix a crash in the normal case if the device is disconnected while not in use. Regards, David Ellingsworth On Thu, May 27, 2010 at 12:39 PM, David Ellingsworth wrote: > This patch series addresses several issues in the dsbr100 driver. > This series is based on the v4l-dvb master git branch and has been > compile tested only. It should be tested before applying. > > This is the second version of this series. An additional patch has > been added to cleanup/clarify the return values from dsbr100_start > and dsbr100_stop. > > The following patches are included in this series: > [PATCH/RFC v2 1/8] dsbr100: implement proper locking > [PATCH/RFC v2 2/8] dsbr100: fix potential use after free > [PATCH/RFC v2 3/8] dsbr100: only change frequency upon success > [PATCH/RFC v2 4/8] dsbr100: remove disconnected indicator > [PATCH/RFC v2 5/8] dsbr100: cleanup return value of start/stop handlers > [PATCH/RFC v2 6/8] dsbr100: properly initialize the radio > [PATCH/RFC v2 7/8] dsbr100: cleanup usb probe routine > [PATCH/RFC v2 8/8] dsbr100: simplify access to radio device > > Regards, > > David Ellingsworth > > -- > To unsubscribe from this list: send the line "unsubscribe linux-media" in > the body of a message to majord...@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: ibmcam (xrilink_cit) and konica webcam driver porting to gspca update
On Sun, Sep 5, 2010 at 4:58 AM, Hans de Goede wrote: > Hi, > > On 08/31/2010 11:43 PM, David Ellingsworth wrote: >> >> Hans, >> >> I haven't had any success with this driver as of yet. My camera is >> shown here: http://www.amazon.com/IBM-Net-Camera-Pro-camera/dp/B0009MH25U >> The part number listed on the bottom is 22P5086. It's also labeled as >> being an IBM Net Camera Pro. > > Ah ok, so you have the same one as I have, that model was never supported > by the old ibmcam driver, so I take it you never had it working with the > old ibmcam driver ? > >> When I plug the camera in, it is detected >> >> by the driver but it does not seem to function in this mode. Every >> attempt to obtain video from it using qv4l2 results in a black or >> green image. >> >> If I use the ibm_netcam_pro module option > > Given that is the same camera as I have using the ibm_netcam_pro module > option is definitely the right thing to do. > > I noticed in your lsusb -v output that you're doing this from within vmware? Correct I was using vmware workstation's usb pass through to test the camera. > > I think that is the cause of things not working. This camera will not > even work when connected through a real hub, let alone through a > virtual one. The only way this camera works for me is when it is > connected to a usb port directly on the motherboard, running Linux > directly on the hardware, can you please try that ? Unfortunately, I'm unable to test with real hardware at the moment. My laptop, which has Linux installed on it is currently out of commission until I can find time to repair it's power adapter. Once I get it fixed, I'll try to retest and we'll go from there. Regards, David Ellingsworth -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH/RFC v2 0/8] dsbr100: driver cleanup and fixes
On Thu, May 27, 2010 at 12:39 PM, David Ellingsworth wrote: > This patch series addresses several issues in the dsbr100 driver. > This series is based on the v4l-dvb master git branch and has been > compile tested only. It should be tested before applying. > > This is the second version of this series. An additional patch has > been added to cleanup/clarify the return values from dsbr100_start > and dsbr100_stop. > > The following patches are included in this series: > [PATCH/RFC v2 1/8] dsbr100: implement proper locking > [PATCH/RFC v2 2/8] dsbr100: fix potential use after free > [PATCH/RFC v2 3/8] dsbr100: only change frequency upon success > [PATCH/RFC v2 4/8] dsbr100: remove disconnected indicator > [PATCH/RFC v2 5/8] dsbr100: cleanup return value of start/stop handlers > [PATCH/RFC v2 6/8] dsbr100: properly initialize the radio > [PATCH/RFC v2 7/8] dsbr100: cleanup usb probe routine > [PATCH/RFC v2 8/8] dsbr100: simplify access to radio device > Mauro, This series has not received any comments and the original author seems to have abandoned this driver. Please review these patches for approval. All changes are relatively straight forward. The second patch in this series is a bug fix for the normal case where the device is unplugged while closed. The current implementation will cause a NULL pointer dereference. The fact that no one has reported this bug is probably due to the lack of people using this driver. The rest of the changes mainly provide general cleanups and reduced overhead. Regards, David Ellingsworth -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: PROBLEM: 2.6.34-rc7 kernel panics "BUG: unable to handle kernel NULL pointer dereference at (null)" while channel scan runnin
On Sun, Jun 13, 2010 at 11:28 AM, Silamael wrote: > Hello! > > In the meanwhile i tried several different kernel versions: > - 2.6.26 (as included in Debian Lenny): crash > - 2.6.32-3 (as in Debian Squeeze): crash > - 2.6.32-5 (updated version in Debian Squeeze): crash > - 2.6.34: crash > > In every kernel version I've tested, the crashdump looks the same. Each > time there's an NULL pointer given to saa7146_buffer_next(). > > Would be nice if someone could give me some hints. I'm not sure whether > it's a broken driver or it's due to broken hardware or some other issues. > > Thanks a lot! > Matthias, While I don't doubt there's probably a bug in this driver, you haven't provided nearly enough information to correct it. Please resubmit with the full backtrace provided by the kernel at the time of the crash. Without this information, it's hard to gauge the exact cause of the error and thus no one will attempt to fix it. Regards, David Ellingsworth -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: question about v4l2_subdev
On Tue, Jun 1, 2010 at 10:04 AM, Sedji Gaouaou wrote: > Hi, > > Sorry to bother you again, but here is the situation: > I have 2 drivers: an ov2640 driver and my atmel driver. > Basically the ov2640 driver is the same as the ov7670 driver. > > So what I don't know is how to call the ov2640 functions(such as set format) > in my atmel driver. > > In the ov2640 I used the function: v4l2_i2c_subdev_init, and in the atmel > driver I used v4l2_device_register. > > But I don't know where I should use the v4l2_i2c_new_subdev function, and > how to link my atmel video struct to the i2c sensor. > > Is there any examples in linux? > > Regards, > Sedji > If I understand what you're saying, ov2640 and ovv7670 are both video drivers but they have shared functionality. If the shared functionality is in the form of controlling say an i2c device of some sorts then you should implement that functionality as a subdev. Otherwise, you should extract the shared functionality into its own module that can be utilized by both drivers (there are many examples of this within the kernel). Regards, David Ellingsworth -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH/RFC v2 8/8] dsbr100: simplify access to radio device
This patch replaces calls to video_drvdata with
references to struct file->private_data which is
set during usb_dsbr100_open. This value is passed
by video_ioctl2 via the *priv argument and is
accessible via file->private_data otherwise.
Signed-off-by: David Ellingsworth
---
drivers/media/radio/dsbr100.c | 15 ---
1 files changed, 8 insertions(+), 7 deletions(-)
diff --git a/drivers/media/radio/dsbr100.c b/drivers/media/radio/dsbr100.c
index 81e6aa5..a8c3d5a 100644
--- a/drivers/media/radio/dsbr100.c
+++ b/drivers/media/radio/dsbr100.c
@@ -366,7 +366,7 @@ static void usb_dsbr100_disconnect(struct usb_interface
*intf)
static int vidioc_querycap(struct file *file, void *priv,
struct v4l2_capability *v)
{
- struct dsbr100_device *radio = video_drvdata(file);
+ struct dsbr100_device *radio = priv;
strlcpy(v->driver, "dsbr100", sizeof(v->driver));
strlcpy(v->card, "D-Link R-100 USB FM Radio", sizeof(v->card));
@@ -379,7 +379,7 @@ static int vidioc_querycap(struct file *file, void *priv,
static int vidioc_g_tuner(struct file *file, void *priv,
struct v4l2_tuner *v)
{
- struct dsbr100_device *radio = video_drvdata(file);
+ struct dsbr100_device *radio = priv;
if (v->index > 0)
return -EINVAL;
@@ -411,7 +411,7 @@ static int vidioc_s_tuner(struct file *file, void *priv,
static int vidioc_s_frequency(struct file *file, void *priv,
struct v4l2_frequency *f)
{
- struct dsbr100_device *radio = video_drvdata(file);
+ struct dsbr100_device *radio = priv;
int retval = dsbr100_setfreq(radio, f->frequency);
if (retval < 0)
@@ -423,7 +423,7 @@ static int vidioc_s_frequency(struct file *file, void *priv,
static int vidioc_g_frequency(struct file *file, void *priv,
struct v4l2_frequency *f)
{
- struct dsbr100_device *radio = video_drvdata(file);
+ struct dsbr100_device *radio = priv;
f->type = V4L2_TUNER_RADIO;
f->frequency = radio->curfreq;
@@ -444,7 +444,7 @@ static int vidioc_queryctrl(struct file *file, void *priv,
static int vidioc_g_ctrl(struct file *file, void *priv,
struct v4l2_control *ctrl)
{
- struct dsbr100_device *radio = video_drvdata(file);
+ struct dsbr100_device *radio = priv;
switch (ctrl->id) {
case V4L2_CID_AUDIO_MUTE:
@@ -457,7 +457,7 @@ static int vidioc_g_ctrl(struct file *file, void *priv,
static int vidioc_s_ctrl(struct file *file, void *priv,
struct v4l2_control *ctrl)
{
- struct dsbr100_device *radio = video_drvdata(file);
+ struct dsbr100_device *radio = priv;
int retval;
switch (ctrl->id) {
@@ -518,7 +518,7 @@ static int vidioc_s_audio(struct file *file, void *priv,
static long usb_dsbr100_ioctl(struct file *file, unsigned int cmd,
unsigned long arg)
{
- struct dsbr100_device *radio = video_drvdata(file);
+ struct dsbr100_device *radio = file->private_data;
long retval = 0;
mutex_lock(&radio->lock);
@@ -556,6 +556,7 @@ static int usb_dsbr100_open(struct file *file)
radio->status |= INITIALIZED;
}
+ file->private_data = radio;
unlock:
mutex_unlock(&radio->lock);
return retval;
--
1.7.1
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH/RFC v2 7/8] dsbr100: cleanup usb probe routine
This patch simplifies the error paths within the
usb_dsbr100_probe routine. It also removes an unnecessary
local variable.
Signed-off-by: David Ellingsworth
---
drivers/media/radio/dsbr100.c | 39 ---
1 files changed, 20 insertions(+), 19 deletions(-)
diff --git a/drivers/media/radio/dsbr100.c b/drivers/media/radio/dsbr100.c
index 96e6128..81e6aa5 100644
--- a/drivers/media/radio/dsbr100.c
+++ b/drivers/media/radio/dsbr100.c
@@ -645,33 +645,28 @@ static int usb_dsbr100_probe(struct usb_interface *intf,
const struct usb_device_id *id)
{
struct dsbr100_device *radio;
- struct v4l2_device *v4l2_dev;
int retval;
radio = kzalloc(sizeof(struct dsbr100_device), GFP_KERNEL);
-
if (!radio)
return -ENOMEM;
radio->transfer_buffer = kmalloc(TB_LEN, GFP_KERNEL);
-
if (!(radio->transfer_buffer)) {
- kfree(radio);
- return -ENOMEM;
+ retval = -ENOMEM;
+ goto err_nobuf;
}
- v4l2_dev = &radio->v4l2_dev;
-
- retval = v4l2_device_register(&intf->dev, v4l2_dev);
+ retval = v4l2_device_register(&intf->dev, &radio->v4l2_dev);
if (retval < 0) {
- v4l2_err(v4l2_dev, "couldn't register v4l2_device\n");
- kfree(radio->transfer_buffer);
- kfree(radio);
- return retval;
+ v4l2_err(&radio->v4l2_dev, "couldn't register v4l2_device\n");
+ goto err_v4l2;
}
- strlcpy(radio->videodev.name, v4l2_dev->name,
sizeof(radio->videodev.name));
- radio->videodev.v4l2_dev = v4l2_dev;
+ strlcpy(radio->videodev.name, radio->v4l2_dev.name,
+ sizeof(radio->videodev.name));
+
+ radio->videodev.v4l2_dev = &radio->v4l2_dev;
radio->videodev.fops = &usb_dsbr100_fops;
radio->videodev.ioctl_ops = &usb_dsbr100_ioctl_ops;
radio->videodev.release = usb_dsbr100_video_device_release;
@@ -685,14 +680,20 @@ static int usb_dsbr100_probe(struct usb_interface *intf,
retval = video_register_device(&radio->videodev, VFL_TYPE_RADIO,
radio_nr);
if (retval < 0) {
- v4l2_err(v4l2_dev, "couldn't register video device\n");
- v4l2_device_unregister(v4l2_dev);
- kfree(radio->transfer_buffer);
- kfree(radio);
- return -EIO;
+ v4l2_err(&radio->v4l2_dev, "couldn't register video device\n");
+ goto err_vdev;
}
+
usb_set_intfdata(intf, radio);
return 0;
+
+err_vdev:
+ v4l2_device_unregister(&radio->v4l2_dev);
+err_v4l2:
+ kfree(radio->transfer_buffer);
+err_nobuf:
+ kfree(radio);
+ return retval;
}
static int __init dsbr100_init(void)
--
1.7.1
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH/RFC v2 6/8] dsbr100: properly initialize the radio
This patch adds a flag to indicate if the radio has been
initialized or not. If the flag has not been set upon open,
the radio is initialized to a known state.
It combines the STARTED/STOPPED indicators into a single
MUTED flag adn defines a couple of macros for determining
the status of the radio.
Signed-off-by: David Ellingsworth
---
drivers/media/radio/dsbr100.c | 53 +++-
1 files changed, 41 insertions(+), 12 deletions(-)
diff --git a/drivers/media/radio/dsbr100.c b/drivers/media/radio/dsbr100.c
index 0e009b7..96e6128 100644
--- a/drivers/media/radio/dsbr100.c
+++ b/drivers/media/radio/dsbr100.c
@@ -125,10 +125,13 @@ devices, that would be 76 and 91. */
#define FREQ_MAX 108.0
#define FREQ_MUL 16000
-/* defines for radio->status */
-#define STARTED0
-#define STOPPED1
+enum dsbr100_status {
+ INITIALIZED = 1 << 0,
+ MUTED = 1 << 1
+};
+#define radio_initialized(r) (r->status & INITIALIZED)
+#define radio_muted(r) (r->status & MUTED)
#define videodev_to_radio(d) container_of(d, struct dsbr100_device, videodev)
static int usb_dsbr100_probe(struct usb_interface *intf,
@@ -151,7 +154,7 @@ struct dsbr100_device {
struct mutex lock; /* buffer locking */
int curfreq;
int stereo;
- int status;
+ enum dsbr100_status status;
};
static struct usb_device_id usb_dsbr100_device_table [] = {
@@ -205,7 +208,7 @@ static int dsbr100_start(struct dsbr100_device *radio)
goto usb_control_msg_failed;
}
- radio->status = STARTED;
+ radio->status &= ~MUTED;
return 0;
usb_control_msg_failed:
@@ -246,7 +249,7 @@ static int dsbr100_stop(struct dsbr100_device *radio)
goto usb_control_msg_failed;
}
- radio->status = STOPPED;
+ radio->status |= MUTED;
return 0;
usb_control_msg_failed:
@@ -532,6 +535,32 @@ unlock:
return retval;
}
+static int usb_dsbr100_open(struct file *file)
+{
+ struct dsbr100_device *radio = video_drvdata(file);
+ int retval = 0;
+
+ mutex_lock(&radio->lock);
+
+ if (!radio->usbdev) {
+ retval = -EIO;
+ goto unlock;
+ }
+
+ if (unlikely(!radio_initialized(radio))) {
+ retval = dsbr100_stop(radio);
+
+ if (retval)
+ goto unlock;
+
+ radio->status |= INITIALIZED;
+ }
+
+unlock:
+ mutex_unlock(&radio->lock);
+ return retval;
+}
+
/* Suspend device - stop device. */
static int usb_dsbr100_suspend(struct usb_interface *intf, pm_message_t
message)
{
@@ -540,17 +569,17 @@ static int usb_dsbr100_suspend(struct usb_interface
*intf, pm_message_t message)
mutex_lock(&radio->lock);
- if (radio->status == STARTED) {
+ if (!radio_muted(radio)) {
retval = dsbr100_stop(radio);
if (retval)
dev_warn(&intf->dev, "dsbr100_stop failed\n");
- /* After dsbr100_stop() status set to STOPPED.
+ /* After dsbr100_stop() status set to MUTED.
* If we want driver to start radio on resume
-* we set status equal to STARTED.
+* we set status equal to not MUTED
* On resume we will check status and run radio if needed.
*/
- radio->status = STARTED;
+ radio->status &= ~MUTED;
}
mutex_unlock(&radio->lock);
@@ -567,7 +596,7 @@ static int usb_dsbr100_resume(struct usb_interface *intf)
mutex_lock(&radio->lock);
- if (radio->status == STARTED) {
+ if (radio_initialized(radio) && !radio_muted(radio)) {
retval = dsbr100_start(radio);
if (retval)
dev_warn(&intf->dev, "dsbr100_start failed\n");
@@ -593,6 +622,7 @@ static void usb_dsbr100_video_device_release(struct
video_device *videodev)
static const struct v4l2_file_operations usb_dsbr100_fops = {
.owner = THIS_MODULE,
.unlocked_ioctl = usb_dsbr100_ioctl,
+ .open = usb_dsbr100_open,
};
static const struct v4l2_ioctl_ops usb_dsbr100_ioctl_ops = {
@@ -650,7 +680,6 @@ static int usb_dsbr100_probe(struct usb_interface *intf,
radio->usbdev = interface_to_usbdev(intf);
radio->curfreq = FREQ_MIN * FREQ_MUL;
- radio->status = STOPPED;
video_set_drvdata(&radio->videodev, radio);
--
1.7.1
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH/RFC v2 2/8] dsbr100: fix potential use after free
Signed-off-by: David Ellingsworth --- drivers/media/radio/dsbr100.c |2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/drivers/media/radio/dsbr100.c b/drivers/media/radio/dsbr100.c index 673eda8..2f96e13 100644 --- a/drivers/media/radio/dsbr100.c +++ b/drivers/media/radio/dsbr100.c @@ -354,8 +354,8 @@ static void usb_dsbr100_disconnect(struct usb_interface *intf) radio->removed = 1; mutex_unlock(&radio->lock); - video_unregister_device(&radio->videodev); v4l2_device_disconnect(&radio->v4l2_dev); + video_unregister_device(&radio->videodev); } -- 1.7.1 -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH/RFC v2 1/8] dsbr100: implement proper locking
Signed-off-by: David Ellingsworth
---
drivers/media/radio/dsbr100.c | 77 +---
1 files changed, 33 insertions(+), 44 deletions(-)
diff --git a/drivers/media/radio/dsbr100.c b/drivers/media/radio/dsbr100.c
index ed9cd7a..673eda8 100644
--- a/drivers/media/radio/dsbr100.c
+++ b/drivers/media/radio/dsbr100.c
@@ -182,7 +182,7 @@ static int dsbr100_start(struct dsbr100_device *radio)
int retval;
int request;
- mutex_lock(&radio->lock);
+ BUG_ON(!mutex_is_locked(&radio->lock));
retval = usb_control_msg(radio->usbdev,
usb_rcvctrlpipe(radio->usbdev, 0),
@@ -207,11 +207,9 @@ static int dsbr100_start(struct dsbr100_device *radio)
}
radio->status = STARTED;
- mutex_unlock(&radio->lock);
return (radio->transfer_buffer)[0];
usb_control_msg_failed:
- mutex_unlock(&radio->lock);
dev_err(&radio->usbdev->dev,
"%s - usb_control_msg returned %i, request %i\n",
__func__, retval, request);
@@ -225,7 +223,7 @@ static int dsbr100_stop(struct dsbr100_device *radio)
int retval;
int request;
- mutex_lock(&radio->lock);
+ BUG_ON(!mutex_is_locked(&radio->lock));
retval = usb_control_msg(radio->usbdev,
usb_rcvctrlpipe(radio->usbdev, 0),
@@ -250,11 +248,9 @@ static int dsbr100_stop(struct dsbr100_device *radio)
}
radio->status = STOPPED;
- mutex_unlock(&radio->lock);
return (radio->transfer_buffer)[0];
usb_control_msg_failed:
- mutex_unlock(&radio->lock);
dev_err(&radio->usbdev->dev,
"%s - usb_control_msg returned %i, request %i\n",
__func__, retval, request);
@@ -269,7 +265,7 @@ static int dsbr100_setfreq(struct dsbr100_device *radio)
int request;
int freq = (radio->curfreq / 16 * 80) / 1000 + 856;
- mutex_lock(&radio->lock);
+ BUG_ON(!mutex_is_locked(&radio->lock));
retval = usb_control_msg(radio->usbdev,
usb_rcvctrlpipe(radio->usbdev, 0),
@@ -306,12 +302,10 @@ static int dsbr100_setfreq(struct dsbr100_device *radio)
}
radio->stereo = !((radio->transfer_buffer)[0] & 0x01);
- mutex_unlock(&radio->lock);
return (radio->transfer_buffer)[0];
usb_control_msg_failed:
radio->stereo = -1;
- mutex_unlock(&radio->lock);
dev_err(&radio->usbdev->dev,
"%s - usb_control_msg returned %i, request %i\n",
__func__, retval, request);
@@ -324,7 +318,7 @@ static void dsbr100_getstat(struct dsbr100_device *radio)
{
int retval;
- mutex_lock(&radio->lock);
+ BUG_ON(!mutex_is_locked(&radio->lock));
retval = usb_control_msg(radio->usbdev,
usb_rcvctrlpipe(radio->usbdev, 0),
@@ -340,8 +334,6 @@ static void dsbr100_getstat(struct dsbr100_device *radio)
} else {
radio->stereo = !(radio->transfer_buffer[0] & 0x01);
}
-
- mutex_unlock(&radio->lock);
}
/* USB subsystem interface begins here */
@@ -385,10 +377,6 @@ static int vidioc_g_tuner(struct file *file, void *priv,
{
struct dsbr100_device *radio = video_drvdata(file);
- /* safety check */
- if (radio->removed)
- return -EIO;
-
if (v->index > 0)
return -EINVAL;
@@ -410,12 +398,6 @@ static int vidioc_g_tuner(struct file *file, void *priv,
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;
@@ -428,17 +410,12 @@ static int vidioc_s_frequency(struct file *file, void
*priv,
struct dsbr100_device *radio = video_drvdata(file);
int retval;
- /* safety check */
- if (radio->removed)
- return -EIO;
-
- mutex_lock(&radio->lock);
radio->curfreq = f->frequency;
- mutex_unlock(&radio->lock);
retval = dsbr100_setfreq(radio);
if (retval < 0)
dev_warn(&radio->usbdev->dev, "Set frequency failed\n");
+
return 0;
}
@@ -447,10 +424,6 @@ static int vidioc_g_frequency(struct file *file, void
*priv,
{
struct dsbr100_device *radio = video_drvdata(file);
- /* safety check */
- if (radio->removed)
- return -EIO;
-
f->type = V4L2_TUNER_RADIO;
f->frequency = radio->curfreq;
[PATCH/RFC v2 4/8] dsbr100: remove disconnected indicator
Signed-off-by: David Ellingsworth
---
drivers/media/radio/dsbr100.c |6 ++
1 files changed, 2 insertions(+), 4 deletions(-)
diff --git a/drivers/media/radio/dsbr100.c b/drivers/media/radio/dsbr100.c
index b62fe40..c949ace 100644
--- a/drivers/media/radio/dsbr100.c
+++ b/drivers/media/radio/dsbr100.c
@@ -151,7 +151,6 @@ struct dsbr100_device {
struct mutex lock; /* buffer locking */
int curfreq;
int stereo;
- int removed;
int status;
};
@@ -353,7 +352,7 @@ static void usb_dsbr100_disconnect(struct usb_interface
*intf)
usb_set_intfdata (intf, NULL);
mutex_lock(&radio->lock);
- radio->removed = 1;
+ radio->usbdev = NULL;
mutex_unlock(&radio->lock);
v4l2_device_disconnect(&radio->v4l2_dev);
@@ -521,7 +520,7 @@ static long usb_dsbr100_ioctl(struct file *file, unsigned
int cmd,
mutex_lock(&radio->lock);
- if (radio->removed) {
+ if (!radio->usbdev) {
retval = -EIO;
goto unlock;
}
@@ -649,7 +648,6 @@ static int usb_dsbr100_probe(struct usb_interface *intf,
mutex_init(&radio->lock);
- radio->removed = 0;
radio->usbdev = interface_to_usbdev(intf);
radio->curfreq = FREQ_MIN * FREQ_MUL;
radio->status = STOPPED;
--
1.7.1
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH/RFC v2 5/8] dsbr100: cleanup return value of start/stop handlers
The value returned from the device in radio->transfer_buffer[0]
isn't clearly defined, but is used as a return code. This value
is an unsigned 8-bit integer that is implicitly converted to an
int. Since this value can never be less than 0, return 0 instead.
This simplifies the verification of calling dsbr100_start and
dsbr100_stop.
Signed-off-by: David Ellingsworth
---
drivers/media/radio/dsbr100.c | 12 ++--
1 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/drivers/media/radio/dsbr100.c b/drivers/media/radio/dsbr100.c
index c949ace..0e009b7 100644
--- a/drivers/media/radio/dsbr100.c
+++ b/drivers/media/radio/dsbr100.c
@@ -206,7 +206,7 @@ static int dsbr100_start(struct dsbr100_device *radio)
}
radio->status = STARTED;
- return (radio->transfer_buffer)[0];
+ return 0;
usb_control_msg_failed:
dev_err(&radio->usbdev->dev,
@@ -247,7 +247,7 @@ static int dsbr100_stop(struct dsbr100_device *radio)
}
radio->status = STOPPED;
- return (radio->transfer_buffer)[0];
+ return 0;
usb_control_msg_failed:
dev_err(&radio->usbdev->dev,
@@ -461,14 +461,14 @@ static int vidioc_s_ctrl(struct file *file, void *priv,
case V4L2_CID_AUDIO_MUTE:
if (ctrl->value) {
retval = dsbr100_stop(radio);
- if (retval < 0) {
+ if (retval) {
dev_warn(&radio->usbdev->dev,
"Radio did not respond properly\n");
return -EBUSY;
}
} else {
retval = dsbr100_start(radio);
- if (retval < 0) {
+ if (retval) {
dev_warn(&radio->usbdev->dev,
"Radio did not respond properly\n");
return -EBUSY;
@@ -542,7 +542,7 @@ static int usb_dsbr100_suspend(struct usb_interface *intf,
pm_message_t message)
if (radio->status == STARTED) {
retval = dsbr100_stop(radio);
- if (retval < 0)
+ if (retval)
dev_warn(&intf->dev, "dsbr100_stop failed\n");
/* After dsbr100_stop() status set to STOPPED.
@@ -569,7 +569,7 @@ static int usb_dsbr100_resume(struct usb_interface *intf)
if (radio->status == STARTED) {
retval = dsbr100_start(radio);
- if (retval < 0)
+ if (retval)
dev_warn(&intf->dev, "dsbr100_start failed\n");
}
--
1.7.1
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH/RFC v2 3/8] dsbr100: only change frequency upon success
Signed-off-by: David Ellingsworth
---
drivers/media/radio/dsbr100.c | 11 +--
1 files changed, 5 insertions(+), 6 deletions(-)
diff --git a/drivers/media/radio/dsbr100.c b/drivers/media/radio/dsbr100.c
index 2f96e13..b62fe40 100644
--- a/drivers/media/radio/dsbr100.c
+++ b/drivers/media/radio/dsbr100.c
@@ -259,11 +259,12 @@ usb_control_msg_failed:
}
/* set a frequency, freq is defined by v4l's TUNER_LOW, i.e. 1/16th kHz */
-static int dsbr100_setfreq(struct dsbr100_device *radio)
+static int dsbr100_setfreq(struct dsbr100_device *radio, int freq)
{
int retval;
int request;
- int freq = (radio->curfreq / 16 * 80) / 1000 + 856;
+
+ freq = (freq / 16 * 80) / 1000 + 856;
BUG_ON(!mutex_is_locked(&radio->lock));
@@ -302,6 +303,7 @@ static int dsbr100_setfreq(struct dsbr100_device *radio)
}
radio->stereo = !((radio->transfer_buffer)[0] & 0x01);
+ radio->curfreq = freq;
return (radio->transfer_buffer)[0];
usb_control_msg_failed:
@@ -408,11 +410,8 @@ static int vidioc_s_frequency(struct file *file, void
*priv,
struct v4l2_frequency *f)
{
struct dsbr100_device *radio = video_drvdata(file);
- int retval;
-
- radio->curfreq = f->frequency;
+ int retval = dsbr100_setfreq(radio, f->frequency);
- retval = dsbr100_setfreq(radio);
if (retval < 0)
dev_warn(&radio->usbdev->dev, "Set frequency failed\n");
--
1.7.1
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH/RFC v2 0/8] dsbr100: driver cleanup and fixes
This patch series addresses several issues in the dsbr100 driver. This series is based on the v4l-dvb master git branch and has been compile tested only. It should be tested before applying. This is the second version of this series. An additional patch has been added to cleanup/clarify the return values from dsbr100_start and dsbr100_stop. The following patches are included in this series: [PATCH/RFC v2 1/8] dsbr100: implement proper locking [PATCH/RFC v2 2/8] dsbr100: fix potential use after free [PATCH/RFC v2 3/8] dsbr100: only change frequency upon success [PATCH/RFC v2 4/8] dsbr100: remove disconnected indicator [PATCH/RFC v2 5/8] dsbr100: cleanup return value of start/stop handlers [PATCH/RFC v2 6/8] dsbr100: properly initialize the radio [PATCH/RFC v2 7/8] dsbr100: cleanup usb probe routine [PATCH/RFC v2 8/8] dsbr100: simplify access to radio device Regards, David Ellingsworth -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] drivers/media/dvb/dvb-usb/dib0700: CodingStyle fixes
See comments below:
On Mon, May 24, 2010 at 6:57 AM, Daniel Mack wrote:
> @@ -106,28 +106,29 @@ int dib0700_ctrl_rd(struct dvb_usb_device *d, u8 *tx,
> u8 txlen, u8 *rx, u8 rxlen
> int dib0700_set_gpio(struct dvb_usb_device *d, enum dib07x0_gpios gpio, u8
> gpio_dir, u8 gpio_val)
> {
> u8 buf[3] = { REQUEST_SET_GPIO, gpio, ((gpio_dir & 0x01) << 7) |
> ((gpio_val & 0x01) << 6) };
> - return dib0700_ctrl_wr(d,buf,3);
> + return dib0700_ctrl_wr(d, buf, sizeof(buf));
> }
>
> static int dib0700_set_usb_xfer_len(struct dvb_usb_device *d, u16
> nb_ts_packets)
> {
> - struct dib0700_state *st = d->priv;
> - u8 b[3];
> - int ret;
> -
> - if (st->fw_version >= 0x10201) {
> - b[0] = REQUEST_SET_USB_XFER_LEN;
> - b[1] = (nb_ts_packets >> 8)&0xff;
> - b[2] = nb_ts_packets & 0xff;
> -
> - deb_info("set the USB xfer len to %i Ts packet\n", nb_ts_packets);
> -
> - ret = dib0700_ctrl_wr(d, b, 3);
> - } else {
> - deb_info("this firmware does not allow to change the USB xfer len\n");
> - ret = -EIO;
> - }
> - return ret;
> + struct dib0700_state *st = d->priv;
> + u8 b[3];
> + int ret;
> +
> + if (st->fw_version >= 0x10201) {
> + b[0] = REQUEST_SET_USB_XFER_LEN;
> + b[1] = (nb_ts_packets >> 8) & 0xff;
> + b[2] = nb_ts_packets & 0xff;
> +
> + deb_info("set the USB xfer len to %i Ts packet\n",
> nb_ts_packets);
> +
> + ret = dib0700_ctrl_wr(d, b, 3);
sizeof(b) would be better than the hard-coded value of 3 above.
> + } else {
> + deb_info("this firmware does not allow to change the USB xfer
> len\n");
> + ret = -EIO;
> + }
> +
> + return ret;
> }
>
> /*
Everything else looks good.
Regards,
David Ellingsworth
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 7/7] v4l: videobuf: Rename vmalloc fields to vaddr
On Tue, May 11, 2010 at 9:36 AM, Laurent Pinchart
wrote:
> The videobuf_dmabuf and videobuf_vmalloc_memory fields have a vmalloc
> field to store the kernel virtual address of vmalloc'ed buffers. Rename
> the field to vaddr.
>
> Signed-off-by: Laurent Pinchart
> ---
> drivers/media/video/cx88/cx88-alsa.c | 2 +-
> drivers/media/video/saa7134/saa7134-alsa.c | 2 +-
> drivers/media/video/videobuf-dma-sg.c | 18
> drivers/media/video/videobuf-vmalloc.c | 30
> ++--
> drivers/staging/cx25821/cx25821-alsa.c | 2 +-
> include/media/videobuf-dma-sg.h | 2 +-
> include/media/videobuf-vmalloc.h | 2 +-
> 7 files changed, 29 insertions(+), 29 deletions(-)
>
> diff --git a/drivers/media/video/cx88/cx88-alsa.c
> b/drivers/media/video/cx88/cx88-alsa.c
> index ebeb9a6..771406f 100644
> --- a/drivers/media/video/cx88/cx88-alsa.c
> +++ b/drivers/media/video/cx88/cx88-alsa.c
> @@ -425,7 +425,7 @@ static int snd_cx88_hw_params(struct snd_pcm_substream *
> substream,
> chip->buf = buf;
> chip->dma_risc = dma;
>
> - substream->runtime->dma_area = chip->dma_risc->vmalloc;
> + substream->runtime->dma_area = chip->dma_risc->vaddr;
> substream->runtime->dma_bytes = chip->dma_size;
> substream->runtime->dma_addr = 0;
> return 0;
> diff --git a/drivers/media/video/saa7134/saa7134-alsa.c
> b/drivers/media/video/saa7134/saa7134-alsa.c
> index 5bca2ab..68b7e8d 100644
> --- a/drivers/media/video/saa7134/saa7134-alsa.c
> +++ b/drivers/media/video/saa7134/saa7134-alsa.c
> @@ -669,7 +669,7 @@ static int snd_card_saa7134_hw_params(struct
> snd_pcm_substream * substream,
> byte, but it doesn't work. So I allocate the DMA using the
> V4L functions, and force ALSA to use that as the DMA area */
>
> - substream->runtime->dma_area = dev->dmasound.dma.vmalloc;
> + substream->runtime->dma_area = dev->dmasound.dma.vaddr;
> substream->runtime->dma_bytes = dev->dmasound.bufsize;
> substream->runtime->dma_addr = 0;
>
> diff --git a/drivers/media/video/videobuf-dma-sg.c
> b/drivers/media/video/videobuf-dma-sg.c
> index 2d64040..06f9a9c 100644
> --- a/drivers/media/video/videobuf-dma-sg.c
> +++ b/drivers/media/video/videobuf-dma-sg.c
> @@ -211,17 +211,17 @@ int videobuf_dma_init_kernel(struct videobuf_dmabuf
> *dma, int direction,
> dprintk(1, "init kernel [%d pages]\n", nr_pages);
>
> dma->direction = direction;
> - dma->vmalloc = vmalloc_32(nr_pages << PAGE_SHIFT);
> - if (NULL == dma->vmalloc) {
> + dma->vaddr = vmalloc_32(nr_pages << PAGE_SHIFT);
> + if (NULL == dma->vaddr) {
> dprintk(1, "vmalloc_32(%d pages) failed\n", nr_pages);
> return -ENOMEM;
> }
>
> dprintk(1, "vmalloc is at addr 0x%08lx, size=%d\n",
> - (unsigned long)dma->vmalloc,
> + (unsigned long)dma->vaddr,
> nr_pages << PAGE_SHIFT);
>
> - memset(dma->vmalloc, 0, nr_pages << PAGE_SHIFT);
> + memset(dma->vaddr, 0, nr_pages << PAGE_SHIFT);
> dma->nr_pages = nr_pages;
>
> return 0;
> @@ -254,8 +254,8 @@ int videobuf_dma_map(struct device *dev, struct
> videobuf_dmabuf *dma)
> dma->sglist = videobuf_pages_to_sg(dma->pages, dma->nr_pages,
> dma->offset);
> }
> - if (dma->vmalloc) {
> - dma->sglist = videobuf_vmalloc_to_sg(dma->vmalloc,
> + if (dma->vaddr) {
> + dma->sglist = videobuf_vmalloc_to_sg(dma->vaddr,
> dma->nr_pages);
> }
> if (dma->bus_addr) {
> @@ -319,8 +319,8 @@ int videobuf_dma_free(struct videobuf_dmabuf *dma)
> dma->pages = NULL;
> }
>
> - vfree(dma->vmalloc);
> - dma->vmalloc = NULL;
> + vfree(dma->vaddr);
> + dma->vaddr = NULL;
>
> if (dma->bus_addr)
> dma->bus_addr = 0;
> @@ -444,7 +444,7 @@ static void *__videobuf_to_vaddr(struct videobuf_buffer
> *buf)
>
> MAGIC_CHECK(mem->magic, MAGIC_SG_MEM);
>
> - return mem->dma.vmalloc;
> + return mem->dma.vaddr;
> }
>
> static int __videobuf_iolock(struct videobuf_queue *q,
> diff --git a/drivers/media/video/videobuf-vmalloc.c
> b/drivers/media/video/videobuf-vmalloc.c
> index f0d7cb8..ea08b5d 100644
> --- a/drivers/media/video/videobuf-vmalloc.c
> +++ b/drivers/media/video/videobuf-vmalloc.c
> @@ -102,10 +102,10 @@ static void videobuf_vm_close(struct vm_area_struct
> *vma)
> called with IRQ's disabled
> */
> dprintk(1, "%s: buf[%d] freeing (%p)\n",
> - __func__, i, mem->vmalloc);
> + __func__, i, m
[PATCH/RFC 7/7] dsbr100: simplify access to radio device
This patch replaces calls to video_drvdata with
references to struct file->private_data which is
set during usb_dsbr100_open. This value is passed
by video_ioctl2 via the *priv argument and is
accessible via file->private_data otherwise.
Signed-off-by: David Ellingsworth
---
drivers/media/radio/dsbr100.c | 15 ---
1 files changed, 8 insertions(+), 7 deletions(-)
diff --git a/drivers/media/radio/dsbr100.c b/drivers/media/radio/dsbr100.c
index cd4eed7..64ba0c8 100644
--- a/drivers/media/radio/dsbr100.c
+++ b/drivers/media/radio/dsbr100.c
@@ -366,7 +366,7 @@ static void usb_dsbr100_disconnect(struct usb_interface
*intf)
static int vidioc_querycap(struct file *file, void *priv,
struct v4l2_capability *v)
{
- struct dsbr100_device *radio = video_drvdata(file);
+ struct dsbr100_device *radio = priv;
strlcpy(v->driver, "dsbr100", sizeof(v->driver));
strlcpy(v->card, "D-Link R-100 USB FM Radio", sizeof(v->card));
@@ -379,7 +379,7 @@ static int vidioc_querycap(struct file *file, void *priv,
static int vidioc_g_tuner(struct file *file, void *priv,
struct v4l2_tuner *v)
{
- struct dsbr100_device *radio = video_drvdata(file);
+ struct dsbr100_device *radio = priv;
if (v->index > 0)
return -EINVAL;
@@ -411,7 +411,7 @@ static int vidioc_s_tuner(struct file *file, void *priv,
static int vidioc_s_frequency(struct file *file, void *priv,
struct v4l2_frequency *f)
{
- struct dsbr100_device *radio = video_drvdata(file);
+ struct dsbr100_device *radio = priv;
int retval = dsbr100_setfreq(radio, f->frequency);
if (retval < 0)
@@ -423,7 +423,7 @@ static int vidioc_s_frequency(struct file *file, void *priv,
static int vidioc_g_frequency(struct file *file, void *priv,
struct v4l2_frequency *f)
{
- struct dsbr100_device *radio = video_drvdata(file);
+ struct dsbr100_device *radio = priv;
f->type = V4L2_TUNER_RADIO;
f->frequency = radio->curfreq;
@@ -444,7 +444,7 @@ static int vidioc_queryctrl(struct file *file, void *priv,
static int vidioc_g_ctrl(struct file *file, void *priv,
struct v4l2_control *ctrl)
{
- struct dsbr100_device *radio = video_drvdata(file);
+ struct dsbr100_device *radio = priv;
switch (ctrl->id) {
case V4L2_CID_AUDIO_MUTE:
@@ -457,7 +457,7 @@ static int vidioc_g_ctrl(struct file *file, void *priv,
static int vidioc_s_ctrl(struct file *file, void *priv,
struct v4l2_control *ctrl)
{
- struct dsbr100_device *radio = video_drvdata(file);
+ struct dsbr100_device *radio = priv;
int retval;
switch (ctrl->id) {
@@ -518,7 +518,7 @@ static int vidioc_s_audio(struct file *file, void *priv,
static long usb_dsbr100_ioctl(struct file *file, unsigned int cmd,
unsigned long arg)
{
- struct dsbr100_device *radio = video_drvdata(file);
+ struct dsbr100_device *radio = file->private_data;
long retval = 0;
mutex_lock(&radio->lock);
@@ -557,6 +557,7 @@ static int usb_dsbr100_open(struct file *file)
radio->status |= INITIALIZED;
}
+ file->private_data = radio;
unlock:
mutex_unlock(&radio->lock);
return retval;
--
1.7.1
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH/RFC 0/7] dsbr100: driver cleanup
This patch series addresses several issues in the dsbr100 driver. This series is based on the v4l-dvb master git branch and has been compile tested only. It should be tested before applying. The following patches are included in this series: [PATCH/RFC 1/7] dsbr100: implement proper locking [PATCH/RFC 2/7] dsbr100: fix potential use after free [PATCH/RFC 3/7] dsbr100: only change frequency upon success [PATCH/RFC 4/7] dsbr100: remove disconnected indicator [PATCH/RFC 5/7] dsbr100: properly initialize the radio [PATCH/RFC 6/7] dsbr100: cleanup usb probe routine [PATCH/RFC 7/7] dsbr100: simplify access to radio device Regards, David Ellingsworth -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH/RFC 4/7] dsbr100: remove disconnected indicator
Signed-off-by: David Ellingsworth
---
drivers/media/radio/dsbr100.c |6 ++
1 files changed, 2 insertions(+), 4 deletions(-)
diff --git a/drivers/media/radio/dsbr100.c b/drivers/media/radio/dsbr100.c
index b62fe40..c949ace 100644
--- a/drivers/media/radio/dsbr100.c
+++ b/drivers/media/radio/dsbr100.c
@@ -151,7 +151,6 @@ struct dsbr100_device {
struct mutex lock; /* buffer locking */
int curfreq;
int stereo;
- int removed;
int status;
};
@@ -353,7 +352,7 @@ static void usb_dsbr100_disconnect(struct usb_interface
*intf)
usb_set_intfdata (intf, NULL);
mutex_lock(&radio->lock);
- radio->removed = 1;
+ radio->usbdev = NULL;
mutex_unlock(&radio->lock);
v4l2_device_disconnect(&radio->v4l2_dev);
@@ -521,7 +520,7 @@ static long usb_dsbr100_ioctl(struct file *file, unsigned
int cmd,
mutex_lock(&radio->lock);
- if (radio->removed) {
+ if (!radio->usbdev) {
retval = -EIO;
goto unlock;
}
@@ -649,7 +648,6 @@ static int usb_dsbr100_probe(struct usb_interface *intf,
mutex_init(&radio->lock);
- radio->removed = 0;
radio->usbdev = interface_to_usbdev(intf);
radio->curfreq = FREQ_MIN * FREQ_MUL;
radio->status = STOPPED;
--
1.7.1
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH/RFC 6/7] dsbr100: cleanup usb probe routine
This patch simplifies the error paths within the
usb_dsbr100_probe routine. It also removes an unnecessary
local variable.
Signed-off-by: David Ellingsworth
---
drivers/media/radio/dsbr100.c | 39 ---
1 files changed, 20 insertions(+), 19 deletions(-)
diff --git a/drivers/media/radio/dsbr100.c b/drivers/media/radio/dsbr100.c
index e2fed0b..cd4eed7 100644
--- a/drivers/media/radio/dsbr100.c
+++ b/drivers/media/radio/dsbr100.c
@@ -646,33 +646,28 @@ static int usb_dsbr100_probe(struct usb_interface *intf,
const struct usb_device_id *id)
{
struct dsbr100_device *radio;
- struct v4l2_device *v4l2_dev;
int retval;
radio = kzalloc(sizeof(struct dsbr100_device), GFP_KERNEL);
-
if (!radio)
return -ENOMEM;
radio->transfer_buffer = kmalloc(TB_LEN, GFP_KERNEL);
-
if (!(radio->transfer_buffer)) {
- kfree(radio);
- return -ENOMEM;
+ retval = -ENOMEM;
+ goto err_nobuf;
}
- v4l2_dev = &radio->v4l2_dev;
-
- retval = v4l2_device_register(&intf->dev, v4l2_dev);
+ retval = v4l2_device_register(&intf->dev, &radio->v4l2_dev);
if (retval < 0) {
- v4l2_err(v4l2_dev, "couldn't register v4l2_device\n");
- kfree(radio->transfer_buffer);
- kfree(radio);
- return retval;
+ v4l2_err(&radio->v4l2_dev, "couldn't register v4l2_device\n");
+ goto err_v4l2;
}
- strlcpy(radio->videodev.name, v4l2_dev->name,
sizeof(radio->videodev.name));
- radio->videodev.v4l2_dev = v4l2_dev;
+ strlcpy(radio->videodev.name, radio->v4l2_dev.name,
+ sizeof(radio->videodev.name));
+
+ radio->videodev.v4l2_dev = &radio->v4l2_dev;
radio->videodev.fops = &usb_dsbr100_fops;
radio->videodev.ioctl_ops = &usb_dsbr100_ioctl_ops;
radio->videodev.release = usb_dsbr100_video_device_release;
@@ -686,14 +681,20 @@ static int usb_dsbr100_probe(struct usb_interface *intf,
retval = video_register_device(&radio->videodev, VFL_TYPE_RADIO,
radio_nr);
if (retval < 0) {
- v4l2_err(v4l2_dev, "couldn't register video device\n");
- v4l2_device_unregister(v4l2_dev);
- kfree(radio->transfer_buffer);
- kfree(radio);
- return -EIO;
+ v4l2_err(&radio->v4l2_dev, "couldn't register video device\n");
+ goto err_vdev;
}
+
usb_set_intfdata(intf, radio);
return 0;
+
+err_vdev:
+ v4l2_device_unregister(&radio->v4l2_dev);
+err_v4l2:
+ kfree(radio->transfer_buffer);
+err_nobuf:
+ kfree(radio);
+ return retval;
}
static int __init dsbr100_init(void)
--
1.7.1
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH/RFC 3/7] dsbr100: only change frequency upon success
Signed-off-by: David Ellingsworth
---
drivers/media/radio/dsbr100.c | 11 +--
1 files changed, 5 insertions(+), 6 deletions(-)
diff --git a/drivers/media/radio/dsbr100.c b/drivers/media/radio/dsbr100.c
index 2f96e13..b62fe40 100644
--- a/drivers/media/radio/dsbr100.c
+++ b/drivers/media/radio/dsbr100.c
@@ -259,11 +259,12 @@ usb_control_msg_failed:
}
/* set a frequency, freq is defined by v4l's TUNER_LOW, i.e. 1/16th kHz */
-static int dsbr100_setfreq(struct dsbr100_device *radio)
+static int dsbr100_setfreq(struct dsbr100_device *radio, int freq)
{
int retval;
int request;
- int freq = (radio->curfreq / 16 * 80) / 1000 + 856;
+
+ freq = (freq / 16 * 80) / 1000 + 856;
BUG_ON(!mutex_is_locked(&radio->lock));
@@ -302,6 +303,7 @@ static int dsbr100_setfreq(struct dsbr100_device *radio)
}
radio->stereo = !((radio->transfer_buffer)[0] & 0x01);
+ radio->curfreq = freq;
return (radio->transfer_buffer)[0];
usb_control_msg_failed:
@@ -408,11 +410,8 @@ static int vidioc_s_frequency(struct file *file, void
*priv,
struct v4l2_frequency *f)
{
struct dsbr100_device *radio = video_drvdata(file);
- int retval;
-
- radio->curfreq = f->frequency;
+ int retval = dsbr100_setfreq(radio, f->frequency);
- retval = dsbr100_setfreq(radio);
if (retval < 0)
dev_warn(&radio->usbdev->dev, "Set frequency failed\n");
--
1.7.1
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH/RFC 1/7] dsbr100: implement proper locking
Signed-off-by: David Ellingsworth
---
drivers/media/radio/dsbr100.c | 77 +---
1 files changed, 33 insertions(+), 44 deletions(-)
diff --git a/drivers/media/radio/dsbr100.c b/drivers/media/radio/dsbr100.c
index ed9cd7a..673eda8 100644
--- a/drivers/media/radio/dsbr100.c
+++ b/drivers/media/radio/dsbr100.c
@@ -182,7 +182,7 @@ static int dsbr100_start(struct dsbr100_device *radio)
int retval;
int request;
- mutex_lock(&radio->lock);
+ BUG_ON(!mutex_is_locked(&radio->lock));
retval = usb_control_msg(radio->usbdev,
usb_rcvctrlpipe(radio->usbdev, 0),
@@ -207,11 +207,9 @@ static int dsbr100_start(struct dsbr100_device *radio)
}
radio->status = STARTED;
- mutex_unlock(&radio->lock);
return (radio->transfer_buffer)[0];
usb_control_msg_failed:
- mutex_unlock(&radio->lock);
dev_err(&radio->usbdev->dev,
"%s - usb_control_msg returned %i, request %i\n",
__func__, retval, request);
@@ -225,7 +223,7 @@ static int dsbr100_stop(struct dsbr100_device *radio)
int retval;
int request;
- mutex_lock(&radio->lock);
+ BUG_ON(!mutex_is_locked(&radio->lock));
retval = usb_control_msg(radio->usbdev,
usb_rcvctrlpipe(radio->usbdev, 0),
@@ -250,11 +248,9 @@ static int dsbr100_stop(struct dsbr100_device *radio)
}
radio->status = STOPPED;
- mutex_unlock(&radio->lock);
return (radio->transfer_buffer)[0];
usb_control_msg_failed:
- mutex_unlock(&radio->lock);
dev_err(&radio->usbdev->dev,
"%s - usb_control_msg returned %i, request %i\n",
__func__, retval, request);
@@ -269,7 +265,7 @@ static int dsbr100_setfreq(struct dsbr100_device *radio)
int request;
int freq = (radio->curfreq / 16 * 80) / 1000 + 856;
- mutex_lock(&radio->lock);
+ BUG_ON(!mutex_is_locked(&radio->lock));
retval = usb_control_msg(radio->usbdev,
usb_rcvctrlpipe(radio->usbdev, 0),
@@ -306,12 +302,10 @@ static int dsbr100_setfreq(struct dsbr100_device *radio)
}
radio->stereo = !((radio->transfer_buffer)[0] & 0x01);
- mutex_unlock(&radio->lock);
return (radio->transfer_buffer)[0];
usb_control_msg_failed:
radio->stereo = -1;
- mutex_unlock(&radio->lock);
dev_err(&radio->usbdev->dev,
"%s - usb_control_msg returned %i, request %i\n",
__func__, retval, request);
@@ -324,7 +318,7 @@ static void dsbr100_getstat(struct dsbr100_device *radio)
{
int retval;
- mutex_lock(&radio->lock);
+ BUG_ON(!mutex_is_locked(&radio->lock));
retval = usb_control_msg(radio->usbdev,
usb_rcvctrlpipe(radio->usbdev, 0),
@@ -340,8 +334,6 @@ static void dsbr100_getstat(struct dsbr100_device *radio)
} else {
radio->stereo = !(radio->transfer_buffer[0] & 0x01);
}
-
- mutex_unlock(&radio->lock);
}
/* USB subsystem interface begins here */
@@ -385,10 +377,6 @@ static int vidioc_g_tuner(struct file *file, void *priv,
{
struct dsbr100_device *radio = video_drvdata(file);
- /* safety check */
- if (radio->removed)
- return -EIO;
-
if (v->index > 0)
return -EINVAL;
@@ -410,12 +398,6 @@ static int vidioc_g_tuner(struct file *file, void *priv,
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;
@@ -428,17 +410,12 @@ static int vidioc_s_frequency(struct file *file, void
*priv,
struct dsbr100_device *radio = video_drvdata(file);
int retval;
- /* safety check */
- if (radio->removed)
- return -EIO;
-
- mutex_lock(&radio->lock);
radio->curfreq = f->frequency;
- mutex_unlock(&radio->lock);
retval = dsbr100_setfreq(radio);
if (retval < 0)
dev_warn(&radio->usbdev->dev, "Set frequency failed\n");
+
return 0;
}
@@ -447,10 +424,6 @@ static int vidioc_g_frequency(struct file *file, void
*priv,
{
struct dsbr100_device *radio = video_drvdata(file);
- /* safety check */
- if (radio->removed)
- return -EIO;
-
f->type = V4L2_TUNER_RADIO;
f->frequency = radio->curfreq;
[PATCH/RFC 5/7] dsbr100: properly initialize the radio
This patch adds a flag to indicate if the radio has been
initialized or not. If the flag has not been set upon open,
the radio initialized to a known state.
It combines the STARTED/STOPPED indicators into a single
MUTED flag and defines a couple of macros for determining
the status of the radio.
Signed-off-by: David Ellingsworth
---
drivers/media/radio/dsbr100.c | 54 +++-
1 files changed, 42 insertions(+), 12 deletions(-)
diff --git a/drivers/media/radio/dsbr100.c b/drivers/media/radio/dsbr100.c
index c949ace..e2fed0b 100644
--- a/drivers/media/radio/dsbr100.c
+++ b/drivers/media/radio/dsbr100.c
@@ -125,10 +125,13 @@ devices, that would be 76 and 91. */
#define FREQ_MAX 108.0
#define FREQ_MUL 16000
-/* defines for radio->status */
-#define STARTED0
-#define STOPPED1
+enum dsbr100_status {
+ INITIALIZED = 1 << 0,
+ MUTED = 1 << 1
+};
+#define radio_initialized(r) (r->status & INITIALIZED)
+#define radio_muted(r) (r->status & MUTED)
#define videodev_to_radio(d) container_of(d, struct dsbr100_device, videodev)
static int usb_dsbr100_probe(struct usb_interface *intf,
@@ -151,7 +154,7 @@ struct dsbr100_device {
struct mutex lock; /* buffer locking */
int curfreq;
int stereo;
- int status;
+ enum dsbr100_status status;
};
static struct usb_device_id usb_dsbr100_device_table [] = {
@@ -205,7 +208,7 @@ static int dsbr100_start(struct dsbr100_device *radio)
goto usb_control_msg_failed;
}
- radio->status = STARTED;
+ radio->status &= ~MUTED;
return (radio->transfer_buffer)[0];
usb_control_msg_failed:
@@ -246,7 +249,7 @@ static int dsbr100_stop(struct dsbr100_device *radio)
goto usb_control_msg_failed;
}
- radio->status = STOPPED;
+ radio->status |= MUTED;
return (radio->transfer_buffer)[0];
usb_control_msg_failed:
@@ -532,6 +535,33 @@ unlock:
return retval;
}
+static int usb_dsbr100_open(struct file *file)
+{
+ struct dsbr100_device *radio = video_drvdata(file);
+ int retval = 0;
+
+ mutex_lock(&radio->lock);
+
+ if (!radio->usbdev) {
+ retval = -EIO;
+ goto unlock;
+ }
+
+ if (unlikely(!radio_initialized(radio))) {
+ retval = dsbr100_stop(radio);
+
+ if (retval < 0)
+ goto unlock;
+
+ retval = 0;
+ radio->status |= INITIALIZED;
+ }
+
+unlock:
+ mutex_unlock(&radio->lock);
+ return retval;
+}
+
/* Suspend device - stop device. */
static int usb_dsbr100_suspend(struct usb_interface *intf, pm_message_t
message)
{
@@ -540,17 +570,17 @@ static int usb_dsbr100_suspend(struct usb_interface
*intf, pm_message_t message)
mutex_lock(&radio->lock);
- if (radio->status == STARTED) {
+ if (!radio_muted(radio)) {
retval = dsbr100_stop(radio);
if (retval < 0)
dev_warn(&intf->dev, "dsbr100_stop failed\n");
- /* After dsbr100_stop() status set to STOPPED.
+ /* After dsbr100_stop() status set to MUTED.
* If we want driver to start radio on resume
-* we set status equal to STARTED.
+* we set status equal to not MUTED
* On resume we will check status and run radio if needed.
*/
- radio->status = STARTED;
+ radio->status &= ~MUTED;
}
mutex_unlock(&radio->lock);
@@ -567,7 +597,7 @@ static int usb_dsbr100_resume(struct usb_interface *intf)
mutex_lock(&radio->lock);
- if (radio->status == STARTED) {
+ if (radio_initialized(radio) && !radio_muted(radio)) {
retval = dsbr100_start(radio);
if (retval < 0)
dev_warn(&intf->dev, "dsbr100_start failed\n");
@@ -593,6 +623,7 @@ static void usb_dsbr100_video_device_release(struct
video_device *videodev)
static const struct v4l2_file_operations usb_dsbr100_fops = {
.owner = THIS_MODULE,
.unlocked_ioctl = usb_dsbr100_ioctl,
+ .open = usb_dsbr100_open,
};
static const struct v4l2_ioctl_ops usb_dsbr100_ioctl_ops = {
@@ -650,7 +681,6 @@ static int usb_dsbr100_probe(struct usb_interface *intf,
radio->usbdev = interface_to_usbdev(intf);
radio->curfreq = FREQ_MIN * FREQ_MUL;
- radio->status = STOPPED;
video_set_drvdata(&radio->videodev, radio);
--
1.7.1
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH/RFC 2/7] dsbr100: fix potential use after free
Signed-off-by: David Ellingsworth --- drivers/media/radio/dsbr100.c |2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/drivers/media/radio/dsbr100.c b/drivers/media/radio/dsbr100.c index 673eda8..2f96e13 100644 --- a/drivers/media/radio/dsbr100.c +++ b/drivers/media/radio/dsbr100.c @@ -354,8 +354,8 @@ static void usb_dsbr100_disconnect(struct usb_interface *intf) radio->removed = 1; mutex_unlock(&radio->lock); - video_unregister_device(&radio->videodev); v4l2_device_disconnect(&radio->v4l2_dev); + video_unregister_device(&radio->videodev); } -- 1.7.1 -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 06/15] [RFC] msp3400: convert to the new control framework
? "yes" : "no",
> + state->volume->val);
> +
> + msp_write_dsp(client, 0x, val);
> + msp_write_dsp(client, 0x0007, reallymuted ? 0x1 : (val |
> 0x1));
> + if (state->has_scart2_out_volume)
> + msp_write_dsp(client, 0x0040, reallymuted ? 0x1 :
> (val | 0x1));
> + if (state->has_headphones)
> + msp_write_dsp(client, 0x0006, val);
> break;
> -
> - case V4L2_CID_AUDIO_TREBLE:
> - if (!state->has_sound_processing)
> - return -EINVAL;
> - ctrl->value = state->treble;
> - break;
> -
> - case V4L2_CID_AUDIO_LOUDNESS:
> - if (!state->has_sound_processing)
> - return -EINVAL;
> - ctrl->value = state->loudness;
> - break;
> -
> - default:
> - return -EINVAL;
> }
> - return 0;
> -}
> -
> -static int msp_s_ctrl(struct v4l2_subdev *sd, struct v4l2_control *ctrl)
> -{
> - struct msp_state *state = to_state(sd);
> - struct i2c_client *client = v4l2_get_subdevdata(sd);
> -
> - switch (ctrl->id) {
> - case V4L2_CID_AUDIO_VOLUME:
> - state->volume = ctrl->value;
> - if (state->volume == 0)
> - state->balance = 32768;
> - break;
> -
> - case V4L2_CID_AUDIO_MUTE:
> - if (ctrl->value < 0 || ctrl->value >= 2)
> - return -ERANGE;
> - state->muted = ctrl->value;
> - break;
>
> case V4L2_CID_AUDIO_BASS:
> - if (!state->has_sound_processing)
> - return -EINVAL;
> - state->bass = ctrl->value;
> + val = ((val - 32768) * 0x60 / 65535) << 8;
> + msp_write_dsp(client, 0x0002, val);
> + if (state->has_headphones)
> + msp_write_dsp(client, 0x0031, val);
> break;
>
> case V4L2_CID_AUDIO_TREBLE:
> - if (!state->has_sound_processing)
> - return -EINVAL;
> - state->treble = ctrl->value;
> + val = ((val - 32768) * 0x60 / 65535) << 8;
> + msp_write_dsp(client, 0x0003, val);
> + if (state->has_headphones)
> + msp_write_dsp(client, 0x0032, val);
> break;
>
> case V4L2_CID_AUDIO_LOUDNESS:
> - if (!state->has_sound_processing)
> - return -EINVAL;
> - state->loudness = ctrl->value;
> + val = val ? ((5 * 4) << 8) : 0;
> + msp_write_dsp(client, 0x0004, val);
> + if (state->has_headphones)
> + msp_write_dsp(client, 0x0033, val);
> break;
>
> case V4L2_CID_AUDIO_BALANCE:
> - if (!state->has_sound_processing)
> - return -EINVAL;
> - state->balance = ctrl->value;
> + val = (u8)((val / 256) - 128);
> + msp_write_dsp(client, 0x0001, val << 8);
> + if (state->has_headphones)
> + msp_write_dsp(client, 0x0030, val << 8);
> break;
>
> default:
> return -EINVAL;
> }
> - msp_set_audio(client);
> return 0;
The return value here should reflect if the update was successful or
not. msp_write_dsp can fail and if does the error should be propagated
to the caller and the value of the control should not be updated.
Also, msp_write_dsp and msp_read_dsp should probably return -EIO in
case of failures rather than -1.
Regards,
David Ellingsworth
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 05/15] [RFC] saa7115: convert to the new control framework
OMA_GAIN_CNTL, ctrl->value | 0x80);
> - break;
> - default:
> - return -EINVAL;
> }
> -
> return 0;
> }
>
> -static int saa711x_g_ctrl(struct v4l2_subdev *sd, struct v4l2_control *ctrl)
> +static int saa711x_s_ctrl(struct v4l2_ctrl *ctrl)
> {
> + struct v4l2_subdev *sd = to_sd(ctrl);
> struct saa711x_state *state = to_state(sd);
>
> switch (ctrl->id) {
> case V4L2_CID_BRIGHTNESS:
> - ctrl->value = state->bright;
> + saa711x_write(sd, R_0A_LUMA_BRIGHT_CNTL, ctrl->val);
> break;
> +
> case V4L2_CID_CONTRAST:
> - ctrl->value = state->contrast;
> + saa711x_write(sd, R_0B_LUMA_CONTRAST_CNTL, ctrl->val);
> break;
> +
> case V4L2_CID_SATURATION:
> - ctrl->value = state->sat;
> + saa711x_write(sd, R_0C_CHROMA_SAT_CNTL, ctrl->val);
> break;
> +
> case V4L2_CID_HUE:
> - ctrl->value = state->hue;
> + saa711x_write(sd, R_0D_CHROMA_HUE_CNTL, ctrl->val);
> break;
> +
> case V4L2_CID_CHROMA_AGC:
> - ctrl->value = state->chroma_agc;
> - break;
> - case V4L2_CID_CHROMA_GAIN:
> - ctrl->value = saa711x_read(sd, R_0F_CHROMA_GAIN_CNTL) & 0x7f;
> + /* chroma gain cluster */
> + if (state->agc->val)
> + saa711x_write(sd, R_0F_CHROMA_GAIN_CNTL,
> state->gain->val);
> + else
> + saa711x_write(sd, R_0F_CHROMA_GAIN_CNTL,
> state->gain->val | 0x80);
> + v4l2_ctrl_activate(state->gain, !state->agc->val);
> break;
> +
> default:
> return -EINVAL;
> }
This might not have been done before, but errors from sa71xx_write
should be propagated to caller. A successful write should not be
assumed. This applies for all uses of sa711x_write above. I'd probably
do the following:
1. Declare a local variable called rc initialized to -EINVAL
2. Store the result of sa711x_write into rc for each case.
3. Remove the default case.
4. Return the value of rc.
Regards,
David Ellingsworth
> @@ -1221,25 +1185,6 @@ static int saa711x_g_tuner(struct v4l2_subdev *sd,
> struct v4l2_tuner *vt)
> return 0;
> }
>
> -static int saa711x_queryctrl(struct v4l2_subdev *sd, struct v4l2_queryctrl
> *qc)
> -{
> - switch (qc->id) {
> - case V4L2_CID_BRIGHTNESS:
> - return v4l2_ctrl_query_fill(qc, 0, 255, 1, 128);
> - case V4L2_CID_CONTRAST:
> - case V4L2_CID_SATURATION:
> - return v4l2_ctrl_query_fill(qc, 0, 127, 1, 64);
> - case V4L2_CID_HUE:
> - return v4l2_ctrl_query_fill(qc, -128, 127, 1, 0);
> - case V4L2_CID_CHROMA_AGC:
> - return v4l2_ctrl_query_fill(qc, 0, 1, 1, 1);
> - case V4L2_CID_CHROMA_GAIN:
> - return v4l2_ctrl_query_fill(qc, 0, 127, 1, 48);
> - default:
> - return -EINVAL;
> - }
> -}
> -
> static int saa711x_s_std(struct v4l2_subdev *sd, v4l2_std_id std)
> {
> struct saa711x_state *state = to_state(sd);
> @@ -1516,17 +1461,27 @@ static int saa711x_log_status(struct v4l2_subdev *sd)
> break;
> }
> v4l2_info(sd, "Width, Height: %d, %d\n", state->width,
> state->height);
> + v4l2_ctrl_handler_log_status(&state->hdl, sd->name);
> return 0;
> }
>
> /* --- */
>
> +static const struct v4l2_ctrl_ops saa711x_ctrl_ops = {
> + .s_ctrl = saa711x_s_ctrl,
> + .g_ctrl = saa711x_g_ctrl,
> +};
> +
> static const struct v4l2_subdev_core_ops saa711x_core_ops = {
> .log_status = saa711x_log_status,
> .g_chip_ident = saa711x_g_chip_ident,
> - .g_ctrl = saa711x_g_ctrl,
> - .s_ctrl = saa711x_s_ctrl,
> - .queryctrl = saa711x_queryctrl,
> + .g_ext_ctrls = v4l2_sd_g_ext_ctrls,
> + .try_ext_ctrls = v4l2_sd_try_ext_ctrls,
> + .s_ext_ctrls = v4l2_sd_s_ext_ctrls,
> + .g_ctrl = v4l2_sd_g_ctrl,
> + .s_ctrl = v4l2_sd_s_ctrl,
> + .queryctrl = v4l2_sd_queryctrl,
> + .querymenu = v4l2_sd_querymenu,
> .s_std = saa711x_s_std,
> .reset = saa711x_reset,
> .s_gpio = saa711x_s_gpio,
> @@ -1571,8 +1526,9 @@ static int saa711x_probe(struct i2c_client *client,
> {
> struct saa711x_state *state;
> str
Re: Re: [RFC] Serialization flag example
After looking at the proposed solution, I personally find the suggestion for a serialization flag to be quite ridiculous. As others have mentioned, the mere presence of the flag means that driver writers will gloss over any concurrency issues that might exist in their driver on the mere assumption that specifying the serialization flag will handle it all for them. As the author of the most recent patches to radio-mr800, the proposed changes not only add additional complexity to the driver but also remove some of the fundamental error checking within the driver. Despite the fact the error check might not always be successful, it will still catch the majority of cases. I therefore NACK the proposed patches to this driver. The right thing to do is to actually correct the issue within all the drivers that need it. Is this a lot of work? Maybe, but it's a far better solution as each driver will be responsible for concurrency issues that it may or may not have. After all, wasn't this what the removal of the BKL was about in the first place? Regards, David Ellingsworth -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] s2255drv: cleanup of driver disconnect code
This patch looks good, but there was one other thing that caught my eye. In s2255_probe_v4l, video_device_alloc is called for each video device, which is nothing more than a call to kzalloc, but the result of the call is never verified. Given that this driver has a fixed number of video device nodes, the array of video_device structs could be allocated within the s2255_dev struct. This would remove the extra calls to video_device_alloc, video_device_release, and the additional error checks that should have been there. If you'd prefer to keep the array of video_device structs independent of the s2255_dev struct, an alternative would be to dynamically allocate the entire array at once using kcalloc and store only the pointer to the array in the s2255_dev struct. In my opinion, either of these methods would be better than calling video_device_alloc for each video device that needs to be registered. Regards, David Ellingsworth -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [patch] video/s255drv: cleanup. remove uneeded NULL check
I don't have any problems with this patch in particular but would like to note that this driver could use a little refactoring. One thing I noticed just while glancing at the code is that the return value from s2255_probe_v4l is not checked in s2255_probe. As a result the driver could fail to register some or all of it's video device nodes and still continue to load. Also the use of kref, while needed in this driver due to the number of video nodes created, is probably a bit overused. In my opinion video_unregister_device should be called in the usb disconnect callback for each registered video device. This ensures that no future calls to open will occur for that device. Subsequently, once all applications have stopped using the previously registered video_device, the release callback of the video_device struct will fire. Therefore if the device kref is incremented for each registered video device (during probe) and then properly decremented during the video_device release callback for each device the device driver's structure may then be freed. This approach should lead to a much cleaner implementation of the open, release, and disconnect callbacks. Regards, David Ellingsworth -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: RFC: Drop V4L1 support in V4L2 drivers
On Fri, Mar 19, 2010 at 1:43 PM, Mauro Carvalho Chehab wrote: > David Ellingsworth wrote: >> On Fri, Mar 19, 2010 at 9:47 AM, Mauro Carvalho Chehab >> wrote: >>> The V4L1 drivers that lasts are the ones without maintainers and probably >>> without >>> a large users base. So, basically legacy hardware. So, their removals make >>> sense. >>> >> >> In many ways the above statement is a catch 22. Most, if not all the >> v4l1 drivers are currently broken or unmaintained. However, this does >> not mean there are users who would not be using these drivers if they >> actually worked or had been properly maintained. I know this to be a >> fact of the ibmcam driver. It is both broken and unmaintained. Because >> of this I'm sure no one is currently using it. > > It makes sense. However, considering that no new V4L1 driver is committed > since 2006, this means that those are old drivers for old hardware. > >> I happen to have a USB >> camera which is supposedly supported by the ibmcam driver. > > In the specific case of ibmcam, we had only 10 commits on -hg since its > addition, back in 2006. > > Just using it as an example about the remaining drivers, for today's hardware, > an ibmusb model 3 webcam has 640x480x3fps, according to his driver. Other > models > have QCIF or QVGA as their maximum resolution. I can easily buy a > 640x480x30fps > camera (or even something better than that) for US$12,00 on a close shopping. The limitation on the frame rate within the driver is not an issue with the camera itself per-say. The camera I have supports 640x480x30fps but the ibmcam driver lacks support for the video mode used to achieve that rate. Specifically speaking, the camera uses a proprietary compressed image format that the current v4l1 ibmcam driver does not support for several reasons. First and foremost, the original author stated that he did not have time to reverse engineer the compressed format. Second even if he had, the code to do so doesn't belong in the driver itself. Yes it is an old camera, but that does not mean there aren't people out there who still own cameras which would otherwise be usable if the driver worked. And sure people could just buy another camera.. but why replace hardware that's obviously not broken? > > So, even if the driver would be 100% functional, I doubt that you would find > too > many users of this webcam, simply because people would need a faster frame > rate > or wanted a higher resolution. > >> Unfortunately, I have not the time nor expertise needed to >> update/fix/replace this driver, though I have previously tried. If >> someone on this list is willing to collaborate with me to make a >> functional v4l2 driver to replace the existing ibmcam driver, I'd be >> more than willing to expend more time and energy in doing so. >> Hopefully someday I'll actually be able to use the camera that I own, >> considering as is it barely works under Windows. > > I agree that it would be interesting to port it to V4L2, instead of just > dropping it. Maybe Hans Geode or someone else with some spare time > could help you on this task. > > -- > > Cheers, > Mauro > -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: RFC: Drop V4L1 support in V4L2 drivers
On Fri, Mar 19, 2010 at 9:47 AM, Mauro Carvalho Chehab wrote: > The V4L1 drivers that lasts are the ones without maintainers and probably > without > a large users base. So, basically legacy hardware. So, their removals make > sense. > In many ways the above statement is a catch 22. Most, if not all the v4l1 drivers are currently broken or unmaintained. However, this does not mean there are users who would not be using these drivers if they actually worked or had been properly maintained. I know this to be a fact of the ibmcam driver. It is both broken and unmaintained. Because of this I'm sure no one is currently using it. I happen to have a USB camera which is supposedly supported by the ibmcam driver. Unfortunately, I have not the time nor expertise needed to update/fix/replace this driver, though I have previously tried. If someone on this list is willing to collaborate with me to make a functional v4l2 driver to replace the existing ibmcam driver, I'd be more than willing to expend more time and energy in doing so. Hopefully someday I'll actually be able to use the camera that I own, considering as is it barely works under Windows. Regards, David Ellingsworth -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: How to store the latest image without modifying videobuf-core.c
On Mon, Oct 12, 2009 at 3:33 AM, Mattias Persson wrote: > Hi, > Please send messages to the new mailing list: linux-media@vger.kernel.org from now on. > I am developing a driver for a camera. As an example I am using the vivi > driver (2.6.28) and the first major difference between my ISR and > thread_tick() is that my driver will always attempt to store the latest > image, even if nobody is waiting for a new image. I believe the standard here is that your driver should simply drop the frame if no one is waiting for it. > > In my driver, when all queued buffers are used any new images will be stored > in the oldest frame which has already been captured (state == VIDEOBUF_DONE) > and here is where my problems start. (If this is wrong, what shall I do to > always keep the latest captured image?) > If no one wants the image you should just drop it but note that it existed. I believe the v4l2 api has frame counters so that the application knows that it missed some. > In the function videobuf_dqbuf in videobuf-core.c, if a new image is returned > by stream_next_buffer and the ISR kicks in before videobuf_dqbuf can set > buf->state to VIDEOBUF_IDLE, my driver will modify the image presented to > userspace and that is not acceptable. Correct, it's not acceptable to modify userspace memory when not asked to do so. > > The only solution I can find is to use the spinlock in videobuf_queue when > the userspace application is requesting a new image (DQBUF/poll) to check for > a new image and set some flag indicating that the buffer can't be overwritten > by the ISR. However, this approach would require changes to videobuf-core.c > and that doesn't seem right. Can someone please give me some guidance on this? > > Regards, > Mattias You might want to take a look at possibly using gspca as a base for your driver. It currently supports hundreds of cameras and there are quite a few drivers that you can use as a reference. gspca doesn't use videobuf.. but should make it less painful to write a driver. Regards, David Ellingsworth -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/RFT 08/14] radio-mr800: fix potential use after free
Version 2 From c2c100652ed74d91ade7fdfb2a22d607ff43acf2 Mon Sep 17 00:00:00 2001 From: David Ellingsworth Date: Mon, 21 Sep 2009 22:17:05 -0400 Subject: [PATCH 08/14] mr800: fix potential use after free Signed-off-by: David Ellingsworth --- drivers/media/radio/radio-mr800.c |2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/drivers/media/radio/radio-mr800.c b/drivers/media/radio/radio-mr800.c index 9fd2342..c8fbdde 100644 --- a/drivers/media/radio/radio-mr800.c +++ b/drivers/media/radio/radio-mr800.c @@ -273,8 +273,8 @@ static void usb_amradio_disconnect(struct usb_interface *intf) mutex_unlock(&radio->lock); usb_set_intfdata(intf, NULL); -video_unregister_device(&radio->videodev); v4l2_device_disconnect(&radio->v4l2_dev); +video_unregister_device(&radio->videodev); } /* vidioc_querycap - query device capabilities */ -- 1.6.4.3 >From c2c100652ed74d91ade7fdfb2a22d607ff43acf2 Mon Sep 17 00:00:00 2001 From: David Ellingsworth Date: Mon, 21 Sep 2009 22:17:05 -0400 Subject: [PATCH 08/14] mr800: fix potential use after free Signed-off-by: David Ellingsworth --- drivers/media/radio/radio-mr800.c |2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/drivers/media/radio/radio-mr800.c b/drivers/media/radio/radio-mr800.c index 9fd2342..c8fbdde 100644 --- a/drivers/media/radio/radio-mr800.c +++ b/drivers/media/radio/radio-mr800.c @@ -273,8 +273,8 @@ static void usb_amradio_disconnect(struct usb_interface *intf) mutex_unlock(&radio->lock); usb_set_intfdata(intf, NULL); - video_unregister_device(&radio->videodev); v4l2_device_disconnect(&radio->v4l2_dev); + video_unregister_device(&radio->videodev); } /* vidioc_querycap - query device capabilities */ -- 1.6.4.3
Re: [RFC/RFT 0/14] radio-mr800 patch series
On Mon, Sep 21, 2009 at 8:03 PM, Alexey Klimov wrote: > Hello, David > > On Mon, Sep 14, 2009 at 11:09 PM, Alexey Klimov > wrote: >> Hello David, >> >> On Sun, Sep 13, 2009 at 7:22 AM, David Ellingsworth >> wrote: >>> What follow is a series of patches to clean up the radio-mr800 driver. I >>> do _not_ have access to this device so these patches need to be tested. >>> These patches should apply to Mauro's git tree and against the 2.6.31 >>> release kernel. The patches in this series are as follows: >>> >>> 01. radio-mr800: implement proper locking >>> 02. radio-mr800: simplify video_device allocation >>> 03. radio-mr800: simplify error paths in usb probe callback >>> 04. radio-mr800: remove an unnecessary local variable >>> 05. radio-mr800: simplify access to amradio_device >>> 06. radio-mr800: simplify locking in ioctl callbacks >>> 07. radio-mr800: remove device-removed indicator >>> 08. radio-mr800: fix potential use after free >>> 09. radio-mr800: remove device initialization from open/close >>> 10. radio-mr800: ensure the radio is initialized to a consistent state >>> 11. radio-mr800: fix behavior of set_radio function >>> 12. radio-mr800: preserve radio state during suspend/resume >>> 13. radio-mr800: simplify device warnings >>> 14. radio-mr800: set radio frequency only upon success >>> >>> The first 7 in this series are the same as those submitted in my last series >>> and will not be resent. The remaining 7 patches in this series will be sent >>> separately for review. > > I applied your patches and tested radio device. Radio works fine, but > unfortunately sudden disconnect while playing radio with gnomeradio > creates troubles. I see such oops in dmesg: > > radio_mr800: version 0.11-david AverMedia MR 800 USB FM radio driver > usb 2-2: new low speed USB device using ohci_hcd and address 16 > usb 2-2: New USB device found, idVendor=07ca, idProduct=b800 > usb 2-2: New USB device strings: Mfr=1, Product=2, SerialNumber=0 > usb 2-2: Product: AVerMedia USB Radio > usb 2-2: Manufacturer: AVerMedia Technologies > usb 2-2: configuration #1 chosen from 1 choice > radio-mr800 2-2:1.0: Non-NULL drvdata on register > usb 2-2: USB disconnect, address 16 > BUG: unable to handle kernel NULL pointer dereference at 009f > IP: [] dev_set_drvdata+0x25/0x30 > PGD 3c0e1067 PUD 33b1d067 PMD 0 > Oops: 0002 [#1] SMP > last sysfs file: /sys/devices/pci:00/:00:02.0/usb2/2-2/2-2:1.0/uevent > CPU 1 > Modules linked in: radio_mr800 v4l2_common videodev v4l1_compat > v4l2_compat_ioctl32 nls_iso8859_1 nls_cp437 vfat fat usb_storage > nls_utf8 cifs ext2 ipt_MASQUERADE iptable_nat nf_nat nf_conntrack_ipv4 > nf_conntrack nf_defrag_ipv4 ip_tables x_tables ppp_async crc_ccitt > ppp_generic slhc cpufreq_powersave powernow_k8 freq_table > snd_seq_dummy snd_seq_oss snd_seq_midi_event snd_seq snd_seq_device > snd_pcm_oss snd_mixer_oss reiserfs usbhid hid snd_hda_codec_analog > snd_hda_intel ohci_hcd ehci_hcd snd_hda_codec snd_hwdep snd_pcm > snd_timer nvidia(P) snd usbcore soundcore snd_page_alloc rtc_cmos sg > rtc_core rtc_lib i2c_nforce2 forcedeth e100 nls_base k8temp mii > i2c_core hwmon thermal button [last unloaded: v4l2_compat_ioctl32] > Pid: 11790, comm: gnomeradio Tainted: P 2.6.31 #12 System Product > Name > RIP: 0010:[] [] dev_set_drvdata+0x25/0x30 > RSP: 0018:880031741e28 EFLAGS: 00010206 > RAX: 0017 RBX: 88003c3a4030 RCX: 8109458a > RDX: RSI: RDI: 88003c3a4030 > RBP: R08: R09: > R10: 880029391e70 R11: 0246 R12: 81349ec0 > R13: 880029391e70 R14: 8800173d7000 R15: 88003fac8300 > FS: 7fa0f88e3750() GS:88000290() knlGS:f7327a10 > CS: 0010 DS: ES: CR0: 80050033 > CR2: 009f CR3: 3165c000 CR4: 06e0 > DR0: DR1: DR2: > DR3: DR6: 0ff0 DR7: 0400 > Process gnomeradio (pid: 11790, threadinfo 88003174, task > 88003bcc5820) > Stack: > 8800173d7000 88003c3a5a60 88003c3a5a60 a0c603cb > <0> 88003c3a5800 a0c603e5 88003c3a5800 88003e400890 > <0> 81349ec0 a000d5c7 8800331593c0 8118fb37 > Call Trace: > [] ? v4l2_device_disconnect+0x13/0x1c [videodev] > [] ? v4l2_device_unregister+0x11/0x4b [videodev] > [] ? usb_amradio_video_device_release+0x11/0x26 > [radio_mr800] > [] ? device_
Re: Create a /dev/video0 file and write directly into it images
On Wed, Sep 16, 2009 at 1:08 PM, Paulo Freitas wrote: > Hi everyone, > > I have an Ethernet Camera, from Prosilica, and I need to somehow emulate > this camera in a /dev/video0 file. My idea is, mount a driver file using > 'makedev', pick up images from the camera and write them into > the /dev/video0 file. You know how V4L can be used to write images > in /dev/video files? I don't know if it is needed to use makedev > probably not. Any suggestion is welcome. > > Thank you for your help. > Best regards, Paulo Freitas. Sounds to me like you need a loop back video driver which is capable of being fed data from a user space application. I don't know if this patch was ever applied http://patchwork.kernel.org/patch/24370/ but it seems like something you might be able to use to achieve what you want to do. Regards, David Ellingsworth P.S. In the future, please post questions to the new mailing list: linux-media@vger.kernel.org -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: V4L2: Add a v4l2-subdev (soc-camera) driver for OmniVision OV9640 sensor
On Mon, Sep 14, 2009 at 4:30 PM, Guennadi Liakhovetski wrote: > On Mon, 14 Sep 2009, Marek Vasut wrote: > >> Dne Po 14. září 2009 21:29:26 Guennadi Liakhovetski napsal(a): >> > From: Marek Vasut >> > >> > Signed-off-by: Marek Vasut >> > Signed-off-by: Guennadi Liakhovetski >> > --- >> > >> > Marek, please confirm, that this version is ok. I'll push it upstream for >> > 2.6.32 then. >> >> No, it's not OK. You removed the RGB part. Either enclose those parts into >> ifdef >> OV9640_RGB_BUGGY or preserve it in some other way. Someone will certainly >> want >> to re-add RGB parts later and will have to figure it out all over again. > > Ok, make a proposal, how you would like to see it. But - I do not want > commented out code, including "#ifdef MACRO_THAT_DOESNT_GET_DEFINED." I > think, I described it in sufficient detail, so that re-adding that code > should not take longer than 10 minutes for anyone sufficiently familiar > with the code. Referencing another driver also has an advantage, that if > we switch to imagebus or any other API, you don't get stale commented out > code, but you look up updated code in a functional driver. But I am open > to your ideas / but no commented out code, please. > I don't know much about this driver, but here's my $0.02. Since the RGB code has known issues and this is a new driver, it is probably best to submit it without the code for RGB support. As is, even without direct support from the driver, users can retrieve RGB data from the camera using libv4l. Your argument is therefore more or less mute. If someone were to want to add RGB support to the driver, I'm sure they'd prefer to have a clean and functional driver to work with rather than wading through a bunch of dead/broken code. Regards, David Ellingsworth -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: parameter for module gspca_sn9c20x
On Mon, Sep 14, 2009 at 12:20 PM, Alan Jenkins wrote: > [CC linux-media] The linux-modules list is for the program "modprobe" > etc, not the actual kernel drivers. > > On 9/14/09, baeck...@gmx.net wrote: >> I have a built-in webcam in my laptop: 0c45:624f Microdia PC Camera >> (SN9C201) >> >> Till today I used the driver from [groups.google.de] to make it work. >> But now I found the driver in the 2.6.31 kernel: >> >> gspca_sn9c20x >> >> Unfortunately I have to flip the image vertically so it is displayed right, >> otherwise it is upside- >> down. >> >> With the google-groups-driver I had to use the parameter vflip=1 to flip the >> image. But with the >> kernel module gspca_sn9c20x this is not working: >> >> # modprobe gspca_sn9c20x vflip=1 >> FATAL: Error inserting gspca_sn9c20x >> (/lib/modules/2.6.31/kernel/drivers/media/video/gspca/gspca_sn9c20x.ko): >> Unknown symbol in module, >> or unknown parameter (see dmesg) >> >> Does anyone know how to flip the image? You might want to take a look at libv4l. Several cameras, like yours are known to have the sensor mounted upside down. Kernel drivers typically do not correct for this since it's impossible for the driver to know if the sensor is upside down or not on all supported devices. >From what I recall, libv4l does a couple of checks to determine if the sensor is upside down or not and corrects the image if necessary. It might also provide additional controls that allow you to flip the image even if it's not a sensor that's known to be mounted upside down. libv4l is compatible with all v4l2 applications simply by pre-loading the library for the application using LDPRELOAD. Regards, David Ellingsworth -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: I can't get all pixels values from my driver. plz help ;0(
Guilherme, In the future, please post to the new mailing list: linux-media@vger.kernel.org That said, your camera won't output a video format that it does not support. In other words, despite you specifically asking for a 160x120 pixel RGB image, you may infact get something else. The formats your camera supports may be obtained by using VIDIOC_ENUM_FMT. Once you've selected a format you should then use VIDIOC_ENUM_FRAMESIZES to determine the size of the frames supported by that format. To simplify things a little, you might want to consider using libv4l since will automatically convert the native format supported by your camera to the one required by your application. Regards, David Ellingsworth On Mon, Sep 14, 2009 at 1:50 AM, Guilherme Longo wrote: > > Hi all. > > After 3 weeks trying to solve my problem, I am about to give up and find > another solution instead of trying to fix this one. > I have changed few things in the capture.c example available for download at > http://www.linuxtv.org/downloads/video4linux/API/V4L2_API/ and > the altered code can be seen here: http://pastebin.com/m7ef25480 > > So... what is the problem? > > Well, I have set the following configuration for capture: > > fmt.type = V4L2_BUF_TYPE_VIDEO_CAPTURE; > fmt.fmt.pix.width = 160; > fmt.fmt.pix.height = 120; > fmt.fmt.pix.pixelformat = V4L2_PIX_FMT_RGB32; > fmt.fmt.pix.field = V4L2_FIELD_INTERLACED; > > All the process is implemented in a function called process_image(void * p) > where p is a void pointer to the buffer where the frames captures should be > stored. > > this is the function that reads the buffer: > > if (-1 == read (fd, buffers[0].start, buffers[0].length)) (length = 160x120 > -> 19200) > > then after, I call the process_image function: > process_image (buffers[0].start); > > What I need is read the buffer separation the R, G, B, A storing them in > unsigned char variables. > > It should have 19.200 pixel (160x120) but instead, look what i have got: > > [0]87 [0]110 [0]68 [0]134 > [1]202 [1]73 [1]119 [1]109 > [2]213 [2]36 [2]73 [2]33 > . > . > [1287]73 [1287]100 [1287]150 [1287]133 > [1288]69 [1288]133 [1288]4 [1288]0 > [1289]0 [1289]0 [1289]0 [1289]0 > [1290]0 [1290]0 [1290]0 [1290]0 > [1291]0 [1291]0 [1291]0 [1291]0 > . > [4799]0 [4799]0 [4799]0 [4799]0 > [0]80 [0]105 [0]145 [0]4 > [1]146 [1]18 [1]108 [1]182 > [2]68 [2]136 [2]137 [2]170 > > As you can see, I get only 1289 pixels with values and all the rest are 0; > When the function is called again, the same happens over and over. > > So... why am I getting only 1289 pixel with values when in fact it should be > 160x120 pixel corresponding to 1 frame? > I am begging help 'cause my arsenal's over.. I am really out of ideas! > > Thanks a lot! > > > > _ > Drag n’ drop—Get easy photo sharing with Windows Live™ Photos. > > http://www.microsoft.com/windows/windowslive/products/photos.aspx-- > video4linux-list mailing list > Unsubscribe mailto:video4linux-list-requ...@redhat.com?subjectunsubscribe > https://www.redhat.com/mailman/listinfo/video4linux-list > -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[RFC/RFT 14/14] radio-mr800: set radio frequency only upon success
From 8c441616f67011244cb15bc1a3dda6fd8706ecd2 Mon Sep 17 00:00:00 2001
From: David Ellingsworth
Date: Sat, 12 Sep 2009 16:04:44 -0400
Subject: [PATCH 08/14] mr800: fix potential use after free
Signed-off-by: David Ellingsworth
---
drivers/media/radio/radio-mr800.c |1 -
1 files changed, 0 insertions(+), 1 deletions(-)
diff --git a/drivers/media/radio/radio-mr800.c
b/drivers/media/radio/radio-mr800.c
index 9fd2342..87b58e3 100644
--- a/drivers/media/radio/radio-mr800.c
+++ b/drivers/media/radio/radio-mr800.c
@@ -274,7 +274,6 @@ static void usb_amradio_disconnect(struct
usb_interface *intf)
usb_set_intfdata(intf, NULL);
video_unregister_device(&radio->videodev);
-v4l2_device_disconnect(&radio->v4l2_dev);
}
/* vidioc_querycap - query device capabilities */
--
1.6.3.3
>From 1c62f52da6114756d16644f8401f1903a50ae455 Mon Sep 17 00:00:00 2001
From: David Ellingsworth
Date: Sat, 12 Sep 2009 22:03:56 -0400
Subject: [PATCH 14/14] mr800: set radio frequency only upon success
Signed-off-by: David Ellingsworth
---
drivers/media/radio/radio-mr800.c |8 ++--
1 files changed, 2 insertions(+), 6 deletions(-)
diff --git a/drivers/media/radio/radio-mr800.c b/drivers/media/radio/radio-mr800.c
index 4d955aa..f609fdf 100644
--- a/drivers/media/radio/radio-mr800.c
+++ b/drivers/media/radio/radio-mr800.c
@@ -235,6 +235,7 @@ static int amradio_setfreq(struct amradio_device *radio, int freq)
if (retval < 0 || size != BUFFER_LENGTH)
goto out_err;
+ radio->curfreq = freq;
goto out;
out_err:
@@ -370,13 +371,8 @@ static int vidioc_s_frequency(struct file *file, void *priv,
struct v4l2_frequency *f)
{
struct amradio_device *radio = file->private_data;
- int retval = 0;
-
- radio->curfreq = f->frequency;
- retval = amradio_setfreq(radio, radio->curfreq);
-
- return retval;
+ return amradio_setfreq(radio, f->frequency);
}
/* vidioc_g_frequency - get tuner radio frequency */
--
1.6.3.3
[RFC/RFT 08/14] radio-mr800: fix potential use after free
From 8c441616f67011244cb15bc1a3dda6fd8706ecd2 Mon Sep 17 00:00:00 2001 From: David Ellingsworth Date: Sat, 12 Sep 2009 16:04:44 -0400 Subject: [PATCH 08/14] mr800: fix potential use after free Signed-off-by: David Ellingsworth --- drivers/media/radio/radio-mr800.c |1 - 1 files changed, 0 insertions(+), 1 deletions(-) diff --git a/drivers/media/radio/radio-mr800.c b/drivers/media/radio/radio-mr800.c index 9fd2342..87b58e3 100644 --- a/drivers/media/radio/radio-mr800.c +++ b/drivers/media/radio/radio-mr800.c @@ -274,7 +274,6 @@ static void usb_amradio_disconnect(struct usb_interface *intf) usb_set_intfdata(intf, NULL); video_unregister_device(&radio->videodev); -v4l2_device_disconnect(&radio->v4l2_dev); } /* vidioc_querycap - query device capabilities */ -- 1.6.3.3 >From 8c441616f67011244cb15bc1a3dda6fd8706ecd2 Mon Sep 17 00:00:00 2001 From: David Ellingsworth Date: Sat, 12 Sep 2009 16:04:44 -0400 Subject: [PATCH 08/14] mr800: fix potential use after free Signed-off-by: David Ellingsworth --- drivers/media/radio/radio-mr800.c |1 - 1 files changed, 0 insertions(+), 1 deletions(-) diff --git a/drivers/media/radio/radio-mr800.c b/drivers/media/radio/radio-mr800.c index 9fd2342..87b58e3 100644 --- a/drivers/media/radio/radio-mr800.c +++ b/drivers/media/radio/radio-mr800.c @@ -274,7 +274,6 @@ static void usb_amradio_disconnect(struct usb_interface *intf) usb_set_intfdata(intf, NULL); video_unregister_device(&radio->videodev); - v4l2_device_disconnect(&radio->v4l2_dev); } /* vidioc_querycap - query device capabilities */ -- 1.6.3.3
[RFC/RFT 13/14] radio-mr800: simplify device warnings
From 8c441616f67011244cb15bc1a3dda6fd8706ecd2 Mon Sep 17 00:00:00 2001
From: David Ellingsworth
Date: Sat, 12 Sep 2009 16:04:44 -0400
Subject: [PATCH 08/14] mr800: fix potential use after free
Signed-off-by: David Ellingsworth
---
drivers/media/radio/radio-mr800.c |1 -
1 files changed, 0 insertions(+), 1 deletions(-)
diff --git a/drivers/media/radio/radio-mr800.c
b/drivers/media/radio/radio-mr800.c
index 9fd2342..87b58e3 100644
--- a/drivers/media/radio/radio-mr800.c
+++ b/drivers/media/radio/radio-mr800.c
@@ -274,7 +274,6 @@ static void usb_amradio_disconnect(struct
usb_interface *intf)
usb_set_intfdata(intf, NULL);
video_unregister_device(&radio->videodev);
-v4l2_device_disconnect(&radio->v4l2_dev);
}
/* vidioc_querycap - query device capabilities */
--
1.6.3.3
>From af0aeff199bfba73db6cfcf540936c4c9279aad1 Mon Sep 17 00:00:00 2001
From: David Ellingsworth
Date: Sat, 12 Sep 2009 22:03:16 -0400
Subject: [PATCH 13/14] mr800: simplify device warnings
Signed-off-by: David Ellingsworth
---
drivers/media/radio/radio-mr800.c | 78
1 files changed, 26 insertions(+), 52 deletions(-)
diff --git a/drivers/media/radio/radio-mr800.c b/drivers/media/radio/radio-mr800.c
index ed734bb..4d955aa 100644
--- a/drivers/media/radio/radio-mr800.c
+++ b/drivers/media/radio/radio-mr800.c
@@ -186,8 +186,10 @@ static int amradio_set_mute(struct amradio_device *radio, char argument)
retval = usb_bulk_msg(radio->usbdev, usb_sndintpipe(radio->usbdev, 2),
(void *) (radio->buffer), BUFFER_LENGTH, &size, USB_TIMEOUT);
- if (retval < 0 || size != BUFFER_LENGTH)
+ if (retval < 0 || size != BUFFER_LENGTH) {
+ amradio_dev_warn(&radio->videodev.dev, "set mute failed\n");
return retval;
+ }
radio->muted = argument;
@@ -216,7 +218,7 @@ static int amradio_setfreq(struct amradio_device *radio, int freq)
(void *) (radio->buffer), BUFFER_LENGTH, &size, USB_TIMEOUT);
if (retval < 0 || size != BUFFER_LENGTH)
- return retval;
+ goto out_err;
/* frequency is calculated from freq_send and placed in first 2 bytes */
radio->buffer[0] = (freq_send >> 8) & 0xff;
@@ -230,6 +232,14 @@ static int amradio_setfreq(struct amradio_device *radio, int freq)
retval = usb_bulk_msg(radio->usbdev, usb_sndintpipe(radio->usbdev, 2),
(void *) (radio->buffer), BUFFER_LENGTH, &size, USB_TIMEOUT);
+ if (retval < 0 || size != BUFFER_LENGTH)
+ goto out_err;
+
+ goto out;
+
+out_err:
+ amradio_dev_warn(&radio->videodev.dev, "set frequency failed\n");
+out:
return retval;
}
@@ -252,8 +262,10 @@ static int amradio_set_stereo(struct amradio_device *radio, char argument)
retval = usb_bulk_msg(radio->usbdev, usb_sndintpipe(radio->usbdev, 2),
(void *) (radio->buffer), BUFFER_LENGTH, &size, USB_TIMEOUT);
- if (retval < 0 || size != BUFFER_LENGTH)
+ if (retval < 0 || size != BUFFER_LENGTH) {
+ amradio_dev_warn(&radio->videodev.dev, "set stereo failed\n");
return retval;
+ }
if (argument == WANT_STEREO)
radio->stereo = 1;
@@ -313,9 +325,6 @@ static int vidioc_g_tuner(struct file *file, void *priv,
* amradio_set_stereo shouldn't be here
*/
retval = amradio_set_stereo(radio, WANT_STEREO);
- if (retval < 0)
- amradio_dev_warn(&radio->videodev.dev,
- "set stereo failed\n");
strcpy(v->name, "FM");
v->type = V4L2_TUNER_RADIO;
@@ -347,15 +356,9 @@ static int vidioc_s_tuner(struct file *file, void *priv,
switch (v->audmode) {
case V4L2_TUNER_MODE_MONO:
retval = amradio_set_stereo(radio, WANT_MONO);
- if (retval < 0)
- amradio_dev_warn(&radio->videodev.dev,
-"set mono failed\n");
break;
case V4L2_TUNER_MODE_STEREO:
retval = amradio_set_stereo(radio, WANT_STEREO);
- if (retval < 0)
- amradio_dev_warn(&radio->videodev.dev,
-"set stereo failed\n");
break;
}
@@ -372,9 +375,6 @@ static int vidioc_s_frequency(struct file *file, void *priv,
radio->curfreq = f->frequency;
retval = amradio_setfreq(radio, radio->curfreq);
- if (retval < 0)
- amradio_dev_warn(&radio->videodev.dev,
- "set frequency failed\n");
return retval;
}
@@ -427,19 +427,11 @@ static int vidioc_s_ctrl(struct file *file, void *priv,
switch (ctrl->id) {
case V4L2_CID_AUDIO_MUTE:
- if (ctrl->value) {
+ if (ctrl->value)
retval = amradio_set_mute(radio, AMRADIO_STOP);
- if (retval < 0) {
-amradio_dev_warn(&radio->videodev.dev,
- "amradio_stop failed\n");
- }
- } else {
+ else
retval = amradio_set_mute(radio, AMRADIO_START);
- if (retval < 0) {
-amradio_dev_warn(&radio->videodev.dev,
- "amradio_start failed\n");
- }
- }
+
break;
}
@@ -487,16 +479,12 @@ static int usb_amradio_init(struc
[RFC/RFT 11/14] radio-mr800: fix behavior of set_stereo function
From 8c441616f67011244cb15bc1a3dda6fd8706ecd2 Mon Sep 17 00:00:00 2001
From: David Ellingsworth
Date: Sat, 12 Sep 2009 16:04:44 -0400
Subject: [PATCH 08/14] mr800: fix potential use after free
Signed-off-by: David Ellingsworth
---
drivers/media/radio/radio-mr800.c |1 -
1 files changed, 0 insertions(+), 1 deletions(-)
diff --git a/drivers/media/radio/radio-mr800.c
b/drivers/media/radio/radio-mr800.c
index 9fd2342..87b58e3 100644
--- a/drivers/media/radio/radio-mr800.c
+++ b/drivers/media/radio/radio-mr800.c
@@ -274,7 +274,6 @@ static void usb_amradio_disconnect(struct
usb_interface *intf)
usb_set_intfdata(intf, NULL);
video_unregister_device(&radio->videodev);
-v4l2_device_disconnect(&radio->v4l2_dev);
}
/* vidioc_querycap - query device capabilities */
--
1.6.3.3
>From ea0c11ec6706fbd0777b0147da8a8a827a537699 Mon Sep 17 00:00:00 2001
From: David Ellingsworth
Date: Sat, 12 Sep 2009 22:00:29 -0400
Subject: [PATCH 11/14] mr800: fix behavior of set_stereo function
Signed-off-by: David Ellingsworth
---
drivers/media/radio/radio-mr800.c |9 +
1 files changed, 5 insertions(+), 4 deletions(-)
diff --git a/drivers/media/radio/radio-mr800.c b/drivers/media/radio/radio-mr800.c
index dbf0dbb..8fc413d 100644
--- a/drivers/media/radio/radio-mr800.c
+++ b/drivers/media/radio/radio-mr800.c
@@ -252,12 +252,13 @@ static int amradio_set_stereo(struct amradio_device *radio, char argument)
retval = usb_bulk_msg(radio->usbdev, usb_sndintpipe(radio->usbdev, 2),
(void *) (radio->buffer), BUFFER_LENGTH, &size, USB_TIMEOUT);
- if (retval < 0 || size != BUFFER_LENGTH) {
- radio->stereo = -1;
+ if (retval < 0 || size != BUFFER_LENGTH)
return retval;
- }
- radio->stereo = 1;
+ if (argument == WANT_STEREO)
+ radio->stereo = 1;
+ else
+ radio->stereo = 0;
return retval;
}
--
1.6.3.3
Re: [RFC/RFT 0/10] radio-mr800 patch series
David Ellingsworth wrote: What follow is a series of patches to clean up the radio-mr800 driver. I do _not_ have access to this device so these patches need to be tested. These patches should apply to Mauro's git tree and against the 2.6.31 release kernel. The patches in this series are as follows: 1. radio-mr800: implement proper locking 2. radio-mr800: simplify video_device allocation 3. radio-mr800: simplify error paths in usb probe callback 4. radio-mr800: remove an unnecessary local variable 5. radio-mr800: simplify access to amradio_device 6. radio-mr800: simplify locking in ioctl callbacks 7. radio-mr800: remove device-removed indicator 8. radio-mr800: turn radio on during first open and off during last close 9. radio-mr800: preserve radio-state during suspend/resume 10. radio-mr800: fix potential use after free Each individual patch will follow in a separate email. Regards, David Ellingsworth This series has been superseded by my new 14 patch series. The first 7 patches of this series remain a part of the new series. Regards, David Ellingsworth -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[RFC/RFT 12/14] radio-mr800: preserve radio state during suspend/resume
From 8c441616f67011244cb15bc1a3dda6fd8706ecd2 Mon Sep 17 00:00:00 2001
From: David Ellingsworth
Date: Sat, 12 Sep 2009 16:04:44 -0400
Subject: [PATCH 08/14] mr800: fix potential use after free
Signed-off-by: David Ellingsworth
---
drivers/media/radio/radio-mr800.c |1 -
1 files changed, 0 insertions(+), 1 deletions(-)
diff --git a/drivers/media/radio/radio-mr800.c
b/drivers/media/radio/radio-mr800.c
index 9fd2342..87b58e3 100644
--- a/drivers/media/radio/radio-mr800.c
+++ b/drivers/media/radio/radio-mr800.c
@@ -274,7 +274,6 @@ static void usb_amradio_disconnect(struct
usb_interface *intf)
usb_set_intfdata(intf, NULL);
video_unregister_device(&radio->videodev);
-v4l2_device_disconnect(&radio->v4l2_dev);
}
/* vidioc_querycap - query device capabilities */
--
1.6.3.3
>From c8c27c663db7294c660a3bac659742c915ce91a9 Mon Sep 17 00:00:00 2001
From: David Ellingsworth
Date: Sat, 12 Sep 2009 22:01:49 -0400
Subject: [PATCH 12/14] mr800: preserve radio state during suspend/resume
Signed-off-by: David Ellingsworth
---
drivers/media/radio/radio-mr800.c | 33 -
1 files changed, 28 insertions(+), 5 deletions(-)
diff --git a/drivers/media/radio/radio-mr800.c b/drivers/media/radio/radio-mr800.c
index 8fc413d..ed734bb 100644
--- a/drivers/media/radio/radio-mr800.c
+++ b/drivers/media/radio/radio-mr800.c
@@ -573,9 +573,12 @@ static int usb_amradio_suspend(struct usb_interface *intf, pm_message_t message)
mutex_lock(&radio->lock);
- retval = amradio_set_mute(radio, AMRADIO_STOP);
- if (retval < 0)
- dev_warn(&intf->dev, "amradio_stop failed\n");
+ if (!radio->muted && radio->initialized) {
+ retval = amradio_set_mute(radio, AMRADIO_STOP);
+ if (retval < 0)
+ dev_warn(&intf->dev, "amradio_stop failed\n");
+ radio->muted = 0;
+ }
dev_info(&intf->dev, "going into suspend..\n");
@@ -591,10 +594,30 @@ static int usb_amradio_resume(struct usb_interface *intf)
mutex_lock(&radio->lock);
- retval = amradio_set_mute(radio, AMRADIO_START);
+ if (unlikely(!radio->initialized))
+ goto unlock;
+
+ if (radio->stereo)
+ retval = amradio_set_stereo(radio, WANT_STEREO);
+ else
+ retval = amradio_set_stereo(radio, WANT_MONO);
+
if (retval < 0)
- dev_warn(&intf->dev, "amradio_start failed\n");
+ amradio_dev_warn(&radio->videodev.dev, "set stereo failed\n");
+ retval = amradio_setfreq(radio, radio->curfreq);
+ if (retval < 0)
+ amradio_dev_warn(&radio->videodev.dev,
+ "set frequency failed\n");
+
+ if (!radio->muted) {
+ retval = amradio_set_mute(radio, AMRADIO_START);
+ if (retval < 0)
+ dev_warn(&radio->videodev.dev,
+"amradio_start failed\n");
+ }
+
+unlock:
dev_info(&intf->dev, "coming out of suspend..\n");
mutex_unlock(&radio->lock);
--
1.6.3.3
[RFC/RFT 10/14] radio-mr800: ensure the radio is initialized to a consistent state
From 8c441616f67011244cb15bc1a3dda6fd8706ecd2 Mon Sep 17 00:00:00 2001
From: David Ellingsworth
Date: Sat, 12 Sep 2009 16:04:44 -0400
Subject: [PATCH 08/14] mr800: fix potential use after free
Signed-off-by: David Ellingsworth
---
drivers/media/radio/radio-mr800.c |1 -
1 files changed, 0 insertions(+), 1 deletions(-)
diff --git a/drivers/media/radio/radio-mr800.c
b/drivers/media/radio/radio-mr800.c
index 9fd2342..87b58e3 100644
--- a/drivers/media/radio/radio-mr800.c
+++ b/drivers/media/radio/radio-mr800.c
@@ -274,7 +274,6 @@ static void usb_amradio_disconnect(struct
usb_interface *intf)
usb_set_intfdata(intf, NULL);
video_unregister_device(&radio->videodev);
-v4l2_device_disconnect(&radio->v4l2_dev);
}
/* vidioc_querycap - query device capabilities */
--
1.6.3.3
>From 8b5f17aeea6cf394bedd6f9029a57b8f5815 Mon Sep 17 00:00:00 2001
From: David Ellingsworth
Date: Sat, 12 Sep 2009 21:59:07 -0400
Subject: [PATCH 10/14] mr800: ensure the radio is initialized to a
consistent state
Signed-off-by: David Ellingsworth
---
drivers/media/radio/radio-mr800.c | 34 --
1 files changed, 32 insertions(+), 2 deletions(-)
diff --git a/drivers/media/radio/radio-mr800.c b/drivers/media/radio/radio-mr800.c
index df020e8..dbf0dbb 100644
--- a/drivers/media/radio/radio-mr800.c
+++ b/drivers/media/radio/radio-mr800.c
@@ -85,6 +85,9 @@ MODULE_LICENSE("GPL");
#define amradio_dev_warn(dev, fmt, arg...)\
dev_warn(dev, MR800_DRIVER_NAME " - " fmt, ##arg)
+#define amradio_dev_err(dev, fmt, arg...) \
+ dev_err(dev, MR800_DRIVER_NAME " - " fmt, ##arg)
+
/* Probably USB_TIMEOUT should be modified in module parameter */
#define BUFFER_LENGTH 8
#define USB_TIMEOUT 500
@@ -137,6 +140,7 @@ struct amradio_device {
int curfreq;
int stereo;
int muted;
+ int initialized;
};
#define vdev_to_amradio(r) container_of(r, struct amradio_device, videodev)
@@ -477,6 +481,31 @@ static int vidioc_s_input(struct file *filp, void *priv, unsigned int i)
return 0;
}
+static int usb_amradio_init(struct amradio_device *radio)
+{
+ int retval;
+
+ retval = amradio_set_mute(radio, AMRADIO_STOP);
+ if (retval < 0) {
+ amradio_dev_warn(&radio->videodev.dev, "amradio_stop failed\n");
+ goto out_err;
+ }
+
+ retval = amradio_set_stereo(radio, WANT_STEREO);
+ if (retval < 0) {
+ amradio_dev_warn(&radio->videodev.dev, "set stereo failed\n");
+ goto out_err;
+ }
+
+ radio->initialized = 1;
+ goto out;
+
+out_err:
+ amradio_dev_err(&radio->videodev.dev, "initialization failed\n");
+out:
+ return retval;
+}
+
/* open device - amradio_start() and amradio_setfreq() */
static int usb_amradio_open(struct file *file)
{
@@ -492,6 +521,9 @@ static int usb_amradio_open(struct file *file)
file->private_data = radio;
+ if (unlikely(!radio->initialized))
+ retval = usb_amradio_init(radio);
+
unlock:
mutex_unlock(&radio->lock);
return retval;
@@ -640,8 +672,6 @@ static int usb_amradio_probe(struct usb_interface *intf,
radio->usbdev = interface_to_usbdev(intf);
radio->curfreq = 95.16 * FREQ_MUL;
- radio->stereo = -1;
- radio->muted = 1;
mutex_init(&radio->lock);
--
1.6.3.3
[RFC/RFT 09/14] radio-mr800: remove device initialization from open/close
From 8c441616f67011244cb15bc1a3dda6fd8706ecd2 Mon Sep 17 00:00:00 2001
From: David Ellingsworth
Date: Sat, 12 Sep 2009 16:04:44 -0400
Subject: [PATCH 08/14] mr800: fix potential use after free
Signed-off-by: David Ellingsworth
---
drivers/media/radio/radio-mr800.c |1 -
1 files changed, 0 insertions(+), 1 deletions(-)
diff --git a/drivers/media/radio/radio-mr800.c
b/drivers/media/radio/radio-mr800.c
index 9fd2342..87b58e3 100644
--- a/drivers/media/radio/radio-mr800.c
+++ b/drivers/media/radio/radio-mr800.c
@@ -274,7 +274,6 @@ static void usb_amradio_disconnect(struct
usb_interface *intf)
usb_set_intfdata(intf, NULL);
video_unregister_device(&radio->videodev);
-v4l2_device_disconnect(&radio->v4l2_dev);
}
/* vidioc_querycap - query device capabilities */
--
1.6.3.3
>From 5d01d49c78e2788dca8981af7369c799b650c706 Mon Sep 17 00:00:00 2001
From: David Ellingsworth
Date: Sat, 12 Sep 2009 16:28:45 -0400
Subject: [PATCH 09/14] mr800: remove device initialization from open/close
Device initialization should happen on an as needed basis.
This change allows the device to continue operating even
when there are no applications using it. This should allow
simple command based applications to turn the device on and
off without a persistent process.
Signed-off-by: David Ellingsworth
---
drivers/media/radio/radio-mr800.c | 35 ++-
1 files changed, 2 insertions(+), 33 deletions(-)
diff --git a/drivers/media/radio/radio-mr800.c b/drivers/media/radio/radio-mr800.c
index 87b58e3..df020e8 100644
--- a/drivers/media/radio/radio-mr800.c
+++ b/drivers/media/radio/radio-mr800.c
@@ -136,7 +136,6 @@ struct amradio_device {
struct mutex lock; /* buffer locking */
int curfreq;
int stereo;
- int users;
int muted;
};
@@ -492,26 +491,6 @@ static int usb_amradio_open(struct file *file)
}
file->private_data = radio;
- radio->users = 1;
- radio->muted = 1;
-
- retval = amradio_set_mute(radio, AMRADIO_START);
- if (retval < 0) {
- amradio_dev_warn(&radio->videodev.dev,
- "radio did not start up properly\n");
- radio->users = 0;
- goto unlock;
- }
-
- retval = amradio_set_stereo(radio, WANT_STEREO);
- if (retval < 0)
- amradio_dev_warn(&radio->videodev.dev,
- "set stereo failed\n");
-
- retval = amradio_setfreq(radio, radio->curfreq);
- if (retval < 0)
- amradio_dev_warn(&radio->videodev.dev,
- "set frequency failed\n");
unlock:
mutex_unlock(&radio->lock);
@@ -526,19 +505,9 @@ static int usb_amradio_close(struct file *file)
mutex_lock(&radio->lock);
- if (!radio->usbdev) {
+ if (!radio->usbdev)
retval = -EIO;
- goto unlock;
- }
-
- radio->users = 0;
- retval = amradio_set_mute(radio, AMRADIO_STOP);
- if (retval < 0)
- amradio_dev_warn(&radio->videodev.dev,
- "amradio_stop failed\n");
-
-unlock:
mutex_unlock(&radio->lock);
return retval;
}
@@ -669,10 +638,10 @@ static int usb_amradio_probe(struct usb_interface *intf,
radio->videodev.ioctl_ops = &usb_amradio_ioctl_ops;
radio->videodev.release = usb_amradio_video_device_release;
- radio->users = 0;
radio->usbdev = interface_to_usbdev(intf);
radio->curfreq = 95.16 * FREQ_MUL;
radio->stereo = -1;
+ radio->muted = 1;
mutex_init(&radio->lock);
--
1.6.3.3
[RFC/RFT 0/14] radio-mr800 patch series
What follow is a series of patches to clean up the radio-mr800 driver. I do _not_ have access to this device so these patches need to be tested. These patches should apply to Mauro's git tree and against the 2.6.31 release kernel. The patches in this series are as follows: 01. radio-mr800: implement proper locking 02. radio-mr800: simplify video_device allocation 03. radio-mr800: simplify error paths in usb probe callback 04. radio-mr800: remove an unnecessary local variable 05. radio-mr800: simplify access to amradio_device 06. radio-mr800: simplify locking in ioctl callbacks 07. radio-mr800: remove device-removed indicator 08. radio-mr800: fix potential use after free 09. radio-mr800: remove device initialization from open/close 10. radio-mr800: ensure the radio is initialized to a consistent state 11. radio-mr800: fix behavior of set_radio function 12. radio-mr800: preserve radio state during suspend/resume 13. radio-mr800: simplify device warnings 14. radio-mr800: set radio frequency only upon success The first 7 in this series are the same as those submitted in my last series and will not be resent. The remaining 7 patches in this series will be sent separately for review. Regards, David Ellingsworth -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/RFT 09/10] radio-mr800: preserve radio state during suspend/resume
On Sat, Sep 12, 2009 at 10:50 AM, David Ellingsworth
wrote:
> From 31243088bd32d5568f06f2044f8ff782641e16b5 Mon Sep 17 00:00:00 2001
> From: David Ellingsworth
> Date: Sat, 12 Sep 2009 02:05:57 -0400
> Subject: [PATCH 09/10] mr800: preserve radio state during suspend/resume
>
> Signed-off-by: David Ellingsworth
> ---
> drivers/media/radio/radio-mr800.c | 17 +++--
> 1 files changed, 11 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/media/radio/radio-mr800.c
> b/drivers/media/radio/radio-mr800.c
> index 11db6ea..10bed62 100644
> --- a/drivers/media/radio/radio-mr800.c
> +++ b/drivers/media/radio/radio-mr800.c
> @@ -574,9 +574,12 @@ static int usb_amradio_suspend(struct usb_interface
> *intf, pm_message_t message)
>
> mutex_lock(&radio->lock);
>
> - retval = amradio_set_mute(radio, AMRADIO_STOP);
> - if (retval < 0)
> - dev_warn(&intf->dev, "amradio_stop failed\n");
> + if (!radio->muted) {
> + retval = amradio_set_mute(radio, AMRADIO_STOP);
> + if (retval < 0)
> + dev_warn(&intf->dev, "amradio_stop failed\n");
> + radio->muted = 0;
> + }
>
> dev_info(&intf->dev, "going into suspend..\n");
>
> @@ -592,9 +595,11 @@ static int usb_amradio_resume(struct usb_interface
> *intf)
>
> mutex_lock(&radio->lock);
>
> - retval = amradio_set_mute(radio, AMRADIO_START);
> - if (retval < 0)
> - dev_warn(&intf->dev, "amradio_start failed\n");
> + if (!radio->muted) {
> + retval = amradio_set_mute(radio, AMRADIO_START);
> + if (retval < 0)
> + dev_warn(&intf->dev, "amradio_start failed\n");
> + }
>
> dev_info(&intf->dev, "coming out of suspend..\n");
>
> --
> 1.6.3.3
>
>
I'm going to rework this patch as well. I think the driver needs to do
more than just turn the radio back on. It should also restore the set
frequency and stereo mode.
Regards,
David Ellingsworth
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/RFT 08/10] radio-mr800: turn radio on during first open and off during last close
On Sat, Sep 12, 2009 at 10:50 AM, David Ellingsworth
wrote:
> From 46c7d395e4ed2df431b21b6c07fb02a075a15e43 Mon Sep 17 00:00:00 2001
> From: David Ellingsworth
> Date: Sat, 12 Sep 2009 01:57:36 -0400
> Subject: [PATCH 08/10] mr800: turn radio on during first open and off during
> last close
>
> Signed-off-by: David Ellingsworth
> ---
> drivers/media/radio/radio-mr800.c | 31 +--
> 1 files changed, 17 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/media/radio/radio-mr800.c
> b/drivers/media/radio/radio-mr800.c
> index 9fd2342..11db6ea 100644
> --- a/drivers/media/radio/radio-mr800.c
> +++ b/drivers/media/radio/radio-mr800.c
> @@ -493,15 +493,14 @@ static int usb_amradio_open(struct file *file)
> }
>
> file->private_data = radio;
> - radio->users = 1;
> - radio->muted = 1;
>
> - retval = amradio_set_mute(radio, AMRADIO_START);
> - if (retval < 0) {
> - amradio_dev_warn(&radio->videodev.dev,
> - "radio did not start up properly\n");
> - radio->users = 0;
> - goto unlock;
> + if (radio->users == 0) {
> + retval = amradio_set_mute(radio, AMRADIO_START);
> + if (retval < 0) {
> + amradio_dev_warn(&radio->videodev.dev,
> + "radio did not start up properly\n");
> + goto unlock;
> + }
> }
>
> retval = amradio_set_stereo(radio, WANT_STEREO);
> @@ -514,6 +513,8 @@ static int usb_amradio_open(struct file *file)
> amradio_dev_warn(&radio->videodev.dev,
> "set frequency failed\n");
>
> + radio->users++;
> +
> unlock:
> mutex_unlock(&radio->lock);
> return retval;
> @@ -526,18 +527,19 @@ static int usb_amradio_close(struct file *file)
> int retval = 0;
>
> mutex_lock(&radio->lock);
> + radio->users--;
>
> if (!radio->usbdev) {
> retval = -EIO;
> goto unlock;
> }
>
> - radio->users = 0;
> -
> - retval = amradio_set_mute(radio, AMRADIO_STOP);
> - if (retval < 0)
> - amradio_dev_warn(&radio->videodev.dev,
> - "amradio_stop failed\n");
> + if (radio->users == 0 && !radio->muted) {
> + retval = amradio_set_mute(radio, AMRADIO_STOP);
> + if (retval < 0)
> + amradio_dev_warn(&radio->videodev.dev,
> + "amradio_stop failed\n");
> + }
>
> unlock:
> mutex_unlock(&radio->lock);
> @@ -674,6 +676,7 @@ static int usb_amradio_probe(struct usb_interface *intf,
> radio->usbdev = interface_to_usbdev(intf);
> radio->curfreq = 95.16 * FREQ_MUL;
> radio->stereo = -1;
> + radio->muted = 1;
>
> mutex_init(&radio->lock);
>
> --
> 1.6.3.3
>
>
I just went back over Alexey Klimov's last patch series and I think
I'm going to rework this patch. I agree with the concept he presented
in his last patch whereby the radio should not be turned on/off during
open close, nor should the device's frequency or stereo be set.
Regards,
David Ellingsworth
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[RFC/RFT 10/10] radio-mr800: fix potential use after free
From 987d22363c7a55a5e48a2746a61a6d805fef8661 Mon Sep 17 00:00:00 2001 From: David Ellingsworth Date: Sat, 12 Sep 2009 02:35:22 -0400 Subject: [PATCH 10/10] mr800: fix potential use after free Signed-off-by: David Ellingsworth --- drivers/media/radio/radio-mr800.c |1 - 1 files changed, 0 insertions(+), 1 deletions(-) diff --git a/drivers/media/radio/radio-mr800.c b/drivers/media/radio/radio-mr800.c index 10bed62..806523c 100644 --- a/drivers/media/radio/radio-mr800.c +++ b/drivers/media/radio/radio-mr800.c @@ -274,7 +274,6 @@ static void usb_amradio_disconnect(struct usb_interface *intf) usb_set_intfdata(intf, NULL); video_unregister_device(&radio->videodev); -v4l2_device_disconnect(&radio->v4l2_dev); } /* vidioc_querycap - query device capabilities */ -- 1.6.3.3 >From 987d22363c7a55a5e48a2746a61a6d805fef8661 Mon Sep 17 00:00:00 2001 From: David Ellingsworth Date: Sat, 12 Sep 2009 02:35:22 -0400 Subject: [PATCH 10/10] mr800: fix potential use after free Signed-off-by: David Ellingsworth --- drivers/media/radio/radio-mr800.c |1 - 1 files changed, 0 insertions(+), 1 deletions(-) diff --git a/drivers/media/radio/radio-mr800.c b/drivers/media/radio/radio-mr800.c index 10bed62..806523c 100644 --- a/drivers/media/radio/radio-mr800.c +++ b/drivers/media/radio/radio-mr800.c @@ -274,7 +274,6 @@ static void usb_amradio_disconnect(struct usb_interface *intf) usb_set_intfdata(intf, NULL); video_unregister_device(&radio->videodev); - v4l2_device_disconnect(&radio->v4l2_dev); } /* vidioc_querycap - query device capabilities */ -- 1.6.3.3
[RFC/RFT 09/10] radio-mr800: preserve radio state during suspend/resume
From 31243088bd32d5568f06f2044f8ff782641e16b5 Mon Sep 17 00:00:00 2001
From: David Ellingsworth
Date: Sat, 12 Sep 2009 02:05:57 -0400
Subject: [PATCH 09/10] mr800: preserve radio state during suspend/resume
Signed-off-by: David Ellingsworth
---
drivers/media/radio/radio-mr800.c | 17 +++--
1 files changed, 11 insertions(+), 6 deletions(-)
diff --git a/drivers/media/radio/radio-mr800.c
b/drivers/media/radio/radio-mr800.c
index 11db6ea..10bed62 100644
--- a/drivers/media/radio/radio-mr800.c
+++ b/drivers/media/radio/radio-mr800.c
@@ -574,9 +574,12 @@ static int usb_amradio_suspend(struct usb_interface
*intf, pm_message_t message)
mutex_lock(&radio->lock);
-retval = amradio_set_mute(radio, AMRADIO_STOP);
-if (retval < 0)
-dev_warn(&intf->dev, "amradio_stop failed\n");
+if (!radio->muted) {
+retval = amradio_set_mute(radio, AMRADIO_STOP);
+if (retval < 0)
+dev_warn(&intf->dev, "amradio_stop failed\n");
+radio->muted = 0;
+}
dev_info(&intf->dev, "going into suspend..\n");
@@ -592,9 +595,11 @@ static int usb_amradio_resume(struct usb_interface
*intf)
mutex_lock(&radio->lock);
-retval = amradio_set_mute(radio, AMRADIO_START);
-if (retval < 0)
-dev_warn(&intf->dev, "amradio_start failed\n");
+if (!radio->muted) {
+retval = amradio_set_mute(radio, AMRADIO_START);
+if (retval < 0)
+dev_warn(&intf->dev, "amradio_start failed\n");
+}
dev_info(&intf->dev, "coming out of suspend..\n");
--
1.6.3.3
>From 31243088bd32d5568f06f2044f8ff782641e16b5 Mon Sep 17 00:00:00 2001
From: David Ellingsworth
Date: Sat, 12 Sep 2009 02:05:57 -0400
Subject: [PATCH 09/10] mr800: preserve radio state during suspend/resume
Signed-off-by: David Ellingsworth
---
drivers/media/radio/radio-mr800.c | 17 +++--
1 files changed, 11 insertions(+), 6 deletions(-)
diff --git a/drivers/media/radio/radio-mr800.c b/drivers/media/radio/radio-mr800.c
index 11db6ea..10bed62 100644
--- a/drivers/media/radio/radio-mr800.c
+++ b/drivers/media/radio/radio-mr800.c
@@ -574,9 +574,12 @@ static int usb_amradio_suspend(struct usb_interface *intf, pm_message_t message)
mutex_lock(&radio->lock);
- retval = amradio_set_mute(radio, AMRADIO_STOP);
- if (retval < 0)
- dev_warn(&intf->dev, "amradio_stop failed\n");
+ if (!radio->muted) {
+ retval = amradio_set_mute(radio, AMRADIO_STOP);
+ if (retval < 0)
+ dev_warn(&intf->dev, "amradio_stop failed\n");
+ radio->muted = 0;
+ }
dev_info(&intf->dev, "going into suspend..\n");
@@ -592,9 +595,11 @@ static int usb_amradio_resume(struct usb_interface *intf)
mutex_lock(&radio->lock);
- retval = amradio_set_mute(radio, AMRADIO_START);
- if (retval < 0)
- dev_warn(&intf->dev, "amradio_start failed\n");
+ if (!radio->muted) {
+ retval = amradio_set_mute(radio, AMRADIO_START);
+ if (retval < 0)
+ dev_warn(&intf->dev, "amradio_start failed\n");
+ }
dev_info(&intf->dev, "coming out of suspend..\n");
--
1.6.3.3
[RFC/RFT 08/10] radio-mr800: turn radio on during first open and off during last close
From 46c7d395e4ed2df431b21b6c07fb02a075a15e43 Mon Sep 17 00:00:00 2001
From: David Ellingsworth
Date: Sat, 12 Sep 2009 01:57:36 -0400
Subject: [PATCH 08/10] mr800: turn radio on during first open and off
during last close
Signed-off-by: David Ellingsworth
---
drivers/media/radio/radio-mr800.c | 31 +--
1 files changed, 17 insertions(+), 14 deletions(-)
diff --git a/drivers/media/radio/radio-mr800.c
b/drivers/media/radio/radio-mr800.c
index 9fd2342..11db6ea 100644
--- a/drivers/media/radio/radio-mr800.c
+++ b/drivers/media/radio/radio-mr800.c
@@ -493,15 +493,14 @@ static int usb_amradio_open(struct file *file)
}
file->private_data = radio;
-radio->users = 1;
-radio->muted = 1;
-retval = amradio_set_mute(radio, AMRADIO_START);
-if (retval < 0) {
-amradio_dev_warn(&radio->videodev.dev,
-"radio did not start up properly\n");
-radio->users = 0;
-goto unlock;
+if (radio->users == 0) {
+retval = amradio_set_mute(radio, AMRADIO_START);
+if (retval < 0) {
+amradio_dev_warn(&radio->videodev.dev,
+"radio did not start up properly\n");
+goto unlock;
+}
}
retval = amradio_set_stereo(radio, WANT_STEREO);
@@ -514,6 +513,8 @@ static int usb_amradio_open(struct file *file)
amradio_dev_warn(&radio->videodev.dev,
"set frequency failed\n");
+radio->users++;
+
unlock:
mutex_unlock(&radio->lock);
return retval;
@@ -526,18 +527,19 @@ static int usb_amradio_close(struct file *file)
int retval = 0;
mutex_lock(&radio->lock);
+radio->users--;
if (!radio->usbdev) {
retval = -EIO;
goto unlock;
}
-radio->users = 0;
-
-retval = amradio_set_mute(radio, AMRADIO_STOP);
-if (retval < 0)
-amradio_dev_warn(&radio->videodev.dev,
-"amradio_stop failed\n");
+if (radio->users == 0 && !radio->muted) {
+retval = amradio_set_mute(radio, AMRADIO_STOP);
+if (retval < 0)
+amradio_dev_warn(&radio->videodev.dev,
+"amradio_stop failed\n");
+}
unlock:
mutex_unlock(&radio->lock);
@@ -674,6 +676,7 @@ static int usb_amradio_probe(struct usb_interface *intf,
radio->usbdev = interface_to_usbdev(intf);
radio->curfreq = 95.16 * FREQ_MUL;
radio->stereo = -1;
+radio->muted = 1;
mutex_init(&radio->lock);
--
1.6.3.3
>From 46c7d395e4ed2df431b21b6c07fb02a075a15e43 Mon Sep 17 00:00:00 2001
From: David Ellingsworth
Date: Sat, 12 Sep 2009 01:57:36 -0400
Subject: [PATCH 08/10] mr800: turn radio on during first open and off during last close
Signed-off-by: David Ellingsworth
---
drivers/media/radio/radio-mr800.c | 31 +--
1 files changed, 17 insertions(+), 14 deletions(-)
diff --git a/drivers/media/radio/radio-mr800.c b/drivers/media/radio/radio-mr800.c
index 9fd2342..11db6ea 100644
--- a/drivers/media/radio/radio-mr800.c
+++ b/drivers/media/radio/radio-mr800.c
@@ -493,15 +493,14 @@ static int usb_amradio_open(struct file *file)
}
file->private_data = radio;
- radio->users = 1;
- radio->muted = 1;
- retval = amradio_set_mute(radio, AMRADIO_START);
- if (retval < 0) {
- amradio_dev_warn(&radio->videodev.dev,
- "radio did not start up properly\n");
- radio->users = 0;
- goto unlock;
+ if (radio->users == 0) {
+ retval = amradio_set_mute(radio, AMRADIO_START);
+ if (retval < 0) {
+ amradio_dev_warn(&radio->videodev.dev,
+"radio did not start up properly\n");
+ goto unlock;
+ }
}
retval = amradio_set_stereo(radio, WANT_STEREO);
@@ -514,6 +513,8 @@ static int usb_amradio_open(struct file *file)
amradio_dev_warn(&radio->videodev.dev,
"set frequency failed\n");
+ radio->users++;
+
unlock:
mutex_unlock(&radio->lock);
return retval;
@@ -526,18 +527,19 @@ static int usb_amradio_close(struct file *file)
int retval = 0;
mutex_lock(&radio->lock);
+ radio->users--;
if (!radio->usbdev) {
retval = -EIO;
goto unlock;
}
- radio->users = 0;
-
- retval = amradio_set_mute(radio, AMRADIO_STOP);
- if (retval < 0)
- amradio_dev_warn(&radio->videodev.dev,
- "amradio_stop failed\n");
+ if (radio->users == 0 && !radio->muted) {
+ retval = amradio_set_mute(radio, AMRADIO_STOP);
+ if (retval < 0)
+ amradio_dev_warn(&radio->videodev.dev,
+"amradio_stop failed\n");
+ }
unlock:
mutex_unlock(&radio->lock);
@@ -674,6 +676,7 @@ static int usb_amradio_probe(struct usb_interface *intf,
radio->usbdev = interface_to_usbdev(intf);
radio->curfreq = 95.16 * FREQ_MUL;
radio->stereo = -1;
+ radio->muted = 1;
mutex_init(&radio->lock);
--
1.6.3.3
[RFC/RFT 07/10] radio-mr800: remove device removed indicator
From a9b0a308892919514efc692f2a0e28b80ea304ac Mon Sep 17 00:00:00 2001
From: David Ellingsworth
Date: Sat, 12 Sep 2009 01:22:57 -0400
Subject: [PATCH 07/10] mr800: remove device removed indicator
Signed-off-by: David Ellingsworth
---
drivers/media/radio/radio-mr800.c | 20
1 files changed, 8 insertions(+), 12 deletions(-)
diff --git a/drivers/media/radio/radio-mr800.c
b/drivers/media/radio/radio-mr800.c
index 71d15ba..9fd2342 100644
--- a/drivers/media/radio/radio-mr800.c
+++ b/drivers/media/radio/radio-mr800.c
@@ -137,7 +137,6 @@ struct amradio_device {
int curfreq;
int stereo;
int users;
-int removed;
int muted;
};
@@ -270,7 +269,7 @@ static void usb_amradio_disconnect(struct
usb_interface *intf)
struct amradio_device *radio = usb_get_intfdata(intf);
mutex_lock(&radio->lock);
-radio->removed = 1;
+radio->usbdev = NULL;
mutex_unlock(&radio->lock);
usb_set_intfdata(intf, NULL);
@@ -488,7 +487,7 @@ static int usb_amradio_open(struct file *file)
mutex_lock(&radio->lock);
-if (radio->removed) {
+if (!radio->usbdev) {
retval = -EIO;
goto unlock;
}
@@ -528,19 +527,17 @@ static int usb_amradio_close(struct file *file)
mutex_lock(&radio->lock);
-if (radio->removed) {
+if (!radio->usbdev) {
retval = -EIO;
goto unlock;
}
radio->users = 0;
-if (!radio->removed) {
-retval = amradio_set_mute(radio, AMRADIO_STOP);
-if (retval < 0)
-amradio_dev_warn(&radio->videodev.dev,
-"amradio_stop failed\n");
-}
+retval = amradio_set_mute(radio, AMRADIO_STOP);
+if (retval < 0)
+amradio_dev_warn(&radio->videodev.dev,
+"amradio_stop failed\n");
unlock:
mutex_unlock(&radio->lock);
@@ -555,7 +552,7 @@ static long usb_amradio_ioctl(struct file *file,
unsigned int cmd,
mutex_lock(&radio->lock);
-if (radio->removed) {
+if (!radio->usbdev) {
retval = -EIO;
goto unlock;
}
@@ -673,7 +670,6 @@ static int usb_amradio_probe(struct usb_interface *intf,
radio->videodev.ioctl_ops = &usb_amradio_ioctl_ops;
radio->videodev.release = usb_amradio_video_device_release;
-radio->removed = 0;
radio->users = 0;
radio->usbdev = interface_to_usbdev(intf);
radio->curfreq = 95.16 * FREQ_MUL;
--
1.6.3.3
>From a9b0a308892919514efc692f2a0e28b80ea304ac Mon Sep 17 00:00:00 2001
From: David Ellingsworth
Date: Sat, 12 Sep 2009 01:22:57 -0400
Subject: [PATCH 07/10] mr800: remove device removed indicator
Signed-off-by: David Ellingsworth
---
drivers/media/radio/radio-mr800.c | 20
1 files changed, 8 insertions(+), 12 deletions(-)
diff --git a/drivers/media/radio/radio-mr800.c b/drivers/media/radio/radio-mr800.c
index 71d15ba..9fd2342 100644
--- a/drivers/media/radio/radio-mr800.c
+++ b/drivers/media/radio/radio-mr800.c
@@ -137,7 +137,6 @@ struct amradio_device {
int curfreq;
int stereo;
int users;
- int removed;
int muted;
};
@@ -270,7 +269,7 @@ static void usb_amradio_disconnect(struct usb_interface *intf)
struct amradio_device *radio = usb_get_intfdata(intf);
mutex_lock(&radio->lock);
- radio->removed = 1;
+ radio->usbdev = NULL;
mutex_unlock(&radio->lock);
usb_set_intfdata(intf, NULL);
@@ -488,7 +487,7 @@ static int usb_amradio_open(struct file *file)
mutex_lock(&radio->lock);
- if (radio->removed) {
+ if (!radio->usbdev) {
retval = -EIO;
goto unlock;
}
@@ -528,19 +527,17 @@ static int usb_amradio_close(struct file *file)
mutex_lock(&radio->lock);
- if (radio->removed) {
+ if (!radio->usbdev) {
retval = -EIO;
goto unlock;
}
radio->users = 0;
- if (!radio->removed) {
- retval = amradio_set_mute(radio, AMRADIO_STOP);
- if (retval < 0)
- amradio_dev_warn(&radio->videodev.dev,
-"amradio_stop failed\n");
- }
+ retval = amradio_set_mute(radio, AMRADIO_STOP);
+ if (retval < 0)
+ amradio_dev_warn(&radio->videodev.dev,
+ "amradio_stop failed\n");
unlock:
mutex_unlock(&radio->lock);
@@ -555,7 +552,7 @@ static long usb_amradio_ioctl(struct file *file, unsigned int cmd,
mutex_lock(&radio->lock);
- if (radio->removed) {
+ if (!radio->usbdev) {
retval = -EIO;
goto unlock;
}
@@ -673,7 +670,6 @@ static int usb_amradio_probe(struct usb_interface *intf,
radio->videodev.ioctl_ops = &usb_amradio_ioctl_ops;
radio->videodev.release = usb_amradio_video_device_release;
- radio->removed = 0;
radio->users = 0;
radio->usbdev = interface_to_usbdev(intf);
radio->curfreq = 95.16 * FREQ_MUL;
--
1.6.3.3
[RFC/RFT 06/10] radio-mr800: simplify locking in ioctl callbacks
From c012b1ac39a225e003b190a12ae942e1dd6ea09b Mon Sep 17 00:00:00 2001
From: David Ellingsworth
Date: Sat, 12 Sep 2009 01:07:13 -0400
Subject: [PATCH 06/10] mr800: simplify locking in ioctl callbacks
Signed-off-by: David Ellingsworth
---
drivers/media/radio/radio-mr800.c | 109
++---
1 files changed, 30 insertions(+), 79 deletions(-)
diff --git a/drivers/media/radio/radio-mr800.c
b/drivers/media/radio/radio-mr800.c
index 7305c96..71d15ba 100644
--- a/drivers/media/radio/radio-mr800.c
+++ b/drivers/media/radio/radio-mr800.c
@@ -299,18 +299,8 @@ static int vidioc_g_tuner(struct file *file, void
*priv,
struct amradio_device *radio = file->private_data;
int retval;
-mutex_lock(&radio->lock);
-
-/* safety check */
-if (radio->removed) {
-retval = -EIO;
-goto unlock;
-}
-
-if (v->index > 0) {
-retval = -EINVAL;
-goto unlock;
-}
+if (v->index > 0)
+return -EINVAL;
/* TODO: Add function which look is signal stereo or not
* amradio_getstat(radio);
@@ -338,8 +328,6 @@ static int vidioc_g_tuner(struct file *file, void *priv,
v->signal = 0x; /* Can't get the signal strength, sad.. */
v->afc = 0; /* Don't know what is this */
-unlock:
-mutex_unlock(&radio->lock);
return retval;
}
@@ -348,20 +336,10 @@ static int vidioc_s_tuner(struct file *file, void
*priv,
struct v4l2_tuner *v)
{
struct amradio_device *radio = file->private_data;
-int retval;
-
-mutex_lock(&radio->lock);
-
-/* safety check */
-if (radio->removed) {
-retval = -EIO;
-goto unlock;
-}
+int retval = -EINVAL;
-if (v->index > 0) {
-retval = -EINVAL;
-goto unlock;
-}
+if (v->index > 0)
+return -EINVAL;
/* mono/stereo selector */
switch (v->audmode) {
@@ -377,12 +355,8 @@ static int vidioc_s_tuner(struct file *file, void
*priv,
amradio_dev_warn(&radio->videodev.dev,
"set stereo failed\n");
break;
-default:
-retval = -EINVAL;
}
-unlock:
-mutex_unlock(&radio->lock);
return retval;
}
@@ -391,15 +365,7 @@ static int vidioc_s_frequency(struct file *file,
void *priv,
struct v4l2_frequency *f)
{
struct amradio_device *radio = file->private_data;
-int retval;
-
-mutex_lock(&radio->lock);
-
-/* safety check */
-if (radio->removed) {
-retval = -EIO;
-goto unlock;
-}
+int retval = 0;
radio->curfreq = f->frequency;
@@ -408,8 +374,6 @@ static int vidioc_s_frequency(struct file *file,
void *priv,
amradio_dev_warn(&radio->videodev.dev,
"set frequency failed\n");
-unlock:
-mutex_unlock(&radio->lock);
return retval;
}
@@ -418,22 +382,11 @@ static int vidioc_g_frequency(struct file *file,
void *priv,
struct v4l2_frequency *f)
{
struct amradio_device *radio = file->private_data;
-int retval = 0;
-
-mutex_lock(&radio->lock);
-
-/* safety check */
-if (radio->removed) {
-retval = -EIO;
-goto unlock;
-}
f->type = V4L2_TUNER_RADIO;
f->frequency = radio->curfreq;
-unlock:
-mutex_unlock(&radio->lock);
-return retval;
+return 0;
}
/* vidioc_queryctrl - enumerate control items */
@@ -453,26 +406,14 @@ static int vidioc_g_ctrl(struct file *file, void
*priv,
struct v4l2_control *ctrl)
{
struct amradio_device *radio = file->private_data;
-int retval = -EINVAL;
-
-mutex_lock(&radio->lock);
-
-/* safety check */
-if (radio->removed) {
-retval = -EIO;
-goto unlock;
-}
switch (ctrl->id) {
case V4L2_CID_AUDIO_MUTE:
ctrl->value = radio->muted;
-retval = 0;
-break;
+return 0;
}
-unlock:
-mutex_unlock(&radio->lock);
-return retval;
+return -EINVAL;
}
/* vidioc_s_ctrl - set the value of a control */
@@ -482,14 +423,6 @@ static int vidioc_s_ctrl(struct file *file, void *priv,
struct amradio_device *radio = file->private_data;
int retval = -EINVAL;
-mutex_lock(&radio->lock);
-
-/* safety check */
-if (radio->removed) {
-retval = -EIO;
-goto unlock;
-}
-
switch (ctrl->id) {
case V4L2_CID_AUDIO_MUTE:
if (ctrl->value) {
@@ -508,8 +441,6 @@ static int vidioc_s_ctrl(struct file *file, void *priv,
break;
}
-unlock:
-mutex_unlock(&radio->lock);
return retval;
}
@@ -616,6 +547,26 @@ unlock:
return retval;
}
+static long usb_amradio_ioctl(struct file *file, unsigned int cmd,
+unsigned long arg)
+{
+struct amradio_device *radio = file->private_data;
+long retva
[RFC/RFT 05/10] radio-mr800: simplify access to amradio_device
From 762337020b7744f791fc02fff7eb983e3e4a2346 Mon Sep 17 00:00:00 2001
From: David Ellingsworth
Date: Sat, 12 Sep 2009 00:45:28 -0400
Subject: [PATCH 05/10] mr800: simplify access to amradio_device
Signed-off-by: David Ellingsworth
---
drivers/media/radio/radio-mr800.c | 23 +--
1 files changed, 13 insertions(+), 10 deletions(-)
diff --git a/drivers/media/radio/radio-mr800.c
b/drivers/media/radio/radio-mr800.c
index fb99c6b..7305c96 100644
--- a/drivers/media/radio/radio-mr800.c
+++ b/drivers/media/radio/radio-mr800.c
@@ -141,6 +141,8 @@ struct amradio_device {
int muted;
};
+#define vdev_to_amradio(r) container_of(r, struct amradio_device, videodev)
+
/* USB Device ID List */
static struct usb_device_id usb_amradio_device_table[] = {
{USB_DEVICE_AND_INTERFACE_INFO(USB_AMRADIO_VENDOR, USB_AMRADIO_PRODUCT,
@@ -280,7 +282,7 @@ static void usb_amradio_disconnect(struct
usb_interface *intf)
static int vidioc_querycap(struct file *file, void *priv,
struct v4l2_capability *v)
{
-struct amradio_device *radio = video_drvdata(file);
+struct amradio_device *radio = file->private_data;
strlcpy(v->driver, "radio-mr800", sizeof(v->driver));
strlcpy(v->card, "AverMedia MR 800 USB FM Radio", sizeof(v->card));
@@ -294,7 +296,7 @@ static int vidioc_querycap(struct file *file, void
*priv,
static int vidioc_g_tuner(struct file *file, void *priv,
struct v4l2_tuner *v)
{
-struct amradio_device *radio = video_get_drvdata(video_devdata(file));
+struct amradio_device *radio = file->private_data;
int retval;
mutex_lock(&radio->lock);
@@ -345,7 +347,7 @@ unlock:
static int vidioc_s_tuner(struct file *file, void *priv,
struct v4l2_tuner *v)
{
-struct amradio_device *radio = video_get_drvdata(video_devdata(file));
+struct amradio_device *radio = file->private_data;
int retval;
mutex_lock(&radio->lock);
@@ -388,7 +390,7 @@ unlock:
static int vidioc_s_frequency(struct file *file, void *priv,
struct v4l2_frequency *f)
{
-struct amradio_device *radio = video_get_drvdata(video_devdata(file));
+struct amradio_device *radio = file->private_data;
int retval;
mutex_lock(&radio->lock);
@@ -415,7 +417,7 @@ unlock:
static int vidioc_g_frequency(struct file *file, void *priv,
struct v4l2_frequency *f)
{
-struct amradio_device *radio = video_get_drvdata(video_devdata(file));
+struct amradio_device *radio = file->private_data;
int retval = 0;
mutex_lock(&radio->lock);
@@ -450,7 +452,7 @@ static int vidioc_queryctrl(struct file *file, void
*priv,
static int vidioc_g_ctrl(struct file *file, void *priv,
struct v4l2_control *ctrl)
{
-struct amradio_device *radio = video_get_drvdata(video_devdata(file));
+struct amradio_device *radio = file->private_data;
int retval = -EINVAL;
mutex_lock(&radio->lock);
@@ -477,7 +479,7 @@ unlock:
static int vidioc_s_ctrl(struct file *file, void *priv,
struct v4l2_control *ctrl)
{
-struct amradio_device *radio = video_get_drvdata(video_devdata(file));
+struct amradio_device *radio = file->private_data;
int retval = -EINVAL;
mutex_lock(&radio->lock);
@@ -550,7 +552,7 @@ static int vidioc_s_input(struct file *filp, void
*priv, unsigned int i)
/* open device - amradio_start() and amradio_setfreq() */
static int usb_amradio_open(struct file *file)
{
-struct amradio_device *radio = video_get_drvdata(video_devdata(file));
+struct amradio_device *radio = vdev_to_amradio(video_devdata(file));
int retval = 0;
mutex_lock(&radio->lock);
@@ -560,6 +562,7 @@ static int usb_amradio_open(struct file *file)
goto unlock;
}
+file->private_data = radio;
radio->users = 1;
radio->muted = 1;
@@ -589,7 +592,7 @@ unlock:
/*close device */
static int usb_amradio_close(struct file *file)
{
-struct amradio_device *radio = video_get_drvdata(video_devdata(file));
+struct amradio_device *radio = file->private_data;
int retval = 0;
mutex_lock(&radio->lock);
@@ -674,7 +677,7 @@ static const struct v4l2_ioctl_ops
usb_amradio_ioctl_ops = {
static void usb_amradio_video_device_release(struct video_device *videodev)
{
-struct amradio_device *radio = video_get_drvdata(videodev);
+struct amradio_device *radio = vdev_to_amradio(videodev);
v4l2_device_unregister(&radio->v4l2_dev);
--
1.6.3.3
>From 762337020b7744f791fc02fff7eb983e3e4a2346 Mon Sep 17 00:00:00 2001
From: David Ellingsworth
Date: Sat, 12 Sep 2009 00:45:28 -0400
Subject: [PATCH 05/10] mr800: simplify access to amradio_device
Signed-off-by: David Ellingsworth
---
drivers/media/radio/radio-mr800.c | 23 +--
1 files changed, 13 insertions(+), 10 deletions(-)
diff --git a/drivers/media/radio/ra
[RFC/RFT 04/10] radio-mr800: remove unnecessary local variable
From f2fdb83ce649e9e69413ab533ec4a84d96850ed4 Mon Sep 17 00:00:00 2001
From: David Ellingsworth
Date: Sat, 12 Sep 2009 00:19:48 -0400
Subject: [PATCH 04/10] mr800: remove unnecessary local variable
Signed-off-by: David Ellingsworth
---
drivers/media/radio/radio-mr800.c | 10 --
1 files changed, 4 insertions(+), 6 deletions(-)
diff --git a/drivers/media/radio/radio-mr800.c
b/drivers/media/radio/radio-mr800.c
index d01b96c..fb99c6b 100644
--- a/drivers/media/radio/radio-mr800.c
+++ b/drivers/media/radio/radio-mr800.c
@@ -688,7 +688,6 @@ static int usb_amradio_probe(struct usb_interface *intf,
const struct usb_device_id *id)
{
struct amradio_device *radio;
-struct v4l2_device *v4l2_dev;
int retval = 0;
radio = kzalloc(sizeof(struct amradio_device), GFP_KERNEL);
@@ -707,16 +706,15 @@ static int usb_amradio_probe(struct usb_interface
*intf,
goto err_nobuf;
}
-v4l2_dev = &radio->v4l2_dev;
-retval = v4l2_device_register(&intf->dev, v4l2_dev);
+retval = v4l2_device_register(&intf->dev, &radio->v4l2_dev);
if (retval < 0) {
dev_err(&intf->dev, "couldn't register v4l2_device\n");
goto err_v4l2;
}
-strlcpy(radio->videodev.name, v4l2_dev->name,
+strlcpy(radio->videodev.name, radio->v4l2_dev.name,
sizeof(radio->videodev.name));
-radio->videodev.v4l2_dev = v4l2_dev;
+radio->videodev.v4l2_dev = &radio->v4l2_dev;
radio->videodev.fops = &usb_amradio_fops;
radio->videodev.ioctl_ops = &usb_amradio_ioctl_ops;
radio->videodev.release = usb_amradio_video_device_release;
@@ -742,7 +740,7 @@ static int usb_amradio_probe(struct usb_interface *intf,
return 0;
err_vdev:
-v4l2_device_unregister(v4l2_dev);
+v4l2_device_unregister(&radio->v4l2_dev);
err_v4l2:
kfree(radio->buffer);
err_nobuf:
--
1.6.3.3
>From f2fdb83ce649e9e69413ab533ec4a84d96850ed4 Mon Sep 17 00:00:00 2001
From: David Ellingsworth
Date: Sat, 12 Sep 2009 00:19:48 -0400
Subject: [PATCH 04/10] mr800: remove unnecessary local variable
Signed-off-by: David Ellingsworth
---
drivers/media/radio/radio-mr800.c | 10 --
1 files changed, 4 insertions(+), 6 deletions(-)
diff --git a/drivers/media/radio/radio-mr800.c b/drivers/media/radio/radio-mr800.c
index d01b96c..fb99c6b 100644
--- a/drivers/media/radio/radio-mr800.c
+++ b/drivers/media/radio/radio-mr800.c
@@ -688,7 +688,6 @@ static int usb_amradio_probe(struct usb_interface *intf,
const struct usb_device_id *id)
{
struct amradio_device *radio;
- struct v4l2_device *v4l2_dev;
int retval = 0;
radio = kzalloc(sizeof(struct amradio_device), GFP_KERNEL);
@@ -707,16 +706,15 @@ static int usb_amradio_probe(struct usb_interface *intf,
goto err_nobuf;
}
- v4l2_dev = &radio->v4l2_dev;
- retval = v4l2_device_register(&intf->dev, v4l2_dev);
+ retval = v4l2_device_register(&intf->dev, &radio->v4l2_dev);
if (retval < 0) {
dev_err(&intf->dev, "couldn't register v4l2_device\n");
goto err_v4l2;
}
- strlcpy(radio->videodev.name, v4l2_dev->name,
+ strlcpy(radio->videodev.name, radio->v4l2_dev.name,
sizeof(radio->videodev.name));
- radio->videodev.v4l2_dev = v4l2_dev;
+ radio->videodev.v4l2_dev = &radio->v4l2_dev;
radio->videodev.fops = &usb_amradio_fops;
radio->videodev.ioctl_ops = &usb_amradio_ioctl_ops;
radio->videodev.release = usb_amradio_video_device_release;
@@ -742,7 +740,7 @@ static int usb_amradio_probe(struct usb_interface *intf,
return 0;
err_vdev:
- v4l2_device_unregister(v4l2_dev);
+ v4l2_device_unregister(&radio->v4l2_dev);
err_v4l2:
kfree(radio->buffer);
err_nobuf:
--
1.6.3.3
[RFC/RFT 03/10] radio-mr800: simplify error paths in usb probe callback
From 0cdbd79a6e87a8a2862a6c1309c8fdf83c80ba61 Mon Sep 17 00:00:00 2001
From: David Ellingsworth
Date: Sat, 12 Sep 2009 00:13:16 -0400
Subject: [PATCH 03/10] mr800: simplify error paths in usb probe callback
Signed-off-by: David Ellingsworth
---
drivers/media/radio/radio-mr800.c | 27 ---
1 files changed, 16 insertions(+), 11 deletions(-)
diff --git a/drivers/media/radio/radio-mr800.c
b/drivers/media/radio/radio-mr800.c
index 3129692..d01b96c 100644
--- a/drivers/media/radio/radio-mr800.c
+++ b/drivers/media/radio/radio-mr800.c
@@ -689,30 +689,29 @@ static int usb_amradio_probe(struct usb_interface
*intf,
{
struct amradio_device *radio;
struct v4l2_device *v4l2_dev;
-int retval;
+int retval = 0;
radio = kzalloc(sizeof(struct amradio_device), GFP_KERNEL);
if (!radio) {
dev_err(&intf->dev, "kmalloc for amradio_device failed\n");
-return -ENOMEM;
+retval = -ENOMEM;
+goto err;
}
radio->buffer = kmalloc(BUFFER_LENGTH, GFP_KERNEL);
if (!radio->buffer) {
dev_err(&intf->dev, "kmalloc for radio->buffer failed\n");
-kfree(radio);
-return -ENOMEM;
+retval = -ENOMEM;
+goto err_nobuf;
}
v4l2_dev = &radio->v4l2_dev;
retval = v4l2_device_register(&intf->dev, v4l2_dev);
if (retval < 0) {
dev_err(&intf->dev, "couldn't register v4l2_device\n");
-kfree(radio->buffer);
-kfree(radio);
-return retval;
+goto err_v4l2;
}
strlcpy(radio->videodev.name, v4l2_dev->name,
@@ -736,14 +735,20 @@ static int usb_amradio_probe(struct usb_interface
*intf,
radio_nr);
if (retval < 0) {
dev_err(&intf->dev, "could not register video device\n");
-v4l2_device_unregister(v4l2_dev);
-kfree(radio->buffer);
-kfree(radio);
-return -EIO;
+goto err_vdev;
}
usb_set_intfdata(intf, radio);
return 0;
+
+err_vdev:
+v4l2_device_unregister(v4l2_dev);
+err_v4l2:
+kfree(radio->buffer);
+err_nobuf:
+kfree(radio);
+err:
+return retval;
}
static int __init amradio_init(void)
--
1.6.3.3
>From 0cdbd79a6e87a8a2862a6c1309c8fdf83c80ba61 Mon Sep 17 00:00:00 2001
From: David Ellingsworth
Date: Sat, 12 Sep 2009 00:13:16 -0400
Subject: [PATCH 03/10] mr800: simplify error paths in usb probe callback
Signed-off-by: David Ellingsworth
---
drivers/media/radio/radio-mr800.c | 27 ---
1 files changed, 16 insertions(+), 11 deletions(-)
diff --git a/drivers/media/radio/radio-mr800.c b/drivers/media/radio/radio-mr800.c
index 3129692..d01b96c 100644
--- a/drivers/media/radio/radio-mr800.c
+++ b/drivers/media/radio/radio-mr800.c
@@ -689,30 +689,29 @@ static int usb_amradio_probe(struct usb_interface *intf,
{
struct amradio_device *radio;
struct v4l2_device *v4l2_dev;
- int retval;
+ int retval = 0;
radio = kzalloc(sizeof(struct amradio_device), GFP_KERNEL);
if (!radio) {
dev_err(&intf->dev, "kmalloc for amradio_device failed\n");
- return -ENOMEM;
+ retval = -ENOMEM;
+ goto err;
}
radio->buffer = kmalloc(BUFFER_LENGTH, GFP_KERNEL);
if (!radio->buffer) {
dev_err(&intf->dev, "kmalloc for radio->buffer failed\n");
- kfree(radio);
- return -ENOMEM;
+ retval = -ENOMEM;
+ goto err_nobuf;
}
v4l2_dev = &radio->v4l2_dev;
retval = v4l2_device_register(&intf->dev, v4l2_dev);
if (retval < 0) {
dev_err(&intf->dev, "couldn't register v4l2_device\n");
- kfree(radio->buffer);
- kfree(radio);
- return retval;
+ goto err_v4l2;
}
strlcpy(radio->videodev.name, v4l2_dev->name,
@@ -736,14 +735,20 @@ static int usb_amradio_probe(struct usb_interface *intf,
radio_nr);
if (retval < 0) {
dev_err(&intf->dev, "could not register video device\n");
- v4l2_device_unregister(v4l2_dev);
- kfree(radio->buffer);
- kfree(radio);
- return -EIO;
+ goto err_vdev;
}
usb_set_intfdata(intf, radio);
return 0;
+
+err_vdev:
+ v4l2_device_unregister(v4l2_dev);
+err_v4l2:
+ kfree(radio->buffer);
+err_nobuf:
+ kfree(radio);
+err:
+ return retval;
}
static int __init amradio_init(void)
--
1.6.3.3
[RFC/RFT 02/10] radio-mr800: simplify video_device allocation
From 2839cd94e21123151c0fe6683991f5a3c88fa877 Mon Sep 17 00:00:00 2001
From: David Ellingsworth
Date: Fri, 11 Sep 2009 23:59:22 -0400
Subject: [PATCH 02/10] mr800: simplify video_device allocation
Signed-off-by: David Ellingsworth
---
drivers/media/radio/radio-mr800.c | 53
++--
1 files changed, 21 insertions(+), 32 deletions(-)
diff --git a/drivers/media/radio/radio-mr800.c
b/drivers/media/radio/radio-mr800.c
index 8e96c8a..3129692 100644
--- a/drivers/media/radio/radio-mr800.c
+++ b/drivers/media/radio/radio-mr800.c
@@ -129,7 +129,7 @@ static int usb_amradio_resume(struct usb_interface
*intf);
struct amradio_device {
/* reference to USB and video device */
struct usb_device *usbdev;
-struct video_device *videodev;
+struct video_device videodev;
struct v4l2_device v4l2_dev;
unsigned char *buffer;
@@ -272,7 +272,7 @@ static void usb_amradio_disconnect(struct
usb_interface *intf)
mutex_unlock(&radio->lock);
usb_set_intfdata(intf, NULL);
-video_unregister_device(radio->videodev);
+video_unregister_device(&radio->videodev);
v4l2_device_disconnect(&radio->v4l2_dev);
}
@@ -320,7 +320,7 @@ static int vidioc_g_tuner(struct file *file, void *priv,
*/
retval = amradio_set_stereo(radio, WANT_STEREO);
if (retval < 0)
-amradio_dev_warn(&radio->videodev->dev,
+amradio_dev_warn(&radio->videodev.dev,
"set stereo failed\n");
strcpy(v->name, "FM");
@@ -366,13 +366,13 @@ static int vidioc_s_tuner(struct file *file, void
*priv,
case V4L2_TUNER_MODE_MONO:
retval = amradio_set_stereo(radio, WANT_MONO);
if (retval < 0)
-amradio_dev_warn(&radio->videodev->dev,
+amradio_dev_warn(&radio->videodev.dev,
"set mono failed\n");
break;
case V4L2_TUNER_MODE_STEREO:
retval = amradio_set_stereo(radio, WANT_STEREO);
if (retval < 0)
-amradio_dev_warn(&radio->videodev->dev,
+amradio_dev_warn(&radio->videodev.dev,
"set stereo failed\n");
break;
default:
@@ -403,7 +403,7 @@ static int vidioc_s_frequency(struct file *file,
void *priv,
retval = amradio_setfreq(radio, radio->curfreq);
if (retval < 0)
-amradio_dev_warn(&radio->videodev->dev,
+amradio_dev_warn(&radio->videodev.dev,
"set frequency failed\n");
unlock:
@@ -493,13 +493,13 @@ static int vidioc_s_ctrl(struct file *file, void
*priv,
if (ctrl->value) {
retval = amradio_set_mute(radio, AMRADIO_STOP);
if (retval < 0) {
-amradio_dev_warn(&radio->videodev->dev,
+amradio_dev_warn(&radio->videodev.dev,
"amradio_stop failed\n");
}
} else {
retval = amradio_set_mute(radio, AMRADIO_START);
if (retval < 0) {
-amradio_dev_warn(&radio->videodev->dev,
+amradio_dev_warn(&radio->videodev.dev,
"amradio_start failed\n");
}
}
@@ -565,7 +565,7 @@ static int usb_amradio_open(struct file *file)
retval = amradio_set_mute(radio, AMRADIO_START);
if (retval < 0) {
-amradio_dev_warn(&radio->videodev->dev,
+amradio_dev_warn(&radio->videodev.dev,
"radio did not start up properly\n");
radio->users = 0;
goto unlock;
@@ -573,12 +573,12 @@ static int usb_amradio_open(struct file *file)
retval = amradio_set_stereo(radio, WANT_STEREO);
if (retval < 0)
-amradio_dev_warn(&radio->videodev->dev,
+amradio_dev_warn(&radio->videodev.dev,
"set stereo failed\n");
retval = amradio_setfreq(radio, radio->curfreq);
if (retval < 0)
-amradio_dev_warn(&radio->videodev->dev,
+amradio_dev_warn(&radio->videodev.dev,
"set frequency failed\n");
unlock:
@@ -604,7 +604,7 @@ static int usb_amradio_close(struct file *file)
if (!radio->removed) {
retval = amradio_set_mute(radio, AMRADIO_STOP);
if (retval < 0)
-amradio_dev_warn(&radio->videodev->dev,
+amradio_dev_warn(&radio->videodev.dev,
"amradio_stop failed\n");
}
@@ -676,9 +676,6 @@ static void usb_amradio_video_device_release(struct
video_device *videodev)
{
struct amradio_device *radio = video_get_drvdata(videodev);
-/* we call v4l to free radio->videodev */
-video_device_release(videodev);
-
v4l2_device_unregister(&radio->v4l2_dev);
/* free rest memory */
@@ -718,20 +715,12 @@ static int usb_amradio_probe(stru
[RFC/RFT 01/10] radio-mr800: implement proper locking
From 1773df59dc8e63ca00a27f5235c293341fd07f36 Mon Sep 17 00:00:00 2001
From: David Ellingsworth
Date: Fri, 11 Sep 2009 23:21:17 -0400
Subject: [PATCH 01/10] mr800: implement proper locking
Signed-off-by: David Ellingsworth
---
drivers/media/radio/radio-mr800.c | 181
++---
1 files changed, 106 insertions(+), 75 deletions(-)
diff --git a/drivers/media/radio/radio-mr800.c
b/drivers/media/radio/radio-mr800.c
index 575bf9d..8e96c8a 100644
--- a/drivers/media/radio/radio-mr800.c
+++ b/drivers/media/radio/radio-mr800.c
@@ -168,11 +168,7 @@ static int amradio_set_mute(struct amradio_device
*radio, char argument)
int retval;
int size;
-/* safety check */
-if (radio->removed)
-return -EIO;
-
-mutex_lock(&radio->lock);
+BUG_ON(!mutex_is_locked(&radio->lock));
radio->buffer[0] = 0x00;
radio->buffer[1] = 0x55;
@@ -186,15 +182,11 @@ static int amradio_set_mute(struct amradio_device
*radio, char argument)
retval = usb_bulk_msg(radio->usbdev, usb_sndintpipe(radio->usbdev, 2),
(void *) (radio->buffer), BUFFER_LENGTH, &size, USB_TIMEOUT);
-if (retval < 0 || size != BUFFER_LENGTH) {
-mutex_unlock(&radio->lock);
+if (retval < 0 || size != BUFFER_LENGTH)
return retval;
-}
radio->muted = argument;
-mutex_unlock(&radio->lock);
-
return retval;
}
@@ -205,11 +197,7 @@ static int amradio_setfreq(struct amradio_device
*radio, int freq)
int size;
unsigned short freq_send = 0x10 + (freq >> 3) / 25;
-/* safety check */
-if (radio->removed)
-return -EIO;
-
-mutex_lock(&radio->lock);
+BUG_ON(!mutex_is_locked(&radio->lock));
radio->buffer[0] = 0x00;
radio->buffer[1] = 0x55;
@@ -223,10 +211,8 @@ static int amradio_setfreq(struct amradio_device
*radio, int freq)
retval = usb_bulk_msg(radio->usbdev, usb_sndintpipe(radio->usbdev, 2),
(void *) (radio->buffer), BUFFER_LENGTH, &size, USB_TIMEOUT);
-if (retval < 0 || size != BUFFER_LENGTH) {
-mutex_unlock(&radio->lock);
+if (retval < 0 || size != BUFFER_LENGTH)
return retval;
-}
/* frequency is calculated from freq_send and placed in first 2
bytes */
radio->buffer[0] = (freq_send >> 8) & 0xff;
@@ -240,13 +226,6 @@ static int amradio_setfreq(struct amradio_device
*radio, int freq)
retval = usb_bulk_msg(radio->usbdev, usb_sndintpipe(radio->usbdev, 2),
(void *) (radio->buffer), BUFFER_LENGTH, &size, USB_TIMEOUT);
-if (retval < 0 || size != BUFFER_LENGTH) {
-mutex_unlock(&radio->lock);
-return retval;
-}
-
-mutex_unlock(&radio->lock);
-
return retval;
}
@@ -255,11 +234,7 @@ static int amradio_set_stereo(struct amradio_device
*radio, char argument)
int retval;
int size;
-/* safety check */
-if (radio->removed)
-return -EIO;
-
-mutex_lock(&radio->lock);
+BUG_ON(!mutex_is_locked(&radio->lock));
radio->buffer[0] = 0x00;
radio->buffer[1] = 0x55;
@@ -275,14 +250,11 @@ static int amradio_set_stereo(struct
amradio_device *radio, char argument)
if (retval < 0 || size != BUFFER_LENGTH) {
radio->stereo = -1;
-mutex_unlock(&radio->lock);
return retval;
}
radio->stereo = 1;
-mutex_unlock(&radio->lock);
-
return retval;
}
@@ -325,12 +297,18 @@ static int vidioc_g_tuner(struct file *file, void
*priv,
struct amradio_device *radio = video_get_drvdata(video_devdata(file));
int retval;
+mutex_lock(&radio->lock);
+
/* safety check */
-if (radio->removed)
-return -EIO;
+if (radio->removed) {
+retval = -EIO;
+goto unlock;
+}
-if (v->index > 0)
-return -EINVAL;
+if (v->index > 0) {
+retval = -EINVAL;
+goto unlock;
+}
/* TODO: Add function which look is signal stereo or not
* amradio_getstat(radio);
@@ -357,7 +335,10 @@ static int vidioc_g_tuner(struct file *file, void
*priv,
v->audmode = V4L2_TUNER_MODE_MONO;
v->signal = 0x; /* Can't get the signal strength, sad.. */
v->afc = 0; /* Don't know what is this */
-return 0;
+
+unlock:
+mutex_unlock(&radio->lock);
+return retval;
}
/* vidioc_s_tuner - set tuner attributes */
@@ -367,12 +348,18 @@ static int vidioc_s_tuner(struct file *file, void
*priv,
struct amradio_device *radio = video_get_drvdata(video_devdata(file));
int retval;
+mutex_lock(&radio->lock);
+
/* safety check */
-if (radio->removed)
-return -EIO;
+if (radio->removed) {
+retval = -EIO;
+goto unlock;
+}
-if (v->index > 0)
-return -EINVAL;
+if (v-&g
[RFC/RFT 0/10] radio-mr800 patch series
What follow is a series of patches to clean up the radio-mr800 driver. I do _not_ have access to this device so these patches need to be tested. These patches should apply to Mauro's git tree and against the 2.6.31 release kernel. The patches in this series are as follows: 1. radio-mr800: implement proper locking 2. radio-mr800: simplify video_device allocation 3. radio-mr800: simplify error paths in usb probe callback 4. radio-mr800: remove an unnecessary local variable 5. radio-mr800: simplify access to amradio_device 6. radio-mr800: simplify locking in ioctl callbacks 7. radio-mr800: remove device-removed indicator 8. radio-mr800: turn radio on during first open and off during last close 9. radio-mr800: preserve radio-state during suspend/resume 10. radio-mr800: fix potential use after free Each individual patch will follow in a separate email. Regards, David Ellingsworth -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [patch review 6/6] radio-mr800: redesign radio->users counter
On Sat, Aug 8, 2009 at 1:46 PM, Alexey Klimov wrote:
> Redesign radio->users counter. Don't allow more that 5 users on radio in
> usb_amradio_open() and don't stop radio device if other userspace
> application uses it in usb_amradio_close().
>
> Signed-off-by: Alexey Klimov
>
> --
> diff -r c2dd9da28106 linux/drivers/media/radio/radio-mr800.c
> --- a/linux/drivers/media/radio/radio-mr800.c Sat Aug 08 17:28:18 2009 +0400
> +++ b/linux/drivers/media/radio/radio-mr800.c Sat Aug 08 18:12:01 2009 +0400
> @@ -540,7 +540,13 @@
> {
> struct amradio_device *radio = video_get_drvdata(video_devdata(file));
>
> - radio->users = 1;
> + /* don't allow more than 5 users on radio */
> + if (radio->users > 4)
> + return -EBUSY;
I agree with what the others have said regarding this.. there should
be no such restriction here. The code is broken anyway as it doesn't
acquire the lock before checking the number of active users. So
technically, while you've tried to limit it to five, six, seven, or
more users could gain access to it under the right conditions.
> +
> + mutex_lock(&radio->lock);
> + radio->users++;
> + mutex_unlock(&radio->lock);
>
> return 0;
> }
> @@ -554,9 +560,20 @@
> return -ENODEV;
>
> mutex_lock(&radio->lock);
> - radio->users = 0;
> + radio->users--;
> mutex_unlock(&radio->lock);
>
> + /* In case several userspace applications opened the radio
> + * and one of them closes and stops it,
> + * we check if others use it and if they do we start the radio again.
> */
> + if (radio->users && radio->status == AMRADIO_STOP) {
More locking issues here as well. Two competing opens might both see
the status as stopped and both try to start the device.
> + int retval;
> + retval = amradio_set_mute(radio, AMRADIO_START);
> + if (retval < 0)
> + dev_warn(&radio->videodev->dev,
> + "amradio_start failed\n");
> + }
> +
> return 0;
> }
>
Regards,
David Ellingsworth
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [patch review 4/6] radio-mr800: make radio->status variable
t;curfreq = 95.16 * FREQ_MUL; > > > -- > Best regards, Klimov Alexey I'm not sure all these checks for the device being disconnected are actually needed. They are all done without taking the lock and won't necessarily prevent a IO error from occurring after the status has been checked. IE, the device might be connected at the time the status is checked, but disconnected immediately after. It would be better for the driver to just assume the device was there and allow usb functions it calls to fail appropriately. Since the usb_disconnect callback most likely modifies the driver's state, it should probably take the lock along with open, close, and any other ioctl which uses or modifies the state of the driver to avoid race conditions between them. With all of your ioctls competing for the lock, the usb_disconnect callback wont be able to do anything while others have the lock. So if the device is disconnected during that time, calls to the usb subsystem will fail. In these cases you should catch the failure, unlock the device, and return EIO. Eventually the usb_disconnect callback will be called, and you'll be able to update the driver's state to indicate that usb calls are no longer valid. IMHO, you shouldn't need the DISCONNECTED state, as you already have an indicator for that (IE the usbdev pointer). Regards, David Ellingsworth -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [patch review 3/6] radio-mr800: no need to pass curfreq value to amradio_setfreq()
On Sat, Aug 8, 2009 at 1:46 PM, Alexey Klimov wrote:
> Small cleanup of amradio_setfreq(). No need to pass radio->curfreq value
> to this function.
>
> Signed-off-by: Alexey Klimov
>
> --
> diff -r 5f3329bebfe4 linux/drivers/media/radio/radio-mr800.c
> --- a/linux/drivers/media/radio/radio-mr800.c Wed Jul 29 12:36:46 2009 +0400
> +++ b/linux/drivers/media/radio/radio-mr800.c Wed Jul 29 12:41:51 2009 +0400
> @@ -202,11 +202,11 @@
> }
>
> /* set a frequency, freq is defined by v4l's TUNER_LOW, i.e. 1/16th kHz */
> -static int amradio_setfreq(struct amradio_device *radio, int freq)
> +static int amradio_setfreq(struct amradio_device *radio)
> {
> int retval;
> int size;
> - unsigned short freq_send = 0x10 + (freq >> 3) / 25;
> + unsigned short freq_send = 0x10 + (radio->curfreq >> 3) / 25;
I suspect there may be race conditions here. Once again you're reading
a value without first acquiring the lock. Since this is another
utility function, the lock should probably be held _before_ calling
this function and any locking in this function should be removed.
Adding a BUG_ON(!is_mutex_locked(&radio->lock)) should probably be
added as well.
>
> /* safety check */
> if (radio->removed)
> @@ -413,7 +413,7 @@
> radio->curfreq = f->frequency;
> mutex_unlock(&radio->lock);
>
> - retval = amradio_setfreq(radio, radio->curfreq);
> + retval = amradio_setfreq(radio);
> if (retval < 0)
> amradio_dev_warn(&radio->videodev->dev,
> "set frequency failed\n");
>
>
>
> --
> Best regards, Klimov Alexey
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-media" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
Regards,
David Ellingsworth
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [patch review 1/6] radio-mr800: remove redundant lock/unlock_kernel
On Sat, Aug 8, 2009 at 1:45 PM, Alexey Klimov wrote: > Remove redundant lock/unlock_kernel() calls from usb_amradio_open/close > functions. > > Signed-off-by: Alexey Klimov > > -- > diff -r ee6cf88cb5d3 linux/drivers/media/radio/radio-mr800.c > --- a/linux/drivers/media/radio/radio-mr800.c Wed Jul 29 01:42:02 2009 -0300 > +++ b/linux/drivers/media/radio/radio-mr800.c Wed Jul 29 10:44:02 2009 +0400 > @@ -540,8 +540,6 @@ > struct amradio_device *radio = video_get_drvdata(video_devdata(file)); > int retval; > > - lock_kernel(); > - Maybe I'm missing something here, but the lock_kernel() call seems very necessary here since you're modifying the state of the driver without taking the driver's lock. Last I checked the v4l2 subsystem doesn't call open with it's lock held. So by removing these locks you've introduced a race condition between open and close. > radio->users = 1; > radio->muted = 1; > > @@ -550,7 +548,6 @@ > amradio_dev_warn(&radio->videodev->dev, > "radio did not start up properly\n"); > radio->users = 0; > - unlock_kernel(); > return -EIO; > } > > @@ -564,7 +561,6 @@ > amradio_dev_warn(&radio->videodev->dev, > "set frequency failed\n"); > > - unlock_kernel(); > return 0; > } > > > > -- > Best regards, Klimov Alexey > Regards, David Ellingsworth -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [patch review 5/6] radio-mr800: update suspend/resume procedure
On Sat, Aug 8, 2009 at 1:46 PM, Alexey Klimov wrote:
> Patch fixes suspend/resume procedure.
>
> Signed-off-by: Alexey Klimov
>
> --
> diff -r 05754a500f80 linux/drivers/media/radio/radio-mr800.c
> --- a/linux/drivers/media/radio/radio-mr800.c Sat Aug 08 20:06:36 2009 +0400
> +++ b/linux/drivers/media/radio/radio-mr800.c Sat Aug 08 20:22:05 2009 +0400
> @@ -564,11 +564,23 @@
> static int usb_amradio_suspend(struct usb_interface *intf, pm_message_t
> message)
> {
> struct amradio_device *radio = usb_get_intfdata(intf);
> - int retval;
>
> - retval = amradio_set_mute(radio, AMRADIO_STOP);
> - if (retval < 0)
> - dev_warn(&intf->dev, "amradio_stop failed\n");
> + if (radio->status == AMRADIO_START) {
I think you need to take a good look at the locking in this driver.
The above condition should technically be checked with the lock held
to ensure that it doesn't change while it's being read.
> + int retval;
> + retval = amradio_set_mute(radio, AMRADIO_STOP);
A good place to start would be with amradio_set_mute. In my opinion,
you should remove the locks from this function, add a BUG_ON for
!mutex_is_locked to the start of the function, and ensure all
functions calling it currently hold the lock.
> + if (retval < 0)
> + dev_warn(&intf->dev, "amradio_stop failed\n");
> +
> + /* After stopping radio status set to AMRADIO_STOP.
> + * If we want driver to start radio on resume
> + * we set status equal to AMRADIO_START.
> + * On resume we will check status and run radio if needed.
> + */
> +
> + mutex_lock(&radio->lock);
> + radio->status = AMRADIO_START;
> + mutex_unlock(&radio->lock);
> + }
>
> dev_info(&intf->dev, "going into suspend..\n");
>
> @@ -579,11 +591,24 @@
> static int usb_amradio_resume(struct usb_interface *intf)
> {
> struct amradio_device *radio = usb_get_intfdata(intf);
> - int retval;
>
> - retval = amradio_set_mute(radio, AMRADIO_START);
> - if (retval < 0)
> - dev_warn(&intf->dev, "amradio_start failed\n");
> + if (radio->status == AMRADIO_START) {
Same as above.
> + int retval;
> + retval = amradio_set_mute(radio, AMRADIO_START);
> + if (retval < 0)
> + dev_warn(&intf->dev, "amradio_start failed\n");
> + retval = amradio_setfreq(radio);
> + if (retval < 0)
> + dev_warn(&intf->dev,
> + "set frequency failed\n");
> +
> + if (radio->stereo) {
Same as above.
> + retval = amradio_set_stereo(radio, WANT_STEREO);
amradio_set_stereo should be refactored here as well.
> + if (retval < 0)
> + dev_warn(&intf->dev, "set stereo failed\n");
> + }
> +
> + }
>
> dev_info(&intf->dev, "coming out of suspend..\n");
>
>
>
> --
> Best regards, Klimov Alexey
>
If you properly address the locking in this driver, you should be able
to remove the dependency of this driver on the lock_kernel() and
unlock_kernel() constructs.
Regards,
David Ellingsworth
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] gspca: add missing .type field check in VIDIOC_G_PARM
2009/3/18 Németh Márton :
> From: Márton Németh
>
> The gspca webcam driver does not check the .type field of struct
> v4l2_streamparm.
> This field is an input parameter for the driver according to V4L2 API
> specification,
> revision 0.24 [1]. Add the missing check.
>
> The missing check was recognised by v4l-test 0.10 [2] together with
> gspca_sunplus driver
> and with "Trust 610 LCD pow...@m ZOOM" webcam. This patch was verified also
> with
> v4l-test 0.10.
>
> References:
> [1] V4L2 API specification, revision 0.24
> http://v4l2spec.bytesex.org/spec/r11680.htm
>
> [2] v4l-test: Test environment for Video For Linux Two API
> http://v4l-test.sourceforge.net/
>
> Signed-off-by: Márton Németh
> ---
> --- linux-2.6.29-rc8/drivers/media/video/gspca/gspca.c.orig 2009-03-14
> 12:29:38.0 +0100
> +++ linux-2.6.29-rc8/drivers/media/video/gspca/gspca.c 2009-03-18
> 16:51:03.0 +0100
> @@ -1320,6 +1320,9 @@ static int vidioc_g_parm(struct file *fi
> {
> struct gspca_dev *gspca_dev = priv;
>
> + if (parm->type != V4L2_BUF_TYPE_VIDEO_CAPTURE)
> + return -EINVAL;
> +
> memset(parm, 0, sizeof *parm);
> parm->type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
^^^
This line should be deleted as it's no longer needed.
> parm->parm.capture.readbuffers = gspca_dev->nbufread;
>
Regards,
David Ellingsworth
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: null pointer access in error path of lgdt3305 driver
On Thu, Mar 12, 2009 at 7:04 AM, Matthias Schwarzott wrote: > Hi Michael! > > Looking at your new lgdt3305 driver, I noticed that the error path of > lgdt3305_attach, that is also jumped to kzalloc errors, sets > state->frontend.demodulator_priv to NULL. > > That will oops in case state is NULL. So you either need two goto labels for > failures or just return in case kzalloc fails. Patches welcomed. :-) Regards, David Ellingsworth -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: POLL: for/against dropping support for kernels < 2.6.22
On Sun, Feb 22, 2009 at 5:15 AM, Hans Verkuil wrote: > Hi all, > > There are lot's of discussions, but it can be hard sometimes to actually > determine someone's opinion. > > So here is a quick poll, please reply either to the list or directly to me > with your yes/no answer and (optional but welcome) a short explanation to > your standpoint. It doesn't matter if you are a user or developer, I'd like > to see your opinion regardless. > > Please DO NOT reply to the replies, I'll summarize the results in a week's > time and then we can discuss it further. > > Should we drop support for kernels <2.6.22 in our v4l-dvb repository? > > _: Yes > _: No YES > > Optional question: Why can't we drop support for all but the latest kernel? > > Why: As others have already pointed out, it is a waste of time for developers who volunteer their time to work on supporting prior kernel revisions for use by enterprise distributions. The task of back-porting driver modifications to earlier kernels lessens the amount of time developers can focus on improving the quality and stability of new and existing drivers. Furthermore, it deters driver development since there an expectation that they will back-port their driver to earlier kernel versions. Finally, as a developer, I have have little interest in what was new yesterday. I usually run the latest kernel whenever possible and for a number of different reasons. Some of those reasons include better hardware support, bug detection, and stability testing. All services greatly valued by other kernel developers. Regards, David Ellingsworth -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
