[PATCH] coresight: etm4x: add CPU hotplug support for probing
Suzuki K Poulose
suzuki.poulose at arm.com
Thu Jun 23 02:25:30 PDT 2022
Hi Tamas
Thanks for the patch. I have tested this on my Juno board and
it works fine. I have some comments inline.
On 16/06/2022 16:18, Tamas Zsoldos wrote:
> etm4x devices cannot be successfully probed when their CPU is offline.
> This adds a mechanism to delay the probing in this case and try again
> once the CPU is brought online.
It may be worth elaborating this a bit. e.g,
"For e.g., if the system is booted with maxcpus=n, the ETM won't
be probed on CPUs n+1, even when the CPUs are brought up online
later"
, >
> Signed-off-by: Tamas Zsoldos <tamas.zsoldos at arm.com>
> ---
> .../coresight/coresight-etm4x-core.c | 195 ++++++++++++++----
> 1 file changed, 155 insertions(+), 40 deletions(-)
>
> diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c
> index 7f416a12000e..f0bff08e9de9 100644
> --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c
> +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c
> @@ -70,6 +70,14 @@ struct etm4_init_arg {
> struct csdev_access *csa;
> };
>
> +struct etm4_delayed_probe {
> + u32 etm_pid;
> + struct device *dev;
> +};
> +
> +static DEFINE_PER_CPU(struct etm4_delayed_probe *, delayed_probe);
> +static enum cpuhp_state hp_probe;
I am wondering if we can reuse the existing hotplug notifier ? See
more on this below.
> +
> /*
> * Check if TRCSSPCICRn(i) is implemented for a given instance.
> *
> @@ -1938,48 +1946,20 @@ static void etm4_pm_clear(void)
> }
> }
>
> -static int etm4_probe(struct device *dev, void __iomem *base, u32 etm_pid)
> +static int etm4_add_coresight_dev(struct device *dev,
> + struct csdev_access *access)
Could this be :
etm4_add_coresight_dev(struct etm4_init_arg *init_arg)
That provides us with "access". Also if replace
init_arg.drvdata field with init_arg.dev
we can use that for
etm4_init_arch_data()
and
etm4_add_coresight_dev.
we can get to drvdata from the dev via, dev_get_drvdata(dev).
> {
> int ret;
> struct coresight_platform_data *pdata = NULL;
> - struct etmv4_drvdata *drvdata;
> + struct etmv4_drvdata *drvdata = dev_get_drvdata(dev);
> struct coresight_desc desc = { 0 };
> - struct etm4_init_arg init_arg = { 0 };
> u8 major, minor;
> char *type_name;
>
> - drvdata = devm_kzalloc(dev, sizeof(*drvdata), GFP_KERNEL);
> if (!drvdata)
> - return -ENOMEM;
> -
> - dev_set_drvdata(dev, drvdata);
> -
> - if (pm_save_enable == PARAM_PM_SAVE_FIRMWARE)
> - pm_save_enable = coresight_loses_context_with_cpu(dev) ?
> - PARAM_PM_SAVE_SELF_HOSTED : PARAM_PM_SAVE_NEVER;
> -
> - if (pm_save_enable != PARAM_PM_SAVE_NEVER) {
> - drvdata->save_state = devm_kmalloc(dev,
> - sizeof(struct etmv4_save_state), GFP_KERNEL);
> - if (!drvdata->save_state)
> - return -ENOMEM;
> - }
> -
> - drvdata->base = base;
> -
> - spin_lock_init(&drvdata->spinlock);
> -
> - drvdata->cpu = coresight_get_cpu(dev);
> - if (drvdata->cpu < 0)
> - return drvdata->cpu;
> -
> - init_arg.drvdata = drvdata;
> - init_arg.csa = &desc.access;
> - init_arg.pid = etm_pid;
> + return -EINVAL;
>
> - if (smp_call_function_single(drvdata->cpu,
> - etm4_init_arch_data, &init_arg, 1))
> - dev_err(dev, "ETM arch init failed\n");
> + desc.access = *access;
>
> if (!drvdata->arch)
> return -EINVAL;
> @@ -2050,6 +2030,69 @@ static int etm4_probe(struct device *dev, void __iomem *base, u32 etm_pid)
> return 0;
> }
>
> +static int etm4_probe(struct device *dev, void __iomem *base, u32 etm_pid)
> +{
> + struct etmv4_drvdata *drvdata;
> + struct csdev_access access = { 0 };
> + struct etm4_init_arg init_arg = { 0 };
> + struct etm4_delayed_probe *delayed;
> +
> + drvdata = devm_kzalloc(dev, sizeof(*drvdata), GFP_KERNEL);
> + if (!drvdata)
> + return -ENOMEM;
> +
> + dev_set_drvdata(dev, drvdata);
> +
> + if (pm_save_enable == PARAM_PM_SAVE_FIRMWARE)
> + pm_save_enable = coresight_loses_context_with_cpu(dev) ?
> + PARAM_PM_SAVE_SELF_HOSTED : PARAM_PM_SAVE_NEVER;
> +
> + if (pm_save_enable != PARAM_PM_SAVE_NEVER) {
> + drvdata->save_state = devm_kmalloc(dev,
> + sizeof(struct etmv4_save_state), GFP_KERNEL);
> + if (!drvdata->save_state)
> + return -ENOMEM;
> + }
> +
> + drvdata->base = base;
> +
> + spin_lock_init(&drvdata->spinlock);
> +
> + drvdata->cpu = coresight_get_cpu(dev);
> + if (drvdata->cpu < 0)
> + return drvdata->cpu;
> +
> + init_arg.drvdata = drvdata;
> + init_arg.csa = &access;
> + init_arg.pid = etm_pid;
> +
> + /*
> + * Serialize against CPUHP callbacks to avoid race condition
> + * between the smp call and saving the delayed probe.
> + */
> + cpus_read_lock();
> + if (smp_call_function_single(drvdata->cpu,
> + etm4_init_arch_data, &init_arg, 1)) {
> + /* The CPU was offline, try again once it comes online. */
> + delayed = devm_kmalloc(dev, sizeof(*delayed), GFP_KERNEL);
> + if (!delayed) {
> + cpus_read_unlock();
> + return -ENOMEM;
> + }
> +
> + delayed->etm_pid = etm_pid;
> + delayed->dev = dev;
> +
> + per_cpu(delayed_probe, drvdata->cpu) = delayed;
> +
> + cpus_read_unlock();
> + return 0;
> + }
> + cpus_read_unlock();
> +
> + return etm4_add_coresight_dev(dev, &access);
> +}
> +
> static int etm4_probe_amba(struct amba_device *adev, const struct amba_id *id)
> {
> void __iomem *base;
> @@ -2088,6 +2131,64 @@ static int etm4_probe_platform_dev(struct platform_device *pdev)
> return ret;
> }
>
> +static int etm4_probe_cpu(unsigned int cpu)
> +{
> + int ret;
> + struct etm4_delayed_probe di;
> + struct csdev_access access = { 0 };
> + struct etm4_init_arg init_arg;
> + struct etm4_delayed_probe *dp = *this_cpu_ptr(&delayed_probe);
This could be called from the existing etm4_starting_cpu() ?
static int etm4_starting_cpu(unsigned int cpu)
{
if (!etmdrvdata[cpu])
- return 0;
+ return etm4_probe_cpu(cpu);
spin_lock(&etmdrvdata[cpu]->spinlock);
if (!etmdrvdata[cpu]->os_unlock)
etm4_os_unlock(etmdrvdata[cpu]);
if (local_read(&etmdrvdata[cpu]->mode))
etm4_enable_hw(etmdrvdata[cpu]);
spin_unlock(&etmdrvdata[cpu]->spinlock);
}
> +
> + if (!dp)
> + return 0;
> +
> + di = *dp;
> + devm_kfree(di.dev, dp);
> + *this_cpu_ptr(&delayed_probe) = NULL;
> +
> + ret = pm_runtime_resume_and_get(di.dev);
> + if (ret < 0) {
> + dev_err(di.dev, "Failed to get PM runtime!\n");
> + return 0;
> + }
> +
> + init_arg.drvdata = dev_get_drvdata(di.dev);
> + if (!init_arg.drvdata)
> + return 0;
> +
> + init_arg.csa = &access;
> + init_arg.pid = di.etm_pid;
> + etm4_init_arch_data(&init_arg);
> +
> + etm4_add_coresight_dev(di.dev, &access);
> +
> + pm_runtime_put(di.dev);
> + return 0;
> +}
> +
> +static int etm4_setup_cpuhp_probe(void)
> +{
> + int ret;
> +
> + ret = cpuhp_setup_state_nocalls(CPUHP_AP_ONLINE_DYN,
> + "arm/coresight4:delayed_probe",
> + etm4_probe_cpu, NULL);
> +
> + if (ret > 0) {
> + hp_probe = ret;
> + return 0;
> + }
> + return ret;
> +}
> +
> +static void etm4_remove_cpuhp_probe(void)
> +{
> + if (hp_probe)
> + cpuhp_remove_state_nocalls(hp_probe);
> +
> + hp_probe = 0;
> +}
> +
And we could get rid of the above ^^
> static struct amba_cs_uci_id uci_id_etm4[] = {
> {
> /* ETMv4 UCI data */
> @@ -2102,16 +2203,20 @@ static void clear_etmdrvdata(void *info)
> int cpu = *(int *)info;
>
> etmdrvdata[cpu] = NULL;
> + per_cpu(delayed_probe, cpu) = NULL;
> }
>
> static int __exit etm4_remove_dev(struct etmv4_drvdata *drvdata)
> {
> - etm_perf_symlink(drvdata->csdev, false);
> + bool had_dev;
> /*
> * Taking hotplug lock here to avoid racing between etm4_remove_dev()
> * and CPU hotplug call backs.
> */
> cpus_read_lock();
> +
> + had_dev = etmdrvdata[drvdata->cpu];
> +
Is this ever false ? Why do we need this change ? How does this patch
affect the tear down ?
Suzuki
More information about the linux-arm-kernel
mailing list