Re: [PATCH] drm/xe/gsc: Improve SW proxy error checking and logging

["Teres Alexis, Alan Previn" <alan.previn.teres.alexis@intel.com>]

Simple patch, LGTM,

Reviewed-by: Alan Previn <alan.previn.teres.alexis@intel.com>

On Tue, 2024-11-05 at 16:24 -0800, Daniele Ceraolo Spurio wrote:
> If an error occurs in the GSC<->CSME handshake, the GSC will send a
> PROXY_END msg to the driver with the status set to an error code. We
> currently don't check the status when receiving a PROXY_END message and
> instead check the proxy initialization status in the FWSTS reg;
> therefore, while still catching any initialization failures, we lose the
> actual returned error code. This can be easily improved by checking the
> status value and printing it to dmesg if it's an error.
> 
> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> Cc: Alan Previn <alan.previn.teres.alexis@intel.com>
> ---
>  drivers/gpu/drm/xe/xe_gsc_proxy.c | 47 +++++++++++++++++++++++--------
>  1 file changed, 36 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/gpu/drm/xe/xe_gsc_proxy.c b/drivers/gpu/drm/xe/xe_gsc_proxy.c
> index fc64b45d324b..24cc6a4f9a96 100644
> --- a/drivers/gpu/drm/xe/xe_gsc_proxy.c
> +++ b/drivers/gpu/drm/xe/xe_gsc_proxy.c
> @@ -139,17 +139,29 @@ static int proxy_send_to_gsc(struct xe_gsc *gsc, u32 size)
>         return 0;
>  }
>  
> -static int validate_proxy_header(struct xe_gsc_proxy_header *header,
> +static int validate_proxy_header(struct xe_gt *gt,
> +                                struct xe_gsc_proxy_header *header,
>                                  u32 source, u32 dest, u32 max_size)
>  {
>         u32 type = FIELD_GET(GSC_PROXY_TYPE, header->hdr);
>         u32 length = FIELD_GET(GSC_PROXY_PAYLOAD_LENGTH, header->hdr);
> +       int ret = 0;
>  
> -       if (header->destination != dest || header->source != source)
> -               return -ENOEXEC;
> +       if (header->destination != dest || header->source != source) {
> +               ret = -ENOEXEC;
> +               goto out;
> +       }
>  
> -       if (length + PROXY_HDR_SIZE > max_size)
> -               return -E2BIG;
> +       if (length + PROXY_HDR_SIZE > max_size) {
> +               ret = -E2BIG;
> +               goto out;
> +       }
> +
> +       /* We only care about the status if this is a message for the driver */
> +       if (dest == GSC_PROXY_ADDRESSING_KMD && header->status != 0) {
> +               ret = -EIO;
> +               goto out;
> +       }
>  
>         switch (type) {
>         case GSC_PROXY_MSG_TYPE_PROXY_PAYLOAD:
> @@ -157,12 +169,20 @@ static int validate_proxy_header(struct xe_gsc_proxy_header *header,
>                         break;
>                 fallthrough;
>         case GSC_PROXY_MSG_TYPE_PROXY_INVALID:
> -               return -EIO;
> +               ret = -EIO;
> +               break;
>         default:
>                 break;
>         }
>  
> -       return 0;
> +out:
> +       if (ret)
> +               xe_gt_err(gt,
> +                         "GSC proxy error: s=0x%x[0x%x], d=0x%x[0x%x], t=%u, l=0x%x, st=0x%x\n",
> +                         header->source, source, header->destination, dest,
> +                         type, length, header->status);
> +
> +       return ret;
>  }
>  
>  #define proxy_header_wr(xe_, map_, offset_, field_, val_) \
> @@ -228,12 +248,17 @@ static int proxy_query(struct xe_gsc *gsc)
>                 xe_map_memcpy_from(xe, to_csme_hdr, &gsc->proxy.from_gsc,
>                                    reply_offset, PROXY_HDR_SIZE);
>  
> -               /* stop if this was the last message */
> -               if (FIELD_GET(GSC_PROXY_TYPE, to_csme_hdr->hdr) == GSC_PROXY_MSG_TYPE_PROXY_END)
> +               /* Check the status and stop if this was the last message */
> +               if (FIELD_GET(GSC_PROXY_TYPE, to_csme_hdr->hdr) == GSC_PROXY_MSG_TYPE_PROXY_END) {
> +                       ret = validate_proxy_header(gt, to_csme_hdr,
> +                                                   GSC_PROXY_ADDRESSING_GSC,
> +                                                   GSC_PROXY_ADDRESSING_KMD,
> +                                                   GSC_PROXY_BUFFER_SIZE - reply_offset);
>                         break;
> +               }
>  
>                 /* make sure the GSC-to-CSME proxy header is sane */
> -               ret = validate_proxy_header(to_csme_hdr,
> +               ret = validate_proxy_header(gt, to_csme_hdr,
>                                             GSC_PROXY_ADDRESSING_GSC,
>                                             GSC_PROXY_ADDRESSING_CSME,
>                                             GSC_PROXY_BUFFER_SIZE - reply_offset);
> @@ -262,7 +287,7 @@ static int proxy_query(struct xe_gsc *gsc)
>                 }
>  
>                 /* make sure the CSME-to-GSC proxy header is sane */
> -               ret = validate_proxy_header(gsc->proxy.from_csme,
> +               ret = validate_proxy_header(gt, gsc->proxy.from_csme,
>                                             GSC_PROXY_ADDRESSING_CSME,
>                                             GSC_PROXY_ADDRESSING_GSC,
>                                             GSC_PROXY_BUFFER_SIZE - reply_offset);