> -----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 _______________________________________________ devel mailing list devel@xxxxxxxxxxxxxxxxxxxxxx http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
