Re: [PATCH] net: stmmac: use dma_addr_t for DMA addresses

[Alex Elder]

On 8/12/26 2:23 AM, Maxime Chevallier wrote:
> Hi Alex,
>
> On 8/11/26 21:27, Alex Elder wrote:
> > In jumbo_frm() (implemented in both "chain_mode.c" and "ring_mode.c"),
> > an unsigned integer local variable is used to hold the value returned
> > by dma_map_single().  On systems where a dma_addr_t is 64 bits, the
> > subsequent dma_mapping_error() check of the returned value operates
> > only on the low 32 bits (whose high bit won't be sign-extended).  In
> > this case, dma_mapping_error() would return 0 (no error) even if there
> > were one.
> >
> > Fix this in both spots by using a dma_addr_t for the local variable.
>
> Thanks :)
>
> I was wondering if it would silently hides another address size related
> issue a little bit lower, as we now have :
>
>    dma_addr_t des2;
>
>    des2 = dma_map_single(...);
>
>    desc->des2 = cpu_to_le32(des2);
>
> desc->des2 is __le32, so for 64 bit addresses we silently truncate the
> address.

I saw that too.  Everything is unsigned, and only the lower
32 bits will be used, but I'd rather see the dma_addr_t
explicitly converted to u32 before being passed to cpu_to_le32().
This should work regardless of the size of dma_addr_t:

	desc->des2 = cpu_to_le32(lower_32_bits(des2));

I didn't look, but there could be other places where this occurs.
Tell me if you'd like me to send another patch that does this
(here and in any other spots I find.)
> However I don't see how jumbo would work on 64-bits anyways as des3 is the next
> chunk of the jumbo frame...
>
> I think this is good as-is, so
>
> Reviewed-by: Maxime Chevallier <maxime.chevallier@xxxxxxxxxxx>

Thank you.

					-Alex

> Maxime
>
> > Reported-by: Sashiko <sashiko-bot@xxxxxxxxxx>
> > Link: https://lore.kernel.org/linux-devicetree/20260606010122.21A211F00899@xxxxxxxxxxxxxxx/
> > Signed-off-by: Alex Elder <elder@xxxxxxxxxxxx>
> > ---
> >   drivers/net/ethernet/stmicro/stmmac/chain_mode.c | 3 ++-
> >   drivers/net/ethernet/stmicro/stmmac/ring_mode.c  | 3 ++-
> >   2 files changed, 4 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/stmicro/stmmac/chain_mode.c b/drivers/net/ethernet/stmicro/stmmac/chain_mode.c
> > index fc04a23342cfc..ec25193d287bb 100644
> > --- a/drivers/net/ethernet/stmicro/stmmac/chain_mode.c
> > +++ b/drivers/net/ethernet/stmicro/stmmac/chain_mode.c
> > @@ -20,9 +20,10 @@ static int jumbo_frm(struct stmmac_tx_queue *tx_q, struct sk_buff *skb,
> >   	unsigned int nopaged_len = skb_headlen(skb);
> >   	struct stmmac_priv *priv = tx_q->priv_data;
> >   	unsigned int entry = tx_q->cur_tx;
> > -	unsigned int bmax, buf_len, des2;
> > +	unsigned int bmax, buf_len;
> >   	unsigned int i = 1, len;
> >   	struct dma_desc *desc;
> > +	dma_addr_t des2;
> >   
> >   	desc = tx_q->dma_tx + entry;
> >   
> > diff --git a/drivers/net/ethernet/stmicro/stmmac/ring_mode.c b/drivers/net/ethernet/stmicro/stmmac/ring_mode.c
> > index 78fc6aa5bbe95..664d8cfb58cdc 100644
> > --- a/drivers/net/ethernet/stmicro/stmmac/ring_mode.c
> > +++ b/drivers/net/ethernet/stmicro/stmmac/ring_mode.c
> > @@ -20,8 +20,9 @@ static int jumbo_frm(struct stmmac_tx_queue *tx_q, struct sk_buff *skb,
> >   	unsigned int nopaged_len = skb_headlen(skb);
> >   	struct stmmac_priv *priv = tx_q->priv_data;
> >   	unsigned int entry = tx_q->cur_tx;
> > -	unsigned int bmax, len, des2;
> > +	unsigned int bmax, len;
> >   	struct dma_desc *desc;
> > +	dma_addr_t des2;
> >   
> >   	if (priv->extend_desc)
> >   		desc = (struct dma_desc *)(tx_q->dma_etx + entry);
> >
> > base-commit: 31397cf1819210bd63fa3d2c7d8c24f7c8667d99