* [PATCH] coresight: etm4x: fix mis-usage of nr_resource in sysfs interface @ 2020-09-02 7:52 Jonathan Zhou 2020-09-03 17:22 ` Mathieu Poirier 0 siblings, 1 reply; 5+ messages in thread From: Jonathan Zhou @ 2020-09-02 7:52 UTC (permalink / raw) To: linux-arm-kernel Cc: Mathieu Poirier, Suzuki K Poulose, lizixian, Shaokun Zhang, Jonathan Zhou, Mike Leach The member @nr_resource represents how many resource selector pairs, and the pair 0 is always implemented and reserved. So let's multiply by 2 when displaying the implemented resource seletors and resetting the seletor configuration . And also update the validation of the input @idx. Cc: Mathieu Poirier <mathieu.poirier@linaro.org> Cc: Suzuki K Poulose <suzuki.poulose@arm.com> Cc: Mike Leach <mike.leach@linaro.org> Cc: Shaokun Zhang <zhangshaokun@hisilicon.com> Cc: lizixian@hisilicon.com Signed-off-by: Jonathan Zhou <jonathan.zhouwen@huawei.com> --- drivers/hwtracing/coresight/coresight-etm4x-sysfs.c | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/drivers/hwtracing/coresight/coresight-etm4x-sysfs.c b/drivers/hwtracing/coresight/coresight-etm4x-sysfs.c index b673e738bc9a..635b93d20efa 100644 --- a/drivers/hwtracing/coresight/coresight-etm4x-sysfs.c +++ b/drivers/hwtracing/coresight/coresight-etm4x-sysfs.c @@ -143,8 +143,10 @@ static ssize_t nr_resource_show(struct device *dev, { unsigned long val; struct etmv4_drvdata *drvdata = dev_get_drvdata(dev->parent); - - val = drvdata->nr_resource; + /* @nr_resource represents how many resource selector pairs, + * hence multiply by 2. + */ + val = 2 * drvdata->nr_resource; return scnprintf(buf, PAGE_SIZE, "%#lx\n", val); } static DEVICE_ATTR_RO(nr_resource); @@ -236,7 +238,7 @@ static ssize_t reset_store(struct device *dev, } config->res_idx = 0x0; - for (i = 0; i < drvdata->nr_resource; i++) + for (i = 2; i < 2 * drvdata->nr_resource; i++) config->res_ctrl[i] = 0x0; config->ss_idx = 0x0; @@ -1663,8 +1665,11 @@ static ssize_t res_idx_store(struct device *dev, if (kstrtoul(buf, 16, &val)) return -EINVAL; - /* Resource selector pair 0 is always implemented and reserved */ - if ((val == 0) || (val >= drvdata->nr_resource)) + /* + * Resource selector pair 0 is always implemented and reserved, + * namely an idx with 0 and 1 is illegal. + */ + if ((val == 0) || (val == 1) || (val >= 2 * drvdata->nr_resource)) return -EINVAL; /* -- 1.9.1 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] coresight: etm4x: fix mis-usage of nr_resource in sysfs interface 2020-09-02 7:52 [PATCH] coresight: etm4x: fix mis-usage of nr_resource in sysfs interface Jonathan Zhou @ 2020-09-03 17:22 ` Mathieu Poirier 2020-09-04 10:08 ` Zhouwen (Jonathan, Turing) 0 siblings, 1 reply; 5+ messages in thread From: Mathieu Poirier @ 2020-09-03 17:22 UTC (permalink / raw) To: Jonathan Zhou Cc: Shaokun Zhang, lizixian, Mike Leach, linux-arm-kernel, Suzuki K Poulose On Wed, Sep 02, 2020 at 03:52:56PM +0800, Jonathan Zhou wrote: > The member @nr_resource represents how many resource selector pairs, > and the pair 0 is always implemented and reserved. > So let's multiply by 2 when displaying the implemented resource seletors > and resetting the seletor configuration . > And also update the validation of the input @idx. > > Cc: Mathieu Poirier <mathieu.poirier@linaro.org> > Cc: Suzuki K Poulose <suzuki.poulose@arm.com> > Cc: Mike Leach <mike.leach@linaro.org> > Cc: Shaokun Zhang <zhangshaokun@hisilicon.com> > Cc: lizixian@hisilicon.com > Signed-off-by: Jonathan Zhou <jonathan.zhouwen@huawei.com> > --- > drivers/hwtracing/coresight/coresight-etm4x-sysfs.c | 15 ++++++++++----- > 1 file changed, 10 insertions(+), 5 deletions(-) > > diff --git a/drivers/hwtracing/coresight/coresight-etm4x-sysfs.c b/drivers/hwtracing/coresight/coresight-etm4x-sysfs.c > index b673e738bc9a..635b93d20efa 100644 > --- a/drivers/hwtracing/coresight/coresight-etm4x-sysfs.c > +++ b/drivers/hwtracing/coresight/coresight-etm4x-sysfs.c > @@ -143,8 +143,10 @@ static ssize_t nr_resource_show(struct device *dev, > { > unsigned long val; > struct etmv4_drvdata *drvdata = dev_get_drvdata(dev->parent); > - > - val = drvdata->nr_resource; > + /* @nr_resource represents how many resource selector pairs, > + * hence multiply by 2. Your comment said it correctly, @nr_resource present the number of _pairs_. As such the code is correct and there is no need to multiply by 2. > + */ > + val = 2 * drvdata->nr_resource; > return scnprintf(buf, PAGE_SIZE, "%#lx\n", val); > } > static DEVICE_ATTR_RO(nr_resource); > @@ -236,7 +238,7 @@ static ssize_t reset_store(struct device *dev, > } > > config->res_idx = 0x0; > - for (i = 0; i < drvdata->nr_resource; i++) > + for (i = 2; i < 2 * drvdata->nr_resource; i++) > config->res_ctrl[i] = 0x0; This hunk is correct - please send a patch for it. > > config->ss_idx = 0x0; > @@ -1663,8 +1665,11 @@ static ssize_t res_idx_store(struct device *dev, > > if (kstrtoul(buf, 16, &val)) > return -EINVAL; > - /* Resource selector pair 0 is always implemented and reserved */ > - if ((val == 0) || (val >= drvdata->nr_resource)) > + /* > + * Resource selector pair 0 is always implemented and reserved, > + * namely an idx with 0 and 1 is illegal. > + */ > + if ((val == 0) || (val == 1) || (val >= 2 * drvdata->nr_resource)) > return -EINVAL; The index, i.e ->res_idx, represent pairs an not individual resource. Thanks, Mathieu > > /* > -- > 1.9.1 > _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] coresight: etm4x: fix mis-usage of nr_resource in sysfs interface 2020-09-03 17:22 ` Mathieu Poirier @ 2020-09-04 10:08 ` Zhouwen (Jonathan, Turing) 2020-09-08 17:35 ` Mathieu Poirier 0 siblings, 1 reply; 5+ messages in thread From: Zhouwen (Jonathan, Turing) @ 2020-09-04 10:08 UTC (permalink / raw) To: Mathieu Poirier Cc: Zhangshaokun, Lizixian, Mike Leach, linux-arm-kernel@lists.infradead.org, Suzuki K Poulose Hi Mathieu, Thanks for your review. 在 2020/9/4 1:22, Mathieu Poirier 写道: > On Wed, Sep 02, 2020 at 03:52:56PM +0800, Jonathan Zhou wrote: >> The member @nr_resource represents how many resource selector pairs, >> and the pair 0 is always implemented and reserved. >> So let's multiply by 2 when displaying the implemented resource seletors >> and resetting the seletor configuration . >> And also update the validation of the input @idx. >> >> Cc: Mathieu Poirier <mathieu.poirier@linaro.org> >> Cc: Suzuki K Poulose <suzuki.poulose@arm.com> >> Cc: Mike Leach <mike.leach@linaro.org> >> Cc: Shaokun Zhang <zhangshaokun@hisilicon.com> >> Cc: lizixian@hisilicon.com >> Signed-off-by: Jonathan Zhou <jonathan.zhouwen@huawei.com> >> --- >> drivers/hwtracing/coresight/coresight-etm4x-sysfs.c | 15 ++++++++++----- >> 1 file changed, 10 insertions(+), 5 deletions(-) >> >> diff --git a/drivers/hwtracing/coresight/coresight-etm4x-sysfs.c b/drivers/hwtracing/coresight/coresight-etm4x-sysfs.c >> index b673e738bc9a..635b93d20efa 100644 >> --- a/drivers/hwtracing/coresight/coresight-etm4x-sysfs.c >> +++ b/drivers/hwtracing/coresight/coresight-etm4x-sysfs.c >> @@ -143,8 +143,10 @@ static ssize_t nr_resource_show(struct device *dev, >> { >> unsigned long val; >> struct etmv4_drvdata *drvdata = dev_get_drvdata(dev->parent); >> - >> - val = drvdata->nr_resource; >> + /* @nr_resource represents how many resource selector pairs, >> + * hence multiply by 2. > > Your comment said it correctly, @nr_resource present the number of _pairs_. As > such the code is correct and there is no need to multiply by 2. > Sorry for not mentioned the reason of modified this. From the function @res_ctrl_store, we can see that only one individual resource selector can be accessed via the ->res_idx. Therefore, it may be better to show the number of individual resource selectors. >> + */ >> + val = 2 * drvdata->nr_resource; >> return scnprintf(buf, PAGE_SIZE, "%#lx\n", val); >> } >> static DEVICE_ATTR_RO(nr_resource); >> @@ -236,7 +238,7 @@ static ssize_t reset_store(struct device *dev, >> } >> >> config->res_idx = 0x0; >> - for (i = 0; i < drvdata->nr_resource; i++) >> + for (i = 2; i < 2 * drvdata->nr_resource; i++) >> config->res_ctrl[i] = 0x0; > > This hunk is correct - please send a patch for it. Ok. > >> >> config->ss_idx = 0x0; >> @@ -1663,8 +1665,11 @@ static ssize_t res_idx_store(struct device *dev, >> >> if (kstrtoul(buf, 16, &val)) >> return -EINVAL; >> - /* Resource selector pair 0 is always implemented and reserved */ >> - if ((val == 0) || (val >= drvdata->nr_resource)) >> + /* >> + * Resource selector pair 0 is always implemented and reserved, >> + * namely an idx with 0 and 1 is illegal. >> + */ >> + if ((val == 0) || (val == 1) || (val >= 2 * drvdata->nr_resource)) >> return -EINVAL; > > The index, i.e ->res_idx, represent pairs an not individual resource. > > Thanks, > Mathieu > If we want all of the ->res_ctrl accessable, the ->res_idx should represent individual resource. That's the reason. Best regards. Jonathan. >> >> /* >> -- >> 1.9.1 >> > . > _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] coresight: etm4x: fix mis-usage of nr_resource in sysfs interface 2020-09-04 10:08 ` Zhouwen (Jonathan, Turing) @ 2020-09-08 17:35 ` Mathieu Poirier 2020-09-09 6:19 ` Jonathan Zhou 0 siblings, 1 reply; 5+ messages in thread From: Mathieu Poirier @ 2020-09-08 17:35 UTC (permalink / raw) To: Zhouwen (Jonathan, Turing) Cc: Zhangshaokun, Lizixian, Mike Leach, linux-arm-kernel@lists.infradead.org, Suzuki K Poulose On Fri, Sep 04, 2020 at 06:08:46PM +0800, Zhouwen (Jonathan, Turing) wrote: > > Hi Mathieu, > > Thanks for your review. > > 在 2020/9/4 1:22, Mathieu Poirier 写道: > > On Wed, Sep 02, 2020 at 03:52:56PM +0800, Jonathan Zhou wrote: > > > The member @nr_resource represents how many resource selector pairs, > > > and the pair 0 is always implemented and reserved. > > > So let's multiply by 2 when displaying the implemented resource seletors > > > and resetting the seletor configuration . > > > And also update the validation of the input @idx. > > > > > > Cc: Mathieu Poirier <mathieu.poirier@linaro.org> > > > Cc: Suzuki K Poulose <suzuki.poulose@arm.com> > > > Cc: Mike Leach <mike.leach@linaro.org> > > > Cc: Shaokun Zhang <zhangshaokun@hisilicon.com> > > > Cc: lizixian@hisilicon.com > > > Signed-off-by: Jonathan Zhou <jonathan.zhouwen@huawei.com> > > > --- > > > drivers/hwtracing/coresight/coresight-etm4x-sysfs.c | 15 ++++++++++----- > > > 1 file changed, 10 insertions(+), 5 deletions(-) > > > > > > diff --git a/drivers/hwtracing/coresight/coresight-etm4x-sysfs.c b/drivers/hwtracing/coresight/coresight-etm4x-sysfs.c > > > index b673e738bc9a..635b93d20efa 100644 > > > --- a/drivers/hwtracing/coresight/coresight-etm4x-sysfs.c > > > +++ b/drivers/hwtracing/coresight/coresight-etm4x-sysfs.c > > > @@ -143,8 +143,10 @@ static ssize_t nr_resource_show(struct device *dev, > > > { > > > unsigned long val; > > > struct etmv4_drvdata *drvdata = dev_get_drvdata(dev->parent); > > > - > > > - val = drvdata->nr_resource; > > > + /* @nr_resource represents how many resource selector pairs, > > > + * hence multiply by 2. > > > > Your comment said it correctly, @nr_resource present the number of _pairs_. As > > such the code is correct and there is no need to multiply by 2. > > > > Sorry for not mentioned the reason of modified this. From the function > @res_ctrl_store, we can see that only one individual resource selector can > be accessed via the ->res_idx. Therefore, it may be better to show the > number of individual resource selectors. The end result does not change, i.e this entry represent the number of pairs available on the system. > > > > + */ > > > + val = 2 * drvdata->nr_resource; > > > return scnprintf(buf, PAGE_SIZE, "%#lx\n", val); > > > } > > > static DEVICE_ATTR_RO(nr_resource); > > > @@ -236,7 +238,7 @@ static ssize_t reset_store(struct device *dev, > > > } > > > config->res_idx = 0x0; > > > - for (i = 0; i < drvdata->nr_resource; i++) > > > + for (i = 2; i < 2 * drvdata->nr_resource; i++) > > > config->res_ctrl[i] = 0x0; > > > > This hunk is correct - please send a patch for it. > > Ok. > > > > > > config->ss_idx = 0x0; > > > @@ -1663,8 +1665,11 @@ static ssize_t res_idx_store(struct device *dev, > > > if (kstrtoul(buf, 16, &val)) > > > return -EINVAL; > > > - /* Resource selector pair 0 is always implemented and reserved */ > > > - if ((val == 0) || (val >= drvdata->nr_resource)) > > > + /* > > > + * Resource selector pair 0 is always implemented and reserved, > > > + * namely an idx with 0 and 1 is illegal. > > > + */ > > > + if ((val == 0) || (val == 1) || (val >= 2 * drvdata->nr_resource)) > > > return -EINVAL; > > > > The index, i.e ->res_idx, represent pairs an not individual resource. > > > > Thanks, > > Mathieu > > > > If we want all of the ->res_ctrl accessable, the ->res_idx should represent > individual resource. That's the reason. I see your point now. I think the above condition should be simplified: if ((val < 2) || (val >= 2 * drvdata->nr_resource)) > > Best regards. > > Jonathan. > > > > /* > > > -- > > > 1.9.1 > > > > > . > > _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] coresight: etm4x: fix mis-usage of nr_resource in sysfs interface 2020-09-08 17:35 ` Mathieu Poirier @ 2020-09-09 6:19 ` Jonathan Zhou 0 siblings, 0 replies; 5+ messages in thread From: Jonathan Zhou @ 2020-09-09 6:19 UTC (permalink / raw) To: Mathieu Poirier Cc: Zhangshaokun, Lizixian, Mike Leach, linux-arm-kernel@lists.infradead.org, Suzuki K Poulose Hi Mathieu, Thanks for reply. On 09/09/2020 01:35, Mathieu Poirier wrote: > On Fri, Sep 04, 2020 at 06:08:46PM +0800, Zhouwen (Jonathan, Turing) wrote: >> >> Hi Mathieu, >> >> Thanks for your review. >> >> 在 2020/9/4 1:22, Mathieu Poirier 写道: >>> On Wed, Sep 02, 2020 at 03:52:56PM +0800, Jonathan Zhou wrote: >>>> The member @nr_resource represents how many resource selector pairs, >>>> and the pair 0 is always implemented and reserved. >>>> So let's multiply by 2 when displaying the implemented resource seletors >>>> and resetting the seletor configuration . >>>> And also update the validation of the input @idx. >>>> >>>> Cc: Mathieu Poirier <mathieu.poirier@linaro.org> >>>> Cc: Suzuki K Poulose <suzuki.poulose@arm.com> >>>> Cc: Mike Leach <mike.leach@linaro.org> >>>> Cc: Shaokun Zhang <zhangshaokun@hisilicon.com> >>>> Cc: lizixian@hisilicon.com >>>> Signed-off-by: Jonathan Zhou <jonathan.zhouwen@huawei.com> >>>> --- >>>> drivers/hwtracing/coresight/coresight-etm4x-sysfs.c | 15 ++++++++++----- >>>> 1 file changed, 10 insertions(+), 5 deletions(-) >>>> >>>> diff --git a/drivers/hwtracing/coresight/coresight-etm4x-sysfs.c b/drivers/hwtracing/coresight/coresight-etm4x-sysfs.c >>>> index b673e738bc9a..635b93d20efa 100644 >>>> --- a/drivers/hwtracing/coresight/coresight-etm4x-sysfs.c >>>> +++ b/drivers/hwtracing/coresight/coresight-etm4x-sysfs.c >>>> @@ -143,8 +143,10 @@ static ssize_t nr_resource_show(struct device *dev, >>>> { >>>> unsigned long val; >>>> struct etmv4_drvdata *drvdata = dev_get_drvdata(dev->parent); >>>> - >>>> - val = drvdata->nr_resource; >>>> + /* @nr_resource represents how many resource selector pairs, >>>> + * hence multiply by 2. >>> >>> Your comment said it correctly, @nr_resource present the number of _pairs_. As >>> such the code is correct and there is no need to multiply by 2. >>> >> >> Sorry for not mentioned the reason of modified this. From the function >> @res_ctrl_store, we can see that only one individual resource selector can >> be accessed via the ->res_idx. Therefore, it may be better to show the >> number of individual resource selectors. > > The end result does not change, i.e this entry represent the number of pairs > available on the system. >It's all right. I will keep it no change. >> >>>> + */ >>>> + val = 2 * drvdata->nr_resource; >>>> return scnprintf(buf, PAGE_SIZE, "%#lx\n", val); >>>> } >>>> static DEVICE_ATTR_RO(nr_resource); >>>> @@ -236,7 +238,7 @@ static ssize_t reset_store(struct device *dev, >>>> } >>>> config->res_idx = 0x0; >>>> - for (i = 0; i < drvdata->nr_resource; i++) >>>> + for (i = 2; i < 2 * drvdata->nr_resource; i++) >>>> config->res_ctrl[i] = 0x0; >>> >>> This hunk is correct - please send a patch for it. >> >> Ok. >> >>> >>>> config->ss_idx = 0x0; >>>> @@ -1663,8 +1665,11 @@ static ssize_t res_idx_store(struct device *dev, >>>> if (kstrtoul(buf, 16, &val)) >>>> return -EINVAL; >>>> - /* Resource selector pair 0 is always implemented and reserved */ >>>> - if ((val == 0) || (val >= drvdata->nr_resource)) >>>> + /* >>>> + * Resource selector pair 0 is always implemented and reserved, >>>> + * namely an idx with 0 and 1 is illegal. >>>> + */ >>>> + if ((val == 0) || (val == 1) || (val >= 2 * drvdata->nr_resource)) >>>> return -EINVAL; >>> >>> The index, i.e ->res_idx, represent pairs an not individual resource. >>> >>> Thanks, >>> Mathieu >>> >> >> If we want all of the ->res_ctrl accessable, the ->res_idx should represent >> individual resource. That's the reason. > > I see your point now. I think the above condition should be simplified: > > if ((val < 2) || (val >= 2 * drvdata->nr_resource)) > You are right. I will do this. Thanks. >> >> Best regards. >> >> Jonathan. >> >>>> /* >>>> -- >>>> 1.9.1 >>>> >>> . >>> > . > _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2020-09-09 6:21 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2020-09-02 7:52 [PATCH] coresight: etm4x: fix mis-usage of nr_resource in sysfs interface Jonathan Zhou 2020-09-03 17:22 ` Mathieu Poirier 2020-09-04 10:08 ` Zhouwen (Jonathan, Turing) 2020-09-08 17:35 ` Mathieu Poirier 2020-09-09 6:19 ` Jonathan Zhou
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox