[PATCH 2/2] staging: goldfish: cleanup in goldfish_audio

[Dan Carpenter]

On Thu, Jun 28, 2018 at 06:19:14PM -0700, rkir at google.com wrote:
> From: Roman Kiryanov <rkir at google.com>
> 
> * added a mutex to protect open/read/write/release calls;
> * put the global atomic counter for opened files into the
>     driver state;
> * retired the global variable for the driver state;

These three should maybe be in the same patch?  It's not clear if they
are separate things or related.  The changelog should say why you are
doing it because it's not clear.

Right now the code only supports one open at a time.  It looks like
this is an effort to add support for multiply opens at the same time
but it's not finished yet?

> * retired the ioctl calls because it does not do much and cmd=315
>     looks obsolete.

This change is definitely unrelated.

>  static int goldfish_audio_open(struct inode *ip, struct file *fp)
>  {
> -	if (!audio_data)
> +	struct goldfish_audio *audio = fp->private_data;

I've never written a misc driver (or any substantial kernel code at all
if I'm being honest.  :P) but I think fp->private_data is a pointer to
&goldfish_audio_device.

But then that wouldn't ever work, and how was this tested then???


> +	int status;
> +
> +	if (!audio)
>  		return -ENODEV;
>  
> -	if (atomic_inc_return(&open_count) == 1) {
> -		fp->private_data = audio_data;
> -		audio_data->buffer_status = (AUDIO_INT_WRITE_BUFFER_1_EMPTY |
> -					     AUDIO_INT_WRITE_BUFFER_2_EMPTY);
> -		audio_write(audio_data, AUDIO_INT_ENABLE, AUDIO_INT_MASK);
> -		return 0;
> +	status = mutex_lock_interruptible(&audio->mutex);
> +	if (status)
> +		return status;
> +
> +	if (audio->open_count) {
> +		status = -EBUSY;
> +		goto done;
>  	}
>  
> -	atomic_dec(&open_count);
> -	return -EBUSY;
> +	++audio->open_count;
> +	audio->buffer_status = (AUDIO_INT_WRITE_BUFFER_1_EMPTY |
> +				AUDIO_INT_WRITE_BUFFER_2_EMPTY);
> +	audio_write(audio, AUDIO_INT_ENABLE, AUDIO_INT_MASK);
> +	fp->private_data = audio;

We set fp->private_data back to itself at the end of the function which
is obviously not right.

> +
> +done:
> +	mutex_unlock(&audio->mutex);
> +	return status;
>  }

regards,
dan carpenter