[PATCH net-next 0/7] net: ipa: a few small fixes

[PATCH net-next 0/7] net: ipa: a few small fixes

[Alex Elder]

This series implements some minor bug fixes or improvements.

The first patch removes an apparently unnecessary restriction, which
results in an error on a 32-bit ARM build.

The second makes a definition used for SDM845 match what is used in
the downstream code.

The third just ensures two netdev pointers are only non-null when
valid.

The fourth simplifies a little code, knowing that a called function
never returns an error.

The fifth and sixth just remove some empty/place holder functions.

And the last patch fixes a comment, makes a function private, and
removes an unnecessary double-negation of a Boolean variable.  This
patch produces a warning from checkpatch, indicating that a pair of
parentheses is unnecessary.  I agree with that advice, but it
conflicts with a suggestion from the compiler.  I left the "problem"
in place to avoid the compiler warning.

					-Alex


Alex Elder (7):
  net: ipa: relax pool entry size requirement
  net: ipa: update sequence type for modem TX endpoint
  net: ipa: only set endpoint netdev pointer when in use
  net: ipa: ipa_stop() does not return an error
  net: ipa: get rid of empty IPA functions
  net: ipa: get rid of empty GSI functions
  net: ipa: three small fixes

 drivers/net/ipa/gsi.c             | 54 ++++---------------------------
 drivers/net/ipa/gsi_trans.c       |  4 +--
 drivers/net/ipa/ipa_data-v3.5.1.c |  1 +
 drivers/net/ipa/ipa_endpoint.c    |  6 ++--
 drivers/net/ipa/ipa_endpoint.h    |  2 --
 drivers/net/ipa/ipa_main.c        | 29 ++++++-----------
 drivers/net/ipa/ipa_mem.c         |  9 ++----
 drivers/net/ipa/ipa_mem.h         |  5 ++-
 drivers/net/ipa/ipa_modem.c       | 34 ++++++++-----------
 drivers/net/ipa/ipa_resource.c    |  8 +----
 drivers/net/ipa/ipa_resource.h    |  8 ++---
 drivers/net/ipa/ipa_table.c       | 26 ++-------------
 drivers/net/ipa/ipa_table.h       | 16 +++------
 13 files changed, 49 insertions(+), 153 deletions(-)

-- 
2.27.0

[PATCH net-next 1/7] net: ipa: relax pool entry size requirement

[Alex Elder]

I no longer know why a validation check ensured the size of an entry
passed to gsi_trans_pool_init() was restricted to be a multiple of 8.
For 32-bit builds, this condition doesn't always hold, and for DMA
pools, the size is rounded up to a power of 2 anyway.

Remove this restriction.

Signed-off-by: Alex Elder <elder@linaro.org>
---
 drivers/net/ipa/gsi_trans.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ipa/gsi_trans.c b/drivers/net/ipa/gsi_trans.c
index 70c2b585f98d6..8c795a6a85986 100644
--- a/drivers/net/ipa/gsi_trans.c
+++ b/drivers/net/ipa/gsi_trans.c
@@ -91,7 +91,7 @@ int gsi_trans_pool_init(struct gsi_trans_pool *pool, size_t size, u32 count,
 	void *virt;
 
 #ifdef IPA_VALIDATE
-	if (!size || size % 8)
+	if (!size)
 		return -EINVAL;
 	if (count < max_alloc)
 		return -EINVAL;
@@ -141,7 +141,7 @@ int gsi_trans_pool_init_dma(struct device *dev, struct gsi_trans_pool *pool,
 	void *virt;
 
 #ifdef IPA_VALIDATE
-	if (!size || size % 8)
+	if (!size)
 		return -EINVAL;
 	if (count < max_alloc)
 		return -EINVAL;
-- 
2.27.0

[PATCH net-next 2/7] net: ipa: update sequence type for modem TX endpoint

[Alex Elder]

On IPA v3.5.1, the sequencer type for the modem TX endpoint does not
define the replication portion in the same way the downstream code
does.  This difference doesn't affect the behavior of the upstream
code, but I'd prefer the two code bases use the same configuration
value here.

Signed-off-by: Alex Elder <elder@linaro.org>
---
 drivers/net/ipa/ipa_data-v3.5.1.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/ipa/ipa_data-v3.5.1.c b/drivers/net/ipa/ipa_data-v3.5.1.c
index 57703e95a3f9c..ead1a82f32f5c 100644
--- a/drivers/net/ipa/ipa_data-v3.5.1.c
+++ b/drivers/net/ipa/ipa_data-v3.5.1.c
@@ -116,6 +116,7 @@ static const struct ipa_gsi_endpoint_data ipa_gsi_endpoint_data[] = {
 				.status_enable	= true,
 				.tx = {
 					.seq_type = IPA_SEQ_2_PASS_SKIP_LAST_UC,
+					.seq_rep_type = IPA_SEQ_REP_DMA_PARSER,
 					.status_endpoint =
 						IPA_ENDPOINT_MODEM_AP_RX,
 				},
-- 
2.27.0

[PATCH net-next 3/7] net: ipa: only set endpoint netdev pointer when in use

[Alex Elder]

In ipa_modem_start(), we set endpoint netdev pointers before the
network device is registered.  If registration fails, we don't undo
those assignments.  Instead, wait to assign the netdev pointer until
after registration succeeds.

Set these endpoint netdev pointers to NULL in ipa_modem_stop()
before unregistering the network device.

Signed-off-by: Alex Elder <elder@linaro.org>
---
 drivers/net/ipa/ipa_modem.c | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/drivers/net/ipa/ipa_modem.c b/drivers/net/ipa/ipa_modem.c
index 9b08eb8239846..8a6ccebde2889 100644
--- a/drivers/net/ipa/ipa_modem.c
+++ b/drivers/net/ipa/ipa_modem.c
@@ -1,7 +1,7 @@
 // SPDX-License-Identifier: GPL-2.0
 
 /* Copyright (c) 2014-2018, The Linux Foundation. All rights reserved.
- * Copyright (C) 2018-2020 Linaro Ltd.
+ * Copyright (C) 2018-2021 Linaro Ltd.
  */
 
 #include <linux/errno.h>
@@ -213,18 +213,18 @@ int ipa_modem_start(struct ipa *ipa)
 		goto out_set_state;
 	}
 
-	ipa->name_map[IPA_ENDPOINT_AP_MODEM_TX]->netdev = netdev;
-	ipa->name_map[IPA_ENDPOINT_AP_MODEM_RX]->netdev = netdev;
-
 	SET_NETDEV_DEV(netdev, &ipa->pdev->dev);
 	priv = netdev_priv(netdev);
 	priv->ipa = ipa;
 
 	ret = register_netdev(netdev);
-	if (ret)
-		free_netdev(netdev);
-	else
+	if (!ret) {
 		ipa->modem_netdev = netdev;
+		ipa->name_map[IPA_ENDPOINT_AP_MODEM_TX]->netdev = netdev;
+		ipa->name_map[IPA_ENDPOINT_AP_MODEM_RX]->netdev = netdev;
+	} else {
+		free_netdev(netdev);
+	}
 
 out_set_state:
 	if (ret)
@@ -263,6 +263,8 @@ int ipa_modem_stop(struct ipa *ipa)
 		if (ret)
 			goto out_set_state;
 
+		ipa->name_map[IPA_ENDPOINT_AP_MODEM_RX]->netdev = NULL;
+		ipa->name_map[IPA_ENDPOINT_AP_MODEM_TX]->netdev = NULL;
 		ipa->modem_netdev = NULL;
 		unregister_netdev(netdev);
 		free_netdev(netdev);
-- 
2.27.0

[PATCH net-next 4/7] net: ipa: ipa_stop() does not return an error

[Alex Elder]

In ipa_modem_stop(), if the modem netdev pointer is non-null we call
ipa_stop().  We check for an error and if one is returned we handle
it.  But ipa_stop() never returns an error, so this extra handling
is unnecessary.  Simplify the code in ipa_modem_stop() based on the
knowledge no error handling is needed at this spot.

Signed-off-by: Alex Elder <elder@linaro.org>
---
 drivers/net/ipa/ipa_modem.c | 18 ++++--------------
 1 file changed, 4 insertions(+), 14 deletions(-)

diff --git a/drivers/net/ipa/ipa_modem.c b/drivers/net/ipa/ipa_modem.c
index 8a6ccebde2889..af9aedbde717a 100644
--- a/drivers/net/ipa/ipa_modem.c
+++ b/drivers/net/ipa/ipa_modem.c
@@ -240,7 +240,6 @@ int ipa_modem_stop(struct ipa *ipa)
 {
 	struct net_device *netdev = ipa->modem_netdev;
 	enum ipa_modem_state state;
-	int ret;
 
 	/* Only attempt to stop the modem if it's running */
 	state = atomic_cmpxchg(&ipa->modem_state, IPA_MODEM_STATE_RUNNING,
@@ -257,29 +256,20 @@ int ipa_modem_stop(struct ipa *ipa)
 	/* Prevent the modem from triggering a call to ipa_setup() */
 	ipa_smp2p_disable(ipa);
 
+	/* Stop the queue and disable the endpoints if it's open */
 	if (netdev) {
-		/* Stop the queue and disable the endpoints if it's open */
-		ret = ipa_stop(netdev);
-		if (ret)
-			goto out_set_state;
-
+		(void)ipa_stop(netdev);
 		ipa->name_map[IPA_ENDPOINT_AP_MODEM_RX]->netdev = NULL;
 		ipa->name_map[IPA_ENDPOINT_AP_MODEM_TX]->netdev = NULL;
 		ipa->modem_netdev = NULL;
 		unregister_netdev(netdev);
 		free_netdev(netdev);
-	} else {
-		ret = 0;
 	}
 
-out_set_state:
-	if (ret)
-		atomic_set(&ipa->modem_state, IPA_MODEM_STATE_RUNNING);
-	else
-		atomic_set(&ipa->modem_state, IPA_MODEM_STATE_STOPPED);
+	atomic_set(&ipa->modem_state, IPA_MODEM_STATE_STOPPED);
 	smp_mb__after_atomic();
 
-	return ret;
+	return 0;
 }
 
 /* Treat a "clean" modem stop the same as a crash */
-- 
2.27.0

[PATCH net-next 5/7] net: ipa: get rid of empty IPA functions

[Alex Elder]

There are place holder functions in the IPA code that do nothing.
For the most part these are inverse functions, for example, once the
routing or filter tables are set up there is no need to perform any
matching teardown activity at shutdown, or in the case of an error.

These can be safely removed, resulting in some code simplification.
Add comments in these spots making it explicit that there is no
inverse.

Signed-off-by: Alex Elder <elder@linaro.org>
---
 drivers/net/ipa/ipa_main.c     | 29 +++++++++--------------------
 drivers/net/ipa/ipa_mem.c      |  9 +++------
 drivers/net/ipa/ipa_mem.h      |  5 ++---
 drivers/net/ipa/ipa_resource.c |  8 +-------
 drivers/net/ipa/ipa_resource.h |  8 ++------
 drivers/net/ipa/ipa_table.c    | 26 +++-----------------------
 drivers/net/ipa/ipa_table.h    | 16 ++++------------
 7 files changed, 24 insertions(+), 77 deletions(-)

diff --git a/drivers/net/ipa/ipa_main.c b/drivers/net/ipa/ipa_main.c
index a970d10e650ef..bfed151f5d6dc 100644
--- a/drivers/net/ipa/ipa_main.c
+++ b/drivers/net/ipa/ipa_main.c
@@ -147,13 +147,13 @@ int ipa_setup(struct ipa *ipa)
 	if (ret)
 		goto err_endpoint_teardown;
 
-	ret = ipa_mem_setup(ipa);
+	ret = ipa_mem_setup(ipa);	/* No matching teardown required */
 	if (ret)
 		goto err_command_disable;
 
-	ret = ipa_table_setup(ipa);
+	ret = ipa_table_setup(ipa);	/* No matching teardown required */
 	if (ret)
-		goto err_mem_teardown;
+		goto err_command_disable;
 
 	/* Enable the exception handling endpoint, and tell the hardware
 	 * to use it by default.
@@ -161,7 +161,7 @@ int ipa_setup(struct ipa *ipa)
 	exception_endpoint = ipa->name_map[IPA_ENDPOINT_AP_LAN_RX];
 	ret = ipa_endpoint_enable_one(exception_endpoint);
 	if (ret)
-		goto err_table_teardown;
+		goto err_command_disable;
 
 	ipa_endpoint_default_route_set(ipa, exception_endpoint->endpoint_id);
 
@@ -179,10 +179,6 @@ int ipa_setup(struct ipa *ipa)
 err_default_route_clear:
 	ipa_endpoint_default_route_clear(ipa);
 	ipa_endpoint_disable_one(exception_endpoint);
-err_table_teardown:
-	ipa_table_teardown(ipa);
-err_mem_teardown:
-	ipa_mem_teardown(ipa);
 err_command_disable:
 	ipa_endpoint_disable_one(command_endpoint);
 err_endpoint_teardown:
@@ -211,8 +207,6 @@ static void ipa_teardown(struct ipa *ipa)
 	ipa_endpoint_default_route_clear(ipa);
 	exception_endpoint = ipa->name_map[IPA_ENDPOINT_AP_LAN_RX];
 	ipa_endpoint_disable_one(exception_endpoint);
-	ipa_table_teardown(ipa);
-	ipa_mem_teardown(ipa);
 	command_endpoint = ipa->name_map[IPA_ENDPOINT_AP_COMMAND_TX];
 	ipa_endpoint_disable_one(command_endpoint);
 	ipa_endpoint_teardown(ipa);
@@ -480,23 +474,20 @@ static int ipa_config(struct ipa *ipa, const struct ipa_data *data)
 	if (ret)
 		goto err_endpoint_deconfig;
 
-	ipa_table_config(ipa);
+	ipa_table_config(ipa);		/* No deconfig required */
 
-	/* Assign resource limitation to each group */
+	/* Assign resource limitation to each group; no deconfig required */
 	ret = ipa_resource_config(ipa, data->resource_data);
 	if (ret)
-		goto err_table_deconfig;
+		goto err_mem_deconfig;
 
 	ret = ipa_modem_config(ipa);
 	if (ret)
-		goto err_resource_deconfig;
+		goto err_mem_deconfig;
 
 	return 0;
 
-err_resource_deconfig:
-	ipa_resource_deconfig(ipa);
-err_table_deconfig:
-	ipa_table_deconfig(ipa);
+err_mem_deconfig:
 	ipa_mem_deconfig(ipa);
 err_endpoint_deconfig:
 	ipa_endpoint_deconfig(ipa);
@@ -514,8 +505,6 @@ static int ipa_config(struct ipa *ipa, const struct ipa_data *data)
 static void ipa_deconfig(struct ipa *ipa)
 {
 	ipa_modem_deconfig(ipa);
-	ipa_resource_deconfig(ipa);
-	ipa_table_deconfig(ipa);
 	ipa_mem_deconfig(ipa);
 	ipa_endpoint_deconfig(ipa);
 	ipa_hardware_deconfig(ipa);
diff --git a/drivers/net/ipa/ipa_mem.c b/drivers/net/ipa/ipa_mem.c
index 32907dde5dc6a..c5c3b1b7e67d5 100644
--- a/drivers/net/ipa/ipa_mem.c
+++ b/drivers/net/ipa/ipa_mem.c
@@ -1,7 +1,7 @@
 // SPDX-License-Identifier: GPL-2.0
 
 /* Copyright (c) 2012-2018, The Linux Foundation. All rights reserved.
- * Copyright (C) 2019-2020 Linaro Ltd.
+ * Copyright (C) 2019-2021 Linaro Ltd.
  */
 
 #include <linux/types.h>
@@ -53,6 +53,8 @@ ipa_mem_zero_region_add(struct gsi_trans *trans, const struct ipa_mem *mem)
  * The AP informs the modem where its portions of memory are located
  * in a QMI exchange that occurs at modem startup.
  *
+ * There is no need for a matching ipa_mem_teardown() function.
+ *
  * Return:	0 if successful, or a negative error code
  */
 int ipa_mem_setup(struct ipa *ipa)
@@ -97,11 +99,6 @@ int ipa_mem_setup(struct ipa *ipa)
 	return 0;
 }
 
-void ipa_mem_teardown(struct ipa *ipa)
-{
-	/* Nothing to do */
-}
-
 #ifdef IPA_VALIDATE
 
 static bool ipa_mem_valid(struct ipa *ipa, enum ipa_mem_id mem_id)
diff --git a/drivers/net/ipa/ipa_mem.h b/drivers/net/ipa/ipa_mem.h
index df61ef48df365..9ca8a47bd4afd 100644
--- a/drivers/net/ipa/ipa_mem.h
+++ b/drivers/net/ipa/ipa_mem.h
@@ -1,7 +1,7 @@
 /* SPDX-License-Identifier: GPL-2.0 */
 
 /* Copyright (c) 2012-2018, The Linux Foundation. All rights reserved.
- * Copyright (C) 2019-2020 Linaro Ltd.
+ * Copyright (C) 2019-2021 Linaro Ltd.
  */
 #ifndef _IPA_MEM_H_
 #define _IPA_MEM_H_
@@ -88,8 +88,7 @@ struct ipa_mem {
 int ipa_mem_config(struct ipa *ipa);
 void ipa_mem_deconfig(struct ipa *ipa);
 
-int ipa_mem_setup(struct ipa *ipa);
-void ipa_mem_teardown(struct ipa *ipa);
+int ipa_mem_setup(struct ipa *ipa);	/* No ipa_mem_teardown() needed */
 
 int ipa_mem_zero_modem(struct ipa *ipa);
 
diff --git a/drivers/net/ipa/ipa_resource.c b/drivers/net/ipa/ipa_resource.c
index 85f922d6f222f..3b2dc216d3a68 100644
--- a/drivers/net/ipa/ipa_resource.c
+++ b/drivers/net/ipa/ipa_resource.c
@@ -158,7 +158,7 @@ static void ipa_resource_config_dst(struct ipa *ipa, u32 resource_type,
 	ipa_resource_config_common(ipa, offset, &resource->limits[6], ylimits);
 }
 
-/* Configure resources */
+/* Configure resources; there is no ipa_resource_deconfig() */
 int ipa_resource_config(struct ipa *ipa, const struct ipa_resource_data *data)
 {
 	u32 i;
@@ -174,9 +174,3 @@ int ipa_resource_config(struct ipa *ipa, const struct ipa_resource_data *data)
 
 	return 0;
 }
-
-/* Inverse of ipa_resource_config() */
-void ipa_resource_deconfig(struct ipa *ipa)
-{
-	/* Nothing to do */
-}
diff --git a/drivers/net/ipa/ipa_resource.h b/drivers/net/ipa/ipa_resource.h
index 9f74036fb95c5..ef5818bff180d 100644
--- a/drivers/net/ipa/ipa_resource.h
+++ b/drivers/net/ipa/ipa_resource.h
@@ -14,14 +14,10 @@ struct ipa_resource_data;
  * @ipa:	IPA pointer
  * @data:	IPA resource configuration data
  *
+ * There is no need for a matching ipa_resource_deconfig() function.
+ *
  * Return:	true if all regions are valid, false otherwise
  */
 int ipa_resource_config(struct ipa *ipa, const struct ipa_resource_data *data);
 
-/**
- * ipa_resource_deconfig() - Inverse of ipa_resource_config()
- * @ipa:	IPA pointer
- */
-void ipa_resource_deconfig(struct ipa *ipa);
-
 #endif /* _IPA_RESOURCE_H_ */
diff --git a/drivers/net/ipa/ipa_table.c b/drivers/net/ipa/ipa_table.c
index 401b568df6a34..3168d72f42450 100644
--- a/drivers/net/ipa/ipa_table.c
+++ b/drivers/net/ipa/ipa_table.c
@@ -497,11 +497,6 @@ int ipa_table_setup(struct ipa *ipa)
 	return 0;
 }
 
-void ipa_table_teardown(struct ipa *ipa)
-{
-	/* Nothing to do */	/* XXX Maybe reset the tables? */
-}
-
 /**
  * ipa_filter_tuple_zero() - Zero an endpoint's hashed filter tuple
  * @endpoint:	Endpoint whose filter hash tuple should be zeroed
@@ -525,6 +520,7 @@ static void ipa_filter_tuple_zero(struct ipa_endpoint *endpoint)
 	iowrite32(val, endpoint->ipa->reg_virt + offset);
 }
 
+/* Configure a hashed filter table; there is no ipa_filter_deconfig() */
 static void ipa_filter_config(struct ipa *ipa, bool modem)
 {
 	enum gsi_ee_id ee_id = modem ? GSI_EE_MODEM : GSI_EE_AP;
@@ -545,11 +541,6 @@ static void ipa_filter_config(struct ipa *ipa, bool modem)
 	}
 }
 
-static void ipa_filter_deconfig(struct ipa *ipa, bool modem)
-{
-	/* Nothing to do */
-}
-
 static bool ipa_route_id_modem(u32 route_id)
 {
 	return route_id >= IPA_ROUTE_MODEM_MIN &&
@@ -576,6 +567,7 @@ static void ipa_route_tuple_zero(struct ipa *ipa, u32 route_id)
 	iowrite32(val, ipa->reg_virt + offset);
 }
 
+/* Configure a hashed route table; there is no ipa_route_deconfig() */
 static void ipa_route_config(struct ipa *ipa, bool modem)
 {
 	u32 route_id;
@@ -588,11 +580,7 @@ static void ipa_route_config(struct ipa *ipa, bool modem)
 			ipa_route_tuple_zero(ipa, route_id);
 }
 
-static void ipa_route_deconfig(struct ipa *ipa, bool modem)
-{
-	/* Nothing to do */
-}
-
+/* Configure a filter and route tables; there is no ipa_table_deconfig() */
 void ipa_table_config(struct ipa *ipa)
 {
 	ipa_filter_config(ipa, false);
@@ -601,14 +589,6 @@ void ipa_table_config(struct ipa *ipa)
 	ipa_route_config(ipa, true);
 }
 
-void ipa_table_deconfig(struct ipa *ipa)
-{
-	ipa_route_deconfig(ipa, true);
-	ipa_route_deconfig(ipa, false);
-	ipa_filter_deconfig(ipa, true);
-	ipa_filter_deconfig(ipa, false);
-}
-
 /*
  * Initialize a coherent DMA allocation containing initialized filter and
  * route table data.  This is used when initializing or resetting the IPA
diff --git a/drivers/net/ipa/ipa_table.h b/drivers/net/ipa/ipa_table.h
index 018045b95aad8..1e2be9fce2f81 100644
--- a/drivers/net/ipa/ipa_table.h
+++ b/drivers/net/ipa/ipa_table.h
@@ -74,27 +74,19 @@ int ipa_table_hash_flush(struct ipa *ipa);
 /**
  * ipa_table_setup() - Set up filter and route tables
  * @ipa:	IPA pointer
+ *
+ * There is no need for a matching ipa_table_teardown() function.
  */
 int ipa_table_setup(struct ipa *ipa);
 
-/**
- * ipa_table_teardown() - Inverse of ipa_table_setup()
- * @ipa:	IPA pointer
- */
-void ipa_table_teardown(struct ipa *ipa);
-
 /**
  * ipa_table_config() - Configure filter and route tables
  * @ipa:	IPA pointer
+ *
+ * There is no need for a matching ipa_table_deconfig() function.
  */
 void ipa_table_config(struct ipa *ipa);
 
-/**
- * ipa_table_deconfig() - Inverse of ipa_table_config()
- * @ipa:	IPA pointer
- */
-void ipa_table_deconfig(struct ipa *ipa);
-
 /**
  * ipa_table_init() - Do early initialization of filter and route tables
  * @ipa:	IPA pointer
-- 
2.27.0

[PATCH net-next 6/7] net: ipa: get rid of empty GSI functions

[Alex Elder]

There are place holder functions in the GSI code that do nothing.
Remove these, knowing we can add something back in their place if
they're really needed someday.

Some of these are inverse functions (such as teardown to match setup).
Explicitly comment that there is no inverse in these cases.

Signed-off-by: Alex Elder <elder@linaro.org>
---
 drivers/net/ipa/gsi.c | 54 +++++--------------------------------------
 1 file changed, 6 insertions(+), 48 deletions(-)

diff --git a/drivers/net/ipa/gsi.c b/drivers/net/ipa/gsi.c
index 1c835b3e1a437..9f06663cef263 100644
--- a/drivers/net/ipa/gsi.c
+++ b/drivers/net/ipa/gsi.c
@@ -198,7 +198,7 @@ static void gsi_irq_type_disable(struct gsi *gsi, enum gsi_irq_type_id type_id)
 	gsi_irq_type_update(gsi, gsi->type_enabled_bitmap & ~BIT(type_id));
 }
 
-/* Turn off all GSI interrupts initially */
+/* Turn off all GSI interrupts initially; there is no gsi_irq_teardown() */
 static void gsi_irq_setup(struct gsi *gsi)
 {
 	/* Disable all interrupt types */
@@ -217,12 +217,6 @@ static void gsi_irq_setup(struct gsi *gsi)
 	iowrite32(0, gsi->virt + GSI_CNTXT_GSI_IRQ_EN_OFFSET);
 }
 
-/* Turn off all GSI interrupts when we're all done */
-static void gsi_irq_teardown(struct gsi *gsi)
-{
-	/* Nothing to do */
-}
-
 /* Event ring commands are performed one at a time.  Their completion
  * is signaled by the event ring control GSI interrupt type, which is
  * only enabled when we issue an event ring command.  Only the event
@@ -786,7 +780,7 @@ static void gsi_channel_trans_quiesce(struct gsi_channel *channel)
 	}
 }
 
-/* Program a channel for use */
+/* Program a channel for use; there is no gsi_channel_deprogram() */
 static void gsi_channel_program(struct gsi_channel *channel, bool doorbell)
 {
 	size_t size = channel->tre_ring.count * GSI_RING_ELEMENT_SIZE;
@@ -874,11 +868,6 @@ static void gsi_channel_program(struct gsi_channel *channel, bool doorbell)
 	/* All done! */
 }
 
-static void gsi_channel_deprogram(struct gsi_channel *channel)
-{
-	/* Nothing to do */
-}
-
 static int __gsi_channel_start(struct gsi_channel *channel, bool start)
 {
 	struct gsi *gsi = channel->gsi;
@@ -1623,18 +1612,6 @@ static u32 gsi_event_bitmap_init(u32 evt_ring_max)
 	return event_bitmap;
 }
 
-/* Setup function for event rings */
-static void gsi_evt_ring_setup(struct gsi *gsi)
-{
-	/* Nothing to do */
-}
-
-/* Inverse of gsi_evt_ring_setup() */
-static void gsi_evt_ring_teardown(struct gsi *gsi)
-{
-	/* Nothing to do */
-}
-
 /* Setup function for a single channel */
 static int gsi_channel_setup_one(struct gsi *gsi, u32 channel_id)
 {
@@ -1684,7 +1661,6 @@ static void gsi_channel_teardown_one(struct gsi *gsi, u32 channel_id)
 
 	netif_napi_del(&channel->napi);
 
-	gsi_channel_deprogram(channel);
 	gsi_channel_de_alloc_command(gsi, channel_id);
 	gsi_evt_ring_reset_command(gsi, evt_ring_id);
 	gsi_evt_ring_de_alloc_command(gsi, evt_ring_id);
@@ -1759,7 +1735,6 @@ static int gsi_channel_setup(struct gsi *gsi)
 	u32 mask;
 	int ret;
 
-	gsi_evt_ring_setup(gsi);
 	gsi_irq_enable(gsi);
 
 	mutex_lock(&gsi->mutex);
@@ -1819,7 +1794,6 @@ static int gsi_channel_setup(struct gsi *gsi)
 	mutex_unlock(&gsi->mutex);
 
 	gsi_irq_disable(gsi);
-	gsi_evt_ring_teardown(gsi);
 
 	return ret;
 }
@@ -1848,7 +1822,6 @@ static void gsi_channel_teardown(struct gsi *gsi)
 	mutex_unlock(&gsi->mutex);
 
 	gsi_irq_disable(gsi);
-	gsi_evt_ring_teardown(gsi);
 }
 
 /* Setup function for GSI.  GSI firmware must be loaded and initialized */
@@ -1856,7 +1829,6 @@ int gsi_setup(struct gsi *gsi)
 {
 	struct device *dev = gsi->dev;
 	u32 val;
-	int ret;
 
 	/* Here is where we first touch the GSI hardware */
 	val = ioread32(gsi->virt + GSI_GSI_STATUS_OFFSET);
@@ -1865,7 +1837,7 @@ int gsi_setup(struct gsi *gsi)
 		return -EIO;
 	}
 
-	gsi_irq_setup(gsi);
+	gsi_irq_setup(gsi);		/* No matching teardown required */
 
 	val = ioread32(gsi->virt + GSI_GSI_HW_PARAM_2_OFFSET);
 
@@ -1899,18 +1871,13 @@ int gsi_setup(struct gsi *gsi)
 	/* Writing 1 indicates IRQ interrupts; 0 would be MSI */
 	iowrite32(1, gsi->virt + GSI_CNTXT_INTSET_OFFSET);
 
-	ret = gsi_channel_setup(gsi);
-	if (ret)
-		gsi_irq_teardown(gsi);
-
-	return ret;
+	return gsi_channel_setup(gsi);
 }
 
 /* Inverse of gsi_setup() */
 void gsi_teardown(struct gsi *gsi)
 {
 	gsi_channel_teardown(gsi);
-	gsi_irq_teardown(gsi);
 }
 
 /* Initialize a channel's event ring */
@@ -1952,7 +1919,7 @@ static void gsi_channel_evt_ring_exit(struct gsi_channel *channel)
 	gsi_evt_ring_id_free(gsi, evt_ring_id);
 }
 
-/* Init function for event rings */
+/* Init function for event rings; there is no gsi_evt_ring_exit() */
 static void gsi_evt_ring_init(struct gsi *gsi)
 {
 	u32 evt_ring_id = 0;
@@ -1964,12 +1931,6 @@ static void gsi_evt_ring_init(struct gsi *gsi)
 	while (++evt_ring_id < GSI_EVT_RING_COUNT_MAX);
 }
 
-/* Inverse of gsi_evt_ring_init() */
-static void gsi_evt_ring_exit(struct gsi *gsi)
-{
-	/* Nothing to do */
-}
-
 static bool gsi_channel_data_valid(struct gsi *gsi,
 				   const struct ipa_gsi_endpoint_data *data)
 {
@@ -2114,7 +2075,7 @@ static int gsi_channel_init(struct gsi *gsi, u32 count,
 	/* IPA v4.2 requires the AP to allocate channels for the modem */
 	modem_alloc = gsi->version == IPA_VERSION_4_2;
 
-	gsi_evt_ring_init(gsi);
+	gsi_evt_ring_init(gsi);			/* No matching exit required */
 
 	/* The endpoint data array is indexed by endpoint name */
 	for (i = 0; i < count; i++) {
@@ -2148,7 +2109,6 @@ static int gsi_channel_init(struct gsi *gsi, u32 count,
 		}
 		gsi_channel_exit_one(&gsi->channel[data->channel_id]);
 	}
-	gsi_evt_ring_exit(gsi);
 
 	return ret;
 }
@@ -2162,8 +2122,6 @@ static void gsi_channel_exit(struct gsi *gsi)
 		gsi_channel_exit_one(&gsi->channel[channel_id]);
 	while (channel_id--);
 	gsi->modem_channel_bitmap = 0;
-
-	gsi_evt_ring_exit(gsi);
 }
 
 /* Init function for GSI.  GSI hardware does not need to be "ready" */
-- 
2.27.0

[PATCH net-next 7/7] net: ipa: three small fixes

[Alex Elder]

Some time ago changes were made to stop referring to clearing the
hardware pipeline as a "tag process."  Fix a comment to use the
newer terminology.

Get rid of a pointless double-negation of the Boolean toward_ipa
flag in ipa_endpoint_config().

make ipa_endpoint_exit_one() private; it's only referenced inside
"ipa_endpoint.c".

Signed-off-by: Alex Elder <elder@linaro.org>
---
 drivers/net/ipa/ipa_endpoint.c | 6 +++---
 drivers/net/ipa/ipa_endpoint.h | 2 --
 2 files changed, 3 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ipa/ipa_endpoint.c b/drivers/net/ipa/ipa_endpoint.c
index dd24179383c1c..72751843b2e48 100644
--- a/drivers/net/ipa/ipa_endpoint.c
+++ b/drivers/net/ipa/ipa_endpoint.c
@@ -397,7 +397,7 @@ int ipa_endpoint_modem_exception_reset_all(struct ipa *ipa)
 	/* We need one command per modem TX endpoint.  We can get an upper
 	 * bound on that by assuming all initialized endpoints are modem->IPA.
 	 * That won't happen, and we could be more precise, but this is fine
-	 * for now.  We need to end the transaction with a "tag process."
+	 * for now.  End the transaction with commands to clear the pipeline.
 	 */
 	count = hweight32(initialized) + ipa_cmd_pipeline_clear_count();
 	trans = ipa_cmd_trans_alloc(ipa, count);
@@ -1755,7 +1755,7 @@ int ipa_endpoint_config(struct ipa *ipa)
 
 		/* Make sure it's pointing in the right direction */
 		endpoint = &ipa->endpoint[endpoint_id];
-		if ((endpoint_id < rx_base) != !!endpoint->toward_ipa) {
+		if ((endpoint_id < rx_base) != endpoint->toward_ipa) {
 			dev_err(dev, "endpoint id %u wrong direction\n",
 				endpoint_id);
 			ret = -EINVAL;
@@ -1791,7 +1791,7 @@ static void ipa_endpoint_init_one(struct ipa *ipa, enum ipa_endpoint_name name,
 	ipa->initialized |= BIT(endpoint->endpoint_id);
 }
 
-void ipa_endpoint_exit_one(struct ipa_endpoint *endpoint)
+static void ipa_endpoint_exit_one(struct ipa_endpoint *endpoint)
 {
 	endpoint->ipa->initialized &= ~BIT(endpoint->endpoint_id);
 
diff --git a/drivers/net/ipa/ipa_endpoint.h b/drivers/net/ipa/ipa_endpoint.h
index f034a9e6ef215..0a859d10312dc 100644
--- a/drivers/net/ipa/ipa_endpoint.h
+++ b/drivers/net/ipa/ipa_endpoint.h
@@ -87,8 +87,6 @@ int ipa_endpoint_modem_exception_reset_all(struct ipa *ipa);
 
 int ipa_endpoint_skb_tx(struct ipa_endpoint *endpoint, struct sk_buff *skb);
 
-void ipa_endpoint_exit_one(struct ipa_endpoint *endpoint);
-
 int ipa_endpoint_enable_one(struct ipa_endpoint *endpoint);
 void ipa_endpoint_disable_one(struct ipa_endpoint *endpoint);
 
-- 
2.27.0

Re: [PATCH net-next 0/7] net: ipa: a few small fixes

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net-next.git (refs/heads/master):

On Fri,  9 Apr 2021 13:07:15 -0500 you wrote:
> This series implements some minor bug fixes or improvements.
> 
> The first patch removes an apparently unnecessary restriction, which
> results in an error on a 32-bit ARM build.
> 
> The second makes a definition used for SDM845 match what is used in
> the downstream code.
> 
> [...]

Here is the summary with links:
  - [net-next,1/7] net: ipa: relax pool entry size requirement
    https://git.kernel.org/netdev/net-next/c/7ad3bd52cbcb
  - [net-next,2/7] net: ipa: update sequence type for modem TX endpoint
    https://git.kernel.org/netdev/net-next/c/49e76a418981
  - [net-next,3/7] net: ipa: only set endpoint netdev pointer when in use
    https://git.kernel.org/netdev/net-next/c/57f63faf0562
  - [net-next,4/7] net: ipa: ipa_stop() does not return an error
    https://git.kernel.org/netdev/net-next/c/077e770f2601
  - [net-next,5/7] net: ipa: get rid of empty IPA functions
    https://git.kernel.org/netdev/net-next/c/74858b63c47c
  - [net-next,6/7] net: ipa: get rid of empty GSI functions
    https://git.kernel.org/netdev/net-next/c/57ab8ca42fa0
  - [net-next,7/7] net: ipa: three small fixes
    https://git.kernel.org/netdev/net-next/c/602a1c76f847

You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

Re: [PATCH net-next 4/7] net: ipa: ipa_stop() does not return an error

[Leon Romanovsky]

On Fri, Apr 09, 2021 at 01:07:19PM -0500, Alex Elder wrote:
> In ipa_modem_stop(), if the modem netdev pointer is non-null we call
> ipa_stop().  We check for an error and if one is returned we handle
> it.  But ipa_stop() never returns an error, so this extra handling
> is unnecessary.  Simplify the code in ipa_modem_stop() based on the
> knowledge no error handling is needed at this spot.
> 
> Signed-off-by: Alex Elder <elder@linaro.org>
> ---
>  drivers/net/ipa/ipa_modem.c | 18 ++++--------------
>  1 file changed, 4 insertions(+), 14 deletions(-)

<...>

> +	/* Stop the queue and disable the endpoints if it's open */
>  	if (netdev) {
> -		/* Stop the queue and disable the endpoints if it's open */
> -		ret = ipa_stop(netdev);
> -		if (ret)
> -			goto out_set_state;
> -
> +		(void)ipa_stop(netdev);

This void casting is not needed here and in more general case sometimes
even be seen as a mistake, for example if the returned attribute declared
as __must_check.

Thanks

Re: [PATCH net-next 4/7] net: ipa: ipa_stop() does not return an error

[Alex Elder]

On 4/11/21 1:34 AM, Leon Romanovsky wrote:
> On Fri, Apr 09, 2021 at 01:07:19PM -0500, Alex Elder wrote:
>> In ipa_modem_stop(), if the modem netdev pointer is non-null we call
>> ipa_stop().  We check for an error and if one is returned we handle
>> it.  But ipa_stop() never returns an error, so this extra handling
>> is unnecessary.  Simplify the code in ipa_modem_stop() based on the
>> knowledge no error handling is needed at this spot.
>>
>> Signed-off-by: Alex Elder <elder@linaro.org>
>> ---
>>  drivers/net/ipa/ipa_modem.c | 18 ++++--------------
>>  1 file changed, 4 insertions(+), 14 deletions(-)
> 
> <...>
> 
>> +	/* Stop the queue and disable the endpoints if it's open */
>>  	if (netdev) {
>> -		/* Stop the queue and disable the endpoints if it's open */
>> -		ret = ipa_stop(netdev);
>> -		if (ret)
>> -			goto out_set_state;
>> -
>> +		(void)ipa_stop(netdev);
> 
> This void casting is not needed here and in more general case sometimes
> even be seen as a mistake, for example if the returned attribute declared
> as __must_check.

I accept your point but I feel like it's sort of a 50/50 thing.

I think *not* checking an available return value is questionable
practice.  I'd really rather have a build option for a
"__need_not_check" tag and have "must_check" be the default.

The void cast here says "I know this returns a result, but I am
intentionally not checking it."  If it had been __must_check I
would certainly have checked it.  

That being said, I don't really care that much, so I'll plan
to post version 2, which will drop this cast (I'll probably
add a comment though).

Thanks.

					-Alex

> 
> Thanks
> 

Re: [PATCH net-next 4/7] net: ipa: ipa_stop() does not return an error

[Leon Romanovsky]

On Sun, Apr 11, 2021 at 08:09:55AM -0500, Alex Elder wrote:
> On 4/11/21 1:34 AM, Leon Romanovsky wrote:
> > On Fri, Apr 09, 2021 at 01:07:19PM -0500, Alex Elder wrote:
> >> In ipa_modem_stop(), if the modem netdev pointer is non-null we call
> >> ipa_stop().  We check for an error and if one is returned we handle
> >> it.  But ipa_stop() never returns an error, so this extra handling
> >> is unnecessary.  Simplify the code in ipa_modem_stop() based on the
> >> knowledge no error handling is needed at this spot.
> >>
> >> Signed-off-by: Alex Elder <elder@linaro.org>
> >> ---
> >>  drivers/net/ipa/ipa_modem.c | 18 ++++--------------
> >>  1 file changed, 4 insertions(+), 14 deletions(-)
> > 
> > <...>
> > 
> >> +	/* Stop the queue and disable the endpoints if it's open */
> >>  	if (netdev) {
> >> -		/* Stop the queue and disable the endpoints if it's open */
> >> -		ret = ipa_stop(netdev);
> >> -		if (ret)
> >> -			goto out_set_state;
> >> -
> >> +		(void)ipa_stop(netdev);
> > 
> > This void casting is not needed here and in more general case sometimes
> > even be seen as a mistake, for example if the returned attribute declared
> > as __must_check.
> 
> I accept your point but I feel like it's sort of a 50/50 thing.
> 
> I think *not* checking an available return value is questionable
> practice.  I'd really rather have a build option for a
> "__need_not_check" tag and have "must_check" be the default.

__need_not_check == void ???

> 
> The void cast here says "I know this returns a result, but I am
> intentionally not checking it."  If it had been __must_check I
> would certainly have checked it.  
> 
> That being said, I don't really care that much, so I'll plan
> to post version 2, which will drop this cast (I'll probably
> add a comment though).

Thanks

> 
> Thanks.
> 
> 					-Alex
> 
> > 
> > Thanks
> > 
> 

Re: [PATCH net-next 4/7] net: ipa: ipa_stop() does not return an error

[Alex Elder]

On 4/11/21 8:28 AM, Leon Romanovsky wrote:
>> I think *not* checking an available return value is questionable
>> practice.  I'd really rather have a build option for a
>> "__need_not_check" tag and have "must_check" be the default.
> __need_not_check == void ???

I'm not sure I understand your statement here, but...

My point is, I'd rather have things like printk() and
strscpy() be marked with (an imaginary) __need_not_check,
than the way things are, with only certain functions being
marked __must_check.

In my view, if a function returns a value, all callers
of that function ought to be checking it.  If the return
value is not necessary it should be a void function if
possible.

I don't expect the world to change, but I just think the
default should be "must check" rather than "check optional".

					-Alex

Re: [PATCH net-next 4/7] net: ipa: ipa_stop() does not return an error

[Leon Romanovsky]

On Sun, Apr 11, 2021 at 08:42:15AM -0500, Alex Elder wrote:
> On 4/11/21 8:28 AM, Leon Romanovsky wrote:
> >> I think *not* checking an available return value is questionable
> >> practice.  I'd really rather have a build option for a
> >> "__need_not_check" tag and have "must_check" be the default.
> > __need_not_check == void ???
> 
> I'm not sure I understand your statement here, but...

We are talking about the same thing. My point was that __need_not_check
is actually void. The API author was supposed to declare that by
declaring that function doesn't return anything.

Thanks

Re: [PATCH net-next 4/7] net: ipa: ipa_stop() does not return an error

[Alex Elder]

On 4/12/21 2:26 AM, Leon Romanovsky wrote:
> On Sun, Apr 11, 2021 at 08:42:15AM -0500, Alex Elder wrote:
>> On 4/11/21 8:28 AM, Leon Romanovsky wrote:
>>>> I think *not* checking an available return value is questionable
>>>> practice.  I'd really rather have a build option for a
>>>> "__need_not_check" tag and have "must_check" be the default.
>>> __need_not_check == void ???
>>
>> I'm not sure I understand your statement here, but...
> 
> We are talking about the same thing. My point was that __need_not_check
> is actually void. The API author was supposed to declare that by
> declaring that function doesn't return anything.

No, we are not.

Functions like strcpy() return a value, but that value is almost
never checked.  The returned value isn't an error, so there is
no real need to check that return value.  This is the kind of
thing I'm talking about that might be tagged __need_not_check.

A function that returns a value for no reason should be void,
I agree with that.

In the ipa_stop() case, the value *must* be returned because
it serves as an ->ndo_stop() function and has to adhere to
that function prototype.  The point of the current patch
was to simplify the code (defined privately in the current
source file), given knowledge that it never returns an error.

The compiler could ensure all calls to functions that return
a value actually check the return value.  And because I think
that's the best practice, I'd like to be able to run such a
check in my code.  But there are always exceptions, and that
would be the purpose of a __need_not_check tag.

I don't think this is worthy of any more discussion.

					-Alex