| Message ID | 20190215065708.6086-10-andrew@aj.id.au |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | ipmi-hiomap: Tests and fixes for event handling | expand |
Andrew Jeffery <andrew@aj.id.au> writes: > impi_hiomap_handle_events()'s implementation contained an early-exit > that was only valid for handling ackable events, but the state provided > in the BMC's notification contains non-ackable state as well. Remove the > early exit to ensure we don't attempt to proceed with requests that we > know will fail which occur after an initial request has cleared the > ackable bits (but failed to restore the window state). > > Include a test case to capture the desired behaviour. > > Cc: Vasant Hegde <hegdevasant@linux.vnet.ibm.com> > Signed-off-by: Andrew Jeffery <andrew@aj.id.au> Also cc stable?
On 02/15/2019 12:26 PM, Andrew Jeffery wrote: > impi_hiomap_handle_events()'s implementation contained an early-exit > that was only valid for handling ackable events, but the state provided > in the BMC's notification contains non-ackable state as well. Remove the > early exit to ensure we don't attempt to proceed with requests that we > know will fail which occur after an initial request has cleared the > ackable bits (but failed to restore the window state). > > Include a test case to capture the desired behaviour. > > Cc: Vasant Hegde <hegdevasant@linux.vnet.ibm.com> > Signed-off-by: Andrew Jeffery <andrew@aj.id.au> Looks good to me. It fixes the issue I was talking earlier. Tested-by: Vasant Hegde <hegdevasant@linux.vnet.ibm.com> @Stewart, We should pick this for stable #v6.0.x + -Vasant
On Fri, 22 Feb 2019, at 16:52, Vasant Hegde wrote: > On 02/15/2019 12:26 PM, Andrew Jeffery wrote: > > impi_hiomap_handle_events()'s implementation contained an early-exit > > that was only valid for handling ackable events, but the state provided > > in the BMC's notification contains non-ackable state as well. Remove the > > early exit to ensure we don't attempt to proceed with requests that we > > know will fail which occur after an initial request has cleared the > > ackable bits (but failed to restore the window state). > > > > Include a test case to capture the desired behaviour. > > > > Cc: Vasant Hegde <hegdevasant@linux.vnet.ibm.com> > > Signed-off-by: Andrew Jeffery <andrew@aj.id.au> > > Looks good to me. It fixes the issue I was talking earlier. > > Tested-by: Vasant Hegde <hegdevasant@linux.vnet.ibm.com> Note that there's a v2 of the series on the list (though I see you found that, and it's also already been merged). Andrew > > @Stewart, > We should pick this for stable #v6.0.x + > > -Vasant > >
diff --git a/libflash/ipmi-hiomap.c b/libflash/ipmi-hiomap.c index ac47a1005ef8..3d6197541cd6 100644 --- a/libflash/ipmi-hiomap.c +++ b/libflash/ipmi-hiomap.c @@ -447,7 +447,6 @@ static void hiomap_event(uint8_t events, void *context) lock(&ctx->lock); ctx->bmc_state = events | (ctx->bmc_state & HIOMAP_E_ACK_MASK); - ctx->update = true; unlock(&ctx->lock); } @@ -562,20 +561,12 @@ static bool ipmi_hiomap_restore_window(struct ipmi_hiomap *ctx) static int ipmi_hiomap_handle_events(struct ipmi_hiomap *ctx) { uint8_t status; - bool update; lock(&ctx->lock); status = ctx->bmc_state; - update = ctx->update; - if (update) { - ctx->bmc_state &= ~HIOMAP_E_ACK_MASK; - ctx->update = false; - } + ctx->bmc_state &= ~HIOMAP_E_ACK_MASK; unlock(&ctx->lock); - if (!update) - return 0; - if (!(status & HIOMAP_E_DAEMON_READY)) { prerror("Daemon not ready\n"); return FLASH_ERR_DEVICE_GONE; diff --git a/libflash/ipmi-hiomap.h b/libflash/ipmi-hiomap.h index f60f725d2ef0..edd4ee0a364f 100644 --- a/libflash/ipmi-hiomap.h +++ b/libflash/ipmi-hiomap.h @@ -48,7 +48,6 @@ struct ipmi_hiomap { * three variables are protected by lock to avoid conflict. */ struct lock lock; - bool update; uint8_t bmc_state; enum lpc_window_state window_state; }; diff --git a/libflash/test/test-ipmi-hiomap.c b/libflash/test/test-ipmi-hiomap.c index 4e1232f22ed0..08c9b32f816a 100644 --- a/libflash/test/test-ipmi-hiomap.c +++ b/libflash/test/test-ipmi-hiomap.c @@ -1,5 +1,6 @@ #include <assert.h> #include <ccan/container_of/container_of.h> +#include <libflash/blocklevel.h> #include <lock.h> #include <lpc.h> #include <hiomap.h> @@ -12,6 +13,9 @@ #include "../ipmi-hiomap.h" #include "../errors.h" +/* Stub for blocklevel debug macros */ +bool libflash_debug; + const struct bmc_sw_config bmc_sw_hiomap = { .ipmi_oem_hiomap_cmd = IPMI_CODE(0x3a, 0x5a), }; @@ -638,6 +642,34 @@ static void test_hiomap_protocol_reset_recovery(void) free(buf); } +static const struct scenario_event +scenario_hiomap_protocol_persistent_error[] = { + { .type = scenario_event_p, .p = &hiomap_ack_call, }, + { .type = scenario_event_p, .p = &hiomap_get_info_call, }, + { .type = scenario_event_p, .p = &hiomap_get_flash_info_call, }, + { .type = scenario_sel, .s = { .bmc_state = HIOMAP_E_PROTOCOL_RESET } }, + SCENARIO_SENTINEL, +}; + +static void test_hiomap_protocol_persistent_error(void) +{ + struct blocklevel_device *bl; + struct ipmi_hiomap *ctx; + char buf; + int rc; + + scenario_enter(scenario_hiomap_protocol_persistent_error); + assert(!ipmi_hiomap_init(&bl)); + ctx = container_of(bl, struct ipmi_hiomap, bl); + assert(ctx->bmc_state == HIOMAP_E_PROTOCOL_RESET); + rc = bl->read(bl, 0, &buf, sizeof(buf)); + assert(rc == FLASH_ERR_DEVICE_GONE); + rc = bl->read(bl, 0, &buf, sizeof(buf)); + assert(rc == FLASH_ERR_DEVICE_GONE); + ipmi_hiomap_exit(bl); + scenario_exit(); +} + struct test_case { const char *name; void (*fn)(void); @@ -653,6 +685,7 @@ struct test_case test_cases[] = { TEST_CASE(test_hiomap_event_daemon_lost_flash_control), TEST_CASE(test_hiomap_event_daemon_regained_flash_control_dirty), TEST_CASE(test_hiomap_protocol_reset_recovery), + TEST_CASE(test_hiomap_protocol_persistent_error), { NULL, NULL }, };
impi_hiomap_handle_events()'s implementation contained an early-exit that was only valid for handling ackable events, but the state provided in the BMC's notification contains non-ackable state as well. Remove the early exit to ensure we don't attempt to proceed with requests that we know will fail which occur after an initial request has cleared the ackable bits (but failed to restore the window state). Include a test case to capture the desired behaviour. Cc: Vasant Hegde <hegdevasant@linux.vnet.ibm.com> Signed-off-by: Andrew Jeffery <andrew@aj.id.au> --- libflash/ipmi-hiomap.c | 11 +---------- libflash/ipmi-hiomap.h | 1 - libflash/test/test-ipmi-hiomap.c | 33 ++++++++++++++++++++++++++++++++ 3 files changed, 34 insertions(+), 11 deletions(-)