Re: [PATCH v2] net: stmmac: allocate separate page for buffer

[Sarosh Hasan]

On 9/12/2024 2:17 PM, Simon Horman wrote:
> On Tue, Sep 10, 2024 at 06:18:41PM +0530, Suraj Jaiswal wrote:
>> Currently for TSO page is mapped with dma_map_single()
>> and then resulting dma address is referenced (and offset)
>> by multiple descriptors until the whole region is
>> programmed into the descriptors.
>> This makes it possible for stmmac_tx_clean() to dma_unmap()
>> the first of the already processed descriptors, while the
>> rest are still being processed by the DMA engine. This leads
>> to an iommu fault due to the DMA engine using unmapped memory
>> as seen below:
>>
>> arm-smmu 15000000.iommu: Unhandled context fault: fsr=0x402,
>> iova=0xfc401000, fsynr=0x60003, cbfrsynra=0x121, cb=38
>>
>> Descriptor content:
>>      TDES0       TDES1   TDES2   TDES3
>> 317: 0xfc400800  0x0     0x36    0xa02c0b68
>> 318: 0xfc400836  0x0     0xb68   0x90000000
>>
>> As we can see above descriptor 317 holding a page address
>> and 318 holding the buffer address by adding offset to page
>> addess. Now if 317 descritor is cleaned as part of tx_clean()
> 
> Hi Suraj,
> 
> As it looks like there will be a v3 anyway, some minor nits from my side.
> 
> addess -> address
> 
> Flagged by checkpatch.pl --codespell
sure . we will take care of all commnet and update latest patch after verification . 
> 
>> then we will get SMMU fault if 318 descriptor is getting accessed.
>>
>> To fix this, let's map each descriptor's memory reference individually.
>> This way there's no risk of unmapping a region that's still being
>> referenced by the DMA engine in a later descriptor.
>>
>> Signed-off-by: Suraj Jaiswal <quic_jsuraj@xxxxxxxxxxx>
>> ---
>>
>> Changes since v2:
>> - Update commit text with more details.
>> - fixed Reverse xmas tree order issue.
>>
>>
>> Changes since v1:
>> - Fixed function description 
>> - Fixed handling of return value.
>>
>>
>>  .../net/ethernet/stmicro/stmmac/stmmac_main.c | 63 ++++++++++++-------
>>  1 file changed, 42 insertions(+), 21 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
>> index 83b654b7a9fd..98d5a4b64cac 100644
>> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
>> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
>> @@ -4136,21 +4136,25 @@ static bool stmmac_vlan_insert(struct stmmac_priv *priv, struct sk_buff *skb,
>>  /**
>>   *  stmmac_tso_allocator - close entry point of the driver
>>   *  @priv: driver private structure
>> - *  @des: buffer start address
>> + *  @addr: Contains either skb frag address or skb->data address
>>   *  @total_len: total length to fill in descriptors
>>   *  @last_segment: condition for the last descriptor
>>   *  @queue: TX queue index
>> + * @is_skb_frag: condition to check whether skb data is part of fragment or not
>>   *  Description:
>>   *  This function fills descriptor and request new descriptors according to
>>   *  buffer length to fill
>> + *  This function returns 0 on success else -ERRNO on fail
> 
> Please consider using a "Return:" or "Returns:" section to document
> return values.
> 
> Flagged by ./scripts/kernel-doc -none -Wall .../stmmac_main.c
> 
>>   */
>> -static void stmmac_tso_allocator(struct stmmac_priv *priv, dma_addr_t des,
>> -				 int total_len, bool last_segment, u32 queue)
>> +static int stmmac_tso_allocator(struct stmmac_priv *priv, void *addr,
>> +				int total_len, bool last_segment, u32 queue, bool is_skb_frag)
> 
> The line above could be trivially wrapped to <= 80 columns wide, as is
> still preferred for networking code. Likewise a little further below.
> 
> Likewise elsewhere in this patch.
> 
> You can pass an option to checkpatch.pl to check for this.
> 
>>  {
>>  	struct stmmac_tx_queue *tx_q = &priv->dma_conf.tx_queue[queue];
>>  	struct dma_desc *desc;
>>  	u32 buff_size;
>>  	int tmp_len;
>> +	unsigned char *data = addr;
>> +	unsigned int offset = 0;
> 
> Please consider arranging local variables in Networking code in
> reverse xmas tree order - longest line to shortest.
> 
> Edward Cree's xmastree tool can be of assistance here:
> https://github.com/ecree-solarflare/xmastree
> 
>>  
>>  	tmp_len = total_len;
>>  
>> @@ -4161,20 +4165,44 @@ static void stmmac_tso_allocator(struct stmmac_priv *priv, dma_addr_t des,
>>  						priv->dma_conf.dma_tx_size);
>>  		WARN_ON(tx_q->tx_skbuff[tx_q->cur_tx]);
>>  
>> +		buff_size = tmp_len >= TSO_MAX_BUFF_SIZE ? TSO_MAX_BUFF_SIZE : tmp_len;
> 
> 		FWIIW, I think that min() would allow this the intent
> 		of the line above to be expressed more succinctly.
> 
>> +
>>  		if (tx_q->tbs & STMMAC_TBS_AVAIL)
>>  			desc = &tx_q->dma_entx[tx_q->cur_tx].basic;
>>  		else
>>  			desc = &tx_q->dma_tx[tx_q->cur_tx];
>>  
>> -		curr_addr = des + (total_len - tmp_len);
>> +		offset = total_len - tmp_len;
>> +		if (!is_skb_frag) {
>> +			curr_addr = dma_map_single(priv->device, data + offset, buff_size,
>> +						   DMA_TO_DEVICE);
>> +
>> +			if (dma_mapping_error(priv->device, curr_addr))
>> +				return -ENOMEM;
>> +
>> +			tx_q->tx_skbuff_dma[tx_q->cur_tx].buf = curr_addr;
>> +			tx_q->tx_skbuff_dma[tx_q->cur_tx].len = buff_size;
>> +			tx_q->tx_skbuff_dma[tx_q->cur_tx].map_as_page = false;
>> +			tx_q->tx_skbuff_dma[tx_q->cur_tx].buf_type = STMMAC_TXBUF_T_SKB;
>> +		} else {
>> +			curr_addr = skb_frag_dma_map(priv->device, addr, offset,
>> +						     buff_size,
>> +						     DMA_TO_DEVICE);
>> +
>> +			if (dma_mapping_error(priv->device, curr_addr))
>> +				return -ENOMEM;
>> +
>> +			tx_q->tx_skbuff_dma[tx_q->cur_tx].buf = curr_addr;
>> +			tx_q->tx_skbuff_dma[tx_q->cur_tx].len = buff_size;
>> +			tx_q->tx_skbuff_dma[tx_q->cur_tx].map_as_page = true;
>> +			tx_q->tx_skbuff_dma[tx_q->cur_tx].buf_type = STMMAC_TXBUF_T_SKB;
>> +		}
> 
> Maybe my eyes are deceiving me, but there seems to be quite a lot of
> repetition in the two arms of the if/else condition above. If so, can it be
> consolidated by moving everything other than the assignment of curr out of
> the conditional blocks?  (And dropping the {}.)
> 
>> +
>>  		if (priv->dma_cap.addr64 <= 32)
>>  			desc->des0 = cpu_to_le32(curr_addr);
>>  		else
>>  			stmmac_set_desc_addr(priv, desc, curr_addr);
>>  
>> -		buff_size = tmp_len >= TSO_MAX_BUFF_SIZE ?
>> -			    TSO_MAX_BUFF_SIZE : tmp_len;
>> -
>>  		stmmac_prepare_tso_tx_desc(priv, desc, 0, buff_size,
>>  				0, 1,
>>  				(last_segment) && (tmp_len <= TSO_MAX_BUFF_SIZE),
> 
> ...