[PATCH net-next 0/5] net: ipa: endpoint configuration updates

[PATCH net-next 0/5] net: ipa: endpoint configuration updates

[Alex Elder]

This series updates code that configures IPA endpoints.  The changes
made mainly affect access to registers that are valid only for RX, or
only for TX endpoints.

The first three patches avoid writing endpoint registers if they are
not defined to be valid.  The fourth patch slightly modifies the
parameters for the offset macros used for these endpoint registers,
to make it explicit when only some endpoints are valid.

The last patch just tweaks one line of code so it uses a convention
used everywhere else in the driver.

					-Alex

Alex Elder (5):
  net: ipa: head-of-line block registers are RX only
  net: ipa: metadata_mask register is RX only
  net: ipa: mode register is TX only
  net: ipa: clarify endpoint register macro constraints
  net: ipa: HOL_BLOCK_EN_FMASK is a 1-bit mask

 drivers/net/ipa/ipa_endpoint.c | 26 ++++++++++++++------
 drivers/net/ipa/ipa_reg.h      | 43 ++++++++++++++++++++--------------
 2 files changed, 44 insertions(+), 25 deletions(-)

-- 
2.25.1

[PATCH net-next 1/5] net: ipa: head-of-line block registers are RX only

[Alex Elder]

The INIT_HOL_BLOCK_EN and INIT_HOL_BLOCK_TIMER endpoint registers
are only valid for RX endpoints.

Have ipa_endpoint_modem_hol_block_clear_all() skip writing these
registers for TX endpoints.

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

diff --git a/drivers/net/ipa/ipa_endpoint.c b/drivers/net/ipa/ipa_endpoint.c
index 9f50d0d11704..3f5a41fc1997 100644
--- a/drivers/net/ipa/ipa_endpoint.c
+++ b/drivers/net/ipa/ipa_endpoint.c
@@ -642,6 +642,8 @@ static int ipa_endpoint_init_hol_block_timer(struct ipa_endpoint *endpoint,
 	u32 offset;
 	u32 val;
 
+	/* assert(!endpoint->toward_ipa); */
+
 	/* XXX We'll fix this when the register definition is clear */
 	if (microseconds) {
 		struct device *dev = &ipa->pdev->dev;
@@ -671,6 +673,8 @@ ipa_endpoint_init_hol_block_enable(struct ipa_endpoint *endpoint, bool enable)
 	u32 offset;
 	u32 val;
 
+	/* assert(!endpoint->toward_ipa); */
+
 	val = u32_encode_bits(enable ? 1 : 0, HOL_BLOCK_EN_FMASK);
 	offset = IPA_REG_ENDP_INIT_HOL_BLOCK_EN_N_OFFSET(endpoint_id);
 	iowrite32(val, endpoint->ipa->reg_virt + offset);
@@ -683,7 +687,7 @@ void ipa_endpoint_modem_hol_block_clear_all(struct ipa *ipa)
 	for (i = 0; i < IPA_ENDPOINT_MAX; i++) {
 		struct ipa_endpoint *endpoint = &ipa->endpoint[i];
 
-		if (endpoint->ee_id != GSI_EE_MODEM)
+		if (endpoint->toward_ipa || endpoint->ee_id != GSI_EE_MODEM)
 			continue;
 
 		(void)ipa_endpoint_init_hol_block_timer(endpoint, 0);
-- 
2.25.1

[PATCH net-next 2/5] net: ipa: metadata_mask register is RX only

[Alex Elder]

The INIT_HDR_METADATA_MASK endpoint configuration register is only
valid for RX endpoints.  Rather than writing a zero to that register
for TX endpoints, avoid writing the register at all.

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

diff --git a/drivers/net/ipa/ipa_endpoint.c b/drivers/net/ipa/ipa_endpoint.c
index 3f5a41fc1997..0c2bec166066 100644
--- a/drivers/net/ipa/ipa_endpoint.c
+++ b/drivers/net/ipa/ipa_endpoint.c
@@ -527,10 +527,12 @@ static void ipa_endpoint_init_hdr_metadata_mask(struct ipa_endpoint *endpoint)
 	u32 val = 0;
 	u32 offset;
 
+	/* assert(!endpoint->toward_ipa); */
+
 	offset = IPA_REG_ENDP_INIT_HDR_METADATA_MASK_N_OFFSET(endpoint_id);
 
 	/* Note that HDR_ENDIANNESS indicates big endian header fields */
-	if (!endpoint->toward_ipa && endpoint->data->qmap)
+	if (endpoint->data->qmap)
 		val = cpu_to_be32(IPA_ENDPOINT_QMAP_METADATA_MASK);
 
 	iowrite32(val, endpoint->ipa->reg_virt + offset);
@@ -1306,10 +1308,10 @@ static void ipa_endpoint_program(struct ipa_endpoint *endpoint)
 			(void)ipa_endpoint_program_suspend(endpoint, false);
 		ipa_endpoint_init_hdr_ext(endpoint);
 		ipa_endpoint_init_aggr(endpoint);
+		ipa_endpoint_init_hdr_metadata_mask(endpoint);
 	}
 	ipa_endpoint_init_cfg(endpoint);
 	ipa_endpoint_init_hdr(endpoint);
-	ipa_endpoint_init_hdr_metadata_mask(endpoint);
 	ipa_endpoint_init_mode(endpoint);
 	ipa_endpoint_status(endpoint);
 }
-- 
2.25.1

[PATCH net-next 3/5] net: ipa: mode register is TX only

[Alex Elder]

The INIT_MODE endpoint configuration register is only valid for TX
endpoints.  Rather than writing a zero to that register for RX
endpoints, avoid writing the register at all.

Add assertion comments communicating that TX endpoints are assumed
for the DEAGGR and SEQ endpoint configuration registers to be
consistent.

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

diff --git a/drivers/net/ipa/ipa_endpoint.c b/drivers/net/ipa/ipa_endpoint.c
index 0c2bec166066..ee8fc22c3abc 100644
--- a/drivers/net/ipa/ipa_endpoint.c
+++ b/drivers/net/ipa/ipa_endpoint.c
@@ -543,7 +543,9 @@ static void ipa_endpoint_init_mode(struct ipa_endpoint *endpoint)
 	u32 offset = IPA_REG_ENDP_INIT_MODE_N_OFFSET(endpoint->endpoint_id);
 	u32 val;
 
-	if (endpoint->toward_ipa && endpoint->data->dma_mode) {
+	/* assert(endpoint->toward_ipa); */
+
+	if (endpoint->data->dma_mode) {
 		enum ipa_endpoint_name name = endpoint->data->dma_endpoint;
 		u32 dma_endpoint_id;
 
@@ -554,7 +556,7 @@ static void ipa_endpoint_init_mode(struct ipa_endpoint *endpoint)
 	} else {
 		val = u32_encode_bits(IPA_BASIC, MODE_FMASK);
 	}
-	/* Other bitfields unspecified (and 0) */
+	/* All other bits unspecified (and 0) */
 
 	iowrite32(val, endpoint->ipa->reg_virt + offset);
 }
@@ -702,6 +704,8 @@ static void ipa_endpoint_init_deaggr(struct ipa_endpoint *endpoint)
 	u32 offset = IPA_REG_ENDP_INIT_DEAGGR_N_OFFSET(endpoint->endpoint_id);
 	u32 val = 0;
 
+	/* assert(endpoint->toward_ipa); */
+
 	/* DEAGGR_HDR_LEN is 0 */
 	/* PACKET_OFFSET_VALID is 0 */
 	/* PACKET_OFFSET_LOCATION is ignored (not valid) */
@@ -716,6 +720,8 @@ static void ipa_endpoint_init_seq(struct ipa_endpoint *endpoint)
 	u32 seq_type = endpoint->seq_type;
 	u32 val = 0;
 
+	/* assert(endpoint->toward_ipa); */
+
 	/* Sequencer type is made up of four nibbles */
 	val |= u32_encode_bits(seq_type & 0xf, HPS_SEQ_TYPE_FMASK);
 	val |= u32_encode_bits((seq_type >> 4) & 0xf, DPS_SEQ_TYPE_FMASK);
@@ -1303,6 +1309,7 @@ static void ipa_endpoint_program(struct ipa_endpoint *endpoint)
 		ipa_endpoint_init_aggr(endpoint);
 		ipa_endpoint_init_deaggr(endpoint);
 		ipa_endpoint_init_seq(endpoint);
+		ipa_endpoint_init_mode(endpoint);
 	} else {
 		if (endpoint->ipa->version == IPA_VERSION_3_5_1)
 			(void)ipa_endpoint_program_suspend(endpoint, false);
@@ -1312,7 +1319,6 @@ static void ipa_endpoint_program(struct ipa_endpoint *endpoint)
 	}
 	ipa_endpoint_init_cfg(endpoint);
 	ipa_endpoint_init_hdr(endpoint);
-	ipa_endpoint_init_mode(endpoint);
 	ipa_endpoint_status(endpoint);
 }
 
-- 
2.25.1

[PATCH net-next 4/5] net: ipa: clarify endpoint register macro constraints

[Alex Elder]

A handful of registers are valid only for RX endpoints, and some
others are valid only for TX endpoints.  For these endpoints, add
a comment above their defined offset macro that indicates the
endpoints to which they apply.

Extend the endpoint parameter naming convention as well, to make
these constraints more explicit.

Signed-off-by: Alex Elder <elder@linaro.org>
---
 drivers/net/ipa/ipa_reg.h | 43 +++++++++++++++++++++++----------------
 1 file changed, 25 insertions(+), 18 deletions(-)

diff --git a/drivers/net/ipa/ipa_reg.h b/drivers/net/ipa/ipa_reg.h
index 0a688d8c1d7c..10e4ac9ead68 100644
--- a/drivers/net/ipa/ipa_reg.h
+++ b/drivers/net/ipa/ipa_reg.h
@@ -32,10 +32,12 @@ struct ipa;
  * parameter is supplied to the offset macro.  The "ee" value is a member of
  * the gsi_ee enumerated type.
  *
- * The offset of a register dependent on endpoint id is computed by a macro
- * that is supplied a parameter "ep".  The "ep" value is assumed to be less
- * than the maximum endpoint value for the current hardware, and that will
- * not exceed IPA_ENDPOINT_MAX.
+ * The offset of a register dependent on endpoint ID is computed by a macro
+ * that is supplied a parameter "ep", "txep", or "rxep".  A register with an
+ * "ep" parameter is valid for any endpoint; a register with a "txep" or
+ * "rxep" parameter is valid only for TX or RX endpoints, respectively.  The
+ * "*ep" value is assumed to be less than the maximum valid endpoint ID
+ * for the current hardware, and that will not exceed IPA_ENDPOINT_MAX.
  *
  * The offset of registers related to filter and route tables is computed
  * by a macro that is supplied a parameter "er".  The "er" represents an
@@ -293,11 +295,13 @@ static inline u32 ipa_reg_idle_indication_cfg_offset(enum ipa_version version)
 #define HDR_TOTAL_LEN_OR_PAD_OFFSET_FMASK	GENMASK(9, 4)
 #define HDR_PAD_TO_ALIGNMENT_FMASK		GENMASK(13, 10)
 
-#define IPA_REG_ENDP_INIT_HDR_METADATA_MASK_N_OFFSET(ep) \
-					(0x00000818 + 0x0070 * (ep))
+/* Valid only for RX (IPA producer) endpoints */
+#define IPA_REG_ENDP_INIT_HDR_METADATA_MASK_N_OFFSET(rxep) \
+					(0x00000818 + 0x0070 * (rxep))
 
-#define IPA_REG_ENDP_INIT_MODE_N_OFFSET(ep) \
-					(0x00000820 + 0x0070 * (ep))
+/* Valid only for TX (IPA consumer) endpoints */
+#define IPA_REG_ENDP_INIT_MODE_N_OFFSET(txep) \
+					(0x00000820 + 0x0070 * (txep))
 #define MODE_FMASK				GENMASK(2, 0)
 #define DEST_PIPE_INDEX_FMASK			GENMASK(8, 4)
 #define BYTE_THRESHOLD_FMASK			GENMASK(27, 12)
@@ -316,19 +320,21 @@ static inline u32 ipa_reg_idle_indication_cfg_offset(enum ipa_version version)
 #define AGGR_FORCE_CLOSE_FMASK			GENMASK(22, 22)
 #define AGGR_HARD_BYTE_LIMIT_ENABLE_FMASK	GENMASK(24, 24)
 
-#define IPA_REG_ENDP_INIT_HOL_BLOCK_EN_N_OFFSET(ep) \
-					(0x0000082c +  0x0070 * (ep))
+/* Valid only for RX (IPA producer) endpoints */
+#define IPA_REG_ENDP_INIT_HOL_BLOCK_EN_N_OFFSET(rxep) \
+					(0x0000082c +  0x0070 * (rxep))
 #define HOL_BLOCK_EN_FMASK			GENMASK(0, 0)
 
-/* The next register is valid only for RX (IPA producer) endpoints */
-#define IPA_REG_ENDP_INIT_HOL_BLOCK_TIMER_N_OFFSET(ep) \
-					(0x00000830 +  0x0070 * (ep))
+/* Valid only for RX (IPA producer) endpoints */
+#define IPA_REG_ENDP_INIT_HOL_BLOCK_TIMER_N_OFFSET(rxep) \
+					(0x00000830 +  0x0070 * (rxep))
 /* The next fields are present for IPA v4.2 only */
 #define BASE_VALUE_FMASK			GENMASK(4, 0)
 #define SCALE_FMASK				GENMASK(12, 8)
 
-#define IPA_REG_ENDP_INIT_DEAGGR_N_OFFSET(ep) \
-					(0x00000834 + 0x0070 * (ep))
+/* Valid only for TX (IPA consumer) endpoints */
+#define IPA_REG_ENDP_INIT_DEAGGR_N_OFFSET(txep) \
+					(0x00000834 + 0x0070 * (txep))
 #define DEAGGR_HDR_LEN_FMASK			GENMASK(5, 0)
 #define PACKET_OFFSET_VALID_FMASK		GENMASK(7, 7)
 #define PACKET_OFFSET_LOCATION_FMASK		GENMASK(13, 8)
@@ -338,8 +344,9 @@ static inline u32 ipa_reg_idle_indication_cfg_offset(enum ipa_version version)
 					(0x00000838 + 0x0070 * (ep))
 #define RSRC_GRP_FMASK				GENMASK(1, 0)
 
-#define IPA_REG_ENDP_INIT_SEQ_N_OFFSET(ep) \
-					(0x0000083c + 0x0070 * (ep))
+/* Valid only for TX (IPA consumer) endpoints */
+#define IPA_REG_ENDP_INIT_SEQ_N_OFFSET(txep) \
+					(0x0000083c + 0x0070 * (txep))
 #define HPS_SEQ_TYPE_FMASK			GENMASK(3, 0)
 #define DPS_SEQ_TYPE_FMASK			GENMASK(7, 4)
 #define HPS_REP_SEQ_TYPE_FMASK			GENMASK(11, 8)
@@ -353,7 +360,7 @@ static inline u32 ipa_reg_idle_indication_cfg_offset(enum ipa_version version)
 /* The next field is present for IPA v4.0 and above */
 #define STATUS_PKT_SUPPRESS_FMASK		GENMASK(9, 9)
 
-/* "er" is either an endpoint id (for filters) or a route id (for routes) */
+/* "er" is either an endpoint ID (for filters) or a route ID (for routes) */
 #define IPA_REG_ENDP_FILTER_ROUTER_HSH_CFG_N_OFFSET(er) \
 					(0x0000085c + 0x0070 * (er))
 #define FILTER_HASH_MSK_SRC_ID_FMASK		GENMASK(0, 0)
-- 
2.25.1

[PATCH net-next 5/5] net: ipa: HOL_BLOCK_EN_FMASK is a 1-bit mask

[Alex Elder]

The convention throughout the IPA driver is to directly use
single-bit field mask values, rather than using (for example)
u32_encode_bits() to set or clear them.

Fix the one place that doesn't follow that convention, which sets
HOL_BLOCK_EN_FMASK in ipa_endpoint_init_hol_block_enable().

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

diff --git a/drivers/net/ipa/ipa_endpoint.c b/drivers/net/ipa/ipa_endpoint.c
index ee8fc22c3abc..447dafab8f18 100644
--- a/drivers/net/ipa/ipa_endpoint.c
+++ b/drivers/net/ipa/ipa_endpoint.c
@@ -679,7 +679,7 @@ ipa_endpoint_init_hol_block_enable(struct ipa_endpoint *endpoint, bool enable)
 
 	/* assert(!endpoint->toward_ipa); */
 
-	val = u32_encode_bits(enable ? 1 : 0, HOL_BLOCK_EN_FMASK);
+	val = enable ? HOL_BLOCK_EN_FMASK : 0;
 	offset = IPA_REG_ENDP_INIT_HOL_BLOCK_EN_N_OFFSET(endpoint_id);
 	iowrite32(val, endpoint->ipa->reg_virt + offset);
 }
-- 
2.25.1

Re: [PATCH net-next 1/5] net: ipa: head-of-line block registers are RX only

[Jakub Kicinski]

On Mon, 29 Jun 2020 16:49:15 -0500 Alex Elder wrote:
> The INIT_HOL_BLOCK_EN and INIT_HOL_BLOCK_TIMER endpoint registers
> are only valid for RX endpoints.
> 
> Have ipa_endpoint_modem_hol_block_clear_all() skip writing these
> registers for TX endpoints.
> 
> Signed-off-by: Alex Elder <elder@linaro.org>
> ---
>  drivers/net/ipa/ipa_endpoint.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ipa/ipa_endpoint.c b/drivers/net/ipa/ipa_endpoint.c
> index 9f50d0d11704..3f5a41fc1997 100644
> --- a/drivers/net/ipa/ipa_endpoint.c
> +++ b/drivers/net/ipa/ipa_endpoint.c
> @@ -642,6 +642,8 @@ static int ipa_endpoint_init_hol_block_timer(struct ipa_endpoint *endpoint,
>  	u32 offset;
>  	u32 val;
>  
> +	/* assert(!endpoint->toward_ipa); */
> +
>  	/* XXX We'll fix this when the register definition is clear */
>  	if (microseconds) {
>  		struct device *dev = &ipa->pdev->dev;
> @@ -671,6 +673,8 @@ ipa_endpoint_init_hol_block_enable(struct ipa_endpoint *endpoint, bool enable)
>  	u32 offset;
>  	u32 val;
>  
> +	/* assert(!endpoint->toward_ipa); */

What are these assert comments for? :S

Re: [PATCH net-next 1/5] net: ipa: head-of-line block registers are RX only

[Alex Elder]

On 6/29/20 7:35 PM, Jakub Kicinski wrote:
> On Mon, 29 Jun 2020 16:49:15 -0500 Alex Elder wrote:
>> The INIT_HOL_BLOCK_EN and INIT_HOL_BLOCK_TIMER endpoint registers
>> are only valid for RX endpoints.
>>
>> Have ipa_endpoint_modem_hol_block_clear_all() skip writing these
>> registers for TX endpoints.
>>
>> Signed-off-by: Alex Elder <elder@linaro.org>
>> ---
>>  drivers/net/ipa/ipa_endpoint.c | 6 +++++-
>>  1 file changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/ipa/ipa_endpoint.c b/drivers/net/ipa/ipa_endpoint.c
>> index 9f50d0d11704..3f5a41fc1997 100644
>> --- a/drivers/net/ipa/ipa_endpoint.c
>> +++ b/drivers/net/ipa/ipa_endpoint.c
>> @@ -642,6 +642,8 @@ static int ipa_endpoint_init_hol_block_timer(struct ipa_endpoint *endpoint,
>>  	u32 offset;
>>  	u32 val;
>>  
>> +	/* assert(!endpoint->toward_ipa); */
>> +
>>  	/* XXX We'll fix this when the register definition is clear */
>>  	if (microseconds) {
>>  		struct device *dev = &ipa->pdev->dev;
>> @@ -671,6 +673,8 @@ ipa_endpoint_init_hol_block_enable(struct ipa_endpoint *endpoint, bool enable)
>>  	u32 offset;
>>  	u32 val;
>>  
>> +	/* assert(!endpoint->toward_ipa); */
> 
> What are these assert comments for? :S

They are place holders for when I can put in a proper assert
function.  For now I'm trying to avoid BUG_ON() calls, but
at some point soon I'll replace these comments with calls
to a macro that does BUG_ON() conditioned on a config option
(or something else if there's a better suggestion).

Even though it's commented, the assert() call does what
I want, which is to communicate to the reader a condition
assumed by the code, succinctly.

					-Alex

Re: [PATCH net-next 1/5] net: ipa: head-of-line block registers are RX only

[David Miller]

From: Alex Elder <elder@linaro.org>
Date: Mon, 29 Jun 2020 20:01:20 -0500

> On 6/29/20 7:35 PM, Jakub Kicinski wrote:
>> On Mon, 29 Jun 2020 16:49:15 -0500 Alex Elder wrote:
>>> The INIT_HOL_BLOCK_EN and INIT_HOL_BLOCK_TIMER endpoint registers
>>> are only valid for RX endpoints.
>>>
>>> Have ipa_endpoint_modem_hol_block_clear_all() skip writing these
>>> registers for TX endpoints.
>>>
>>> Signed-off-by: Alex Elder <elder@linaro.org>
>>> ---
>>>  drivers/net/ipa/ipa_endpoint.c | 6 +++++-
>>>  1 file changed, 5 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/net/ipa/ipa_endpoint.c b/drivers/net/ipa/ipa_endpoint.c
>>> index 9f50d0d11704..3f5a41fc1997 100644
>>> --- a/drivers/net/ipa/ipa_endpoint.c
>>> +++ b/drivers/net/ipa/ipa_endpoint.c
>>> @@ -642,6 +642,8 @@ static int ipa_endpoint_init_hol_block_timer(struct ipa_endpoint *endpoint,
>>>  	u32 offset;
>>>  	u32 val;
>>>  
>>> +	/* assert(!endpoint->toward_ipa); */
>>> +
>>>  	/* XXX We'll fix this when the register definition is clear */
>>>  	if (microseconds) {
>>>  		struct device *dev = &ipa->pdev->dev;
>>> @@ -671,6 +673,8 @@ ipa_endpoint_init_hol_block_enable(struct ipa_endpoint *endpoint, bool enable)
>>>  	u32 offset;
>>>  	u32 val;
>>>  
>>> +	/* assert(!endpoint->toward_ipa); */
>> 
>> What are these assert comments for? :S
> 
> They are place holders for when I can put in a proper assert
> function.  For now I'm trying to avoid BUG_ON() calls, but
> at some point soon I'll replace these comments with calls
> to a macro that does BUG_ON() conditioned on a config option
> (or something else if there's a better suggestion).
> 
> Even though it's commented, the assert() call does what
> I want, which is to communicate to the reader a condition
> assumed by the code, succinctly.

Never BUG_ON() unless you absolutely cannot continue executing kernel
without corrupting memory or similar.

If you can error out in some way at all, do not BUG().

Re: [PATCH net-next 1/5] net: ipa: head-of-line block registers are RX only

[Alex Elder]

On 6/29/20 8:03 PM, David Miller wrote:
> Never BUG_ON() unless you absolutely cannot continue executing kernel
> without corrupting memory or similar.

Yes, that's basically why I don't use it.  But the reason I was
considering it conditional on a config option is that Qualcomm
has a crash analysis tool that expects a BUG() call to stop the
system so its instant state can be captured.  I don't use this
tool, and I might be mistaken about what's required.

What I would *really* like to do is have a way to gracefully
shut down just the IPA driver when an unexpected condition occurs,
so I can stop everything without crashing the system.  But doing
that in a way that works in all cases is Hard.

Do you have any suggestions?

					-Alex

Re: [PATCH net-next 1/5] net: ipa: head-of-line block registers are RX only

[David Miller]

From: Alex Elder <elder@linaro.org>
Date: Mon, 29 Jun 2020 20:09:58 -0500

> But the reason I was
> considering it conditional on a config option is that Qualcomm
> has a crash analysis tool that expects a BUG() call to stop the
> system so its instant state can be captured.  I don't use this
> tool, and I might be mistaken about what's required.

A Qualcomm debugging tool with poorly choosen expectations does not
determine how we do things in the kernel.

> What I would *really* like to do is have a way to gracefully
> shut down just the IPA driver when an unexpected condition occurs,
> so I can stop everything without crashing the system.  But doing
> that in a way that works in all cases is Hard.

Users would like their system and the IPA device to continue, even
if in a reduced functionality manner, if possible.

Doing things to make that less likely to be possible is undesirable.

Re: [PATCH net-next 1/5] net: ipa: head-of-line block registers are RX only

[Alex Elder]

On 6/30/20 2:21 PM, David Miller wrote:
> From: Alex Elder <elder@linaro.org>
> Date: Mon, 29 Jun 2020 20:09:58 -0500
> 
>> But the reason I was
>> considering it conditional on a config option is that Qualcomm
>> has a crash analysis tool that expects a BUG() call to stop the
>> system so its instant state can be captured.  I don't use this
>> tool, and I might be mistaken about what's required.
> 
> A Qualcomm debugging tool with poorly choosen expectations does not
> determine how we do things in the kernel.

Of course.  I have no problem saying "that can't be done
upstream."  But I wasn't as sure (before now) that the use
of BUG() even in this way would be a "hard no."  I won't
waste any time trying to implement it.

>> What I would *really* like to do is have a way to gracefully
>> shut down just the IPA driver when an unexpected condition occurs,
>> so I can stop everything without crashing the system.  But doing
>> that in a way that works in all cases is Hard.
> 
> Users would like their system and the IPA device to continue, even
> if in a reduced functionality manner, if possible.

Here too, I completely agree, though I might have done a poor
job of conveying that.  My intention is to recover from any
error if possible, even if it means being only partially
functional.

The only conditions I'd ever treat in this way would be those
that mean "we must not go on," basically along the lines of
what you described for BUG_ON() calls.  My point was to try
to isolate the damage done to the IPA device and driver,
rather than killing the system.

					-Alex

> Doing things to make that less likely to be possible is undesirable.

Re: [PATCH net-next 1/5] net: ipa: head-of-line block registers are RX only

[David Miller]

From: Alex Elder <elder@linaro.org>
Date: Tue, 30 Jun 2020 17:41:44 -0500

> My point was to try to isolate the damage done to the IPA device and
> driver, rather than killing the system.

Excellent, then we are both on the same page.