diff mbox series

[net-next,2/9] ibmvnic: Fix up some comments and messages

Message ID 20210901000812.120968-3-sukadev@linux.ibm.com (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series ibmvnic: Reuse ltb, rx, tx pools | expand

Checks

Context Check Description
netdev/cover_letter success Link
netdev/fixes_present success Link
netdev/patch_count success Link
netdev/tree_selection success Clearly marked for net-next
netdev/subject_prefix success Link
netdev/cc_maintainers warning 7 maintainers not CCed: mpe@ellerman.id.au paulus@samba.org tlfalcon@linux.ibm.com davem@davemloft.net linuxppc-dev@lists.ozlabs.org kuba@kernel.org benh@kernel.crashing.org
netdev/source_inline success Was 0 now: 0
netdev/verify_signedoff success Link
netdev/module_param success Was 0 now: 0
netdev/build_32bit success Errors and warnings before: 0 this patch: 0
netdev/kdoc fail Errors and warnings before: 0 this patch: 4
netdev/verify_fixes success Link
netdev/checkpatch warning WARNING: Possible unnecessary 'out of memory' message
netdev/build_allmodconfig_warn success Errors and warnings before: 0 this patch: 0
netdev/header_inline success Link

Commit Message

Sukadev Bhattiprolu Sept. 1, 2021, 12:08 a.m. UTC 
Add/update some comments/function headers and fix up some messages.

Signed-off-by: Sukadev Bhattiprolu <sukadev@linux.ibm.com>
---
 drivers/net/ethernet/ibm/ibmvnic.c | 40 +++++++++++++++++++++++++-----
 1 file changed, 34 insertions(+), 6 deletions(-)

Comments

Dany Madden Sept. 1, 2021, 1:28 a.m. UTC | #1 
On 2021-08-31 17:08, Sukadev Bhattiprolu wrote:
> Add/update some comments/function headers and fix up some messages.
> 
> Signed-off-by: Sukadev Bhattiprolu <sukadev@linux.ibm.com>
Reviewed-by: Dany Madden <drt@linux.ibm.com>

> ---
>  drivers/net/ethernet/ibm/ibmvnic.c | 40 +++++++++++++++++++++++++-----
>  1 file changed, 34 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/net/ethernet/ibm/ibmvnic.c
> b/drivers/net/ethernet/ibm/ibmvnic.c
> index e8b1231be485..911315b10731 100644
> --- a/drivers/net/ethernet/ibm/ibmvnic.c
> +++ b/drivers/net/ethernet/ibm/ibmvnic.c
> @@ -243,14 +243,13 @@ static int alloc_long_term_buff(struct
> ibmvnic_adapter *adapter,
> 
>  	rc = ibmvnic_wait_for_completion(adapter, &adapter->fw_done, 10000);
>  	if (rc) {
> -		dev_err(dev,
> -			"Long term map request aborted or timed out,rc = %d\n",
> +		dev_err(dev, "LTB map request aborted or timed out, rc = %d\n",
>  			rc);
>  		goto out;
>  	}
> 
>  	if (adapter->fw_done_rc) {
> -		dev_err(dev, "Couldn't map long term buffer,rc = %d\n",
> +		dev_err(dev, "Couldn't map LTB, rc = %d\n",
>  			adapter->fw_done_rc);
>  		rc = -1;
>  		goto out;
> @@ -281,7 +280,9 @@ static void free_long_term_buff(struct
> ibmvnic_adapter *adapter,
>  	    adapter->reset_reason != VNIC_RESET_MOBILITY &&
>  	    adapter->reset_reason != VNIC_RESET_TIMEOUT)
>  		send_request_unmap(adapter, ltb->map_id);
> +
>  	dma_free_coherent(dev, ltb->size, ltb->buff, ltb->addr);
> +
>  	ltb->buff = NULL;
>  	ltb->map_id = 0;
>  }
> @@ -574,6 +575,10 @@ static int reset_rx_pools(struct ibmvnic_adapter 
> *adapter)
>  	return 0;
>  }
> 
> +/**
> + * Release any rx_pools attached to @adapter.
> + * Safe to call this multiple times - even if no pools are attached.
> + */
>  static void release_rx_pools(struct ibmvnic_adapter *adapter)
>  {
>  	struct ibmvnic_rx_pool *rx_pool;
> @@ -628,6 +633,9 @@ static int init_rx_pools(struct net_device *netdev)
>  		return -1;
>  	}
> 
> +	/* Set num_active_rx_pools early. If we fail below after partial
> +	 * allocation, release_rx_pools() will know how many to look for.
> +	 */
>  	adapter->num_active_rx_pools = rxadd_subcrqs;
> 
>  	for (i = 0; i < rxadd_subcrqs; i++) {
> @@ -646,6 +654,7 @@ static int init_rx_pools(struct net_device *netdev)
>  		rx_pool->free_map = kcalloc(rx_pool->size, sizeof(int),
>  					    GFP_KERNEL);
>  		if (!rx_pool->free_map) {
> +			dev_err(dev, "Couldn't alloc free_map %d\n", i);
>  			release_rx_pools(adapter);
>  			return -1;
>  		}
> @@ -739,10 +748,17 @@ static void release_one_tx_pool(struct
> ibmvnic_adapter *adapter,
>  	free_long_term_buff(adapter, &tx_pool->long_term_buff);
>  }
> 
> +/**
> + * Release any tx and tso pools attached to @adapter.
> + * Safe to call this multiple times - even if no pools are attached.
> + */
>  static void release_tx_pools(struct ibmvnic_adapter *adapter)
>  {
>  	int i;
> 
> +	/* init_tx_pools() ensures that ->tx_pool and ->tso_pool are
> +	 * both NULL or both non-NULL. So we only need to check one.
> +	 */
>  	if (!adapter->tx_pool)
>  		return;
> 
> @@ -793,6 +809,7 @@ static int init_one_tx_pool(struct net_device 
> *netdev,
>  static int init_tx_pools(struct net_device *netdev)
>  {
>  	struct ibmvnic_adapter *adapter = netdev_priv(netdev);
> +	struct device *dev = &adapter->vdev->dev;
>  	int tx_subcrqs;
>  	u64 buff_size;
>  	int i, rc;
> @@ -805,17 +822,27 @@ static int init_tx_pools(struct net_device 
> *netdev)
> 
>  	adapter->tso_pool = kcalloc(tx_subcrqs,
>  				    sizeof(struct ibmvnic_tx_pool), GFP_KERNEL);
> +	/* To simplify release_tx_pools() ensure that ->tx_pool and
> +	 * ->tso_pool are either both NULL or both non-NULL.
> +	 */
>  	if (!adapter->tso_pool) {
>  		kfree(adapter->tx_pool);
>  		adapter->tx_pool = NULL;
>  		return -1;
>  	}
> 
> +	/* Set num_active_tx_pools early. If we fail below after partial
> +	 * allocation, release_tx_pools() will know how many to look for.
> +	 */
>  	adapter->num_active_tx_pools = tx_subcrqs;
> 
>  	for (i = 0; i < tx_subcrqs; i++) {
>  		buff_size = adapter->req_mtu + VLAN_HLEN;
>  		buff_size = ALIGN(buff_size, L1_CACHE_BYTES);
> +
> +		dev_dbg(dev, "Init tx pool %d [%llu, %llu]\n",
> +			i, adapter->req_tx_entries_per_subcrq, buff_size);
> +
>  		rc = init_one_tx_pool(netdev, &adapter->tx_pool[i],
>  				      adapter->req_tx_entries_per_subcrq,
>  				      buff_size);
> @@ -4774,9 +4801,10 @@ static void handle_query_map_rsp(union 
> ibmvnic_crq *crq,
>  		dev_err(dev, "Error %ld in QUERY_MAP_RSP\n", rc);
>  		return;
>  	}
> -	netdev_dbg(netdev, "page_size = %d\ntot_pages = %d\nfree_pages = 
> %d\n",
> -		   crq->query_map_rsp.page_size, crq->query_map_rsp.tot_pages,
> -		   crq->query_map_rsp.free_pages);
> +	netdev_dbg(netdev, "page_size = %d\ntot_pages = %u\nfree_pages = 
> %u\n",
> +		   crq->query_map_rsp.page_size,
> +		   __be32_to_cpu(crq->query_map_rsp.tot_pages),
> +		   __be32_to_cpu(crq->query_map_rsp.free_pages));
>  }
> 
>  static void handle_query_cap_rsp(union ibmvnic_crq *crq,
kernel test robot Sept. 1, 2021, 8:58 a.m. UTC | #2 
Hi Sukadev,

I love your patch! Perhaps something to improve:

[auto build test WARNING on net-next/master]

url:    https://github.com/0day-ci/linux/commits/Sukadev-Bhattiprolu/ibmvnic-Reuse-ltb-rx-tx-pools/20210901-081123
base:   https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git 29ce8f9701072fc221d9c38ad952de1a9578f95c
config: powerpc-allyesconfig (attached as .config)
compiler: powerpc64-linux-gcc (GCC) 11.2.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/a28141472dfbc71fb3b53b1ba9213b3450435588
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Sukadev-Bhattiprolu/ibmvnic-Reuse-ltb-rx-tx-pools/20210901-081123
        git checkout a28141472dfbc71fb3b53b1ba9213b3450435588
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross ARCH=powerpc 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

>> drivers/net/ethernet/ibm/ibmvnic.c:579: warning: This comment starts with '/**', but isn't a kernel-doc comment. Refer Documentation/doc-guide/kernel-doc.rst
    * Release any rx_pools attached to @adapter.
   drivers/net/ethernet/ibm/ibmvnic.c:752: warning: This comment starts with '/**', but isn't a kernel-doc comment. Refer Documentation/doc-guide/kernel-doc.rst
    * Release any tx and tso pools attached to @adapter.


vim +579 drivers/net/ethernet/ibm/ibmvnic.c

   577	
   578	/**
 > 579	 * Release any rx_pools attached to @adapter.
   580	 * Safe to call this multiple times - even if no pools are attached.
   581	 */
   582	static void release_rx_pools(struct ibmvnic_adapter *adapter)
   583	{
   584		struct ibmvnic_rx_pool *rx_pool;
   585		int i, j;
   586	
   587		if (!adapter->rx_pool)
   588			return;
   589	
   590		for (i = 0; i < adapter->num_active_rx_pools; i++) {
   591			rx_pool = &adapter->rx_pool[i];
   592	
   593			netdev_dbg(adapter->netdev, "Releasing rx_pool[%d]\n", i);
   594	
   595			kfree(rx_pool->free_map);
   596			free_long_term_buff(adapter, &rx_pool->long_term_buff);
   597	
   598			if (!rx_pool->rx_buff)
   599				continue;
   600	
   601			for (j = 0; j < rx_pool->size; j++) {
   602				if (rx_pool->rx_buff[j].skb) {
   603					dev_kfree_skb_any(rx_pool->rx_buff[j].skb);
   604					rx_pool->rx_buff[j].skb = NULL;
   605				}
   606			}
   607	
   608			kfree(rx_pool->rx_buff);
   609		}
   610	
   611		kfree(adapter->rx_pool);
   612		adapter->rx_pool = NULL;
   613		adapter->num_active_rx_pools = 0;
   614	}
   615	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
diff mbox series

Patch

diff --git a/drivers/net/ethernet/ibm/ibmvnic.c b/drivers/net/ethernet/ibm/ibmvnic.c
index e8b1231be485..911315b10731 100644
--- a/drivers/net/ethernet/ibm/ibmvnic.c
+++ b/drivers/net/ethernet/ibm/ibmvnic.c
@@ -243,14 +243,13 @@  static int alloc_long_term_buff(struct ibmvnic_adapter *adapter,
 
 	rc = ibmvnic_wait_for_completion(adapter, &adapter->fw_done, 10000);
 	if (rc) {
-		dev_err(dev,
-			"Long term map request aborted or timed out,rc = %d\n",
+		dev_err(dev, "LTB map request aborted or timed out, rc = %d\n",
 			rc);
 		goto out;
 	}
 
 	if (adapter->fw_done_rc) {
-		dev_err(dev, "Couldn't map long term buffer,rc = %d\n",
+		dev_err(dev, "Couldn't map LTB, rc = %d\n",
 			adapter->fw_done_rc);
 		rc = -1;
 		goto out;
@@ -281,7 +280,9 @@  static void free_long_term_buff(struct ibmvnic_adapter *adapter,
 	    adapter->reset_reason != VNIC_RESET_MOBILITY &&
 	    adapter->reset_reason != VNIC_RESET_TIMEOUT)
 		send_request_unmap(adapter, ltb->map_id);
+
 	dma_free_coherent(dev, ltb->size, ltb->buff, ltb->addr);
+
 	ltb->buff = NULL;
 	ltb->map_id = 0;
 }
@@ -574,6 +575,10 @@  static int reset_rx_pools(struct ibmvnic_adapter *adapter)
 	return 0;
 }
 
+/**
+ * Release any rx_pools attached to @adapter.
+ * Safe to call this multiple times - even if no pools are attached.
+ */
 static void release_rx_pools(struct ibmvnic_adapter *adapter)
 {
 	struct ibmvnic_rx_pool *rx_pool;
@@ -628,6 +633,9 @@  static int init_rx_pools(struct net_device *netdev)
 		return -1;
 	}
 
+	/* Set num_active_rx_pools early. If we fail below after partial
+	 * allocation, release_rx_pools() will know how many to look for.
+	 */
 	adapter->num_active_rx_pools = rxadd_subcrqs;
 
 	for (i = 0; i < rxadd_subcrqs; i++) {
@@ -646,6 +654,7 @@  static int init_rx_pools(struct net_device *netdev)
 		rx_pool->free_map = kcalloc(rx_pool->size, sizeof(int),
 					    GFP_KERNEL);
 		if (!rx_pool->free_map) {
+			dev_err(dev, "Couldn't alloc free_map %d\n", i);
 			release_rx_pools(adapter);
 			return -1;
 		}
@@ -739,10 +748,17 @@  static void release_one_tx_pool(struct ibmvnic_adapter *adapter,
 	free_long_term_buff(adapter, &tx_pool->long_term_buff);
 }
 
+/**
+ * Release any tx and tso pools attached to @adapter.
+ * Safe to call this multiple times - even if no pools are attached.
+ */
 static void release_tx_pools(struct ibmvnic_adapter *adapter)
 {
 	int i;
 
+	/* init_tx_pools() ensures that ->tx_pool and ->tso_pool are
+	 * both NULL or both non-NULL. So we only need to check one.
+	 */
 	if (!adapter->tx_pool)
 		return;
 
@@ -793,6 +809,7 @@  static int init_one_tx_pool(struct net_device *netdev,
 static int init_tx_pools(struct net_device *netdev)
 {
 	struct ibmvnic_adapter *adapter = netdev_priv(netdev);
+	struct device *dev = &adapter->vdev->dev;
 	int tx_subcrqs;
 	u64 buff_size;
 	int i, rc;
@@ -805,17 +822,27 @@  static int init_tx_pools(struct net_device *netdev)
 
 	adapter->tso_pool = kcalloc(tx_subcrqs,
 				    sizeof(struct ibmvnic_tx_pool), GFP_KERNEL);
+	/* To simplify release_tx_pools() ensure that ->tx_pool and
+	 * ->tso_pool are either both NULL or both non-NULL.
+	 */
 	if (!adapter->tso_pool) {
 		kfree(adapter->tx_pool);
 		adapter->tx_pool = NULL;
 		return -1;
 	}
 
+	/* Set num_active_tx_pools early. If we fail below after partial
+	 * allocation, release_tx_pools() will know how many to look for.
+	 */
 	adapter->num_active_tx_pools = tx_subcrqs;
 
 	for (i = 0; i < tx_subcrqs; i++) {
 		buff_size = adapter->req_mtu + VLAN_HLEN;
 		buff_size = ALIGN(buff_size, L1_CACHE_BYTES);
+
+		dev_dbg(dev, "Init tx pool %d [%llu, %llu]\n",
+			i, adapter->req_tx_entries_per_subcrq, buff_size);
+
 		rc = init_one_tx_pool(netdev, &adapter->tx_pool[i],
 				      adapter->req_tx_entries_per_subcrq,
 				      buff_size);
@@ -4774,9 +4801,10 @@  static void handle_query_map_rsp(union ibmvnic_crq *crq,
 		dev_err(dev, "Error %ld in QUERY_MAP_RSP\n", rc);
 		return;
 	}
-	netdev_dbg(netdev, "page_size = %d\ntot_pages = %d\nfree_pages = %d\n",
-		   crq->query_map_rsp.page_size, crq->query_map_rsp.tot_pages,
-		   crq->query_map_rsp.free_pages);
+	netdev_dbg(netdev, "page_size = %d\ntot_pages = %u\nfree_pages = %u\n",
+		   crq->query_map_rsp.page_size,
+		   __be32_to_cpu(crq->query_map_rsp.tot_pages),
+		   __be32_to_cpu(crq->query_map_rsp.free_pages));
 }
 
 static void handle_query_cap_rsp(union ibmvnic_crq *crq,