[PATCH net-next 0/4] net: ipa: little fixes

[PATCH net-next 0/4] net: ipa: little fixes

[Alex Elder]

This series adds a few small fixes to the IPA code.

The first patch appeared in a different form in June, and got some
pushback from David because he felt a problem that can be caught at
build time *should* be caught at build time.
  https://lore.kernel.org/netdev/20200610195332.2612233-1-elder@linaro.org/
I agree with that, but in this case the "problem" was never actually
a problem.  There's a little more explanation on the patch, but
basically now we remove the BUILD_BUG_ON() call entirely.

The second deletes a line of code that isn't needed.

The third converts a warning message to be a debug, as requested by
Stephen Boyd.

And the last one just gets rid of an error message that would be
output after a different message had already reported a problem.

					-Alex

Alex Elder (4):
  net: ipa: don't break build on large transaction size
  net: ipa: get rid of a useless line of code
  net: ipa: change a warning to debug
  net: ipa: drop an error message

 drivers/net/ipa/gsi.c      | 7 +------
 drivers/net/ipa/ipa_main.c | 6 +-----
 drivers/net/ipa/ipa_mem.c  | 8 ++++----
 3 files changed, 6 insertions(+), 15 deletions(-)

-- 
2.20.1

[PATCH net-next 1/4] net: ipa: don't break build on large transaction size

[Alex Elder]

The following call in ipa_validate_build() is erroneous:

    BUILD_BUG_ON(sizeof(struct gsi_trans) > 128);

The fact is, it is not a bug for the size of a GSI transaction to be
bigger than 128 bytes.  The correct operation of the driver is not
dependent on the size of this structure.  The only consequence of
the transaction being large is that the amount of memory required
is larger.

The problem this was trying to flag is that a *slight* increase in
the size of this structure will have a disproportionate effect on
the amount of memory used.  E.g. if the structure grew to 132 bytes
the memory requirement for the transaction arrays would be about
double.

With various debugging build flags enabled, the size grows to 160
bytes.  But there's no reason to treat that as a build-time bug.

Signed-off-by: Alex Elder <elder@linaro.org>
---
 drivers/net/ipa/ipa_main.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/net/ipa/ipa_main.c b/drivers/net/ipa/ipa_main.c
index a580cab794b1c..d1e582707800a 100644
--- a/drivers/net/ipa/ipa_main.c
+++ b/drivers/net/ipa/ipa_main.c
@@ -680,9 +680,6 @@ static void ipa_validate_build(void)
 	 */
 	BUILD_BUG_ON(GSI_TLV_MAX > U8_MAX);
 
-	/* Exceeding 128 bytes makes the transaction pool *much* larger */
-	BUILD_BUG_ON(sizeof(struct gsi_trans) > 128);
-
 	/* This is used as a divisor */
 	BUILD_BUG_ON(!IPA_AGGR_GRANULARITY);
 
-- 
2.20.1

[PATCH net-next 2/4] net: ipa: get rid of a useless line of code

[Alex Elder]

Delete a spurious line of code in ipa_hardware_config().  It reads a
register value then ignores the value, so is completely unnecessary.

Add a missing word in a comment.

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

diff --git a/drivers/net/ipa/ipa_main.c b/drivers/net/ipa/ipa_main.c
index d1e582707800a..bfe95a46acaf1 100644
--- a/drivers/net/ipa/ipa_main.c
+++ b/drivers/net/ipa/ipa_main.c
@@ -335,7 +335,6 @@ static void ipa_hardware_config(struct ipa *ipa)
 	ipa_hardware_config_qsb(ipa);
 
 	/* Configure aggregation granularity */
-	val = ioread32(ipa->reg_virt + IPA_REG_COUNTER_CFG_OFFSET);
 	granularity = ipa_aggr_granularity_val(IPA_AGGR_GRANULARITY);
 	val = u32_encode_bits(granularity, AGGR_GRANULARITY);
 	iowrite32(val, ipa->reg_virt + IPA_REG_COUNTER_CFG_OFFSET);
@@ -787,7 +786,7 @@ static int ipa_probe(struct platform_device *pdev)
 	if (ret)
 		goto err_mem_exit;
 
-	/* Result is a non-zero mask endpoints that support filtering */
+	/* Result is a non-zero mask of endpoints that support filtering */
 	ipa->filter_map = ipa_endpoint_init(ipa, data->endpoint_count,
 					    data->endpoint_data);
 	if (!ipa->filter_map) {
-- 
2.20.1

[PATCH net-next 3/4] net: ipa: change a warning to debug

[Alex Elder]

When we determine from hardware what the size of IPA memory is
we compare it against what we learned about it from DT.

If DT defines a region that's larger than actual memory, we use the
smaller actual size and issue a warning.

If DT defines a smaller region than actual memory we issue a warning
too.  But in this case the difference is harmless; so rather than
issuing a warning, just provide a debug message instead.

Reorder these checks so the one that matters more is done first.

Reported-by: Stephen Boyd <swboyd@chromium.org>
Signed-off-by: Alex Elder <elder@linaro.org>
---
 drivers/net/ipa/ipa_mem.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ipa/ipa_mem.c b/drivers/net/ipa/ipa_mem.c
index ecfd1f91fce3b..0cc3a3374caa2 100644
--- a/drivers/net/ipa/ipa_mem.c
+++ b/drivers/net/ipa/ipa_mem.c
@@ -160,13 +160,13 @@ int ipa_mem_config(struct ipa *ipa)
 	mem_size = 8 * u32_get_bits(val, SHARED_MEM_SIZE_FMASK);
 
 	/* If the sizes don't match, issue a warning */
-	if (ipa->mem_offset + mem_size > ipa->mem_size) {
-		dev_warn(dev, "ignoring larger reported memory size: 0x%08x\n",
-			mem_size);
-	} else if (ipa->mem_offset + mem_size < ipa->mem_size) {
+	if (ipa->mem_offset + mem_size < ipa->mem_size) {
 		dev_warn(dev, "limiting IPA memory size to 0x%08x\n",
 			 mem_size);
 		ipa->mem_size = mem_size;
+	} else if (ipa->mem_offset + mem_size > ipa->mem_size) {
+		dev_dbg(dev, "ignoring larger reported memory size: 0x%08x\n",
+			mem_size);
 	}
 
 	/* Prealloc DMA memory for zeroing regions */
-- 
2.20.1

[PATCH net-next 4/4] net: ipa: drop an error message

[Alex Elder]

There is no need for gsi_modem_channel_halt() to report an error,
because gsi_generic_command() will already have done that if the
command times out.  So get rid of the extra message.

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

diff --git a/drivers/net/ipa/gsi.c b/drivers/net/ipa/gsi.c
index 961a11d4fb270..3a5998a037dab 100644
--- a/drivers/net/ipa/gsi.c
+++ b/drivers/net/ipa/gsi.c
@@ -1661,12 +1661,7 @@ static int gsi_modem_channel_alloc(struct gsi *gsi, u32 channel_id)
 
 static void gsi_modem_channel_halt(struct gsi *gsi, u32 channel_id)
 {
-	int ret;
-
-	ret = gsi_generic_command(gsi, channel_id, GSI_GENERIC_HALT_CHANNEL);
-	if (ret)
-		dev_err(gsi->dev, "error %d halting modem channel %u\n",
-			ret, channel_id);
+	(void)gsi_generic_command(gsi, channel_id, GSI_GENERIC_HALT_CHANNEL);
 }
 
 /* Setup function for channels */
-- 
2.20.1

Re: [PATCH net-next 0/4] net: ipa: little fixes

[Jakub Kicinski]

On Mon,  9 Nov 2020 10:56:31 -0600 Alex Elder wrote:
> This series adds a few small fixes to the IPA code.
> 
> The first patch appeared in a different form in June, and got some
> pushback from David because he felt a problem that can be caught at
> build time *should* be caught at build time.
>   https://lore.kernel.org/netdev/20200610195332.2612233-1-elder@linaro.org/
> I agree with that, but in this case the "problem" was never actually
> a problem.  There's a little more explanation on the patch, but
> basically now we remove the BUILD_BUG_ON() call entirely.
> 
> The second deletes a line of code that isn't needed.
> 
> The third converts a warning message to be a debug, as requested by
> Stephen Boyd.
> 
> And the last one just gets rid of an error message that would be
> output after a different message had already reported a problem.

Applied, thanks!