On 30/04/09 21:18, Andrew Morton wrote:
> On Thu, 23 Apr 2009 18:32:13 +0100
> Simon Arlott <si...@fire.lp0.eu> wrote:
>
>> dvb_dvr_read may unlock the dmxdev mutex and return -ENODEV,
>> except this function is a file op and will never be called
>> with the mutex held.
>>
>> There's existing mutex_lock and mutex_unlock around the actual
>> read but it's commented out. These should probably be uncommented
>> but the read blocks and this could block another non-blocking
>> reader on the mutex instead.
>>
>> This change comments out the extra mutex_unlock.
>>
>> Signed-off-by: Simon Arlott <si...@fire.lp0.eu>
>> ---
>> This has been on my TODO list for far too long... I did come
>> up with a mutex_trylock/mutex_lock_interruptible version but
>> claiming that it'll block when it may not doesn't make sense
>> (and any blocking read would cause all non-blocking reads to
>> continually return -EWOULDBLOCK until there is data).
>>
>> drivers/media/dvb/dvb-core/dmxdev.c | 2 +-
>> 1 files changed, 1 insertions(+), 1 deletions(-)
>>
>> diff --git a/drivers/media/dvb/dvb-core/dmxdev.c
>> b/drivers/media/dvb/dvb-core/dmxdev.c
>> index c35fbb8..d6d098a 100644
>> --- a/drivers/media/dvb/dvb-core/dmxdev.c
>> +++ b/drivers/media/dvb/dvb-core/dmxdev.c
>> @@ -247,7 +247,7 @@ static ssize_t dvb_dvr_read(struct file *file, char
>> __user *buf, size_t count,
>> int ret;
>>
>> if (dmxdev->exit) {
>> - mutex_unlock(&dmxdev->mutex);
>> + //mutex_unlock(&dmxdev->mutex);
>> return -ENODEV;
>> }
>
> Is there any value in retaining all the commented-out lock operations,
> or can we zap 'em?
I'm assuming they should really be there - it's just not practical
because the call to dvb_dmxdev_buffer_read is likely to block waiting
for data.
> ---
> a/drivers/media/dvb/dvb-core/dmxdev.c~dvb-core-fix-potential-mutex_unlock-without-mutex_lock-in-dvb_dvr_read-fix
> +++ a/drivers/media/dvb/dvb-core/dmxdev.c
> @@ -244,19 +244,13 @@ static ssize_t dvb_dvr_read(struct file
> {
> struct dvb_device *dvbdev = file->private_data;
> struct dmxdev *dmxdev = dvbdev->priv;
> - int ret;
>
> - if (dmxdev->exit) {
> - //mutex_unlock(&dmxdev->mutex);
> + if (dmxdev->exit)
> return -ENODEV;
> - }
>
> - //mutex_lock(&dmxdev->mutex);
> - ret = dvb_dmxdev_buffer_read(&dmxdev->dvr_buffer,
> - file->f_flags & O_NONBLOCK,
> - buf, count, ppos);
> - //mutex_unlock(&dmxdev->mutex);
> - return ret;
> + return dvb_dmxdev_buffer_read(&dmxdev->dvr_buffer,
> + file->f_flags & O_NONBLOCK,
> + buf, count, ppos);
> }
>
> static int dvb_dvr_set_buffer_size(struct dmxdev *dmxdev,
> _
>
--
Simon Arlott
--
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
