Message-ID: <e6293b33-163f-87a0-7420-6c3cae40de18@redhat.com>
Date:   Tue, 15 Dec 2020 15:43:17 +0100
From:   Hans de Goede <hdegoede@...hat.com>
To:     Maximilian Luz <luzmaximilian@...il.com>,
        linux-kernel@...r.kernel.org
Cc:     Mark Gross <mgross@...ux.intel.com>,
        Andy Shevchenko <andy.shevchenko@...il.com>,
        Barnabás Pőcze <pobrn@...tonmail.com>,
        Arnd Bergmann <arnd@...db.de>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Steven Rostedt <rostedt@...dmis.org>,
        Ingo Molnar <mingo@...hat.com>,
        Blaž Hrastnik <blaz@...n.io>,
        Dorian Stoll <dorian.stoll@...p.io>,
        platform-driver-x86@...r.kernel.org
Subject: Re: [PATCH v2 5/9] platform/surface: aggregator: Add error injection
 capabilities

Hi,

On 12/3/20 10:26 PM, Maximilian Luz wrote:
> This commit adds error injection hooks to the Surface Serial Hub
> communication protocol implementation, to:
> 
>  - simulate simple serial transmission errors,
> 
>  - drop packets, requests, and responses, simulating communication
>    failures and potentially trigger retransmission timeouts, as well as
> 
>  - inject invalid data into submitted and received packets.
> 
> Together with the trace points introduced in the previous commit, these
> facilities are intended to aid in testing, validation, and debugging of
> the Surface Aggregator communication layer.
> 
> Signed-off-by: Maximilian Luz <luzmaximilian@...il.com>

Thanks, patch looks good to me:

Reviewed-by: Hans de Goede <hdegoede@...hat.com>

Regards,

Hans

> ---
> 
> Changes in v1 (from RFC):
>  - remove unnecessary default in Kconfig entry
> 
> Changes in v2:
>  - use dedicated trace event class for data error injection
>  - spell check comments and strings, fix typos
>  - unify comment style
>  - run checkpatch --strict, fix warnings and style issues
> 
> ---
>  drivers/platform/surface/aggregator/Kconfig   |  14 +
>  .../surface/aggregator/ssh_packet_layer.c     | 296 +++++++++++++++++-
>  .../surface/aggregator/ssh_request_layer.c    |  35 +++
>  drivers/platform/surface/aggregator/trace.h   |  31 ++
>  4 files changed, 375 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/platform/surface/aggregator/Kconfig b/drivers/platform/surface/aggregator/Kconfig
> index ce34941ef91b..48f40c345e29 100644
> --- a/drivers/platform/surface/aggregator/Kconfig
> +++ b/drivers/platform/surface/aggregator/Kconfig
> @@ -41,3 +41,17 @@ menuconfig SURFACE_AGGREGATOR
>  	  Choose m if you want to build the SAM subsystem core and SSH driver as
>  	  module, y if you want to build it into the kernel and n if you don't
>  	  want it at all.
> +
> +config SURFACE_AGGREGATOR_ERROR_INJECTION
> +	bool "Surface System Aggregator Module Error Injection Capabilities"
> +	depends on SURFACE_AGGREGATOR
> +	depends on FUNCTION_ERROR_INJECTION
> +	help
> +	  Provides error-injection capabilities for the Surface System
> +	  Aggregator Module subsystem and Surface Serial Hub driver.
> +
> +	  Specifically, exports error injection hooks to be used with the
> +	  kernel's function error injection capabilities to simulate underlying
> +	  transport and communication problems, such as invalid data sent to or
> +	  received from the EC, dropped data, and communication timeouts.
> +	  Intended for development and debugging.
> diff --git a/drivers/platform/surface/aggregator/ssh_packet_layer.c b/drivers/platform/surface/aggregator/ssh_packet_layer.c
> index f5ec58a1faa2..0205be9380bf 100644
> --- a/drivers/platform/surface/aggregator/ssh_packet_layer.c
> +++ b/drivers/platform/surface/aggregator/ssh_packet_layer.c
> @@ -7,6 +7,7 @@
> 
>  #include <asm/unaligned.h>
>  #include <linux/atomic.h>
> +#include <linux/error-injection.h>
>  #include <linux/jiffies.h>
>  #include <linux/kfifo.h>
>  #include <linux/kref.h>
> @@ -225,6 +226,286 @@
>   */
>  #define SSH_PTL_RX_FIFO_LEN			4096
> 
> +#ifdef CONFIG_SURFACE_AGGREGATOR_ERROR_INJECTION
> +
> +/**
> + * ssh_ptl_should_drop_ack_packet() - Error injection hook to drop ACK packets.
> + *
> + * Useful to test detection and handling of automated re-transmits by the EC.
> + * Specifically of packets that the EC considers not-ACKed but the driver
> + * already considers ACKed (due to dropped ACK). In this case, the EC
> + * re-transmits the packet-to-be-ACKed and the driver should detect it as
> + * duplicate/already handled. Note that the driver should still send an ACK
> + * for the re-transmitted packet.
> + */
> +static noinline bool ssh_ptl_should_drop_ack_packet(void)
> +{
> +	return false;
> +}
> +ALLOW_ERROR_INJECTION(ssh_ptl_should_drop_ack_packet, TRUE);
> +
> +/**
> + * ssh_ptl_should_drop_nak_packet() - Error injection hook to drop NAK packets.
> + *
> + * Useful to test/force automated (timeout-based) re-transmit by the EC.
> + * Specifically, packets that have not reached the driver completely/with valid
> + * checksums. Only useful in combination with receival of (injected) bad data.
> + */
> +static noinline bool ssh_ptl_should_drop_nak_packet(void)
> +{
> +	return false;
> +}
> +ALLOW_ERROR_INJECTION(ssh_ptl_should_drop_nak_packet, TRUE);
> +
> +/**
> + * ssh_ptl_should_drop_dsq_packet() - Error injection hook to drop sequenced
> + * data packet.
> + *
> + * Useful to test re-transmit timeout of the driver. If the data packet has not
> + * been ACKed after a certain time, the driver should re-transmit the packet up
> + * to limited number of times defined in SSH_PTL_MAX_PACKET_TRIES.
> + */
> +static noinline bool ssh_ptl_should_drop_dsq_packet(void)
> +{
> +	return false;
> +}
> +ALLOW_ERROR_INJECTION(ssh_ptl_should_drop_dsq_packet, TRUE);
> +
> +/**
> + * ssh_ptl_should_fail_write() - Error injection hook to make
> + * serdev_device_write() fail.
> + *
> + * Hook to simulate errors in serdev_device_write when transmitting packets.
> + */
> +static noinline int ssh_ptl_should_fail_write(void)
> +{
> +	return 0;
> +}
> +ALLOW_ERROR_INJECTION(ssh_ptl_should_fail_write, ERRNO);
> +
> +/**
> + * ssh_ptl_should_corrupt_tx_data() - Error injection hook to simulate invalid
> + * data being sent to the EC.
> + *
> + * Hook to simulate corrupt/invalid data being sent from host (driver) to EC.
> + * Causes the packet data to be actively corrupted by overwriting it with
> + * pre-defined values, such that it becomes invalid, causing the EC to respond
> + * with a NAK packet. Useful to test handling of NAK packets received by the
> + * driver.
> + */
> +static noinline bool ssh_ptl_should_corrupt_tx_data(void)
> +{
> +	return false;
> +}
> +ALLOW_ERROR_INJECTION(ssh_ptl_should_corrupt_tx_data, TRUE);
> +
> +/**
> + * ssh_ptl_should_corrupt_rx_syn() - Error injection hook to simulate invalid
> + * data being sent by the EC.
> + *
> + * Hook to simulate invalid SYN bytes, i.e. an invalid start of messages and
> + * test handling thereof in the driver.
> + */
> +static noinline bool ssh_ptl_should_corrupt_rx_syn(void)
> +{
> +	return false;
> +}
> +ALLOW_ERROR_INJECTION(ssh_ptl_should_corrupt_rx_syn, TRUE);
> +
> +/**
> + * ssh_ptl_should_corrupt_rx_data() - Error injection hook to simulate invalid
> + * data being sent by the EC.
> + *
> + * Hook to simulate invalid data/checksum of the message frame and test handling
> + * thereof in the driver.
> + */
> +static noinline bool ssh_ptl_should_corrupt_rx_data(void)
> +{
> +	return false;
> +}
> +ALLOW_ERROR_INJECTION(ssh_ptl_should_corrupt_rx_data, TRUE);
> +
> +static bool __ssh_ptl_should_drop_ack_packet(struct ssh_packet *packet)
> +{
> +	if (likely(!ssh_ptl_should_drop_ack_packet()))
> +		return false;
> +
> +	trace_ssam_ei_tx_drop_ack_packet(packet);
> +	ptl_info(packet->ptl, "packet error injection: dropping ACK packet %p\n",
> +		 packet);
> +
> +	return true;
> +}
> +
> +static bool __ssh_ptl_should_drop_nak_packet(struct ssh_packet *packet)
> +{
> +	if (likely(!ssh_ptl_should_drop_nak_packet()))
> +		return false;
> +
> +	trace_ssam_ei_tx_drop_nak_packet(packet);
> +	ptl_info(packet->ptl, "packet error injection: dropping NAK packet %p\n",
> +		 packet);
> +
> +	return true;
> +}
> +
> +static bool __ssh_ptl_should_drop_dsq_packet(struct ssh_packet *packet)
> +{
> +	if (likely(!ssh_ptl_should_drop_dsq_packet()))
> +		return false;
> +
> +	trace_ssam_ei_tx_drop_dsq_packet(packet);
> +	ptl_info(packet->ptl,
> +		 "packet error injection: dropping sequenced data packet %p\n",
> +		 packet);
> +
> +	return true;
> +}
> +
> +static bool ssh_ptl_should_drop_packet(struct ssh_packet *packet)
> +{
> +	/* Ignore packets that don't carry any data (i.e. flush). */
> +	if (!packet->data.ptr || !packet->data.len)
> +		return false;
> +
> +	switch (packet->data.ptr[SSH_MSGOFFSET_FRAME(type)]) {
> +	case SSH_FRAME_TYPE_ACK:
> +		return __ssh_ptl_should_drop_ack_packet(packet);
> +
> +	case SSH_FRAME_TYPE_NAK:
> +		return __ssh_ptl_should_drop_nak_packet(packet);
> +
> +	case SSH_FRAME_TYPE_DATA_SEQ:
> +		return __ssh_ptl_should_drop_dsq_packet(packet);
> +
> +	default:
> +		return false;
> +	}
> +}
> +
> +static int ssh_ptl_write_buf(struct ssh_ptl *ptl, struct ssh_packet *packet,
> +			     const unsigned char *buf, size_t count)
> +{
> +	int status;
> +
> +	status = ssh_ptl_should_fail_write();
> +	if (unlikely(status)) {
> +		trace_ssam_ei_tx_fail_write(packet, status);
> +		ptl_info(packet->ptl,
> +			 "packet error injection: simulating transmit error %d, packet %p\n",
> +			 status, packet);
> +
> +		return status;
> +	}
> +
> +	return serdev_device_write_buf(ptl->serdev, buf, count);
> +}
> +
> +static void ssh_ptl_tx_inject_invalid_data(struct ssh_packet *packet)
> +{
> +	/* Ignore packets that don't carry any data (i.e. flush). */
> +	if (!packet->data.ptr || !packet->data.len)
> +		return;
> +
> +	/* Only allow sequenced data packets to be modified. */
> +	if (packet->data.ptr[SSH_MSGOFFSET_FRAME(type)] != SSH_FRAME_TYPE_DATA_SEQ)
> +		return;
> +
> +	if (likely(!ssh_ptl_should_corrupt_tx_data()))
> +		return;
> +
> +	trace_ssam_ei_tx_corrupt_data(packet);
> +	ptl_info(packet->ptl,
> +		 "packet error injection: simulating invalid transmit data on packet %p\n",
> +		 packet);
> +
> +	/*
> +	 * NB: The value 0xb3 has been chosen more or less randomly so that it
> +	 * doesn't have any (major) overlap with the SYN bytes (aa 55) and is
> +	 * non-trivial (i.e. non-zero, non-0xff).
> +	 */
> +	memset(packet->data.ptr, 0xb3, packet->data.len);
> +}
> +
> +static void ssh_ptl_rx_inject_invalid_syn(struct ssh_ptl *ptl,
> +					  struct ssam_span *data)
> +{
> +	struct ssam_span frame;
> +
> +	/* Check if there actually is something to corrupt. */
> +	if (!sshp_find_syn(data, &frame))
> +		return;
> +
> +	if (likely(!ssh_ptl_should_corrupt_rx_syn()))
> +		return;
> +
> +	trace_ssam_ei_rx_corrupt_syn(data->len);
> +
> +	data->ptr[1] = 0xb3;	/* Set second byte of SYN to "random" value. */
> +}
> +
> +static void ssh_ptl_rx_inject_invalid_data(struct ssh_ptl *ptl,
> +					   struct ssam_span *frame)
> +{
> +	size_t payload_len, message_len;
> +	struct ssh_frame *sshf;
> +
> +	/* Ignore incomplete messages, will get handled once it's complete. */
> +	if (frame->len < SSH_MESSAGE_LENGTH(0))
> +		return;
> +
> +	/* Ignore incomplete messages, part 2. */
> +	payload_len = get_unaligned_le16(&frame->ptr[SSH_MSGOFFSET_FRAME(len)]);
> +	message_len = SSH_MESSAGE_LENGTH(payload_len);
> +	if (frame->len < message_len)
> +		return;
> +
> +	if (likely(!ssh_ptl_should_corrupt_rx_data()))
> +		return;
> +
> +	sshf = (struct ssh_frame *)&frame->ptr[SSH_MSGOFFSET_FRAME(type)];
> +	trace_ssam_ei_rx_corrupt_data(sshf);
> +
> +	/*
> +	 * Flip bits in first byte of payload checksum. This is basically
> +	 * equivalent to a payload/frame data error without us having to worry
> +	 * about (the, arguably pretty small, probability of) accidental
> +	 * checksum collisions.
> +	 */
> +	frame->ptr[frame->len - 2] = ~frame->ptr[frame->len - 2];
> +}
> +
> +#else /* CONFIG_SURFACE_AGGREGATOR_ERROR_INJECTION */
> +
> +static inline bool ssh_ptl_should_drop_packet(struct ssh_packet *packet)
> +{
> +	return false;
> +}
> +
> +static inline int ssh_ptl_write_buf(struct ssh_ptl *ptl,
> +				    struct ssh_packet *packet,
> +				    const unsigned char *buf,
> +				    size_t count)
> +{
> +	return serdev_device_write_buf(ptl->serdev, buf, count);
> +}
> +
> +static inline void ssh_ptl_tx_inject_invalid_data(struct ssh_packet *packet)
> +{
> +}
> +
> +static inline void ssh_ptl_rx_inject_invalid_syn(struct ssh_ptl *ptl,
> +						 struct ssam_span *data)
> +{
> +}
> +
> +static inline void ssh_ptl_rx_inject_invalid_data(struct ssh_ptl *ptl,
> +						  struct ssam_span *frame)
> +{
> +}
> +
> +#endif /* CONFIG_SURFACE_AGGREGATOR_ERROR_INJECTION */
> +
>  static void __ssh_ptl_packet_release(struct kref *kref)
>  {
>  	struct ssh_packet *p = container_of(kref, struct ssh_packet, refcnt);
> @@ -769,6 +1050,13 @@ static int ssh_ptl_tx_packet(struct ssh_ptl *ptl, struct ssh_packet *packet)
>  	if (unlikely(!packet->data.ptr))
>  		return 0;
> 
> +	/* Error injection: drop packet to simulate transmission problem. */
> +	if (ssh_ptl_should_drop_packet(packet))
> +		return 0;
> +
> +	/* Error injection: simulate invalid packet data. */
> +	ssh_ptl_tx_inject_invalid_data(packet);
> +
>  	ptl_dbg(ptl, "tx: sending data (length: %zu)\n", packet->data.len);
>  	print_hex_dump_debug("tx: ", DUMP_PREFIX_OFFSET, 16, 1,
>  			     packet->data.ptr, packet->data.len, false);
> @@ -780,7 +1068,7 @@ static int ssh_ptl_tx_packet(struct ssh_ptl *ptl, struct ssh_packet *packet)
>  		buf = packet->data.ptr + offset;
>  		len = packet->data.len - offset;
> 
> -		status = serdev_device_write_buf(ptl->serdev, buf, len);
> +		status = ssh_ptl_write_buf(ptl, packet, buf, len);
>  		if (status < 0)
>  			return status;
> 
> @@ -1389,6 +1677,9 @@ static size_t ssh_ptl_rx_eval(struct ssh_ptl *ptl, struct ssam_span *source)
>  	bool syn_found;
>  	int status;
> 
> +	/* Error injection: Modify data to simulate corrupt SYN bytes. */
> +	ssh_ptl_rx_inject_invalid_syn(ptl, source);
> +
>  	/* Find SYN. */
>  	syn_found = sshp_find_syn(source, &aligned);
> 
> @@ -1419,6 +1710,9 @@ static size_t ssh_ptl_rx_eval(struct ssh_ptl *ptl, struct ssam_span *source)
>  	if (unlikely(!syn_found))
>  		return aligned.ptr - source->ptr;
> 
> +	/* Error injection: Modify data to simulate corruption. */
> +	ssh_ptl_rx_inject_invalid_data(ptl, &aligned);
> +
>  	/* Parse and validate frame. */
>  	status = sshp_parse_frame(&ptl->serdev->dev, &aligned, &frame, &payload,
>  				  SSH_PTL_RX_BUF_LEN);
> diff --git a/drivers/platform/surface/aggregator/ssh_request_layer.c b/drivers/platform/surface/aggregator/ssh_request_layer.c
> index e91d2ed4c173..bf1782c85063 100644
> --- a/drivers/platform/surface/aggregator/ssh_request_layer.c
> +++ b/drivers/platform/surface/aggregator/ssh_request_layer.c
> @@ -8,6 +8,7 @@
>  #include <asm/unaligned.h>
>  #include <linux/atomic.h>
>  #include <linux/completion.h>
> +#include <linux/error-injection.h>
>  #include <linux/ktime.h>
>  #include <linux/limits.h>
>  #include <linux/list.h>
> @@ -58,6 +59,30 @@
>   */
>  #define SSH_RTL_TX_BATCH		10
> 
> +#ifdef CONFIG_SURFACE_AGGREGATOR_ERROR_INJECTION
> +
> +/**
> + * ssh_rtl_should_drop_response() - Error injection hook to drop request
> + * responses.
> + *
> + * Useful to cause request transmission timeouts in the driver by dropping the
> + * response to a request.
> + */
> +static noinline bool ssh_rtl_should_drop_response(void)
> +{
> +	return false;
> +}
> +ALLOW_ERROR_INJECTION(ssh_rtl_should_drop_response, TRUE);
> +
> +#else
> +
> +static inline bool ssh_rtl_should_drop_response(void)
> +{
> +	return false;
> +}
> +
> +#endif
> +
>  static u16 ssh_request_get_rqid(struct ssh_request *rqst)
>  {
>  	return get_unaligned_le16(rqst->packet.data.ptr
> @@ -459,6 +484,16 @@ static void ssh_rtl_complete(struct ssh_rtl *rtl,
>  		if (unlikely(ssh_request_get_rqid(p) != rqid))
>  			continue;
> 
> +		/* Simulate response timeout. */
> +		if (ssh_rtl_should_drop_response()) {
> +			spin_unlock(&rtl->pending.lock);
> +
> +			trace_ssam_ei_rx_drop_response(p);
> +			rtl_info(rtl, "request error injection: dropping response for request %p\n",
> +				 &p->packet);
> +			return;
> +		}
> +
>  		/*
>  		 * Mark as "response received" and "locked" as we're going to
>  		 * complete it.
> diff --git a/drivers/platform/surface/aggregator/trace.h b/drivers/platform/surface/aggregator/trace.h
> index dcca8007d876..eb332bb53ae4 100644
> --- a/drivers/platform/surface/aggregator/trace.h
> +++ b/drivers/platform/surface/aggregator/trace.h
> @@ -565,6 +565,28 @@ DECLARE_EVENT_CLASS(ssam_pending_class,
>  		TP_ARGS(pending)					\
>  	)
> 
> +DECLARE_EVENT_CLASS(ssam_data_class,
> +	TP_PROTO(size_t length),
> +
> +	TP_ARGS(length),
> +
> +	TP_STRUCT__entry(
> +		__field(size_t, length)
> +	),
> +
> +	TP_fast_assign(
> +		__entry->length = length;
> +	),
> +
> +	TP_printk("length=%zu", __entry->length)
> +);
> +
> +#define DEFINE_SSAM_DATA_EVENT(name)					\
> +	DEFINE_EVENT(ssam_data_class, ssam_##name,			\
> +		TP_PROTO(size_t length),				\
> +		TP_ARGS(length)						\
> +	)
> +
>  DEFINE_SSAM_FRAME_EVENT(rx_frame_received);
>  DEFINE_SSAM_COMMAND_EVENT(rx_response_received);
>  DEFINE_SSAM_COMMAND_EVENT(rx_event_received);
> @@ -583,6 +605,15 @@ DEFINE_SSAM_REQUEST_EVENT(request_cancel);
>  DEFINE_SSAM_REQUEST_STATUS_EVENT(request_complete);
>  DEFINE_SSAM_PENDING_EVENT(rtl_timeout_reap);
> 
> +DEFINE_SSAM_PACKET_EVENT(ei_tx_drop_ack_packet);
> +DEFINE_SSAM_PACKET_EVENT(ei_tx_drop_nak_packet);
> +DEFINE_SSAM_PACKET_EVENT(ei_tx_drop_dsq_packet);
> +DEFINE_SSAM_PACKET_STATUS_EVENT(ei_tx_fail_write);
> +DEFINE_SSAM_PACKET_EVENT(ei_tx_corrupt_data);
> +DEFINE_SSAM_DATA_EVENT(ei_rx_corrupt_syn);
> +DEFINE_SSAM_FRAME_EVENT(ei_rx_corrupt_data);
> +DEFINE_SSAM_REQUEST_EVENT(ei_rx_drop_response);
> +
>  DEFINE_SSAM_ALLOC_EVENT(ctrl_packet_alloc);
>  DEFINE_SSAM_FREE_EVENT(ctrl_packet_free);
> 
> --
> 2.29.2
> 

Powered by blists - more mailing lists