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

[Dan Carpenter <dan.carpenter@oracle.com>]

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