linux-kernel - Re: [PATCH] firmware: arm_scmi: Use signed integer to report transfer status

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 9 Jun 2020 15:00:05 +0100
From:   Lukasz Luba <lukasz.luba@....com>
To:     Sudeep Holla <sudeep.holla@....com>, linux-kernel@...r.kernel.org,
        linux-arm-kernel@...ts.infradead.org
Cc:     Jim Quinlan <james.quinlan@...adcom.com>
Subject: Re: [PATCH] firmware: arm_scmi: Use signed integer to report transfer
 status



On 6/9/20 2:45 PM, Sudeep Holla wrote:
> Currently the trace event 'scmi_xfer_end' reports the status of the
> transfer using the unsigned status field read from the firmware. This
> may not be easy to interpret and also may miss to present any timeouts
> that happen in the driver.
> 
> Let us use signed integer so that error values are emitted out after
> they are mapped from firmware errors to standard linux error codes.
> While at this, let us also include any timeouts in the driver itself.
> 
> Cc: Jim Quinlan <james.quinlan@...adcom.com>
> Cc: Lukasz Luba <lukasz.luba@....com>
> Signed-off-by: Sudeep Holla <sudeep.holla@....com>
> ---
>   drivers/firmware/arm_scmi/driver.c | 3 +--
>   include/trace/events/scmi.h        | 6 +++---
>   2 files changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c
> index 7483cacf63f9..136acbe2f4a1 100644
> --- a/drivers/firmware/arm_scmi/driver.c
> +++ b/drivers/firmware/arm_scmi/driver.c
> @@ -392,8 +392,7 @@ int scmi_do_xfer(const struct scmi_handle *handle, struct scmi_xfer *xfer)
>   		info->desc->ops->mark_txdone(cinfo, ret);
>   
>   	trace_scmi_xfer_end(xfer->transfer_id, xfer->hdr.id,
> -			    xfer->hdr.protocol_id, xfer->hdr.seq,
> -			    xfer->hdr.status);
> +			    xfer->hdr.protocol_id, xfer->hdr.seq, ret);
>   
>   	return ret;
>   }
> diff --git a/include/trace/events/scmi.h b/include/trace/events/scmi.h
> index f076c430d243..f3a4b4d60714 100644
> --- a/include/trace/events/scmi.h
> +++ b/include/trace/events/scmi.h
> @@ -35,7 +35,7 @@ TRACE_EVENT(scmi_xfer_begin,
>   
>   TRACE_EVENT(scmi_xfer_end,
>   	TP_PROTO(int transfer_id, u8 msg_id, u8 protocol_id, u16 seq,
> -		 u32 status),
> +		 int status),
>   	TP_ARGS(transfer_id, msg_id, protocol_id, seq, status),
>   
>   	TP_STRUCT__entry(
> @@ -43,7 +43,7 @@ TRACE_EVENT(scmi_xfer_end,
>   		__field(u8, msg_id)
>   		__field(u8, protocol_id)
>   		__field(u16, seq)
> -		__field(u32, status)
> +		__field(int, status)
>   	),
>   
>   	TP_fast_assign(
> @@ -54,7 +54,7 @@ TRACE_EVENT(scmi_xfer_end,
>   		__entry->status = status;
>   	),
>   
> -	TP_printk("transfer_id=%d msg_id=%u protocol_id=%u seq=%u status=%u",
> +	TP_printk("transfer_id=%d msg_id=%u protocol_id=%u seq=%u status=%d",
>   		__entry->transfer_id, __entry->msg_id, __entry->protocol_id,
>   		__entry->seq, __entry->status)
>   );
> 

Indeed looks better with the Linux like ret status

Reviewed-by: Lukasz Luba <lukasz.luba@....com>

Regards,
Lukasz