On Tue, Sep 11, 2018 at 07:41:10AM +0000, Nicholas Mc Guire wrote:
On Tue, Sep 11, 2018 at 08:53:35AM +0200, Hans de Goede wrote:
Hi,
On 11-09-18 08:48, 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.
The use of "if (ret)" checks for functions which return 0 on success negative value on error is a standard pattern in the kernel, so I would prefer to keep this as is.
Well as noted it does the right thing - just checking the use of pci_enable_device() in the existing drivers it seems that it is roughly balanced between checking < 0 and !0. The rational for < 0 would be that the negative return values mandate a signed type, whilc !0 does not and if someone then uses and unsigned type the error case would return as success while the < 0 would be detected at compile time (or other static code checkers).
If the function returns int but ret is an unsigned int and we do "if (ret)", then yes we don't print a static checker warning. But the code works perfectly so it doesn't matter.
I'm going to publish some code soon which will complain if a function returns specific negatives and you save it to an unsigned type which is smaller than an int.
regards, dan carpenter