| Message ID | 20180910183039.3339-1-hdegoede@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [4.19,regression,fix,1/2] staging: vboxvideo: Fix IRQs no longer working | expand |
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. > > Fixes: 1daddbc8dec5 ("staging: vboxvideo: Update driver to use ...") > Cc: Fabio Rafael da Rosa <fdr@pid42.net> > Cc: Nicholas Mc Guire <der.herr@hofr.at> > Signed-off-by: Hans de Goede <hdegoede@redhat.com> Reviewed-by: Nicholas Mc Guire <der.herr@hofr.at> > --- > 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 >
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. >> Fixes: 1daddbc8dec5 ("staging: vboxvideo: Update driver to use ...") >> Cc: Fabio Rafael da Rosa <fdr@pid42.net> >> Cc: Nicholas Mc Guire <der.herr@hofr.at> >> Signed-off-by: Hans de Goede <hdegoede@redhat.com> > Reviewed-by: Nicholas Mc Guire <der.herr@hofr.at> 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 >>
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
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 <fdr@pid42.net> > >>Cc: Nicholas Mc Guire <der.herr@hofr.at> > >>Signed-off-by: Hans de Goede <hdegoede@redhat.com> > >Reviewed-by: Nicholas Mc Guire <der.herr@hofr.at> > > 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 > >>
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
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
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;
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. Fixes: 1daddbc8dec5 ("staging: vboxvideo: Update driver to use ...") Cc: Fabio Rafael da Rosa <fdr@pid42.net> Cc: Nicholas Mc Guire <der.herr@hofr.at> Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- drivers/staging/vboxvideo/vbox_drv.c | 7 +++++++ 1 file changed, 7 insertions(+)