Re: [PATCH] tty/n_hdlc: fix sleep in !TASK_RUNNING state warning

[Jiri Slaby <jslaby@suse.cz>]

On 02. 01. 19, 16:04, Tetsuo Handa wrote:
> +	if (wait_event_interruptible(tty->read_wait,
> +	     (ret = -EIO, test_bit(TTY_OTHER_CLOSED, &tty->flags)) ||
> +	     (ret = 0, tty_hung_up_p(file)) ||
> +	     (rbuf = n_hdlc_buf_get(&n_hdlc->rx_buf_list)) != NULL ||
> +	     (ret = -EAGAIN, tty_io_nonblock(tty, file))))
> +		return -EINTR;

Oh, that looks really ugly. Could you move all this to a function
returning a bool and taking &ret and &rbuf as parameters?

> +	if (rbuf) {
> +		if (rbuf->count > nr)
> +			/* too large for caller's buffer */
> +			ret = -EOVERFLOW;
> +		else if (copy_to_user(buf, rbuf->buf, rbuf->count))
> +			ret = -EFAULT;
> +		else
> +			ret = rbuf->count;
> +		if (n_hdlc->rx_free_buf_list.count > DEFAULT_RX_BUF_COUNT)
> +			kfree(rbuf);
> +		else
> +			n_hdlc_buf_put(&n_hdlc->rx_free_buf_list, rbuf);
>  	}
> -
> -	remove_wait_queue(&tty->read_wait, &wait);
> -	__set_current_state(TASK_RUNNING);
> -
>  	return ret;
> -	
>  }	/* end of n_hdlc_tty_read() */
>  
>  /**
> @@ -645,21 +612,13 @@ static ssize_t n_hdlc_tty_read(struct tty_struct *tty, struct file *file,
>  static ssize_t n_hdlc_tty_write(struct tty_struct *tty, struct file *file,
>  			    const unsigned char *data, size_t count)
>  {
> -	struct n_hdlc *n_hdlc = tty2n_hdlc (tty);
> +	struct n_hdlc *n_hdlc;
>  	int error = 0;
> -	DECLARE_WAITQUEUE(wait, current);
> -	struct n_hdlc_buf *tbuf;
> +	struct n_hdlc_buf *tbuf = NULL;
>  
>  	if (debuglevel >= DEBUG_LEVEL_INFO)	
>  		printk("%s(%d)n_hdlc_tty_write() called count=%zd\n",
>  			__FILE__,__LINE__,count);
> -		
> -	/* Verify pointers */
> -	if (!n_hdlc)
> -		return -EIO;
> -
> -	if (n_hdlc->magic != HDLC_MAGIC)
> -		return -EIO;
>  
>  	/* verify frame size */
>  	if (count > maxframe ) {
> @@ -670,40 +629,14 @@ static ssize_t n_hdlc_tty_write(struct tty_struct *tty, struct file *file,
>  				maxframe );
>  		count = maxframe;
>  	}
> -	
> -	add_wait_queue(&tty->write_wait, &wait);
>  
> -	for (;;) {
> -		set_current_state(TASK_INTERRUPTIBLE);
> -	
> -		tbuf = n_hdlc_buf_get(&n_hdlc->tx_free_buf_list);
> -		if (tbuf)
> -			break;
> -
> -		if (tty_io_nonblock(tty, file)) {
> -			error = -EAGAIN;
> -			break;
> -		}
> -		schedule();
> -			
> -		n_hdlc = tty2n_hdlc (tty);
> -		if (!n_hdlc || n_hdlc->magic != HDLC_MAGIC || 
> -		    tty != n_hdlc->tty) {
> -			printk("n_hdlc_tty_write: %p invalid after wait!\n", n_hdlc);
> -			error = -EIO;
> -			break;
> -		}
> -			
> -		if (signal_pending(current)) {
> -			error = -EINTR;
> -			break;
> -		}
> -	}
> -
> -	__set_current_state(TASK_RUNNING);
> -	remove_wait_queue(&tty->write_wait, &wait);
> -
> -	if (!error) {		
> +	if (wait_event_interruptible(tty->write_wait,
> +	     (error = -EIO, n_hdlc = tty2n_hdlc(tty), /* Verify pointers */
> +	      !n_hdlc || n_hdlc->magic != HDLC_MAGIC || tty != n_hdlc->tty) ||
> +	     (tbuf = n_hdlc_buf_get(&n_hdlc->tx_free_buf_list)) != NULL ||
> +	     (error = -EAGAIN, tty_io_nonblock(tty, file))))
> +		return -EINTR;

This is even worse. So detto as above?

> +	if (tbuf) {
>  		/* Retrieve the user's buffer */
>  		memcpy(tbuf->buf, data, count);
>  
> @@ -711,8 +644,9 @@ static ssize_t n_hdlc_tty_write(struct tty_struct *tty, struct file *file,
>  		tbuf->count = error = count;
>  		n_hdlc_buf_put(&n_hdlc->tx_buf_list,tbuf);
>  		n_hdlc_send_frames(n_hdlc,tty);
> +	} else if (error == -EIO) {
> +		printk("n_hdlc_tty_write: %p invalid!\n", n_hdlc);
>  	}
> -
>  	return error;
>  	
>  }	/* end of n_hdlc_tty_write() */
> 

thanks,
-- 
js
suse labs