[PATCH net] net: wan: fsl_uhdlc_hdlc: fix dma_rmb usage in hdlc_rx_done

[PATCH net] net: wan: fsl_uhdlc_hdlc: fix dma_rmb usage in hdlc_rx_done

[Holger Brunck]

If dma_rmb is used it has to be done after reading bd_status and checking
if R_E_S is zero. Therefore we need to move it into the while loop.

Fixes: c19b6d246a35 ("drivers/net: support hdlc function for QE-UCC")
Signed-off-by: Holger Brunck <holger.brunck@hitachienergy.com>
---
 drivers/net/wan/fsl_ucc_hdlc.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/net/wan/fsl_ucc_hdlc.c b/drivers/net/wan/fsl_ucc_hdlc.c
index 15bfb78381d4..09081f128a98 100644
--- a/drivers/net/wan/fsl_ucc_hdlc.c
+++ b/drivers/net/wan/fsl_ucc_hdlc.c
@@ -523,12 +523,12 @@ static int hdlc_rx_done(struct ucc_hdlc_private *priv, int rx_work_limit)
 	u16 length, howmany = 0;
 	u8 *bdbuffer;
 
-	dma_rmb();
 	bd = priv->currx_bd;
 	bd_status = be16_to_cpu(bd->status);
 
 	/* while there are received buffers and BD is full (~R_E) */
 	while (!((bd_status & (R_E_S)) || (--rx_work_limit < 0))) {
+		dma_rmb();
 		if (bd_status & (RX_BD_ERRORS)) {
 			dev->stats.rx_errors++;
 
@@ -610,7 +610,6 @@ static int hdlc_rx_done(struct ucc_hdlc_private *priv, int rx_work_limit)
 
 		bd_status = be16_to_cpu(bd->status);
 	}
-	dma_rmb();
 
 	priv->currx_bd = bd;
 	return howmany;
-- 
2.47.3

Re: [PATCH net] net: wan: fsl_uhdlc_hdlc: fix dma_rmb usage in hdlc_rx_done

[Christophe Leroy (CS GROUP)]

Hi,

Le 04/05/2026 à 17:56, Holger Brunck a écrit :
> If dma_rmb is used it has to be done after reading bd_status and checking
> if R_E_S is zero. Therefore we need to move it into the while loop.

Can you give more details ? Why does dma_rmb() has to be done after 
reading bd_status and checking if R_E_S is zero ?

> 
> Fixes: c19b6d246a35 ("drivers/net: support hdlc function for QE-UCC")
> Signed-off-by: Holger Brunck <holger.brunck@hitachienergy.com>
> ---
>   drivers/net/wan/fsl_ucc_hdlc.c | 3 +--
>   1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/net/wan/fsl_ucc_hdlc.c b/drivers/net/wan/fsl_ucc_hdlc.c
> index 15bfb78381d4..09081f128a98 100644
> --- a/drivers/net/wan/fsl_ucc_hdlc.c
> +++ b/drivers/net/wan/fsl_ucc_hdlc.c
> @@ -523,12 +523,12 @@ static int hdlc_rx_done(struct ucc_hdlc_private *priv, int rx_work_limit)
>   	u16 length, howmany = 0;
>   	u8 *bdbuffer;
>   
> -	dma_rmb();
>   	bd = priv->currx_bd;
>   	bd_status = be16_to_cpu(bd->status);
>   
>   	/* while there are received buffers and BD is full (~R_E) */
>   	while (!((bd_status & (R_E_S)) || (--rx_work_limit < 0))) {
> +		dma_rmb();
>   		if (bd_status & (RX_BD_ERRORS)) {
>   			dev->stats.rx_errors++;
>   
> @@ -610,7 +610,6 @@ static int hdlc_rx_done(struct ucc_hdlc_private *priv, int rx_work_limit)
>   
>   		bd_status = be16_to_cpu(bd->status);
>   	}
> -	dma_rmb();
>   
>   	priv->currx_bd = bd;
>   	return howmany;

RE: [PATCH net] net: wan: fsl_uhdlc_hdlc: fix dma_rmb usage in hdlc_rx_done

[Holger Brunck]

> 
> Le 04/05/2026 à 17:56, Holger Brunck a écrit :
> > If dma_rmb is used it has to be done after reading bd_status and
> > checking if R_E_S is zero. Therefore we need to move it into the while loop.
> 
> Can you give more details ? Why does dma_rmb() has to be done after reading
> bd_status and checking if R_E_S is zero ?
> 

when R_E_S is zero in the status of the buffer descriptor it means the buffer is
filled with data from the device.  Now the CPU owns the descriptor. Now we
should execute the dma_rmb to be sure that we read the data correctly. 
And this we need to redo for each buffer descriptor which is filled with data,
that’s why it must be done within the for loop and not before and after.

This is also consistent with the example in Documentation/memory-barriers.txt

Best regards
Holger

Re: [PATCH net] net: wan: fsl_uhdlc_hdlc: fix dma_rmb usage in hdlc_rx_done

[Christophe Leroy (CS GROUP)]

Le 05/05/2026 à 10:14, Holger Brunck a écrit :
>>
>> Le 04/05/2026 à 17:56, Holger Brunck a écrit :
>>> If dma_rmb is used it has to be done after reading bd_status and
>>> checking if R_E_S is zero. Therefore we need to move it into the while loop.
>>
>> Can you give more details ? Why does dma_rmb() has to be done after reading
>> bd_status and checking if R_E_S is zero ?
>>
> 
> when R_E_S is zero in the status of the buffer descriptor it means the buffer is
> filled with data from the device.  Now the CPU owns the descriptor. Now we
> should execute the dma_rmb to be sure that we read the data correctly.
> And this we need to redo for each buffer descriptor which is filled with data,
> that’s why it must be done within the for loop and not before and after.

We enter hdlc_rx_done() after an interrupt which triggers scheduling of 
ucc_hdlc_poll(). I think dma_rmb() is needed _before_ reading the first 
status, otherwise it might read an erroneous status.

Once we are here the interrupt has been cleared so any new buffer will 
trigger a new interrupt and call again this function. Therefore I don't 
think it is worth the cost of a dma_rmb() inside the loop.

Christophe

RE: [PATCH net] net: wan: fsl_uhdlc_hdlc: fix dma_rmb usage in hdlc_rx_done

[Holger Brunck]

> 
> Le 05/05/2026 à 10:14, Holger Brunck a écrit :
> >>
> >> Le 04/05/2026 à 17:56, Holger Brunck a écrit :
> >>> If dma_rmb is used it has to be done after reading bd_status and
> >>> checking if R_E_S is zero. Therefore we need to move it into the while loop.
> >>
> >> Can you give more details ? Why does dma_rmb() has to be done after
> >> reading bd_status and checking if R_E_S is zero ?
> >>
> >
> > when R_E_S is zero in the status of the buffer descriptor it means the
> > buffer is filled with data from the device.  Now the CPU owns the
> > descriptor. Now we should execute the dma_rmb to be sure that we read the
> data correctly.
> > And this we need to redo for each buffer descriptor which is filled
> > with data, that’s why it must be done within the for loop and not before and
> after.
> 
> We enter hdlc_rx_done() after an interrupt which triggers scheduling of
> ucc_hdlc_poll(). I think dma_rmb() is needed _before_ reading the first status,
> otherwise it might read an erroneous status.
> 

we end up in hdlc_rx_done also if a TX interrupt was triggered due to the NAPI layer.
If RX bd_status is still 1 we would read a pending RX packet next time.  So the dma_rmb()
before is not necessary. But if we have read a zero in bd_status we need to be sure that
the data written from the QE to RAM is fully available. That is why I think doing it after
reading the status is correct here.
 
> Once we are here the interrupt has been cleared so any new buffer will trigger a
> new interrupt and call again this function. Therefore I don't think it is worth the
> cost of a dma_rmb() inside the loop.
> 

Yes, but while we are in the for loop the QE might have written the next packet to
the memory in background and sets the bd_status to zero. And if this is the
case we need again make sure that the memory we read is correct.

For example the following driver does the same AFAIU.
drivers/net/ethernet/intel/e1000e/netdev.c  
from line 1524 ff

It first reads the status for the RX packet then does dma_rmb() and then reads
the data itself. And it is done within the while loop as any further available packet
may have arrived just now and we need to be sure that the data is valid. 

Best regards
Holger