Re: [PATCH v4 4/9] coresight: etm4x: fix inconsistencies with sysfs configuration

[Yeoreum Yun]

[...]

> > @@ -616,23 +622,46 @@ static int etm4_enable_hw(struct etmv4_drvdata *drvdata)
> >   static void etm4_enable_sysfs_smp_call(void *info)
> >   {
> >   	struct etm4_enable_arg *arg = info;
> > +	struct etmv4_drvdata *drvdata;
> >   	struct coresight_device *csdev;
> >   	if (WARN_ON(!arg))
> >   		return;
> > -	csdev = arg->drvdata->csdev;
> > +	drvdata = arg->drvdata;
> > +	csdev = drvdata->csdev;
> >   	if (!coresight_take_mode(csdev, CS_MODE_SYSFS)) {
> >   		/* Someone is already using the tracer */
> >   		arg->rc = -EBUSY;
> >   		return;
> >   	}
> > -	arg->rc = etm4_enable_hw(arg->drvdata);
> > +	drvdata->active_config = arg->config;
> > -	/* The tracer didn't start */
> > +	if (arg->cfg_hash) {
> > +		arg->rc = cscfg_csdev_enable_active_config(csdev,
> > +							   arg->cfg_hash,
> > +							   arg->preset);
> > +		if (arg->rc)
> > +			goto err;
> > +	}
> > +
> > +	drvdata->trcid = arg->trace_id;
> > +
> > +	/* Tracer will never be paused in sysfs mode */
> > +	drvdata->paused = false;
> > +
> > +	arg->rc = etm4_enable_hw(drvdata);
> >   	if (arg->rc)
> > -		coresight_set_mode(csdev, CS_MODE_DISABLED);
> > +		goto err;
> > +
> > +	drvdata->sticky_enable = true;
> > +
> > +	return;
> > +err:
> > +	/* The tracer didn't start */
> > +	etm4_release_trace_id(drvdata);
>
> [NIT] better move this error handle to etm4_enable_sysfs.
>
> smp_call_function_single possible return before call
> etm4_enable_sysfs_smp_call if the cpu is offline. The error path here cannot
> handle this error, breaking previous logic(handle all errors in
> etm4_enable_sysfs by releasing trace id).
>
> There is no harm in not releasing the trace id here because ETM can re-use
> the allocated trace ID. But it's better to fix the inconsistent logic.

Agree. I've missed this!

Thanks ;)

[...]


--
Sincerely,
Yeoreum Yun