[PATCH v3] staging: vboxvideo: Add vboxvideo to drivers/staging

[Hans de Goede]

Hi,

On 22-06-17 14:03, Dan Carpenter wrote:
 > This is obviously much better and easier to review.  I ended up
 > reviewing it like I would normal code instead of staging code.  My main
 > thing is could you please re-write the module init error handling so
 > that each function does its own error handling instead of just calling
 > vbox_driver_unload().  Doing it that way is always going to buggy and
 > impossible to review.

Thanks for the review I'm preparing a v4 addressing all your comments,
one small remark and one question about your remarks:

<mega-snip>

 >> +int vbox_irq_init(struct vbox_private *vbox)
 >> +{
 >> +	int ret;
 >> +
 >> +	vbox_update_mode_hints(vbox);
 >> +	ret = drm_irq_install(vbox->dev, vbox->dev->pdev->irq);
 >> +	if (unlikely(ret != 0)) {
 >
 >
 > This is a slow path so don't use likely/unlikely.  Just say:
 >
 > 	if (ret)
 >
 >> +		vbox_irq_fini(vbox);
 >
 > No need.  Btw, the error handling in this driver is very frustrating for
 > me.  There is a massive do-everything cleanup function we call if init
 > fails or we need to unload the module.  But here we are doing our own
 > cleanup so vbox_irq_fini() is called twice.  And, of course, if the
 > resource isn't needed for the entire lifetime of the module then we have
 > to do normal kernel style error handling.  And then there is
 > vbox_mode_fini() which is not implemented.  The not implemented function
 > is not a coincidence, I see that a lot when people start down the path
 > of writing do-everything-style error handling.
 >
 > My suggestion is to use normal kernel style error handling everywhere
 > because it's simpler in the end.  Every function cleans up after itself
 > so you never return half the allocations you wanted.  Choose the label
 > name to reflect what you are doing at the label.  You only need to
 > remember the most recent allocation instead of remembering every single
 > thing you allocated.
 >
 > 	one = alloc();
 > 	if (!one)
 > 		return -ENOMEM;
 >
 > 	two = alloc();
 > 	if (!two) {
 > 		rc = -ENOMEM;
 > 		goto free_one;
 > 	}
 >
 > 	three = alloc();
 > 	if (!three) {
 > 		rc = -ENOMEM;
 > 		goto free_two;
 > 	}
 >
 > It's really simple if you plod along one thing at time.

Ack, I had already converted some functions to this style,
also to avoid nested success checks leading to an indentation
ever further to the right. I will move more code over to
this style for v4.

 >> +		DRM_ERROR("Failed installing irq: %d\n", ret);
 >> +		return 1;
 >
 > Preserve the error code.  "return ret;"
 >
 >
 >> +	}
 >> +	INIT_WORK(&vbox->hotplug_work, vbox_hotplug_worker);
 >> +	vbox->isr_installed = true;
 >
 > The isr_installed variable is not needed.
 >
 >> +	return 0;
 >> +}
 >> +
 >> +void vbox_irq_fini(struct vbox_private *vbox)
 >> +{
 >> +	if (vbox->isr_installed) {
 >> +		drm_irq_uninstall(vbox->dev);
 >> +		flush_work(&vbox->hotplug_work);
 >
 > These should be freed in the reverse order from how they were allocated
 > so I don't have to audit for use after frees or whatever.

You're right in principle, but your example is wrong:

 > void vbox_irq_fini(struct vbox_private *vbox)
 > {
 > 	flush_work(&vbox->hotplug_work);
 > 	drm_irq_uninstall(vbox->dev);
 > }

The irq can schedule the work, so we need to uninstall it
and then wait for any pending work to finish, so the original
fini order is right, the init order is wrong however. So I've
fixed the init order to be the reverse of the fini code.

<more snip>

 >> +int vbox_dumb_create(struct drm_file *file,
 >> +		     struct drm_device *dev, struct drm_mode_create_dumb *args)
 >> +{
 >> +	int ret;
 >> +	struct drm_gem_object *gobj;
 >> +	u32 handle;
 >> +
 >> +	args->pitch = args->width * ((args->bpp + 7) / 8);
 >> +	args->size = args->pitch * args->height;
 >> +
 >> +	ret = vbox_gem_create(dev, args->size, false, &gobj);
 >> +	if (ret)
 >> +		return ret;
 >> +
 >> +	ret = drm_gem_handle_create(file, gobj, &handle);
 >> +	drm_gem_object_unreference_unlocked(gobj);
 >> +	if (ret)
 >> +		return ret;
 >
 > This is a resource leak.

What makes you say that? Note the unreference done before the
ret check. I could be missing something here, but I think this is fine ?

<2nd mega-snip>

Regards,

Hans