Re: [PATCH net 1/2] ibmvnic: Ensure that SCRQ entry reads are correctly ordered
Thomas Falcon writes: > On 11/24/20 11:43 PM, Michael Ellerman wrote: >> Thomas Falcon 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 >>> --- >>> 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
Re: [PATCH net 1/2] ibmvnic: Ensure that SCRQ entry reads are correctly ordered
On 11/24/20 11:43 PM, Michael Ellerman wrote: Thomas Falcon 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 --- 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
Re: [PATCH net 1/2] ibmvnic: Ensure that SCRQ entry reads are correctly ordered
Thomas Falcon 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 > --- > 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
[PATCH net 1/2] ibmvnic: Ensure that SCRQ entry reads are correctly ordered
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 --- 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(); 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; } -- 1.8.3.1