On Tue, 24 Mar 2015 11:31:55 +0100 Jiri Slaby <jsl...@suse.cz> 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
pgpvBC5WyiLJT.pgp
Description: OpenPGP digital signature
