RE: [PATCH 5/5] staging: fsl-mc: Management Complex restool driver

[Lijun Pan]

> -----Original Message-----
> From: Dan Carpenter [mailto:dan.carpenter@xxxxxxxxxx]
> Sent: Monday, October 26, 2015 1:20 AM
> To: Pan Lijun-B44306 <Lijun.Pan@xxxxxxxxxxxxx>
> Cc: gregkh@xxxxxxxxxxxxxxxxxxx; arnd@xxxxxxxx; devel@xxxxxxxxxxxxxxxxxxxx;
> linux-kernel@xxxxxxxxxxxxxxx; Yoder Stuart-B08248
> <stuart.yoder@xxxxxxxxxxxxx>; katz Itai-RM05202 <itai.katz@xxxxxxxxxxxxx>;
> Rivera Jose-B46482 <German.Rivera@xxxxxxxxxxxxx>; Li Yang-Leo-R58472
> <LeoLi@xxxxxxxxxxxxx>; Wood Scott-B07421 <scottwood@xxxxxxxxxxxxx>;
> agraf@xxxxxxx; Hamciuc Bogdan-BHAMCIU1 <bhamciu1@xxxxxxxxxxxxx>;
> Marginean Alexandru-R89243 <R89243@xxxxxxxxxxxxx>; Sharma Bhupesh-
> B45370 <bhupesh.sharma@xxxxxxxxxxxxx>; Erez Nir-RM30794
> <nir.erez@xxxxxxxxxxxxx>; Schmitt Richard-B43082
> <richard.schmitt@xxxxxxxxxxxxx>
> 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?

Yes, this driver works for a long time internally.
This driver actually helps in the resource management and debug some way.
I will take your suggestions on all the areas and
I will remove pr_debug() stuff in the next Patch v2.

> 
> > +
> > +	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@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/