* [PATCH] vDPA/ifcvf: match pointer check to use
@ 2022-03-15 12:41 trix
2022-03-15 13:23 ` Michael S. Tsirkin
2022-03-15 13:28 ` Michael S. Tsirkin
0 siblings, 2 replies; 6+ messages in thread
From: trix @ 2022-03-15 12:41 UTC (permalink / raw)
To: mst, jasowang, nathan, ndesaulniers, lingshan.zhu, sgarzare, xieyongji
Cc: virtualization, linux-kernel, llvm, Tom Rix
From: Tom Rix <trix@redhat.com>
Clang static analysis reports this issue
ifcvf_main.c:49:4: warning: Called function
pointer is null (null dereference)
vf->vring->cb.callback(vring->cb.private);
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
The check
vring = &vf->vring[i];
if (vring->cb.callback)
Does not match the use. Change dereference so they match.
Fixes: 79333575b8bd ("vDPA/ifcvf: implement shared IRQ feature")
Signed-off-by: Tom Rix <trix@redhat.com>
---
drivers/vdpa/ifcvf/ifcvf_main.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/vdpa/ifcvf/ifcvf_main.c b/drivers/vdpa/ifcvf/ifcvf_main.c
index 3b48e717e89f7..4366320fb68d3 100644
--- a/drivers/vdpa/ifcvf/ifcvf_main.c
+++ b/drivers/vdpa/ifcvf/ifcvf_main.c
@@ -46,7 +46,7 @@ static irqreturn_t ifcvf_vqs_reused_intr_handler(int irq, void *arg)
for (i = 0; i < vf->nr_vring; i++) {
vring = &vf->vring[i];
if (vring->cb.callback)
- vf->vring->cb.callback(vring->cb.private);
+ vring->cb.callback(vring->cb.private);
}
return IRQ_HANDLED;
--
2.26.3
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH] vDPA/ifcvf: match pointer check to use 2022-03-15 12:41 [PATCH] vDPA/ifcvf: match pointer check to use trix @ 2022-03-15 13:23 ` Michael S. Tsirkin 2022-03-15 13:28 ` Michael S. Tsirkin 1 sibling, 0 replies; 6+ messages in thread From: Michael S. Tsirkin @ 2022-03-15 13:23 UTC (permalink / raw) To: trix Cc: jasowang, nathan, ndesaulniers, lingshan.zhu, sgarzare, xieyongji, virtualization, linux-kernel, llvm On Tue, Mar 15, 2022 at 05:41:30AM -0700, trix@redhat.com wrote: > From: Tom Rix <trix@redhat.com> > > Clang static analysis reports this issue > ifcvf_main.c:49:4: warning: Called function > pointer is null (null dereference) > vf->vring->cb.callback(vring->cb.private); > ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > The check > vring = &vf->vring[i]; > if (vring->cb.callback) > > Does not match the use. Change dereference so they match. > > Fixes: 79333575b8bd ("vDPA/ifcvf: implement shared IRQ feature") > Signed-off-by: Tom Rix <trix@redhat.com> > --- > drivers/vdpa/ifcvf/ifcvf_main.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/vdpa/ifcvf/ifcvf_main.c b/drivers/vdpa/ifcvf/ifcvf_main.c > index 3b48e717e89f7..4366320fb68d3 100644 > --- a/drivers/vdpa/ifcvf/ifcvf_main.c > +++ b/drivers/vdpa/ifcvf/ifcvf_main.c > @@ -46,7 +46,7 @@ static irqreturn_t ifcvf_vqs_reused_intr_handler(int irq, void *arg) > for (i = 0; i < vf->nr_vring; i++) { > vring = &vf->vring[i]; > if (vring->cb.callback) > - vf->vring->cb.callback(vring->cb.private); > + vring->cb.callback(vring->cb.private); > } > > return IRQ_HANDLED; Oh, absolutely. In fact vf->vring->cb.callback is just vf->vring[0].cb.callback so it's wrong for any ring except 0. Does not make sense. So how did it work in testing then? No idea. Zhu Lingshan, care to comment? > -- > 2.26.3 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] vDPA/ifcvf: match pointer check to use 2022-03-15 12:41 [PATCH] vDPA/ifcvf: match pointer check to use trix 2022-03-15 13:23 ` Michael S. Tsirkin @ 2022-03-15 13:28 ` Michael S. Tsirkin 2022-03-15 15:03 ` Tom Rix 1 sibling, 1 reply; 6+ messages in thread From: Michael S. Tsirkin @ 2022-03-15 13:28 UTC (permalink / raw) To: trix Cc: jasowang, nathan, ndesaulniers, lingshan.zhu, sgarzare, xieyongji, virtualization, linux-kernel, llvm On Tue, Mar 15, 2022 at 05:41:30AM -0700, trix@redhat.com wrote: > From: Tom Rix <trix@redhat.com> > > Clang static analysis reports this issue > ifcvf_main.c:49:4: warning: Called function > pointer is null (null dereference) > vf->vring->cb.callback(vring->cb.private); > ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > The check > vring = &vf->vring[i]; > if (vring->cb.callback) > > Does not match the use. Change dereference so they match. > > Fixes: 79333575b8bd ("vDPA/ifcvf: implement shared IRQ feature") Thanks a lot! I squashed this into the offending patch - no point in breaking bisect. Pushed to linux. However I'm now having second thoughts about applying that patchset - I'd like soma analysis explaining how this got through testing. > Signed-off-by: Tom Rix <trix@redhat.com> > --- > drivers/vdpa/ifcvf/ifcvf_main.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/vdpa/ifcvf/ifcvf_main.c b/drivers/vdpa/ifcvf/ifcvf_main.c > index 3b48e717e89f7..4366320fb68d3 100644 > --- a/drivers/vdpa/ifcvf/ifcvf_main.c > +++ b/drivers/vdpa/ifcvf/ifcvf_main.c > @@ -46,7 +46,7 @@ static irqreturn_t ifcvf_vqs_reused_intr_handler(int irq, void *arg) > for (i = 0; i < vf->nr_vring; i++) { > vring = &vf->vring[i]; > if (vring->cb.callback) > - vf->vring->cb.callback(vring->cb.private); > + vring->cb.callback(vring->cb.private); > } > > return IRQ_HANDLED; > -- > 2.26.3 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] vDPA/ifcvf: match pointer check to use 2022-03-15 13:28 ` Michael S. Tsirkin @ 2022-03-15 15:03 ` Tom Rix 2022-03-15 15:15 ` Michael S. Tsirkin 0 siblings, 1 reply; 6+ messages in thread From: Tom Rix @ 2022-03-15 15:03 UTC (permalink / raw) To: Michael S. Tsirkin Cc: jasowang, nathan, ndesaulniers, lingshan.zhu, sgarzare, xieyongji, virtualization, linux-kernel, llvm On 3/15/22 6:28 AM, Michael S. Tsirkin wrote: > On Tue, Mar 15, 2022 at 05:41:30AM -0700, trix@redhat.com wrote: >> From: Tom Rix <trix@redhat.com> >> >> Clang static analysis reports this issue >> ifcvf_main.c:49:4: warning: Called function >> pointer is null (null dereference) >> vf->vring->cb.callback(vring->cb.private); >> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ >> >> The check >> vring = &vf->vring[i]; >> if (vring->cb.callback) >> >> Does not match the use. Change dereference so they match. >> >> Fixes: 79333575b8bd ("vDPA/ifcvf: implement shared IRQ feature") > Thanks a lot! I squashed this into the offending patch - no point in > breaking bisect. Pushed to linux. However I'm now > having second thoughts about applying that patchset - I'd like > soma analysis explaining how this got through testing. static analysis is something i do treewide. There are currently ~2500 issues in linux-next, do not panic! many are false positives. It is pretty easy to setup and once you have a baseline you can filter only your files. Tom >> Signed-off-by: Tom Rix <trix@redhat.com> >> --- >> drivers/vdpa/ifcvf/ifcvf_main.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/vdpa/ifcvf/ifcvf_main.c b/drivers/vdpa/ifcvf/ifcvf_main.c >> index 3b48e717e89f7..4366320fb68d3 100644 >> --- a/drivers/vdpa/ifcvf/ifcvf_main.c >> +++ b/drivers/vdpa/ifcvf/ifcvf_main.c >> @@ -46,7 +46,7 @@ static irqreturn_t ifcvf_vqs_reused_intr_handler(int irq, void *arg) >> for (i = 0; i < vf->nr_vring; i++) { >> vring = &vf->vring[i]; >> if (vring->cb.callback) >> - vf->vring->cb.callback(vring->cb.private); >> + vring->cb.callback(vring->cb.private); >> } >> >> return IRQ_HANDLED; >> -- >> 2.26.3 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] vDPA/ifcvf: match pointer check to use 2022-03-15 15:03 ` Tom Rix @ 2022-03-15 15:15 ` Michael S. Tsirkin 2022-03-16 2:25 ` Zhu, Lingshan 0 siblings, 1 reply; 6+ messages in thread From: Michael S. Tsirkin @ 2022-03-15 15:15 UTC (permalink / raw) To: Tom Rix Cc: jasowang, nathan, ndesaulniers, lingshan.zhu, sgarzare, xieyongji, virtualization, linux-kernel, llvm On Tue, Mar 15, 2022 at 08:03:26AM -0700, Tom Rix wrote: > > On 3/15/22 6:28 AM, Michael S. Tsirkin wrote: > > On Tue, Mar 15, 2022 at 05:41:30AM -0700, trix@redhat.com wrote: > > > From: Tom Rix <trix@redhat.com> > > > > > > Clang static analysis reports this issue > > > ifcvf_main.c:49:4: warning: Called function > > > pointer is null (null dereference) > > > vf->vring->cb.callback(vring->cb.private); > > > ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > > > > > The check > > > vring = &vf->vring[i]; > > > if (vring->cb.callback) > > > > > > Does not match the use. Change dereference so they match. > > > > > > Fixes: 79333575b8bd ("vDPA/ifcvf: implement shared IRQ feature") > > Thanks a lot! I squashed this into the offending patch - no point in > > breaking bisect. Pushed to linux. However I'm now > > having second thoughts about applying that patchset - I'd like > > soma analysis explaining how this got through testing. > > static analysis is something i do treewide. > > There are currently ~2500 issues in linux-next, do not panic! many are false > positives. > > It is pretty easy to setup and once you have a baseline you can filter only > your files. > > Tom Thanks for that info! I was actually directing this question to the contributor since the code does not look like it could have ever worked. I don't have the hardware in question myself. > > > Signed-off-by: Tom Rix <trix@redhat.com> > > > --- > > > drivers/vdpa/ifcvf/ifcvf_main.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/drivers/vdpa/ifcvf/ifcvf_main.c b/drivers/vdpa/ifcvf/ifcvf_main.c > > > index 3b48e717e89f7..4366320fb68d3 100644 > > > --- a/drivers/vdpa/ifcvf/ifcvf_main.c > > > +++ b/drivers/vdpa/ifcvf/ifcvf_main.c > > > @@ -46,7 +46,7 @@ static irqreturn_t ifcvf_vqs_reused_intr_handler(int irq, void *arg) > > > for (i = 0; i < vf->nr_vring; i++) { > > > vring = &vf->vring[i]; > > > if (vring->cb.callback) > > > - vf->vring->cb.callback(vring->cb.private); > > > + vring->cb.callback(vring->cb.private); > > > } > > > return IRQ_HANDLED; > > > -- > > > 2.26.3 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] vDPA/ifcvf: match pointer check to use 2022-03-15 15:15 ` Michael S. Tsirkin @ 2022-03-16 2:25 ` Zhu, Lingshan 0 siblings, 0 replies; 6+ messages in thread From: Zhu, Lingshan @ 2022-03-16 2:25 UTC (permalink / raw) To: Michael S. Tsirkin, Tom Rix Cc: jasowang, nathan, ndesaulniers, sgarzare, xieyongji, virtualization, linux-kernel, llvm On 3/15/2022 11:15 PM, Michael S. Tsirkin wrote: > On Tue, Mar 15, 2022 at 08:03:26AM -0700, Tom Rix wrote: >> On 3/15/22 6:28 AM, Michael S. Tsirkin wrote: >>> On Tue, Mar 15, 2022 at 05:41:30AM -0700, trix@redhat.com wrote: >>>> From: Tom Rix <trix@redhat.com> >>>> >>>> Clang static analysis reports this issue >>>> ifcvf_main.c:49:4: warning: Called function >>>> pointer is null (null dereference) >>>> vf->vring->cb.callback(vring->cb.private); >>>> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ >>>> >>>> The check >>>> vring = &vf->vring[i]; >>>> if (vring->cb.callback) >>>> >>>> Does not match the use. Change dereference so they match. >>>> >>>> Fixes: 79333575b8bd ("vDPA/ifcvf: implement shared IRQ feature") >>> Thanks a lot! I squashed this into the offending patch - no point in >>> breaking bisect. Pushed to linux. However I'm now >>> having second thoughts about applying that patchset - I'd like >>> soma analysis explaining how this got through testing. >> static analysis is something i do treewide. >> >> There are currently ~2500 issues in linux-next, do not panic! many are false >> positives. >> >> It is pretty easy to setup and once you have a baseline you can filter only >> your files. >> >> Tom > Thanks for that info! I was actually directing this question to the > contributor since the code does not look like it could have ever > worked. I don't have the hardware in question myself. Oh, that's my bad introducing this bug by typo, thanks Tom for fixing that! In my previous testing, I tolerated the lower performance(only one queue working as you pointed out) due to the shared irq, and did not check mq status, sorry for that. After fixing this issue, test again: (1)set nvectors = 2 after allocate MSI vectors, force the queues share one vector. from /proc/interrupts, we can see: [lszhu@localhost linux]$ cat /proc/interrupts | grep ifcvf 241: 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 2724424 0 0 0 0 0 0 0 0 0 IR-PCI-MSI 534528-edge ifcvf[0000:01:00.5]-vqs-reused-irq 242: 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 IR-PCI-MSI 534529-edge ifcvf[0000:01:00.5]-config 251: 0 0 0 0 0 0 0 0 0 0 0 2693318 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 IR-PCI-MSI 536576-edge ifcvf[0000:01:00.6]-vqs-reused-irq 252: 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 IR-PCI-MSI 536577-edge ifcvf[0000:01:00.6]-config (2) after several rounds of scp from a VF to another, at the source side, ethtool -S shows(cut off, only tx): localhost:/home/lszhu # ethtool -S eth1 NIC statistics: tx_queue_0_packets: 437256 tx_queue_0_bytes: 629246017 tx_queue_0_xdp_tx: 0 tx_queue_0_xdp_tx_drops: 0 tx_queue_0_kicks: 34089 tx_queue_1_packets: 73 tx_queue_1_bytes: 96721 tx_queue_1_xdp_tx: 0 tx_queue_1_xdp_tx_drops: 0 tx_queue_1_kicks: 12 tx_queue_2_packets: 294647 tx_queue_2_bytes: 433949815 tx_queue_2_xdp_tx: 0 tx_queue_2_xdp_tx_drops: 0 tx_queue_2_kicks: 14948 tx_queue_3_packets: 20226451 tx_queue_3_bytes: 29735633548 tx_queue_3_xdp_tx: 0 tx_queue_3_xdp_tx_drops: 0 tx_queue_3_kicks: 1123675 so every queue carries traffic now, all enabled Thanks, Zhu Lingshan > > >>>> Signed-off-by: Tom Rix <trix@redhat.com> >>>> --- >>>> drivers/vdpa/ifcvf/ifcvf_main.c | 2 +- >>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>> >>>> diff --git a/drivers/vdpa/ifcvf/ifcvf_main.c b/drivers/vdpa/ifcvf/ifcvf_main.c >>>> index 3b48e717e89f7..4366320fb68d3 100644 >>>> --- a/drivers/vdpa/ifcvf/ifcvf_main.c >>>> +++ b/drivers/vdpa/ifcvf/ifcvf_main.c >>>> @@ -46,7 +46,7 @@ static irqreturn_t ifcvf_vqs_reused_intr_handler(int irq, void *arg) >>>> for (i = 0; i < vf->nr_vring; i++) { >>>> vring = &vf->vring[i]; >>>> if (vring->cb.callback) >>>> - vf->vring->cb.callback(vring->cb.private); >>>> + vring->cb.callback(vring->cb.private); >>>> } >>>> return IRQ_HANDLED; >>>> -- >>>> 2.26.3 ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2022-03-16 2:25 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-03-15 12:41 [PATCH] vDPA/ifcvf: match pointer check to use trix 2022-03-15 13:23 ` Michael S. Tsirkin 2022-03-15 13:28 ` Michael S. Tsirkin 2022-03-15 15:03 ` Tom Rix 2022-03-15 15:15 ` Michael S. Tsirkin 2022-03-16 2:25 ` Zhu, Lingshan
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).