This series adds support for more than 32 IPA endpoints. To do
this, five registers whose bits represent endpoint state are
replicated as needed to represent endpoints beyond 32. For existing
platforms, the number of endpoints is never greater than 32, so
there is just one of each register. IPA v5.0+ supports more than
that though; these changes prepare the code for that.
Beyond that, the IPA fields that represent endpoints in a 32-bit
bitmask are updated to support an arbitrary number of these endpoint
registers. (There is one exception, explained in patch 7.)
The first two patches are some sort of unrelated cleanups, making
use of a helper function introduced recently.
The third and fourth use parameterized functions to determine the
register offset for registers that represent endpoints.
The last five convert fields representing endpoints to allow more
than 32 endpoints to be represented.
Signed-off-by: Alex Elder <[email protected]>
Alex Elder (9):
net: ipa: reduce arguments to ipa_table_init_add()
net: ipa: use ipa_table_mem() in ipa_table_reset_add()
net: ipa: add a parameter to aggregation registers
net: ipa: add a parameter to suspend registers
net: ipa: use a bitmap for defined endpoints
net: ipa: use a bitmap for available endpoints
net: ipa: support more filtering endpoints
net: ipa: use a bitmap for set-up endpoints
net: ipa: use a bitmap for enabled endpoints
drivers/net/ipa/ipa.h | 26 +++--
drivers/net/ipa/ipa_endpoint.c | 167 +++++++++++++++------------
drivers/net/ipa/ipa_endpoint.h | 2 +-
drivers/net/ipa/ipa_interrupt.c | 34 ++++--
drivers/net/ipa/ipa_main.c | 7 +-
drivers/net/ipa/ipa_table.c | 88 +++++++-------
drivers/net/ipa/ipa_table.h | 6 +-
drivers/net/ipa/reg/ipa_reg-v3.1.c | 13 ++-
drivers/net/ipa/reg/ipa_reg-v3.5.1.c | 13 ++-
drivers/net/ipa/reg/ipa_reg-v4.11.c | 13 ++-
drivers/net/ipa/reg/ipa_reg-v4.2.c | 13 ++-
drivers/net/ipa/reg/ipa_reg-v4.5.c | 13 ++-
drivers/net/ipa/reg/ipa_reg-v4.9.c | 13 ++-
13 files changed, 227 insertions(+), 181 deletions(-)
--
2.34.1
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 <[email protected]>
---
drivers/net/ipa/ipa.h | 4 ++--
drivers/net/ipa/ipa_endpoint.c | 28 ++++++++++++++++------------
2 files changed, 18 insertions(+), 14 deletions(-)
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;
--
2.34.1
Similar to the previous commit, pass flags rather than a memory
region ID to ipa_table_reset_add(), and there use ipa_table_mem() to
look up the memory region affected based on those flags.
Currently all eight of these table memory regions are assumed to
exist, because they all have canaries within them. Stop assuming
that will always be the case, and in ipa_table_reset_add() allow
these memory regions to be non-existent.
Signed-off-by: Alex Elder <[email protected]>
---
drivers/net/ipa/ipa_table.c | 31 +++++++++++++++----------------
1 file changed, 15 insertions(+), 16 deletions(-)
diff --git a/drivers/net/ipa/ipa_table.c b/drivers/net/ipa/ipa_table.c
index 94bb7611e574b..3a14465bf8a64 100644
--- a/drivers/net/ipa/ipa_table.c
+++ b/drivers/net/ipa/ipa_table.c
@@ -200,16 +200,17 @@ static dma_addr_t ipa_table_addr(struct ipa *ipa, bool filter_mask, u16 count)
}
static void ipa_table_reset_add(struct gsi_trans *trans, bool filter,
- u16 first, u16 count, enum ipa_mem_id mem_id)
+ bool hashed, bool ipv6, u16 first, u16 count)
{
struct ipa *ipa = container_of(trans->gsi, struct ipa, gsi);
- const struct ipa_mem *mem = ipa_mem_find(ipa, mem_id);
+ const struct ipa_mem *mem;
dma_addr_t addr;
u32 offset;
u16 size;
- /* Nothing to do if the table memory region is empty */
- if (!mem->size)
+ /* Nothing to do if the memory region is doesn't exist or is empty */
+ mem = ipa_table_mem(ipa, filter, hashed, ipv6);
+ if (!mem || !mem->size)
return;
if (filter)
@@ -227,7 +228,7 @@ static void ipa_table_reset_add(struct gsi_trans *trans, bool filter,
* for the IPv4 and IPv6 non-hashed and hashed filter tables.
*/
static int
-ipa_filter_reset_table(struct ipa *ipa, enum ipa_mem_id mem_id, bool modem)
+ipa_filter_reset_table(struct ipa *ipa, bool hashed, bool ipv6, bool modem)
{
u32 ep_mask = ipa->filter_map;
u32 count = hweight32(ep_mask);
@@ -253,7 +254,7 @@ ipa_filter_reset_table(struct ipa *ipa, enum ipa_mem_id mem_id, bool modem)
if (endpoint->ee_id != ee_id)
continue;
- ipa_table_reset_add(trans, true, endpoint_id, 1, mem_id);
+ ipa_table_reset_add(trans, true, hashed, ipv6, endpoint_id, 1);
}
gsi_trans_commit_wait(trans);
@@ -269,18 +270,18 @@ static int ipa_filter_reset(struct ipa *ipa, bool modem)
{
int ret;
- ret = ipa_filter_reset_table(ipa, IPA_MEM_V4_FILTER, modem);
+ ret = ipa_filter_reset_table(ipa, false, false, modem);
if (ret)
return ret;
- ret = ipa_filter_reset_table(ipa, IPA_MEM_V4_FILTER_HASHED, modem);
+ ret = ipa_filter_reset_table(ipa, true, false, modem);
if (ret)
return ret;
- ret = ipa_filter_reset_table(ipa, IPA_MEM_V6_FILTER, modem);
+ ret = ipa_filter_reset_table(ipa, false, true, modem);
if (ret)
return ret;
- ret = ipa_filter_reset_table(ipa, IPA_MEM_V6_FILTER_HASHED, modem);
+ ret = ipa_filter_reset_table(ipa, true, true, modem);
return ret;
}
@@ -312,13 +313,11 @@ static int ipa_route_reset(struct ipa *ipa, bool modem)
count = ipa->route_count - modem_route_count;
}
- ipa_table_reset_add(trans, false, first, count, IPA_MEM_V4_ROUTE);
- ipa_table_reset_add(trans, false, first, count,
- IPA_MEM_V4_ROUTE_HASHED);
+ ipa_table_reset_add(trans, false, false, false, first, count);
+ ipa_table_reset_add(trans, false, true, false, first, count);
- ipa_table_reset_add(trans, false, first, count, IPA_MEM_V6_ROUTE);
- ipa_table_reset_add(trans, false, first, count,
- IPA_MEM_V6_ROUTE_HASHED);
+ ipa_table_reset_add(trans, false, false, true, first, count);
+ ipa_table_reset_add(trans, false, true, true, first, count);
gsi_trans_commit_wait(trans);
--
2.34.1
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);