| Message ID | 1249753564.15160.248.camel@tux.localhost (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Douglas Landgraf |
| Headers | show |
On Sat, Aug 8, 2009 at 1:46 PM, Alexey Klimov<klimov.linux@gmail.com> wrote: > Small cleanup of amradio_setfreq(). No need to pass radio->curfreq value > to this function. > > Signed-off-by: Alexey Klimov <klimov.linux@gmail.com> > > -- > 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 majordomo@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 majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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; /* 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");
Small cleanup of amradio_setfreq(). No need to pass radio->curfreq value to this function. Signed-off-by: Alexey Klimov <klimov.linux@gmail.com> --