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

[NeilBrown]

On Tue, 24 Mar 2015 11:31:55 +0100 Jiri Slaby <jslaby@xxxxxxx> wrote:

> 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.

Yes of course, thanks.  I've fix the 'kfree'.


> 
> > +}
> > +
> > +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();

device_initialize is only needed if you call device_add()
I use device_register() which calls both.


> 
> > +	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.

Done, thanks.

> 
> > +	}
> > +	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?

I guess they are on their own - they don't get any help...
Is there a problem that I'm not seeing here?

I think this only applies to code that calls device_create(tty_class, ...)
which is pty.c, vt.c, and "/dev/tty".  These are all virtual devices so
having a slave doesn't make much sense... does it?

> 
> What happens when the tty is unregistered?

Good question.  I hadn't thought that through.

When the tty is unregistered, it will drop the reference it has on ->parent
which the tty_slave.  So we want that to be the last reference.
So in tty_slave_finalize() I need a put_dev() on the slave.
That must be the "put_dev" that you thought was missing earlier.

However that leaves a loose end. destruct_tty_driver() calls
tty_unregister_device() on all registered ttys for that driver.
If one had a tty_slave which was finalized, that will drop the reference on
the slave so it will disappear, which is all good.
But if one has a slave for which the driver hasn't been found yet, then there
will be no tty to unregister so the tty_slave cannot be dropped.

I guess I need to either take a reference to the driver - which doesn't seem
like a good idea - or use bus_find_device() to find any tty_slaves with that
driver, and drop them.
I'll see how that works out.


Thanks a lot!

NeilBrown

Attachment: pgpo6FpAqErLH.pgp
Description: OpenPGP digital signature