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

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

[Alex Elder]

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.

Reported-by: Sashiko <sashiko-bot@kernel.org>
Link: https://lore.kernel.org/linux-devicetree/20260606010122.21A211F00899@smtp.kernel.org/
Signed-off-by: Alex Elder <elder@riscstar.com>
---
 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
-- 
2.53.0

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

[Maxime Chevallier]

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.

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@bootlin.com>

Maxime

> 
> Reported-by: Sashiko <sashiko-bot@kernel.org>
> Link: https://lore.kernel.org/linux-devicetree/20260606010122.21A211F00899@smtp.kernel.org/
> Signed-off-by: Alex Elder <elder@riscstar.com>
> ---
>  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

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@bootlin.com>

Thank you.

					-Alex

> Maxime
> 
>>
>> Reported-by: Sashiko <sashiko-bot@kernel.org>
>> Link: https://lore.kernel.org/linux-devicetree/20260606010122.21A211F00899@smtp.kernel.org/
>> Signed-off-by: Alex Elder <elder@riscstar.com>
>> ---
>>   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
> 

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

[Maxime Chevallier]

Hi,

On 8/12/26 14:25, Alex Elder wrote:
> 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));

Fine by me, if you add that in V2 feel free to keep my review tag :)

Maxime

> 
> 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@bootlin.com>
> 
> Thank you.
> 
>                     -Alex
> 
>> Maxime
>>
>>>
>>> Reported-by: Sashiko <sashiko-bot@kernel.org>
>>> Link: https://lore.kernel.org/linux-devicetree/20260606010122.21A211F00899@smtp.kernel.org/
>>> Signed-off-by: Alex Elder <elder@riscstar.com>
>>> ---
>>>   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
>>
>