| Message ID | 20230728084837.276551-1-anshuman.khandual@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | coresight: tmc: Make etr buffer mode user configurable from sysfs | expand |
On 28/07/2023 09:48, Anshuman Khandual wrote: > Currently TMC-ETR automatically selects the buffer mode from all available > methods in the following sequentially fallback manner - also in that order. > > 1. FLAT mode with or without IOMMU > 2. TMC-ETR-SG (scatter gather) mode when available > 3. CATU mode when available > > But this order might not be ideal for all situations. For example if there > is a CATU connected to ETR, it may be better to use TMC-ETR scatter gather > method, rather than CATU. The statement is wrong. But hard coding such order changes will prevent > us from testing or using a particular mode. This change provides following > new sysfs tunables for the user to control TMC-ETR buffer mode explicitly, > if required. > > /sys/bus/coresight/devices/tmc_etr<N>/etr_buf_modes_available > /sys/bus/coresight/devices/tmc_etr<N>/etr_buf_mode_current Given this only appears for TMC-ETR, could this be simple : available_buf_modes and preferred_buf_mode. We should fall back to the auto logic to use an appropriate mode if the "perferred" mode cannot satisfy the request. (e.g., flat mode with a large buffer. This may be possible on a system without much load). > $ cat etr_buf_modes_available > auto flat tmc-sg catu ------------------> Supported TMC-ETR buffer modes > $ echo catu > etr_buf_mode_current -------> Explicit buffer mode request > > But explicit user request has to be within supported ETR buffer modes only. > These sysfs interface files are exclussive to ETR, and hence not available > for other TMC devices such as ETB or ETF etc. This required separating out > new 'coresight_etr_groups' from common 'coresight_tmc_groups'. strip this, you don't need implementation commentary. > > This adds a new element 'etr_mode' in 'struct tmc_drvdata' which will track > such explicit user directives. this too. 'auto' mode has been added to help fallback > to the existing default behaviour. ETR_MODE_FLAT mode availability follows > existing logic as in tmc_alloc_etr_buf() creating a common helper function > i.e etr_supports_flat_mode(). this too. > > Cc: Suzuki K Poulose <suzuki.poulose@arm.com> > Cc: Mike Leach <mike.leach@linaro.org> > Cc: James Clark <james.clark@arm.com> > Cc: Leo Yan <leo.yan@linaro.org> > Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com> > Cc: coresight@lists.linaro.org > Cc: linux-arm-kernel@lists.infradead.org > Cc: linux-kernel@vger.kernel.org > Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com> > --- > .../testing/sysfs-bus-coresight-devices-tmc | 16 +++ > .../hwtracing/coresight/coresight-tmc-core.c | 103 +++++++++++++++++- > .../hwtracing/coresight/coresight-tmc-etr.c | 27 +++-- > drivers/hwtracing/coresight/coresight-tmc.h | 10 ++ > 4 files changed, 143 insertions(+), 13 deletions(-) > > diff --git a/Documentation/ABI/testing/sysfs-bus-coresight-devices-tmc b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tmc > index 6aa527296c71..956a2f090950 100644 > --- a/Documentation/ABI/testing/sysfs-bus-coresight-devices-tmc > +++ b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tmc > @@ -91,3 +91,19 @@ Contact: Mathieu Poirier <mathieu.poirier@linaro.org> > Description: (RW) Size of the trace buffer for TMC-ETR when used in SYSFS > mode. Writable only for TMC-ETR configurations. The value > should be aligned to the kernel pagesize. > + > +What: /sys/bus/coresight/devices/<memory_map>.tmc/etr_buf_modes_available > +Date: July 2023 > +KernelVersion: 6.6 > +Contact: Anshuman Khandual <anshuman.khandual@arm.com> > +Description: (Read) Shows all supported Coresight TMC-ETR buffer modes available > + for the users to configure explicitly. This file is avaialble only > + for TMC ETR devices. > + > +What: /sys/bus/coresight/devices/<memory_map>.tmc/etr_buf_mode_current > +Date: July 2023 > +KernelVersion: 6.6 > +Contact: Anshuman Khandual <anshuman.khandual@arm.com> > +Description: (RW) Current Coresight TMC-ETR buffer mode selected. But user could > + only provide a mode which is supported for a given ETR device. This > + file is available only for TMC ETR devices. > diff --git a/drivers/hwtracing/coresight/coresight-tmc-core.c b/drivers/hwtracing/coresight/coresight-tmc-core.c > index c106d142e632..ce97ff5e0997 100644 > --- a/drivers/hwtracing/coresight/coresight-tmc-core.c > +++ b/drivers/hwtracing/coresight/coresight-tmc-core.c > @@ -10,6 +10,7 @@ > #include <linux/device.h> > #include <linux/idr.h> > #include <linux/io.h> > +#include <linux/iommu.h> > #include <linux/err.h> > #include <linux/fs.h> > #include <linux/miscdevice.h> > @@ -329,6 +330,85 @@ static ssize_t buffer_size_store(struct device *dev, > > static DEVICE_ATTR_RW(buffer_size); > > +static const char *const buf_modes_str[] = { > + [ETR_MODE_FLAT] = "flat", > + [ETR_MODE_ETR_SG] = "tmc-sg", > + [ETR_MODE_CATU] = "catu", > + [ETR_MODE_AUTO] = "auto", > +}; > + > +void get_etr_buf_hw(struct device *dev, struct etr_buf_hw *buf_hw) > +{ > + struct tmc_drvdata *drvdata = dev_get_drvdata(dev->parent); > + > + buf_hw->has_iommu = iommu_get_domain_for_dev(dev->parent); > + buf_hw->has_etr_sg = tmc_etr_has_cap(drvdata, TMC_ETR_SG); > + buf_hw->has_catu = !!tmc_etr_get_catu_device(drvdata); > +} > + > +bool etr_supports_flat_mode(struct etr_buf_hw *buf_hw, ssize_t etr_buf_size) > +{ > + bool has_sg = buf_hw->has_catu || buf_hw->has_etr_sg; > + > + return !has_sg || buf_hw->has_iommu || etr_buf_size < SZ_1M; > +} > + Flat mode is always supported and the user must be allowed to "prefer" it. This logic can be applied to the "auto mode" though and should be renamed to etr_can_use_flat_mode(buf_hw, size) > +static ssize_t etr_buf_modes_available_show(struct device *dev, > + struct device_attribute *attr, char *buf) > +{ > + struct tmc_drvdata *drvdata = dev_get_drvdata(dev->parent); > + struct etr_buf_hw buf_hw; > + ssize_t size = 0; > + > + get_etr_buf_hw(dev, &buf_hw); > + size += sysfs_emit(buf, "%s ", buf_modes_str[ETR_MODE_AUTO]); > + if (etr_supports_flat_mode(&buf_hw, drvdata->size)) > + size += sysfs_emit_at(buf, size, "%s ", buf_modes_str[ETR_MODE_FLAT]); Always supported and must be available > + > + if (buf_hw.has_etr_sg) > + size += sysfs_emit_at(buf, size, "%s ", buf_modes_str[ETR_MODE_ETR_SG]); > + > + if (buf_hw.has_catu) > + size += sysfs_emit_at(buf, size, "%s ", buf_modes_str[ETR_MODE_CATU]); > + > + size += sysfs_emit_at(buf, size, "\n"); > + return size; > +} > +static DEVICE_ATTR_RO(etr_buf_modes_available); > + > +static ssize_t etr_buf_mode_current_show(struct device *dev, > + struct device_attribute *attr, char *buf) > +{ > + struct tmc_drvdata *drvdata = dev_get_drvdata(dev->parent); > + > + return sysfs_emit(buf, "%s\n", buf_modes_str[drvdata->etr_mode]); > +} > + > +static ssize_t etr_buf_mode_current_store(struct device *dev, > + struct device_attribute *attr, > + const char *buf, size_t size) > +{ > + struct tmc_drvdata *drvdata = dev_get_drvdata(dev->parent); > + struct etr_buf_hw buf_hw; > + > + get_etr_buf_hw(dev, &buf_hw); > + if (sysfs_streq(buf, buf_modes_str[ETR_MODE_FLAT]) && > + etr_supports_flat_mode(&buf_hw, drvdata->size)) Please remove this check, given the input is a "preference" > + drvdata->etr_mode = ETR_MODE_FLAT; > + else if (sysfs_streq(buf, buf_modes_str[ETR_MODE_ETR_SG]) && buf_hw.has_etr_sg) > + drvdata->etr_mode = ETR_MODE_ETR_SG; > + else if (sysfs_streq(buf, buf_modes_str[ETR_MODE_CATU]) && buf_hw.has_catu) > + drvdata->etr_mode = ETR_MODE_CATU; > + else if (sysfs_streq(buf, buf_modes_str[ETR_MODE_AUTO])) > + drvdata->etr_mode = ETR_MODE_AUTO; > + else > + return -EINVAL; > + > + return size; > + > +} > +static DEVICE_ATTR_RW(etr_buf_mode_current); > + > static struct attribute *coresight_tmc_attrs[] = { > &dev_attr_trigger_cntr.attr, > &dev_attr_buffer_size.attr, > @@ -350,6 +430,24 @@ static const struct attribute_group *coresight_tmc_groups[] = { > NULL, > }; > > +static struct attribute *coresight_etr_attrs[] = { > + &dev_attr_trigger_cntr.attr, > + &dev_attr_buffer_size.attr, Why don't we reuse the tmc_attrs in the etr_groups ? That way, it is much cleaner and easy to reason about. Also rename the coresight_tmc_groups => coresight_etf_groups (inline with the driver file name, coresight-tmc-etf.c ) > + &dev_attr_etr_buf_modes_available.attr, > + &dev_attr_etr_buf_mode_current.attr, > + NULL, > +}; > + > +static const struct attribute_group coresight_etr_group = { > + .attrs = coresight_etr_attrs, > +}; > + > +static const struct attribute_group *coresight_etr_groups[] = { > + &coresight_etr_group, and add: + &coresight_tmc_group, > + &coresight_tmc_mgmt_group, > + NULL, > +}; > + All of the above functions and the coresight_etr_group and related attributes could live in coresight-tmc-etr.c and we could simply expose the coresight_etr_group to the tmc-core.c That way, the code is all contained in coresight-tmc-etr.c and you don't have to expose the functions way at the bottom. > static inline bool tmc_etr_can_use_sg(struct device *dev) > { > return fwnode_property_present(dev->fwnode, "arm,scatter-gather"); > @@ -465,6 +563,7 @@ static int tmc_probe(struct amba_device *adev, const struct amba_id *id) > drvdata->memwidth = tmc_get_memwidth(devid); > /* This device is not associated with a session */ > drvdata->pid = -1; > + drvdata->etr_mode = ETR_MODE_AUTO; > > if (drvdata->config_type == TMC_CONFIG_TYPE_ETR) { > drvdata->size = tmc_etr_get_default_buffer_size(dev); > @@ -474,16 +573,17 @@ static int tmc_probe(struct amba_device *adev, const struct amba_id *id) > } > > desc.dev = dev; > - desc.groups = coresight_tmc_groups; > > switch (drvdata->config_type) { > case TMC_CONFIG_TYPE_ETB: > + desc.groups = coresight_tmc_groups; > desc.type = CORESIGHT_DEV_TYPE_SINK; > desc.subtype.sink_subtype = CORESIGHT_DEV_SUBTYPE_SINK_BUFFER; > desc.ops = &tmc_etb_cs_ops; > dev_list = &etb_devs; > break; > case TMC_CONFIG_TYPE_ETR: > + desc.groups = coresight_etr_groups; > desc.type = CORESIGHT_DEV_TYPE_SINK; > desc.subtype.sink_subtype = CORESIGHT_DEV_SUBTYPE_SINK_SYSMEM; > desc.ops = &tmc_etr_cs_ops; > @@ -496,6 +596,7 @@ static int tmc_probe(struct amba_device *adev, const struct amba_id *id) > dev_list = &etr_devs; > break; > case TMC_CONFIG_TYPE_ETF: > + desc.groups = coresight_tmc_groups; > desc.type = CORESIGHT_DEV_TYPE_LINKSINK; > desc.subtype.sink_subtype = CORESIGHT_DEV_SUBTYPE_SINK_BUFFER; > desc.subtype.link_subtype = CORESIGHT_DEV_SUBTYPE_LINK_FIFO; > diff --git a/drivers/hwtracing/coresight/coresight-tmc-etr.c b/drivers/hwtracing/coresight/coresight-tmc-etr.c > index 766325de0e29..d48455188243 100644 > --- a/drivers/hwtracing/coresight/coresight-tmc-etr.c > +++ b/drivers/hwtracing/coresight/coresight-tmc-etr.c > @@ -841,23 +841,27 @@ static struct etr_buf *tmc_alloc_etr_buf(struct tmc_drvdata *drvdata, > int node, void **pages) > { > int rc = -ENOMEM; > - bool has_etr_sg, has_iommu; > - bool has_sg, has_catu; > struct etr_buf *etr_buf; > + struct etr_buf_hw buf_hw; > struct device *dev = &drvdata->csdev->dev; > > - has_etr_sg = tmc_etr_has_cap(drvdata, TMC_ETR_SG); > - has_iommu = iommu_get_domain_for_dev(dev->parent); > - has_catu = !!tmc_etr_get_catu_device(drvdata); > - > - has_sg = has_catu || has_etr_sg; > - > + get_etr_buf_hw(dev, &buf_hw); > etr_buf = kzalloc(sizeof(*etr_buf), GFP_KERNEL); > if (!etr_buf) > return ERR_PTR(-ENOMEM); > > etr_buf->size = size; > > + /* If there is user directive for buffer mode, try that first */ > + if (drvdata->etr_mode != ETR_MODE_AUTO) { > + rc = tmc_etr_mode_alloc_buf(drvdata->etr_mode, drvdata, > + etr_buf, node, pages); As mentioned above we should fall back to the AUTO mode of action if the above fails. Given the ETR could be used in sysfs (size via sysfs) vs perf mode (size via perf aux_buf) and the sizes controlled by different entities, a tunable set in the sysfs could cause failures. We should treat the user selection as a "preferred" mode and try that first. If that is not available, we should fallback to the "auto" logic (without resetting the preferred mode), skipping the "preferred" mode. See below. > + if (rc) { > + kfree(etr_buf); > + return ERR_PTR(rc); > + } > + } > + > /* > * If we have to use an existing list of pages, we cannot reliably > * use a contiguous DMA memory (even if we have an IOMMU). Otherwise, > @@ -870,14 +874,13 @@ static struct etr_buf *tmc_alloc_etr_buf(struct tmc_drvdata *drvdata, > * Fallback to available mechanisms. > * > */ > - if (!pages && > - (!has_sg || has_iommu || size < SZ_1M)) > + if (!pages && etr_supports_flat_mode(&buf_hw, size)) > rc = tmc_etr_mode_alloc_buf(ETR_MODE_FLAT, drvdata, > etr_buf, node, pages); > - if (rc && has_etr_sg) > + if (rc && buf_hw.has_etr_sg) > rc = tmc_etr_mode_alloc_buf(ETR_MODE_ETR_SG, drvdata, > etr_buf, node, pages); > - if (rc && has_catu) > + if (rc && buf_hw.has_catu) > rc = tmc_etr_mode_alloc_buf(ETR_MODE_CATU, drvdata, > etr_buf, node, pages); We could do : do { if (etr_mode != ETR_MODE_FLAT && !pages && etr_can_use_flat_mode(buf_hw, size)) rc = tmc_etr_mode_alloc_buf(ETR_MODE_FLAT, drvdata, etr_buf, node, pages); if (!rc) break; if (etr_mode != ETR_MODE_ETR_SG && buf_hw.has_etr_sg) rc = tmc_etr_mode_alloc_buf(ETR_MODE_ETR_SG, drvdata, etr_buf, node, pages); if (!rc) break; if (etr_mode != ETR_MODE_ETR_CATU && buf_hw.has_catu) rc = tmc_etr_mode_alloc_buf(ETR_MODE_CATU, drvdata, etr_buf, node, pages); } while (0); Suzuki > if (rc) { > diff --git a/drivers/hwtracing/coresight/coresight-tmc.h b/drivers/hwtracing/coresight/coresight-tmc.h > index b97da39652d2..ca15ccb1d807 100644 > --- a/drivers/hwtracing/coresight/coresight-tmc.h > +++ b/drivers/hwtracing/coresight/coresight-tmc.h > @@ -135,6 +135,13 @@ enum etr_mode { > ETR_MODE_FLAT, /* Uses contiguous flat buffer */ > ETR_MODE_ETR_SG, /* Uses in-built TMC ETR SG mechanism */ > ETR_MODE_CATU, /* Use SG mechanism in CATU */ > + ETR_MODE_AUTO, /* Use the default mechanism */ > +}; > + > +struct etr_buf_hw { > + bool has_iommu; > + bool has_etr_sg; > + bool has_catu; > }; > > struct etr_buf_operations; > @@ -207,6 +214,7 @@ struct tmc_drvdata { > enum tmc_mem_intf_width memwidth; > u32 trigger_cntr; > u32 etr_caps; > + enum etr_mode etr_mode; > struct idr idr; > struct mutex idr_mutex; > struct etr_buf *sysfs_buf; > @@ -334,5 +342,7 @@ void tmc_etr_set_catu_ops(const struct etr_buf_operations *catu); > void tmc_etr_remove_catu_ops(void); > struct etr_buf *tmc_etr_get_buffer(struct coresight_device *csdev, > enum cs_mode mode, void *data); > +void get_etr_buf_hw(struct device *dev, struct etr_buf_hw *buf_hw); > +bool etr_supports_flat_mode(struct etr_buf_hw *buf_hw, ssize_t etr_buf_size); > > #endif
On 8/11/23 20:03, Suzuki K Poulose wrote: > On 28/07/2023 09:48, Anshuman Khandual wrote: >> Currently TMC-ETR automatically selects the buffer mode from all available >> methods in the following sequentially fallback manner - also in that order. >> >> 1. FLAT mode with or without IOMMU >> 2. TMC-ETR-SG (scatter gather) mode when available >> 3. CATU mode when available >> >> But this order might not be ideal for all situations. For example if there >> is a CATU connected to ETR, it may be better to use TMC-ETR scatter gather >> method, rather than CATU. > > The statement is wrong. > > But hard coding such order changes will prevent >> us from testing or using a particular mode. This change provides following >> new sysfs tunables for the user to control TMC-ETR buffer mode explicitly, >> if required. >> >> /sys/bus/coresight/devices/tmc_etr<N>/etr_buf_modes_available >> /sys/bus/coresight/devices/tmc_etr<N>/etr_buf_mode_current > > Given this only appears for TMC-ETR, could this be simple : > > available_buf_modes and > > preferred_buf_mode. Make sense, will drop 'etr' from these sysfs file names, and update the documentation as required. But Mike Leach had suggested earlier to have common sub strings right at the beginning for both these sysfs file names. Hence will just keep them in the same format i.e * buf_modes_available * buf_mode_preferred (as you are suggesting to replace 'current' with 'preferred') > > We should fall back to the auto logic to use an appropriate mode > if the "perferred" mode cannot satisfy the request. (e.g., flat mode > with a large buffer. This may be possible on a system without much > load). That's a semantics change from the current proposal, which does not try to fallback on anything, in case the requested buffer mode does not work. Instead it expects the user to try a different buffer mode in such cases. > >> $ cat etr_buf_modes_available >> auto flat tmc-sg catu ------------------> Supported TMC-ETR buffer modes >> $ echo catu > etr_buf_mode_current -------> Explicit buffer mode request >> >> But explicit user request has to be within supported ETR buffer modes only. >> These sysfs interface files are exclussive to ETR, and hence not available >> for other TMC devices such as ETB or ETF etc. > > > This required separating out >> new 'coresight_etr_groups' from common 'coresight_tmc_groups'. > > strip this, you don't need implementation commentary. > >> >> This adds a new element 'etr_mode' in 'struct tmc_drvdata' which will track >> such explicit user directives. > > this too. Although I could understand not mentioning about refactoring 'coresight_tmc_groups', is not this comment necessary in explaining why 'tmc_drvdata' structure is expanded here ? > > 'auto' mode has been added to help fallback >> to the existing default behaviour. > > > ETR_MODE_FLAT mode availability follows >> existing logic as in tmc_alloc_etr_buf() creating a common helper function >> i.e etr_supports_flat_mode(). > > this too. I believe parts of the comments above in the commit message are still important but will try and reorganize them better for clarity. > >> >> Cc: Suzuki K Poulose <suzuki.poulose@arm.com> >> Cc: Mike Leach <mike.leach@linaro.org> >> Cc: James Clark <james.clark@arm.com> >> Cc: Leo Yan <leo.yan@linaro.org> >> Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com> >> Cc: coresight@lists.linaro.org >> Cc: linux-arm-kernel@lists.infradead.org >> Cc: linux-kernel@vger.kernel.org >> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com> >> --- >> .../testing/sysfs-bus-coresight-devices-tmc | 16 +++ >> .../hwtracing/coresight/coresight-tmc-core.c | 103 +++++++++++++++++- >> .../hwtracing/coresight/coresight-tmc-etr.c | 27 +++-- >> drivers/hwtracing/coresight/coresight-tmc.h | 10 ++ >> 4 files changed, 143 insertions(+), 13 deletions(-) >> >> diff --git a/Documentation/ABI/testing/sysfs-bus-coresight-devices-tmc b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tmc >> index 6aa527296c71..956a2f090950 100644 >> --- a/Documentation/ABI/testing/sysfs-bus-coresight-devices-tmc >> +++ b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tmc >> @@ -91,3 +91,19 @@ Contact: Mathieu Poirier <mathieu.poirier@linaro.org> >> Description: (RW) Size of the trace buffer for TMC-ETR when used in SYSFS >> mode. Writable only for TMC-ETR configurations. The value >> should be aligned to the kernel pagesize. >> + >> +What: /sys/bus/coresight/devices/<memory_map>.tmc/etr_buf_modes_available >> +Date: July 2023 >> +KernelVersion: 6.6 >> +Contact: Anshuman Khandual <anshuman.khandual@arm.com> >> +Description: (Read) Shows all supported Coresight TMC-ETR buffer modes available >> + for the users to configure explicitly. This file is avaialble only >> + for TMC ETR devices. >> + >> +What: /sys/bus/coresight/devices/<memory_map>.tmc/etr_buf_mode_current >> +Date: July 2023 >> +KernelVersion: 6.6 >> +Contact: Anshuman Khandual <anshuman.khandual@arm.com> >> +Description: (RW) Current Coresight TMC-ETR buffer mode selected. But user could >> + only provide a mode which is supported for a given ETR device. This >> + file is available only for TMC ETR devices. >> diff --git a/drivers/hwtracing/coresight/coresight-tmc-core.c b/drivers/hwtracing/coresight/coresight-tmc-core.c >> index c106d142e632..ce97ff5e0997 100644 >> --- a/drivers/hwtracing/coresight/coresight-tmc-core.c >> +++ b/drivers/hwtracing/coresight/coresight-tmc-core.c >> @@ -10,6 +10,7 @@ >> #include <linux/device.h> >> #include <linux/idr.h> >> #include <linux/io.h> >> +#include <linux/iommu.h> >> #include <linux/err.h> >> #include <linux/fs.h> >> #include <linux/miscdevice.h> >> @@ -329,6 +330,85 @@ static ssize_t buffer_size_store(struct device *dev, >> static DEVICE_ATTR_RW(buffer_size); >> +static const char *const buf_modes_str[] = { >> + [ETR_MODE_FLAT] = "flat", >> + [ETR_MODE_ETR_SG] = "tmc-sg", >> + [ETR_MODE_CATU] = "catu", >> + [ETR_MODE_AUTO] = "auto", >> +}; >> + >> +void get_etr_buf_hw(struct device *dev, struct etr_buf_hw *buf_hw) >> +{ >> + struct tmc_drvdata *drvdata = dev_get_drvdata(dev->parent); >> + >> + buf_hw->has_iommu = iommu_get_domain_for_dev(dev->parent); >> + buf_hw->has_etr_sg = tmc_etr_has_cap(drvdata, TMC_ETR_SG); >> + buf_hw->has_catu = !!tmc_etr_get_catu_device(drvdata); >> +} >> + >> +bool etr_supports_flat_mode(struct etr_buf_hw *buf_hw, ssize_t etr_buf_size) >> +{ >> + bool has_sg = buf_hw->has_catu || buf_hw->has_etr_sg; >> + >> + return !has_sg || buf_hw->has_iommu || etr_buf_size < SZ_1M; >> +} >> + > > Flat mode is always supported and the user must be allowed to "prefer" > it. This logic can be applied to the "auto mode" though and should be > renamed to > > etr_can_use_flat_mode(buf_hw, size) Sure, will rename the function here but not sure if I understand this completely. Are you suggesting anything else which needs to be done here ? > > > >> +static ssize_t etr_buf_modes_available_show(struct device *dev, >> + struct device_attribute *attr, char *buf) >> +{ >> + struct tmc_drvdata *drvdata = dev_get_drvdata(dev->parent); >> + struct etr_buf_hw buf_hw; >> + ssize_t size = 0; >> + >> + get_etr_buf_hw(dev, &buf_hw); >> + size += sysfs_emit(buf, "%s ", buf_modes_str[ETR_MODE_AUTO]); >> + if (etr_supports_flat_mode(&buf_hw, drvdata->size)) >> + size += sysfs_emit_at(buf, size, "%s ", buf_modes_str[ETR_MODE_FLAT]); > > Always supported and must be available But if etr_can_use_flat_mode() returns negative, should it still be shown here as an option at all ? Could flat mode be offered for buffer mode preference, if it cannot be used as determined via etr_can_use_flat_mode(). > >> + >> + if (buf_hw.has_etr_sg) >> + size += sysfs_emit_at(buf, size, "%s ", buf_modes_str[ETR_MODE_ETR_SG]); >> + >> + if (buf_hw.has_catu) >> + size += sysfs_emit_at(buf, size, "%s ", buf_modes_str[ETR_MODE_CATU]); >> + >> + size += sysfs_emit_at(buf, size, "\n"); >> + return size; >> +} >> +static DEVICE_ATTR_RO(etr_buf_modes_available); >> + >> +static ssize_t etr_buf_mode_current_show(struct device *dev, >> + struct device_attribute *attr, char *buf) >> +{ >> + struct tmc_drvdata *drvdata = dev_get_drvdata(dev->parent); >> + >> + return sysfs_emit(buf, "%s\n", buf_modes_str[drvdata->etr_mode]); >> +} >> + >> +static ssize_t etr_buf_mode_current_store(struct device *dev, >> + struct device_attribute *attr, >> + const char *buf, size_t size) >> +{ >> + struct tmc_drvdata *drvdata = dev_get_drvdata(dev->parent); >> + struct etr_buf_hw buf_hw; >> + >> + get_etr_buf_hw(dev, &buf_hw); >> + if (sysfs_streq(buf, buf_modes_str[ETR_MODE_FLAT]) && >> + etr_supports_flat_mode(&buf_hw, drvdata->size)) > > Please remove this check, given the input is a "preference" But could flat mode be accepted for preference, if it cannot be used as determined via etr_can_use_flat_mode() ? > >> + drvdata->etr_mode = ETR_MODE_FLAT; >> + else if (sysfs_streq(buf, buf_modes_str[ETR_MODE_ETR_SG]) && buf_hw.has_etr_sg) >> + drvdata->etr_mode = ETR_MODE_ETR_SG; >> + else if (sysfs_streq(buf, buf_modes_str[ETR_MODE_CATU]) && buf_hw.has_catu) >> + drvdata->etr_mode = ETR_MODE_CATU; >> + else if (sysfs_streq(buf, buf_modes_str[ETR_MODE_AUTO])) >> + drvdata->etr_mode = ETR_MODE_AUTO; >> + else >> + return -EINVAL; >> + >> + return size; >> + >> +} >> +static DEVICE_ATTR_RW(etr_buf_mode_current); >> + >> static struct attribute *coresight_tmc_attrs[] = { >> &dev_attr_trigger_cntr.attr, >> &dev_attr_buffer_size.attr, >> @@ -350,6 +430,24 @@ static const struct attribute_group *coresight_tmc_groups[] = { >> NULL, >> }; >> +static struct attribute *coresight_etr_attrs[] = { >> + &dev_attr_trigger_cntr.attr, >> + &dev_attr_buffer_size.attr, > > Why don't we reuse the tmc_attrs in the etr_groups ? That way, > it is much cleaner and easy to reason about. Also rename the I suppose you are suggesting to reuse coresight_tmc_group[] to bring in both 'trigger_cntr' and 'buffer_size' sysfs files for the ETR listing. Following change does what is required. diff --git a/drivers/hwtracing/coresight/coresight-tmc-core.c b/drivers/hwtracing/coresight/coresight-tmc-core.c index ad0efa528731..c8d108dd39d8 100644 --- a/drivers/hwtracing/coresight/coresight-tmc-core.c +++ b/drivers/hwtracing/coresight/coresight-tmc-core.c @@ -431,8 +431,6 @@ static const struct attribute_group *coresight_tmc_groups[] = { }; static struct attribute *coresight_etr_attrs[] = { - &dev_attr_trigger_cntr.attr, - &dev_attr_buffer_size.attr, &dev_attr_buf_modes_available.attr, &dev_attr_buf_mode_preferred.attr, NULL, @@ -444,6 +442,7 @@ static const struct attribute_group coresight_etr_group = { static const struct attribute_group *coresight_etr_groups[] = { &coresight_etr_group, + &coresight_tmc_group, &coresight_tmc_mgmt_group, NULL, }; > coresight_tmc_groups => coresight_etf_groups (inline with > the driver file name, coresight-tmc-etf.c ) Sure, will do that. > > >> + &dev_attr_etr_buf_modes_available.attr, >> + &dev_attr_etr_buf_mode_current.attr, >> + NULL, >> +}; >> + >> +static const struct attribute_group coresight_etr_group = { >> + .attrs = coresight_etr_attrs, >> +}; >> + >> +static const struct attribute_group *coresight_etr_groups[] = { >> + &coresight_etr_group, > > and add: > > + &coresight_tmc_group, Right, included in the change set as suggested. > >> + &coresight_tmc_mgmt_group, >> + NULL, >> +}; >> + > > All of the above functions and the coresight_etr_group and related attributes could live in coresight-tmc-etr.c and we could simply > expose the coresight_etr_group to the tmc-core.c > > That way, the code is all contained in coresight-tmc-etr.c and > you don't have to expose the functions way at the bottom. These attribute group assignments happen inside tmc_probe(), so the definitions need to be in the same file itself. This is applicable for both ETF and ETR. I am trying to see how this is beneficial. Besides, this reorganization could be accomplished in a subsequent patch and should not be included in this proposed change IMHO. > >> static inline bool tmc_etr_can_use_sg(struct device *dev) >> { >> return fwnode_property_present(dev->fwnode, "arm,scatter-gather"); >> @@ -465,6 +563,7 @@ static int tmc_probe(struct amba_device *adev, const struct amba_id *id) >> drvdata->memwidth = tmc_get_memwidth(devid); >> /* This device is not associated with a session */ >> drvdata->pid = -1; >> + drvdata->etr_mode = ETR_MODE_AUTO; >> if (drvdata->config_type == TMC_CONFIG_TYPE_ETR) { >> drvdata->size = tmc_etr_get_default_buffer_size(dev); >> @@ -474,16 +573,17 @@ static int tmc_probe(struct amba_device *adev, const struct amba_id *id) >> } >> desc.dev = dev; >> - desc.groups = coresight_tmc_groups; >> switch (drvdata->config_type) { >> case TMC_CONFIG_TYPE_ETB: >> + desc.groups = coresight_tmc_groups; >> desc.type = CORESIGHT_DEV_TYPE_SINK; >> desc.subtype.sink_subtype = CORESIGHT_DEV_SUBTYPE_SINK_BUFFER; >> desc.ops = &tmc_etb_cs_ops; >> dev_list = &etb_devs; >> break; >> case TMC_CONFIG_TYPE_ETR: >> + desc.groups = coresight_etr_groups; >> desc.type = CORESIGHT_DEV_TYPE_SINK; >> desc.subtype.sink_subtype = CORESIGHT_DEV_SUBTYPE_SINK_SYSMEM; >> desc.ops = &tmc_etr_cs_ops; >> @@ -496,6 +596,7 @@ static int tmc_probe(struct amba_device *adev, const struct amba_id *id) >> dev_list = &etr_devs; >> break; >> case TMC_CONFIG_TYPE_ETF: >> + desc.groups = coresight_tmc_groups; >> desc.type = CORESIGHT_DEV_TYPE_LINKSINK; >> desc.subtype.sink_subtype = CORESIGHT_DEV_SUBTYPE_SINK_BUFFER; >> desc.subtype.link_subtype = CORESIGHT_DEV_SUBTYPE_LINK_FIFO; >> diff --git a/drivers/hwtracing/coresight/coresight-tmc-etr.c b/drivers/hwtracing/coresight/coresight-tmc-etr.c >> index 766325de0e29..d48455188243 100644 >> --- a/drivers/hwtracing/coresight/coresight-tmc-etr.c >> +++ b/drivers/hwtracing/coresight/coresight-tmc-etr.c >> @@ -841,23 +841,27 @@ static struct etr_buf *tmc_alloc_etr_buf(struct tmc_drvdata *drvdata, >> int node, void **pages) >> { >> int rc = -ENOMEM; >> - bool has_etr_sg, has_iommu; >> - bool has_sg, has_catu; >> struct etr_buf *etr_buf; >> + struct etr_buf_hw buf_hw; >> struct device *dev = &drvdata->csdev->dev; >> - has_etr_sg = tmc_etr_has_cap(drvdata, TMC_ETR_SG); >> - has_iommu = iommu_get_domain_for_dev(dev->parent); >> - has_catu = !!tmc_etr_get_catu_device(drvdata); >> - >> - has_sg = has_catu || has_etr_sg; >> - >> + get_etr_buf_hw(dev, &buf_hw); >> etr_buf = kzalloc(sizeof(*etr_buf), GFP_KERNEL); >> if (!etr_buf) >> return ERR_PTR(-ENOMEM); >> etr_buf->size = size; >> + /* If there is user directive for buffer mode, try that first */ >> + if (drvdata->etr_mode != ETR_MODE_AUTO) { >> + rc = tmc_etr_mode_alloc_buf(drvdata->etr_mode, drvdata, >> + etr_buf, node, pages); > > As mentioned above we should fall back to the AUTO mode of action if the > above fails. Given the ETR could be used in sysfs (size via sysfs) vs > perf mode (size via perf aux_buf) and the sizes controlled by different > entities, a tunable set in the sysfs could cause failures. > > We should treat the user selection as a "preferred" mode and try that > first. If that is not available, we should fallback to the "auto" logic > (without resetting the preferred mode), skipping the "preferred" mode. > See below. > > >> + if (rc) { >> + kfree(etr_buf); >> + return ERR_PTR(rc);>> + } >> + } The suggested auto mode fallback could be achieved by just dropping off the above code hunk which tests 'rc' return value. >> + >> /* >> * If we have to use an existing list of pages, we cannot reliably >> * use a contiguous DMA memory (even if we have an IOMMU). Otherwise, >> @@ -870,14 +874,13 @@ static struct etr_buf *tmc_alloc_etr_buf(struct tmc_drvdata *drvdata, >> * Fallback to available mechanisms. >> * >> */ >> - if (!pages && >> - (!has_sg || has_iommu || size < SZ_1M)) >> + if (!pages && etr_supports_flat_mode(&buf_hw, size)) >> rc = tmc_etr_mode_alloc_buf(ETR_MODE_FLAT, drvdata, >> etr_buf, node, pages); >> - if (rc && has_etr_sg) >> + if (rc && buf_hw.has_etr_sg) >> rc = tmc_etr_mode_alloc_buf(ETR_MODE_ETR_SG, drvdata, >> etr_buf, node, pages); >> - if (rc && has_catu) >> + if (rc && buf_hw.has_catu) >> rc = tmc_etr_mode_alloc_buf(ETR_MODE_CATU, drvdata, >> etr_buf, node, pages); > > We could do : > > do { > if (etr_mode != ETR_MODE_FLAT && > !pages && etr_can_use_flat_mode(buf_hw, size)) > rc = tmc_etr_mode_alloc_buf(ETR_MODE_FLAT, drvdata, > etr_buf, node, pages); > if (!rc) > break; > if (etr_mode != ETR_MODE_ETR_SG && buf_hw.has_etr_sg) > rc = tmc_etr_mode_alloc_buf(ETR_MODE_ETR_SG, drvdata, > etr_buf, node, pages); > if (!rc) > break; > if (etr_mode != ETR_MODE_ETR_CATU && buf_hw.has_catu) > rc = tmc_etr_mode_alloc_buf(ETR_MODE_CATU, drvdata, > etr_buf, node, pages); > > } while (0); I guess you meant 'etr_mode ==' in all the above conditional checks instead ? Besides, what is benefit of converting all this into a do { } while (0) block apart from just those nice !rc breakouts ?
On 16/08/2023 06:39, Anshuman Khandual wrote: > > > On 8/11/23 20:03, Suzuki K Poulose wrote: >> On 28/07/2023 09:48, Anshuman Khandual wrote: >>> Currently TMC-ETR automatically selects the buffer mode from all available >>> methods in the following sequentially fallback manner - also in that order. >>> >>> 1. FLAT mode with or without IOMMU >>> 2. TMC-ETR-SG (scatter gather) mode when available >>> 3. CATU mode when available >>> >>> But this order might not be ideal for all situations. For example if there >>> is a CATU connected to ETR, it may be better to use TMC-ETR scatter gather >>> method, rather than CATU. >> >> The statement is wrong. >> >> But hard coding such order changes will prevent > >>> us from testing or using a particular mode. This change provides following >>> new sysfs tunables for the user to control TMC-ETR buffer mode explicitly, >>> if required. >>> >>> /sys/bus/coresight/devices/tmc_etr<N>/etr_buf_modes_available >>> /sys/bus/coresight/devices/tmc_etr<N>/etr_buf_mode_current >> >> Given this only appears for TMC-ETR, could this be simple : >> >> available_buf_modes and >> >> preferred_buf_mode. > > Make sense, will drop 'etr' from these sysfs file names, and update the documentation > as required. But Mike Leach had suggested earlier to have common sub strings right at > the beginning for both these sysfs file names. Hence will just keep them in the same > format i.e > > * buf_modes_available > * buf_mode_preferred (as you are suggesting to replace 'current' with 'preferred') > >> >> We should fall back to the auto logic to use an appropriate mode >> if the "perferred" mode cannot satisfy the request. (e.g., flat mode >> with a large buffer. This may be possible on a system without much >> load). > > That's a semantics change from the current proposal, which does not try to fallback on > anything, in case the requested buffer mode does not work. Instead it expects the user > to try a different buffer mode in such cases. True, but I prefer a fallback to something works mode, given there are different ways this can be used and leaving the control in an unusable state is not ideal. > >> >>> $ cat etr_buf_modes_available >>> auto flat tmc-sg catu ------------------> Supported TMC-ETR buffer modes >>> $ echo catu > etr_buf_mode_current -------> Explicit buffer mode request >>> >>> But explicit user request has to be within supported ETR buffer modes only. >>> These sysfs interface files are exclussive to ETR, and hence not available >>> for other TMC devices such as ETB or ETF etc. >> >> >> This required separating out >>> new 'coresight_etr_groups' from common 'coresight_tmc_groups'. >> >> strip this, you don't need implementation commentary. >> >>> >>> This adds a new element 'etr_mode' in 'struct tmc_drvdata' which will track >>> such explicit user directives. >> >> this too. > > Although I could understand not mentioning about refactoring 'coresight_tmc_groups', is > not this comment necessary in explaining why 'tmc_drvdata' structure is expanded here ? Not needed, instead the code and the comment in the structure is self explanatory. > >> >> 'auto' mode has been added to help fallback >>> to the existing default behaviour. >> >> >> ETR_MODE_FLAT mode availability follows >>> existing logic as in tmc_alloc_etr_buf() creating a common helper function >>> i.e etr_supports_flat_mode(). >> >> this too. > > I believe parts of the comments above in the commit message are still important but > will try and reorganize them better for clarity. > >> >>> >>> Cc: Suzuki K Poulose <suzuki.poulose@arm.com> >>> Cc: Mike Leach <mike.leach@linaro.org> >>> Cc: James Clark <james.clark@arm.com> >>> Cc: Leo Yan <leo.yan@linaro.org> >>> Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com> >>> Cc: coresight@lists.linaro.org >>> Cc: linux-arm-kernel@lists.infradead.org >>> Cc: linux-kernel@vger.kernel.org >>> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com> >>> --- >>> .../testing/sysfs-bus-coresight-devices-tmc | 16 +++ >>> .../hwtracing/coresight/coresight-tmc-core.c | 103 +++++++++++++++++- >>> .../hwtracing/coresight/coresight-tmc-etr.c | 27 +++-- >>> drivers/hwtracing/coresight/coresight-tmc.h | 10 ++ >>> 4 files changed, 143 insertions(+), 13 deletions(-) >>> >>> diff --git a/Documentation/ABI/testing/sysfs-bus-coresight-devices-tmc b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tmc >>> index 6aa527296c71..956a2f090950 100644 >>> --- a/Documentation/ABI/testing/sysfs-bus-coresight-devices-tmc >>> +++ b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tmc >>> @@ -91,3 +91,19 @@ Contact: Mathieu Poirier <mathieu.poirier@linaro.org> >>> Description: (RW) Size of the trace buffer for TMC-ETR when used in SYSFS >>> mode. Writable only for TMC-ETR configurations. The value >>> should be aligned to the kernel pagesize. >>> + >>> +What: /sys/bus/coresight/devices/<memory_map>.tmc/etr_buf_modes_available >>> +Date: July 2023 >>> +KernelVersion: 6.6 >>> +Contact: Anshuman Khandual <anshuman.khandual@arm.com> >>> +Description: (Read) Shows all supported Coresight TMC-ETR buffer modes available >>> + for the users to configure explicitly. This file is avaialble only >>> + for TMC ETR devices. >>> + >>> +What: /sys/bus/coresight/devices/<memory_map>.tmc/etr_buf_mode_current >>> +Date: July 2023 >>> +KernelVersion: 6.6 >>> +Contact: Anshuman Khandual <anshuman.khandual@arm.com> >>> +Description: (RW) Current Coresight TMC-ETR buffer mode selected. But user could >>> + only provide a mode which is supported for a given ETR device. This >>> + file is available only for TMC ETR devices. >>> diff --git a/drivers/hwtracing/coresight/coresight-tmc-core.c b/drivers/hwtracing/coresight/coresight-tmc-core.c >>> index c106d142e632..ce97ff5e0997 100644 >>> --- a/drivers/hwtracing/coresight/coresight-tmc-core.c >>> +++ b/drivers/hwtracing/coresight/coresight-tmc-core.c >>> @@ -10,6 +10,7 @@ >>> #include <linux/device.h> >>> #include <linux/idr.h> >>> #include <linux/io.h> >>> +#include <linux/iommu.h> >>> #include <linux/err.h> >>> #include <linux/fs.h> >>> #include <linux/miscdevice.h> >>> @@ -329,6 +330,85 @@ static ssize_t buffer_size_store(struct device *dev, >>> static DEVICE_ATTR_RW(buffer_size); >>> +static const char *const buf_modes_str[] = { >>> + [ETR_MODE_FLAT] = "flat", >>> + [ETR_MODE_ETR_SG] = "tmc-sg", >>> + [ETR_MODE_CATU] = "catu", >>> + [ETR_MODE_AUTO] = "auto", >>> +}; >>> + >>> +void get_etr_buf_hw(struct device *dev, struct etr_buf_hw *buf_hw) >>> +{ >>> + struct tmc_drvdata *drvdata = dev_get_drvdata(dev->parent); >>> + >>> + buf_hw->has_iommu = iommu_get_domain_for_dev(dev->parent); >>> + buf_hw->has_etr_sg = tmc_etr_has_cap(drvdata, TMC_ETR_SG); >>> + buf_hw->has_catu = !!tmc_etr_get_catu_device(drvdata); >>> +} >>> + >>> +bool etr_supports_flat_mode(struct etr_buf_hw *buf_hw, ssize_t etr_buf_size) >>> +{ >>> + bool has_sg = buf_hw->has_catu || buf_hw->has_etr_sg; >>> + >>> + return !has_sg || buf_hw->has_iommu || etr_buf_size < SZ_1M; >>> +} >>> + >> >> Flat mode is always supported and the user must be allowed to "prefer" >> it. This logic can be applied to the "auto mode" though and should be >> renamed to >> >> etr_can_use_flat_mode(buf_hw, size) > > Sure, will rename the function here but not sure if I understand this completely. > Are you suggesting anything else which needs to be done here ? My point is, this helper must be only used for "auto" mode. Not for etr_mode == flat. If the user requests "flat" try it (without checking the size required) and let it fallthrough to the auto mode. > >> >> >> >>> +static ssize_t etr_buf_modes_available_show(struct device *dev, >>> + struct device_attribute *attr, char *buf) >>> +{ >>> + struct tmc_drvdata *drvdata = dev_get_drvdata(dev->parent); >>> + struct etr_buf_hw buf_hw; >>> + ssize_t size = 0; >>> + >>> + get_etr_buf_hw(dev, &buf_hw); >>> + size += sysfs_emit(buf, "%s ", buf_modes_str[ETR_MODE_AUTO]); >>> + if (etr_supports_flat_mode(&buf_hw, drvdata->size)) >>> + size += sysfs_emit_at(buf, size, "%s ", buf_modes_str[ETR_MODE_FLAT]); >> >> Always supported and must be available > > But if etr_can_use_flat_mode() returns negative, should it still be shown here as > an option at all ? Could flat mode be offered for buffer mode preference, if it The point is the check is made on a "size" that is set via sysfs. For perf mode we may get a size from the perf handle. It doesn't make sense for denying the option based on the size. As I said above, "flat" mode is always supported and let the user select it (irrespective of the size) and try it (without checking the size). So, we don't need the check here, instead it can be used for the "auto" mode only. > cannot be used as determined via etr_can_use_flat_mode(). > >> >>> + >>> + if (buf_hw.has_etr_sg) >>> + size += sysfs_emit_at(buf, size, "%s ", buf_modes_str[ETR_MODE_ETR_SG]); >>> + >>> + if (buf_hw.has_catu) >>> + size += sysfs_emit_at(buf, size, "%s ", buf_modes_str[ETR_MODE_CATU]); >>> + >>> + size += sysfs_emit_at(buf, size, "\n"); >>> + return size; >>> +} >>> +static DEVICE_ATTR_RO(etr_buf_modes_available); >>> + >>> +static ssize_t etr_buf_mode_current_show(struct device *dev, >>> + struct device_attribute *attr, char *buf) >>> +{ >>> + struct tmc_drvdata *drvdata = dev_get_drvdata(dev->parent); >>> + >>> + return sysfs_emit(buf, "%s\n", buf_modes_str[drvdata->etr_mode]); >>> +} >>> + >>> +static ssize_t etr_buf_mode_current_store(struct device *dev, >>> + struct device_attribute *attr, >>> + const char *buf, size_t size) >>> +{ >>> + struct tmc_drvdata *drvdata = dev_get_drvdata(dev->parent); >>> + struct etr_buf_hw buf_hw; >>> + >>> + get_etr_buf_hw(dev, &buf_hw); >>> + if (sysfs_streq(buf, buf_modes_str[ETR_MODE_FLAT]) && >>> + etr_supports_flat_mode(&buf_hw, drvdata->size)) >> >> Please remove this check, given the input is a "preference" > > But could flat mode be accepted for preference, if it cannot be used as determined > via etr_can_use_flat_mode() ? As explained above, can_use_flat_mode() only for auto mode. > >> >>> + drvdata->etr_mode = ETR_MODE_FLAT; >>> + else if (sysfs_streq(buf, buf_modes_str[ETR_MODE_ETR_SG]) && buf_hw.has_etr_sg) >>> + drvdata->etr_mode = ETR_MODE_ETR_SG; >>> + else if (sysfs_streq(buf, buf_modes_str[ETR_MODE_CATU]) && buf_hw.has_catu) >>> + drvdata->etr_mode = ETR_MODE_CATU; >>> + else if (sysfs_streq(buf, buf_modes_str[ETR_MODE_AUTO])) >>> + drvdata->etr_mode = ETR_MODE_AUTO; >>> + else >>> + return -EINVAL; >>> + >>> + return size; >>> + >>> +} >>> +static DEVICE_ATTR_RW(etr_buf_mode_current); >>> + >>> static struct attribute *coresight_tmc_attrs[] = { >>> &dev_attr_trigger_cntr.attr, >>> &dev_attr_buffer_size.attr, >>> @@ -350,6 +430,24 @@ static const struct attribute_group *coresight_tmc_groups[] = { >>> NULL, >>> }; >>> +static struct attribute *coresight_etr_attrs[] = { >>> + &dev_attr_trigger_cntr.attr, >>> + &dev_attr_buffer_size.attr, >> >> Why don't we reuse the tmc_attrs in the etr_groups ? That way, >> it is much cleaner and easy to reason about. Also rename the > > I suppose you are suggesting to reuse coresight_tmc_group[] to bring in both 'trigger_cntr' > and 'buffer_size' sysfs files for the ETR listing. Following change does what is required. > > diff --git a/drivers/hwtracing/coresight/coresight-tmc-core.c b/drivers/hwtracing/coresight/coresight-tmc-core.c > index ad0efa528731..c8d108dd39d8 100644 > --- a/drivers/hwtracing/coresight/coresight-tmc-core.c > +++ b/drivers/hwtracing/coresight/coresight-tmc-core.c > @@ -431,8 +431,6 @@ static const struct attribute_group *coresight_tmc_groups[] = { > }; > > static struct attribute *coresight_etr_attrs[] = { > - &dev_attr_trigger_cntr.attr, > - &dev_attr_buffer_size.attr, > &dev_attr_buf_modes_available.attr, > &dev_attr_buf_mode_preferred.attr, > NULL, > @@ -444,6 +442,7 @@ static const struct attribute_group coresight_etr_group = { > > static const struct attribute_group *coresight_etr_groups[] = { > &coresight_etr_group, > + &coresight_tmc_group, > &coresight_tmc_mgmt_group, > NULL, > }; Yes > >> coresight_tmc_groups => coresight_etf_groups (inline with >> the driver file name, coresight-tmc-etf.c ) > > Sure, will do that. > >> >> >>> + &dev_attr_etr_buf_modes_available.attr, >>> + &dev_attr_etr_buf_mode_current.attr, >>> + NULL, >>> +}; >>> + >>> +static const struct attribute_group coresight_etr_group = { >>> + .attrs = coresight_etr_attrs, >>> +}; >>> + >>> +static const struct attribute_group *coresight_etr_groups[] = { >>> + &coresight_etr_group, >> >> and add: >> >> + &coresight_tmc_group, > > Right, included in the change set as suggested. > >> >>> + &coresight_tmc_mgmt_group, >>> + NULL, >>> +}; >>> + >> >> All of the above functions and the coresight_etr_group and related attributes could live in coresight-tmc-etr.c and we could simply >> expose the coresight_etr_group to the tmc-core.c >> >> That way, the code is all contained in coresight-tmc-etr.c and >> you don't have to expose the functions way at the bottom. > > These attribute group assignments happen inside tmc_probe(), so the definitions need > to be in the same file itself. This is applicable for both ETF and ETR. I am trying Surely could do : const struct attribute_group coresight_etr_group; and in the coresight-tmc-etr.c static struct attribute *coresight_etr_attrs[] = { &dev_attr_etr_buf_modes_available.attr, &dev_attr_etr_buf_mode_current.attr, }; const struct attribute_group coresight_etr_group = { .attrs = coresight_etr_attrs, }; > to see how this is beneficial. Besides, this reorganization could be accomplished in > a subsequent patch and should not be included in this proposed change IMHO. On the other hand you are moving a lot of ETR specific functions to this core file, which doesn't look nice. Instead, we simply make the coresight_etr_group global (not "exported"). > >> >>> static inline bool tmc_etr_can_use_sg(struct device *dev) >>> { >>> return fwnode_property_present(dev->fwnode, "arm,scatter-gather"); >>> @@ -465,6 +563,7 @@ static int tmc_probe(struct amba_device *adev, const struct amba_id *id) >>> drvdata->memwidth = tmc_get_memwidth(devid); >>> /* This device is not associated with a session */ >>> drvdata->pid = -1; >>> + drvdata->etr_mode = ETR_MODE_AUTO; >>> if (drvdata->config_type == TMC_CONFIG_TYPE_ETR) { >>> drvdata->size = tmc_etr_get_default_buffer_size(dev); >>> @@ -474,16 +573,17 @@ static int tmc_probe(struct amba_device *adev, const struct amba_id *id) >>> } >>> desc.dev = dev; >>> - desc.groups = coresight_tmc_groups; >>> switch (drvdata->config_type) { >>> case TMC_CONFIG_TYPE_ETB: >>> + desc.groups = coresight_tmc_groups; >>> desc.type = CORESIGHT_DEV_TYPE_SINK; >>> desc.subtype.sink_subtype = CORESIGHT_DEV_SUBTYPE_SINK_BUFFER; >>> desc.ops = &tmc_etb_cs_ops; >>> dev_list = &etb_devs; >>> break; >>> case TMC_CONFIG_TYPE_ETR: >>> + desc.groups = coresight_etr_groups; >>> desc.type = CORESIGHT_DEV_TYPE_SINK; >>> desc.subtype.sink_subtype = CORESIGHT_DEV_SUBTYPE_SINK_SYSMEM; >>> desc.ops = &tmc_etr_cs_ops; >>> @@ -496,6 +596,7 @@ static int tmc_probe(struct amba_device *adev, const struct amba_id *id) >>> dev_list = &etr_devs; >>> break; >>> case TMC_CONFIG_TYPE_ETF: >>> + desc.groups = coresight_tmc_groups; >>> desc.type = CORESIGHT_DEV_TYPE_LINKSINK; >>> desc.subtype.sink_subtype = CORESIGHT_DEV_SUBTYPE_SINK_BUFFER; >>> desc.subtype.link_subtype = CORESIGHT_DEV_SUBTYPE_LINK_FIFO; >>> diff --git a/drivers/hwtracing/coresight/coresight-tmc-etr.c b/drivers/hwtracing/coresight/coresight-tmc-etr.c >>> index 766325de0e29..d48455188243 100644 >>> --- a/drivers/hwtracing/coresight/coresight-tmc-etr.c >>> +++ b/drivers/hwtracing/coresight/coresight-tmc-etr.c >>> @@ -841,23 +841,27 @@ static struct etr_buf *tmc_alloc_etr_buf(struct tmc_drvdata *drvdata, >>> int node, void **pages) >>> { >>> int rc = -ENOMEM; >>> - bool has_etr_sg, has_iommu; >>> - bool has_sg, has_catu; >>> struct etr_buf *etr_buf; >>> + struct etr_buf_hw buf_hw; >>> struct device *dev = &drvdata->csdev->dev; >>> - has_etr_sg = tmc_etr_has_cap(drvdata, TMC_ETR_SG); >>> - has_iommu = iommu_get_domain_for_dev(dev->parent); >>> - has_catu = !!tmc_etr_get_catu_device(drvdata); >>> - >>> - has_sg = has_catu || has_etr_sg; >>> - >>> + get_etr_buf_hw(dev, &buf_hw); >>> etr_buf = kzalloc(sizeof(*etr_buf), GFP_KERNEL); >>> if (!etr_buf) >>> return ERR_PTR(-ENOMEM); >>> etr_buf->size = size; >>> + /* If there is user directive for buffer mode, try that first */ >>> + if (drvdata->etr_mode != ETR_MODE_AUTO) { >>> + rc = tmc_etr_mode_alloc_buf(drvdata->etr_mode, drvdata, >>> + etr_buf, node, pages); >> >> As mentioned above we should fall back to the AUTO mode of action if the >> above fails. Given the ETR could be used in sysfs (size via sysfs) vs >> perf mode (size via perf aux_buf) and the sizes controlled by different >> entities, a tunable set in the sysfs could cause failures. >> >> We should treat the user selection as a "preferred" mode and try that >> first. If that is not available, we should fallback to the "auto" logic >> (without resetting the preferred mode), skipping the "preferred" mode. >> See below. >> >> >>> + if (rc) { >>> + kfree(etr_buf); >>> + return ERR_PTR(rc);>> + } >>> + } > > The suggested auto mode fallback could be achieved by just dropping off the > above code hunk which tests 'rc' return value. > >>> + >>> /* >>> * If we have to use an existing list of pages, we cannot reliably >>> * use a contiguous DMA memory (even if we have an IOMMU). Otherwise, >>> @@ -870,14 +874,13 @@ static struct etr_buf *tmc_alloc_etr_buf(struct tmc_drvdata *drvdata, >>> * Fallback to available mechanisms. >>> * >>> */ >>> - if (!pages && >>> - (!has_sg || has_iommu || size < SZ_1M)) >>> + if (!pages && etr_supports_flat_mode(&buf_hw, size)) >>> rc = tmc_etr_mode_alloc_buf(ETR_MODE_FLAT, drvdata, >>> etr_buf, node, pages); >>> - if (rc && has_etr_sg) >>> + if (rc && buf_hw.has_etr_sg) >>> rc = tmc_etr_mode_alloc_buf(ETR_MODE_ETR_SG, drvdata, >>> etr_buf, node, pages); >>> - if (rc && has_catu) >>> + if (rc && buf_hw.has_catu) >>> rc = tmc_etr_mode_alloc_buf(ETR_MODE_CATU, drvdata, >>> etr_buf, node, pages); >> >> We could do : >> >> do { >> if (etr_mode != ETR_MODE_FLAT && >> !pages && etr_can_use_flat_mode(buf_hw, size)) >> rc = tmc_etr_mode_alloc_buf(ETR_MODE_FLAT, drvdata, >> etr_buf, node, pages); >> if (!rc) >> break; >> if (etr_mode != ETR_MODE_ETR_SG && buf_hw.has_etr_sg) >> rc = tmc_etr_mode_alloc_buf(ETR_MODE_ETR_SG, drvdata, >> etr_buf, node, pages); >> if (!rc) >> break; >> if (etr_mode != ETR_MODE_ETR_CATU && buf_hw.has_catu) >> rc = tmc_etr_mode_alloc_buf(ETR_MODE_CATU, drvdata, >> etr_buf, node, pages); >> >> } while (0); > > I guess you meant 'etr_mode ==' in all the above conditional checks instead ? Besides, I meant, etr_mode !=.. i.e., if the preferred mode is the same as the one we are trying to check, we could skip it as it already failed. > what is benefit of converting all this into a do { } while (0) block apart from just > those nice !rc breakouts ? To avoid the number of gotos. Or we could do : if (rc && ... Suzuki
On 8/17/23 19:25, Suzuki K Poulose wrote: > On 16/08/2023 06:39, Anshuman Khandual wrote: >> >> >> On 8/11/23 20:03, Suzuki K Poulose wrote: >>> On 28/07/2023 09:48, Anshuman Khandual wrote: >>>> Currently TMC-ETR automatically selects the buffer mode from all available >>>> methods in the following sequentially fallback manner - also in that order. >>>> >>>> 1. FLAT mode with or without IOMMU >>>> 2. TMC-ETR-SG (scatter gather) mode when available >>>> 3. CATU mode when available >>>> >>>> But this order might not be ideal for all situations. For example if there >>>> is a CATU connected to ETR, it may be better to use TMC-ETR scatter gather >>>> method, rather than CATU. >>> >>> The statement is wrong. >>> >>> But hard coding such order changes will prevent >> >>>> us from testing or using a particular mode. This change provides following >>>> new sysfs tunables for the user to control TMC-ETR buffer mode explicitly, >>>> if required. >>>> >>>> /sys/bus/coresight/devices/tmc_etr<N>/etr_buf_modes_available >>>> /sys/bus/coresight/devices/tmc_etr<N>/etr_buf_mode_current >>> >>> Given this only appears for TMC-ETR, could this be simple : >>> >>> available_buf_modes and >>> >>> preferred_buf_mode. >> >> Make sense, will drop 'etr' from these sysfs file names, and update the documentation >> as required. But Mike Leach had suggested earlier to have common sub strings right at >> the beginning for both these sysfs file names. Hence will just keep them in the same >> format i.e >> >> * buf_modes_available >> * buf_mode_preferred (as you are suggesting to replace 'current' with 'preferred') >> >>> >>> We should fall back to the auto logic to use an appropriate mode >>> if the "perferred" mode cannot satisfy the request. (e.g., flat mode >>> with a large buffer. This may be possible on a system without much >>> load). >> >> That's a semantics change from the current proposal, which does not try to fallback on >> anything, in case the requested buffer mode does not work. Instead it expects the user >> to try a different buffer mode in such cases. > > True, but I prefer a fallback to something works mode, given there are > different ways this can be used and leaving the control in an unusable > state is not ideal. Got it. > > >> >>> >>>> $ cat etr_buf_modes_available >>>> auto flat tmc-sg catu ------------------> Supported TMC-ETR buffer modes >>>> $ echo catu > etr_buf_mode_current -------> Explicit buffer mode request >>>> >>>> But explicit user request has to be within supported ETR buffer modes only. >>>> These sysfs interface files are exclussive to ETR, and hence not available >>>> for other TMC devices such as ETB or ETF etc. >>> >>> >>> This required separating out >>>> new 'coresight_etr_groups' from common 'coresight_tmc_groups'. >>> >>> strip this, you don't need implementation commentary. >>> >>>> >>>> This adds a new element 'etr_mode' in 'struct tmc_drvdata' which will track >>>> such explicit user directives. >>> >>> this too. >> >> Although I could understand not mentioning about refactoring 'coresight_tmc_groups', is >> not this comment necessary in explaining why 'tmc_drvdata' structure is expanded here ? > > Not needed, instead the code and the comment in the structure is self explanatory. > >> >>> >>> 'auto' mode has been added to help fallback >>>> to the existing default behaviour. >>> >>> >>> ETR_MODE_FLAT mode availability follows >>>> existing logic as in tmc_alloc_etr_buf() creating a common helper function >>>> i.e etr_supports_flat_mode(). >>> >>> this too. >> >> I believe parts of the comments above in the commit message are still important but >> will try and reorganize them better for clarity. >> >>> >>>> >>>> Cc: Suzuki K Poulose <suzuki.poulose@arm.com> >>>> Cc: Mike Leach <mike.leach@linaro.org> >>>> Cc: James Clark <james.clark@arm.com> >>>> Cc: Leo Yan <leo.yan@linaro.org> >>>> Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com> >>>> Cc: coresight@lists.linaro.org >>>> Cc: linux-arm-kernel@lists.infradead.org >>>> Cc: linux-kernel@vger.kernel.org >>>> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com> >>>> --- >>>> .../testing/sysfs-bus-coresight-devices-tmc | 16 +++ >>>> .../hwtracing/coresight/coresight-tmc-core.c | 103 +++++++++++++++++- >>>> .../hwtracing/coresight/coresight-tmc-etr.c | 27 +++-- >>>> drivers/hwtracing/coresight/coresight-tmc.h | 10 ++ >>>> 4 files changed, 143 insertions(+), 13 deletions(-) >>>> >>>> diff --git a/Documentation/ABI/testing/sysfs-bus-coresight-devices-tmc b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tmc >>>> index 6aa527296c71..956a2f090950 100644 >>>> --- a/Documentation/ABI/testing/sysfs-bus-coresight-devices-tmc >>>> +++ b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tmc >>>> @@ -91,3 +91,19 @@ Contact: Mathieu Poirier <mathieu.poirier@linaro.org> >>>> Description: (RW) Size of the trace buffer for TMC-ETR when used in SYSFS >>>> mode. Writable only for TMC-ETR configurations. The value >>>> should be aligned to the kernel pagesize. >>>> + >>>> +What: /sys/bus/coresight/devices/<memory_map>.tmc/etr_buf_modes_available >>>> +Date: July 2023 >>>> +KernelVersion: 6.6 >>>> +Contact: Anshuman Khandual <anshuman.khandual@arm.com> >>>> +Description: (Read) Shows all supported Coresight TMC-ETR buffer modes available >>>> + for the users to configure explicitly. This file is avaialble only >>>> + for TMC ETR devices. >>>> + >>>> +What: /sys/bus/coresight/devices/<memory_map>.tmc/etr_buf_mode_current >>>> +Date: July 2023 >>>> +KernelVersion: 6.6 >>>> +Contact: Anshuman Khandual <anshuman.khandual@arm.com> >>>> +Description: (RW) Current Coresight TMC-ETR buffer mode selected. But user could >>>> + only provide a mode which is supported for a given ETR device. This >>>> + file is available only for TMC ETR devices. >>>> diff --git a/drivers/hwtracing/coresight/coresight-tmc-core.c b/drivers/hwtracing/coresight/coresight-tmc-core.c >>>> index c106d142e632..ce97ff5e0997 100644 >>>> --- a/drivers/hwtracing/coresight/coresight-tmc-core.c >>>> +++ b/drivers/hwtracing/coresight/coresight-tmc-core.c >>>> @@ -10,6 +10,7 @@ >>>> #include <linux/device.h> >>>> #include <linux/idr.h> >>>> #include <linux/io.h> >>>> +#include <linux/iommu.h> >>>> #include <linux/err.h> >>>> #include <linux/fs.h> >>>> #include <linux/miscdevice.h> >>>> @@ -329,6 +330,85 @@ static ssize_t buffer_size_store(struct device *dev, >>>> static DEVICE_ATTR_RW(buffer_size); >>>> +static const char *const buf_modes_str[] = { >>>> + [ETR_MODE_FLAT] = "flat", >>>> + [ETR_MODE_ETR_SG] = "tmc-sg", >>>> + [ETR_MODE_CATU] = "catu", >>>> + [ETR_MODE_AUTO] = "auto", >>>> +}; >>>> + >>>> +void get_etr_buf_hw(struct device *dev, struct etr_buf_hw *buf_hw) >>>> +{ >>>> + struct tmc_drvdata *drvdata = dev_get_drvdata(dev->parent); >>>> + >>>> + buf_hw->has_iommu = iommu_get_domain_for_dev(dev->parent); >>>> + buf_hw->has_etr_sg = tmc_etr_has_cap(drvdata, TMC_ETR_SG); >>>> + buf_hw->has_catu = !!tmc_etr_get_catu_device(drvdata); >>>> +} >>>> + >>>> +bool etr_supports_flat_mode(struct etr_buf_hw *buf_hw, ssize_t etr_buf_size) >>>> +{ >>>> + bool has_sg = buf_hw->has_catu || buf_hw->has_etr_sg; >>>> + >>>> + return !has_sg || buf_hw->has_iommu || etr_buf_size < SZ_1M; >>>> +} >>>> + >>> >>> Flat mode is always supported and the user must be allowed to "prefer" >>> it. This logic can be applied to the "auto mode" though and should be >>> renamed to >>> >>> etr_can_use_flat_mode(buf_hw, size) >> >> Sure, will rename the function here but not sure if I understand this completely. >> Are you suggesting anything else which needs to be done here ? > > My point is, this helper must be only used for "auto" mode. Not > for etr_mode == flat. If the user requests "flat" try it (without > checking the size required) and let it fallthrough to the auto mode. Got it. > >> >>> >>> >>> >>>> +static ssize_t etr_buf_modes_available_show(struct device *dev, >>>> + struct device_attribute *attr, char *buf) >>>> +{ >>>> + struct tmc_drvdata *drvdata = dev_get_drvdata(dev->parent); >>>> + struct etr_buf_hw buf_hw; >>>> + ssize_t size = 0; >>>> + >>>> + get_etr_buf_hw(dev, &buf_hw); >>>> + size += sysfs_emit(buf, "%s ", buf_modes_str[ETR_MODE_AUTO]); >>>> + if (etr_supports_flat_mode(&buf_hw, drvdata->size)) >>>> + size += sysfs_emit_at(buf, size, "%s ", buf_modes_str[ETR_MODE_FLAT]); >>> >>> Always supported and must be available >> >> But if etr_can_use_flat_mode() returns negative, should it still be shown here as >> an option at all ? Could flat mode be offered for buffer mode preference, if it > > The point is the check is made on a "size" that is set via sysfs. For perf mode we may get a size from the perf handle. It doesn't make sense > for denying the option based on the size. > > As I said above, "flat" mode is always supported and let the user select > it (irrespective of the size) and try it (without checking the size). Understood. > > So, we don't need the check here, instead it can be used for the "auto" mode only. Understood. There will not be any etr_can_use_flat_mode() check in sysfs interface. Flat mode is always offered, and always accepted to be tried first irrespective of the size variable. > > >> cannot be used as determined via etr_can_use_flat_mode(). >> >>> >>>> + >>>> + if (buf_hw.has_etr_sg) >>>> + size += sysfs_emit_at(buf, size, "%s ", buf_modes_str[ETR_MODE_ETR_SG]); >>>> + >>>> + if (buf_hw.has_catu) >>>> + size += sysfs_emit_at(buf, size, "%s ", buf_modes_str[ETR_MODE_CATU]); >>>> + >>>> + size += sysfs_emit_at(buf, size, "\n"); >>>> + return size; >>>> +} >>>> +static DEVICE_ATTR_RO(etr_buf_modes_available); >>>> + >>>> +static ssize_t etr_buf_mode_current_show(struct device *dev, >>>> + struct device_attribute *attr, char *buf) >>>> +{ >>>> + struct tmc_drvdata *drvdata = dev_get_drvdata(dev->parent); >>>> + >>>> + return sysfs_emit(buf, "%s\n", buf_modes_str[drvdata->etr_mode]); >>>> +} >>>> + >>>> +static ssize_t etr_buf_mode_current_store(struct device *dev, >>>> + struct device_attribute *attr, >>>> + const char *buf, size_t size) >>>> +{ >>>> + struct tmc_drvdata *drvdata = dev_get_drvdata(dev->parent); >>>> + struct etr_buf_hw buf_hw; >>>> + >>>> + get_etr_buf_hw(dev, &buf_hw); >>>> + if (sysfs_streq(buf, buf_modes_str[ETR_MODE_FLAT]) && >>>> + etr_supports_flat_mode(&buf_hw, drvdata->size)) >>> >>> Please remove this check, given the input is a "preference" >> >> But could flat mode be accepted for preference, if it cannot be used as determined >> via etr_can_use_flat_mode() ? > > As explained above, can_use_flat_mode() only for auto mode. Got it. > >> >>> >>>> + drvdata->etr_mode = ETR_MODE_FLAT; >>>> + else if (sysfs_streq(buf, buf_modes_str[ETR_MODE_ETR_SG]) && buf_hw.has_etr_sg) >>>> + drvdata->etr_mode = ETR_MODE_ETR_SG; >>>> + else if (sysfs_streq(buf, buf_modes_str[ETR_MODE_CATU]) && buf_hw.has_catu) >>>> + drvdata->etr_mode = ETR_MODE_CATU; >>>> + else if (sysfs_streq(buf, buf_modes_str[ETR_MODE_AUTO])) >>>> + drvdata->etr_mode = ETR_MODE_AUTO; >>>> + else >>>> + return -EINVAL; >>>> + >>>> + return size; >>>> + >>>> +} >>>> +static DEVICE_ATTR_RW(etr_buf_mode_current); >>>> + >>>> static struct attribute *coresight_tmc_attrs[] = { >>>> &dev_attr_trigger_cntr.attr, >>>> &dev_attr_buffer_size.attr, >>>> @@ -350,6 +430,24 @@ static const struct attribute_group *coresight_tmc_groups[] = { >>>> NULL, >>>> }; >>>> +static struct attribute *coresight_etr_attrs[] = { >>>> + &dev_attr_trigger_cntr.attr, >>>> + &dev_attr_buffer_size.attr, >>> >>> Why don't we reuse the tmc_attrs in the etr_groups ? That way, >>> it is much cleaner and easy to reason about. Also rename the >> >> I suppose you are suggesting to reuse coresight_tmc_group[] to bring in both 'trigger_cntr' >> and 'buffer_size' sysfs files for the ETR listing. Following change does what is required. >> >> diff --git a/drivers/hwtracing/coresight/coresight-tmc-core.c b/drivers/hwtracing/coresight/coresight-tmc-core.c >> index ad0efa528731..c8d108dd39d8 100644 >> --- a/drivers/hwtracing/coresight/coresight-tmc-core.c >> +++ b/drivers/hwtracing/coresight/coresight-tmc-core.c >> @@ -431,8 +431,6 @@ static const struct attribute_group *coresight_tmc_groups[] = { >> }; >> static struct attribute *coresight_etr_attrs[] = { >> - &dev_attr_trigger_cntr.attr, >> - &dev_attr_buffer_size.attr, >> &dev_attr_buf_modes_available.attr, >> &dev_attr_buf_mode_preferred.attr, >> NULL, >> @@ -444,6 +442,7 @@ static const struct attribute_group coresight_etr_group = { >> static const struct attribute_group *coresight_etr_groups[] = { >> &coresight_etr_group, >> + &coresight_tmc_group, >> &coresight_tmc_mgmt_group, >> NULL, >> }; > > Yes > >> >>> coresight_tmc_groups => coresight_etf_groups (inline with >>> the driver file name, coresight-tmc-etf.c ) >> >> Sure, will do that. >> >>> >>> >>>> + &dev_attr_etr_buf_modes_available.attr, >>>> + &dev_attr_etr_buf_mode_current.attr, >>>> + NULL, >>>> +}; >>>> + >>>> +static const struct attribute_group coresight_etr_group = { >>>> + .attrs = coresight_etr_attrs, >>>> +}; >>>> + >>>> +static const struct attribute_group *coresight_etr_groups[] = { >>>> + &coresight_etr_group, >>> >>> and add: >>> >>> + &coresight_tmc_group, >> >> Right, included in the change set as suggested. >> >>> >>>> + &coresight_tmc_mgmt_group, >>>> + NULL, >>>> +}; >>>> + >>> >>> All of the above functions and the coresight_etr_group and related attributes could live in coresight-tmc-etr.c and we could simply >>> expose the coresight_etr_group to the tmc-core.c >>> >>> That way, the code is all contained in coresight-tmc-etr.c and >>> you don't have to expose the functions way at the bottom. >> >> These attribute group assignments happen inside tmc_probe(), so the definitions need >> to be in the same file itself. This is applicable for both ETF and ETR. I am trying > > Surely could do : > > const struct attribute_group coresight_etr_group; > > and in the coresight-tmc-etr.c > > static struct attribute *coresight_etr_attrs[] = { > &dev_attr_etr_buf_modes_available.attr, > &dev_attr_etr_buf_mode_current.attr, > }; > > const struct attribute_group coresight_etr_group = { > .attrs = coresight_etr_attrs, > }; > > >> to see how this is beneficial. Besides, this reorganization could be accomplished in >> a subsequent patch and should not be included in this proposed change IMHO. > > On the other hand you are moving a lot of ETR specific functions to this > core file, which doesn't look nice. Instead, we simply make the coresight_etr_group global (not "exported"). Then bring it inside coresight-tmc-core.c via an extern declaration through coresight-tmc.h header. extern const struct attribute_group coresight_etr_group Hence after this change only the high level attribute groups for etf and etr remain in coresight-tmc-core.c, as expected. static const struct attribute_group *coresight_etf_groups[] = { &coresight_tmc_group, &coresight_tmc_mgmt_group, NULL, }; static const struct attribute_group *coresight_etr_groups[] = { &coresight_etr_group, &coresight_tmc_group, &coresight_tmc_mgmt_group, NULL, }; Where as coresight_etr_attrs[], and coresight_etr_group is defined explicitly along with their relevant helpers, inside coresight-tmc-etr.c static struct attribute *coresight_etr_attrs[] = { &dev_attr_buf_modes_available.attr, &dev_attr_buf_mode_preferred.attr, NULL, }; const struct attribute_group coresight_etr_group = { .attrs = coresight_etr_attrs, }; > >> >>> >>>> static inline bool tmc_etr_can_use_sg(struct device *dev) >>>> { >>>> return fwnode_property_present(dev->fwnode, "arm,scatter-gather"); >>>> @@ -465,6 +563,7 @@ static int tmc_probe(struct amba_device *adev, const struct amba_id *id) >>>> drvdata->memwidth = tmc_get_memwidth(devid); >>>> /* This device is not associated with a session */ >>>> drvdata->pid = -1; >>>> + drvdata->etr_mode = ETR_MODE_AUTO; >>>> if (drvdata->config_type == TMC_CONFIG_TYPE_ETR) { >>>> drvdata->size = tmc_etr_get_default_buffer_size(dev); >>>> @@ -474,16 +573,17 @@ static int tmc_probe(struct amba_device *adev, const struct amba_id *id) >>>> } >>>> desc.dev = dev; >>>> - desc.groups = coresight_tmc_groups; >>>> switch (drvdata->config_type) { >>>> case TMC_CONFIG_TYPE_ETB: >>>> + desc.groups = coresight_tmc_groups; >>>> desc.type = CORESIGHT_DEV_TYPE_SINK; >>>> desc.subtype.sink_subtype = CORESIGHT_DEV_SUBTYPE_SINK_BUFFER; >>>> desc.ops = &tmc_etb_cs_ops; >>>> dev_list = &etb_devs; >>>> break; >>>> case TMC_CONFIG_TYPE_ETR: >>>> + desc.groups = coresight_etr_groups; >>>> desc.type = CORESIGHT_DEV_TYPE_SINK; >>>> desc.subtype.sink_subtype = CORESIGHT_DEV_SUBTYPE_SINK_SYSMEM; >>>> desc.ops = &tmc_etr_cs_ops; >>>> @@ -496,6 +596,7 @@ static int tmc_probe(struct amba_device *adev, const struct amba_id *id) >>>> dev_list = &etr_devs; >>>> break; >>>> case TMC_CONFIG_TYPE_ETF: >>>> + desc.groups = coresight_tmc_groups; >>>> desc.type = CORESIGHT_DEV_TYPE_LINKSINK; >>>> desc.subtype.sink_subtype = CORESIGHT_DEV_SUBTYPE_SINK_BUFFER; >>>> desc.subtype.link_subtype = CORESIGHT_DEV_SUBTYPE_LINK_FIFO; >>>> diff --git a/drivers/hwtracing/coresight/coresight-tmc-etr.c b/drivers/hwtracing/coresight/coresight-tmc-etr.c >>>> index 766325de0e29..d48455188243 100644 >>>> --- a/drivers/hwtracing/coresight/coresight-tmc-etr.c >>>> +++ b/drivers/hwtracing/coresight/coresight-tmc-etr.c >>>> @@ -841,23 +841,27 @@ static struct etr_buf *tmc_alloc_etr_buf(struct tmc_drvdata *drvdata, >>>> int node, void **pages) >>>> { >>>> int rc = -ENOMEM; >>>> - bool has_etr_sg, has_iommu; >>>> - bool has_sg, has_catu; >>>> struct etr_buf *etr_buf; >>>> + struct etr_buf_hw buf_hw; >>>> struct device *dev = &drvdata->csdev->dev; >>>> - has_etr_sg = tmc_etr_has_cap(drvdata, TMC_ETR_SG); >>>> - has_iommu = iommu_get_domain_for_dev(dev->parent); >>>> - has_catu = !!tmc_etr_get_catu_device(drvdata); >>>> - >>>> - has_sg = has_catu || has_etr_sg; >>>> - >>>> + get_etr_buf_hw(dev, &buf_hw); >>>> etr_buf = kzalloc(sizeof(*etr_buf), GFP_KERNEL); >>>> if (!etr_buf) >>>> return ERR_PTR(-ENOMEM); >>>> etr_buf->size = size; >>>> + /* If there is user directive for buffer mode, try that first */ >>>> + if (drvdata->etr_mode != ETR_MODE_AUTO) { >>>> + rc = tmc_etr_mode_alloc_buf(drvdata->etr_mode, drvdata, >>>> + etr_buf, node, pages); >>> >>> As mentioned above we should fall back to the AUTO mode of action if the >>> above fails. Given the ETR could be used in sysfs (size via sysfs) vs >>> perf mode (size via perf aux_buf) and the sizes controlled by different >>> entities, a tunable set in the sysfs could cause failures. >>> >>> We should treat the user selection as a "preferred" mode and try that >>> first. If that is not available, we should fallback to the "auto" logic >>> (without resetting the preferred mode), skipping the "preferred" mode. >>> See below. >>> >>> >>>> + if (rc) { >>>> + kfree(etr_buf); >>>> + return ERR_PTR(rc);>> + } >>>> + } >> >> The suggested auto mode fallback could be achieved by just dropping off the >> above code hunk which tests 'rc' return value. >> >>>> + >>>> /* >>>> * If we have to use an existing list of pages, we cannot reliably >>>> * use a contiguous DMA memory (even if we have an IOMMU). Otherwise, >>>> @@ -870,14 +874,13 @@ static struct etr_buf *tmc_alloc_etr_buf(struct tmc_drvdata *drvdata, >>>> * Fallback to available mechanisms. >>>> * >>>> */ >>>> - if (!pages && >>>> - (!has_sg || has_iommu || size < SZ_1M)) >>>> + if (!pages && etr_supports_flat_mode(&buf_hw, size)) >>>> rc = tmc_etr_mode_alloc_buf(ETR_MODE_FLAT, drvdata, >>>> etr_buf, node, pages); >>>> - if (rc && has_etr_sg) >>>> + if (rc && buf_hw.has_etr_sg) >>>> rc = tmc_etr_mode_alloc_buf(ETR_MODE_ETR_SG, drvdata, >>>> etr_buf, node, pages); >>>> - if (rc && has_catu) >>>> + if (rc && buf_hw.has_catu) >>>> rc = tmc_etr_mode_alloc_buf(ETR_MODE_CATU, drvdata, >>>> etr_buf, node, pages); >>> >>> We could do : >>> >>> do { >>> if (etr_mode != ETR_MODE_FLAT && >>> !pages && etr_can_use_flat_mode(buf_hw, size)) >>> rc = tmc_etr_mode_alloc_buf(ETR_MODE_FLAT, drvdata, >>> etr_buf, node, pages); >>> if (!rc) >>> break; >>> if (etr_mode != ETR_MODE_ETR_SG && buf_hw.has_etr_sg) >>> rc = tmc_etr_mode_alloc_buf(ETR_MODE_ETR_SG, drvdata, >>> etr_buf, node, pages); >>> if (!rc) >>> break; >>> if (etr_mode != ETR_MODE_ETR_CATU && buf_hw.has_catu) >>> rc = tmc_etr_mode_alloc_buf(ETR_MODE_CATU, drvdata, >>> etr_buf, node, pages); >>> >>> } while (0); >> >> I guess you meant 'etr_mode ==' in all the above conditional checks instead ? Besides, > > I meant, etr_mode !=.. i.e., if the preferred mode is the same as the one we are trying to check, we could skip it as it already failed. Understood but seems more convoluted though. > >> what is benefit of converting all this into a do { } while (0) block apart from just >> those nice !rc breakouts ? > > To avoid the number of gotos. Or we could do : > > if (rc && ... That looks simpler and less code churn as well.
diff --git a/Documentation/ABI/testing/sysfs-bus-coresight-devices-tmc b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tmc index 6aa527296c71..956a2f090950 100644 --- a/Documentation/ABI/testing/sysfs-bus-coresight-devices-tmc +++ b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tmc @@ -91,3 +91,19 @@ Contact: Mathieu Poirier <mathieu.poirier@linaro.org> Description: (RW) Size of the trace buffer for TMC-ETR when used in SYSFS mode. Writable only for TMC-ETR configurations. The value should be aligned to the kernel pagesize. + +What: /sys/bus/coresight/devices/<memory_map>.tmc/etr_buf_modes_available +Date: July 2023 +KernelVersion: 6.6 +Contact: Anshuman Khandual <anshuman.khandual@arm.com> +Description: (Read) Shows all supported Coresight TMC-ETR buffer modes available + for the users to configure explicitly. This file is avaialble only + for TMC ETR devices. + +What: /sys/bus/coresight/devices/<memory_map>.tmc/etr_buf_mode_current +Date: July 2023 +KernelVersion: 6.6 +Contact: Anshuman Khandual <anshuman.khandual@arm.com> +Description: (RW) Current Coresight TMC-ETR buffer mode selected. But user could + only provide a mode which is supported for a given ETR device. This + file is available only for TMC ETR devices. diff --git a/drivers/hwtracing/coresight/coresight-tmc-core.c b/drivers/hwtracing/coresight/coresight-tmc-core.c index c106d142e632..ce97ff5e0997 100644 --- a/drivers/hwtracing/coresight/coresight-tmc-core.c +++ b/drivers/hwtracing/coresight/coresight-tmc-core.c @@ -10,6 +10,7 @@ #include <linux/device.h> #include <linux/idr.h> #include <linux/io.h> +#include <linux/iommu.h> #include <linux/err.h> #include <linux/fs.h> #include <linux/miscdevice.h> @@ -329,6 +330,85 @@ static ssize_t buffer_size_store(struct device *dev, static DEVICE_ATTR_RW(buffer_size); +static const char *const buf_modes_str[] = { + [ETR_MODE_FLAT] = "flat", + [ETR_MODE_ETR_SG] = "tmc-sg", + [ETR_MODE_CATU] = "catu", + [ETR_MODE_AUTO] = "auto", +}; + +void get_etr_buf_hw(struct device *dev, struct etr_buf_hw *buf_hw) +{ + struct tmc_drvdata *drvdata = dev_get_drvdata(dev->parent); + + buf_hw->has_iommu = iommu_get_domain_for_dev(dev->parent); + buf_hw->has_etr_sg = tmc_etr_has_cap(drvdata, TMC_ETR_SG); + buf_hw->has_catu = !!tmc_etr_get_catu_device(drvdata); +} + +bool etr_supports_flat_mode(struct etr_buf_hw *buf_hw, ssize_t etr_buf_size) +{ + bool has_sg = buf_hw->has_catu || buf_hw->has_etr_sg; + + return !has_sg || buf_hw->has_iommu || etr_buf_size < SZ_1M; +} + +static ssize_t etr_buf_modes_available_show(struct device *dev, + struct device_attribute *attr, char *buf) +{ + struct tmc_drvdata *drvdata = dev_get_drvdata(dev->parent); + struct etr_buf_hw buf_hw; + ssize_t size = 0; + + get_etr_buf_hw(dev, &buf_hw); + size += sysfs_emit(buf, "%s ", buf_modes_str[ETR_MODE_AUTO]); + if (etr_supports_flat_mode(&buf_hw, drvdata->size)) + size += sysfs_emit_at(buf, size, "%s ", buf_modes_str[ETR_MODE_FLAT]); + + if (buf_hw.has_etr_sg) + size += sysfs_emit_at(buf, size, "%s ", buf_modes_str[ETR_MODE_ETR_SG]); + + if (buf_hw.has_catu) + size += sysfs_emit_at(buf, size, "%s ", buf_modes_str[ETR_MODE_CATU]); + + size += sysfs_emit_at(buf, size, "\n"); + return size; +} +static DEVICE_ATTR_RO(etr_buf_modes_available); + +static ssize_t etr_buf_mode_current_show(struct device *dev, + struct device_attribute *attr, char *buf) +{ + struct tmc_drvdata *drvdata = dev_get_drvdata(dev->parent); + + return sysfs_emit(buf, "%s\n", buf_modes_str[drvdata->etr_mode]); +} + +static ssize_t etr_buf_mode_current_store(struct device *dev, + struct device_attribute *attr, + const char *buf, size_t size) +{ + struct tmc_drvdata *drvdata = dev_get_drvdata(dev->parent); + struct etr_buf_hw buf_hw; + + get_etr_buf_hw(dev, &buf_hw); + if (sysfs_streq(buf, buf_modes_str[ETR_MODE_FLAT]) && + etr_supports_flat_mode(&buf_hw, drvdata->size)) + drvdata->etr_mode = ETR_MODE_FLAT; + else if (sysfs_streq(buf, buf_modes_str[ETR_MODE_ETR_SG]) && buf_hw.has_etr_sg) + drvdata->etr_mode = ETR_MODE_ETR_SG; + else if (sysfs_streq(buf, buf_modes_str[ETR_MODE_CATU]) && buf_hw.has_catu) + drvdata->etr_mode = ETR_MODE_CATU; + else if (sysfs_streq(buf, buf_modes_str[ETR_MODE_AUTO])) + drvdata->etr_mode = ETR_MODE_AUTO; + else + return -EINVAL; + + return size; + +} +static DEVICE_ATTR_RW(etr_buf_mode_current); + static struct attribute *coresight_tmc_attrs[] = { &dev_attr_trigger_cntr.attr, &dev_attr_buffer_size.attr, @@ -350,6 +430,24 @@ static const struct attribute_group *coresight_tmc_groups[] = { NULL, }; +static struct attribute *coresight_etr_attrs[] = { + &dev_attr_trigger_cntr.attr, + &dev_attr_buffer_size.attr, + &dev_attr_etr_buf_modes_available.attr, + &dev_attr_etr_buf_mode_current.attr, + NULL, +}; + +static const struct attribute_group coresight_etr_group = { + .attrs = coresight_etr_attrs, +}; + +static const struct attribute_group *coresight_etr_groups[] = { + &coresight_etr_group, + &coresight_tmc_mgmt_group, + NULL, +}; + static inline bool tmc_etr_can_use_sg(struct device *dev) { return fwnode_property_present(dev->fwnode, "arm,scatter-gather"); @@ -465,6 +563,7 @@ static int tmc_probe(struct amba_device *adev, const struct amba_id *id) drvdata->memwidth = tmc_get_memwidth(devid); /* This device is not associated with a session */ drvdata->pid = -1; + drvdata->etr_mode = ETR_MODE_AUTO; if (drvdata->config_type == TMC_CONFIG_TYPE_ETR) { drvdata->size = tmc_etr_get_default_buffer_size(dev); @@ -474,16 +573,17 @@ static int tmc_probe(struct amba_device *adev, const struct amba_id *id) } desc.dev = dev; - desc.groups = coresight_tmc_groups; switch (drvdata->config_type) { case TMC_CONFIG_TYPE_ETB: + desc.groups = coresight_tmc_groups; desc.type = CORESIGHT_DEV_TYPE_SINK; desc.subtype.sink_subtype = CORESIGHT_DEV_SUBTYPE_SINK_BUFFER; desc.ops = &tmc_etb_cs_ops; dev_list = &etb_devs; break; case TMC_CONFIG_TYPE_ETR: + desc.groups = coresight_etr_groups; desc.type = CORESIGHT_DEV_TYPE_SINK; desc.subtype.sink_subtype = CORESIGHT_DEV_SUBTYPE_SINK_SYSMEM; desc.ops = &tmc_etr_cs_ops; @@ -496,6 +596,7 @@ static int tmc_probe(struct amba_device *adev, const struct amba_id *id) dev_list = &etr_devs; break; case TMC_CONFIG_TYPE_ETF: + desc.groups = coresight_tmc_groups; desc.type = CORESIGHT_DEV_TYPE_LINKSINK; desc.subtype.sink_subtype = CORESIGHT_DEV_SUBTYPE_SINK_BUFFER; desc.subtype.link_subtype = CORESIGHT_DEV_SUBTYPE_LINK_FIFO; diff --git a/drivers/hwtracing/coresight/coresight-tmc-etr.c b/drivers/hwtracing/coresight/coresight-tmc-etr.c index 766325de0e29..d48455188243 100644 --- a/drivers/hwtracing/coresight/coresight-tmc-etr.c +++ b/drivers/hwtracing/coresight/coresight-tmc-etr.c @@ -841,23 +841,27 @@ static struct etr_buf *tmc_alloc_etr_buf(struct tmc_drvdata *drvdata, int node, void **pages) { int rc = -ENOMEM; - bool has_etr_sg, has_iommu; - bool has_sg, has_catu; struct etr_buf *etr_buf; + struct etr_buf_hw buf_hw; struct device *dev = &drvdata->csdev->dev; - has_etr_sg = tmc_etr_has_cap(drvdata, TMC_ETR_SG); - has_iommu = iommu_get_domain_for_dev(dev->parent); - has_catu = !!tmc_etr_get_catu_device(drvdata); - - has_sg = has_catu || has_etr_sg; - + get_etr_buf_hw(dev, &buf_hw); etr_buf = kzalloc(sizeof(*etr_buf), GFP_KERNEL); if (!etr_buf) return ERR_PTR(-ENOMEM); etr_buf->size = size; + /* If there is user directive for buffer mode, try that first */ + if (drvdata->etr_mode != ETR_MODE_AUTO) { + rc = tmc_etr_mode_alloc_buf(drvdata->etr_mode, drvdata, + etr_buf, node, pages); + if (rc) { + kfree(etr_buf); + return ERR_PTR(rc); + } + } + /* * If we have to use an existing list of pages, we cannot reliably * use a contiguous DMA memory (even if we have an IOMMU). Otherwise, @@ -870,14 +874,13 @@ static struct etr_buf *tmc_alloc_etr_buf(struct tmc_drvdata *drvdata, * Fallback to available mechanisms. * */ - if (!pages && - (!has_sg || has_iommu || size < SZ_1M)) + if (!pages && etr_supports_flat_mode(&buf_hw, size)) rc = tmc_etr_mode_alloc_buf(ETR_MODE_FLAT, drvdata, etr_buf, node, pages); - if (rc && has_etr_sg) + if (rc && buf_hw.has_etr_sg) rc = tmc_etr_mode_alloc_buf(ETR_MODE_ETR_SG, drvdata, etr_buf, node, pages); - if (rc && has_catu) + if (rc && buf_hw.has_catu) rc = tmc_etr_mode_alloc_buf(ETR_MODE_CATU, drvdata, etr_buf, node, pages); if (rc) { diff --git a/drivers/hwtracing/coresight/coresight-tmc.h b/drivers/hwtracing/coresight/coresight-tmc.h index b97da39652d2..ca15ccb1d807 100644 --- a/drivers/hwtracing/coresight/coresight-tmc.h +++ b/drivers/hwtracing/coresight/coresight-tmc.h @@ -135,6 +135,13 @@ enum etr_mode { ETR_MODE_FLAT, /* Uses contiguous flat buffer */ ETR_MODE_ETR_SG, /* Uses in-built TMC ETR SG mechanism */ ETR_MODE_CATU, /* Use SG mechanism in CATU */ + ETR_MODE_AUTO, /* Use the default mechanism */ +}; + +struct etr_buf_hw { + bool has_iommu; + bool has_etr_sg; + bool has_catu; }; struct etr_buf_operations; @@ -207,6 +214,7 @@ struct tmc_drvdata { enum tmc_mem_intf_width memwidth; u32 trigger_cntr; u32 etr_caps; + enum etr_mode etr_mode; struct idr idr; struct mutex idr_mutex; struct etr_buf *sysfs_buf; @@ -334,5 +342,7 @@ void tmc_etr_set_catu_ops(const struct etr_buf_operations *catu); void tmc_etr_remove_catu_ops(void); struct etr_buf *tmc_etr_get_buffer(struct coresight_device *csdev, enum cs_mode mode, void *data); +void get_etr_buf_hw(struct device *dev, struct etr_buf_hw *buf_hw); +bool etr_supports_flat_mode(struct etr_buf_hw *buf_hw, ssize_t etr_buf_size); #endif
Currently TMC-ETR automatically selects the buffer mode from all available methods in the following sequentially fallback manner - also in that order. 1. FLAT mode with or without IOMMU 2. TMC-ETR-SG (scatter gather) mode when available 3. CATU mode when available But this order might not be ideal for all situations. For example if there is a CATU connected to ETR, it may be better to use TMC-ETR scatter gather method, rather than CATU. But hard coding such order changes will prevent us from testing or using a particular mode. This change provides following new sysfs tunables for the user to control TMC-ETR buffer mode explicitly, if required. /sys/bus/coresight/devices/tmc_etr<N>/etr_buf_modes_available /sys/bus/coresight/devices/tmc_etr<N>/etr_buf_mode_current $ cat etr_buf_modes_available auto flat tmc-sg catu ------------------> Supported TMC-ETR buffer modes $ echo catu > etr_buf_mode_current -------> Explicit buffer mode request But explicit user request has to be within supported ETR buffer modes only. These sysfs interface files are exclussive to ETR, and hence not available for other TMC devices such as ETB or ETF etc. This required separating out new 'coresight_etr_groups' from common 'coresight_tmc_groups'. This adds a new element 'etr_mode' in 'struct tmc_drvdata' which will track such explicit user directives. 'auto' mode has been added to help fallback to the existing default behaviour. ETR_MODE_FLAT mode availability follows existing logic as in tmc_alloc_etr_buf() creating a common helper function i.e etr_supports_flat_mode(). Cc: Suzuki K Poulose <suzuki.poulose@arm.com> Cc: Mike Leach <mike.leach@linaro.org> Cc: James Clark <james.clark@arm.com> Cc: Leo Yan <leo.yan@linaro.org> Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com> Cc: coresight@lists.linaro.org Cc: linux-arm-kernel@lists.infradead.org Cc: linux-kernel@vger.kernel.org Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com> --- .../testing/sysfs-bus-coresight-devices-tmc | 16 +++ .../hwtracing/coresight/coresight-tmc-core.c | 103 +++++++++++++++++- .../hwtracing/coresight/coresight-tmc-etr.c | 27 +++-- drivers/hwtracing/coresight/coresight-tmc.h | 10 ++ 4 files changed, 143 insertions(+), 13 deletions(-)