Netdev Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Christophe Leroy <christophe.leroy@csgroup.eu>
To: Geoff Levand <geoff@infradead.org>
Cc: linuxppc-dev@lists.ozlabs.org, netdev@vger.kernel.org,
	Jakub Kicinski <kuba@kernel.org>,
	"David S. Miller" <davem@davemloft.net>
Subject: Re: [PATCH v4 10/10] net/ps3_gelic: Fix DMA mapping problems
Date: Sun, 25 Jul 2021 20:38:04 +0200	[thread overview]
Message-ID: <20210725203804.Horde.sruKcwZQKonIWKjB98332A2@messagerie.c-s.fr> (raw)
In-Reply-To: <7aa1d9b1b4ffadcbdc6f88e4f8d4a323da307595.1627068552.git.geoff@infradead.org>

Geoff Levand <geoff@infradead.org> a écrit :

> Fixes several DMA mapping problems with the PS3's gelic network driver:
>
>  * Change from checking the return value of dma_map_single to using the
>    dma_mapping_error routine.
>  * Use the correct buffer length when mapping the RX skb.
>  * Improved error checking and debug logging.

The patch is quite big and probably deserves more explanation. For  
instance, explain why the buffer length is not correct today.

Also as it is a bug fixing patch, it should include a 'fixes' tag, and  
a Cc: to stable@vger.kernel.org. Also, when possible, bug fixes should  
be one of the first patches in a series like that so that they can be  
applied to stable without applying the whole series.

Christophe

>
> Fixes runtime errors like these, and also other randomly occurring errors:
>
>   IP-Config: Complete:
>   DMA-API: ps3_gelic_driver sb_05: device driver failed to check map error
>   WARNING: CPU: 0 PID: 0 at kernel/dma/debug.c:1027 .check_unmap+0x888/0x8dc
>
> Signed-off-by: Geoff Levand <geoff@infradead.org>
> ---
>  drivers/net/ethernet/toshiba/ps3_gelic_net.c | 183 +++++++++++--------
>  1 file changed, 108 insertions(+), 75 deletions(-)
>
> diff --git a/drivers/net/ethernet/toshiba/ps3_gelic_net.c  
> b/drivers/net/ethernet/toshiba/ps3_gelic_net.c
> index 42f4de9ad5fe..11ddeacb1159 100644
> --- a/drivers/net/ethernet/toshiba/ps3_gelic_net.c
> +++ b/drivers/net/ethernet/toshiba/ps3_gelic_net.c
> @@ -336,22 +336,31 @@ static int gelic_card_init_chain(struct  
> gelic_card *card,
>  	struct gelic_descr_chain *chain, struct gelic_descr *start_descr,
>  	int descr_count)
>  {
> -	int i;
> -	struct gelic_descr *descr;
> +	struct gelic_descr *descr = start_descr;
>  	struct device *dev = ctodev(card);
> +	unsigned int index;
>
> -	descr = start_descr;
> -	memset(descr, 0, sizeof(*descr) *descr_count);
> +	memset(start_descr, 0, descr_count * sizeof(*start_descr));
>
> -	for (i = 0; i < descr_count; i++, descr++) {
> -		descr->link.size = sizeof(struct gelic_hw_regs);
> +	for (index = 0, descr = start_descr; index < descr_count;
> +		index++, descr++) {
>  		gelic_descr_set_status(descr, GELIC_DESCR_DMA_NOT_IN_USE);
> -		descr->link.cpu_addr =
> -			dma_map_single(dev, descr, descr->link.size,
> -				DMA_BIDIRECTIONAL);
>
> -		if (!descr->link.cpu_addr)
> -			goto iommu_error;
> +		descr->link.size = sizeof(struct gelic_hw_regs);
> +		descr->link.cpu_addr = dma_map_single(dev, descr,
> +			descr->link.size, DMA_BIDIRECTIONAL);
> +
> +		if (unlikely(dma_mapping_error(dev, descr->link.cpu_addr))) {
> +			dev_err(dev, "%s:%d: dma_mapping_error\n", __func__,
> +				__LINE__);
> +
> +			for (index--, descr--; index > 0; index--, descr--) {
> +				if (descr->link.cpu_addr) {
> +					gelic_unmap_link(dev, descr);
> +				}
> +			}
> +			return -ENOMEM;
> +		}
>
>  		descr->next = descr + 1;
>  		descr->prev = descr - 1;
> @@ -360,8 +369,9 @@ static int gelic_card_init_chain(struct gelic_card *card,
>  	(descr - 1)->next = start_descr;
>  	start_descr->prev = (descr - 1);
>
> -	descr = start_descr;
> -	for (i = 0; i < descr_count; i++, descr++) {
> +	/* chain bus addr of hw descriptor */
> +	for (index = 0, descr = start_descr; index < descr_count;
> +		index++, descr++) {
>  		descr->hw_regs.next_descr_addr =
>  			cpu_to_be32(descr->next->link.cpu_addr);
>  	}
> @@ -373,12 +383,6 @@ static int gelic_card_init_chain(struct  
> gelic_card *card,
>  	(descr - 1)->hw_regs.next_descr_addr = 0;
>
>  	return 0;
> -
> -iommu_error:
> -	for (i--, descr--; 0 <= i; i--, descr--)
> -		if (descr->link.cpu_addr)
> -			gelic_unmap_link(dev, descr);
> -	return -ENOMEM;
>  }
>
>  /**
> @@ -395,49 +399,63 @@ static int gelic_descr_prepare_rx(struct  
> gelic_card *card,
>  	struct gelic_descr *descr)
>  {
>  	struct device *dev = ctodev(card);
> -	int offset;
> -	unsigned int bufsize;
> +	struct aligned_buff {
> +		unsigned int total_bytes;
> +		unsigned int offset;
> +	};
> +	struct aligned_buff a_buf;
> +	dma_addr_t cpu_addr;
>
>  	if (gelic_descr_get_status(descr) !=  GELIC_DESCR_DMA_NOT_IN_USE) {
>  		dev_err(dev, "%s:%d: ERROR status\n", __func__, __LINE__);
>  	}
>
> -	/* we need to round up the buffer size to a multiple of 128 */
> -	bufsize = ALIGN(GELIC_NET_MAX_MTU, GELIC_NET_RXBUF_ALIGN);
> +	a_buf.total_bytes = ALIGN(GELIC_NET_MAX_MTU, GELIC_NET_RXBUF_ALIGN)
> +		+ GELIC_NET_RXBUF_ALIGN;
> +
> +	descr->skb = dev_alloc_skb(a_buf.total_bytes);
>
> -	/* and we need to have it 128 byte aligned, therefore we allocate a
> -	 * bit more */
> -	descr->skb = dev_alloc_skb(bufsize + GELIC_NET_RXBUF_ALIGN - 1);
>  	if (!descr->skb) {
> -		descr->hw_regs.payload.dev_addr = 0; /* tell DMAC don't touch memory */
> +		descr->hw_regs.payload.dev_addr = 0;
> +		descr->hw_regs.payload.size = 0;
>  		return -ENOMEM;
>  	}
> -	descr->hw_regs.payload.size = cpu_to_be32(bufsize);
> +
> +	a_buf.offset = PTR_ALIGN(descr->skb->data, GELIC_NET_RXBUF_ALIGN)
> +		- descr->skb->data;
> +
> +	if (a_buf.offset) {
> +		dev_dbg(dev, "%s:%d: offset=%u\n", __func__, __LINE__,
> +			a_buf.offset);
> +		skb_reserve(descr->skb, a_buf.offset);
> +	}
> +
>  	descr->hw_regs.dmac_cmd_status = 0;
>  	descr->hw_regs.result_size = 0;
>  	descr->hw_regs.valid_size = 0;
>  	descr->hw_regs.data_error = 0;
>
> -	offset = ((unsigned long)descr->skb->data) &
> -		(GELIC_NET_RXBUF_ALIGN - 1);
> -	if (offset)
> -		skb_reserve(descr->skb, GELIC_NET_RXBUF_ALIGN - offset);
> -	/* io-mmu-map the skb */
> -	descr->hw_regs.payload.dev_addr = cpu_to_be32(dma_map_single(dev,
> -						     descr->skb->data,
> -						     GELIC_NET_MAX_MTU,
> -						     DMA_FROM_DEVICE));
> -	if (!descr->hw_regs.payload.dev_addr) {
> +	descr->hw_regs.payload.size = a_buf.total_bytes - a_buf.offset;
> +	cpu_addr = dma_map_single(dev, descr->skb->data,
> +		descr->hw_regs.payload.size, DMA_FROM_DEVICE);
> +	descr->hw_regs.payload.dev_addr = cpu_to_be32(cpu_addr);
> +
> +	if (unlikely(dma_mapping_error(dev, cpu_addr))) {
> +		dev_err(dev, "%s:%d: dma_mapping_error\n", __func__, __LINE__);
> +
> +		descr->hw_regs.payload.dev_addr = 0;
> +		descr->hw_regs.payload.size = 0;
> +
>  		dev_kfree_skb_any(descr->skb);
>  		descr->skb = NULL;
> -		dev_info(dev,
> -			 "%s:Could not iommu-map rx buffer\n", __func__);
> +
>  		gelic_descr_set_status(descr, GELIC_DESCR_DMA_NOT_IN_USE);
> +
>  		return -ENOMEM;
> -	} else {
> -		gelic_descr_set_status(descr, GELIC_DESCR_DMA_CARDOWNED);
> -		return 0;
>  	}
> +
> +	gelic_descr_set_status(descr, GELIC_DESCR_DMA_CARDOWNED);
> +	return 0;
>  }
>
>  /**
> @@ -454,13 +472,18 @@ static void gelic_card_release_rx_chain(struct  
> gelic_card *card)
>  		if (descr->skb) {
>  			dma_unmap_single(dev,
>  				be32_to_cpu(descr->hw_regs.payload.dev_addr),
> -				descr->skb->len, DMA_FROM_DEVICE);
> -			descr->hw_regs.payload.dev_addr = 0;
> +				descr->hw_regs.payload.size, DMA_FROM_DEVICE);
> +
>  			dev_kfree_skb_any(descr->skb);
>  			descr->skb = NULL;
> +
>  			gelic_descr_set_status(descr,
>  				GELIC_DESCR_DMA_NOT_IN_USE);
>  		}
> +
> +		descr->hw_regs.payload.dev_addr = 0;
> +		descr->hw_regs.payload.size = 0;
> +
>  		descr = descr->next;
>  	} while (descr != card->rx_chain.head);
>  }
> @@ -526,17 +549,19 @@ static void gelic_descr_release_tx(struct  
> gelic_card *card,
>  		GELIC_DESCR_TX_TAIL));
>
>  	dma_unmap_single(dev, be32_to_cpu(descr->hw_regs.payload.dev_addr),
> -		skb->len, DMA_TO_DEVICE);
> -	dev_kfree_skb_any(skb);
> +		descr->hw_regs.payload.size, DMA_TO_DEVICE);
>
>  	descr->hw_regs.payload.dev_addr = 0;
>  	descr->hw_regs.payload.size = 0;
> +
> +	dev_kfree_skb_any(skb);
> +	descr->skb = NULL;
> +
>  	descr->hw_regs.next_descr_addr = 0;
>  	descr->hw_regs.result_size = 0;
>  	descr->hw_regs.valid_size = 0;
>  	descr->hw_regs.data_status = 0;
>  	descr->hw_regs.data_error = 0;
> -	descr->skb = NULL;
>
>  	gelic_descr_set_status(descr, GELIC_DESCR_DMA_NOT_IN_USE);
>  }
> @@ -565,31 +590,34 @@ static void gelic_card_wake_queues(struct  
> gelic_card *card)
>  static void gelic_card_release_tx_chain(struct gelic_card *card, int stop)
>  {
>  	struct gelic_descr_chain *tx_chain;
> -	enum gelic_descr_dma_status status;
>  	struct device *dev = ctodev(card);
> -	struct net_device *netdev;
> -	int release = 0;
> +	int release;
> +
> +	for (release = 0, tx_chain = &card->tx_chain;
> +		tx_chain->head != tx_chain->tail && tx_chain->tail;
> +		tx_chain->tail = tx_chain->tail->next) {
> +		enum gelic_descr_dma_status status;
> +		struct gelic_descr *descr;
> +		struct net_device *netdev;
> +
> +		descr = tx_chain->tail;
> +		status = gelic_descr_get_status(descr);
> +		netdev = descr->skb->dev;
>
> -	for (tx_chain = &card->tx_chain;
> -	     tx_chain->head != tx_chain->tail && tx_chain->tail;
> -	     tx_chain->tail = tx_chain->tail->next) {
> -		status = gelic_descr_get_status(tx_chain->tail);
> -		netdev = tx_chain->tail->skb->dev;
>  		switch (status) {
>  		case GELIC_DESCR_DMA_RESPONSE_ERROR:
>  		case GELIC_DESCR_DMA_PROTECTION_ERROR:
>  		case GELIC_DESCR_DMA_FORCE_END:
> -			 dev_info_ratelimited(dev,
> -					 "%s:%d: forcing end of tx descriptor with status %x\n",
> -					 __func__, __LINE__, status);
> +			dev_info_ratelimited(dev,
> +				"%s:%d: forcing end of tx descriptor with status %x\n",
> +				__func__, __LINE__, status);
>  			netdev->stats.tx_dropped++;
>  			break;
>
>  		case GELIC_DESCR_DMA_COMPLETE:
> -			if (tx_chain->tail->skb) {
> +			if (descr->skb) {
>  				netdev->stats.tx_packets++;
> -				netdev->stats.tx_bytes +=
> -					tx_chain->tail->skb->len;
> +				netdev->stats.tx_bytes += descr->skb->len;
>  			}
>  			break;
>
> @@ -599,7 +627,7 @@ static void gelic_card_release_tx_chain(struct  
> gelic_card *card, int stop)
>  			}
>  		}
>
> -		gelic_descr_release_tx(card, tx_chain->tail);
> +		gelic_descr_release_tx(card, descr);
>  		release++;
>  	}
>  out:
> @@ -703,19 +731,19 @@ int gelic_net_stop(struct net_device *netdev)
>   *
>   * returns the address of the next descriptor, or NULL if not available.
>   */
> -static struct gelic_descr *
> -gelic_card_get_next_tx_descr(struct gelic_card *card)
> +static struct gelic_descr *gelic_card_get_next_tx_descr(struct  
> gelic_card *card)
>  {
>  	if (!card->tx_chain.head)
>  		return NULL;
> +
>  	/*  see if the next descriptor is free */
>  	if (card->tx_chain.tail != card->tx_chain.head->next &&
> -		gelic_descr_get_status(card->tx_chain.head) ==
> -			GELIC_DESCR_DMA_NOT_IN_USE)
> +		(gelic_descr_get_status(card->tx_chain.head) ==
> +			GELIC_DESCR_DMA_NOT_IN_USE)) {
>  		return card->tx_chain.head;
> -	else
> -		return NULL;
> +	}
>
> +	return NULL;
>  }
>
>  /**
> @@ -809,18 +837,23 @@ static int gelic_descr_prepare_tx(struct  
> gelic_card *card,
>  		if (!skb_tmp) {
>  			return -ENOMEM;
>  		}
> +
>  		skb = skb_tmp;
>  	}
>
> -	cpu_addr = dma_map_single(dev, skb->data, skb->len, DMA_TO_DEVICE);
> +	descr->hw_regs.payload.size = skb->len;
> +	cpu_addr = dma_map_single(dev, skb->data, descr->hw_regs.payload.size,
> +		DMA_TO_DEVICE);
> +	descr->hw_regs.payload.dev_addr = cpu_to_be32(cpu_addr);
>
> -	if (!cpu_addr) {
> +	if (unlikely(dma_mapping_error(dev, cpu_addr))) {
>  		dev_err(dev, "%s:%d: dma_mapping_error\n", __func__, __LINE__);
> +
> +		descr->hw_regs.payload.dev_addr = 0;
> +		descr->hw_regs.payload.size = 0;
>  		return -ENOMEM;
>  	}
>
> -	descr->hw_regs.payload.dev_addr = cpu_to_be32(cpu_addr);
> -	descr->hw_regs.payload.size = cpu_to_be32(skb->len);
>  	descr->skb = skb;
>  	descr->hw_regs.data_status = 0;
>  	descr->hw_regs.next_descr_addr = 0; /* terminate hw descr */
> @@ -948,9 +981,9 @@ static void gelic_net_pass_skb_up(struct  
> gelic_descr *descr,
>
>  	data_status = be32_to_cpu(descr->hw_regs.data_status);
>  	data_error = be32_to_cpu(descr->hw_regs.data_error);
> -	/* unmap skb buffer */
> +
>  	dma_unmap_single(dev, be32_to_cpu(descr->hw_regs.payload.dev_addr),
> -			 GELIC_NET_MAX_MTU, DMA_FROM_DEVICE);
> +			 descr->hw_regs.payload.size, DMA_FROM_DEVICE);
>
>  	skb_put(skb, be32_to_cpu(descr->hw_regs.valid_size) ?
>  		be32_to_cpu(descr->hw_regs.valid_size) :
> --
> 2.25.1



  parent reply	other threads:[~2021-07-25 18:33 UTC|newest]

Thread overview: 16+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
     [not found] <cover.1627068552.git.geoff@infradead.org>
     [not found] ` <56efff53fcf563a1741904ea0f078d50c378b6cc.1627068552.git.geoff@infradead.org>
2021-07-24 15:44   ` [PATCH v4 03/10] net/ps3_gelic: Format cleanups Christophe Leroy
2021-08-05  5:06   ` Christophe Leroy
     [not found] ` <26f56a1c8332d227156cd33586b176a329570117.1627068552.git.geoff@infradead.org>
2021-07-24 15:51   ` [PATCH v4 02/10] net/ps3_gelic: Use local dev variable Christophe Leroy
2021-08-05  5:05   ` Christophe Leroy
     [not found] ` <c95aa8e57aca8b3af6893f13f2e03731f8198184.1627068552.git.geoff@infradead.org>
2021-08-05  5:03   ` [PATCH v4 01/10] net/ps3_gelic: Add gelic_descr structures Christophe Leroy
     [not found] ` <bc659850d4eec3b2358c1ccb0e00952ceaa6012f.1627068552.git.geoff@infradead.org>
2021-07-25 18:20   ` [PATCH v4 04/10] net/ps3_gelic: Add new macro BUG_ON_DEBUG Christophe Leroy
2021-08-05  5:07   ` Christophe Leroy
     [not found] ` <1cdd7f718dde93dcaebf7ddd025869901aa30523.1627068552.git.geoff@infradead.org>
2021-08-05  5:07   ` [PATCH v4 05/10] net/ps3_gelic: Add vlan_id structure Christophe Leroy
     [not found] ` <8421aa2c148d840b11b7115208e5276017999c2a.1627068552.git.geoff@infradead.org>
2021-08-05  5:08   ` [PATCH v4 06/10] net/ps3_gelic: Cleanup debug code Christophe Leroy
     [not found] ` <024b88e07095f00bc2eabfae2f526851600ee272.1627068552.git.geoff@infradead.org>
2021-08-05  5:09   ` [PATCH v4 07/10] net/ps3_gelic: Add new routine gelic_unmap_link Christophe Leroy
     [not found] ` <07e42ec30037d514c1d63f33efe4642364d89802.1627068552.git.geoff@infradead.org>
2021-07-25 18:27   ` [PATCH v4 08/10] net/ps3_gelic: Rename no to descr_count Christophe Leroy
2021-08-05  5:09   ` Christophe Leroy
     [not found] ` <5634f7c76a67345c9735e05b68228ea899a8bf9d.1627068552.git.geoff@infradead.org>
2021-07-25 18:29   ` [PATCH v4 09/10] net/ps3_gelic: Add new routine gelic_work_to_card Christophe Leroy
2021-08-05  5:10   ` Christophe Leroy
     [not found] ` <7aa1d9b1b4ffadcbdc6f88e4f8d4a323da307595.1627068552.git.geoff@infradead.org>
2021-07-25 18:38   ` Christophe Leroy [this message]
2021-08-05  5:10   ` [PATCH v4 10/10] net/ps3_gelic: Fix DMA mapping problems Christophe Leroy

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20210725203804.Horde.sruKcwZQKonIWKjB98332A2@messagerie.c-s.fr \
    --to=christophe.leroy@csgroup.eu \
    --cc=davem@davemloft.net \
    --cc=geoff@infradead.org \
    --cc=kuba@kernel.org \
    --cc=linuxppc-dev@lists.ozlabs.org \
    --cc=netdev@vger.kernel.org \
    --subject='Re: [PATCH v4 10/10] net/ps3_gelic: Fix DMA mapping problems' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).