* Coverity CID 142811: pci_set_vga_state() operands don't affect result
@ 2014-04-03 20:43 Bjorn Helgaas
2014-04-04 0:06 ` Yinghai Lu
2014-04-04 15:47 ` Bjorn Helgaas
0 siblings, 2 replies; 5+ messages in thread
From: Bjorn Helgaas @ 2014-04-03 20:43 UTC (permalink / raw)
To: Dave Airlie; +Cc: linux-pci
Coverity complains about this in drivers/pci/pci.c:
CID 142811 (#1 of 1): Operands don't affect result (CONSTANT_EXPRESSION_RESULT)
result_independent_of_operands: flags & (2U /* 1 << 1 */) &
(command_bits & 4294967292U /* ~(1 | 2) */) is always 0 regardless of
the values of its operands. This occurs as the logical operand of if.
4128 WARN_ON((flags & PCI_VGA_STATE_CHANGE_DECODES) &
(command_bits & ~(PCI_COMMAND_IO|PCI_COMMAND_MEMORY)));
This is a result of 3448a19da479 "vgaarb: use bridges to control VGA
routing where possible."
I wonder if that middle "&" was intended to be "&&"?
Bjorn
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: Coverity CID 142811: pci_set_vga_state() operands don't affect result
2014-04-03 20:43 Coverity CID 142811: pci_set_vga_state() operands don't affect result Bjorn Helgaas
@ 2014-04-04 0:06 ` Yinghai Lu
2014-04-04 0:12 ` David Airlie
2014-04-04 15:47 ` Bjorn Helgaas
1 sibling, 1 reply; 5+ messages in thread
From: Yinghai Lu @ 2014-04-04 0:06 UTC (permalink / raw)
To: Bjorn Helgaas; +Cc: Dave Airlie, linux-pci
On Thu, Apr 3, 2014 at 1:43 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> Coverity complains about this in drivers/pci/pci.c:
>
> CID 142811 (#1 of 1): Operands don't affect result (CONSTANT_EXPRESSION_RESULT)
> result_independent_of_operands: flags & (2U /* 1 << 1 */) &
> (command_bits & 4294967292U /* ~(1 | 2) */) is always 0 regardless of
> the values of its operands. This occurs as the logical operand of if.
>
> 4128 WARN_ON((flags & PCI_VGA_STATE_CHANGE_DECODES) &
> (command_bits & ~(PCI_COMMAND_IO|PCI_COMMAND_MEMORY)));
>
> This is a result of 3448a19da479 "vgaarb: use bridges to control VGA
> routing where possible."
>
> I wonder if that middle "&" was intended to be "&&"?
must be. otherwise always get 0.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: Coverity CID 142811: pci_set_vga_state() operands don't affect result
2014-04-04 0:06 ` Yinghai Lu
@ 2014-04-04 0:12 ` David Airlie
0 siblings, 0 replies; 5+ messages in thread
From: David Airlie @ 2014-04-04 0:12 UTC (permalink / raw)
To: Yinghai Lu; +Cc: Bjorn Helgaas, linux-pci
----- Original Message -----
> From: "Yinghai Lu" <yinghai@kernel.org>
> To: "Bjorn Helgaas" <bhelgaas@google.com>
> Cc: "Dave Airlie" <airlied@redhat.com>, linux-pci@vger.kernel.org
> Sent: Friday, 4 April, 2014 10:06:32 AM
> Subject: Re: Coverity CID 142811: pci_set_vga_state() operands don't affect result
>
> On Thu, Apr 3, 2014 at 1:43 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> > Coverity complains about this in drivers/pci/pci.c:
> >
> > CID 142811 (#1 of 1): Operands don't affect result
> > (CONSTANT_EXPRESSION_RESULT)
> > result_independent_of_operands: flags & (2U /* 1 << 1 */) &
> > (command_bits & 4294967292U /* ~(1 | 2) */) is always 0 regardless of
> > the values of its operands. This occurs as the logical operand of if.
> >
> > 4128 WARN_ON((flags & PCI_VGA_STATE_CHANGE_DECODES) &
> > (command_bits & ~(PCI_COMMAND_IO|PCI_COMMAND_MEMORY)));
> >
> > This is a result of 3448a19da479 "vgaarb: use bridges to control VGA
> > routing where possible."
> >
> > I wonder if that middle "&" was intended to be "&&"?
>
> must be. otherwise always get 0.
Yeah I'd agree :-)
Dave.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: Coverity CID 142811: pci_set_vga_state() operands don't affect result
2014-04-03 20:43 Coverity CID 142811: pci_set_vga_state() operands don't affect result Bjorn Helgaas
2014-04-04 0:06 ` Yinghai Lu
@ 2014-04-04 15:47 ` Bjorn Helgaas
2014-04-25 17:21 ` Bjorn Helgaas
1 sibling, 1 reply; 5+ messages in thread
From: Bjorn Helgaas @ 2014-04-04 15:47 UTC (permalink / raw)
To: Dave Airlie; +Cc: linux-pci
On Thu, Apr 03, 2014 at 02:43:05PM -0600, Bjorn Helgaas wrote:
> Coverity complains about this in drivers/pci/pci.c:
>
> CID 142811 (#1 of 1): Operands don't affect result (CONSTANT_EXPRESSION_RESULT)
> result_independent_of_operands: flags & (2U /* 1 << 1 */) &
> (command_bits & 4294967292U /* ~(1 | 2) */) is always 0 regardless of
> the values of its operands. This occurs as the logical operand of if.
>
> 4128 WARN_ON((flags & PCI_VGA_STATE_CHANGE_DECODES) &
> (command_bits & ~(PCI_COMMAND_IO|PCI_COMMAND_MEMORY)));
>
> This is a result of 3448a19da479 "vgaarb: use bridges to control VGA
> routing where possible."
>
> I wonder if that middle "&" was intended to be "&&"?
I propose the following patch for this. Unless there's objection, I'll
queue this for v3.16. I took the liberty of interpreting your email
responses as acks.
PCI: Fix incorrect vgaarb conditional in WARN_ON()
From: Bjorn Helgaas <bhelgaas@google.com>
3448a19da479 "vgaarb: use bridges to control VGA routing where possible"
added the "flags & PCI_VGA_STATE_CHANGE_DECODES" condition to an existing
WARN_ON(), but used bitwise AND (&) instead of logical AND (&&), so the
condition is never true. Replace with logical AND.
Found by Coverity (CID 142811).
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
Acked-by: Yinghai Lu <yinghai@kernel.org>
Acked-by: David Airlie <airlied@redhat.com>
---
drivers/pci/pci.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 7325d43bf030..39012831867e 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -4125,7 +4125,7 @@ int pci_set_vga_state(struct pci_dev *dev, bool decode,
u16 cmd;
int rc;
- WARN_ON((flags & PCI_VGA_STATE_CHANGE_DECODES) & (command_bits & ~(PCI_COMMAND_IO|PCI_COMMAND_MEMORY)));
+ WARN_ON((flags & PCI_VGA_STATE_CHANGE_DECODES) && (command_bits & ~(PCI_COMMAND_IO|PCI_COMMAND_MEMORY)));
/* ARCH specific VGA enables */
rc = pci_set_vga_state_arch(dev, decode, command_bits, flags);
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: Coverity CID 142811: pci_set_vga_state() operands don't affect result
2014-04-04 15:47 ` Bjorn Helgaas
@ 2014-04-25 17:21 ` Bjorn Helgaas
0 siblings, 0 replies; 5+ messages in thread
From: Bjorn Helgaas @ 2014-04-25 17:21 UTC (permalink / raw)
To: Dave Airlie; +Cc: linux-pci
On Fri, Apr 04, 2014 at 09:47:44AM -0600, Bjorn Helgaas wrote:
> On Thu, Apr 03, 2014 at 02:43:05PM -0600, Bjorn Helgaas wrote:
> > Coverity complains about this in drivers/pci/pci.c:
> >
> > CID 142811 (#1 of 1): Operands don't affect result (CONSTANT_EXPRESSION_RESULT)
> > result_independent_of_operands: flags & (2U /* 1 << 1 */) &
> > (command_bits & 4294967292U /* ~(1 | 2) */) is always 0 regardless of
> > the values of its operands. This occurs as the logical operand of if.
> >
> > 4128 WARN_ON((flags & PCI_VGA_STATE_CHANGE_DECODES) &
> > (command_bits & ~(PCI_COMMAND_IO|PCI_COMMAND_MEMORY)));
> >
> > This is a result of 3448a19da479 "vgaarb: use bridges to control VGA
> > routing where possible."
> >
> > I wonder if that middle "&" was intended to be "&&"?
>
> I propose the following patch for this. Unless there's objection, I'll
> queue this for v3.16. I took the liberty of interpreting your email
> responses as acks.
>
>
> PCI: Fix incorrect vgaarb conditional in WARN_ON()
>
> From: Bjorn Helgaas <bhelgaas@google.com>
>
> 3448a19da479 "vgaarb: use bridges to control VGA routing where possible"
> added the "flags & PCI_VGA_STATE_CHANGE_DECODES" condition to an existing
> WARN_ON(), but used bitwise AND (&) instead of logical AND (&&), so the
> condition is never true. Replace with logical AND.
>
> Found by Coverity (CID 142811).
>
> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> Acked-by: Yinghai Lu <yinghai@kernel.org>
> Acked-by: David Airlie <airlied@redhat.com>
I applied this to pci/misc for v3.16.
I didn't mark it for stable because the only effect of backporting it would
be to cause warnings in some new cases, without actually fixing any
problems.
> ---
> drivers/pci/pci.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 7325d43bf030..39012831867e 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -4125,7 +4125,7 @@ int pci_set_vga_state(struct pci_dev *dev, bool decode,
> u16 cmd;
> int rc;
>
> - WARN_ON((flags & PCI_VGA_STATE_CHANGE_DECODES) & (command_bits & ~(PCI_COMMAND_IO|PCI_COMMAND_MEMORY)));
> + WARN_ON((flags & PCI_VGA_STATE_CHANGE_DECODES) && (command_bits & ~(PCI_COMMAND_IO|PCI_COMMAND_MEMORY)));
>
> /* ARCH specific VGA enables */
> rc = pci_set_vga_state_arch(dev, decode, command_bits, flags);
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2014-04-25 17:21 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-04-03 20:43 Coverity CID 142811: pci_set_vga_state() operands don't affect result Bjorn Helgaas
2014-04-04 0:06 ` Yinghai Lu
2014-04-04 0:12 ` David Airlie
2014-04-04 15:47 ` Bjorn Helgaas
2014-04-25 17:21 ` Bjorn Helgaas
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.