Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Subject: Re: [PATCH v2] staging: pi433: cleanup tx_fifo locking
To:     Valentin Vidic <Valentin.Vidic@CARNet.hr>
Cc:     =?UTF-8?Q?Simon_Sandstr=c3=b6m?= <simon@nikanor.nu>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        =?UTF-8?Q?Luca_S=c3=b6the?= <luca@acul.me>,
        Marcin Ciupak <marcin.s.ciupak@gmail.com>,
        Michael Panzlaff <michael.panzlaff@fau.de>,
        Derek Robson <robsonde@gmail.com>, devel@driverdev.osuosl.org,
        linux-kernel@vger.kernel.org
References: <4dba7594-2661-edd5-24ba-cffdf24021c5@smarthome-wolf.de>
 <20180419102530.5927-1-Valentin.Vidic@CARNet.hr>
From:   Marcus Wolf <marcus.wolf@smarthome-wolf.de>
Openpgp: preference=signencrypt
Message-ID: <20b592c2-33b1-f2f6-875e-39402e5ab058@smarthome-wolf.de>
Date:   Thu, 19 Apr 2018 12:40:14 +0200
User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:52.0) Gecko/20100101
 Thunderbird/52.7.0
MIME-Version: 1.0
In-Reply-To: <20180419102530.5927-1-Valentin.Vidic@CARNet.hr>
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Reviewed-by: Marcus Wolf <marcus.wolf@wolf-entwicklungen.de>

Am 19.04.2018 um 12:25 schrieb Valentin Vidic:
> pi433_write requires locking due to multiple writers.  After acquiring
> the lock check if enough free space is available in the kfifo to write
> the whole message. This check should prevent partial writes to tx_fifo
> so kfifo_reset is not needed anymore.
> 
> pi433_tx_thread is the only reader so it does not require locking
> after kfifo_reset is removed.
> 
> Signed-off-by: Valentin Vidic <Valentin.Vidic@CARNet.hr>
> ---
> v2: print a warning if partial fifo write happens
> 
>  drivers/staging/pi433/pi433_if.c | 23 ++++++++++++++---------
>  1 file changed, 14 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/staging/pi433/pi433_if.c b/drivers/staging/pi433/pi433_if.c
> index d1e0ddbc79ce..2a05eff88469 100644
> --- a/drivers/staging/pi433/pi433_if.c
> +++ b/drivers/staging/pi433/pi433_if.c
> @@ -87,7 +87,7 @@ struct pi433_device {
>  
>  	/* tx related values */
>  	STRUCT_KFIFO_REC_1(MSG_FIFO_SIZE) tx_fifo;
> -	struct mutex		tx_fifo_lock; // TODO: check, whether necessary or obsolete
> +	struct mutex		tx_fifo_lock; /* serialize userspace writers */
>  	struct task_struct	*tx_task_struct;
>  	wait_queue_head_t	tx_wait_queue;
>  	u8			free_in_fifo;
> @@ -589,19 +589,15 @@ pi433_tx_thread(void *data)
>  		 * - size of message
>  		 * - message
>  		 */
> -		mutex_lock(&device->tx_fifo_lock);
> -
>  		retval = kfifo_out(&device->tx_fifo, &tx_cfg, sizeof(tx_cfg));
>  		if (retval != sizeof(tx_cfg)) {
>  			dev_dbg(device->dev, "reading tx_cfg from fifo failed: got %d byte(s), expected %d", retval, (unsigned int)sizeof(tx_cfg));
> -			mutex_unlock(&device->tx_fifo_lock);
>  			continue;
>  		}
>  
>  		retval = kfifo_out(&device->tx_fifo, &size, sizeof(size_t));
>  		if (retval != sizeof(size_t)) {
>  			dev_dbg(device->dev, "reading msg size from fifo failed: got %d, expected %d", retval, (unsigned int)sizeof(size_t));
> -			mutex_unlock(&device->tx_fifo_lock);
>  			continue;
>  		}
>  
> @@ -634,7 +630,6 @@ pi433_tx_thread(void *data)
>  				   sizeof(device->buffer) - position);
>  		dev_dbg(device->dev,
>  			"read %d message byte(s) from fifo queue.", retval);
> -		mutex_unlock(&device->tx_fifo_lock);
>  
>  		/* if rx is active, we need to interrupt the waiting for
>  		 * incoming telegrams, to be able to send something.
> @@ -818,7 +813,7 @@ pi433_write(struct file *filp, const char __user *buf,
>  	struct pi433_instance	*instance;
>  	struct pi433_device	*device;
>  	int                     retval;
> -	unsigned int		copied;
> +	unsigned int		required, available, copied;
>  
>  	instance = filp->private_data;
>  	device = instance->device;
> @@ -833,6 +828,16 @@ pi433_write(struct file *filp, const char __user *buf,
>  	 * - message
>  	 */
>  	mutex_lock(&device->tx_fifo_lock);
> +
> +	required = sizeof(instance->tx_cfg) + sizeof(size_t) + count;
> +	available = kfifo_avail(&device->tx_fifo);
> +	if (required > available) {
> +		dev_dbg(device->dev, "write to fifo failed: %d bytes required but %d available",
> +			required, available);
> +		mutex_unlock(&device->tx_fifo_lock);
> +		return -EAGAIN;
> +	}
> +
>  	retval = kfifo_in(&device->tx_fifo, &instance->tx_cfg,
>  			  sizeof(instance->tx_cfg));
>  	if (retval != sizeof(instance->tx_cfg))
> @@ -855,8 +860,8 @@ pi433_write(struct file *filp, const char __user *buf,
>  	return copied;
>  
>  abort:
> -	dev_dbg(device->dev, "write to fifo failed: 0x%x", retval);
> -	kfifo_reset(&device->tx_fifo); // TODO: maybe find a solution, not to discard already stored, valid entries
> +	dev_warn(device->dev,
> +		 "write to fifo failed, non recoverable: 0x%x", retval);
>  	mutex_unlock(&device->tx_fifo_lock);
>  	return -EAGAIN;
>  }
>