From mboxrd@z Thu Jan 1 00:00:00 1970 From: Nicholas Mc Guire Subject: Re: [PATCH 4.19 regression fix 1/2] staging: vboxvideo: Fix IRQs no longer working Date: Tue, 11 Sep 2018 07:41:10 +0000 Message-ID: <20180911074110.GA14401@osadl.at> References: <20180910183039.3339-1-hdegoede@redhat.com> <20180911064827.GA4837@osadl.at> <8861019b-1ae3-d53b-a16e-cc5906f207ec@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Content-Disposition: inline In-Reply-To: <8861019b-1ae3-d53b-a16e-cc5906f207ec@redhat.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: driverdev-devel-bounces@linuxdriverproject.org Sender: "devel" To: Hans de Goede Cc: devel@driverdev.osuosl.org, Greg Kroah-Hartman , Michael Thayer , dri-devel@lists.freedesktop.org List-Id: dri-devel@lists.freedesktop.org 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). thx! hofrat > >>Fixes: 1daddbc8dec5 ("staging: vboxvideo: Update driver to use ...") > >>Cc: Fabio Rafael da Rosa > >>Cc: Nicholas Mc Guire > >>Signed-off-by: Hans de Goede > >Reviewed-by: Nicholas Mc Guire > > Thanks. > > Regards, > > Hans > > > > > >>--- > >> drivers/staging/vboxvideo/vbox_drv.c | 7 +++++++ > >> 1 file changed, 7 insertions(+) > >> > >>diff --git a/drivers/staging/vboxvideo/vbox_drv.c b/drivers/staging/vboxvideo/vbox_drv.c > >>index da92c493f157..69cc508af1bc 100644 > >>--- a/drivers/staging/vboxvideo/vbox_drv.c > >>+++ b/drivers/staging/vboxvideo/vbox_drv.c > >>@@ -59,6 +59,11 @@ static int vbox_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent) > >> ret = PTR_ERR(dev); > >> goto err_drv_alloc; > >> } > >>+ > >>+ ret = pci_enable_device(pdev); > >>+ if (ret) > >>+ goto err_pci_enable; > >>+ > >> dev->pdev = pdev; > >> pci_set_drvdata(pdev, dev); > >>@@ -75,6 +80,8 @@ static int vbox_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent) > >> err_drv_dev_register: > >> vbox_driver_unload(dev); > >> err_vbox_driver_load: > >>+ pci_disable_device(pdev); > >>+ err_pci_enable: > >> drm_dev_put(dev); > >> err_drv_alloc: > >> return ret; > >>-- > >>2.19.0.rc0 > >>