Re: [PATCH 1/4] parport: modify parport subsystem to use devicemodel

[Dan Carpenter <dan.carpenter@oracle.com>]

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