Re: [RFC PATCH 01/20] coresight: etm3x: splitting 'etm_enable_hw()' operations

[Alexander Shishkin]

Mathieu Poirier  writes:

> On 30 September 2015 at 03:58, Alexander Shishkin
>  wrote:
>> Most of these things can also be bypassed, as at least initially perf
>> events won't be using trigger/sequencer configurations, so we could
>> simply clear all these things out when a first perf event is created
>> (which would also disallow any sysfs poking around the etm/etb) and not
>> worry about them in the pmu callbacks.
>
> I don't want to restrain configuration options to what is available
> through Perf's cmd line.  The sysFS interface is there to complement
> what is currently not available.

In practice this means that part of your trace configuration will be
global and part -- per event, which can indeed work if you only have one
event at a time, but again, makes using perf infrastructure redundant
for this driver. For multiple events this doesn't make much sense.

Regards,
--
Alex
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [RFC PATCH 01/20] coresight: etm3x: splitting 'etm_enable_hw()' operations

[Mathieu Poirier]

On 30 September 2015 at 03:58, Alexander Shishkin
 wrote:
> Mathieu Poirier  writes:
>
>> -static void etm_enable_hw(void *info)
>> +static void etm_power_up_cpu(void *info)
>>  {
>> - int i;
>> - u32 etmcr;
>> - struct etm_drvdata *drvdata = info;
>> + struct coresight_device *csdev = info;
>> + struct etm_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
>> +
>> + WARN_ON(drvdata->cpu != smp_processor_id());
>
> Maybe WARN_ON_ONCE() is sufficient here (and other similar places).

Yes, let's go with that.

>
>> +static void etm_power_down_cpu(void *info)
>> +{
>> + struct coresight_device *csdev = info;
>> + struct etm_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
>> +
>> + WARN_ON(drvdata->cpu != smp_processor_id());
>
> Likewise.
>
>> +/**
>> + * etm_configure_cpu - configure ETM registers
>> + * @csdev - the etm that needs to be configure.
>> + *
>> + * Applies a configuration set to the ETM registers _without_ enabling the
>> + * tracer.  This function needs to be executed on the CPU who's tracer is
>> + * being configured.
>> + */
>> +static void etm_configure_cpu(void *info)
>> +{
>> + int i;
>> + u32 etmcr;
>> + struct coresight_device *csdev = info;
>> + struct etm_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
>> +
>> + WARN_ON(drvdata->cpu != smp_processor_id());
>
> Likewise.
>
>> +
>> + CS_UNLOCK(drvdata->base);
>>   etm_set_prog(drvdata);
>>
>>   etmcr = etm_readl(drvdata, ETMCR);
>> - etmcr &= (ETMCR_PWD_DWN | ETMCR_ETM_PRG);
>>   etmcr |= drvdata->port_size;
>>   etm_writel(drvdata, drvdata->ctrl | etmcr, ETMCR);
>>   etm_writel(drvdata, drvdata->trigger_event, ETMTRIGGER);
>
> Most of these things can also be bypassed, as at least initially perf
> events won't be using trigger/sequencer configurations, so we could
> simply clear all these things out when a first perf event is created
> (which would also disallow any sysfs poking around the etm/etb) and not
> worry about them in the pmu callbacks.

I don't want to restrain configuration options to what is available
through Perf's cmd line.  The sysFS interface is there to complement
what is currently not available.  Poking around in the sysFS registers
is allowed for a long as a tracer hasn't been commissioned by Perf.  I
do agree though that a mechanism need to be set in place to prevent
changing configuration values once Perf has taken control of a tracer.


>
> Regards,
> --
> Alex
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [RFC PATCH 01/20] coresight: etm3x: splitting 'etm_enable_hw()' operations

[Mathieu Poirier]

On 30 September 2015 at 03:58, Alexander Shishkin
 wrote:
> Mathieu Poirier  writes:
>
>> -static void etm_enable_hw(void *info)
>> +static void etm_power_up_cpu(void *info)
>>  {
>> - int i;
>> - u32 etmcr;
>> - struct etm_drvdata *drvdata = info;
>> + struct coresight_device *csdev = info;
>> + struct etm_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
>> +
>> + WARN_ON(drvdata->cpu != smp_processor_id());
>
> Maybe WARN_ON_ONCE() is sufficient here (and other similar places).

Yes, let's go with that.

>
>> +static void etm_power_down_cpu(void *info)
>> +{
>> + struct coresight_device *csdev = info;
>> + struct etm_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
>> +
>> + WARN_ON(drvdata->cpu != smp_processor_id());
>
> Likewise.
>
>> +/**
>> + * etm_configure_cpu - configure ETM registers
>> + * @csdev - the etm that needs to be configure.
>> + *
>> + * Applies a configuration set to the ETM registers _without_ enabling the
>> + * tracer.  This function needs to be executed on the CPU who's tracer is
>> + * being configured.
>> + */
>> +static void etm_configure_cpu(void *info)
>> +{
>> + int i;
>> + u32 etmcr;
>> + struct coresight_device *csdev = info;
>> + struct etm_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
>> +
>> + WARN_ON(drvdata->cpu != smp_processor_id());
>
> Likewise.
>
>> +
>> + CS_UNLOCK(drvdata->base);
>>   etm_set_prog(drvdata);
>>
>>   etmcr = etm_readl(drvdata, ETMCR);
>> - etmcr &= (ETMCR_PWD_DWN | ETMCR_ETM_PRG);
>>   etmcr |= drvdata->port_size;
>>   etm_writel(drvdata, drvdata->ctrl | etmcr, ETMCR);
>>   etm_writel(drvdata, drvdata->trigger_event, ETMTRIGGER);
>
> Most of these things can also be bypassed, as at least initially perf
> events won't be using trigger/sequencer configurations, so we could
> simply clear all these things out when a first perf event is created
> (which would also disallow any sysfs poking around the etm/etb) and not
> worry about them in the pmu callbacks.

I don't want to restrain configuration options to what is available
through Perf's cmd line.  The sysFS interface is there to complement
what is currently not available.  Poking around in the sysFS registers
is allowed for a long as a tracer hasn't been commissioned by Perf.  I
do agree though that a mechanism need to be set in place to prevent
changing configuration values once Perf has taken control of a tracer.


>
> Regards,
> --
> Alex
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [RFC PATCH 01/20] coresight: etm3x: splitting 'etm_enable_hw()' operations

[Alexander Shishkin]

Mathieu Poirier  writes:

> On 30 September 2015 at 03:58, Alexander Shishkin
>  wrote:
>> Most of these things can also be bypassed, as at least initially perf
>> events won't be using trigger/sequencer configurations, so we could
>> simply clear all these things out when a first perf event is created
>> (which would also disallow any sysfs poking around the etm/etb) and not
>> worry about them in the pmu callbacks.
>
> I don't want to restrain configuration options to what is available
> through Perf's cmd line.  The sysFS interface is there to complement
> what is currently not available.

In practice this means that part of your trace configuration will be
global and part -- per event, which can indeed work if you only have one
event at a time, but again, makes using perf infrastructure redundant
for this driver. For multiple events this doesn't make much sense.

Regards,
--
Alex
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [RFC PATCH 01/20] coresight: etm3x: splitting 'etm_enable_hw()' operations

[Alexander Shishkin]

Mathieu Poirier  writes:

> -static void etm_enable_hw(void *info)
> +static void etm_power_up_cpu(void *info)
>  {
> - int i;
> - u32 etmcr;
> - struct etm_drvdata *drvdata = info;
> + struct coresight_device *csdev = info;
> + struct etm_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
> +
> + WARN_ON(drvdata->cpu != smp_processor_id());

Maybe WARN_ON_ONCE() is sufficient here (and other similar places).

> +static void etm_power_down_cpu(void *info)
> +{
> + struct coresight_device *csdev = info;
> + struct etm_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
> +
> + WARN_ON(drvdata->cpu != smp_processor_id());

Likewise.

> +/**
> + * etm_configure_cpu - configure ETM registers
> + * @csdev - the etm that needs to be configure.
> + *
> + * Applies a configuration set to the ETM registers _without_ enabling the
> + * tracer.  This function needs to be executed on the CPU who's tracer is
> + * being configured.
> + */
> +static void etm_configure_cpu(void *info)
> +{
> + int i;
> + u32 etmcr;
> + struct coresight_device *csdev = info;
> + struct etm_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
> +
> + WARN_ON(drvdata->cpu != smp_processor_id());

Likewise.

> +
> + CS_UNLOCK(drvdata->base);
>   etm_set_prog(drvdata);
>  
>   etmcr = etm_readl(drvdata, ETMCR);
> - etmcr &= (ETMCR_PWD_DWN | ETMCR_ETM_PRG);
>   etmcr |= drvdata->port_size;
>   etm_writel(drvdata, drvdata->ctrl | etmcr, ETMCR);
>   etm_writel(drvdata, drvdata->trigger_event, ETMTRIGGER);

Most of these things can also be bypassed, as at least initially perf
events won't be using trigger/sequencer configurations, so we could
simply clear all these things out when a first perf event is created
(which would also disallow any sysfs poking around the etm/etb) and not
worry about them in the pmu callbacks.

Regards,
--
Alex
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [RFC PATCH 01/20] coresight: etm3x: splitting 'etm_enable_hw()' operations

[Alexander Shishkin]

Mathieu Poirier  writes:

> -static void etm_enable_hw(void *info)
> +static void etm_power_up_cpu(void *info)
>  {
> - int i;
> - u32 etmcr;
> - struct etm_drvdata *drvdata = info;
> + struct coresight_device *csdev = info;
> + struct etm_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
> +
> + WARN_ON(drvdata->cpu != smp_processor_id());

Maybe WARN_ON_ONCE() is sufficient here (and other similar places).

> +static void etm_power_down_cpu(void *info)
> +{
> + struct coresight_device *csdev = info;
> + struct etm_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
> +
> + WARN_ON(drvdata->cpu != smp_processor_id());

Likewise.

> +/**
> + * etm_configure_cpu - configure ETM registers
> + * @csdev - the etm that needs to be configure.
> + *
> + * Applies a configuration set to the ETM registers _without_ enabling the
> + * tracer.  This function needs to be executed on the CPU who's tracer is
> + * being configured.
> + */
> +static void etm_configure_cpu(void *info)
> +{
> + int i;
> + u32 etmcr;
> + struct coresight_device *csdev = info;
> + struct etm_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
> +
> + WARN_ON(drvdata->cpu != smp_processor_id());

Likewise.

> +
> + CS_UNLOCK(drvdata->base);
>   etm_set_prog(drvdata);
>  
>   etmcr = etm_readl(drvdata, ETMCR);
> - etmcr &= (ETMCR_PWD_DWN | ETMCR_ETM_PRG);
>   etmcr |= drvdata->port_size;
>   etm_writel(drvdata, drvdata->ctrl | etmcr, ETMCR);
>   etm_writel(drvdata, drvdata->trigger_event, ETMTRIGGER);

Most of these things can also be bypassed, as at least initially perf
events won't be using trigger/sequencer configurations, so we could
simply clear all these things out when a first perf event is created
(which would also disallow any sysfs poking around the etm/etb) and not
worry about them in the pmu callbacks.

Regards,
--
Alex
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/