Re: [PATCH] um: Use tty_port

[Richard Weinberger <richard@nod.at>]

[-- Attachment #1: Type: text/plain, Size: 3330 bytes --]

Am 12.02.2012 14:01, schrieb Jiri Slaby:
> This and the ioctl change above fullfils the subject of the patch in no
> way. Do this fix and the cleanup above separately, please.

Fair point.

> 
>>> @@ -404,27 +334,27 @@ int line_setup_irq(int fd, int input, int output, struct line *line, void *data)
>>>   * first open or last close.  Otherwise, open and close just return.
>>>   */
>>>  
>>> -int line_open(struct line *lines, struct tty_struct *tty)
>>> +int line_open(struct tty_struct *tty, struct file *filp)
>>>  {
>>> -	struct line *line = &lines[tty->index];
>>> -	int err = -ENODEV;
>>> +	struct line *line = tty->driver_data;
>>> +	return tty_port_open(&line->port, tty, filp);
>>> +}
>>>  
>>> -	spin_lock(&line->count_lock);
>>> -	if (!line->valid)
>>> -		goto out_unlock;
>>> +int line_install(struct tty_driver *driver, struct tty_struct *tty, struct line *line)
> 
> As it stands, it should be tty_port->ops->activate, not
> tty->ops->install. Yes, you can still set driver_data and check for
> multiple opens in install. Then use also tty_standard_install.

Okay.

>>> +{
>>> +	int ret = tty_init_termios(tty);
>>>  
>>> -	err = 0;
>>> -	if (line->count++)
>>> -		goto out_unlock;
>>> +	if (ret)
>>> +		return ret;
>>>  
>>> -	BUG_ON(tty->driver_data);
>>> +	tty_driver_kref_get(driver);
>>> +	tty->count++;
>>>  	tty->driver_data = line;
>>> -	line->tty = tty;
>>> +	driver->ttys[tty->index] = tty;
>>>  
>>> -	spin_unlock(&line->count_lock);
>>> -	err = enable_chan(line);
>>> -	if (err) /* line_close() will be called by our caller */
>>> -		return err;
>>> +	ret = enable_chan(line);
>>> +	if (ret)
>>> +		return ret;
>>>  
>>>  	INIT_DELAYED_WORK(&line->task, line_timer_cb);
>>>  
>>> @@ -437,48 +367,36 @@ int line_open(struct line *lines, struct tty_struct *tty)
>>>  			 &tty->winsize.ws_col);
>>>  
>>>  	return 0;
>>> -
>>> -out_unlock:
>>> -	spin_unlock(&line->count_lock);
>>> -	return err;
>>>  }
> ...
>>> +void line_close(struct tty_struct *tty, struct file * filp)
>>> +{
>>> +	struct line *line = tty->driver_data;
>>> +
>>> +	if (!line)
>>> +		return;
> 
> Unless you set tty->driver_data to NULL somewhere, this can never
> happen. If you do, why -- this tends to be racy?

The old driver set tty->driver_data to NULL.
I guess we can remove it.

>>> +	tty_port_close(&line->port, tty, filp);
>>> +}
> ...
>>> --- a/arch/um/drivers/ssl.c
>>> +++ b/arch/um/drivers/ssl.c
>>> @@ -92,6 +92,7 @@ static int ssl_remove(int n, char **error_out)
>>>  			   error_out);
>>>  }
>>>  
>>> +#if 0
> 
> Hmm, remove unused code instead.

Will do.

>>>  static int ssl_open(struct tty_struct *tty, struct file *filp)
>>>  {
>>>  	int err = line_open(serial_lines, tty);
> ...
>>> @@ -124,8 +124,16 @@ void ssl_hangup(struct tty_struct *tty)
>>>  }
>>>  #endif
>>>  
>>> +static int ssl_install(struct tty_driver *driver, struct tty_struct *tty)
>>> +{
>>> +	if (tty->index < NR_PORTS)
> 
> Do you register more than NR_PORTS when allocating tty_driver? If not,
> this test is always true. But presumably you won't need this hook anyway.

Okay.

>>> +		return line_install(driver, tty, &serial_lines[tty->index]);
>>> +	else
>>> +		return -ENODEV;
>>> +}
> 
> thanks,

Thanks,
//richard


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 490 bytes --]