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