[PATCH 4.19 regression fix 1/2] staging: vboxvideo: Fix IRQs no longer working

[PATCH 4.19 regression fix 1/2] staging: vboxvideo: Fix IRQs no longer working

[Hans de Goede]

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

[PATCH 4.19 regression fix 2/2] staging: vboxvideo: Change address of scanout buffer on page-flip

[Hans de Goede]

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

Re: [PATCH 4.19 regression fix 2/2] staging: vboxvideo: Change address of scanout buffer on page-flip

[Steve Longerbeam]

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)

Re: [PATCH 4.19 regression fix 2/2] staging: vboxvideo: Change address of scanout buffer on page-flip

[Hans de Goede]

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

Re: [PATCH 4.19 regression fix 1/2] staging: vboxvideo: Fix IRQs no longer working

[Nicholas Mc Guire]

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
> 

Re: [PATCH 4.19 regression fix 1/2] staging: vboxvideo: Fix IRQs no longer working

[Hans de Goede]

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
>>

Re: [PATCH 4.19 regression fix 1/2] staging: vboxvideo: Fix IRQs no longer working

[Dan Carpenter]

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

Re: [PATCH 4.19 regression fix 1/2] staging: vboxvideo: Fix IRQs no longer working

[Nicholas Mc Guire]

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
> >>

Re: [PATCH 4.19 regression fix 1/2] staging: vboxvideo: Fix IRQs no longer working

[Nicholas Mc Guire]

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

Re: [PATCH 4.19 regression fix 1/2] staging: vboxvideo: Fix IRQs no longer working

[Dan Carpenter]

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