On Sun, Nov 08, 2015 at 08:14:48AM +0200, Mike Rapoport wrote:
> The lynxfb_pci_probe always returned -ENODEV in case of error. Modify it
> so that actual error code will be propogated to the caller.
>
> Signed-off-by: Mike Rapoport <mike.rapoport@xxxxxxxxx>
This one introduces bugs. If framebuffer_alloc() fails then we return
success.
> ---
> drivers/staging/sm750fb/sm750.c | 20 ++++++++++----------
> 1 file changed, 10 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/staging/sm750fb/sm750.c b/drivers/staging/sm750fb/sm750.c
> index c80b11c..8f666c0 100644
> --- a/drivers/staging/sm750fb/sm750.c
> +++ b/drivers/staging/sm750fb/sm750.c
> @@ -1006,17 +1006,18 @@ static int lynxfb_pci_probe(struct pci_dev *pdev,
> struct fb_info *info[] = {NULL, NULL};
> struct sm750_dev *sm750_dev = NULL;
> int fbidx;
> + int err = -ENOMEM;
Don't do this.
>
> /* enable device */
> if (pci_enable_device(pdev)) {
Always propagate the error code. Error handling should be basically
mindless. There are a few rules to follow and you do it the same way
every time and it's easy to do correctly.
> pr_err("can not enable device.\n");
> - goto err_enable;
> + return -ENODEV;
return err;
> }
>
> sm750_dev = kzalloc(sizeof(*sm750_dev), GFP_KERNEL);
> if (!sm750_dev) {
> pr_err("Could not allocate memory for share.\n");
> - goto err_share;
> + goto err_alloc_dev;
err = -ENOMEM;
goto disable_pci;
The goto line should say what the goto does. The label name should
say what the label does similar to how a function name says what a
function does.
You could also write this as:
err = -ENOMEM;
sm750_dev = kzalloc(sizeof(*sm750_dev), GFP_KERNEL);
if (!sm750_dev)
goto disable_pci;
Remove the pr_err(), it just hurts readability.
Btw, it seems a little wrong to call pci_enable() at the start of this
function. Shouldn't that be one of the last things we do?
Actually, that's pretty much it so far as rules go.
1) Choose good label names so we don't have to scroll down to see what
a goto does.
2) Unwind in the reverse mirror order from how you allocated things. A
condition in the allocation code should have a condition in the
unwind code.
3) Don't forget to use the correct return code. Propagate it or set it
right there next to the function call so we don't have to scroll up
to see what the error code is.
regards,
dan carpenter
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
