Message-ID: <20151026062018.GE7289@mwanda>
Date:	Mon, 26 Oct 2015 09:20:18 +0300
From:	Dan Carpenter <dan.carpenter@...cle.com>
To:	Lijun Pan <Lijun.Pan@...escale.com>
Cc:	gregkh@...uxfoundation.org, arnd@...db.de,
	devel@...verdev.osuosl.org, linux-kernel@...r.kernel.org,
	stuart.yoder@...escale.com, itai.katz@...escale.com,
	german.rivera@...escale.com, leoli@...escale.com,
	scottwood@...escale.com, agraf@...e.de, bhamciu1@...escale.com,
	R89243@...escale.com, bhupesh.sharma@...escale.com,
	nir.erez@...escale.com, richard.schmitt@...escale.com
Subject: Re: [PATCH 5/5] staging: fsl-mc: Management Complex restool driver

A few style issues and error handling bugs.  See below.

On Sun, Oct 25, 2015 at 05:41:23PM -0500, Lijun Pan wrote:
> +static int fsl_mc_restool_dev_open(struct inode *inode, struct file *filep)
> +{
> +	struct fsl_mc_device *root_mc_dev;
> +	int error = 0;
> +	struct fsl_mc_io *dynamic_mc_io = NULL;
> +	struct restool_misc *restool_misc;
> +	struct restool_misc *restool_misc_cursor;
> +
> +	pr_debug("%s: inode's dev_t == %u\n", __func__, inode->i_rdev);
> +
> +	list_for_each_entry(restool_misc_cursor, &misc_list, list) {
> +		if (restool_misc_cursor->miscdevt == inode->i_rdev) {
> +			pr_debug("%s: Found the restool_misc\n", __func__);
> +			restool_misc = restool_misc_cursor;
> +			break;
> +		}
> +	}
> +
> +	if (!restool_misc)
> +		return -EINVAL;
> +
> +	if (WARN_ON(restool_misc->dev == NULL))
> +		return -EINVAL;
> +
> +	mutex_lock(&restool_misc->mutex);
> +
> +	if (!restool_misc->static_instance_in_use) {
> +		restool_misc->static_instance_in_use = true;
> +		filep->private_data = restool_misc->static_mc_io;
> +	} else {
> +		dynamic_mc_io = kzalloc(sizeof(struct fsl_mc_io), GFP_KERNEL);
> +		if (dynamic_mc_io == NULL) {
> +			error = -ENOMEM;
> +			goto error;
> +		}
> +
> +		root_mc_dev = to_fsl_mc_device(restool_misc->dev);
> +		error = fsl_mc_portal_allocate(root_mc_dev, 0, &dynamic_mc_io);
> +		if (error < 0) {
> +			pr_err("Not able to allocate MC portal\n");
> +			goto error;
> +		}
> +		++restool_misc->dynamic_instance_count;
> +		filep->private_data = dynamic_mc_io;
> +	}
> +
> +	mutex_unlock(&restool_misc->mutex);
> +
> +	return 0;
> +error:
> +	if (dynamic_mc_io != NULL) {
> +		fsl_mc_portal_free(dynamic_mc_io);
> +		kfree(dynamic_mc_io);
> +	}
> +
> +	mutex_unlock(&restool_misc->mutex);
> +
> +	return error;
> +}

Don't do One Err style error handling where there is only one error
label.  It is bug prone.  In this example if we call
fsl_mc_portal_free() when fsl_mc_portal_allocate() failed then it can
trigger a WARN_ON().  The handling.fsl_mc_portal_allocate() function has
some very complicated error handling so this probably causes a crash but
I am too last to untangle it...  Anyway do it like this:

	if (WARN_ON(restool_misc->dev == NULL))
		return -EINVAL;

	mutex_lock(&restool_misc->mutex);

	if (!restool_misc->static_instance_in_use) {
		restool_misc->static_instance_in_use = true;
		filep->private_data = restool_misc->static_mc_io;
	} else {
		dynamic_mc_io = kzalloc(sizeof(struct fsl_mc_io), GFP_KERNEL);
		if (dynamic_mc_io == NULL) {
			error = -ENOMEM;
			goto err_unlock;
		}

		root_mc_dev = to_fsl_mc_device(restool_misc->dev);
		error = fsl_mc_portal_allocate(root_mc_dev, 0, &dynamic_mc_io);
		if (error < 0) {
			pr_err("Not able to allocate MC portal\n");
			goto free_dynamic_mc_io;
		}
		++restool_misc->dynamic_instance_count;
		filep->private_data = dynamic_mc_io;
	}

	mutex_unlock(&restool_misc->mutex);

	return 0;

free_dynamic_mc_io:
	kfree(dynamic_mc_io);
err_unlock:
	mutex_unlock(&restool_misc->mutex);

	return error;
}

When you write it like this then 1) we don't free anything that we have
not allocated.  2)  There are no if statements or indenting so it is
simple.  The reason One Err style error handling is bug prone is that
when you combine all the error paths and try to handle them all at the
same time it's more complicated than doing one thing at a time.

> +static int restool_send_mc_command(unsigned long arg,
> +				struct fsl_mc_io *local_mc_io)
> +{
> +	int error = -EINVAL;

No need.

> +	struct mc_command mc_cmd;
> +
> +	error = copy_from_user(&mc_cmd, (void __user *)arg, sizeof(mc_cmd));
> +	if (error < 0) {
> +		pr_err("copy_to_user() failed with error %d\n", error);
> +		goto error;
> +	}

copy_from_user() returns the number of bytes NOT copied, it doesn't
return a negative number.  Don't print an error if the copy fails
because users can trigger it and fill /var/log/messages.  Do it like
this:

	if (copy_from_user(&mc_cmd, (void __user *)arg, sizeof(mc_cmd)))
		return -EFAULT;

> +
> +	/*
> +	 * Send MC command to the MC:
> +	 */
> +	error = mc_send_command(local_mc_io, &mc_cmd);
> +	if (error < 0)
> +		goto error;
> +
> +	error = copy_to_user((void __user *)arg, &mc_cmd, sizeof(mc_cmd));
> +	if (error < 0) {
> +		pr_err("copy_to_user() failed with error %d\n", error);
> +		goto error;
> +	}

Same.

> +
> +	return 0;
> +error:
> +	return error;

Ugh..  Don't do do-nothing gotos.  It's just a waste of time and does
not prevent future bugs.  I have examined this, by looking through git
history at locking bugs.

> +}
> +
> +static long
> +fsl_mc_restool_dev_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
> +{
> +	int error = 0;

No need.  Initializing variables to bogus values silences the GCC
warning about uninitialized variables and has caused some bugs.

> +
> +	switch (cmd) {
> +	case RESTOOL_DPRC_SYNC:
> +		pr_debug("syncing...\n");
> +		error = restool_dprc_sync(file->f_inode);
> +		pr_debug("syncing finished...\n");
> +		break;
> +	case RESTOOL_SEND_MC_COMMAND:
> +		error = restool_send_mc_command(arg, file->private_data);
> +		break;
> +	default:
> +		pr_err("%s: unexpected ioctl call number\n", __func__);
> +		error = -EINVAL;
> +	}
> +
> +	return error;
> +}
> +
> +static const struct file_operations fsl_mc_restool_dev_fops = {
> +	.owner = THIS_MODULE,
> +	.open = fsl_mc_restool_dev_open,
> +	.release = fsl_mc_restool_dev_release,
> +	.unlocked_ioctl = fsl_mc_restool_dev_ioctl,
> +};
> +
> +static int restool_add_device_file(struct device *dev)
> +{
> +	uint32_t name1 = 0;
> +	char name2[20] = {0};
> +	int error = 0;
> +	struct fsl_mc_device *root_mc_dev;
> +	struct restool_misc *restool_misc;
> +
> +	pr_debug("newly scanned/notified device: %s, whose parent:%s\n",
> +		 dev_name(dev), dev_name(dev->parent));
> +
> +	if (dev->bus == &platform_bus_type && dev->driver_data) {
> +		if (sscanf(dev_name(dev), "%x.%s", &name1, name2) != 2) {
> +			pr_err("sscanf failure\n");

Remove this printk.

> +			return -EINVAL;
> +		}
> +		if (strcmp(name2, "fsl-mc") == 0)
> +			pr_debug("platform's root dprc name is: %s\n",
> +			dev_name(&(((struct fsl_mc *)(dev->driver_data))
> +			->root_mc_bus_dev->dev)));

The indenting here is nasty.

> +	}
> +
> +	if (dev->bus == &fsl_mc_bus_type)
> +		pr_debug("%s's bus type: fsl_mc_bus_type\n", dev_name(dev));
> +	else if (dev->bus == &platform_bus_type)
> +		pr_debug("%s's bus type: platform_bus_type\n", dev_name(dev));
> +	else
> +		pr_debug("%s's bus type: NEITHER fsl_mc_bus_type NOR platform_bus_type\n",
> +			 dev_name(dev));
> +
> +	if (is_root_dprc(dev)) {

Flip this condition around and pull everything in one indent level.

> +		pr_debug("I am root dprc, create /dev/%s\n", dev_name(dev));
> +		restool_misc = kzalloc(sizeof(struct restool_misc), GFP_KERNEL);
> +		if (restool_misc == NULL)
> +			return -ENOMEM;
> +
> +		restool_misc->dev = dev;
> +		root_mc_dev = to_fsl_mc_device(dev);
> +		error = fsl_mc_portal_allocate(root_mc_dev, 0,
> +				&restool_misc->static_mc_io);
> +		if (error < 0) {
> +			pr_err("Not able to allocate MC portal\n");
> +			goto err_portal;

Label the names after what the label does not the goto location.  I'm
here at the goto location so I already know that the portal allocation
failed, I want to know what the goto will do.

If fsl_mc_portal_allocate() fails, it cleans up after itself.  This code
is buggy.  Just do:  goto free_restool_misc;

> +		}
> +
> +		restool_misc->misc.minor = MISC_DYNAMIC_MINOR;
> +		restool_misc->misc.name = dev_name(dev);
> +		restool_misc->misc.fops = &fsl_mc_restool_dev_fops;
> +
> +		error = misc_register(&restool_misc->misc);
> +		if (error < 0) {
> +			pr_err("misc_register() failed: %d\n", error);
> +			goto err_reg;

This should be goto free_portal.  If misc_register() fails, then we
do not need to call misc_deregister().  It is a bugy.

> +		}
> +
> +		restool_misc->miscdevt = restool_misc->misc.this_device->devt;
> +		mutex_init(&restool_misc->mutex);
> +		list_add(&restool_misc->list, &misc_list);
> +		pr_info("/dev/%s driver registered\n", dev_name(dev));
> +	} else
> +		pr_info("%s is not root dprc, miscdevice cannot be created/associated\n",
> +			dev_name(dev));
> +
> +	return 0;
> +err_reg:
> +	misc_deregister(&restool_misc->misc);
> +
> +err_portal:
> +	if (restool_misc->static_mc_io)
> +		fsl_mc_portal_free(restool_misc->static_mc_io);
> +	if (restool_misc)
> +		kfree(restool_misc);
> +
> +	return error;
> +}
> +
> +static int restool_bus_notifier(struct notifier_block *nb,
> +			      unsigned long action, void *data)
> +{
> +	struct device *dev = data;
> +
> +	pr_debug("entering %s...\n", __func__);

Too much debug code.  Also this can be done with ftrace.

> +	pr_debug("being notified by device: %s\n", dev_name(dev));
> +
> +	if (dev->bus == &fsl_mc_bus_type)
> +		pr_debug("%s's bus type: fsl_mc_bus_type\n", dev_name(dev));
> +	else if (dev->bus == &platform_bus_type)
> +		pr_debug("%s's bus type: platform_bus_type\n", dev_name(dev));
> +	else
> +		pr_debug("%s's bus type: NEITHER fsl_mc_bus_type NOR platform_bus_type\n",
> +			 dev_name(dev));
> +
> +	switch (action) {
> +	case BUS_NOTIFY_ADD_DEVICE:
> +		pr_info("bus notify device added: %s\n", dev_name(dev));
> +		restool_add_device_file(dev);

Add error handling.

> +		break;
> +	case BUS_NOTIFY_DEL_DEVICE:
> +		pr_info("bus notify device to be removed: %s\n", dev_name(dev));
> +		break;
> +	case BUS_NOTIFY_REMOVED_DEVICE:
> +		pr_info("bus notify device removed: %s\n", dev_name(dev));
> +		break;
> +	case BUS_NOTIFY_BIND_DRIVER:
> +		pr_info("bus notify driver about to be bound to device: %s\n", dev_name(dev));
> +		break;
> +	case BUS_NOTIFY_BOUND_DRIVER:
> +		pr_info("bus notify driver bound to device: %s\n", dev_name(dev));
> +		break;
> +	case BUS_NOTIFY_UNBIND_DRIVER:
> +		pr_info("bus notify driver about to unbind from device: %s\n", dev_name(dev));
> +		break;
> +	case BUS_NOTIFY_UNBOUND_DRIVER:
> +		pr_info("bus notify driver unbind from device: %s\n", dev_name(dev));
> +		break;
> +	default:
> +		pr_err("%s: unrecognized device action from %s\n", __func__, dev_name(dev));
> +		return -EINVAL;
> +	}
> +
> +	pr_debug("leaving %s...\n", __func__);

The whole rest of this function is just debugging stubs...  Does this
driver work yet?

> +
> +	return 0;
> +}
> +
> +static int add_to_restool(struct device *dev, void *data)
> +{
> +	pr_debug("verify *data: %s\n", (char *)data);
> +	restool_add_device_file(dev);
> +	return 0;
> +}
> +
> +static int __init fsl_mc_restool_driver_init(void)
> +{
> +	int error = 0;
> +	struct notifier_block *nb;
> +	char *data = "Add me to device file if I am a root dprc";
> +
> +	nb = kzalloc(sizeof(struct notifier_block), GFP_KERNEL);
> +	if (!nb)
> +		return -ENOMEM;
> +
> +	nb->notifier_call = restool_bus_notifier;
> +	pr_debug("restool will register notifier...\n");
> +	error = bus_register_notifier(&fsl_mc_bus_type, nb);
> +	pr_debug("restool finish register notifier...\n");
> +
> +	if (error) {
> +		kfree(nb);
> +		return error;
> +	}

The debug printks are so many that they obscure the code.  Put the error
hanling next to the function call.  Use a goto for error handling.

	nb->notifier_call = restool_bus_notifier;
	error = bus_register_notifier(&fsl_mc_bus_type, nb);
	if (error)
		goto free_nb;



> +
> +	pr_debug("restool scan bus for each device...\n");
> +	/*
> +	 * This driver runs after fsl-mc bus driver runs.
> +	 * Hence, many of the root dprcs are already attached to fsl-mc bus
> +	 * In order to make sure we find all the root dprcs,
> +	 * we need to scan the fsl_mc_bus_type.
> +	 */
> +	error  = bus_for_each_dev(&fsl_mc_bus_type, NULL, data, add_to_restool);

The add_to_restool() function ignores errors, but here we are saying
it can fail.  Probably, lets fix add_to_restool().

> +	if (error) {
> +		bus_unregister_notifier(&fsl_mc_bus_type, nb);
> +		kfree(nb);
> +		pr_err("restool driver registration failure\n");
> +		return error;
> +	}
> +	pr_debug("end restool scan bus for each device...\n");
> +
> +	return 0;
> +}
> +
> +module_init(fsl_mc_restool_driver_init);
> +
> +static void __exit fsl_mc_restool_driver_exit(void)
> +{
> +	struct restool_misc *restool_misc;
> +	struct restool_misc *restool_misc_tmp;
> +	char name1[20] = {0};
> +	uint32_t name2 = 0;
> +
> +	list_for_each_entry_safe(restool_misc, restool_misc_tmp,
> +				 &misc_list, list) {
> +		if (sscanf(restool_misc->misc.name, "%4s.%u", name1, &name2)
> +		    != 2) {
> +			pr_err("sscanf failure\n");

Delete the printk.

> +			return;

Should this be a continue?

> +		}
> +		pr_debug("name1=%s,name2=%u\n", name1, name2);
> +		pr_debug("misc-device: %s\n", restool_misc->misc.name);
> +		if (strcmp(name1, "dprc") == 0) {

Flip this around.

		if (strcmp(name1, "dprc") != 0)
			continue;

> +			if (WARN_ON(
> +			    restool_misc->static_mc_io == NULL))

This fits in 80 characters so no need for a line break.  And also these
days the prefered style is:

			if (WARN_ON(!restool_misc->static_mc_io))

> +				return;
> +
> +			if (WARN_ON(restool_misc->dynamic_instance_count != 0))
> +				return;
> +
> +			if (WARN_ON(restool_misc->static_instance_in_use))
> +				return;
> +
> +			misc_deregister(&restool_misc->misc);
> +			pr_info("/dev/%s driver unregistered\n",
> +				restool_misc->misc.name);
> +			fsl_mc_portal_free(
> +				restool_misc->static_mc_io);

This fits in 80 characters.

> +			list_del(&restool_misc->list);
> +			kfree(restool_misc);
> +		}
> +	}
> +}

regards,
dan carpenter
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists