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

2022-03-15 Thread 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

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

2022-03-15 Thread 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

2022-03-14 Thread 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 = >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(>dev, vf->config_irq,
> >  308ifcvf_config_changed, 0,
> >  309vf->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

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

2022-03-11 Thread 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 = >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(>dev, vf->config_irq,
308ifcvf_config_changed, 0,
309vf->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