radeon_dp_aux_transfer_native: 74 callbacks suppressed

[Lyude Paul]

The main reason I added this was because the radeon driver's hotplugging paths
for DP do a ton of unnessecary probing, and because the driver usually also
checks all connectors every time there's a hotplug (there isn't much of a good
reason for this, it's just an old driver) it's not at all difficult to get
well more then 70 callbacks from this that end up filling the kernel log with
useless information (all the messages mean is that for some reason or another,
a DP aux transaction fails which usually just means the port is disconnected).

Ideally: We should be printing the first error code that the i2c handlers for
radeon return in addition to the suppressed messages (and this itself should
not be suppressed iirc), since that's almost always the only piece of
information that matters.

I wouldn't drop the message entirely though unless we're printing something
from DRM. Additionally, we should also just fix this ratelimit macro anyway
since it's intended purpose is not to print anything when debugging isn't
enabled. What do you think Alex?

On Mon, 2017-11-06 at 10:21 +0100, Jean Delvare wrote:
> Hi all,
> 
> commit 92c177b7947d9c889ea7b024871445015ea74221
> Author: Lyude
> Date:   Wed Feb 22 16:34:53 2017 -0500
> 
>     drm/radeon/dp_auxch: Ratelimit aux transfer debug messages
> 
> does more harm than good in my opinion. Since this commit, I see
> several occurrences of the following message in my kernel log daily:
> 
> radeon_dp_aux_transfer_native: 74 callbacks suppressed
> 
> I never got to see the "callback" in question though, not even once, as
> this is a debug message which is off by default. Before the change, I
> would not get any such message in the kernel log (as I would expect
> when everything works as intended.)
> 
> Does this debug message really have any practical value? If not, the
> easiest solution would be to simply drop it:
> 
> --- a/drivers/gpu/drm/radeon/radeon_dp_auxch.c
> +++ b/drivers/gpu/drm/radeon/radeon_dp_auxch.c
> @@ -168,8 +168,10 @@ radeon_dp_aux_transfer_native(struct drm
>  		goto done;
>  	}
>  	if (tmp & AUX_RX_ERROR_FLAGS) {
> -		DRM_DEBUG_KMS_RATELIMITED("dp_aux_ch flags not zero:
> %08x\n",
> -					  tmp);
> +		/*
> +		 * aux transfers always fail with non-zero status flags
> when
> +		 * there's nothing connected on the port
> +		 */
>  		ret = -EIO;
>  		goto done;
>  	}
> 
> I can resend this as a formal patch if you agree with this solution.
> 
> The actual cause of the problem is that ___ratelimit() prints its
> message at KERN_WARNING level regardless of the level of the message
> being suppressed. This makes ratelimiting debug, info or notice
> messages awkward. Looks like a design overlook to me, maybe it should
> be fixed, but that's a much bigger and intrusive change than the
> proposal above.
>