| Message ID | 20200629214919.1196017-2-elder@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | net: ipa: endpoint configuration updates | expand |
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
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
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().
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
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.
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.
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.
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);
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(-) -- 2.25.1