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

[Nicholas Mc Guire <der.herr@hofr.at>]

On Tue, Sep 11, 2018 at 10:20:41AM +0300, Dan Carpenter wrote:
> 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.
>
Probably true - there is quite a bit of incorrect type issues in the
kernel and there are a cases of comparing to e.g. <= 0 for signed
types is used, so I personally prefere if the check allows type
inference - if I see a "ret < 0" it can be infered that the type must
be signed and an unsigned is an error while for !0 case does not allow
such inference.

Anyway - as noted the patch seems correct with respect to the intent and
if the general preference is for "if (ret)" then no objections.

thanks for the clarification !

hofrat