| Message ID | 20200731064012.8076-21-tingwei@codeaurora.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | coresight: allow to build coresight as modules | expand |
Hi Tingwei, On Fri, 31 Jul 2020 at 07:42, Tingwei Zhang <tingwei@codeaurora.org> wrote: > > If associated ect device is not enabled at first place, disable > routine should not be called. Add ect_enabled flag to check whether > ect device is enabled. Fix the issue in below case. Ect device is > not available when associated coresight device enabled and the > association is established after coresight device is enabled. > > Signed-off-by: Mike Leach <mike.leach@linaro.org> > Signed-off-by: Tingwei Zhang <tingwei@codeaurora.org> > --- > drivers/hwtracing/coresight/coresight.c | 11 ++++++++--- > include/linux/coresight.h | 1 + > 2 files changed, 9 insertions(+), 3 deletions(-) > > diff --git a/drivers/hwtracing/coresight/coresight.c b/drivers/hwtracing/coresight/coresight.c > index 8b55383cfcf1..83a46cf37968 100644 > --- a/drivers/hwtracing/coresight/coresight.c > +++ b/drivers/hwtracing/coresight/coresight.c > @@ -245,13 +245,18 @@ coresight_control_assoc_ectdev(struct coresight_device *csdev, bool enable) > > if (!ect_csdev) > return 0; > + if ((!ect_ops(ect_csdev)->enable) || (!ect_ops(ect_csdev)->disable)) > + return 0; > > if (enable) { > - if (ect_ops(ect_csdev)->enable) > - ect_ret = ect_ops(ect_csdev)->enable(ect_csdev); > + ect_ret = ect_ops(ect_csdev)->enable(ect_csdev); > + if (ect_ret) Should this be:- if (!ect_ret) However, this problem is corrected in the next patch - so the set as tested overall works. This needs fixing or it might be better to combine this patch and the next patch into a single patch? Both are required because the CTI can be loaded as a module. Both adjust the same block of functionality. Mathieu may have a view on the best solution option here. Mike > + csdev->ect_enabled = true; > } else { > - if (ect_ops(ect_csdev)->disable) > + if (csdev->ect_enabled) { > ect_ret = ect_ops(ect_csdev)->disable(ect_csdev); > + csdev->ect_enabled = false; > + } > } > > /* output warning if ECT enable is preventing trace operation */ > diff --git a/include/linux/coresight.h b/include/linux/coresight.h > index 3bb738f9a326..7d3c87e5b97c 100644 > --- a/include/linux/coresight.h > +++ b/include/linux/coresight.h > @@ -208,6 +208,7 @@ struct coresight_device { > /* sysfs links between components */ > int nr_links; > bool has_conns_grp; > + bool ect_enabled; /* true only if associated ect device is enabled */ > }; > > /* > -- > The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, > a Linux Foundation Collaborative Project > -- Mike Leach Principal Engineer, ARM Ltd. Manchester Design Centre. UK
Hi Mike, On Tue, Aug 04, 2020 at 01:13:06AM +0800, Mike Leach wrote: > Hi Tingwei, > > On Fri, 31 Jul 2020 at 07:42, Tingwei Zhang <tingwei@codeaurora.org> wrote: > > > > If associated ect device is not enabled at first place, disable > > routine should not be called. Add ect_enabled flag to check whether > > ect device is enabled. Fix the issue in below case. Ect device is > > not available when associated coresight device enabled and the > > association is established after coresight device is enabled. > > > > Signed-off-by: Mike Leach <mike.leach@linaro.org> > > Signed-off-by: Tingwei Zhang <tingwei@codeaurora.org> > > --- > > drivers/hwtracing/coresight/coresight.c | 11 ++++++++--- > > include/linux/coresight.h | 1 + > > 2 files changed, 9 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/hwtracing/coresight/coresight.c > > b/drivers/hwtracing/coresight/coresight.c > > index 8b55383cfcf1..83a46cf37968 100644 > > --- a/drivers/hwtracing/coresight/coresight.c > > +++ b/drivers/hwtracing/coresight/coresight.c > > @@ -245,13 +245,18 @@ coresight_control_assoc_ectdev(struct > > coresight_device *csdev, bool enable) > > > > if (!ect_csdev) > > return 0; > > + if ((!ect_ops(ect_csdev)->enable) || > > (!ect_ops(ect_csdev)->disable)) > > + return 0; > > > > if (enable) { > > - if (ect_ops(ect_csdev)->enable) > > - ect_ret = ect_ops(ect_csdev)->enable(ect_csdev); > > + ect_ret = ect_ops(ect_csdev)->enable(ect_csdev); > > + if (ect_ret) > > Should this be:- > if (!ect_ret) > > However, this problem is corrected in the next patch - so the set as > tested overall works. Thanks a lot for testing this series, Mike. Sorry for this stupid mistake. I'll address it in v7. > > This needs fixing or it might be better to combine this patch and the > next patch into a single patch? > Both are required because the CTI can be loaded as a module. Both > adjust the same block of functionality. I separate them into two since this patch adds ect_enabled flag and next patch add module and device reference. Kind of two things in my opinion. I'm open for discusion and merge them into one path. Thanks, Tingwei > > Mathieu may have a view on the best solution option here. > > Mike > > > > > + csdev->ect_enabled = true; > > } else { > > - if (ect_ops(ect_csdev)->disable) > > + if (csdev->ect_enabled) { > > ect_ret = ect_ops(ect_csdev)->disable(ect_csdev); > > + csdev->ect_enabled = false; > > + } > > } > > > > /* output warning if ECT enable is preventing trace operation */ > > diff --git a/include/linux/coresight.h b/include/linux/coresight.h > > index 3bb738f9a326..7d3c87e5b97c 100644 > > --- a/include/linux/coresight.h > > +++ b/include/linux/coresight.h > > @@ -208,6 +208,7 @@ struct coresight_device { > > /* sysfs links between components */ > > int nr_links; > > bool has_conns_grp; > > + bool ect_enabled; /* true only if associated ect device is enabled > > */ > > }; > > > > /* > > -- > > The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, > > a Linux Foundation Collaborative Project > > > > > -- > Mike Leach > Principal Engineer, ARM Ltd. > Manchester Design Centre. UK
Hi Tingwei, On Tue, 4 Aug 2020 at 01:22, Tingwei Zhang <tingweiz@codeaurora.org> wrote: > > Hi Mike, > On Tue, Aug 04, 2020 at 01:13:06AM +0800, Mike Leach wrote: > > Hi Tingwei, > > > > On Fri, 31 Jul 2020 at 07:42, Tingwei Zhang <tingwei@codeaurora.org> wrote: > > > > > > If associated ect device is not enabled at first place, disable > > > routine should not be called. Add ect_enabled flag to check whether > > > ect device is enabled. Fix the issue in below case. Ect device is > > > not available when associated coresight device enabled and the > > > association is established after coresight device is enabled. > > > > > > Signed-off-by: Mike Leach <mike.leach@linaro.org> > > > Signed-off-by: Tingwei Zhang <tingwei@codeaurora.org> > > > --- > > > drivers/hwtracing/coresight/coresight.c | 11 ++++++++--- > > > include/linux/coresight.h | 1 + > > > 2 files changed, 9 insertions(+), 3 deletions(-) > > > > > > diff --git a/drivers/hwtracing/coresight/coresight.c > > > b/drivers/hwtracing/coresight/coresight.c > > > index 8b55383cfcf1..83a46cf37968 100644 > > > --- a/drivers/hwtracing/coresight/coresight.c > > > +++ b/drivers/hwtracing/coresight/coresight.c > > > @@ -245,13 +245,18 @@ coresight_control_assoc_ectdev(struct > > > coresight_device *csdev, bool enable) > > > > > > if (!ect_csdev) > > > return 0; > > > + if ((!ect_ops(ect_csdev)->enable) || > > > (!ect_ops(ect_csdev)->disable)) > > > + return 0; > > > > > > if (enable) { > > > - if (ect_ops(ect_csdev)->enable) > > > - ect_ret = ect_ops(ect_csdev)->enable(ect_csdev); > > > + ect_ret = ect_ops(ect_csdev)->enable(ect_csdev); > > > + if (ect_ret) > > > > Should this be:- > > if (!ect_ret) > > > > However, this problem is corrected in the next patch - so the set as > > tested overall works. > > Thanks a lot for testing this series, Mike. > > Sorry for this stupid mistake. I'll address it in v7. > > > > > This needs fixing or it might be better to combine this patch and the > > next patch into a single patch? > > Both are required because the CTI can be loaded as a module. Both > > adjust the same block of functionality. > > I separate them into two since this patch adds ect_enabled flag and > next patch add module and device reference. Kind of two things in > my opinion. I'm open for discusion and merge them into one path. > I don't mind which solution is used. I mention it because I have had objections in the past where two patches in a set change the same piece of code. Regards Mike > Thanks, > Tingwei > > > > > Mathieu may have a view on the best solution option here. > > > > Mike > > > > > > > > > + csdev->ect_enabled = true; > > > } else { > > > - if (ect_ops(ect_csdev)->disable) > > > + if (csdev->ect_enabled) { > > > ect_ret = ect_ops(ect_csdev)->disable(ect_csdev); > > > + csdev->ect_enabled = false; > > > + } > > > } > > > > > > /* output warning if ECT enable is preventing trace operation */ > > > diff --git a/include/linux/coresight.h b/include/linux/coresight.h > > > index 3bb738f9a326..7d3c87e5b97c 100644 > > > --- a/include/linux/coresight.h > > > +++ b/include/linux/coresight.h > > > @@ -208,6 +208,7 @@ struct coresight_device { > > > /* sysfs links between components */ > > > int nr_links; > > > bool has_conns_grp; > > > + bool ect_enabled; /* true only if associated ect device is enabled > > > */ > > > }; > > > > > > /* > > > -- > > > The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, > > > a Linux Foundation Collaborative Project > > > > > > > > > -- > > Mike Leach > > Principal Engineer, ARM Ltd. > > Manchester Design Centre. UK
diff --git a/drivers/hwtracing/coresight/coresight.c b/drivers/hwtracing/coresight/coresight.c index 8b55383cfcf1..83a46cf37968 100644 --- a/drivers/hwtracing/coresight/coresight.c +++ b/drivers/hwtracing/coresight/coresight.c @@ -245,13 +245,18 @@ coresight_control_assoc_ectdev(struct coresight_device *csdev, bool enable) if (!ect_csdev) return 0; + if ((!ect_ops(ect_csdev)->enable) || (!ect_ops(ect_csdev)->disable)) + return 0; if (enable) { - if (ect_ops(ect_csdev)->enable) - ect_ret = ect_ops(ect_csdev)->enable(ect_csdev); + ect_ret = ect_ops(ect_csdev)->enable(ect_csdev); + if (ect_ret) + csdev->ect_enabled = true; } else { - if (ect_ops(ect_csdev)->disable) + if (csdev->ect_enabled) { ect_ret = ect_ops(ect_csdev)->disable(ect_csdev); + csdev->ect_enabled = false; + } } /* output warning if ECT enable is preventing trace operation */ diff --git a/include/linux/coresight.h b/include/linux/coresight.h index 3bb738f9a326..7d3c87e5b97c 100644 --- a/include/linux/coresight.h +++ b/include/linux/coresight.h @@ -208,6 +208,7 @@ struct coresight_device { /* sysfs links between components */ int nr_links; bool has_conns_grp; + bool ect_enabled; /* true only if associated ect device is enabled */ }; /*