| Message ID | 20210121061710.53217-3-ljp@linux.ibm.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | fixes the memory barrier for SCRQ/CRQ entry | expand |
On Thu, 21 Jan 2021 00:17:09 -0600 Lijun Pan wrote: > rmb() was introduced to load rx_scrq->msgs after calling > pending_scrq(). Now since pending_scrq() itself already > has dma_rmb() at the end of the function, rmb() is > duplicated and can be removed. > > Fixes: ec20f36bb41a ("ibmvnic: Correctly re-enable interrupts in NAPI polling routine") > Signed-off-by: Lijun Pan <ljp@linux.ibm.com> rmb() is a stronger barrier than dma_rmb() also again, I don't see how this fixes any bugs
On Sat, Jan 23, 2021 at 11:11 PM Jakub Kicinski <kuba@kernel.org> wrote: > > On Thu, 21 Jan 2021 00:17:09 -0600 Lijun Pan wrote: > > rmb() was introduced to load rx_scrq->msgs after calling > > pending_scrq(). Now since pending_scrq() itself already > > has dma_rmb() at the end of the function, rmb() is > > duplicated and can be removed. > > > > Fixes: ec20f36bb41a ("ibmvnic: Correctly re-enable interrupts in NAPI polling routine") > > Signed-off-by: Lijun Pan <ljp@linux.ibm.com> > > rmb() is a stronger barrier than dma_rmb() Yes. I think the weaker dma_rmb() here is enough. And I let it reuse the dma_rmb() in the pending_scrq(). > > also again, I don't see how this fixes any bugs I will send to net-next if you are ok with it.
On Sun, 24 Jan 2021 22:38:02 -0600 Lijun Pan wrote: > On Sat, Jan 23, 2021 at 11:11 PM Jakub Kicinski <kuba@kernel.org> wrote: > > On Thu, 21 Jan 2021 00:17:09 -0600 Lijun Pan wrote: > > > rmb() was introduced to load rx_scrq->msgs after calling > > > pending_scrq(). Now since pending_scrq() itself already > > > has dma_rmb() at the end of the function, rmb() is > > > duplicated and can be removed. > > > > > > Fixes: ec20f36bb41a ("ibmvnic: Correctly re-enable interrupts in NAPI polling routine") > > > Signed-off-by: Lijun Pan <ljp@linux.ibm.com> > > > > rmb() is a stronger barrier than dma_rmb() > > Yes. I think the weaker dma_rmb() here is enough. > And I let it reuse the dma_rmb() in the pending_scrq(). > > > > > also again, I don't see how this fixes any bugs > > I will send to net-next if you are ok with it. If there is consensus at IBM that the first 2 changes are an improvement you can drop the Fixes tags and resubmit to net-next. In patch 3 it looks like the dma_rmb() may indeed be missing so that one could go to net, but I don't think the dma_wmb() is needed, especially not where you put it. I think dma_wmb() is only needed before the device is notified that new buffer was posted, not on completion.
diff --git a/drivers/net/ethernet/ibm/ibmvnic.c b/drivers/net/ethernet/ibm/ibmvnic.c index 8e043683610f..933e8fb71a8b 100644 --- a/drivers/net/ethernet/ibm/ibmvnic.c +++ b/drivers/net/ethernet/ibm/ibmvnic.c @@ -2577,7 +2577,6 @@ static int ibmvnic_poll(struct napi_struct *napi, int budget) if (napi_complete_done(napi, frames_processed)) { enable_scrq_irq(adapter, rx_scrq); if (pending_scrq(adapter, rx_scrq)) { - rmb(); if (napi_reschedule(napi)) { disable_scrq_irq(adapter, rx_scrq); goto restart_poll;
rmb() was introduced to load rx_scrq->msgs after calling pending_scrq(). Now since pending_scrq() itself already has dma_rmb() at the end of the function, rmb() is duplicated and can be removed. Fixes: ec20f36bb41a ("ibmvnic: Correctly re-enable interrupts in NAPI polling routine") Signed-off-by: Lijun Pan <ljp@linux.ibm.com> --- drivers/net/ethernet/ibm/ibmvnic.c | 1 - 1 file changed, 1 deletion(-)