* Re: [PATCH 5/5] staging: fsl-mc: Management Complex restool driver
2015-10-25 22:41 ` [PATCH 5/5] staging: fsl-mc: Management Complex restool driver Lijun Pan
@ 2015-10-26 6:20 ` Dan Carpenter
2015-10-26 16:01 ` Lijun Pan
2015-10-26 15:52 ` Stuart Yoder
` (2 subsequent siblings)
3 siblings, 1 reply; 17+ messages in thread
From: Dan Carpenter @ 2015-10-26 6:20 UTC (permalink / raw)
To: Lijun Pan
Cc: gregkh, arnd, devel, linux-kernel, stuart.yoder, itai.katz,
german.rivera, leoli, scottwood, agraf, bhamciu1, R89243,
bhupesh.sharma, nir.erez, richard.schmitt
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
^ permalink raw reply [flat|nested] 17+ messages in thread* RE: [PATCH 5/5] staging: fsl-mc: Management Complex restool driver
2015-10-26 6:20 ` Dan Carpenter
@ 2015-10-26 16:01 ` Lijun Pan
0 siblings, 0 replies; 17+ messages in thread
From: Lijun Pan @ 2015-10-26 16:01 UTC (permalink / raw)
To: Dan Carpenter
Cc: gregkh, arnd, devel, linux-kernel, Stuart Yoder, Katz Itai,
Jose Rivera, Li Leo, Scott Wood, agraf, Hamciuc Bogdan,
Marginean Alexandru, Sharma Bhupesh, Erez Nir, Richard Schmitt
> -----Original Message-----
> From: Dan Carpenter [mailto:dan.carpenter@oracle.com]
> Sent: Monday, October 26, 2015 1:20 AM
> To: Pan Lijun-B44306 <Lijun.Pan@freescale.com>
> Cc: gregkh@linuxfoundation.org; arnd@arndb.de; devel@driverdev.osuosl.org;
> linux-kernel@vger.kernel.org; Yoder Stuart-B08248
> <stuart.yoder@freescale.com>; katz Itai-RM05202 <itai.katz@freescale.com>;
> Rivera Jose-B46482 <German.Rivera@freescale.com>; Li Yang-Leo-R58472
> <LeoLi@freescale.com>; Wood Scott-B07421 <scottwood@freescale.com>;
> agraf@suse.de; Hamciuc Bogdan-BHAMCIU1 <bhamciu1@freescale.com>;
> Marginean Alexandru-R89243 <R89243@freescale.com>; Sharma Bhupesh-
> B45370 <bhupesh.sharma@freescale.com>; Erez Nir-RM30794
> <nir.erez@freescale.com>; Schmitt Richard-B43082
> <richard.schmitt@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
^ permalink raw reply [flat|nested] 17+ messages in thread
* RE: [PATCH 5/5] staging: fsl-mc: Management Complex restool driver
2015-10-25 22:41 ` [PATCH 5/5] staging: fsl-mc: Management Complex restool driver Lijun Pan
2015-10-26 6:20 ` Dan Carpenter
@ 2015-10-26 15:52 ` Stuart Yoder
2015-10-27 2:11 ` Stuart Yoder
2015-10-27 5:16 ` Scott Wood
3 siblings, 0 replies; 17+ messages in thread
From: Stuart Yoder @ 2015-10-26 15:52 UTC (permalink / raw)
To: Lijun Pan, gregkh, arnd, devel, linux-kernel
Cc: Katz Itai, Jose Rivera, Li Leo, Scott Wood, agraf,
Hamciuc Bogdan, Marginean Alexandru, Sharma Bhupesh, Erez Nir,
Richard Schmitt, dan.carpenter, Lijun Pan
> -----Original Message-----
> From: Lijun Pan [mailto:Lijun.Pan@freescale.com]
> Sent: Sunday, October 25, 2015 5:41 PM
> To: gregkh@linuxfoundation.org; arnd@arndb.de; devel@driverdev.osuosl.org; linux-kernel@vger.kernel.org
> Cc: Yoder Stuart-B08248; katz Itai-RM05202; Rivera Jose-B46482; Li Yang-Leo-R58472; Wood Scott-B07421;
> agraf@suse.de; Hamciuc Bogdan-BHAMCIU1; Marginean Alexandru-R89243; Sharma Bhupesh-B45370; Erez Nir-RM30794;
> Schmitt Richard-B43082; dan.carpenter@oracle.com; Pan Lijun-B44306
> Subject: [PATCH 5/5] staging: fsl-mc: Management Complex restool driver
>
> The kernel support for the restool (a user space resource management
> tool) is a driver for the /dev/dprc.N device file.
> Its purpose is to provide an ioctl interface,
> which the restool uses to interact with the MC bus driver
> and with the MC firmware.
> We allocate a dpmcp at driver initialization,
> and keep that dpmcp until driver exit.
> We use that dpmcp by default.
> If that dpmcp is in use, we create another portal at run time
> and destroy the newly created portal after use.
> The ioctl RESTOOL_SEND_MC_COMMAND sends user space command to fsl-mc
> bus and utilizes the fsl-mc bus to communicate with MC firmware.
> The ioctl RESTOOL_DPRC_SYNC request the mc-bus launch
> objects scan under root dprc.
> In order to support multiple root dprc, we utilize the bus notify
> mechanism to scan fsl_mc_bus_type for the newly added root dprc.
> After discovering the root dprc, it creates a miscdevice
> /dev/dprc.N to associate with this root dprc.
>
> Signed-off-by: Lijun Pan <Lijun.Pan@freescale.com>
> ---
> drivers/staging/fsl-mc/bus/Kconfig | 7 +-
> drivers/staging/fsl-mc/bus/Makefile | 3 +
> drivers/staging/fsl-mc/bus/mc-ioctl.h | 24 ++
> drivers/staging/fsl-mc/bus/mc-restool.c | 488 ++++++++++++++++++++++++++++++++
> 4 files changed, 521 insertions(+), 1 deletion(-)
> create mode 100644 drivers/staging/fsl-mc/bus/mc-ioctl.h
> create mode 100644 drivers/staging/fsl-mc/bus/mc-restool.c
>
> diff --git a/drivers/staging/fsl-mc/bus/Kconfig b/drivers/staging/fsl-mc/bus/Kconfig
> index 0d779d9..39c6ef9 100644
> --- a/drivers/staging/fsl-mc/bus/Kconfig
> +++ b/drivers/staging/fsl-mc/bus/Kconfig
> @@ -21,4 +21,9 @@ config FSL_MC_BUS
> Only enable this option when building the kernel for
> Freescale QorQIQ LS2xxxx SoCs.
>
> -
> +config FSL_MC_RESTOOL
> + tristate "Freescale Management Complex (MC) restool driver"
> + depends on FSL_MC_BUS
> + help
> + Driver that provides kernel support for the Freescale Management
> + Complex resource manager user-space tool.
> diff --git a/drivers/staging/fsl-mc/bus/Makefile b/drivers/staging/fsl-mc/bus/Makefile
> index 25433a9..28b5fc0 100644
> --- a/drivers/staging/fsl-mc/bus/Makefile
> +++ b/drivers/staging/fsl-mc/bus/Makefile
> @@ -15,3 +15,6 @@ mc-bus-driver-objs := mc-bus.o \
> mc-allocator.o \
> dpmcp.o \
> dpbp.o
> +
> +# MC restool kernel support
> +obj-$(CONFIG_FSL_MC_RESTOOL) += mc-restool.o
> diff --git a/drivers/staging/fsl-mc/bus/mc-ioctl.h b/drivers/staging/fsl-mc/bus/mc-ioctl.h
> new file mode 100644
> index 0000000..e52f907
> --- /dev/null
> +++ b/drivers/staging/fsl-mc/bus/mc-ioctl.h
> @@ -0,0 +1,24 @@
> +/*
> + * Freescale Management Complex (MC) ioclt interface
> + *
> + * Copyright (C) 2014 Freescale Semiconductor, Inc.
> + * Author: Lijun Pan <Lijun.Pan@freescale.com>
> + *
> + * This file is licensed under the terms of the GNU General Public
> + * License version 2. This program is licensed "as is" without any
> + * warranty of any kind, whether express or implied.
> + */
> +#ifndef _FSL_MC_IOCTL_H_
> +#define _FSL_MC_IOCTL_H_
> +
> +#include <linux/ioctl.h>
> +
> +#define RESTOOL_IOCTL_TYPE 'R'
> +
> +#define RESTOOL_DPRC_SYNC \
> + _IO(RESTOOL_IOCTL_TYPE, 0x2)
We no longer need a sync ioctl, as a the /sys/bus/fsl-mc/rescan mechanism
in the previous patch replaces this.
> +#define RESTOOL_SEND_MC_COMMAND \
> + _IOWR(RESTOOL_IOCTL_TYPE, 0x4, struct mc_command)
> +
> +#endif /* _FSL_MC_IOCTL_H_ */
> diff --git a/drivers/staging/fsl-mc/bus/mc-restool.c b/drivers/staging/fsl-mc/bus/mc-restool.c
> new file mode 100644
> index 0000000..a219172
> --- /dev/null
> +++ b/drivers/staging/fsl-mc/bus/mc-restool.c
> @@ -0,0 +1,488 @@
> +/*
> + * Freescale Management Complex (MC) restool driver
> + *
> + * Copyright (C) 2014 Freescale Semiconductor, Inc.
> + * Author: Lijun Pan <Lijun.Pan@freescale.com>
> + *
> + * This file is licensed under the terms of the GNU General Public
> + * License version 2. This program is licensed "as is" without any
> + * warranty of any kind, whether express or implied.
> + */
> +
> +#include "../include/mc-private.h"
> +#include <linux/module.h>
> +#include <linux/fs.h>
> +#include <linux/miscdevice.h>
> +#include <linux/mm.h>
> +#include <linux/slab.h>
> +#include <linux/uaccess.h>
> +#include <linux/mutex.h>
> +#include <linux/platform_device.h>
> +#include "mc-ioctl.h"
> +#include "../include/mc-sys.h"
> +#include "../include/mc-cmd.h"
> +#include "../include/dpmng.h"
> +
> +/**
> + * Maximum number of DPRCs that can be opened at the same time
> + */
> +#define MAX_DPRC_HANDLES 64
> +
> +/**
> + * restool_misc - information associated with the newly added miscdevice
> + * @misc: newly created miscdevice associated with root dprc
> + * @miscdevt: device id of this miscdevice
> + * @list: a linked list node representing this miscdevcie
> + * @static_mc_io: pointer to the static MC I/O object used by the restool
> + * @dynamic_instance_count: number of dynamically created instances
> + * @static_instance_in_use: static instance is in use or not
> + * @mutex: mutex lock to serialze the operations
> + * @dev: root dprc associated with this miscdevice
> + */
> +struct restool_misc {
> + struct miscdevice misc;
> + dev_t miscdevt;
> + struct list_head list;
> + struct fsl_mc_io *static_mc_io;
> + uint32_t dynamic_instance_count;
> + bool static_instance_in_use;
> + struct mutex mutex;
> + struct device *dev;
> +};
> +
> +/*
> + * initialize a global list to link all
> + * the miscdevice nodes (struct restool_misc)
> + */
> +LIST_HEAD(misc_list);
> +
> +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;
> +}
> +
> +static int fsl_mc_restool_dev_release(struct inode *inode, struct file *filep)
> +{
> + struct fsl_mc_io *local_mc_io = filep->private_data;
> + struct restool_misc *restool_misc;
> + struct restool_misc *restool_misc_cursor;
> +
> + if (WARN_ON(filep->private_data == NULL))
> + return -EINVAL;
> +
> + 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;
> +
> + mutex_lock(&restool_misc->mutex);
> +
> + if (WARN_ON(restool_misc->dynamic_instance_count == 0 &&
> + !restool_misc->static_instance_in_use)) {
> + mutex_unlock(&restool_misc->mutex);
> + return -EINVAL;
> + }
> +
> + /* Globally clean up opened/untracked handles */
> + fsl_mc_portal_reset(local_mc_io);
> +
> + pr_debug("dynamic instance count: %d\n",
> + restool_misc->dynamic_instance_count);
> + pr_debug("static instance count: %d\n",
> + restool_misc->static_instance_in_use);
> +
> + /*
> + * must check
> + * whether local_mc_io is dynamic or static instance
> + * Otherwise it will free up the reserved portal by accident
> + * or even not free up the dynamic allocated portal
> + * if 2 or more instances running concurrently
> + */
> + if (local_mc_io == restool_misc->static_mc_io) {
> + pr_debug("this is reserved portal");
> + pr_debug("reserved portal not in use\n");
> + restool_misc->static_instance_in_use = false;
> + } else {
> + pr_debug("this is dynamically allocated portal");
> + pr_debug("free one dynamically allocated portal\n");
> + fsl_mc_portal_free(local_mc_io);
> + kfree(filep->private_data);
> + --restool_misc->dynamic_instance_count;
> + }
> +
> + filep->private_data = NULL;
> + mutex_unlock(&restool_misc->mutex);
> +
> + return 0;
> +}
> +
> +static int restool_dprc_sync(struct inode *inode)
> +{
> + int error = 0;
> + struct fsl_mc_device *root_mc_dev;
> + struct fsl_mc_bus *root_mc_bus;
> + 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;
> +
> + root_mc_dev = to_fsl_mc_device(restool_misc->dev);
> + root_mc_bus = to_fsl_mc_bus(root_mc_dev);
> +
> + mutex_lock(&root_mc_bus->scan_mutex);
> + error = dprc_scan_objects(root_mc_dev);
> + mutex_unlock(&root_mc_bus->scan_mutex);
> + pr_debug("sync_error = %d\n", error);
> +
> + return error;
> +}
We no longer need/use the ioctl interface for sync.
> +static int restool_send_mc_command(unsigned long arg,
> + struct fsl_mc_io *local_mc_io)
> +{
> + int error = -EINVAL;
> + 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;
> + }
> +
> + /*
> + * 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;
> + }
> +
> + return 0;
> +error:
> + return error;
> +}
> +
> +static long
> +fsl_mc_restool_dev_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
> +{
> + int error = 0;
> +
> + switch (cmd) {
> + case RESTOOL_DPRC_SYNC:
> + pr_debug("syncing...\n");
> + error = restool_dprc_sync(file->f_inode);
> + pr_debug("syncing finished...\n");
> + break;
Remove the sync ioctl.
> + 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");
> + 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)));
> + }
> +
> + 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)) {
> + 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;
> + }
> +
> + 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;
> + }
> +
> + 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__);
> + 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);
The only time we want to do add a new device file is for root DPRCs.
This code would seem to add a device file any time any device is
added to the bus? How are you filtering out all the other
object types?...it's not clear to me.
> + 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__);
> +
> + return 0;
> +}
There is only a single line of functionality in the above function. All the
rest pr_debug and pr_info. We certainly don't need pr_info statements
every time a bus notify occurs.
In general I think this driver has too many debug prints in it.
Thanks,
Stuart
^ permalink raw reply [flat|nested] 17+ messages in thread* RE: [PATCH 5/5] staging: fsl-mc: Management Complex restool driver
2015-10-25 22:41 ` [PATCH 5/5] staging: fsl-mc: Management Complex restool driver Lijun Pan
2015-10-26 6:20 ` Dan Carpenter
2015-10-26 15:52 ` Stuart Yoder
@ 2015-10-27 2:11 ` Stuart Yoder
2015-10-27 5:16 ` Scott Wood
3 siblings, 0 replies; 17+ messages in thread
From: Stuart Yoder @ 2015-10-27 2:11 UTC (permalink / raw)
To: Lijun Pan, gregkh, arnd, devel, linux-kernel
Cc: Katz Itai, Jose Rivera, Li Leo, Scott Wood, agraf,
Hamciuc Bogdan, Marginean Alexandru, Sharma Bhupesh, Erez Nir,
Richard Schmitt, dan.carpenter, Lijun Pan
> -----Original Message-----
> From: Lijun Pan [mailto:Lijun.Pan@freescale.com]
> Sent: Sunday, October 25, 2015 5:41 PM
> To: gregkh@linuxfoundation.org; arnd@arndb.de; devel@driverdev.osuosl.org; linux-kernel@vger.kernel.org
> Cc: Yoder Stuart-B08248; katz Itai-RM05202; Rivera Jose-B46482; Li Yang-Leo-R58472; Wood Scott-B07421;
> agraf@suse.de; Hamciuc Bogdan-BHAMCIU1; Marginean Alexandru-R89243; Sharma Bhupesh-B45370; Erez Nir-RM30794;
> Schmitt Richard-B43082; dan.carpenter@oracle.com; Pan Lijun-B44306
> Subject: [PATCH 5/5] staging: fsl-mc: Management Complex restool driver
>
> The kernel support for the restool (a user space resource management
> tool) is a driver for the /dev/dprc.N device file.
> Its purpose is to provide an ioctl interface,
> which the restool uses to interact with the MC bus driver
Name of the user space tool should be updated to be current: s/restool/ls-restool/
> and with the MC firmware.
> We allocate a dpmcp at driver initialization,
> and keep that dpmcp until driver exit.
> We use that dpmcp by default.
> If that dpmcp is in use, we create another portal at run time
> and destroy the newly created portal after use.
> The ioctl RESTOOL_SEND_MC_COMMAND sends user space command to fsl-mc
> bus and utilizes the fsl-mc bus to communicate with MC firmware.
> The ioctl RESTOOL_DPRC_SYNC request the mc-bus launch
> objects scan under root dprc.
> In order to support multiple root dprc, we utilize the bus notify
> mechanism to scan fsl_mc_bus_type for the newly added root dprc.
> After discovering the root dprc, it creates a miscdevice
> /dev/dprc.N to associate with this root dprc.
>
> Signed-off-by: Lijun Pan <Lijun.Pan@freescale.com>
> ---
> drivers/staging/fsl-mc/bus/Kconfig | 7 +-
> drivers/staging/fsl-mc/bus/Makefile | 3 +
> drivers/staging/fsl-mc/bus/mc-ioctl.h | 24 ++
> drivers/staging/fsl-mc/bus/mc-restool.c | 488 ++++++++++++++++++++++++++++++++
> 4 files changed, 521 insertions(+), 1 deletion(-)
> create mode 100644 drivers/staging/fsl-mc/bus/mc-ioctl.h
> create mode 100644 drivers/staging/fsl-mc/bus/mc-restool.c
>
> diff --git a/drivers/staging/fsl-mc/bus/Kconfig b/drivers/staging/fsl-mc/bus/Kconfig
> index 0d779d9..39c6ef9 100644
> --- a/drivers/staging/fsl-mc/bus/Kconfig
> +++ b/drivers/staging/fsl-mc/bus/Kconfig
> @@ -21,4 +21,9 @@ config FSL_MC_BUS
> Only enable this option when building the kernel for
> Freescale QorQIQ LS2xxxx SoCs.
>
> -
> +config FSL_MC_RESTOOL
> + tristate "Freescale Management Complex (MC) restool driver"
s/restool/ls-restool/
Thanks,
Stuart
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 5/5] staging: fsl-mc: Management Complex restool driver
2015-10-25 22:41 ` [PATCH 5/5] staging: fsl-mc: Management Complex restool driver Lijun Pan
` (2 preceding siblings ...)
2015-10-27 2:11 ` Stuart Yoder
@ 2015-10-27 5:16 ` Scott Wood
2015-10-29 23:54 ` Lijun Pan
2015-10-30 16:43 ` Lijun Pan
3 siblings, 2 replies; 17+ messages in thread
From: Scott Wood @ 2015-10-27 5:16 UTC (permalink / raw)
To: Lijun Pan
Cc: gregkh, arnd, devel, linux-kernel, stuart.yoder, itai.katz,
german.rivera, leoli, agraf, bhamciu1, R89243, bhupesh.sharma,
nir.erez, richard.schmitt, dan.carpenter
On Sun, 2015-10-25 at 17:41 -0500, Lijun Pan wrote:
> The kernel support for the restool (a user space resource management
> tool) is a driver for the /dev/dprc.N device file.
> Its purpose is to provide an ioctl interface,
> which the restool uses to interact with the MC bus driver
> and with the MC firmware.
> We allocate a dpmcp at driver initialization,
> and keep that dpmcp until driver exit.
> We use that dpmcp by default.
> If that dpmcp is in use, we create another portal at run time
> and destroy the newly created portal after use.
> The ioctl RESTOOL_SEND_MC_COMMAND sends user space command to fsl-mc
> bus and utilizes the fsl-mc bus to communicate with MC firmware.
> The ioctl RESTOOL_DPRC_SYNC request the mc-bus launch
> objects scan under root dprc.
> In order to support multiple root dprc, we utilize the bus notify
> mechanism to scan fsl_mc_bus_type for the newly added root dprc.
> After discovering the root dprc, it creates a miscdevice
> /dev/dprc.N to associate with this root dprc.
>
> Signed-off-by: Lijun Pan <Lijun.Pan@freescale.com>
> ---
> drivers/staging/fsl-mc/bus/Kconfig | 7 +-
> drivers/staging/fsl-mc/bus/Makefile | 3 +
> drivers/staging/fsl-mc/bus/mc-ioctl.h | 24 ++
> drivers/staging/fsl-mc/bus/mc-restool.c | 488
> ++++++++++++++++++++++++++++++++
> 4 files changed, 521 insertions(+), 1 deletion(-)
> create mode 100644 drivers/staging/fsl-mc/bus/mc-ioctl.h
> create mode 100644 drivers/staging/fsl-mc/bus/mc-restool.c
>
> diff --git a/drivers/staging/fsl-mc/bus/Kconfig b/drivers/staging/fsl-
> mc/bus/Kconfig
> index 0d779d9..39c6ef9 100644
> --- a/drivers/staging/fsl-mc/bus/Kconfig
> +++ b/drivers/staging/fsl-mc/bus/Kconfig
> @@ -21,4 +21,9 @@ config FSL_MC_BUS
> Only enable this option when building the kernel for
> Freescale QorQIQ LS2xxxx SoCs.
>
> -
> +config FSL_MC_RESTOOL
> + tristate "Freescale Management Complex (MC) restool driver"
> + depends on FSL_MC_BUS
> + help
> + Driver that provides kernel support for the Freescale Management
> + Complex resource manager user-space tool.
> diff --git a/drivers/staging/fsl-mc/bus/Makefile b/drivers/staging/fsl-
> mc/bus/Makefile
> index 25433a9..28b5fc0 100644
> --- a/drivers/staging/fsl-mc/bus/Makefile
> +++ b/drivers/staging/fsl-mc/bus/Makefile
> @@ -15,3 +15,6 @@ mc-bus-driver-objs := mc-bus.o \
> mc-allocator.o \
> dpmcp.o \
> dpbp.o
> +
> +# MC restool kernel support
> +obj-$(CONFIG_FSL_MC_RESTOOL) += mc-restool.o
> diff --git a/drivers/staging/fsl-mc/bus/mc-ioctl.h b/drivers/staging/fsl-
> mc/bus/mc-ioctl.h
> new file mode 100644
> index 0000000..e52f907
> --- /dev/null
> +++ b/drivers/staging/fsl-mc/bus/mc-ioctl.h
> @@ -0,0 +1,24 @@
> +/*
> + * Freescale Management Complex (MC) ioclt interface
ioctl
> + *
> + * Copyright (C) 2014 Freescale Semiconductor, Inc.
> + * Author: Lijun Pan <Lijun.Pan@freescale.com>
> + *
> + * This file is licensed under the terms of the GNU General Public
> + * License version 2. This program is licensed "as is" without any
> + * warranty of any kind, whether express or implied.
> + */
> +#ifndef _FSL_MC_IOCTL_H_
> +#define _FSL_MC_IOCTL_H_
> +
> +#include <linux/ioctl.h>
> +
> +#define RESTOOL_IOCTL_TYPE 'R'
> +
> +#define RESTOOL_DPRC_SYNC \
> + _IO(RESTOOL_IOCTL_TYPE, 0x2)
> +
> +#define RESTOOL_SEND_MC_COMMAND \
> + _IOWR(RESTOOL_IOCTL_TYPE, 0x4, struct mc_command)
Look at Documentation/ioctl/ioctl-number.txt and reserve a range within 'R'
that doesn't conflict.
Add thorough documentation of this API.
I'm not sure how it's usually handled with staging drivers, but eventually
this will need to move to an appropriate uapi header. Is this functionality
even needed before the driver comes out of staging? I don't see "userspace
restool support" in drivers/staging/fsl-mc/TODO.
Don't reference struct mc_command without including the header that defines
it.
> +#endif /* _FSL_MC_IOCTL_H_ */
> diff --git a/drivers/staging/fsl-mc/bus/mc-restool.c b/drivers/staging/fsl-
> mc/bus/mc-restool.c
> new file mode 100644
> index 0000000..a219172
> --- /dev/null
> +++ b/drivers/staging/fsl-mc/bus/mc-restool.c
> @@ -0,0 +1,488 @@
> +/*
> + * Freescale Management Complex (MC) restool driver
> + *
> + * Copyright (C) 2014 Freescale Semiconductor, Inc.
> + * Author: Lijun Pan <Lijun.Pan@freescale.com>
> + *
> + * This file is licensed under the terms of the GNU General Public
> + * License version 2. This program is licensed "as is" without any
> + * warranty of any kind, whether express or implied.
> + */
> +
> +#include "../include/mc-private.h"
> +#include <linux/module.h>
> +#include <linux/fs.h>
> +#include <linux/miscdevice.h>
> +#include <linux/mm.h>
> +#include <linux/slab.h>
> +#include <linux/uaccess.h>
> +#include <linux/mutex.h>
> +#include <linux/platform_device.h>
> +#include "mc-ioctl.h"
> +#include "../include/mc-sys.h"
> +#include "../include/mc-cmd.h"
> +#include "../include/dpmng.h"
> +
> +/**
> + * Maximum number of DPRCs that can be opened at the same time
> + */
> +#define MAX_DPRC_HANDLES 64
> +
> +/**
> + * restool_misc - information associated with the newly added miscdevice
> + * @misc: newly created miscdevice associated with root dprc
> + * @miscdevt: device id of this miscdevice
> + * @list: a linked list node representing this miscdevcie
> + * @static_mc_io: pointer to the static MC I/O object used by the restool
> + * @dynamic_instance_count: number of dynamically created instances
> + * @static_instance_in_use: static instance is in use or not
> + * @mutex: mutex lock to serialze the operations
> + * @dev: root dprc associated with this miscdevice
> + */
> +struct restool_misc {
> + struct miscdevice misc;
> + dev_t miscdevt;
> + struct list_head list;
> + struct fsl_mc_io *static_mc_io;
> + uint32_t dynamic_instance_count;
> + bool static_instance_in_use;
> + struct mutex mutex;
> + struct device *dev;
> +};
> +
> +/*
> + * initialize a global list to link all
> + * the miscdevice nodes (struct restool_misc)
> + */
> +LIST_HEAD(misc_list);
static
> +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;
> + }
> + }
No synchronization on the list?
> +
> + 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;
> + }
Why is the first user handled differently from subsequent users? Does each
user really need its own portal, or can you just synchronize access?
-Scott
^ permalink raw reply [flat|nested] 17+ messages in thread* RE: [PATCH 5/5] staging: fsl-mc: Management Complex restool driver
2015-10-27 5:16 ` Scott Wood
@ 2015-10-29 23:54 ` Lijun Pan
2015-10-30 16:43 ` Lijun Pan
1 sibling, 0 replies; 17+ messages in thread
From: Lijun Pan @ 2015-10-29 23:54 UTC (permalink / raw)
To: Scott Wood
Cc: gregkh, arnd, devel, linux-kernel, Stuart Yoder, Katz Itai,
Jose Rivera, Li Leo, agraf, Hamciuc Bogdan, Marginean Alexandru,
Sharma Bhupesh, Erez Nir, Richard Schmitt, dan.carpenter
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 9864 bytes --]
> -----Original Message-----
> From: Wood Scott-B07421
> Sent: Tuesday, October 27, 2015 12:17 AM
> To: Pan Lijun-B44306 <Lijun.Pan@freescale.com>
> Cc: gregkh@linuxfoundation.org; arnd@arndb.de; devel@driverdev.osuosl.org;
> linux-kernel@vger.kernel.org; Yoder Stuart-B08248
> <stuart.yoder@freescale.com>; katz Itai-RM05202 <itai.katz@freescale.com>;
> Rivera Jose-B46482 <German.Rivera@freescale.com>; Li Yang-Leo-R58472
> <LeoLi@freescale.com>; agraf@suse.de; Hamciuc Bogdan-BHAMCIU1
> <bhamciu1@freescale.com>; Marginean Alexandru-R89243
> <R89243@freescale.com>; Sharma Bhupesh-B45370
> <bhupesh.sharma@freescale.com>; Erez Nir-RM30794
> <nir.erez@freescale.com>; Schmitt Richard-B43082
> <richard.schmitt@freescale.com>; dan.carpenter@oracle.com
> Subject: Re: [PATCH 5/5] staging: fsl-mc: Management Complex restool driver
>
> On Sun, 2015-10-25 at 17:41 -0500, Lijun Pan wrote:
> > The kernel support for the restool (a user space resource management
> > tool) is a driver for the /dev/dprc.N device file.
> > Its purpose is to provide an ioctl interface, which the restool uses
> > to interact with the MC bus driver and with the MC firmware.
> > We allocate a dpmcp at driver initialization, and keep that dpmcp
> > until driver exit.
> > We use that dpmcp by default.
> > If that dpmcp is in use, we create another portal at run time and
> > destroy the newly created portal after use.
> > The ioctl RESTOOL_SEND_MC_COMMAND sends user space command to fsl-
> mc
> > bus and utilizes the fsl-mc bus to communicate with MC firmware.
> > The ioctl RESTOOL_DPRC_SYNC request the mc-bus launch objects scan
> > under root dprc.
> > In order to support multiple root dprc, we utilize the bus notify
> > mechanism to scan fsl_mc_bus_type for the newly added root dprc.
> > After discovering the root dprc, it creates a miscdevice /dev/dprc.N
> > to associate with this root dprc.
> >
> > Signed-off-by: Lijun Pan <Lijun.Pan@freescale.com>
> > ---
> > drivers/staging/fsl-mc/bus/Kconfig | 7 +-
> > drivers/staging/fsl-mc/bus/Makefile | 3 +
> > drivers/staging/fsl-mc/bus/mc-ioctl.h | 24 ++
> > drivers/staging/fsl-mc/bus/mc-restool.c | 488
> > ++++++++++++++++++++++++++++++++
> > 4 files changed, 521 insertions(+), 1 deletion(-) create mode 100644
> > drivers/staging/fsl-mc/bus/mc-ioctl.h
> > create mode 100644 drivers/staging/fsl-mc/bus/mc-restool.c
> >
> > diff --git a/drivers/staging/fsl-mc/bus/Kconfig b/drivers/staging/fsl-
> > mc/bus/Kconfig index 0d779d9..39c6ef9 100644
> > --- a/drivers/staging/fsl-mc/bus/Kconfig
> > +++ b/drivers/staging/fsl-mc/bus/Kconfig
> > @@ -21,4 +21,9 @@ config FSL_MC_BUS
> > Only enable this option when building the kernel for
> > Freescale QorQIQ LS2xxxx SoCs.
> >
> > -
> > +config FSL_MC_RESTOOL
> > + tristate "Freescale Management Complex (MC) restool driver"
> > + depends on FSL_MC_BUS
> > + help
> > + Driver that provides kernel support for the Freescale Management
> > + Complex resource manager user-space tool.
> > diff --git a/drivers/staging/fsl-mc/bus/Makefile
> > b/drivers/staging/fsl- mc/bus/Makefile index 25433a9..28b5fc0 100644
> > --- a/drivers/staging/fsl-mc/bus/Makefile
> > +++ b/drivers/staging/fsl-mc/bus/Makefile
> > @@ -15,3 +15,6 @@ mc-bus-driver-objs := mc-bus.o \
> > mc-allocator.o \
> > dpmcp.o \
> > dpbp.o
> > +
> > +# MC restool kernel support
> > +obj-$(CONFIG_FSL_MC_RESTOOL) += mc-restool.o
> > diff --git a/drivers/staging/fsl-mc/bus/mc-ioctl.h
> > b/drivers/staging/fsl- mc/bus/mc-ioctl.h new file mode 100644 index
> > 0000000..e52f907
> > --- /dev/null
> > +++ b/drivers/staging/fsl-mc/bus/mc-ioctl.h
> > @@ -0,0 +1,24 @@
> > +/*
> > + * Freescale Management Complex (MC) ioclt interface
>
> ioctl
>
> > + *
> > + * Copyright (C) 2014 Freescale Semiconductor, Inc.
> > + * Author: Lijun Pan <Lijun.Pan@freescale.com>
> > + *
> > + * This file is licensed under the terms of the GNU General Public
> > + * License version 2. This program is licensed "as is" without any
> > + * warranty of any kind, whether express or implied.
> > + */
> > +#ifndef _FSL_MC_IOCTL_H_
> > +#define _FSL_MC_IOCTL_H_
> > +
> > +#include <linux/ioctl.h>
> > +
> > +#define RESTOOL_IOCTL_TYPE 'R'
> > +
> > +#define RESTOOL_DPRC_SYNC \
> > + _IO(RESTOOL_IOCTL_TYPE, 0x2)
> > +
> > +#define RESTOOL_SEND_MC_COMMAND \
> > + _IOWR(RESTOOL_IOCTL_TYPE, 0x4, struct mc_command)
>
> Look at Documentation/ioctl/ioctl-number.txt and reserve a range within 'R'
> that doesn't conflict.
>
> Add thorough documentation of this API.
What path do you recommend me to put documentation of this API?
>
> I'm not sure how it's usually handled with staging drivers, but eventually this
> will need to move to an appropriate uapi header. Is this functionality even
> needed before the driver comes out of staging? I don't see "userspace restool
> support" in drivers/staging/fsl-mc/TODO.
>
> Don't reference struct mc_command without including the header that defines
> it.
>
> > +#endif /* _FSL_MC_IOCTL_H_ */
> > diff --git a/drivers/staging/fsl-mc/bus/mc-restool.c
> > b/drivers/staging/fsl- mc/bus/mc-restool.c new file mode 100644 index
> > 0000000..a219172
> > --- /dev/null
> > +++ b/drivers/staging/fsl-mc/bus/mc-restool.c
> > @@ -0,0 +1,488 @@
> > +/*
> > + * Freescale Management Complex (MC) restool driver
> > + *
> > + * Copyright (C) 2014 Freescale Semiconductor, Inc.
> > + * Author: Lijun Pan <Lijun.Pan@freescale.com>
> > + *
> > + * This file is licensed under the terms of the GNU General Public
> > + * License version 2. This program is licensed "as is" without any
> > + * warranty of any kind, whether express or implied.
> > + */
> > +
> > +#include "../include/mc-private.h"
> > +#include <linux/module.h>
> > +#include <linux/fs.h>
> > +#include <linux/miscdevice.h>
> > +#include <linux/mm.h>
> > +#include <linux/slab.h>
> > +#include <linux/uaccess.h>
> > +#include <linux/mutex.h>
> > +#include <linux/platform_device.h>
> > +#include "mc-ioctl.h"
> > +#include "../include/mc-sys.h"
> > +#include "../include/mc-cmd.h"
> > +#include "../include/dpmng.h"
> > +
> > +/**
> > + * Maximum number of DPRCs that can be opened at the same time */
> > +#define MAX_DPRC_HANDLES 64
> > +
> > +/**
> > + * restool_misc - information associated with the newly added
> > +miscdevice
> > + * @misc: newly created miscdevice associated with root dprc
> > + * @miscdevt: device id of this miscdevice
> > + * @list: a linked list node representing this miscdevcie
> > + * @static_mc_io: pointer to the static MC I/O object used by the
> > +restool
> > + * @dynamic_instance_count: number of dynamically created instances
> > + * @static_instance_in_use: static instance is in use or not
> > + * @mutex: mutex lock to serialze the operations
> > + * @dev: root dprc associated with this miscdevice */ struct
> > +restool_misc {
> > + struct miscdevice misc;
> > + dev_t miscdevt;
> > + struct list_head list;
> > + struct fsl_mc_io *static_mc_io;
> > + uint32_t dynamic_instance_count;
> > + bool static_instance_in_use;
> > + struct mutex mutex;
> > + struct device *dev;
> > +};
> > +
> > +/*
> > + * initialize a global list to link all
> > + * the miscdevice nodes (struct restool_misc) */
> > +LIST_HEAD(misc_list);
>
> static
>
> > +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;
> > + }
> > + }
>
> No synchronization on the list?
>
> > +
> > + 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;
> > + }
>
> Why is the first user handled differently from subsequent users? Does each
> user really need its own portal, or can you just synchronize access?
First user get the statically allocated portal.
Second user need to dynamically allocate one.
Some operations may take a long time (say 1 second) in MC object creation.
In order to support asynchronous portal operation,
We allocate portal for each user.
Lijun
>
> -Scott
ÿôèº{.nÇ+‰·Ÿ®‰†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a¢f£¢·hšïêÿ‘êçz_è®\x03(階ŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨èÚ&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥
^ permalink raw reply [flat|nested] 17+ messages in thread* RE: [PATCH 5/5] staging: fsl-mc: Management Complex restool driver
2015-10-27 5:16 ` Scott Wood
2015-10-29 23:54 ` Lijun Pan
@ 2015-10-30 16:43 ` Lijun Pan
1 sibling, 0 replies; 17+ messages in thread
From: Lijun Pan @ 2015-10-30 16:43 UTC (permalink / raw)
To: Scott Wood
Cc: gregkh, arnd, devel, linux-kernel, Stuart Yoder, Katz Itai,
Jose Rivera, Li Leo, agraf, Hamciuc Bogdan, Marginean Alexandru,
Sharma Bhupesh, Erez Nir, Richard Schmitt, dan.carpenter
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 11277 bytes --]
> -----Original Message-----
> From: Pan Lijun-B44306
> Sent: Thursday, October 29, 2015 6:55 PM
> To: Wood Scott-B07421 <scottwood@freescale.com>
> Cc: gregkh@linuxfoundation.org; arnd@arndb.de; devel@driverdev.osuosl.org;
> linux-kernel@vger.kernel.org; Yoder Stuart-B08248
> <stuart.yoder@freescale.com>; katz Itai-RM05202 <itai.katz@freescale.com>;
> Rivera Jose-B46482 <German.Rivera@freescale.com>; Li Yang-Leo-R58472
> <LeoLi@freescale.com>; agraf@suse.de; Hamciuc Bogdan-BHAMCIU1
> <bhamciu1@freescale.com>; Marginean Alexandru-R89243
> <R89243@freescale.com>; Sharma Bhupesh-B45370
> <bhupesh.sharma@freescale.com>; Erez Nir-RM30794
> <nir.erez@freescale.com>; Schmitt Richard-B43082
> <richard.schmitt@freescale.com>; dan.carpenter@oracle.com
> Subject: RE: [PATCH 5/5] staging: fsl-mc: Management Complex restool driver
>
>
>
> > -----Original Message-----
> > From: Wood Scott-B07421
> > Sent: Tuesday, October 27, 2015 12:17 AM
> > To: Pan Lijun-B44306 <Lijun.Pan@freescale.com>
> > Cc: gregkh@linuxfoundation.org; arnd@arndb.de;
> > devel@driverdev.osuosl.org; linux-kernel@vger.kernel.org; Yoder
> > Stuart-B08248 <stuart.yoder@freescale.com>; katz Itai-RM05202
> > <itai.katz@freescale.com>; Rivera Jose-B46482
> > <German.Rivera@freescale.com>; Li Yang-Leo-R58472
> > <LeoLi@freescale.com>; agraf@suse.de; Hamciuc Bogdan-BHAMCIU1
> > <bhamciu1@freescale.com>; Marginean Alexandru-R89243
> > <R89243@freescale.com>; Sharma Bhupesh-B45370
> > <bhupesh.sharma@freescale.com>; Erez Nir-RM30794
> > <nir.erez@freescale.com>; Schmitt Richard-B43082
> > <richard.schmitt@freescale.com>; dan.carpenter@oracle.com
> > Subject: Re: [PATCH 5/5] staging: fsl-mc: Management Complex restool
> > driver
> >
> > On Sun, 2015-10-25 at 17:41 -0500, Lijun Pan wrote:
> > > The kernel support for the restool (a user space resource management
> > > tool) is a driver for the /dev/dprc.N device file.
> > > Its purpose is to provide an ioctl interface, which the restool uses
> > > to interact with the MC bus driver and with the MC firmware.
> > > We allocate a dpmcp at driver initialization, and keep that dpmcp
> > > until driver exit.
> > > We use that dpmcp by default.
> > > If that dpmcp is in use, we create another portal at run time and
> > > destroy the newly created portal after use.
> > > The ioctl RESTOOL_SEND_MC_COMMAND sends user space command to
> fsl-
> > mc
> > > bus and utilizes the fsl-mc bus to communicate with MC firmware.
> > > The ioctl RESTOOL_DPRC_SYNC request the mc-bus launch objects scan
> > > under root dprc.
> > > In order to support multiple root dprc, we utilize the bus notify
> > > mechanism to scan fsl_mc_bus_type for the newly added root dprc.
> > > After discovering the root dprc, it creates a miscdevice /dev/dprc.N
> > > to associate with this root dprc.
> > >
> > > Signed-off-by: Lijun Pan <Lijun.Pan@freescale.com>
> > > ---
> > > drivers/staging/fsl-mc/bus/Kconfig | 7 +-
> > > drivers/staging/fsl-mc/bus/Makefile | 3 +
> > > drivers/staging/fsl-mc/bus/mc-ioctl.h | 24 ++
> > > drivers/staging/fsl-mc/bus/mc-restool.c | 488
> > > ++++++++++++++++++++++++++++++++
> > > 4 files changed, 521 insertions(+), 1 deletion(-) create mode
> > > 100644 drivers/staging/fsl-mc/bus/mc-ioctl.h
> > > create mode 100644 drivers/staging/fsl-mc/bus/mc-restool.c
> > >
> > > diff --git a/drivers/staging/fsl-mc/bus/Kconfig
> > > b/drivers/staging/fsl- mc/bus/Kconfig index 0d779d9..39c6ef9 100644
> > > --- a/drivers/staging/fsl-mc/bus/Kconfig
> > > +++ b/drivers/staging/fsl-mc/bus/Kconfig
> > > @@ -21,4 +21,9 @@ config FSL_MC_BUS
> > > Only enable this option when building the kernel for
> > > Freescale QorQIQ LS2xxxx SoCs.
> > >
> > > -
> > > +config FSL_MC_RESTOOL
> > > + tristate "Freescale Management Complex (MC) restool driver"
> > > + depends on FSL_MC_BUS
> > > + help
> > > + Driver that provides kernel support for the Freescale Management
> > > + Complex resource manager user-space tool.
> > > diff --git a/drivers/staging/fsl-mc/bus/Makefile
> > > b/drivers/staging/fsl- mc/bus/Makefile index 25433a9..28b5fc0 100644
> > > --- a/drivers/staging/fsl-mc/bus/Makefile
> > > +++ b/drivers/staging/fsl-mc/bus/Makefile
> > > @@ -15,3 +15,6 @@ mc-bus-driver-objs := mc-bus.o \
> > > mc-allocator.o \
> > > dpmcp.o \
> > > dpbp.o
> > > +
> > > +# MC restool kernel support
> > > +obj-$(CONFIG_FSL_MC_RESTOOL) += mc-restool.o
> > > diff --git a/drivers/staging/fsl-mc/bus/mc-ioctl.h
> > > b/drivers/staging/fsl- mc/bus/mc-ioctl.h new file mode 100644 index
> > > 0000000..e52f907
> > > --- /dev/null
> > > +++ b/drivers/staging/fsl-mc/bus/mc-ioctl.h
> > > @@ -0,0 +1,24 @@
> > > +/*
> > > + * Freescale Management Complex (MC) ioclt interface
> >
> > ioctl
> >
> > > + *
> > > + * Copyright (C) 2014 Freescale Semiconductor, Inc.
> > > + * Author: Lijun Pan <Lijun.Pan@freescale.com>
> > > + *
> > > + * This file is licensed under the terms of the GNU General Public
> > > + * License version 2. This program is licensed "as is" without any
> > > + * warranty of any kind, whether express or implied.
> > > + */
> > > +#ifndef _FSL_MC_IOCTL_H_
> > > +#define _FSL_MC_IOCTL_H_
> > > +
> > > +#include <linux/ioctl.h>
> > > +
> > > +#define RESTOOL_IOCTL_TYPE 'R'
> > > +
> > > +#define RESTOOL_DPRC_SYNC \
> > > + _IO(RESTOOL_IOCTL_TYPE, 0x2)
> > > +
> > > +#define RESTOOL_SEND_MC_COMMAND \
> > > + _IOWR(RESTOOL_IOCTL_TYPE, 0x4, struct mc_command)
> >
> > Look at Documentation/ioctl/ioctl-number.txt and reserve a range within 'R'
> > that doesn't conflict.
> >
> > Add thorough documentation of this API.
>
> What path do you recommend me to put documentation of this API?
Is Documentation/misc-devices/mc-restool.txt the correct location?
> >
> > I'm not sure how it's usually handled with staging drivers, but
> > eventually this will need to move to an appropriate uapi header. Is
> > this functionality even needed before the driver comes out of staging?
> > I don't see "userspace restool support" in drivers/staging/fsl-mc/TODO.
> >
> > Don't reference struct mc_command without including the header that
> > defines it.
> >
> > > +#endif /* _FSL_MC_IOCTL_H_ */
> > > diff --git a/drivers/staging/fsl-mc/bus/mc-restool.c
> > > b/drivers/staging/fsl- mc/bus/mc-restool.c new file mode 100644
> > > index
> > > 0000000..a219172
> > > --- /dev/null
> > > +++ b/drivers/staging/fsl-mc/bus/mc-restool.c
> > > @@ -0,0 +1,488 @@
> > > +/*
> > > + * Freescale Management Complex (MC) restool driver
> > > + *
> > > + * Copyright (C) 2014 Freescale Semiconductor, Inc.
> > > + * Author: Lijun Pan <Lijun.Pan@freescale.com>
> > > + *
> > > + * This file is licensed under the terms of the GNU General Public
> > > + * License version 2. This program is licensed "as is" without any
> > > + * warranty of any kind, whether express or implied.
> > > + */
> > > +
> > > +#include "../include/mc-private.h"
> > > +#include <linux/module.h>
> > > +#include <linux/fs.h>
> > > +#include <linux/miscdevice.h>
> > > +#include <linux/mm.h>
> > > +#include <linux/slab.h>
> > > +#include <linux/uaccess.h>
> > > +#include <linux/mutex.h>
> > > +#include <linux/platform_device.h>
> > > +#include "mc-ioctl.h"
> > > +#include "../include/mc-sys.h"
> > > +#include "../include/mc-cmd.h"
> > > +#include "../include/dpmng.h"
> > > +
> > > +/**
> > > + * Maximum number of DPRCs that can be opened at the same time */
> > > +#define MAX_DPRC_HANDLES 64
> > > +
> > > +/**
> > > + * restool_misc - information associated with the newly added
> > > +miscdevice
> > > + * @misc: newly created miscdevice associated with root dprc
> > > + * @miscdevt: device id of this miscdevice
> > > + * @list: a linked list node representing this miscdevcie
> > > + * @static_mc_io: pointer to the static MC I/O object used by the
> > > +restool
> > > + * @dynamic_instance_count: number of dynamically created instances
> > > + * @static_instance_in_use: static instance is in use or not
> > > + * @mutex: mutex lock to serialze the operations
> > > + * @dev: root dprc associated with this miscdevice */ struct
> > > +restool_misc {
> > > + struct miscdevice misc;
> > > + dev_t miscdevt;
> > > + struct list_head list;
> > > + struct fsl_mc_io *static_mc_io;
> > > + uint32_t dynamic_instance_count;
> > > + bool static_instance_in_use;
> > > + struct mutex mutex;
> > > + struct device *dev;
> > > +};
> > > +
> > > +/*
> > > + * initialize a global list to link all
> > > + * the miscdevice nodes (struct restool_misc) */
> > > +LIST_HEAD(misc_list);
> >
> > static
> >
> > > +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;
> > > + }
> > > + }
> >
> > No synchronization on the list?
> >
> > > +
> > > + 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;
> > > + }
> >
> > Why is the first user handled differently from subsequent users? Does
> > each user really need its own portal, or can you just synchronize access?
>
> First user get the statically allocated portal.
> Second user need to dynamically allocate one.
> Some operations may take a long time (say 1 second) in MC object creation.
> In order to support asynchronous portal operation, We allocate portal for each
> user.
>
> Lijun
>
> >
> > -Scott
ÿôèº{.nÇ+‰·Ÿ®‰†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a¢f£¢·hšïêÿ‘êçz_è®\x03(階ŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨èÚ&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥
^ permalink raw reply [flat|nested] 17+ messages in thread