Version 2 of this series corrects a misspelling of "outstanding"
pointed out by the netdev test bots. (For some reason I don't see
that when I run "checkpatch".) I found and fixed a second instance
of that word being misspelled as well.
This series includes three changes to the transaction code. The
first adds a new transaction list that represents a distinct state
that has not been maintained. The second moves a field in the
transaction information structure, and reorders its initialization
a bit. The third skips a function call when it is known not to be
necessary.
The last two are very small "leftover" patches.
-Alex
Alex Elder (5):
net: ipa: add a transaction committed list
net: ipa: rearrange transaction initialization
net: ipa: skip some cleanup for unused transactions
net: ipa: report when the driver has been removed
net: ipa: fix an outdated comment
drivers/net/ipa/gsi.c | 5 ++-
drivers/net/ipa/gsi.h | 8 ++--
drivers/net/ipa/gsi_trans.c | 89 +++++++++++++++++++++++--------------
drivers/net/ipa/ipa_main.c | 2 +
4 files changed, 66 insertions(+), 38 deletions(-)
--
2.34.1
In gsi_trans_free(), there's no point in ipa_gsi_trans_release() if
a transaction is unused. No used TREs means no IPA layer resources
to clean up. So only call ipa_gsi_trans_release() if at least one
TRE was used.
Signed-off-by: Alex Elder <[email protected]>
---
drivers/net/ipa/gsi_trans.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/net/ipa/gsi_trans.c b/drivers/net/ipa/gsi_trans.c
index 3f52932e9e413..55987e35af2dd 100644
--- a/drivers/net/ipa/gsi_trans.c
+++ b/drivers/net/ipa/gsi_trans.c
@@ -404,7 +404,8 @@ void gsi_trans_free(struct gsi_trans *trans)
if (!last)
return;
- ipa_gsi_trans_release(trans);
+ if (trans->used_count)
+ ipa_gsi_trans_release(trans);
/* Releasing the reserved TREs implicitly frees the sgl[] and
* (if present) info[] arrays, plus the transaction itself.
--
2.34.1
The transaction map is really associated with the transaction pool;
move its definition earlier in the gsi_trans_info structure.
Rearrange initialization in gsi_channel_trans_init() so it
sets the tre_avail value first, then initializes the transaction
pool, and finally allocating the transaction map.
Update comments.
Signed-off-by: Alex Elder <[email protected]>
---
v2: Fixed the misspelling of "outstanding" in two spots.
drivers/net/ipa/gsi.h | 3 +-
drivers/net/ipa/gsi_trans.c | 60 +++++++++++++++++++------------------
2 files changed, 33 insertions(+), 30 deletions(-)
diff --git a/drivers/net/ipa/gsi.h b/drivers/net/ipa/gsi.h
index 1c8941911069d..c197df64e69a2 100644
--- a/drivers/net/ipa/gsi.h
+++ b/drivers/net/ipa/gsi.h
@@ -82,9 +82,10 @@ struct gsi_trans_pool {
struct gsi_trans_info {
atomic_t tre_avail; /* TREs available for allocation */
struct gsi_trans_pool pool; /* transaction pool */
+ struct gsi_trans **map; /* TRE -> transaction map */
+
struct gsi_trans_pool sg_pool; /* scatterlist pool */
struct gsi_trans_pool cmd_pool; /* command payload DMA pool */
- struct gsi_trans **map; /* TRE -> transaction map */
spinlock_t spinlock; /* protects updates to the lists */
struct list_head alloc; /* allocated, not committed */
diff --git a/drivers/net/ipa/gsi_trans.c b/drivers/net/ipa/gsi_trans.c
index 45572ebb76e95..3f52932e9e413 100644
--- a/drivers/net/ipa/gsi_trans.c
+++ b/drivers/net/ipa/gsi_trans.c
@@ -709,6 +709,7 @@ void gsi_trans_read_byte_done(struct gsi *gsi, u32 channel_id)
int gsi_channel_trans_init(struct gsi *gsi, u32 channel_id)
{
struct gsi_channel *channel = &gsi->channel[channel_id];
+ u32 tre_count = channel->tre_count;
struct gsi_trans_info *trans_info;
u32 tre_max;
int ret;
@@ -716,30 +717,40 @@ int gsi_channel_trans_init(struct gsi *gsi, u32 channel_id)
/* Ensure the size of a channel element is what's expected */
BUILD_BUG_ON(sizeof(struct gsi_tre) != GSI_RING_ELEMENT_SIZE);
- /* The map array is used to determine what transaction is associated
- * with a TRE that the hardware reports has completed. We need one
- * map entry per TRE.
- */
trans_info = &channel->trans_info;
- trans_info->map = kcalloc(channel->tre_count, sizeof(*trans_info->map),
- GFP_KERNEL);
- if (!trans_info->map)
- return -ENOMEM;
- /* We can't use more TREs than there are available in the ring.
+ /* The tre_avail field is what ultimately limits the number of
+ * outstanding transactions and their resources. A transaction
+ * allocation succeeds only if the TREs available are sufficient
+ * for what the transaction might need.
+ */
+ tre_max = gsi_channel_tre_max(channel->gsi, channel_id);
+ atomic_set(&trans_info->tre_avail, tre_max);
+
+ /* We can't use more TREs than the number available in the ring.
* This limits the number of transactions that can be outstanding.
* Worst case is one TRE per transaction (but we actually limit
- * it to something a little less than that). We allocate resources
- * for transactions (including transaction structures) based on
- * this maximum number.
+ * it to something a little less than that). By allocating a
+ * power-of-two number of transactions we can use an index
+ * modulo that number to determine the next one that's free.
+ * Transactions are allocated one at a time.
*/
- tre_max = gsi_channel_tre_max(channel->gsi, channel_id);
-
- /* Transactions are allocated one at a time. */
ret = gsi_trans_pool_init(&trans_info->pool, sizeof(struct gsi_trans),
tre_max, 1);
if (ret)
- goto err_kfree;
+ return -ENOMEM;
+
+ /* A completion event contains a pointer to the TRE that caused
+ * the event (which will be the last one used by the transaction).
+ * Each entry in this map records the transaction associated
+ * with a corresponding completed TRE.
+ */
+ trans_info->map = kcalloc(tre_count, sizeof(*trans_info->map),
+ GFP_KERNEL);
+ if (!trans_info->map) {
+ ret = -ENOMEM;
+ goto err_trans_free;
+ }
/* A transaction uses a scatterlist array to represent the data
* transfers implemented by the transaction. Each scatterlist
@@ -751,16 +762,7 @@ int gsi_channel_trans_init(struct gsi *gsi, u32 channel_id)
sizeof(struct scatterlist),
tre_max, channel->trans_tre_max);
if (ret)
- goto err_trans_pool_exit;
-
- /* Finally, the tre_avail field is what ultimately limits the number
- * of outstanding transactions and their resources. A transaction
- * allocation succeeds only if the TREs available are sufficient for
- * what the transaction might need. Transaction resource pools are
- * sized based on the maximum number of outstanding TREs, so there
- * will always be resources available if there are TREs available.
- */
- atomic_set(&trans_info->tre_avail, tre_max);
+ goto err_map_free;
spin_lock_init(&trans_info->spinlock);
INIT_LIST_HEAD(&trans_info->alloc);
@@ -771,10 +773,10 @@ int gsi_channel_trans_init(struct gsi *gsi, u32 channel_id)
return 0;
-err_trans_pool_exit:
- gsi_trans_pool_exit(&trans_info->pool);
-err_kfree:
+err_map_free:
kfree(trans_info->map);
+err_trans_free:
+ gsi_trans_pool_exit(&trans_info->pool);
dev_err(gsi->dev, "error %d initializing channel %u transactions\n",
ret, channel_id);
--
2.34.1
When the IPA driver has completed its initialization and setup
stages, it emits a brief message to the log. Add a small message
that reports when it has been removed.
Signed-off-by: Alex Elder <[email protected]>
---
drivers/net/ipa/ipa_main.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/net/ipa/ipa_main.c b/drivers/net/ipa/ipa_main.c
index 3757ce3de2c59..96c649d889a7c 100644
--- a/drivers/net/ipa/ipa_main.c
+++ b/drivers/net/ipa/ipa_main.c
@@ -836,6 +836,8 @@ static int ipa_remove(struct platform_device *pdev)
kfree(ipa);
ipa_power_exit(power);
+ dev_info(dev, "IPA driver removed");
+
return 0;
}
--
2.34.1
Hello:
This series was applied to netdev/net-next.git (master)
by Jakub Kicinski <[email protected]>:
On Tue, 19 Jul 2022 13:10:15 -0500 you wrote:
> Version 2 of this series corrects a misspelling of "outstanding"
> pointed out by the netdev test bots. (For some reason I don't see
> that when I run "checkpatch".) I found and fixed a second instance
> of that word being misspelled as well.
>
> This series includes three changes to the transaction code. The
> first adds a new transaction list that represents a distinct state
> that has not been maintained. The second moves a field in the
> transaction information structure, and reorders its initialization
> a bit. The third skips a function call when it is known not to be
> necessary.
>
> [...]
Here is the summary with links:
- [net-next,v2,1/5] net: ipa: add a transaction committed list
https://git.kernel.org/netdev/net-next/c/b63f507c06e6
- [net-next,v2,2/5] net: ipa: rearrange transaction initialization
(no matching commit)
- [net-next,v2,3/5] net: ipa: skip some cleanup for unused transactions
https://git.kernel.org/netdev/net-next/c/4d8996cbeeab
- [net-next,v2,4/5] net: ipa: report when the driver has been removed
https://git.kernel.org/netdev/net-next/c/3c91c86d1bb6
- [net-next,v2,5/5] net: ipa: fix an outdated comment
https://git.kernel.org/netdev/net-next/c/616c4a83b6ea
You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html