| Message ID | 20210121061710.53217-3-ljp@linux.ibm.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | fixes the memory barrier for SCRQ/CRQ entry | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Clearly marked for net |
| netdev/subject_prefix | success | Link |
| netdev/cc_maintainers | warning | 9 maintainers not CCed: drt@linux.ibm.com dnbanerg@us.ibm.com benh@kernel.crashing.org mpe@ellerman.id.au paulus@samba.org sukadev@linux.ibm.com linuxppc-dev@lists.ozlabs.org davem@davemloft.net kuba@kernel.org |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 0 this patch: 0 |
| netdev/kdoc | success | Errors and warnings before: 19 this patch: 19 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 7 lines checked |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
| netdev/header_inline | success | Link |
| netdev/stable | success | Stable not CCed |
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(-)