diff mbox series

[v2,3/3] libsbefifo: Set long read timeout for chip-ops

Message ID 20220223053446.262033-4-joel@jms.id.au
State New
Headers show
Series sbefifo long running chip-op ioctl support | expand

Checks

Context Check Description
snowpatch_ozlabs/github-CI success Successfully ran 1 jobs.
snowpatch_ozlabs/github-build_and_test success Successfully ran 1 jobs.

Commit Message

Joel Stanley Feb. 23, 2022, 5:34 a.m. UTC 
From: Amitay Isaacs <amitay@ozlabs.org>

SBE interface specification lists several sbe chip-ops that require long
read timeout as those chip-ops can take up to 30 seconds to complete.

Reset the long timeout back to the default after performing the chip-op.

The list of chip-ops from the spec (v2.0):

 0xA1	0x01	Execute istep
 0xA2 	0x05	Execute multi-SCOM
 0xA3	0x01	Get Ring
	0x02	Set Ring
 0xA4	0x01	Get Memory
	0x02	Put Memory
	0x03	Get SRAM
	0x04	Put SRAM
 0xA5	0x01	Get Architected Register
 	0x02	Put Architected Register
 0xA6	0x01	Control Fast Array
 	0x02	Control Trace Array
 0xA8	0x03	Quiesce SBE
 0xA9	0x01	Enter MPIPL
 	0x02	Continue MPIPL
	0x04	TI Info

Signed-off-by: Amitay Isaacs <amitay@ozlabs.org>
Signed-off-by: Joel Stanley <joel@jms.id.au>
---
v2: Split out from second patch
    Add list of ops from spec to commit message

 libsbefifo/cmd_array.c    | 12 ++++++++++++
 libsbefifo/cmd_control.c  |  6 ++++++
 libsbefifo/cmd_generic.c  |  6 ++++++
 libsbefifo/cmd_memory.c   | 23 +++++++++++++++++++++++
 libsbefifo/cmd_mpipl.c    | 18 ++++++++++++++++++
 libsbefifo/cmd_register.c | 12 ++++++++++++
 libsbefifo/cmd_ring.c     | 18 ++++++++++++++++++
 7 files changed, 95 insertions(+)

Comments

Nicholas Piggin Feb. 23, 2022, 11:54 a.m. UTC | #1 
Excerpts from Joel Stanley's message of February 23, 2022 3:34 pm:
> From: Amitay Isaacs <amitay@ozlabs.org>
> 
> SBE interface specification lists several sbe chip-ops that require long
> read timeout as those chip-ops can take up to 30 seconds to complete.
> 
> Reset the long timeout back to the default after performing the chip-op.
> 
> The list of chip-ops from the spec (v2.0):
> 
>  0xA1	0x01	Execute istep
>  0xA2 	0x05	Execute multi-SCOM
>  0xA3	0x01	Get Ring
> 	0x02	Set Ring
>  0xA4	0x01	Get Memory
> 	0x02	Put Memory
> 	0x03	Get SRAM
> 	0x04	Put SRAM
>  0xA5	0x01	Get Architected Register
>  	0x02	Put Architected Register
>  0xA6	0x01	Control Fast Array
>  	0x02	Control Trace Array
>  0xA8	0x03	Quiesce SBE
>  0xA9	0x01	Enter MPIPL
>  	0x02	Continue MPIPL
> 	0x04	TI Info
> 
> Signed-off-by: Amitay Isaacs <amitay@ozlabs.org>
> Signed-off-by: Joel Stanley <joel@jms.id.au>
> ---
> v2: Split out from second patch
>     Add list of ops from spec to commit message

That's useful ^ could add it in a comment or doc somewhere maybe?


> 
>  libsbefifo/cmd_array.c    | 12 ++++++++++++
>  libsbefifo/cmd_control.c  |  6 ++++++
>  libsbefifo/cmd_generic.c  |  6 ++++++
>  libsbefifo/cmd_memory.c   | 23 +++++++++++++++++++++++
>  libsbefifo/cmd_mpipl.c    | 18 ++++++++++++++++++
>  libsbefifo/cmd_register.c | 12 ++++++++++++
>  libsbefifo/cmd_ring.c     | 18 ++++++++++++++++++
>  7 files changed, 95 insertions(+)
> 
> diff --git a/libsbefifo/cmd_array.c b/libsbefifo/cmd_array.c
> index 1ff8c03d7278..d579eafdfb77 100644
> --- a/libsbefifo/cmd_array.c
> +++ b/libsbefifo/cmd_array.c
> @@ -69,12 +69,18 @@ int sbefifo_control_fast_array(struct sbefifo_context *sctx, uint16_t target_typ
>  	if (rc)
>  		return rc;
>  
> +	rc = sbefifo_set_long_timeout(sctx);
> +	if (rc)
> +		return rc;

Leaks msg if we fail.

> +
>  	out_len = 0;
>  	rc = sbefifo_operation(sctx, msg, msg_len, &out, &out_len);
>  	free(msg);
>  	if (rc)
>  		return rc;
>  
> +	sbefifo_reset_timeout(sctx);

This misses the reset if we fail. Same issues repeat a few times.

Thanks,
Nick
Joel Stanley March 2, 2022, 6:09 a.m. UTC | #2 
On Wed, 23 Feb 2022 at 11:54, Nicholas Piggin <npiggin@gmail.com> wrote:
>
> Excerpts from Joel Stanley's message of February 23, 2022 3:34 pm:
> > From: Amitay Isaacs <amitay@ozlabs.org>
> >
> > SBE interface specification lists several sbe chip-ops that require long
> > read timeout as those chip-ops can take up to 30 seconds to complete.
> >
> > Reset the long timeout back to the default after performing the chip-op.
> >
> > The list of chip-ops from the spec (v2.0):
> >
> >  0xA1 0x01    Execute istep
> >  0xA2         0x05    Execute multi-SCOM
> >  0xA3 0x01    Get Ring
> >       0x02    Set Ring
> >  0xA4 0x01    Get Memory
> >       0x02    Put Memory
> >       0x03    Get SRAM
> >       0x04    Put SRAM
> >  0xA5 0x01    Get Architected Register
> >       0x02    Put Architected Register
> >  0xA6 0x01    Control Fast Array
> >       0x02    Control Trace Array
> >  0xA8 0x03    Quiesce SBE
> >  0xA9 0x01    Enter MPIPL
> >       0x02    Continue MPIPL
> >       0x04    TI Info
> >
> > Signed-off-by: Amitay Isaacs <amitay@ozlabs.org>
> > Signed-off-by: Joel Stanley <joel@jms.id.au>
> > ---
> > v2: Split out from second patch
> >     Add list of ops from spec to commit message
>
> That's useful ^ could add it in a comment or doc somewhere maybe?

It comes from the SBE spec, which I don't think is published.

>
>
> >
> >  libsbefifo/cmd_array.c    | 12 ++++++++++++
> >  libsbefifo/cmd_control.c  |  6 ++++++
> >  libsbefifo/cmd_generic.c  |  6 ++++++
> >  libsbefifo/cmd_memory.c   | 23 +++++++++++++++++++++++
> >  libsbefifo/cmd_mpipl.c    | 18 ++++++++++++++++++
> >  libsbefifo/cmd_register.c | 12 ++++++++++++
> >  libsbefifo/cmd_ring.c     | 18 ++++++++++++++++++
> >  7 files changed, 95 insertions(+)
> >
> > diff --git a/libsbefifo/cmd_array.c b/libsbefifo/cmd_array.c
> > index 1ff8c03d7278..d579eafdfb77 100644
> > --- a/libsbefifo/cmd_array.c
> > +++ b/libsbefifo/cmd_array.c
> > @@ -69,12 +69,18 @@ int sbefifo_control_fast_array(struct sbefifo_context *sctx, uint16_t target_typ
> >       if (rc)
> >               return rc;
> >
> > +     rc = sbefifo_set_long_timeout(sctx);
> > +     if (rc)
> > +             return rc;
>
> Leaks msg if we fail.

Ah tricky. The _push function allocates the memory, and it is freed in
the sbefifo_operation.

If we set the timeout before the _push call, but then we need to
unwind it if _push fails. I'll leave it where it is and do a free().

>
> > +
> >       out_len = 0;
> >       rc = sbefifo_operation(sctx, msg, msg_len, &out, &out_len);
> >       free(msg);
> >       if (rc)
> >               return rc;
> >
> > +     sbefifo_reset_timeout(sctx);
>
> This misses the reset if we fail. Same issues repeat a few times.

Good catch. We can do the reset straight after the sbefifo_operation.
diff mbox series

Patch

diff --git a/libsbefifo/cmd_array.c b/libsbefifo/cmd_array.c
index 1ff8c03d7278..d579eafdfb77 100644
--- a/libsbefifo/cmd_array.c
+++ b/libsbefifo/cmd_array.c
@@ -69,12 +69,18 @@  int sbefifo_control_fast_array(struct sbefifo_context *sctx, uint16_t target_typ
 	if (rc)
 		return rc;
 
+	rc = sbefifo_set_long_timeout(sctx);
+	if (rc)
+		return rc;
+
 	out_len = 0;
 	rc = sbefifo_operation(sctx, msg, msg_len, &out, &out_len);
 	free(msg);
 	if (rc)
 		return rc;
 
+	sbefifo_reset_timeout(sctx);
+
 	rc = sbefifo_control_fast_array_pull(out, out_len);
 	if (out)
 		free(out);
@@ -136,6 +142,10 @@  int sbefifo_control_trace_array(struct sbefifo_context *sctx, uint16_t target_ty
 	if (rc)
 		return rc;
 
+	rc = sbefifo_set_long_timeout(sctx);
+	if (rc)
+		return rc;
+
 	/* The size of returned data is just a guess */
 	out_len = 16 * sizeof(uint32_t);
 	rc = sbefifo_operation(sctx, msg, msg_len, &out, &out_len);
@@ -143,6 +153,8 @@  int sbefifo_control_trace_array(struct sbefifo_context *sctx, uint16_t target_ty
 	if (rc)
 		return rc;
 
+	sbefifo_reset_timeout(sctx);
+
 	rc = sbefifo_control_trace_array_pull(out, out_len, trace_data, trace_data_len);
 	if (out)
 		free(out);
diff --git a/libsbefifo/cmd_control.c b/libsbefifo/cmd_control.c
index e64f555c665d..5fb44d7a80cc 100644
--- a/libsbefifo/cmd_control.c
+++ b/libsbefifo/cmd_control.c
@@ -64,12 +64,18 @@  int sbefifo_istep_execute(struct sbefifo_context *sctx, uint8_t major, uint8_t m
 	if (rc)
 		return rc;
 
+	rc = sbefifo_set_long_timeout(sctx);
+	if (rc)
+		return rc;
+
 	out_len = 0;
 	rc = sbefifo_operation(sctx, msg, msg_len, &out, &out_len);
 	free(msg);
 	if (rc)
 		return rc;
 
+	sbefifo_reset_timeout(sctx);
+
 	rc = sbefifo_istep_execute_pull(out, out_len);
 	if (out)
 		free(out);
diff --git a/libsbefifo/cmd_generic.c b/libsbefifo/cmd_generic.c
index 16c61f4a7863..d0514cd1da6b 100644
--- a/libsbefifo/cmd_generic.c
+++ b/libsbefifo/cmd_generic.c
@@ -199,12 +199,18 @@  int sbefifo_quiesce(struct sbefifo_context *sctx)
 	if (rc)
 		return rc;
 
+	rc = sbefifo_set_long_timeout(sctx);
+	if (rc)
+		return rc;
+
 	out_len = 0;
 	rc = sbefifo_operation(sctx, msg, msg_len, &out, &out_len);
 	free(msg);
 	if (rc)
 		return rc;
 
+	sbefifo_reset_timeout(sctx);
+
 	rc = sbefifo_quiesce_pull(out, out_len);
 	if (out)
 		free(out);
diff --git a/libsbefifo/cmd_memory.c b/libsbefifo/cmd_memory.c
index 4195a1269963..7390ee36d435 100644
--- a/libsbefifo/cmd_memory.c
+++ b/libsbefifo/cmd_memory.c
@@ -125,6 +125,10 @@  int sbefifo_mem_get(struct sbefifo_context *sctx, uint64_t addr, uint32_t size,
 	if (rc)
 		return rc;
 
+	rc = sbefifo_set_long_timeout(sctx);
+	if (rc)
+		return rc;
+
 	/* length is 6th word in the request */
 	len = be32toh(*(uint32_t *)(msg + 20));
 	extra_bytes = 0;
@@ -141,6 +145,7 @@  int sbefifo_mem_get(struct sbefifo_context *sctx, uint64_t addr, uint32_t size,
 	if (rc)
 		return rc;
 
+	sbefifo_reset_timeout(sctx);
 
 	rc = sbefifo_mem_get_pull(out, out_len, addr, size, flags, data);
 	if (out)
@@ -206,12 +211,18 @@  int sbefifo_mem_put(struct sbefifo_context *sctx, uint64_t addr, uint8_t *data,
 	if (rc)
 		return rc;
 
+	rc = sbefifo_set_long_timeout(sctx);
+	if (rc)
+		return rc;
+
 	out_len = 1 * 4;
 	rc = sbefifo_operation(sctx, msg, msg_len, &out, &out_len);
 	free(msg);
 	if (rc)
 		return rc;
 
+	sbefifo_reset_timeout(sctx);
+
 	rc = sbefifo_mem_put_pull(out, out_len);
 	if (out)
 		free(out);
@@ -295,6 +306,10 @@  int sbefifo_sram_get(struct sbefifo_context *sctx, uint16_t chiplet_id, uint64_t
 	if (rc)
 		return rc;
 
+	rc = sbefifo_set_long_timeout(sctx);
+	if (rc)
+		return rc;
+
 	/* length is 6th word in the request */
 	len = be32toh(*(uint32_t *)(msg + 20));
 
@@ -304,6 +319,8 @@  int sbefifo_sram_get(struct sbefifo_context *sctx, uint16_t chiplet_id, uint64_t
 	if (rc)
 		return rc;
 
+	sbefifo_reset_timeout(sctx);
+
 	rc = sbefifo_sram_get_pull(out, out_len, addr, size, data, data_len);
 	if (out)
 		free(out);
@@ -375,12 +392,18 @@  int sbefifo_sram_put(struct sbefifo_context *sctx, uint16_t chiplet_id, uint64_t
 	if (rc)
 		return rc;
 
+	rc = sbefifo_set_long_timeout(sctx);
+	if (rc)
+		return rc;
+
 	out_len = 4;
 	rc = sbefifo_operation(sctx, msg, msg_len, &out, &out_len);
 	free(msg);
 	if (rc)
 		return rc;
 
+	sbefifo_reset_timeout(sctx);
+
 	rc = sbefifo_sram_put_pull(out, out_len);
 	if (out)
 		free(out);
diff --git a/libsbefifo/cmd_mpipl.c b/libsbefifo/cmd_mpipl.c
index 1d3249483375..9248b9536886 100644
--- a/libsbefifo/cmd_mpipl.c
+++ b/libsbefifo/cmd_mpipl.c
@@ -61,12 +61,18 @@  int sbefifo_mpipl_enter(struct sbefifo_context *sctx)
 	if (rc)
 		return rc;
 
+	rc = sbefifo_set_long_timeout(sctx);
+	if (rc)
+		return rc;
+
 	out_len = 0;
 	rc = sbefifo_operation(sctx, msg, msg_len, &out, &out_len);
 	free(msg);
 	if (rc)
 		return rc;
 
+	sbefifo_reset_timeout(sctx);
+
 	rc = sbefifo_mpipl_enter_pull(out, out_len);
 	if (out)
 		free(out);
@@ -112,12 +118,18 @@  int sbefifo_mpipl_continue(struct sbefifo_context *sctx)
 	if (rc)
 		return rc;
 
+	rc = sbefifo_set_long_timeout(sctx);
+	if (rc)
+		return rc;
+
 	out_len = 0;
 	rc = sbefifo_operation(sctx, msg, msg_len, &out, &out_len);
 	free(msg);
 	if (rc)
 		return rc;
 
+	sbefifo_reset_timeout(sctx);
+
 	rc = sbefifo_mpipl_continue_pull(out, out_len);
 	if (out)
 		free(out);
@@ -224,12 +236,18 @@  int sbefifo_mpipl_get_ti_info(struct sbefifo_context *sctx, uint8_t **data, uint
 	if (rc)
 		return rc;
 
+	rc = sbefifo_set_long_timeout(sctx);
+	if (rc)
+		return rc;
+
 	out_len = 0;
 	rc = sbefifo_operation(sctx, msg, msg_len, &out, &out_len);
 	free(msg);
 	if (rc)
 		return rc;
 
+	sbefifo_reset_timeout(sctx);
+
 	rc = sbefifo_mpipl_get_ti_info_pull(out, out_len, data, data_len);
 	if (out)
 		free(out);
diff --git a/libsbefifo/cmd_register.c b/libsbefifo/cmd_register.c
index 1e74b6ec4eb9..076b774cb905 100644
--- a/libsbefifo/cmd_register.c
+++ b/libsbefifo/cmd_register.c
@@ -88,12 +88,18 @@  int sbefifo_register_get(struct sbefifo_context *sctx, uint8_t core_id, uint8_t
 	if (rc)
 		return rc;
 
+	rc = sbefifo_set_long_timeout(sctx);
+	if (rc)
+		return rc;
+
 	out_len = reg_count * 8;
 	rc = sbefifo_operation(sctx, msg, msg_len, &out, &out_len);
 	free(msg);
 	if (rc)
 		return rc;
 
+	sbefifo_reset_timeout(sctx);
+
 	rc = sbefifo_register_get_pull(out, out_len, reg_count, value);
 	if (out)
 		free(out);
@@ -154,12 +160,18 @@  int sbefifo_register_put(struct sbefifo_context *sctx, uint8_t core_id, uint8_t
 	if (rc)
 		return rc;
 
+	rc = sbefifo_set_long_timeout(sctx);
+	if (rc)
+		return rc;
+
 	out_len = 0;
 	rc = sbefifo_operation(sctx, msg, msg_len, &out, &out_len);
 	free(msg);
 	if (rc)
 		return rc;
 
+	sbefifo_reset_timeout(sctx);
+
 	rc = sbefifo_register_put_pull(out, out_len);
 	if (out)
 		free(out);
diff --git a/libsbefifo/cmd_ring.c b/libsbefifo/cmd_ring.c
index 44e8effc4c99..cffb3a119b02 100644
--- a/libsbefifo/cmd_ring.c
+++ b/libsbefifo/cmd_ring.c
@@ -71,6 +71,10 @@  int sbefifo_ring_get(struct sbefifo_context *sctx, uint32_t ring_addr, uint32_t
 	if (rc)
 		return rc;
 
+	rc = sbefifo_set_long_timeout(sctx);
+	if (rc)
+		return rc;
+
 	/* multiples of 64 bits */
 	out_len = (ring_len_bits + 63) / 8;
 	rc = sbefifo_operation(sctx, msg, msg_len, &out, &out_len);
@@ -78,6 +82,8 @@  int sbefifo_ring_get(struct sbefifo_context *sctx, uint32_t ring_addr, uint32_t
 	if (rc)
 		return rc;
 
+	sbefifo_reset_timeout(sctx);
+
 	rc = sbefifo_ring_get_pull(out, out_len, ring_len_bits, ring_data, ring_len);
 	if (out)
 		free(out);
@@ -125,12 +131,18 @@  int sbefifo_ring_put(struct sbefifo_context *sctx, uint16_t ring_mode, uint8_t *
 	if (rc)
 		return rc;
 
+	rc = sbefifo_set_long_timeout(sctx);
+	if (rc)
+		return rc;
+
 	out_len = 0;
 	rc = sbefifo_operation(sctx, msg, msg_len, &out, &out_len);
 	free(msg);
 	if (rc)
 		return rc;
 
+	sbefifo_reset_timeout(sctx);
+
 	rc = sbefifo_ring_put_pull(out, out_len);
 	if (out)
 		free(out);
@@ -185,12 +197,18 @@  int sbefifo_ring_put_from_image(struct sbefifo_context *sctx, uint16_t target, u
 	if (rc)
 		return rc;
 
+	rc = sbefifo_set_long_timeout(sctx);
+	if (rc)
+		return rc;
+
 	out_len = 0;
 	rc = sbefifo_operation(sctx, msg, msg_len, &out, &out_len);
 	free(out);
 	if (rc)
 		return rc;
 
+	sbefifo_reset_timeout(sctx);
+
 	rc = sbefifo_ring_put_from_image_pull(out, out_len);
 	if (out)
 		free(out);