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