From: Willem de Bruijn <willemdebruijn.kernel@gmail.com>
To: Alex Elder <elder@linaro.org>
Cc: David Miller <davem@davemloft.net>,
Jakub Kicinski <kuba@kernel.org>,
evgreen@chromium.org,
Subash Abhinov Kasiviswanathan <subashab@codeaurora.org>,
cpratapa@codeaurora.org, bjorn.andersson@linaro.org,
Network Development <netdev@vger.kernel.org>,
linux-kernel <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH net 5/5] net: ipa: avoid going past end of resource group array
Date: Tue, 27 Oct 2020 20:14:51 -0400 [thread overview]
Message-ID: <CA+FuTSdGCBG0tZXfPTJqTnV7zRNv2VmuThOydwj080NWw4PU9Q@mail.gmail.com> (raw)
In-Reply-To: <20201027161120.5575-6-elder@linaro.org>
On Tue, Oct 27, 2020 at 12:38 PM Alex Elder <elder@linaro.org> wrote:
>
> The minimum and maximum limits for resources assigned to a given
> resource group are programmed in pairs, with the limits for two
> groups set in a single register.
>
> If the number of supported resource groups is odd, only half of the
> register that defines these limits is valid for the last group; that
> group has no second group in the pair.
>
> Currently we ignore this constraint, and it turns out to be harmless,
If nothing currently calls it with an odd number of registers, is this
a bugfix or a new feature (anticipating future expansion, I guess)?
> but it is not guaranteed to be. This patch addresses that, and adds
> support for programming the 5th resource group's limits.
>
> Rework how the resource group limit registers are programmed by
> having a single function program all group pairs rather than having
> one function program each pair. Add the programming of the 4-5
> resource group pair limits to this function. If a resource group is
> not supported, pass a null pointer to ipa_resource_config_common()
> for that group and have that function write zeroes in that case.
>
> Fixes: cdf2e9419dd91 ("soc: qcom: ipa: main code")
> Signed-off-by: Alex Elder <elder@linaro.org>
> ---
> drivers/net/ipa/ipa_main.c | 89 +++++++++++++++++++++++---------------
> 1 file changed, 53 insertions(+), 36 deletions(-)
>
> diff --git a/drivers/net/ipa/ipa_main.c b/drivers/net/ipa/ipa_main.c
> index 74b1e15ebd6b2..09c8a16d216df 100644
> --- a/drivers/net/ipa/ipa_main.c
> +++ b/drivers/net/ipa/ipa_main.c
> @@ -370,8 +370,11 @@ static bool ipa_resource_limits_valid(struct ipa *ipa,
> u32 i;
> u32 j;
>
> + /* We program at most 6 source or destination resource group limits */
> + BUILD_BUG_ON(IPA_RESOURCE_GROUP_SRC_MAX > 6);
> +
> group_count = ipa_resource_group_src_count(ipa->version);
> - if (!group_count)
> + if (!group_count || group_count >= IPA_RESOURCE_GROUP_SRC_MAX)
> return false;
Perhaps more a comment to the previous patch, but _MAX usually denotes
the end of an inclusive range, here 5. The previous name COUNT better
reflects the number of elements in range [0, 5], which is 6.
> /* Return an error if a non-zero resource limit is specified
> @@ -387,7 +390,7 @@ static bool ipa_resource_limits_valid(struct ipa *ipa,
> }
>
> group_count = ipa_resource_group_dst_count(ipa->version);
> - if (!group_count)
> + if (!group_count || group_count >= IPA_RESOURCE_GROUP_DST_MAX)
> return false;
>
> for (i = 0; i < data->resource_dst_count; i++) {
> @@ -421,46 +424,64 @@ ipa_resource_config_common(struct ipa *ipa, u32 offset,
>
> val = u32_encode_bits(xlimits->min, X_MIN_LIM_FMASK);
> val |= u32_encode_bits(xlimits->max, X_MAX_LIM_FMASK);
> - val |= u32_encode_bits(ylimits->min, Y_MIN_LIM_FMASK);
> - val |= u32_encode_bits(ylimits->max, Y_MAX_LIM_FMASK);
> + if (ylimits) {
> + val |= u32_encode_bits(ylimits->min, Y_MIN_LIM_FMASK);
> + val |= u32_encode_bits(ylimits->max, Y_MAX_LIM_FMASK);
> + }
>
> iowrite32(val, ipa->reg_virt + offset);
> }
>
> -static void ipa_resource_config_src_01(struct ipa *ipa,
> - const struct ipa_resource_src *resource)
> +static void ipa_resource_config_src(struct ipa *ipa,
> + const struct ipa_resource_src *resource)
> {
> - u32 offset = IPA_REG_SRC_RSRC_GRP_01_RSRC_TYPE_N_OFFSET(resource->type);
> + u32 group_count = ipa_resource_group_src_count(ipa->version);
> + const struct ipa_resource_limits *ylimits;
> + u32 offset;
>
> - ipa_resource_config_common(ipa, offset,
> - &resource->limits[0], &resource->limits[1]);
> -}
> + offset = IPA_REG_SRC_RSRC_GRP_01_RSRC_TYPE_N_OFFSET(resource->type);
> + ylimits = group_count == 1 ? NULL : &resource->limits[1];
> + ipa_resource_config_common(ipa, offset, &resource->limits[0], ylimits);
>
> -static void ipa_resource_config_src_23(struct ipa *ipa,
> - const struct ipa_resource_src *resource)
> -{
> - u32 offset = IPA_REG_SRC_RSRC_GRP_23_RSRC_TYPE_N_OFFSET(resource->type);
> + if (group_count < 2)
> + return;
>
> - ipa_resource_config_common(ipa, offset,
> - &resource->limits[2], &resource->limits[3]);
> -}
> + offset = IPA_REG_SRC_RSRC_GRP_23_RSRC_TYPE_N_OFFSET(resource->type);
> + ylimits = group_count == 3 ? NULL : &resource->limits[3];
> + ipa_resource_config_common(ipa, offset, &resource->limits[2], ylimits);
>
> -static void ipa_resource_config_dst_01(struct ipa *ipa,
> - const struct ipa_resource_dst *resource)
> -{
> - u32 offset = IPA_REG_DST_RSRC_GRP_01_RSRC_TYPE_N_OFFSET(resource->type);
> + if (group_count < 4)
> + return;
>
> - ipa_resource_config_common(ipa, offset,
> - &resource->limits[0], &resource->limits[1]);
> + offset = IPA_REG_SRC_RSRC_GRP_45_RSRC_TYPE_N_OFFSET(resource->type);
> + ylimits = group_count == 5 ? NULL : &resource->limits[5];
Due to the check
> + if (!group_count || group_count >= IPA_RESOURCE_GROUP_DST_MAX)
> return false;
above, group_count can never be greater than 5. Should be greater than?
next prev parent reply other threads:[~2020-10-28 1:39 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-10-27 16:11 [PATCH net 0/5] net: ipa: minor bug fixes Alex Elder
2020-10-27 16:11 ` [PATCH net 1/5] net: ipa: assign proper packet context base Alex Elder
2020-10-27 16:11 ` [PATCH net 2/5] net: ipa: fix resource group field mask definition Alex Elder
2020-10-27 16:11 ` [PATCH net 3/5] net: ipa: assign endpoint to a resource group Alex Elder
2020-10-27 16:11 ` [PATCH net 4/5] net: ipa: distinguish between resource group types Alex Elder
2020-10-27 16:11 ` [PATCH net 5/5] net: ipa: avoid going past end of resource group array Alex Elder
2020-10-28 0:14 ` Willem de Bruijn [this message]
2020-10-28 12:45 ` Alex Elder
2020-10-28 13:42 ` Willem de Bruijn
2020-10-28 13:30 ` Alex Elder
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CA+FuTSdGCBG0tZXfPTJqTnV7zRNv2VmuThOydwj080NWw4PU9Q@mail.gmail.com \
--to=willemdebruijn.kernel@gmail.com \
--cc=bjorn.andersson@linaro.org \
--cc=cpratapa@codeaurora.org \
--cc=davem@davemloft.net \
--cc=elder@linaro.org \
--cc=evgreen@chromium.org \
--cc=kuba@kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=netdev@vger.kernel.org \
--cc=subashab@codeaurora.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).