On Mon, 20 Aug 2018 15:36:47 +0100
Suzuki K Poulose [off-list ref] wrote:

On 08/20/2018 03:22 PM, Kim Phillips wrote:

quoted

On Mon, 20 Aug 2018 11:03:03 +0100
Suzuki K Poulose [off-list ref] wrote:

quoted

On 08/16/2018 08:28 PM, Mathieu Poirier wrote:

quoted

On Wed, 15 Aug 2018 at 09:28, Kim Phillips [off-list ref] wrote:

quoted

On Wed, 15 Aug 2018 10:39:13 +0100
Will Deacon [off-list ref] wrote:

quoted

On Tue, Aug 14, 2018 at 01:42:27PM -0600, Mathieu Poirier wrote:

quoted

On Tue, 14 Aug 2018 at 11:09, Kim Phillips [off-list ref] wrote:

quoted

The other thing that's going on here is that I'm becoming numb to the
loathsome "failed to mmap with 12 (Cannot allocate memory)" being
returned no matter what the error is/was. E.g., an error that would
indicate a sense of non-implementation would be much better
appreciated than presumably what the above is doing, i.e., returning
-ENOMEM.  That, backed up with specific details in the form of human
readable text in dmesg would be *most* welcome.

As part of the refactoring of the code to support CPU-wide scenarios I
intend to emit better diagnostic messages from the driver.  Modifying
rb_alloc_aux() to propagate the error message generated by the
architecture specific PMUs doesn't look hard either and I _may_ get to
it as part of this work.

For the record, I will continue to oppose PMU drivers that dump diagnostics
about user-controlled input into dmesg, but the coresight drivers are yours
so it's up to you and I won't get in the way!

That sounds technically self-contradicting to me.  Why shouldn't
coresight share the same policies as those used for PMU drivers?  Or
why not allow the individual vendor PMU driver authors control the
level of user-friendliness of their own drivers?

That being said, Matheiu, would you accept patches that make coresight
more verbose in dmesg?

It depends on the issue you're hoping to address.  I'd rather see the
root cause of the problem fixed than adding temporary code.  Suzuki
added the ETR perf API and I'm currently working on CPU-wide
scenarios.  From there and with regards to what can happen in
setup_aux(), we should have things covered.

I think the main issue is the lack of error code propagation from
setup_aux() back to the perf_aux_output_handle_begin(), which always
return -ENOMEM. If we fix that, we could get better idea of whats
wrong.

Why get a better idea when we can get the exact details?

The different values for error numbers are there for a reason...

But the same error number, e.g., EINVAL, can be returned for different
reasons.

quoted

quoted

If someone is planning to add verbose messages, they may do so by adding
dev_dbg() / pr_debug(), which can be turned on as and when needed.

I disagree:  that just adds another usage and kernel configuration
obstacle.  Why not use pr_err straight up?

I personally don't agree to usage of pr_err() in paths which are easily
triggered from user input.

Why not?  pr_* are ratelimited.

Also, we are moving all the "debugging"
messages to the dynamic debug, to prevent lockdep splats.

Are you referring to this?:

https://lkml.org/lkml/2018/5/1/73

Re-reading the thread, AFAICT, it was never proven that the splats
occurred due to the dev_infos, and there's no dev_info in this
stacktrace:

https://lkml.org/lkml/2018/5/10/269

But even if it were, wouldn't the splats also occur with dev_dbg?

Kim