Date:   Wed, 28 Oct 2020 07:45:29 -0500
From:   Alex Elder <elder@...aro.org>
To:     Willem de Bruijn <willemdebruijn.kernel@...il.com>
Cc:     David Miller <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>, evgreen@...omium.org,
        Subash Abhinov Kasiviswanathan <subashab@...eaurora.org>,
        cpratapa@...eaurora.org, bjorn.andersson@...aro.org,
        Network Development <netdev@...r.kernel.org>,
        linux-kernel <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH net 5/5] net: ipa: avoid going past end of resource group
 array

On 10/27/20 7:14 PM, Willem de Bruijn wrote:
> On Tue, Oct 27, 2020 at 12:38 PM Alex Elder <elder@...aro.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@...aro.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.

I agree with your point, but the max here represents something different
from what you're expecting.

For a given resource type (source or destination) there is some fixed
number (count) of resources available based on the version of SoC.
The *driver* can handle any number of them up to the maximum number
(max) for any SoC it supports.  In that respect, it *does* represent
the largest value in an inclusive range.

I could change the suffix to something like SRC_COUNT_MAX, but in
general the symbol names are longer than I like in this driver and
I'm trying to shorten them where possible.

If you still want me to change this, please say so and tell me what
you suggest I use instead.

Your observation below about using ">" rather than ">=" is correct,
and applies here as well.  That error might have led you to make
your comment about "max" representing an inclusive maximum.

I will send out a new version of the series to fix that.  I'll
wait until later today to give some time for more feedback before
I prepare them.

Thanks a lot for reviewing this.

					-Alex

>>         /* 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?
> 

Powered by blists - more mailing lists