[PATCH 4.19 regression fix 1/2] staging: vboxvideo: Fix IRQs no longer working

[Dan Carpenter]

On Tue, Sep 11, 2018 at 06:48:27AM +0000, Nicholas Mc Guire wrote:
> On Mon, Sep 10, 2018 at 08:30:38PM +0200, Hans de Goede wrote:
> > Commit 1daddbc8dec5 ("staging: vboxvideo: Update driver to use
> > drm_dev_register.") replaced the obsolere drm_get_pci_dev() with
> > normal pci probe and remove functions.
> > 
> > But the new vbox_pci_probe() is missing a pci_enable_device() call,
> > causing interrupts to not be delivered. This causes resizes of the
> > vm window to not get seen by the drm/kms code.
> > 
> > This commit adds the missing pci_enable_device() call, fixing this.
> 
> pci_enable_device is the wrapper to pci_enable_device_flags() the later
> return < 0 on error - so while the check for if(ret) will do the right
> think here I think it would be prefereable to explicidly use if (ret < 0)
> as all error values pci_enable_device_flags() returns are negative.
> 

It could go either way, I think.  "if (ret) " is sort of as explicit as
"if (ret < 0) " when you consider the false side.  When I see "if (ret)"
then I know the code returns negative error codes and zero, but when I
see "if (ret < 0)" then I think maybe this returns positive non-zero
values as well.

As a static analysis person, the "if (ret)" style is easier for me.
Sometimes Smatch doesn't know what a function returns.  Maybe the error
code comes from a different thread and Smatch doesn't understand
threads.  So then when people use "if (ret)" Smatch knows that non-zero
means *param1 is not initialized.  Then the caller does "if (ret < 0)"
that means that positive non-zero values are not handled so let's print
an uninitialized variable warning.  Just to spell it out a little more,
the error code won't be printed for "if (ret)" because negatives are a
subset of non-zero.

Of course, if you do it consistently there won't be a warning message.
I never see the consistent subsystems, so I don't know if they exist.

regards,
dan carpenter