This series contains some minor code improvements.
The first patch verifies that the configuration is compatible with a
recently-defined limit. The second and third rename two fields so
they better reflect their use in the code. The next gets rid of an
empty function by reworking its only caller.
The last two begin to remove the assumption that an event ring is
associated with a single channel. Eventually we'll support having
multiple channels share an event ring but some more needs to be done
before that can happen.
-Alex
Alex Elder (6):
net: ipa: verify command channel TLV count
net: ipa: rename channel->tlv_count
net: ipa: rename endpoint->trans_tre_max
net: ipa: simplify endpoint transaction completion
net: ipa: determine channel from event
net: ipa: derive channel from transaction
drivers/net/ipa/gsi.c | 107 ++++++++++++++++++---------------
drivers/net/ipa/gsi.h | 11 +---
drivers/net/ipa/gsi_private.h | 12 ++--
drivers/net/ipa/gsi_trans.c | 10 +--
drivers/net/ipa/ipa_cmd.c | 8 +--
drivers/net/ipa/ipa_endpoint.c | 27 +++------
drivers/net/ipa/ipa_endpoint.h | 4 +-
7 files changed, 80 insertions(+), 99 deletions(-)
--
2.34.1
In commit 8797972afff3d ("net: ipa: remove command info pool"), the
maximum number of IPA commands that would be sent in a single
transaction was defined. That number can't exceed the size of the
TLV FIFO on the command channel, and we can check that at runtime.
To add this check, pass a new flag to gsi_channel_data_valid() to
indicate the channel being checked is being used for IPA commands.
Knowing that we can also verify the channel direction is correct.
Use a new local variable that refers to the command-specific portion
of the data being checked.
Signed-off-by: Alex Elder <[email protected]>
---
drivers/net/ipa/gsi.c | 39 +++++++++++++++++++++++++++------------
1 file changed, 27 insertions(+), 12 deletions(-)
diff --git a/drivers/net/ipa/gsi.c b/drivers/net/ipa/gsi.c
index 9cfe84319ee4d..65ed5a697577e 100644
--- a/drivers/net/ipa/gsi.c
+++ b/drivers/net/ipa/gsi.c
@@ -2001,9 +2001,10 @@ static void gsi_channel_evt_ring_exit(struct gsi_channel *channel)
gsi_evt_ring_id_free(gsi, evt_ring_id);
}
-static bool gsi_channel_data_valid(struct gsi *gsi,
+static bool gsi_channel_data_valid(struct gsi *gsi, bool command,
const struct ipa_gsi_endpoint_data *data)
{
+ const struct gsi_channel_data *channel_data;
u32 channel_id = data->channel_id;
struct device *dev = gsi->dev;
@@ -2019,10 +2020,24 @@ static bool gsi_channel_data_valid(struct gsi *gsi,
return false;
}
- if (!data->channel.tlv_count ||
- data->channel.tlv_count > GSI_TLV_MAX) {
+ if (command && !data->toward_ipa) {
+ dev_err(dev, "command channel %u is not TX\n", channel_id);
+ return false;
+ }
+
+ channel_data = &data->channel;
+
+ if (!channel_data->tlv_count ||
+ channel_data->tlv_count > GSI_TLV_MAX) {
dev_err(dev, "channel %u bad tlv_count %u; must be 1..%u\n",
- channel_id, data->channel.tlv_count, GSI_TLV_MAX);
+ channel_id, channel_data->tlv_count, GSI_TLV_MAX);
+ return false;
+ }
+
+ if (command && IPA_COMMAND_TRANS_TRE_MAX > channel_data->tlv_count) {
+ dev_err(dev, "command TRE max too big for channel %u (%u > %u)\n",
+ channel_id, IPA_COMMAND_TRANS_TRE_MAX,
+ channel_data->tlv_count);
return false;
}
@@ -2031,22 +2046,22 @@ static bool gsi_channel_data_valid(struct gsi *gsi,
* gsi_channel_tre_max() is computed, tre_count has to be almost
* twice the TLV FIFO size to satisfy this requirement.
*/
- if (data->channel.tre_count < 2 * data->channel.tlv_count - 1) {
+ if (channel_data->tre_count < 2 * channel_data->tlv_count - 1) {
dev_err(dev, "channel %u TLV count %u exceeds TRE count %u\n",
- channel_id, data->channel.tlv_count,
- data->channel.tre_count);
+ channel_id, channel_data->tlv_count,
+ channel_data->tre_count);
return false;
}
- if (!is_power_of_2(data->channel.tre_count)) {
+ if (!is_power_of_2(channel_data->tre_count)) {
dev_err(dev, "channel %u bad tre_count %u; not power of 2\n",
- channel_id, data->channel.tre_count);
+ channel_id, channel_data->tre_count);
return false;
}
- if (!is_power_of_2(data->channel.event_count)) {
+ if (!is_power_of_2(channel_data->event_count)) {
dev_err(dev, "channel %u bad event_count %u; not power of 2\n",
- channel_id, data->channel.event_count);
+ channel_id, channel_data->event_count);
return false;
}
@@ -2062,7 +2077,7 @@ static int gsi_channel_init_one(struct gsi *gsi,
u32 tre_count;
int ret;
- if (!gsi_channel_data_valid(gsi, data))
+ if (!gsi_channel_data_valid(gsi, command, data))
return -EINVAL;
/* Worst case we need an event for every outstanding TRE */
--
2.34.1
Each event in an event ring describes the TRE whose completion
caused the event. Currently, every event ring is dedicated to a
single channel, so the channel is easily derived from the event
ring.
An event ring can actually be shared by more than one channel
though, and to distinguish events for one channel from another, the
event structure contains a field indicating which channel the event
is associated with.
In gsi_event_trans(), use the channel ID in an event to determine
which channel the event is for. This makes the channel pointer now
passed to that function irrelevant; pass the GSI pointer to that
function instead.
And although it shouldn't happen, warn if an event arrives that
records a channel ID that's not in use, or if the event does not
have a transaction associated with it.
Signed-off-by: Alex Elder <[email protected]>
---
drivers/net/ipa/gsi.c | 26 +++++++++++++++++++++-----
1 file changed, 21 insertions(+), 5 deletions(-)
diff --git a/drivers/net/ipa/gsi.c b/drivers/net/ipa/gsi.c
index b1acc7d36b23b..64417668b8a9a 100644
--- a/drivers/net/ipa/gsi.c
+++ b/drivers/net/ipa/gsi.c
@@ -1327,17 +1327,29 @@ static int gsi_irq_init(struct gsi *gsi, struct platform_device *pdev)
}
/* Return the transaction associated with a transfer completion event */
-static struct gsi_trans *gsi_event_trans(struct gsi_channel *channel,
- struct gsi_event *event)
+static struct gsi_trans *
+gsi_event_trans(struct gsi *gsi, struct gsi_event *event)
{
+ u32 channel_id = event->chid;
+ struct gsi_channel *channel;
+ struct gsi_trans *trans;
u32 tre_offset;
u32 tre_index;
+ channel = &gsi->channel[channel_id];
+ if (WARN(!channel->gsi, "event has bad channel %u\n", channel_id))
+ return NULL;
+
/* Event xfer_ptr records the TRE it's associated with */
tre_offset = lower_32_bits(le64_to_cpu(event->xfer_ptr));
tre_index = gsi_ring_index(&channel->tre_ring, tre_offset);
- return gsi_channel_trans_mapped(channel, tre_index);
+ trans = gsi_channel_trans_mapped(channel, tre_index);
+
+ if (WARN(!trans, "channel %u event with no transaction\n", channel_id))
+ return NULL;
+
+ return trans;
}
/**
@@ -1381,7 +1393,9 @@ static void gsi_evt_ring_rx_update(struct gsi_evt_ring *evt_ring, u32 index)
*/
old_index = ring->index;
event = gsi_ring_virt(ring, old_index);
- trans = gsi_event_trans(channel, event);
+ trans = gsi_event_trans(channel->gsi, event);
+ if (!trans)
+ return;
/* Compute the number of events to process before we wrap,
* and determine when we'll be done processing events.
@@ -1493,7 +1507,9 @@ static struct gsi_trans *gsi_channel_update(struct gsi_channel *channel)
return NULL;
/* Get the transaction for the latest completed event. */
- trans = gsi_event_trans(channel, gsi_ring_virt(ring, index - 1));
+ trans = gsi_event_trans(gsi, gsi_ring_virt(ring, index - 1));
+ if (!trans)
+ return NULL;
/* For RX channels, update each completed transaction with the number
* of bytes that were actually received. For TX channels, report
--
2.34.1
The trans_tre_max field of the IPA endpoint structure is only used
to limit the number of fragments allowed for an SKB being prepared
for transmission. Recognizing that, rename the field skb_frag_max,
and reduce its value by 1.
Signed-off-by: Alex Elder <[email protected]>
---
drivers/net/ipa/ipa_endpoint.c | 4 ++--
drivers/net/ipa/ipa_endpoint.h | 4 ++--
2 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/net/ipa/ipa_endpoint.c b/drivers/net/ipa/ipa_endpoint.c
index 57507a109269b..86ef91f83eb68 100644
--- a/drivers/net/ipa/ipa_endpoint.c
+++ b/drivers/net/ipa/ipa_endpoint.c
@@ -1020,7 +1020,7 @@ int ipa_endpoint_skb_tx(struct ipa_endpoint *endpoint, struct sk_buff *skb)
* If not, see if we can linearize it before giving up.
*/
nr_frags = skb_shinfo(skb)->nr_frags;
- if (1 + nr_frags > endpoint->trans_tre_max) {
+ if (nr_frags > endpoint->skb_frag_max) {
if (skb_linearize(skb))
return -E2BIG;
nr_frags = 0;
@@ -1721,7 +1721,7 @@ static void ipa_endpoint_setup_one(struct ipa_endpoint *endpoint)
if (endpoint->ee_id != GSI_EE_AP)
return;
- endpoint->trans_tre_max = gsi->channel[channel_id].trans_tre_max;
+ endpoint->skb_frag_max = gsi->channel[channel_id].trans_tre_max - 1;
if (!endpoint->toward_ipa) {
/* RX transactions require a single TRE, so the maximum
* backlog is the same as the maximum outstanding TREs.
diff --git a/drivers/net/ipa/ipa_endpoint.h b/drivers/net/ipa/ipa_endpoint.h
index 01790c60bee8d..28e0a7386fd72 100644
--- a/drivers/net/ipa/ipa_endpoint.h
+++ b/drivers/net/ipa/ipa_endpoint.h
@@ -142,7 +142,7 @@ enum ipa_replenish_flag {
* @endpoint_id: IPA endpoint number
* @toward_ipa: Endpoint direction (true = TX, false = RX)
* @config: Default endpoint configuration
- * @trans_tre_max: Maximum number of TRE descriptors per transaction
+ * @skb_frag_max: Maximum allowed number of TX SKB fragments
* @evt_ring_id: GSI event ring used by the endpoint
* @netdev: Network device pointer, if endpoint uses one
* @replenish_flags: Replenishing state flags
@@ -157,7 +157,7 @@ struct ipa_endpoint {
bool toward_ipa;
struct ipa_endpoint_config config;
- u32 trans_tre_max;
+ u32 skb_frag_max; /* Used for netdev TX only */
u32 evt_ring_id;
/* Net device this endpoint is associated with, if any */
--
2.34.1
When a GSI transaction completes, ipa_endpoint_trans_complete() is
eventually called. That handles TX and RX completions separately,
but ipa_endpoint_tx_complete() is a no-op.
Instead, have ipa_endpoint_trans_complete() return immediately for a
TX transaction, and incorporate code from ipa_endpoint_rx_complete()
to handle RX transactions.
Signed-off-by: Alex Elder <[email protected]>
---
drivers/net/ipa/ipa_endpoint.c | 23 +++++------------------
1 file changed, 5 insertions(+), 18 deletions(-)
diff --git a/drivers/net/ipa/ipa_endpoint.c b/drivers/net/ipa/ipa_endpoint.c
index 86ef91f83eb68..66d2bfdf9e423 100644
--- a/drivers/net/ipa/ipa_endpoint.c
+++ b/drivers/net/ipa/ipa_endpoint.c
@@ -1368,18 +1368,14 @@ static void ipa_endpoint_status_parse(struct ipa_endpoint *endpoint,
}
}
-/* Complete a TX transaction, command or from ipa_endpoint_skb_tx() */
-static void ipa_endpoint_tx_complete(struct ipa_endpoint *endpoint,
- struct gsi_trans *trans)
-{
-}
-
-/* Complete transaction initiated in ipa_endpoint_replenish_one() */
-static void ipa_endpoint_rx_complete(struct ipa_endpoint *endpoint,
- struct gsi_trans *trans)
+void ipa_endpoint_trans_complete(struct ipa_endpoint *endpoint,
+ struct gsi_trans *trans)
{
struct page *page;
+ if (endpoint->toward_ipa)
+ return;
+
if (trans->cancelled)
goto done;
@@ -1393,15 +1389,6 @@ static void ipa_endpoint_rx_complete(struct ipa_endpoint *endpoint,
ipa_endpoint_replenish(endpoint);
}
-void ipa_endpoint_trans_complete(struct ipa_endpoint *endpoint,
- struct gsi_trans *trans)
-{
- if (endpoint->toward_ipa)
- ipa_endpoint_tx_complete(endpoint, trans);
- else
- ipa_endpoint_rx_complete(endpoint, trans);
-}
-
void ipa_endpoint_trans_release(struct ipa_endpoint *endpoint,
struct gsi_trans *trans)
{
--
2.34.1
Hello:
This series was applied to netdev/net-next.git (master)
by David S. Miller <[email protected]>:
On Fri, 10 Jun 2022 10:46:09 -0500 you wrote:
> This series contains some minor code improvements.
>
> The first patch verifies that the configuration is compatible with a
> recently-defined limit. The second and third rename two fields so
> they better reflect their use in the code. The next gets rid of an
> empty function by reworking its only caller.
>
> [...]
Here is the summary with links:
- [net-next,1/6] net: ipa: verify command channel TLV count
https://git.kernel.org/netdev/net-next/c/92f78f81ac4d
- [net-next,2/6] net: ipa: rename channel->tlv_count
https://git.kernel.org/netdev/net-next/c/88e03057e4df
- [net-next,3/6] net: ipa: rename endpoint->trans_tre_max
https://git.kernel.org/netdev/net-next/c/317595d2ce77
- [net-next,4/6] net: ipa: simplify endpoint transaction completion
https://git.kernel.org/netdev/net-next/c/983a1a3081bb
- [net-next,5/6] net: ipa: determine channel from event
https://git.kernel.org/netdev/net-next/c/7dd9558feddf
- [net-next,6/6] net: ipa: derive channel from transaction
https://git.kernel.org/netdev/net-next/c/bcec9ecbaf60
You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html