| Message ID | 20230914112223.27165-6-yangyicong@huawei.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Several updates for PTT driver | expand |
On 14/09/2023 12:22, Yicong Yang wrote: > From: Junhao He <hejunhao3@huawei.com> > > When start trace with perf option "-C $cpu" and immediately stop it > with SIGTERM or others, the perf core will invoke pmu::read() while > the driver doesn't implement it. Add a dummy pmu::read() to avoid > any issues. What issues are we talking about here ? Shouldn't the core perf skip the call, if pmu::read() is not available ? Suzuki > > Signed-off-by: Junhao He <hejunhao3@huawei.com> > Signed-off-by: Yicong Yang <yangyicong@hisilicon.com> > Acked-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > --- > drivers/hwtracing/ptt/hisi_ptt.c | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/drivers/hwtracing/ptt/hisi_ptt.c b/drivers/hwtracing/ptt/hisi_ptt.c > index 62a444f5228e..c1b5fd2b8974 100644 > --- a/drivers/hwtracing/ptt/hisi_ptt.c > +++ b/drivers/hwtracing/ptt/hisi_ptt.c > @@ -1184,6 +1184,10 @@ static void hisi_ptt_pmu_del(struct perf_event *event, int flags) > hisi_ptt_pmu_stop(event, PERF_EF_UPDATE); > } > > +static void hisi_ptt_pmu_read(struct perf_event *event) > +{ > +} > + > static void hisi_ptt_remove_cpuhp_instance(void *hotplug_node) > { > cpuhp_state_remove_instance_nocalls(hisi_ptt_pmu_online, hotplug_node); > @@ -1227,6 +1231,7 @@ static int hisi_ptt_register_pmu(struct hisi_ptt *hisi_ptt) > .stop = hisi_ptt_pmu_stop, > .add = hisi_ptt_pmu_add, > .del = hisi_ptt_pmu_del, > + .read = hisi_ptt_pmu_read, > }; > > reg = readl(hisi_ptt->iobase + HISI_PTT_LOCATION);
On 2023/9/15 20:53, Suzuki K Poulose wrote: > On 14/09/2023 12:22, Yicong Yang wrote: >> From: Junhao He <hejunhao3@huawei.com> >> >> When start trace with perf option "-C $cpu" and immediately stop it >> with SIGTERM or others, the perf core will invoke pmu::read() while >> the driver doesn't implement it. Add a dummy pmu::read() to avoid >> any issues. > > What issues are we talking about here ? Shouldn't the core perf > skip the call, if pmu::read() is not available ? > Actually no, the core doesn't check it. So I think that's why some PMUs like SPE implements a dummy pmu::read() callback. Otherwise we'll dereference a NULL pointer. Currently we only met this on emulated platforms with very slow CPUs, follow the instructions in the commit above. > Suzuki > >> >> Signed-off-by: Junhao He <hejunhao3@huawei.com> >> Signed-off-by: Yicong Yang <yangyicong@hisilicon.com> >> Acked-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> >> --- >> drivers/hwtracing/ptt/hisi_ptt.c | 5 +++++ >> 1 file changed, 5 insertions(+) >> >> diff --git a/drivers/hwtracing/ptt/hisi_ptt.c b/drivers/hwtracing/ptt/hisi_ptt.c >> index 62a444f5228e..c1b5fd2b8974 100644 >> --- a/drivers/hwtracing/ptt/hisi_ptt.c >> +++ b/drivers/hwtracing/ptt/hisi_ptt.c >> @@ -1184,6 +1184,10 @@ static void hisi_ptt_pmu_del(struct perf_event *event, int flags) >> hisi_ptt_pmu_stop(event, PERF_EF_UPDATE); >> } >> +static void hisi_ptt_pmu_read(struct perf_event *event) >> +{ >> +} >> + >> static void hisi_ptt_remove_cpuhp_instance(void *hotplug_node) >> { >> cpuhp_state_remove_instance_nocalls(hisi_ptt_pmu_online, hotplug_node); >> @@ -1227,6 +1231,7 @@ static int hisi_ptt_register_pmu(struct hisi_ptt *hisi_ptt) >> .stop = hisi_ptt_pmu_stop, >> .add = hisi_ptt_pmu_add, >> .del = hisi_ptt_pmu_del, >> + .read = hisi_ptt_pmu_read, >> }; >> reg = readl(hisi_ptt->iobase + HISI_PTT_LOCATION); > > > .
On 19/09/2023 14:03, Yicong Yang wrote: > On 2023/9/15 20:53, Suzuki K Poulose wrote: >> On 14/09/2023 12:22, Yicong Yang wrote: >>> From: Junhao He <hejunhao3@huawei.com> >>> >>> When start trace with perf option "-C $cpu" and immediately stop it >>> with SIGTERM or others, the perf core will invoke pmu::read() while >>> the driver doesn't implement it. Add a dummy pmu::read() to avoid >>> any issues. >> >> What issues are we talking about here ? Shouldn't the core perf >> skip the call, if pmu::read() is not available ? >> > > Actually no, the core doesn't check it. So I think that's why some PMUs > like SPE implements a dummy pmu::read() callback. Otherwise we'll > dereference a NULL pointer. > > Currently we only met this on emulated platforms with very slow CPUs, > follow the instructions in the commit above. Ok, then it calls for a Fixes tag. Please tag it to the commit that introduced the PMU. Suzuki
On 2023/9/20 1:01, Suzuki K Poulose wrote: > On 19/09/2023 14:03, Yicong Yang wrote: >> On 2023/9/15 20:53, Suzuki K Poulose wrote: >>> On 14/09/2023 12:22, Yicong Yang wrote: >>>> From: Junhao He <hejunhao3@huawei.com> >>>> >>>> When start trace with perf option "-C $cpu" and immediately stop it >>>> with SIGTERM or others, the perf core will invoke pmu::read() while >>>> the driver doesn't implement it. Add a dummy pmu::read() to avoid >>>> any issues. >>> >>> What issues are we talking about here ? Shouldn't the core perf >>> skip the call, if pmu::read() is not available ? >>> >> >> Actually no, the core doesn't check it. So I think that's why some PMUs >> like SPE implements a dummy pmu::read() callback. Otherwise we'll >> dereference a NULL pointer. >> >> Currently we only met this on emulated platforms with very slow CPUs, >> follow the instructions in the commit above. > > Ok, then it calls for a Fixes tag. Please tag it to the commit that > introduced the PMU. > Sure. I'll add the tag in v3. Thanks.
diff --git a/drivers/hwtracing/ptt/hisi_ptt.c b/drivers/hwtracing/ptt/hisi_ptt.c index 62a444f5228e..c1b5fd2b8974 100644 --- a/drivers/hwtracing/ptt/hisi_ptt.c +++ b/drivers/hwtracing/ptt/hisi_ptt.c @@ -1184,6 +1184,10 @@ static void hisi_ptt_pmu_del(struct perf_event *event, int flags) hisi_ptt_pmu_stop(event, PERF_EF_UPDATE); } +static void hisi_ptt_pmu_read(struct perf_event *event) +{ +} + static void hisi_ptt_remove_cpuhp_instance(void *hotplug_node) { cpuhp_state_remove_instance_nocalls(hisi_ptt_pmu_online, hotplug_node); @@ -1227,6 +1231,7 @@ static int hisi_ptt_register_pmu(struct hisi_ptt *hisi_ptt) .stop = hisi_ptt_pmu_stop, .add = hisi_ptt_pmu_add, .del = hisi_ptt_pmu_del, + .read = hisi_ptt_pmu_read, }; reg = readl(hisi_ptt->iobase + HISI_PTT_LOCATION);