| Message ID | 20221030001828.754010-10-elder@linaro.org (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: ipa: support more endpoints | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Clearly marked for net-next |
| netdev/fixes_present | success | Fixes tag not required for -next series |
| netdev/subject_prefix | success | Link |
| netdev/cover_letter | success | Series has a cover letter |
| netdev/patch_count | success | Link |
| netdev/header_inline | success | No static functions without inline keyword in header files |
| netdev/build_32bit | success | Errors and warnings before: 0 this patch: 0 |
| netdev/cc_maintainers | success | CCed 6 of 6 maintainers |
| netdev/build_clang | success | Errors and warnings before: 0 this patch: 0 |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Signed-off-by tag matches author and committer |
| netdev/check_selftest | success | No net selftest shell script |
| netdev/verify_fixes | success | No Fixes tag |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 112 lines checked |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/source_inline | success | Was 0 now: 0 |
On Sat, 29 Oct 2022 19:18:28 -0500 Alex Elder wrote: > /* Set up the defined endpoint bitmap */ > ipa->defined = bitmap_zalloc(ipa->endpoint_count, GFP_KERNEL); > ipa->set_up = bitmap_zalloc(ipa->endpoint_count, GFP_KERNEL); > + ipa->enabled = bitmap_zalloc(ipa->endpoint_count, GFP_KERNEL); > if (!ipa->defined || !ipa->set_up) { This condition should now check if ipa->enabled And the error handling patch needs to free it, in case it was something else that didn't get allocated? Frankly I have gotten mass-NULL-checks wrong more than once myself so I'd steer clear of those, they are strangely error prone. > dev_err(dev, "unable to allocate endpoint bitmaps\n"); this error message should not be here (patch 5 adds it I think) memory allocation failures produce a splat, no need to print errors > + bitmap_free(ipa->set_up); > + ipa->set_up = NULL; > bitmap_free(ipa->defined);
On 11/1/22 11:34 PM, Jakub Kicinski wrote: > On Sat, 29 Oct 2022 19:18:28 -0500 Alex Elder wrote: >> /* Set up the defined endpoint bitmap */ >> ipa->defined = bitmap_zalloc(ipa->endpoint_count, GFP_KERNEL); >> ipa->set_up = bitmap_zalloc(ipa->endpoint_count, GFP_KERNEL); >> + ipa->enabled = bitmap_zalloc(ipa->endpoint_count, GFP_KERNEL); >> if (!ipa->defined || !ipa->set_up) { > > This condition should now check if ipa->enabled You're right. > And the error handling patch needs to free it, in case it was something > else that didn't get allocated? See below, but in any case, I'll make sure this is right. > Frankly I have gotten mass-NULL-checks wrong more than once myself so > I'd steer clear of those, they are strangely error prone. I don't typically do it, and generally don't like it, but I think I was trying to make it look cleaner somehow. I'll check every one in separately in v2. >> dev_err(dev, "unable to allocate endpoint bitmaps\n"); > > this error message should not be here (patch 5 adds it I think) > memory allocation failures produce a splat, no need to print errors At the end of the series, the structure of this error handling changes, and I think this is correct. But now that you point this out I think the way this evolves in the series could use some improvement so I'll take another look at it and hopefully make it better. I don't normally report anything on allocation failures for that reason; thanks for pointing this out. I really appreciate your feedback. I'll send out version 2 today. -Alex >> + bitmap_free(ipa->set_up); >> + ipa->set_up = NULL; >> bitmap_free(ipa->defined);
diff --git a/drivers/net/ipa/ipa.h b/drivers/net/ipa/ipa.h index f14d1bd34e7e5..5372db58b5bdc 100644 --- a/drivers/net/ipa/ipa.h +++ b/drivers/net/ipa/ipa.h @@ -67,7 +67,7 @@ struct ipa_interrupt; * @available: Bitmap of endpoints supported by hardware * @filtered: Bitmap of endpoints that support filtering * @set_up: Bitmap of endpoints that are set up for use - * @enabled: Bit mask indicating endpoints enabled + * @enabled: Bitmap of currently enabled endpoints * @modem_tx_count: Number of defined modem TX endoints * @endpoint: Array of endpoint information * @channel_map: Mapping of GSI channel to IPA endpoint @@ -125,7 +125,7 @@ struct ipa { unsigned long *available; /* Supported by hardware */ u64 filtered; /* Support filtering (AP and modem) */ unsigned long *set_up; - u32 enabled; + unsigned long *enabled; u32 modem_tx_count; struct ipa_endpoint endpoint[IPA_ENDPOINT_MAX]; diff --git a/drivers/net/ipa/ipa_endpoint.c b/drivers/net/ipa/ipa_endpoint.c index 564a209f75a0f..ea9ed2da4ff0c 100644 --- a/drivers/net/ipa/ipa_endpoint.c +++ b/drivers/net/ipa/ipa_endpoint.c @@ -1666,6 +1666,7 @@ static void ipa_endpoint_program(struct ipa_endpoint *endpoint) int ipa_endpoint_enable_one(struct ipa_endpoint *endpoint) { + u32 endpoint_id = endpoint->endpoint_id; struct ipa *ipa = endpoint->ipa; struct gsi *gsi = &ipa->gsi; int ret; @@ -1675,37 +1676,35 @@ int ipa_endpoint_enable_one(struct ipa_endpoint *endpoint) dev_err(&ipa->pdev->dev, "error %d starting %cX channel %u for endpoint %u\n", ret, endpoint->toward_ipa ? 'T' : 'R', - endpoint->channel_id, endpoint->endpoint_id); + endpoint->channel_id, endpoint_id); return ret; } if (!endpoint->toward_ipa) { - ipa_interrupt_suspend_enable(ipa->interrupt, - endpoint->endpoint_id); + ipa_interrupt_suspend_enable(ipa->interrupt, endpoint_id); ipa_endpoint_replenish_enable(endpoint); } - ipa->enabled |= BIT(endpoint->endpoint_id); + __set_bit(endpoint_id, ipa->enabled); return 0; } void ipa_endpoint_disable_one(struct ipa_endpoint *endpoint) { - u32 mask = BIT(endpoint->endpoint_id); + u32 endpoint_id = endpoint->endpoint_id; struct ipa *ipa = endpoint->ipa; struct gsi *gsi = &ipa->gsi; int ret; - if (!(ipa->enabled & mask)) + if (!test_bit(endpoint_id, ipa->enabled)) return; - ipa->enabled ^= mask; + __clear_bit(endpoint_id, endpoint->ipa->enabled); if (!endpoint->toward_ipa) { ipa_endpoint_replenish_disable(endpoint); - ipa_interrupt_suspend_disable(ipa->interrupt, - endpoint->endpoint_id); + ipa_interrupt_suspend_disable(ipa->interrupt, endpoint_id); } /* Note that if stop fails, the channel's state is not well-defined */ @@ -1713,7 +1712,7 @@ void ipa_endpoint_disable_one(struct ipa_endpoint *endpoint) if (ret) dev_err(&ipa->pdev->dev, "error %d attempting to stop endpoint %u\n", ret, - endpoint->endpoint_id); + endpoint_id); } void ipa_endpoint_suspend_one(struct ipa_endpoint *endpoint) @@ -1722,7 +1721,7 @@ void ipa_endpoint_suspend_one(struct ipa_endpoint *endpoint) struct gsi *gsi = &endpoint->ipa->gsi; int ret; - if (!(endpoint->ipa->enabled & BIT(endpoint->endpoint_id))) + if (!test_bit(endpoint->endpoint_id, endpoint->ipa->enabled)) return; if (!endpoint->toward_ipa) { @@ -1742,7 +1741,7 @@ void ipa_endpoint_resume_one(struct ipa_endpoint *endpoint) struct gsi *gsi = &endpoint->ipa->gsi; int ret; - if (!(endpoint->ipa->enabled & BIT(endpoint->endpoint_id))) + if (!test_bit(endpoint->endpoint_id, endpoint->ipa->enabled)) return; if (!endpoint->toward_ipa) @@ -1970,8 +1969,10 @@ void ipa_endpoint_exit(struct ipa *ipa) for_each_set_bit(endpoint_id, ipa->defined, ipa->endpoint_count) ipa_endpoint_exit_one(&ipa->endpoint[endpoint_id]); + bitmap_free(ipa->enabled); bitmap_free(ipa->set_up); bitmap_free(ipa->defined); + ipa->enabled = NULL; ipa->set_up = NULL; ipa->defined = NULL; @@ -1997,8 +1998,11 @@ int ipa_endpoint_init(struct ipa *ipa, u32 count, /* Set up the defined endpoint bitmap */ ipa->defined = bitmap_zalloc(ipa->endpoint_count, GFP_KERNEL); ipa->set_up = bitmap_zalloc(ipa->endpoint_count, GFP_KERNEL); + ipa->enabled = bitmap_zalloc(ipa->endpoint_count, GFP_KERNEL); if (!ipa->defined || !ipa->set_up) { dev_err(dev, "unable to allocate endpoint bitmaps\n"); + bitmap_free(ipa->set_up); + ipa->set_up = NULL; bitmap_free(ipa->defined); ipa->defined = NULL; return -ENOMEM;
Replace the 32-bit unsigned used to track enabled endpoints with a Linux bitmap, to allow an arbitrary number of endpoints to be represented. Signed-off-by: Alex Elder <elder@linaro.org> --- drivers/net/ipa/ipa.h | 4 ++-- drivers/net/ipa/ipa_endpoint.c | 28 ++++++++++++++++------------ 2 files changed, 18 insertions(+), 14 deletions(-)