* [PATCH 4.19 regression fix 1/2] staging: vboxvideo: Fix IRQs no longer working
@ 2018-09-10 18:30 Hans de Goede
2018-09-10 18:30 ` [PATCH 4.19 regression fix 2/2] staging: vboxvideo: Change address of scanout buffer on page-flip Hans de Goede
2018-09-11 6:48 ` [PATCH 4.19 regression fix 1/2] staging: vboxvideo: Fix IRQs no longer working Nicholas Mc Guire
0 siblings, 2 replies; 10+ messages in thread
From: Hans de Goede @ 2018-09-10 18:30 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: devel, Michael Thayer, dri-devel, Hans de Goede,
Nicholas Mc Guire, Fabio Rafael da Rosa
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(+)
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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH 4.19 regression fix 2/2] staging: vboxvideo: Change address of scanout buffer on page-flip
2018-09-10 18:30 [PATCH 4.19 regression fix 1/2] staging: vboxvideo: Fix IRQs no longer working Hans de Goede
@ 2018-09-10 18:30 ` Hans de Goede
2018-09-10 23:08 ` Steve Longerbeam
2018-09-11 6:48 ` [PATCH 4.19 regression fix 1/2] staging: vboxvideo: Fix IRQs no longer working Nicholas Mc Guire
1 sibling, 1 reply; 10+ messages in thread
From: Hans de Goede @ 2018-09-10 18:30 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: devel, Hans de Goede, Michael Thayer, Steve Longerbeam, dri-devel
Commit 2408898e3b6c ("staging: vboxvideo: Add page-flip support") only
calls vbox_crtc_do_set_base() on page-flips, but despite that function's
name it only pins the new fb, unpins the old fb and sets
vbox_crtc->fb_offset. It does not program the hardware to scan out at the
new vbox_crtc->fb_offset value.
This was causing only every other frame (assuming page-flipping between 2
buffers) to be shown since we kept scanning out of the old (now unpinned!)
buffer.
This commit fixes this by adding code to vbox_crtc_page_flip() to tell
the hardware to scanout from the new fb_offset.
Fixes: 2408898e3b6c ("staging: vboxvideo: Add page-flip support")
Cc: Steve Longerbeam <steve_longerbeam@mentor.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
drivers/staging/vboxvideo/vbox_mode.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/drivers/staging/vboxvideo/vbox_mode.c b/drivers/staging/vboxvideo/vbox_mode.c
index a83eac8668d0..79836c8fb909 100644
--- a/drivers/staging/vboxvideo/vbox_mode.c
+++ b/drivers/staging/vboxvideo/vbox_mode.c
@@ -323,6 +323,11 @@ static int vbox_crtc_page_flip(struct drm_crtc *crtc,
if (rc)
return rc;
+ mutex_lock(&vbox->hw_mutex);
+ vbox_set_view(crtc);
+ vbox_do_modeset(crtc, &crtc->mode);
+ mutex_unlock(&vbox->hw_mutex);
+
spin_lock_irqsave(&drm->event_lock, flags);
if (event)
--
2.19.0.rc0
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 4.19 regression fix 2/2] staging: vboxvideo: Change address of scanout buffer on page-flip
2018-09-10 18:30 ` [PATCH 4.19 regression fix 2/2] staging: vboxvideo: Change address of scanout buffer on page-flip Hans de Goede
@ 2018-09-10 23:08 ` Steve Longerbeam
2018-09-11 6:28 ` Hans de Goede
0 siblings, 1 reply; 10+ messages in thread
From: Steve Longerbeam @ 2018-09-10 23:08 UTC (permalink / raw)
To: Hans de Goede, Greg Kroah-Hartman; +Cc: devel, Michael Thayer, dri-devel
Hi Hans,
On 09/10/2018 11:30 AM, Hans de Goede wrote:
> Commit 2408898e3b6c ("staging: vboxvideo: Add page-flip support") only
> calls vbox_crtc_do_set_base() on page-flips, but despite that function's
> name it only pins the new fb, unpins the old fb and sets
> vbox_crtc->fb_offset. It does not program the hardware to scan out at the
> new vbox_crtc->fb_offset value.
Has that always been the case of vbox_crtc_do_set_base()? Or was
there a recent commit that changed that behavior?
I tested this patch using a Weston EGL mock navigation test app around
4.14 time-frame, that exercises page flip and it was scanning out the
new fb, but maybe what I was looking at was a scan-out of an old/now stale
fb from a previous page-flip.
In any case thanks for fixing.
Steve
> This was causing only every other frame (assuming page-flipping between 2
> buffers) to be shown since we kept scanning out of the old (now unpinned!)
> buffer.
>
> This commit fixes this by adding code to vbox_crtc_page_flip() to tell
> the hardware to scanout from the new fb_offset.
>
> Fixes: 2408898e3b6c ("staging: vboxvideo: Add page-flip support")
> Cc: Steve Longerbeam <steve_longerbeam@mentor.com>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
> drivers/staging/vboxvideo/vbox_mode.c | 5 +++++
> 1 file changed, 5 insertions(+)
>
> diff --git a/drivers/staging/vboxvideo/vbox_mode.c b/drivers/staging/vboxvideo/vbox_mode.c
> index a83eac8668d0..79836c8fb909 100644
> --- a/drivers/staging/vboxvideo/vbox_mode.c
> +++ b/drivers/staging/vboxvideo/vbox_mode.c
> @@ -323,6 +323,11 @@ static int vbox_crtc_page_flip(struct drm_crtc *crtc,
> if (rc)
> return rc;
>
> + mutex_lock(&vbox->hw_mutex);
> + vbox_set_view(crtc);
> + vbox_do_modeset(crtc, &crtc->mode);
> + mutex_unlock(&vbox->hw_mutex);
> +
> spin_lock_irqsave(&drm->event_lock, flags);
>
> if (event)
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 4.19 regression fix 2/2] staging: vboxvideo: Change address of scanout buffer on page-flip
2018-09-10 23:08 ` Steve Longerbeam
@ 2018-09-11 6:28 ` Hans de Goede
0 siblings, 0 replies; 10+ messages in thread
From: Hans de Goede @ 2018-09-11 6:28 UTC (permalink / raw)
To: Steve Longerbeam, Greg Kroah-Hartman; +Cc: devel, Michael Thayer, dri-devel
Hi,
On 11-09-18 01:08, Steve Longerbeam wrote:
> Hi Hans,
>
>
> On 09/10/2018 11:30 AM, Hans de Goede wrote:
>> Commit 2408898e3b6c ("staging: vboxvideo: Add page-flip support") only
>> calls vbox_crtc_do_set_base() on page-flips, but despite that function's
>> name it only pins the new fb, unpins the old fb and sets
>> vbox_crtc->fb_offset. It does not program the hardware to scan out at the
>> new vbox_crtc->fb_offset value.
>
> Has that always been the case of vbox_crtc_do_set_base()? Or was
> there a recent commit that changed that behavior?
I believe that always was the case.
> I tested this patch using a Weston EGL mock navigation test app around
> 4.14 time-frame, that exercises page flip and it was scanning out the
> new fb, but maybe what I was looking at was a scan-out of an old/now stale
> fb from a previous page-flip.
Yes I think that is what you saw, this was not easy to notice, I noticed
that when typing slowly my letters would always appear 2 at a time, then
I deliberately typed: 'a' waited a couple of seconds and the 'a' would
not show up until I also typed 'b' and then both showed up at once.
This was with GNOME3 as Wayland compositor, which I believe stops
sending new frames when nothing changes which makes this very noticeable :)
> In any case thanks for fixing.
You are welcome.
Regards,
Hans
>
> Steve
>
>> This was causing only every other frame (assuming page-flipping between 2
>> buffers) to be shown since we kept scanning out of the old (now unpinned!)
>> buffer.
>>
>> This commit fixes this by adding code to vbox_crtc_page_flip() to tell
>> the hardware to scanout from the new fb_offset.
>>
>> Fixes: 2408898e3b6c ("staging: vboxvideo: Add page-flip support")
>> Cc: Steve Longerbeam <steve_longerbeam@mentor.com>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>> drivers/staging/vboxvideo/vbox_mode.c | 5 +++++
>> 1 file changed, 5 insertions(+)
>>
>> diff --git a/drivers/staging/vboxvideo/vbox_mode.c b/drivers/staging/vboxvideo/vbox_mode.c
>> index a83eac8668d0..79836c8fb909 100644
>> --- a/drivers/staging/vboxvideo/vbox_mode.c
>> +++ b/drivers/staging/vboxvideo/vbox_mode.c
>> @@ -323,6 +323,11 @@ static int vbox_crtc_page_flip(struct drm_crtc *crtc,
>> if (rc)
>> return rc;
>> + mutex_lock(&vbox->hw_mutex);
>> + vbox_set_view(crtc);
>> + vbox_do_modeset(crtc, &crtc->mode);
>> + mutex_unlock(&vbox->hw_mutex);
>> +
>> spin_lock_irqsave(&drm->event_lock, flags);
>> if (event)
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 4.19 regression fix 1/2] staging: vboxvideo: Fix IRQs no longer working
2018-09-10 18:30 [PATCH 4.19 regression fix 1/2] staging: vboxvideo: Fix IRQs no longer working Hans de Goede
2018-09-10 18:30 ` [PATCH 4.19 regression fix 2/2] staging: vboxvideo: Change address of scanout buffer on page-flip Hans de Goede
@ 2018-09-11 6:48 ` Nicholas Mc Guire
2018-09-11 6:53 ` Hans de Goede
2018-09-11 7:20 ` Dan Carpenter
1 sibling, 2 replies; 10+ messages in thread
From: Nicholas Mc Guire @ 2018-09-11 6:48 UTC (permalink / raw)
To: Hans de Goede; +Cc: devel, Greg Kroah-Hartman, Michael Thayer, dri-devel
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
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 4.19 regression fix 1/2] staging: vboxvideo: Fix IRQs no longer working
2018-09-11 6:48 ` [PATCH 4.19 regression fix 1/2] staging: vboxvideo: Fix IRQs no longer working Nicholas Mc Guire
@ 2018-09-11 6:53 ` Hans de Goede
2018-09-11 7:41 ` Nicholas Mc Guire
2018-09-11 7:20 ` Dan Carpenter
1 sibling, 1 reply; 10+ messages in thread
From: Hans de Goede @ 2018-09-11 6:53 UTC (permalink / raw)
To: Nicholas Mc Guire; +Cc: devel, Greg Kroah-Hartman, Michael Thayer, dri-devel
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
>>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 4.19 regression fix 1/2] staging: vboxvideo: Fix IRQs no longer working
2018-09-11 6:48 ` [PATCH 4.19 regression fix 1/2] staging: vboxvideo: Fix IRQs no longer working Nicholas Mc Guire
2018-09-11 6:53 ` Hans de Goede
@ 2018-09-11 7:20 ` Dan Carpenter
2018-09-11 7:56 ` Nicholas Mc Guire
1 sibling, 1 reply; 10+ messages in thread
From: Dan Carpenter @ 2018-09-11 7:20 UTC (permalink / raw)
To: Nicholas Mc Guire
Cc: devel, Hans de Goede, Michael Thayer, dri-devel, Greg Kroah-Hartman
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 4.19 regression fix 1/2] staging: vboxvideo: Fix IRQs no longer working
2018-09-11 6:53 ` Hans de Goede
@ 2018-09-11 7:41 ` Nicholas Mc Guire
2018-09-11 7:57 ` Dan Carpenter
0 siblings, 1 reply; 10+ messages in thread
From: Nicholas Mc Guire @ 2018-09-11 7:41 UTC (permalink / raw)
To: Hans de Goede; +Cc: devel, Greg Kroah-Hartman, Michael Thayer, dri-devel
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
> >>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 4.19 regression fix 1/2] staging: vboxvideo: Fix IRQs no longer working
2018-09-11 7:20 ` Dan Carpenter
@ 2018-09-11 7:56 ` Nicholas Mc Guire
0 siblings, 0 replies; 10+ messages in thread
From: Nicholas Mc Guire @ 2018-09-11 7:56 UTC (permalink / raw)
To: Dan Carpenter
Cc: devel, Hans de Goede, Michael Thayer, dri-devel, Greg Kroah-Hartman
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 4.19 regression fix 1/2] staging: vboxvideo: Fix IRQs no longer working
2018-09-11 7:41 ` Nicholas Mc Guire
@ 2018-09-11 7:57 ` Dan Carpenter
0 siblings, 0 replies; 10+ messages in thread
From: Dan Carpenter @ 2018-09-11 7:57 UTC (permalink / raw)
To: Nicholas Mc Guire
Cc: devel, Hans de Goede, Michael Thayer, dri-devel, Greg Kroah-Hartman
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
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2018-09-11 7:57 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-09-10 18:30 [PATCH 4.19 regression fix 1/2] staging: vboxvideo: Fix IRQs no longer working Hans de Goede
2018-09-10 18:30 ` [PATCH 4.19 regression fix 2/2] staging: vboxvideo: Change address of scanout buffer on page-flip Hans de Goede
2018-09-10 23:08 ` Steve Longerbeam
2018-09-11 6:28 ` Hans de Goede
2018-09-11 6:48 ` [PATCH 4.19 regression fix 1/2] staging: vboxvideo: Fix IRQs no longer working Nicholas Mc Guire
2018-09-11 6:53 ` Hans de Goede
2018-09-11 7:41 ` Nicholas Mc Guire
2018-09-11 7:57 ` Dan Carpenter
2018-09-11 7:20 ` Dan Carpenter
2018-09-11 7:56 ` Nicholas Mc Guire
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.