| Message ID | 20201027161120.5575-6-elder@linaro.org (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: ipa: minor bug fixes | expand |
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?
On 10/27/20 7:14 PM, Willem de Bruijn wrote: > 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. 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? >
On 10/27/20 7:14 PM, Willem de Bruijn wrote: > 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)? . . . Sorry, I missed this comment. Yes, I'm working on support for an upcoming IPA hardware version that has 5 resources of each type. And it is a bug fix, though the bug doesn't bite us (because the maximum number of resources supported is even), so "it turns out to be harmless." -Alex
> >> + /* 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. Makes sense. Can you then call this out more explicitly in the commit message? That MAX here means max count, not max element id.
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; /* 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]; + ipa_resource_config_common(ipa, offset, &resource->limits[4], ylimits); } -static void ipa_resource_config_dst_23(struct ipa *ipa, - const struct ipa_resource_dst *resource) +static void ipa_resource_config_dst(struct ipa *ipa, + const struct ipa_resource_dst *resource) { - u32 offset = IPA_REG_DST_RSRC_GRP_23_RSRC_TYPE_N_OFFSET(resource->type); + u32 group_count = ipa_resource_group_dst_count(ipa->version); + const struct ipa_resource_limits *ylimits; + u32 offset; + + offset = IPA_REG_DST_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); + + if (group_count < 2) + return; + + offset = IPA_REG_DST_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); + + if (group_count < 4) + return; - ipa_resource_config_common(ipa, offset, - &resource->limits[2], &resource->limits[3]); + offset = IPA_REG_DST_RSRC_GRP_45_RSRC_TYPE_N_OFFSET(resource->type); + ylimits = group_count == 5 ? NULL : &resource->limits[5]; + ipa_resource_config_common(ipa, offset, &resource->limits[4], ylimits); } static int @@ -471,15 +492,11 @@ ipa_resource_config(struct ipa *ipa, const struct ipa_resource_data *data) if (!ipa_resource_limits_valid(ipa, data)) return -EINVAL; - for (i = 0; i < data->resource_src_count; i++) { - ipa_resource_config_src_01(ipa, &data->resource_src[i]); - ipa_resource_config_src_23(ipa, &data->resource_src[i]); - } + for (i = 0; i < data->resource_src_count; i++) + ipa_resource_config_src(ipa, data->resource_src); - for (i = 0; i < data->resource_dst_count; i++) { - ipa_resource_config_dst_01(ipa, &data->resource_dst[i]); - ipa_resource_config_dst_23(ipa, &data->resource_dst[i]); - } + for (i = 0; i < data->resource_dst_count; i++) + ipa_resource_config_dst(ipa, data->resource_dst); return 0; }
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, 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(-)