[PATCH 5/5] staging: fsl-mc: Management Complex restool driver
Lijun Pan
Lijun.Pan at freescale.com
Mon Oct 26 16:01:59 UTC 2015
> -----Original Message-----
> From: Dan Carpenter [mailto:dan.carpenter at oracle.com]
> Sent: Monday, October 26, 2015 1:20 AM
> To: Pan Lijun-B44306 <Lijun.Pan at freescale.com>
> Cc: gregkh at linuxfoundation.org; arnd at arndb.de; devel at driverdev.osuosl.org;
> linux-kernel at vger.kernel.org; Yoder Stuart-B08248
> <stuart.yoder at freescale.com>; katz Itai-RM05202 <itai.katz at freescale.com>;
> Rivera Jose-B46482 <German.Rivera at freescale.com>; Li Yang-Leo-R58472
> <LeoLi at freescale.com>; Wood Scott-B07421 <scottwood at freescale.com>;
> agraf at suse.de; Hamciuc Bogdan-BHAMCIU1 <bhamciu1 at freescale.com>;
> Marginean Alexandru-R89243 <R89243 at freescale.com>; Sharma Bhupesh-
> B45370 <bhupesh.sharma at freescale.com>; Erez Nir-RM30794
> <nir.erez at freescale.com>; Schmitt Richard-B43082
> <richard.schmitt at freescale.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?
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
More information about the devel
mailing list