| Message ID | 1599901386-41601-1-git-send-email-zhangshaokun@hisilicon.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drivers/perf: hisi: Fix DEVICE_ATTR style tests warning for later PMU driver | expand |
On Sat, Sep 12, 2020 at 05:03:06PM +0800, Shaokun Zhang wrote: > Since commit 001804689b0d ("checkpatch: add a few DEVICE_ATTR style > tests") has checked DEVICE_ATTR style, let's cleanup the sysfs interface > to get rid of the warning for later HiSilicon uncore PMU drivers. > Otherwise the warning is throwed by checkpatch.pl for new drivers as > follow: > WARNING: Consider renaming function(s) 'hisi_cpumask_sysfs_show' to > 'cpumask_show' [...] > diff --git a/drivers/perf/hisilicon/hisi_uncore_pmu.c b/drivers/perf/hisilicon/hisi_uncore_pmu.c > index 97aff877a4e7..e2612a73edf6 100644 > --- a/drivers/perf/hisilicon/hisi_uncore_pmu.c > +++ b/drivers/perf/hisilicon/hisi_uncore_pmu.c > @@ -54,14 +54,14 @@ EXPORT_SYMBOL_GPL(hisi_event_sysfs_show); > /* > * sysfs cpumask attributes. For uncore PMU, we only have a single CPU to show > */ > -ssize_t hisi_cpumask_sysfs_show(struct device *dev, > - struct device_attribute *attr, char *buf) > +ssize_t cpumask_show(struct device *dev, struct device_attribute *attr, > + char *buf) > { > struct hisi_pmu *hisi_pmu = to_hisi_pmu(dev_get_drvdata(dev)); > > return sprintf(buf, "%d\n", hisi_pmu->on_cpu); > } > -EXPORT_SYMBOL_GPL(hisi_cpumask_sysfs_show); > +EXPORT_SYMBOL_GPL(cpumask_show); This seems like a colossally bad idea to me. Will
Hi Will, 在 2020/9/14 20:29, Will Deacon 写道: > On Sat, Sep 12, 2020 at 05:03:06PM +0800, Shaokun Zhang wrote: >> Since commit 001804689b0d ("checkpatch: add a few DEVICE_ATTR style >> tests") has checked DEVICE_ATTR style, let's cleanup the sysfs interface >> to get rid of the warning for later HiSilicon uncore PMU drivers. >> Otherwise the warning is throwed by checkpatch.pl for new drivers as >> follow: >> WARNING: Consider renaming function(s) 'hisi_cpumask_sysfs_show' to >> 'cpumask_show' > > [...] > >> diff --git a/drivers/perf/hisilicon/hisi_uncore_pmu.c b/drivers/perf/hisilicon/hisi_uncore_pmu.c >> index 97aff877a4e7..e2612a73edf6 100644 >> --- a/drivers/perf/hisilicon/hisi_uncore_pmu.c >> +++ b/drivers/perf/hisilicon/hisi_uncore_pmu.c >> @@ -54,14 +54,14 @@ EXPORT_SYMBOL_GPL(hisi_event_sysfs_show); >> /* >> * sysfs cpumask attributes. For uncore PMU, we only have a single CPU to show >> */ >> -ssize_t hisi_cpumask_sysfs_show(struct device *dev, >> - struct device_attribute *attr, char *buf) >> +ssize_t cpumask_show(struct device *dev, struct device_attribute *attr, >> + char *buf) >> { >> struct hisi_pmu *hisi_pmu = to_hisi_pmu(dev_get_drvdata(dev)); >> >> return sprintf(buf, "%d\n", hisi_pmu->on_cpu); >> } >> -EXPORT_SYMBOL_GPL(hisi_cpumask_sysfs_show); >> +EXPORT_SYMBOL_GPL(cpumask_show); > > This seems like a colossally bad idea to me. Apologies. We are developing new uncore PMU module drivers which will be sent to commuinity later and do checkpatch.pl that introduces this warning. Since the interface is not new developed, so you suggest to keep it the same, right? Thanks, Shaokun > > Will > > . >
On Tue, Sep 15, 2020 at 10:39:28AM +0800, Shaokun Zhang wrote: > 在 2020/9/14 20:29, Will Deacon 写道: > > On Sat, Sep 12, 2020 at 05:03:06PM +0800, Shaokun Zhang wrote: > >> Since commit 001804689b0d ("checkpatch: add a few DEVICE_ATTR style > >> tests") has checked DEVICE_ATTR style, let's cleanup the sysfs interface > >> to get rid of the warning for later HiSilicon uncore PMU drivers. > >> Otherwise the warning is throwed by checkpatch.pl for new drivers as > >> follow: > >> WARNING: Consider renaming function(s) 'hisi_cpumask_sysfs_show' to > >> 'cpumask_show' > > > > [...] > > > >> diff --git a/drivers/perf/hisilicon/hisi_uncore_pmu.c b/drivers/perf/hisilicon/hisi_uncore_pmu.c > >> index 97aff877a4e7..e2612a73edf6 100644 > >> --- a/drivers/perf/hisilicon/hisi_uncore_pmu.c > >> +++ b/drivers/perf/hisilicon/hisi_uncore_pmu.c > >> @@ -54,14 +54,14 @@ EXPORT_SYMBOL_GPL(hisi_event_sysfs_show); > >> /* > >> * sysfs cpumask attributes. For uncore PMU, we only have a single CPU to show > >> */ > >> -ssize_t hisi_cpumask_sysfs_show(struct device *dev, > >> - struct device_attribute *attr, char *buf) > >> +ssize_t cpumask_show(struct device *dev, struct device_attribute *attr, > >> + char *buf) > >> { > >> struct hisi_pmu *hisi_pmu = to_hisi_pmu(dev_get_drvdata(dev)); > >> > >> return sprintf(buf, "%d\n", hisi_pmu->on_cpu); > >> } > >> -EXPORT_SYMBOL_GPL(hisi_cpumask_sysfs_show); > >> +EXPORT_SYMBOL_GPL(cpumask_show); > > > > This seems like a colossally bad idea to me. > > Apologies. We are developing new uncore PMU module drivers which will be sent to commuinity > later and do checkpatch.pl that introduces this warning. Since the interface is not new > developed, so you suggest to keep it the same, right? I don't think checkpatch warnings hold much value, but in this case having a global (exported!) 'cpumask_show' symbol sounds like it is going to conflict with anybody else trying to fix similar warnings. By all means cleanup the static symbols, but I don't think it's a good idea to go beyond that. Will
Hi Will, 在 2020/9/15 18:30, Will Deacon 写道: > On Tue, Sep 15, 2020 at 10:39:28AM +0800, Shaokun Zhang wrote: >> 在 2020/9/14 20:29, Will Deacon 写道: >>> On Sat, Sep 12, 2020 at 05:03:06PM +0800, Shaokun Zhang wrote: >>>> Since commit 001804689b0d ("checkpatch: add a few DEVICE_ATTR style >>>> tests") has checked DEVICE_ATTR style, let's cleanup the sysfs interface >>>> to get rid of the warning for later HiSilicon uncore PMU drivers. >>>> Otherwise the warning is throwed by checkpatch.pl for new drivers as >>>> follow: >>>> WARNING: Consider renaming function(s) 'hisi_cpumask_sysfs_show' to >>>> 'cpumask_show' >>> >>> [...] >>> >>>> diff --git a/drivers/perf/hisilicon/hisi_uncore_pmu.c b/drivers/perf/hisilicon/hisi_uncore_pmu.c >>>> index 97aff877a4e7..e2612a73edf6 100644 >>>> --- a/drivers/perf/hisilicon/hisi_uncore_pmu.c >>>> +++ b/drivers/perf/hisilicon/hisi_uncore_pmu.c >>>> @@ -54,14 +54,14 @@ EXPORT_SYMBOL_GPL(hisi_event_sysfs_show); >>>> /* >>>> * sysfs cpumask attributes. For uncore PMU, we only have a single CPU to show >>>> */ >>>> -ssize_t hisi_cpumask_sysfs_show(struct device *dev, >>>> - struct device_attribute *attr, char *buf) >>>> +ssize_t cpumask_show(struct device *dev, struct device_attribute *attr, >>>> + char *buf) >>>> { >>>> struct hisi_pmu *hisi_pmu = to_hisi_pmu(dev_get_drvdata(dev)); >>>> >>>> return sprintf(buf, "%d\n", hisi_pmu->on_cpu); >>>> } >>>> -EXPORT_SYMBOL_GPL(hisi_cpumask_sysfs_show); >>>> +EXPORT_SYMBOL_GPL(cpumask_show); >>> >>> This seems like a colossally bad idea to me. >> >> Apologies. We are developing new uncore PMU module drivers which will be sent to commuinity >> later and do checkpatch.pl that introduces this warning. Since the interface is not new >> developed, so you suggest to keep it the same, right? > > I don't think checkpatch warnings hold much value, but in this case having a > global (exported!) 'cpumask_show' symbol sounds like it is going to conflict > with anybody else trying to fix similar warnings. By all means cleanup the Oh, correct, I don't consider it enough. > static symbols, but I don't think it's a good idea to go beyond that. Got it. Thanks, Shaokun > > Will > . >
diff --git a/drivers/perf/hisilicon/hisi_uncore_ddrc_pmu.c b/drivers/perf/hisilicon/hisi_uncore_ddrc_pmu.c index 5e3645c96443..a6305ae8dd79 100644 --- a/drivers/perf/hisilicon/hisi_uncore_ddrc_pmu.c +++ b/drivers/perf/hisilicon/hisi_uncore_ddrc_pmu.c @@ -297,7 +297,7 @@ static const struct attribute_group hisi_ddrc_pmu_events_group = { .attrs = hisi_ddrc_pmu_events_attr, }; -static DEVICE_ATTR(cpumask, 0444, hisi_cpumask_sysfs_show, NULL); +static DEVICE_ATTR_RO(cpumask); static struct attribute *hisi_ddrc_pmu_cpumask_attrs[] = { &dev_attr_cpumask.attr, diff --git a/drivers/perf/hisilicon/hisi_uncore_hha_pmu.c b/drivers/perf/hisilicon/hisi_uncore_hha_pmu.c index 5eb8168029c0..a7fffa3b701f 100644 --- a/drivers/perf/hisilicon/hisi_uncore_hha_pmu.c +++ b/drivers/perf/hisilicon/hisi_uncore_hha_pmu.c @@ -309,7 +309,7 @@ static const struct attribute_group hisi_hha_pmu_events_group = { .attrs = hisi_hha_pmu_events_attr, }; -static DEVICE_ATTR(cpumask, 0444, hisi_cpumask_sysfs_show, NULL); +static DEVICE_ATTR_RO(cpumask); static struct attribute *hisi_hha_pmu_cpumask_attrs[] = { &dev_attr_cpumask.attr, diff --git a/drivers/perf/hisilicon/hisi_uncore_l3c_pmu.c b/drivers/perf/hisilicon/hisi_uncore_l3c_pmu.c index 3e8b5eab5514..dc7dfcec15df 100644 --- a/drivers/perf/hisilicon/hisi_uncore_l3c_pmu.c +++ b/drivers/perf/hisilicon/hisi_uncore_l3c_pmu.c @@ -299,7 +299,7 @@ static const struct attribute_group hisi_l3c_pmu_events_group = { .attrs = hisi_l3c_pmu_events_attr, }; -static DEVICE_ATTR(cpumask, 0444, hisi_cpumask_sysfs_show, NULL); +static DEVICE_ATTR_RO(cpumask); static struct attribute *hisi_l3c_pmu_cpumask_attrs[] = { &dev_attr_cpumask.attr, diff --git a/drivers/perf/hisilicon/hisi_uncore_pmu.c b/drivers/perf/hisilicon/hisi_uncore_pmu.c index 97aff877a4e7..e2612a73edf6 100644 --- a/drivers/perf/hisilicon/hisi_uncore_pmu.c +++ b/drivers/perf/hisilicon/hisi_uncore_pmu.c @@ -54,14 +54,14 @@ EXPORT_SYMBOL_GPL(hisi_event_sysfs_show); /* * sysfs cpumask attributes. For uncore PMU, we only have a single CPU to show */ -ssize_t hisi_cpumask_sysfs_show(struct device *dev, - struct device_attribute *attr, char *buf) +ssize_t cpumask_show(struct device *dev, struct device_attribute *attr, + char *buf) { struct hisi_pmu *hisi_pmu = to_hisi_pmu(dev_get_drvdata(dev)); return sprintf(buf, "%d\n", hisi_pmu->on_cpu); } -EXPORT_SYMBOL_GPL(hisi_cpumask_sysfs_show); +EXPORT_SYMBOL_GPL(cpumask_show); static bool hisi_validate_event_group(struct perf_event *event) { diff --git a/drivers/perf/hisilicon/hisi_uncore_pmu.h b/drivers/perf/hisilicon/hisi_uncore_pmu.h index b59ec22169ab..59a16e9d9f70 100644 --- a/drivers/perf/hisilicon/hisi_uncore_pmu.h +++ b/drivers/perf/hisilicon/hisi_uncore_pmu.h @@ -93,8 +93,8 @@ ssize_t hisi_event_sysfs_show(struct device *dev, struct device_attribute *attr, char *buf); ssize_t hisi_format_sysfs_show(struct device *dev, struct device_attribute *attr, char *buf); -ssize_t hisi_cpumask_sysfs_show(struct device *dev, - struct device_attribute *attr, char *buf); +ssize_t cpumask_show(struct device *dev, + struct device_attribute *attr, char *buf); int hisi_uncore_pmu_online_cpu(unsigned int cpu, struct hlist_node *node); int hisi_uncore_pmu_offline_cpu(unsigned int cpu, struct hlist_node *node); #endif /* __HISI_UNCORE_PMU_H__ */
Since commit 001804689b0d ("checkpatch: add a few DEVICE_ATTR style tests") has checked DEVICE_ATTR style, let's cleanup the sysfs interface to get rid of the warning for later HiSilicon uncore PMU drivers. Otherwise the warning is throwed by checkpatch.pl for new drivers as follow: WARNING: Consider renaming function(s) 'hisi_cpumask_sysfs_show' to 'cpumask_show' Cc: Will Deacon <will@kernel.org> Cc: Mark Rutland <mark.rutland@arm.com> Signed-off-by: Shaokun Zhang <zhangshaokun@hisilicon.com> Signed-off-by: Qi Liu <liuqi115@huawei.com> --- drivers/perf/hisilicon/hisi_uncore_ddrc_pmu.c | 2 +- drivers/perf/hisilicon/hisi_uncore_hha_pmu.c | 2 +- drivers/perf/hisilicon/hisi_uncore_l3c_pmu.c | 2 +- drivers/perf/hisilicon/hisi_uncore_pmu.c | 6 +++--- drivers/perf/hisilicon/hisi_uncore_pmu.h | 4 ++-- 5 files changed, 8 insertions(+), 8 deletions(-)