Re: [bug report] vDPA/ifcvf: implement shared IRQ feature

From: Dan Carpenter <dan.carpenter@oracle.com>
To: "Zhu, Lingshan" <lingshan.zhu@intel.com>
Cc: virtualization@lists.linux-foundation.org
Subject: Re: [bug report] vDPA/ifcvf: implement shared IRQ feature
Date: Tue, 15 Mar 2022 11:54:03 +0300	[thread overview]
Message-ID: <20220315085403.GU3315@kadam> (raw)
In-Reply-To: <5a0462a2-8361-4b08-19b3-d4771e177764@intel.com>

On Tue, Mar 15, 2022 at 10:27:35AM +0800, Zhu, Lingshan wrote:
> 
> 
> On 3/14/2022 6:37 PM, Dan Carpenter wrote:
> > On Mon, Mar 14, 2022 at 10:22:03AM +0800, Zhu, Lingshan wrote:
> > > Hello Dan,
> > > 
> > > Thanks for your suggestions and this auto-testing efforts!
> > > On handling the vector for device config interrupt, there are three
> > > possibilities:
> > > (1)it has a dedicated vector(2)it shares a vector with datapath(3)no
> > > vectors.
> > > 
> > > So in these code below, it handles the three cases, or it should be -EINVAL,
> > > so IMHO we don't need
> > > an else there, just leave it -EINVAL.
> > I'm confused about why you're talking about -EINVAL...  There is no
> > -EINVAL in this function.
> Oh, sorry I didn't explain this well. It is a series of patches, in other
> patches, we initialize hw->config_irq = -EINVAL
> as a safe default value. In this function ifcvf_request_config_irq(),
> hw->config_irq is generated by request_config_irq().
> 
> Then config_vector matters, there are only three possible values the
> config_vector can be(listed above),
> depending on vf->msix_vector_status. And vf->msix_vector_status depends on
> how many MSI vectors we got,
> 
> so there are only three values it can take, IMHO, it is a complete set of
> the possibilities, we already
> handled "else" in ifcvf_request_irq().

Alright, fine.  Yes, I verified this and it's a false positive.  But GCC
will also warn about this code if we manage to enable the GCC warning
again.

If we make a chart like this:

              [looks correct] [ looks buggy ]
[ no warnings]     1                 2
  [ warnings ]     3                 4

Where you want to be is in box 1 where it looks correct and has no
warnings.  Boxes 2 and 3 are a second best option because if it doesn't
have static checker warnings, people won't notice it.  Or if the
warnings are obvious false postives they can skip over it quickly.

But this code is box 4 where it is a big waste of time for anyone
running a static checker.

I almost prefer actual bugs to code which is deliberately written to
fit in box 4.

regards,
dan carpenter
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization