Re: [PATCH v3 1/8] PCI/AER: Remove ID from aer_agent_string[]

[Shuah Khan]

On 10/4/21 8:29 AM, Naveen Naidu wrote:
> Before 010caed4ccb6 ("PCI/AER: Decode Error Source RequesterID")
> the AER error logs looked like:
>
>    pcieport 0000:00:03.0: AER: Corrected error received: id=0018
>    pcieport 0000:00:03.0: PCIe Bus Error: severity=Corrected, type=Data Link Layer, id=0018 (Receiver ID)
>    pcieport 0000:00:03.0:   device [1b36:000c] error status/mask=00000040/0000e000
>    pcieport 0000:00:03.0:    [ 6] BadTLP
>
> In 010caed4ccb6 ("PCI/AER: Decode Error Source Requester ID"),
> the "id" field was removed from the AER error logs, so currently AER
> logs look like:
>
>    pcieport 0000:00:03.0: AER: Corrected error received: 0000:00:03:0
>    pcieport 0000:00:03.0: PCIe Bus Error: severity=Corrected, type=Data Link Layer, (Receiver ID)
>    pcieport 0000:00:03.0:   device [1b36:000c] error status/mask=00000040/0000e000
>    pcieport 0000:00:03.0:    [ 6] BadTLP
>
> The second line in the above logs prints "(Receiver ID)", even when
> there is no "id" in the log line. This is confusing.

Starting your commit log to say that message are confusing and then talk
about why will make it easier to understand why the change is needed.

> Remove the "ID" from the aer_agent_string[]. The error logs will
> look as follows (Sample from dummy error injected by aer-inject):
>
>    pcieport 0000:00:03.0: AER: Corrected error received: 0000:00:03.0
>    pcieport 0000:00:03.0: PCIe Bus Error: severity=Corrected, type=Data Link Layer, (Receiver)
>    pcieport 0000:00:03.0:   device [1b36:000c] error status/mask=00000040/0000e000
>    pcieport 0000:00:03.0:    [ 6] BadTLP

It is good to see before and after messages. However, it will be helpful
to know why this change is necessary. It isn't very clear why in this
commit log.

> Signed-off-by: Naveen Naidu <naveennaidu479@xxxxxxxxx
> Signed-off-by: Naveen Naidu <naveennaidu479@xxxxxxxxx>

Extra signed-off-by?

> ---
>   drivers/pci/pcie/aer.c | 10 +++++-----
>   1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c
> index 9784fdcf3006..241ff361b43c 100644
> --- a/drivers/pci/pcie/aer.c
> +++ b/drivers/pci/pcie/aer.c
> @@ -516,10 +516,10 @@ static const char *aer_uncorrectable_error_string[] = {
>   };
>   
>   static const char *aer_agent_string[] = {
> -	"Receiver ID",
> -	"Requester ID",
> -	"Completer ID",
> -	"Transmitter ID"
> +	"Receiver",
> +	"Requester",
> +	"Completer",
> +	"Transmitter"
>   };
>   
>   #define aer_stats_dev_attr(name, stats_array, strings_array,		\
> @@ -703,7 +703,7 @@ void aer_print_error(struct pci_dev *dev, struct aer_err_info *info)
>   	const char *level;
>   
>   	if (!info->status) {
> -		pci_err(dev, "PCIe Bus Error: severity=%s, type=Inaccessible, (Unregistered Agent ID)\n",
> +		pci_err(dev, "PCIe Bus Error: severity=%s, type=Inaccessible, (Unregistered Agent)\n",
>   			aer_error_severity_string[info->severity]);
>   		goto out;
>   	}

thanks,
-- Shuah