Re: [PATCH net 5/5] net: ipa: avoid going past end of resource group array

[Willem de Bruijn]

On Tue, Oct 27, 2020 at 12:38 PM Alex Elder <elder@xxxxxxxxxx> 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@xxxxxxxxxx>
> ---
>  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?