| Message ID | 20210121061710.53217-2-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 benh@kernel.crashing.org mpe@ellerman.id.au paulus@samba.org sukadev@linux.ibm.com tlfalcon@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, 68 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:08 -0600 Lijun Pan wrote: > Move the dma_rmb() between pending_scrq() and ibmvnic_next_scrq() > into the end of pending_scrq(), and explain why. > Explain in detail why the dma_rmb() is placed at the end of > ibmvnic_next_scrq(). > > Fixes: b71ec9522346 ("ibmvnic: Ensure that SCRQ entry reads are correctly ordered") > Signed-off-by: Lijun Pan <ljp@linux.ibm.com> I fail to see why this is a fix. You move the barrier from caller to callee but there are no new barriers here. Did I miss some? > diff --git a/drivers/net/ethernet/ibm/ibmvnic.c b/drivers/net/ethernet/ibm/ibmvnic.c > index 9778c83150f1..8e043683610f 100644 > --- a/drivers/net/ethernet/ibm/ibmvnic.c > +++ b/drivers/net/ethernet/ibm/ibmvnic.c > @@ -2511,12 +2511,6 @@ static int ibmvnic_poll(struct napi_struct *napi, int budget) > > if (!pending_scrq(adapter, rx_scrq)) > break; > - /* The queue entry at the current index is peeked at above > - * to determine that there is a valid descriptor awaiting > - * processing. We want to be sure that the current slot > - * holds a valid descriptor before reading its contents. > - */ > - dma_rmb(); > next = ibmvnic_next_scrq(adapter, rx_scrq); > rx_buff = > (struct ibmvnic_rx_buff *)be64_to_cpu(next-> > @@ -3256,13 +3250,6 @@ static int ibmvnic_complete_tx(struct ibmvnic_adapter *adapter, > int total_bytes = 0; > int num_packets = 0; > > - /* The queue entry at the current index is peeked at above > - * to determine that there is a valid descriptor awaiting > - * processing. We want to be sure that the current slot > - * holds a valid descriptor before reading its contents. > - */ > - dma_rmb(); > - > next = ibmvnic_next_scrq(adapter, scrq); > for (i = 0; i < next->tx_comp.num_comps; i++) { > if (next->tx_comp.rcs[i]) > @@ -3636,11 +3623,25 @@ static int pending_scrq(struct ibmvnic_adapter *adapter, > struct ibmvnic_sub_crq_queue *scrq) > { > union sub_crq *entry = &scrq->msgs[scrq->cur]; > + int rc; > > if (entry->generic.first & IBMVNIC_CRQ_CMD_RSP) > - return 1; > + rc = 1; > else > - return 0; > + rc = 0; > + > + /* Ensure that the entire SCRQ descriptor scrq->msgs > + * has been loaded before reading its contents. This comment is confusing. IMHO the comments you're removing are much clearer. > + * This barrier makes sure this function's entry, esp. > + * entry->generic.first & IBMVNIC_CRQ_CMD_RSP > + * 1. is loaded before ibmvnic_next_scrq()'s > + * entry->generic.first & IBMVNIC_CRQ_CMD_RSP; > + * 2. OR is loaded before ibmvnic_poll()'s > + * disable_scrq_irq()'s scrq->hw_irq. > + */ > + dma_rmb(); > + > + return rc; > } > > static union sub_crq *ibmvnic_next_scrq(struct ibmvnic_adapter *adapter,
On Sat, Jan 23, 2021 at 11:10 PM Jakub Kicinski <kuba@kernel.org> wrote: > > On Thu, 21 Jan 2021 00:17:08 -0600 Lijun Pan wrote: > > Move the dma_rmb() between pending_scrq() and ibmvnic_next_scrq() > > into the end of pending_scrq(), and explain why. > > Explain in detail why the dma_rmb() is placed at the end of > > ibmvnic_next_scrq(). > > > > Fixes: b71ec9522346 ("ibmvnic: Ensure that SCRQ entry reads are correctly ordered") > > Signed-off-by: Lijun Pan <ljp@linux.ibm.com> > > I fail to see why this is a fix. You move the barrier from caller to > callee but there are no new barriers here. Did I miss some? I want to save the code since it is used more than twice. Maybe I should send to net-next. > > > diff --git a/drivers/net/ethernet/ibm/ibmvnic.c b/drivers/net/ethernet/ibm/ibmvnic.c > > index 9778c83150f1..8e043683610f 100644 > > --- a/drivers/net/ethernet/ibm/ibmvnic.c > > +++ b/drivers/net/ethernet/ibm/ibmvnic.c > > @@ -2511,12 +2511,6 @@ static int ibmvnic_poll(struct napi_struct *napi, int budget) > > > > if (!pending_scrq(adapter, rx_scrq)) > > break; > > - /* The queue entry at the current index is peeked at above > > - * to determine that there is a valid descriptor awaiting > > - * processing. We want to be sure that the current slot > > - * holds a valid descriptor before reading its contents. > > - */ > > - dma_rmb(); > > next = ibmvnic_next_scrq(adapter, rx_scrq); > > rx_buff = > > (struct ibmvnic_rx_buff *)be64_to_cpu(next-> > > @@ -3256,13 +3250,6 @@ static int ibmvnic_complete_tx(struct ibmvnic_adapter *adapter, > > int total_bytes = 0; > > int num_packets = 0; > > > > - /* The queue entry at the current index is peeked at above > > - * to determine that there is a valid descriptor awaiting > > - * processing. We want to be sure that the current slot > > - * holds a valid descriptor before reading its contents. > > - */ > > - dma_rmb(); > > - > > next = ibmvnic_next_scrq(adapter, scrq); > > for (i = 0; i < next->tx_comp.num_comps; i++) { > > if (next->tx_comp.rcs[i]) > > @@ -3636,11 +3623,25 @@ static int pending_scrq(struct ibmvnic_adapter *adapter, > > struct ibmvnic_sub_crq_queue *scrq) > > { > > union sub_crq *entry = &scrq->msgs[scrq->cur]; > > + int rc; > > > > if (entry->generic.first & IBMVNIC_CRQ_CMD_RSP) > > - return 1; > > + rc = 1; > > else > > - return 0; > > + rc = 0; > > + > > + /* Ensure that the entire SCRQ descriptor scrq->msgs > > + * has been loaded before reading its contents. > > This comment is confusing. IMHO the comments you're removing are much > clearer. I did not quite get what fields in the data structure are supposed to be guarded from the old comment. Then I asked around and figured it out. So I updated it with the new comment, which tells specifically what the barrier protects against. > > > + * This barrier makes sure this function's entry, esp. > > + * entry->generic.first & IBMVNIC_CRQ_CMD_RSP > > + * 1. is loaded before ibmvnic_next_scrq()'s > > + * entry->generic.first & IBMVNIC_CRQ_CMD_RSP; > > + * 2. OR is loaded before ibmvnic_poll()'s > > + * disable_scrq_irq()'s scrq->hw_irq. > > + */ > > + dma_rmb(); > > + > > + return rc; > > } > > > > static union sub_crq *ibmvnic_next_scrq(struct ibmvnic_adapter *adapter, >
diff --git a/drivers/net/ethernet/ibm/ibmvnic.c b/drivers/net/ethernet/ibm/ibmvnic.c index 9778c83150f1..8e043683610f 100644 --- a/drivers/net/ethernet/ibm/ibmvnic.c +++ b/drivers/net/ethernet/ibm/ibmvnic.c @@ -2511,12 +2511,6 @@ static int ibmvnic_poll(struct napi_struct *napi, int budget) if (!pending_scrq(adapter, rx_scrq)) break; - /* The queue entry at the current index is peeked at above - * to determine that there is a valid descriptor awaiting - * processing. We want to be sure that the current slot - * holds a valid descriptor before reading its contents. - */ - dma_rmb(); next = ibmvnic_next_scrq(adapter, rx_scrq); rx_buff = (struct ibmvnic_rx_buff *)be64_to_cpu(next-> @@ -3256,13 +3250,6 @@ static int ibmvnic_complete_tx(struct ibmvnic_adapter *adapter, int total_bytes = 0; int num_packets = 0; - /* The queue entry at the current index is peeked at above - * to determine that there is a valid descriptor awaiting - * processing. We want to be sure that the current slot - * holds a valid descriptor before reading its contents. - */ - dma_rmb(); - next = ibmvnic_next_scrq(adapter, scrq); for (i = 0; i < next->tx_comp.num_comps; i++) { if (next->tx_comp.rcs[i]) @@ -3636,11 +3623,25 @@ static int pending_scrq(struct ibmvnic_adapter *adapter, struct ibmvnic_sub_crq_queue *scrq) { union sub_crq *entry = &scrq->msgs[scrq->cur]; + int rc; if (entry->generic.first & IBMVNIC_CRQ_CMD_RSP) - return 1; + rc = 1; else - return 0; + rc = 0; + + /* Ensure that the entire SCRQ descriptor scrq->msgs + * has been loaded before reading its contents. + * This barrier makes sure this function's entry, esp. + * entry->generic.first & IBMVNIC_CRQ_CMD_RSP + * 1. is loaded before ibmvnic_next_scrq()'s + * entry->generic.first & IBMVNIC_CRQ_CMD_RSP; + * 2. OR is loaded before ibmvnic_poll()'s + * disable_scrq_irq()'s scrq->hw_irq. + */ + dma_rmb(); + + return rc; } static union sub_crq *ibmvnic_next_scrq(struct ibmvnic_adapter *adapter, @@ -3659,8 +3660,14 @@ static union sub_crq *ibmvnic_next_scrq(struct ibmvnic_adapter *adapter, } spin_unlock_irqrestore(&scrq->lock, flags); - /* Ensure that the entire buffer descriptor has been - * loaded before reading its contents + /* Ensure that the entire SCRQ descriptor scrq->msgs + * has been loaded before reading its contents. + * This barrier makes sure this function's entry, esp. + * entry->generic.first & IBMVNIC_CRQ_CMD_RSP + * 1. is loaded before ibmvnic_poll()'s + * be64_to_cpu(next->rx_comp.correlator); + * 2. OR is loaded before ibmvnic_complet_tx()'s + * be32_to_cpu(next->tx_comp.correlators[i]). */ dma_rmb();
Move the dma_rmb() between pending_scrq() and ibmvnic_next_scrq() into the end of pending_scrq(), and explain why. Explain in detail why the dma_rmb() is placed at the end of ibmvnic_next_scrq(). Fixes: b71ec9522346 ("ibmvnic: Ensure that SCRQ entry reads are correctly ordered") Signed-off-by: Lijun Pan <ljp@linux.ibm.com> --- drivers/net/ethernet/ibm/ibmvnic.c | 41 +++++++++++++++++------------- 1 file changed, 24 insertions(+), 17 deletions(-)