A previous group of patches added ID fields to track the state of
transactions:
https://lore.kernel.org/netdev/[email protected]
This series starts using those IDs instead of the lists used
previously. Most of this series involves reworking the function
that determines which transaction is the "last", which determines
when a channel has been quiesed. The last patch is mainly used to
prove that the new index method of tracking transaction state is
equivalent to the previous use of lists.
-Alex
Alex Elder (6):
net: ipa: rework last transaction determination
net: ipa: use IDs for last allocated transaction
net: ipa: use IDs exclusively for last transaction
net: ipa: simplify gsi_channel_trans_last()
net: ipa: further simplify gsi_channel_trans_last()
net: ipa: verify a few more IDs
drivers/net/ipa/gsi.c | 53 +++++++++++----------------
drivers/net/ipa/gsi_private.h | 14 ++++++++
drivers/net/ipa/gsi_trans.c | 68 ++++++++++++++++++++++-------------
3 files changed, 78 insertions(+), 57 deletions(-)
--
2.34.1
When quiescing a channel, we find the "last" transaction, which is
the latest one to have been allocated. (New transaction allocation
will have been prevented by the time this is called.)
Currently we do this by looking for the first non-empty transaction
list in each state, then return the last entry from that last.
Instead, determine the last entry in each list (if any) and return
that entry if found.
Temporarily (locally) introduce list_last_entry_or_null() as a
helper for this, mirroring list_first_entry_or_null(). This macro
definition will be removed by an upcoming patch.
Remove the temporary warnings added by the previous commit.
Signed-off-by: Alex Elder <[email protected]>
---
drivers/net/ipa/gsi.c | 28 ++++++++++++++--------------
drivers/net/ipa/gsi_private.h | 14 ++++++++++++++
drivers/net/ipa/gsi_trans.c | 22 ----------------------
3 files changed, 28 insertions(+), 36 deletions(-)
diff --git a/drivers/net/ipa/gsi.c b/drivers/net/ipa/gsi.c
index 9e307eebd33f9..0ea98fa5dee56 100644
--- a/drivers/net/ipa/gsi.c
+++ b/drivers/net/ipa/gsi.c
@@ -710,7 +710,6 @@ static void gsi_evt_ring_program(struct gsi *gsi, u32 evt_ring_id)
static struct gsi_trans *gsi_channel_trans_last(struct gsi_channel *channel)
{
struct gsi_trans_info *trans_info = &channel->trans_info;
- const struct list_head *list;
struct gsi_trans *trans;
spin_lock_bh(&trans_info->spinlock);
@@ -719,29 +718,30 @@ static struct gsi_trans *gsi_channel_trans_last(struct gsi_channel *channel)
* before we disabled transmits, so check for that.
*/
if (channel->toward_ipa) {
- list = &trans_info->alloc;
- if (!list_empty(list))
+ trans = list_last_entry_or_null(&trans_info->alloc,
+ struct gsi_trans, links);
+ if (trans)
goto done;
- list = &trans_info->committed;
- if (!list_empty(list))
+ trans = list_last_entry_or_null(&trans_info->committed,
+ struct gsi_trans, links);
+ if (trans)
goto done;
- list = &trans_info->pending;
- if (!list_empty(list))
+ trans = list_last_entry_or_null(&trans_info->pending,
+ struct gsi_trans, links);
+ if (trans)
goto done;
}
/* Otherwise (TX or RX) we want to wait for anything that
* has completed, or has been polled but not released yet.
*/
- list = &trans_info->complete;
- if (!list_empty(list))
+ trans = list_last_entry_or_null(&trans_info->complete,
+ struct gsi_trans, links);
+ if (trans)
goto done;
- list = &trans_info->polled;
- if (list_empty(list))
- list = NULL;
+ trans = list_last_entry_or_null(&trans_info->polled,
+ struct gsi_trans, links);
done:
- trans = list ? list_last_entry(list, struct gsi_trans, links) : NULL;
-
/* Caller will wait for this, so take a reference */
if (trans)
refcount_inc(&trans->refcount);
diff --git a/drivers/net/ipa/gsi_private.h b/drivers/net/ipa/gsi_private.h
index 0b2516fa21b5d..51bbc7a40dc2d 100644
--- a/drivers/net/ipa/gsi_private.h
+++ b/drivers/net/ipa/gsi_private.h
@@ -16,6 +16,20 @@ struct gsi_channel;
#define GSI_RING_ELEMENT_SIZE 16 /* bytes; must be a power of 2 */
+/**
+ * list_last_entry_or_null - get the last element from a list
+ * @ptr: the list head to take the element from.
+ * @type: the type of the struct this is embedded in.
+ * @member: the name of the list_head within the struct.
+ *
+ * Note that if the list is empty, it returns NULL.
+ */
+#define list_last_entry_or_null(ptr, type, member) ({ \
+ struct list_head *head__ = (ptr); \
+ struct list_head *pos__ = READ_ONCE(head__->prev); \
+ pos__ != head__ ? list_entry(pos__, type, member) : NULL; \
+})
+
/**
* gsi_trans_move_complete() - Mark a GSI transaction completed
* @trans: Transaction to commit
diff --git a/drivers/net/ipa/gsi_trans.c b/drivers/net/ipa/gsi_trans.c
index 4eef1480c2005..b4a6f2b563566 100644
--- a/drivers/net/ipa/gsi_trans.c
+++ b/drivers/net/ipa/gsi_trans.c
@@ -309,23 +309,15 @@ void gsi_trans_move_polled(struct gsi_trans *trans)
{
struct gsi_channel *channel = &trans->gsi->channel[trans->channel_id];
struct gsi_trans_info *trans_info = &channel->trans_info;
- u16 trans_index;
spin_lock_bh(&trans_info->spinlock);
list_move_tail(&trans->links, &trans_info->polled);
- trans = list_first_entry(&trans_info->polled,
- struct gsi_trans, links);
-
spin_unlock_bh(&trans_info->spinlock);
/* This completed transaction is now polled */
trans_info->completed_id++;
-
- WARN_ON(trans_info->polled_id == trans_info->completed_id);
- trans_index = trans_info->polled_id % channel->tre_count;
- WARN_ON(trans != &trans_info->trans[trans_index]);
}
/* Reserve some number of TREs on a channel. Returns true if successful */
@@ -413,11 +405,8 @@ struct gsi_trans *gsi_channel_trans_alloc(struct gsi *gsi, u32 channel_id,
/* Free a previously-allocated transaction */
void gsi_trans_free(struct gsi_trans *trans)
{
- struct gsi_channel *channel = &trans->gsi->channel[trans->channel_id];
refcount_t *refcount = &trans->refcount;
struct gsi_trans_info *trans_info;
- struct gsi_trans *polled;
- u16 trans_index;
bool last;
/* We must hold the lock to release the last reference */
@@ -433,9 +422,6 @@ void gsi_trans_free(struct gsi_trans *trans)
if (last)
list_del(&trans->links);
- polled = list_first_entry_or_null(&trans_info->polled,
- struct gsi_trans, links);
-
spin_unlock_bh(&trans_info->spinlock);
if (!last)
@@ -456,14 +442,6 @@ void gsi_trans_free(struct gsi_trans *trans)
/* This transaction is now free */
trans_info->polled_id++;
- if (polled) {
- trans_index = trans_info->polled_id % channel->tre_count;
- WARN_ON(polled != &trans_info->trans[trans_index]);
- } else {
- WARN_ON(trans_info->polled_id !=
- trans_info->completed_id);
- }
-
/* Releasing the reserved TREs implicitly frees the sgl[] and
* (if present) info[] arrays, plus the transaction itself.
*/
--
2.34.1
Use the allocated and free transaction IDs to determine whether the
"last" transaction used for quiescing a channel is in allocated
state. The last allocated transaction that has not been committed
(if any) immediately precedes the first free transaction in the
transaction array.
Signed-off-by: Alex Elder <[email protected]>
---
drivers/net/ipa/gsi.c | 12 +++++++++---
1 file changed, 9 insertions(+), 3 deletions(-)
diff --git a/drivers/net/ipa/gsi.c b/drivers/net/ipa/gsi.c
index 0ea98fa5dee56..135e51980d793 100644
--- a/drivers/net/ipa/gsi.c
+++ b/drivers/net/ipa/gsi.c
@@ -711,6 +711,8 @@ static struct gsi_trans *gsi_channel_trans_last(struct gsi_channel *channel)
{
struct gsi_trans_info *trans_info = &channel->trans_info;
struct gsi_trans *trans;
+ u16 trans_index;
+ u16 trans_id;
spin_lock_bh(&trans_info->spinlock);
@@ -718,10 +720,14 @@ static struct gsi_trans *gsi_channel_trans_last(struct gsi_channel *channel)
* before we disabled transmits, so check for that.
*/
if (channel->toward_ipa) {
- trans = list_last_entry_or_null(&trans_info->alloc,
- struct gsi_trans, links);
- if (trans)
+ /* The last allocated transaction precedes the first free */
+ if (trans_info->allocated_id != trans_info->free_id) {
+ trans_id = trans_info->free_id - 1;
+ trans_index = trans_id % channel->tre_count;
+ trans = &trans_info->trans[trans_index];
goto done;
+ }
+
trans = list_last_entry_or_null(&trans_info->committed,
struct gsi_trans, links);
if (trans)
--
2.34.1
Do a little more refactoring in gsi_channel_trans_last() to simplify
it further. The resulting code should behave exactly as before.
Signed-off-by: Alex Elder <[email protected]>
---
drivers/net/ipa/gsi.c | 46 +++++++++++++++++--------------------------
1 file changed, 18 insertions(+), 28 deletions(-)
diff --git a/drivers/net/ipa/gsi.c b/drivers/net/ipa/gsi.c
index 841a946bc286a..16df699009a86 100644
--- a/drivers/net/ipa/gsi.c
+++ b/drivers/net/ipa/gsi.c
@@ -710,42 +710,32 @@ static void gsi_evt_ring_program(struct gsi *gsi, u32 evt_ring_id)
static struct gsi_trans *gsi_channel_trans_last(struct gsi_channel *channel)
{
struct gsi_trans_info *trans_info = &channel->trans_info;
+ u32 pending_id = trans_info->pending_id;
struct gsi_trans *trans;
- u16 trans_index;
u16 trans_id;
- /* There is a small chance a TX transaction got allocated just
- * before we disabled transmits, so check for that.
- */
- if (channel->toward_ipa) {
- /* The last allocated, committed, or pending transaction
+ if (channel->toward_ipa && pending_id != trans_info->free_id) {
+ /* There is a small chance a TX transaction got allocated
+ * just before we disabled transmits, so check for that.
+ * The last allocated, committed, or pending transaction
* precedes the first free transaction.
*/
- if (trans_info->pending_id != trans_info->free_id) {
- trans_id = trans_info->free_id - 1;
- trans_index = trans_id % channel->tre_count;
- trans = &trans_info->trans[trans_index];
- goto done;
- }
- }
-
- /* Otherwise (TX or RX) we want to wait for anything that
- * has completed, or has been polled but not released yet.
- *
- * The last completed or polled transaction precedes the
- * first pending transaction.
- */
- if (trans_info->polled_id != trans_info->pending_id) {
- trans_id = trans_info->pending_id - 1;
- trans_index = trans_id % channel->tre_count;
- trans = &trans_info->trans[trans_index];
+ trans_id = trans_info->free_id - 1;
+ } else if (trans_info->polled_id != pending_id) {
+ /* Otherwise (TX or RX) we want to wait for anything that
+ * has completed, or has been polled but not released yet.
+ *
+ * The last completed or polled transaction precedes the
+ * first pending transaction.
+ */
+ trans_id = pending_id - 1;
} else {
- trans = NULL;
+ return NULL;
}
-done:
+
/* Caller will wait for this, so take a reference */
- if (trans)
- refcount_inc(&trans->refcount);
+ trans = &trans_info->trans[trans_id % channel->tre_count];
+ refcount_inc(&trans->refcount);
return trans;
}
--
2.34.1
Always use transaction IDs when finding the "last" transaction to
await when quiescing a channel. This basically extends what was
done in the previous patch to all other transaction state IDs.
As a result we are no longer updating any transaction lists inside
gsi_channel_trans_last(), so there's no need to take the spinlock.
Signed-off-by: Alex Elder <[email protected]>
---
drivers/net/ipa/gsi.c | 42 +++++++++++++++++++++++++++---------------
1 file changed, 27 insertions(+), 15 deletions(-)
diff --git a/drivers/net/ipa/gsi.c b/drivers/net/ipa/gsi.c
index 135e51980d793..0983a11409f2d 100644
--- a/drivers/net/ipa/gsi.c
+++ b/drivers/net/ipa/gsi.c
@@ -714,8 +714,6 @@ static struct gsi_trans *gsi_channel_trans_last(struct gsi_channel *channel)
u16 trans_index;
u16 trans_id;
- spin_lock_bh(&trans_info->spinlock);
-
/* There is a small chance a TX transaction got allocated just
* before we disabled transmits, so check for that.
*/
@@ -728,32 +726,46 @@ static struct gsi_trans *gsi_channel_trans_last(struct gsi_channel *channel)
goto done;
}
- trans = list_last_entry_or_null(&trans_info->committed,
- struct gsi_trans, links);
- if (trans)
+ /* Last committed transaction precedes the first allocated */
+ if (trans_info->committed_id != trans_info->allocated_id) {
+ trans_id = trans_info->allocated_id - 1;
+ trans_index = trans_id % channel->tre_count;
+ trans = &trans_info->trans[trans_index];
goto done;
- trans = list_last_entry_or_null(&trans_info->pending,
- struct gsi_trans, links);
- if (trans)
+ }
+
+ /* Last pending transaction precedes the first committed */
+ if (trans_info->pending_id != trans_info->committed_id) {
+ trans_id = trans_info->committed_id - 1;
+ trans_index = trans_id % channel->tre_count;
+ trans = &trans_info->trans[trans_index];
goto done;
+ }
}
/* Otherwise (TX or RX) we want to wait for anything that
* has completed, or has been polled but not released yet.
+ *
+ * The last pending transaction precedes the first committed.
*/
- trans = list_last_entry_or_null(&trans_info->complete,
- struct gsi_trans, links);
- if (trans)
+ if (trans_info->completed_id != trans_info->pending_id) {
+ trans_id = trans_info->pending_id - 1;
+ trans_index = trans_id % channel->tre_count;
+ trans = &trans_info->trans[trans_index];
goto done;
- trans = list_last_entry_or_null(&trans_info->polled,
- struct gsi_trans, links);
+ }
+ if (trans_info->polled_id != trans_info->completed_id) {
+ trans_id = trans_info->completed_id - 1;
+ trans_index = trans_id % channel->tre_count;
+ trans = &trans_info->trans[trans_index];
+ } else {
+ trans = NULL;
+ }
done:
/* Caller will wait for this, so take a reference */
if (trans)
refcount_inc(&trans->refcount);
- spin_unlock_bh(&trans_info->spinlock);
-
return trans;
}
--
2.34.1
The completed transaction list is used in gsi_channel_trans_complete()
to return the next transaction in completed state.
Add some temporary checks to verify the transaction indicated by the
completed ID matches the one first in this list.
Similarly, we use the pending and completed transaction lists when
cancelling pending transactions in gsi_channel_trans_cancel_pending().
Add temporary checks there to verify the transactions indicated by
IDs match those tracked by these lists.
Signed-off-by: Alex Elder <[email protected]>
---
drivers/net/ipa/gsi_trans.c | 46 ++++++++++++++++++++++++++++++++++---
1 file changed, 43 insertions(+), 3 deletions(-)
diff --git a/drivers/net/ipa/gsi_trans.c b/drivers/net/ipa/gsi_trans.c
index b4a6f2b563566..05ab4d052c68b 100644
--- a/drivers/net/ipa/gsi_trans.c
+++ b/drivers/net/ipa/gsi_trans.c
@@ -237,8 +237,24 @@ gsi_channel_trans_mapped(struct gsi_channel *channel, u32 index)
/* Return the oldest completed transaction for a channel (or null) */
struct gsi_trans *gsi_channel_trans_complete(struct gsi_channel *channel)
{
- return list_first_entry_or_null(&channel->trans_info.complete,
- struct gsi_trans, links);
+ struct gsi_trans_info *trans_info = &channel->trans_info;
+ u16 trans_id = trans_info->completed_id;
+ struct gsi_trans *trans;
+
+ trans = list_first_entry_or_null(&trans_info->complete,
+ struct gsi_trans, links);
+
+ if (!trans) {
+ WARN_ON(trans_id != trans_info->pending_id);
+ return NULL;
+ }
+
+ if (!WARN_ON(trans_id == trans_info->pending_id)) {
+ trans_id %= channel->tre_count;
+ WARN_ON(trans != &trans_info->trans[trans_id]);
+ }
+
+ return trans;
}
/* Move a transaction from the allocated list to the committed list */
@@ -690,6 +706,8 @@ void gsi_channel_trans_cancel_pending(struct gsi_channel *channel)
{
struct gsi_trans_info *trans_info = &channel->trans_info;
struct gsi_trans *trans;
+ struct gsi_trans *first;
+ struct gsi_trans *last;
bool cancelled;
/* channel->gsi->mutex is held by caller */
@@ -701,11 +719,33 @@ void gsi_channel_trans_cancel_pending(struct gsi_channel *channel)
list_splice_tail_init(&trans_info->pending, &trans_info->complete);
+ first = list_first_entry_or_null(&trans_info->complete,
+ struct gsi_trans, links);
+ last = list_last_entry_or_null(&trans_info->complete,
+ struct gsi_trans, links);
+
spin_unlock_bh(&trans_info->spinlock);
+ /* All pending transactions are now completed */
+ WARN_ON(cancelled != (trans_info->pending_id !=
+ trans_info->committed_id));
+
+ trans_info->pending_id = trans_info->committed_id;
+
/* Schedule NAPI polling to complete the cancelled transactions */
- if (cancelled)
+ if (cancelled) {
+ u16 trans_id;
+
napi_schedule(&channel->napi);
+
+ trans_id = trans_info->completed_id;
+ trans = &trans_info->trans[trans_id % channel->tre_count];
+ WARN_ON(trans != first);
+
+ trans_id = trans_info->pending_id - 1;
+ trans = &trans_info->trans[trans_id % channel->tre_count];
+ WARN_ON(trans != last);
+ }
}
/* Issue a command to read a single byte from a channel */
--
2.34.1
Using a little logic we can simplify gsi_channel_trans_last().
The first condition in that function looks like this:
if (trans_info->allocated_id != trans_info->free_id)
And if that's false, we proceed to the next one:
if (trans_info->committed_id != trans_info->allocated_id)
Failure of the first test implies:
trans_info->allocated_id == trans_info->free_id
And therefore, the second one can be rewritten this way:
if (trans_info->committed_id != trans_info->free_id)
Substituting free_id for allocated_id and committed_id can also be
done in the code blocks executed when these conditions yield true.
The net result is that all three blocks for TX endpoints can be
consolidated into just one.
The two blocks of code at the end of that function (used for both TX
and RX channels) can be similarly consolidated into a single block.
Signed-off-by: Alex Elder <[email protected]>
---
drivers/net/ipa/gsi.c | 33 +++++++--------------------------
1 file changed, 7 insertions(+), 26 deletions(-)
diff --git a/drivers/net/ipa/gsi.c b/drivers/net/ipa/gsi.c
index 0983a11409f2d..841a946bc286a 100644
--- a/drivers/net/ipa/gsi.c
+++ b/drivers/net/ipa/gsi.c
@@ -718,46 +718,27 @@ static struct gsi_trans *gsi_channel_trans_last(struct gsi_channel *channel)
* before we disabled transmits, so check for that.
*/
if (channel->toward_ipa) {
- /* The last allocated transaction precedes the first free */
- if (trans_info->allocated_id != trans_info->free_id) {
+ /* The last allocated, committed, or pending transaction
+ * precedes the first free transaction.
+ */
+ if (trans_info->pending_id != trans_info->free_id) {
trans_id = trans_info->free_id - 1;
trans_index = trans_id % channel->tre_count;
trans = &trans_info->trans[trans_index];
goto done;
}
-
- /* Last committed transaction precedes the first allocated */
- if (trans_info->committed_id != trans_info->allocated_id) {
- trans_id = trans_info->allocated_id - 1;
- trans_index = trans_id % channel->tre_count;
- trans = &trans_info->trans[trans_index];
- goto done;
- }
-
- /* Last pending transaction precedes the first committed */
- if (trans_info->pending_id != trans_info->committed_id) {
- trans_id = trans_info->committed_id - 1;
- trans_index = trans_id % channel->tre_count;
- trans = &trans_info->trans[trans_index];
- goto done;
- }
}
/* Otherwise (TX or RX) we want to wait for anything that
* has completed, or has been polled but not released yet.
*
- * The last pending transaction precedes the first committed.
+ * The last completed or polled transaction precedes the
+ * first pending transaction.
*/
- if (trans_info->completed_id != trans_info->pending_id) {
+ if (trans_info->polled_id != trans_info->pending_id) {
trans_id = trans_info->pending_id - 1;
trans_index = trans_id % channel->tre_count;
trans = &trans_info->trans[trans_index];
- goto done;
- }
- if (trans_info->polled_id != trans_info->completed_id) {
- trans_id = trans_info->completed_id - 1;
- trans_index = trans_id % channel->tre_count;
- trans = &trans_info->trans[trans_index];
} else {
trans = NULL;
}
--
2.34.1
Hello:
This series was applied to netdev/net-next.git (master)
by David S. Miller <[email protected]>:
On Fri, 2 Sep 2022 16:02:12 -0500 you wrote:
> A previous group of patches added ID fields to track the state of
> transactions:
> https://lore.kernel.org/netdev/[email protected]
>
> This series starts using those IDs instead of the lists used
> previously. Most of this series involves reworking the function
> that determines which transaction is the "last", which determines
> when a channel has been quiesed. The last patch is mainly used to
> prove that the new index method of tracking transaction state is
> equivalent to the previous use of lists.
>
> [...]
Here is the summary with links:
- [net-next,1/6] net: ipa: rework last transaction determination
https://git.kernel.org/netdev/net-next/c/b2abe33d23cf
- [net-next,2/6] net: ipa: use IDs for last allocated transaction
https://git.kernel.org/netdev/net-next/c/c30623ea0b3a
- [net-next,3/6] net: ipa: use IDs exclusively for last transaction
https://git.kernel.org/netdev/net-next/c/897c0ce665d6
- [net-next,4/6] net: ipa: simplify gsi_channel_trans_last()
https://git.kernel.org/netdev/net-next/c/e68d1d1591fd
- [net-next,5/6] net: ipa: further simplify gsi_channel_trans_last()
https://git.kernel.org/netdev/net-next/c/4601e75596cb
- [net-next,6/6] net: ipa: verify a few more IDs
https://git.kernel.org/netdev/net-next/c/8672bab7eb94
You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html