| Message ID | 1606238776-30259-2-git-send-email-tlfalcon@linux.ibm.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | ibmvnic: Bug fixes for queue descriptor processing | 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/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: 18 this patch: 18 |
| netdev/verify_fixes | fail | Link |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 26 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 |
Thomas Falcon <tlfalcon@linux.ibm.com> writes: > Ensure that received Subordinate Command-Response Queue (SCRQ) > entries are properly read in order by the driver. These queues > are used in the ibmvnic device to process RX buffer and TX completion > descriptors. dma_rmb barriers have been added after checking for a > pending descriptor to ensure the correct descriptor entry is checked > and after reading the SCRQ descriptor to ensure the entire > descriptor is read before processing. > > Fixes: 032c5e828 ("Driver for IBM System i/p VNIC protocol") > Signed-off-by: Thomas Falcon <tlfalcon@linux.ibm.com> > --- > drivers/net/ethernet/ibm/ibmvnic.c | 8 ++++++++ > 1 file changed, 8 insertions(+) > > diff --git a/drivers/net/ethernet/ibm/ibmvnic.c b/drivers/net/ethernet/ibm/ibmvnic.c > index 2aa40b2..489ed5e 100644 > --- a/drivers/net/ethernet/ibm/ibmvnic.c > +++ b/drivers/net/ethernet/ibm/ibmvnic.c > @@ -2403,6 +2403,8 @@ static int ibmvnic_poll(struct napi_struct *napi, int budget) > > if (!pending_scrq(adapter, adapter->rx_scrq[scrq_num])) > break; > + /* ensure that we do not prematurely exit the polling loop */ > + dma_rmb(); I'd be happier if these comments were more specific about which read(s) they are ordering vs which other read(s). I'm sure it's obvious to you, but it may not be to a future author, and/or after the code has been refactored over time. > next = ibmvnic_next_scrq(adapter, adapter->rx_scrq[scrq_num]); > rx_buff = > (struct ibmvnic_rx_buff *)be64_to_cpu(next-> > @@ -3098,6 +3100,9 @@ static int ibmvnic_complete_tx(struct ibmvnic_adapter *adapter, > unsigned int pool = scrq->pool_index; > int num_entries = 0; > > + /* ensure that the correct descriptor entry is read */ > + dma_rmb(); > + > next = ibmvnic_next_scrq(adapter, scrq); > for (i = 0; i < next->tx_comp.num_comps; i++) { > if (next->tx_comp.rcs[i]) { > @@ -3498,6 +3503,9 @@ static union sub_crq *ibmvnic_next_scrq(struct ibmvnic_adapter *adapter, > } > spin_unlock_irqrestore(&scrq->lock, flags); > > + /* ensure that the entire SCRQ descriptor is read */ > + dma_rmb(); > + > return entry; > } cheers
On 11/24/20 11:43 PM, Michael Ellerman wrote: > Thomas Falcon <tlfalcon@linux.ibm.com> writes: >> Ensure that received Subordinate Command-Response Queue (SCRQ) >> entries are properly read in order by the driver. These queues >> are used in the ibmvnic device to process RX buffer and TX completion >> descriptors. dma_rmb barriers have been added after checking for a >> pending descriptor to ensure the correct descriptor entry is checked >> and after reading the SCRQ descriptor to ensure the entire >> descriptor is read before processing. >> >> Fixes: 032c5e828 ("Driver for IBM System i/p VNIC protocol") >> Signed-off-by: Thomas Falcon <tlfalcon@linux.ibm.com> >> --- >> drivers/net/ethernet/ibm/ibmvnic.c | 8 ++++++++ >> 1 file changed, 8 insertions(+) >> >> diff --git a/drivers/net/ethernet/ibm/ibmvnic.c b/drivers/net/ethernet/ibm/ibmvnic.c >> index 2aa40b2..489ed5e 100644 >> --- a/drivers/net/ethernet/ibm/ibmvnic.c >> +++ b/drivers/net/ethernet/ibm/ibmvnic.c >> @@ -2403,6 +2403,8 @@ static int ibmvnic_poll(struct napi_struct *napi, int budget) >> >> if (!pending_scrq(adapter, adapter->rx_scrq[scrq_num])) >> break; >> + /* ensure that we do not prematurely exit the polling loop */ >> + dma_rmb(); > I'd be happier if these comments were more specific about which read(s) > they are ordering vs which other read(s). > > I'm sure it's obvious to you, but it may not be to a future author, > and/or after the code has been refactored over time. Thank you for reviewing! I will submit a v2 soon with clearer comments on the reads being ordered here. Thanks, Tom > >> next = ibmvnic_next_scrq(adapter, adapter->rx_scrq[scrq_num]); >> rx_buff = >> (struct ibmvnic_rx_buff *)be64_to_cpu(next-> >> @@ -3098,6 +3100,9 @@ static int ibmvnic_complete_tx(struct ibmvnic_adapter *adapter, >> unsigned int pool = scrq->pool_index; >> int num_entries = 0; >> >> + /* ensure that the correct descriptor entry is read */ >> + dma_rmb(); >> + >> next = ibmvnic_next_scrq(adapter, scrq); >> for (i = 0; i < next->tx_comp.num_comps; i++) { >> if (next->tx_comp.rcs[i]) { >> @@ -3498,6 +3503,9 @@ static union sub_crq *ibmvnic_next_scrq(struct ibmvnic_adapter *adapter, >> } >> spin_unlock_irqrestore(&scrq->lock, flags); >> >> + /* ensure that the entire SCRQ descriptor is read */ >> + dma_rmb(); >> + >> return entry; >> } > cheers
Thomas Falcon <tlfalcon@linux.ibm.com> writes: > On 11/24/20 11:43 PM, Michael Ellerman wrote: >> Thomas Falcon <tlfalcon@linux.ibm.com> writes: >>> Ensure that received Subordinate Command-Response Queue (SCRQ) >>> entries are properly read in order by the driver. These queues >>> are used in the ibmvnic device to process RX buffer and TX completion >>> descriptors. dma_rmb barriers have been added after checking for a >>> pending descriptor to ensure the correct descriptor entry is checked >>> and after reading the SCRQ descriptor to ensure the entire >>> descriptor is read before processing. >>> >>> Fixes: 032c5e828 ("Driver for IBM System i/p VNIC protocol") >>> Signed-off-by: Thomas Falcon <tlfalcon@linux.ibm.com> >>> --- >>> drivers/net/ethernet/ibm/ibmvnic.c | 8 ++++++++ >>> 1 file changed, 8 insertions(+) >>> >>> diff --git a/drivers/net/ethernet/ibm/ibmvnic.c b/drivers/net/ethernet/ibm/ibmvnic.c >>> index 2aa40b2..489ed5e 100644 >>> --- a/drivers/net/ethernet/ibm/ibmvnic.c >>> +++ b/drivers/net/ethernet/ibm/ibmvnic.c >>> @@ -2403,6 +2403,8 @@ static int ibmvnic_poll(struct napi_struct *napi, int budget) >>> >>> if (!pending_scrq(adapter, adapter->rx_scrq[scrq_num])) >>> break; >>> + /* ensure that we do not prematurely exit the polling loop */ >>> + dma_rmb(); >> I'd be happier if these comments were more specific about which read(s) >> they are ordering vs which other read(s). >> >> I'm sure it's obvious to you, but it may not be to a future author, >> and/or after the code has been refactored over time. > > Thank you for reviewing! I will submit a v2 soon with clearer comments > on the reads being ordered here. Thanks. cheers
diff --git a/drivers/net/ethernet/ibm/ibmvnic.c b/drivers/net/ethernet/ibm/ibmvnic.c index 2aa40b2..489ed5e 100644 --- a/drivers/net/ethernet/ibm/ibmvnic.c +++ b/drivers/net/ethernet/ibm/ibmvnic.c @@ -2403,6 +2403,8 @@ static int ibmvnic_poll(struct napi_struct *napi, int budget) if (!pending_scrq(adapter, adapter->rx_scrq[scrq_num])) break; + /* ensure that we do not prematurely exit the polling loop */ + dma_rmb(); next = ibmvnic_next_scrq(adapter, adapter->rx_scrq[scrq_num]); rx_buff = (struct ibmvnic_rx_buff *)be64_to_cpu(next-> @@ -3098,6 +3100,9 @@ static int ibmvnic_complete_tx(struct ibmvnic_adapter *adapter, unsigned int pool = scrq->pool_index; int num_entries = 0; + /* ensure that the correct descriptor entry is read */ + dma_rmb(); + next = ibmvnic_next_scrq(adapter, scrq); for (i = 0; i < next->tx_comp.num_comps; i++) { if (next->tx_comp.rcs[i]) { @@ -3498,6 +3503,9 @@ static union sub_crq *ibmvnic_next_scrq(struct ibmvnic_adapter *adapter, } spin_unlock_irqrestore(&scrq->lock, flags); + /* ensure that the entire SCRQ descriptor is read */ + dma_rmb(); + return entry; }
Ensure that received Subordinate Command-Response Queue (SCRQ) entries are properly read in order by the driver. These queues are used in the ibmvnic device to process RX buffer and TX completion descriptors. dma_rmb barriers have been added after checking for a pending descriptor to ensure the correct descriptor entry is checked and after reading the SCRQ descriptor to ensure the entire descriptor is read before processing. Fixes: 032c5e828 ("Driver for IBM System i/p VNIC protocol") Signed-off-by: Thomas Falcon <tlfalcon@linux.ibm.com> --- drivers/net/ethernet/ibm/ibmvnic.c | 8 ++++++++ 1 file changed, 8 insertions(+)