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

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

[Dan Carpenter]

Hello Zhu Lingshan,

The patch 79333575b8bd: "vDPA/ifcvf: implement shared IRQ feature"
from Feb 22, 2022, leads to the following Smatch static checker
warning:

	drivers/vdpa/ifcvf/ifcvf_main.c:306 ifcvf_request_config_irq()
	error: uninitialized symbol 'config_vector'.

drivers/vdpa/ifcvf/ifcvf_main.c
    287 static int ifcvf_request_config_irq(struct ifcvf_adapter *adapter)
    288 {
    289         struct pci_dev *pdev = adapter->pdev;
    290         struct ifcvf_hw *vf = &adapter->vf;
    291         int config_vector, ret;
    292 
    293         if (vf->msix_vector_status == MSIX_VECTOR_DEV_SHARED)
    294                 return 0;
    295 
    296         if (vf->msix_vector_status == MSIX_VECTOR_PER_VQ_AND_CONFIG)
    297                 /* vector 0 ~ vf->nr_vring for vqs, num vf->nr_vring vector for config interrupt */
    298                 config_vector = vf->nr_vring;

Set here.

    299 
    300         if (vf->msix_vector_status ==  MSIX_VECTOR_SHARED_VQ_AND_CONFIG)
    301                 /* vector 0 for vqs and 1 for config interrupt */
    302                 config_vector = 1;

And here.  But no else path.

    303 
    304         snprintf(vf->config_msix_name, 256, "ifcvf[%s]-config\n",
    305                  pci_name(pdev));
--> 306         vf->config_irq = pci_irq_vector(pdev, config_vector);
    307         ret = devm_request_irq(&pdev->dev, vf->config_irq,
    308                                ifcvf_config_changed, 0,
    309                                vf->config_msix_name, vf);
    310         if (ret) {
    311                 IFCVF_ERR(pdev, "Failed to request config irq\n");
    312                 goto err;
    313         }
    314 
    315         ret = ifcvf_set_config_vector(vf, config_vector);
    316         if (ret == VIRTIO_MSI_NO_VECTOR) {
    317                 IFCVF_ERR(pdev, "No msix vector for device config\n");
    318                 goto err;
    319         }
    320 
    321         return 0;
    322 err:
    323         ifcvf_free_irq(adapter);
    324 
    325         return -EFAULT;
    326 }

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

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

[Dan Carpenter]

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.

This code is not necessarily buggy.  Right now we have GCC uninitialized
variable warnings turned off so it also doesn't cause a build issue.
But I think we should try to work towards a future where we can
re-enable the GCC warning.  GCC catches a lot of stupid uninitialized
variable bugs and it's better if we can catch them earlier instead of
relying on the kbuild-bot.

regards,
dan carpenter

> 
> Thanks for your efforts!
> Zhu Lingshan
> 
> On 3/11/2022 5:00 PM, Dan Carpenter wrote:
> > Hello Zhu Lingshan,
> > 
> > The patch 79333575b8bd: "vDPA/ifcvf: implement shared IRQ feature"
> > from Feb 22, 2022, leads to the following Smatch static checker
> > warning:
> > 
> > 	drivers/vdpa/ifcvf/ifcvf_main.c:306 ifcvf_request_config_irq()
> > 	error: uninitialized symbol 'config_vector'.> > 
> > drivers/vdpa/ifcvf/ifcvf_main.c
> >      287 static int ifcvf_request_config_irq(struct ifcvf_adapter *adapter)
> >      288 {
> >      289         struct pci_dev *pdev = adapter->pdev;
> >      290         struct ifcvf_hw *vf = &adapter->vf;
> >      291         int config_vector, ret;
> >      292
> >      293         if (vf->msix_vector_status == MSIX_VECTOR_DEV_SHARED)
> >      294                 return 0;
> >      295
> >      296         if (vf->msix_vector_status == MSIX_VECTOR_PER_VQ_AND_CONFIG)
> >      297                 /* vector 0 ~ vf->nr_vring for vqs, num vf->nr_vring vector for config interrupt */
> >      298                 config_vector = vf->nr_vring;
> > 
> > Set here.
> > 
> >      299
> >      300         if (vf->msix_vector_status ==  MSIX_VECTOR_SHARED_VQ_AND_CONFIG)
> >      301                 /* vector 0 for vqs and 1 for config interrupt */
> >      302                 config_vector = 1;
> > 
> > And here.  But no else path.
> > 
> >      303
> >      304         snprintf(vf->config_msix_name, 256, "ifcvf[%s]-config\n",
> >      305                  pci_name(pdev));
> > --> 306         vf->config_irq = pci_irq_vector(pdev, config_vector);
> >      307         ret = devm_request_irq(&pdev->dev, vf->config_irq,
> >      308                                ifcvf_config_changed, 0,
> >      309                                vf->config_msix_name, vf);
> >      310         if (ret) {
> >      311                 IFCVF_ERR(pdev, "Failed to request config irq\n");
> >      312                 goto err;
> >      313         }
> >      314
> >      315         ret = ifcvf_set_config_vector(vf, config_vector);
> >      316         if (ret == VIRTIO_MSI_NO_VECTOR) {
> >      317                 IFCVF_ERR(pdev, "No msix vector for device config\n");
> >      318                 goto err;
> >      319         }
> >      320
> >      321         return 0;
> >      322 err:
> >      323         ifcvf_free_irq(adapter);
> >      324
> >      325         return -EFAULT;
> >      326 }
> > 
> > regards,
> > dan carpenter
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

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

[Dan Carpenter]

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

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

[Dan Carpenter]

What I really want is to re-enable GCC's uninitialized variable warning.

$ rm drivers/vdpa/ifcvf/ifcvf_main.o
$ make W=2 drivers/vdpa/ifcvf/ifcvf_main.o

It prints a ton of output but this is the relevant bit.

drivers/vdpa/ifcvf/ifcvf_main.c: In function ‘ifcvf_vdpa_set_status’:
drivers/vdpa/ifcvf/ifcvf_main.c:291:6: warning: ‘config_vector’ may be used uninitialized in this function [-Wmaybe-uninitialized]
  int config_vector, ret;
      ^~~~~~~~~~~~~

regards,
dan carpenter


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