Re: [PATCH 2/3] TTY: add support for tty_slave devices.

[Jiri Slaby <jslaby@suse.cz>]

Hi,

On 03/18/2015, 06:58 AM, NeilBrown wrote:
> --- /dev/null
> +++ b/drivers/tty/slave/tty_slave_core.c
> @@ -0,0 +1,136 @@

...

> +static int tty_slave_match(struct device *dev, struct device_driver *drv)
> +{
> +	return of_driver_match_device(dev, drv);
> +}
> +
> +static void tty_slave_release(struct device *dev)
> +{
> +	kfree(dev);

This should free the slave where the dev is contained. This is never
called IMO due to missing put_device's in the code.

> +}
> +
> +struct bus_type tty_slave_bus_type = {
> +	.name		= "tty-slave",
> +	.match		= tty_slave_match,
> +};
> +
> +int tty_slave_register(struct device *parent, struct device_node *node,
> +		       struct device *tty, struct tty_driver *drv)
> +{
> +	struct tty_slave *slave;
> +	int retval;
> +
> +	if (!of_get_property(node, "compatible", NULL))
> +		return -ENODEV;
> +
> +	slave = kzalloc(sizeof(*slave), GFP_KERNEL);
> +	if (!slave)
> +		return -ENOMEM;
> +

device_initialize();

> +	slave->dev.bus = &tty_slave_bus_type;
> +	slave->dev.parent = parent;
> +	slave->dev.release = tty_slave_release;
> +	slave->dev.of_node = of_node_get(node);
> +	dev_set_name(&slave->dev, "%s", node->name);
> +	slave->tty_dev = tty;
> +	slave->tty_drv = drv;
> +	slave->ops = *drv->ops;
> +	retval = device_register(&slave->dev);
> +	if (retval) {
> +		of_node_put(node);
> +		kfree(slave);

Do device_put() instead of the two. And do the two in .release.

> +	}
> +	return retval;
> +}
> +EXPORT_SYMBOL(tty_slave_register);
...
> --- a/drivers/tty/tty_io.c
> +++ b/drivers/tty/tty_io.c
...
> @@ -3205,6 +3208,29 @@ static void tty_device_create_release(struct device *dev)
>  	kfree(dev);
>  }
>  
> +int tty_register_finalize(struct tty_driver *driver, struct device *dev)
> +{
> +	int retval;
> +	bool cdev = false;
> +	int index = dev->devt - MKDEV(driver->major,
> +				      driver->minor_start);
> +	printk("REGISTER %d %d 0x%x %d\n", driver->major, driver->minor_start, dev->devt, index);
> +	if (!(driver->flags & TTY_DRIVER_DYNAMIC_ALLOC)) {
> +		retval = tty_cdev_add(driver,
> +				      dev->devt,
> +				      index, 1);
> +		if (retval)
> +			return retval;
> +		cdev = true;
> +	}
> +	retval = device_register(dev);
> +	if (retval == 0)
> +		return 0;
> +	if (cdev)
> +		cdev_del(&driver->cdevs[index]);
> +	return retval;
> +}
> +EXPORT_SYMBOL(tty_register_finalize);
>  /**
>   *	tty_register_device_attr - register a tty device
>   *	@driver: the tty driver that describes the tty device
> @@ -3234,7 +3260,8 @@ struct device *tty_register_device_attr(struct tty_driver *driver,
>  	dev_t devt = MKDEV(driver->major, driver->minor_start) + index;
>  	struct device *dev = NULL;
>  	int retval = -ENODEV;
> -	bool cdev = false;
> +	struct device_node *node;
> +	bool slave_registered = false;
>  
>  	if (index >= driver->num) {
>  		printk(KERN_ERR "Attempt to register invalid tty line number "
> @@ -3247,13 +3274,6 @@ struct device *tty_register_device_attr(struct tty_driver *driver,
>  	else
>  		tty_line_name(driver, index, name);
>  
> -	if (!(driver->flags & TTY_DRIVER_DYNAMIC_ALLOC)) {
> -		retval = tty_cdev_add(driver, devt, index, 1);
> -		if (retval)
> -			goto error;
> -		cdev = true;
> -	}
> -
>  	dev = kzalloc(sizeof(*dev), GFP_KERNEL);
>  	if (!dev) {
>  		retval = -ENOMEM;
> @@ -3268,16 +3288,24 @@ struct device *tty_register_device_attr(struct tty_driver *driver,
>  	dev->groups = attr_grp;
>  	dev_set_drvdata(dev, drvdata);
>  
> -	retval = device_register(dev);
> -	if (retval)
> -		goto error;
> +	if (device && device->of_node)
> +		for_each_available_child_of_node(device->of_node, node) {
> +			if (tty_slave_register(device, node, dev, driver) == 0)
> +				slave_registered = true;
> +			if (slave_registered)
> +				break;
> +		}
> +
> +	if (!slave_registered) {
> +		retval = tty_register_finalize(driver, dev);
> +		if (retval)
> +			goto error;
> +	}
>  
>  	return dev;

And what about ttys not using the tty_register_device* helpers?

What happens when the tty is unregistered?

> --- /dev/null
> +++ b/include/linux/tty_slave.h
> @@ -0,0 +1,26 @@
> +
> +struct tty_slave {
> +	struct device *tty_dev;
> +	struct tty_driver *tty_drv;
> +	struct tty_operations ops;
> +	struct device dev;
> +};
> +
> +int tty_slave_finalize(struct tty_slave *slave);
> +int tty_slave_driver_register(struct device_driver *drv);
> +#if config_enabled(CONFIG_TTY_SLAVE)

Since TTY_SLAVE is tristate, this will work only for TTY_SLAVE == y, not
m. Use IS_ENABLED instead.

> +void tty_slave_activate(struct tty_struct *tty);
> +int tty_slave_register(struct device *parent, struct device_node *node,
> +		       struct device *tty, struct tty_driver *drv);
> +#else
> +static inline void tty_slave_activate(struct tty_struct *tty)
> +{
> +}
> +static inline int tty_slave_register(struct device *parent,
> +				     struct device_node *node,
> +				     struct device *tty,
> +				     struct tty_driver *drv)
> +{
> +	return -ENODEV;
> +}
> +#endif

thanks,
-- 
js
suse labs