Sorry, I still haven't done a proper review.
On Wed, Apr 15, 2015 at 01:18:41PM +0530, Sudip Mukherjee wrote:
> +struct pardevice *
> +parport_register_dev(struct parport *port, const char *name,
> + int (*pf)(void *), void (*kf)(void *),
> + void (*irq_func)(void *), int flags,
> + void *handle, struct parport_driver *drv)
> +{
> + struct pardevice *tmp;
"tmp" is inaccurate. It's not a tmp variable. Use par_dev or
something.
> + int ret;
> + char *devname;
> +
> + if (port->physport->flags & PARPORT_FLAG_EXCL) {
> + /* An exclusive device is registered. */
> + pr_debug("%s: no more devices allowed\n",
> + port->name);
> + return NULL;
> + }
> +
> + if (flags & PARPORT_DEV_LURK) {
> + if (!pf || !kf) {
> + pr_info("%s: refused to register lurking device (%s) without callbacks\n",
> + port->name, name);
> + return NULL;
> + }
> + }
> +
> + if (!try_module_get(port->ops->owner))
> + return NULL;
> +
> + parport_get_port(port);
> +
> + tmp = kzalloc(sizeof(*tmp), GFP_KERNEL);
> + if (!tmp) {
> + pr_warn("%s: memory squeeze, couldn't register %s.\n",
> + port->name, name);
Don't print warnings on kmalloc() failure.
> + goto out;
out is a bad label name. Use err_put_port or something descriptive.
> + }
> +
> + tmp->state = kmalloc(sizeof(*tmp->state), GFP_KERNEL);
I think kzalloc() is better here. That way if the ->init_state()
functions don't set it, then we know it's zeroed out.
> + if (!tmp->state) {
> + pr_warn("%s: memory squeeze, couldn't register %s.\n",
> + port->name, name);
> + goto out_free_pardevice;
> + }
> +
> + tmp->name = name;
I wonder who frees this name variable. My concern is that it gets
freed before we are done using it or something. (I have not looked at
the details).
> + tmp->port = port;
> + tmp->daisy = -1;
> + tmp->preempt = pf;
> + tmp->wakeup = kf;
> + tmp->private = handle;
handle sounds like a function pointer. It should be private_data.
> + tmp->flags = flags;
> + tmp->irq_func = irq_func;
> + tmp->waiting = 0;
> + tmp->timeout = 5 * HZ;
> +
> + tmp->dev.parent = &port->ddev;
> + devname = kstrdup(name, GFP_KERNEL);
kstrdup() can fail.
> + dev_set_name(&tmp->dev, "%s", name);
> + tmp->dev.driver = &drv->driver;
> + tmp->dev.release = free_pardevice;
> + tmp->devmodel = true;
> + ret = device_register(&tmp->dev);
> + if (ret)
> + goto out_free_all;
out_free_all is a bad label name. It should be free_state. Except the
most recently allocated resource is devname. It should be
goto free_devname. The beauty of labeling things this way is that you
only have to read back a few lines to see what is being freed.
> +
> + /* Chain this onto the list */
> + tmp->prev = NULL;
Not really needed because this was allocated with kzalloc(), of course.
But sometimes people like to say things twice for documentation so
that's also fine.
> + /*
> + * This function must not run from an irq handler so we don' t need
> + * to clear irq on the local CPU. -arca
> + */
> + spin_lock(&port->physport->pardevice_lock);
> +
> + if (flags & PARPORT_DEV_EXCL) {
> + if (port->physport->devices) {
> + spin_unlock(&port->physport->pardevice_lock);
> + pr_debug("%s: cannot grant exclusive access for device %s\n",
> + port->name, name);
> + goto out_free_dev;
goto err_put_dev;
> + }
> + port->flags |= PARPORT_FLAG_EXCL;
> + }
> +
> + tmp->next = port->physport->devices;
> + wmb(); /*
> + * Make sure that tmp->next is written before it's
> + * added to the list; see comments marked 'no locking
> + * required'
> + */
> + if (port->physport->devices)
> + port->physport->devices->prev = tmp;
> + port->physport->devices = tmp;
> + spin_unlock(&port->physport->pardevice_lock);
> +
> + init_waitqueue_head(&tmp->wait_q);
> + tmp->timeslice = parport_default_timeslice;
> + tmp->waitnext = NULL;
> + tmp->waitprev = NULL;
> +
> + /*
> + * This has to be run as last thing since init_state may need other
> + * pardevice fields. -arca
> + */
> + port->ops->init_state(tmp, tmp->state);
> + if (!test_and_set_bit(PARPORT_DEVPROC_REGISTERED, &port->devflags)) {
> + port->proc_device = tmp;
> + parport_device_proc_register(tmp);
> + }
I don't understand this test_and_set_bit() condition. It's weird to me
that parport_register_dev() succeeds even though we haven't called
parport_device_proc_register(tmp).
> +
> + return tmp;
Put a blank line here.
> +out_free_dev:
> + put_device(&tmp->dev);
> +out_free_all:
> + kfree(tmp->state);
> +out_free_pardevice:
> + kfree(tmp);
> +out:
> + parport_put_port(port);
> + module_put(port->ops->owner);
> +
> + return NULL;
> +}
> +
regards,
dan carpenter
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
