[3/3] test-ipmi-hiomap: Add write-one-byte test

Add test case to write:
  - 1 byte
  - 1 block and 1 byte data

Cc: Andrew Jeffery <andrew@aj.id.au>
Cc: skiboot-stable@lists.ozlabs.org
Signed-off-by: Vasant Hegde <hegdevasant@linux.vnet.ibm.com>
---
 libflash/test/test-ipmi-hiomap.c | 38 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 38 insertions(+)

On Fri, 5 Apr 2019, at 00:03, Vasant Hegde wrote:
> Add test case to write:
>   - 1 byte
>   - 1 block and 1 byte data
> 
> Cc: Andrew Jeffery <andrew@aj.id.au>
> Cc: skiboot-stable@lists.ozlabs.org
> Signed-off-by: Vasant Hegde <hegdevasant@linux.vnet.ibm.com>
> ---
>  libflash/test/test-ipmi-hiomap.c | 38 ++++++++++++++++++++++++++++++++++++++
>  1 file changed, 38 insertions(+)
> 
> diff --git a/libflash/test/test-ipmi-hiomap.c b/libflash/test/test-ipmi-hiomap.c
> index c4cc76d8c..315d76248 100644
> --- a/libflash/test/test-ipmi-hiomap.c
> +++ b/libflash/test/test-ipmi-hiomap.c
> @@ -1061,6 +1061,23 @@ static void test_hiomap_protocol_write_one_block(void)
>  	scenario_exit();
>  }
>  
> +static void test_hiomap_protocol_write_one_byte(void)
> +{
> +	struct blocklevel_device *bl;
> +	uint8_t *buf;
> +	size_t len;
> +
> +	scenario_enter(scenario_hiomap_protocol_write_one_block);
> +	assert(!ipmi_hiomap_init(&bl));
> +	len = 1;
> +	buf = calloc(1, len);
> +	assert(buf);
> +	assert(!bl->write(bl, 0, buf, len));
> +	free(buf);
> +	ipmi_hiomap_exit(bl);
> +	scenario_exit();
> +}
> +
>  static const struct scenario_event
>  scenario_hiomap_protocol_write_two_blocks[] = {
>  	{ .type = scenario_event_p, .p = &hiomap_ack_call, },
> @@ -1128,6 +1145,25 @@ static void test_hiomap_protocol_write_two_blocks(void)
>  	scenario_exit();
>  }
>  
> +static void test_hiomap_protocol_write_1block_1byte(void)
> +{
> +	struct blocklevel_device *bl;
> +	struct ipmi_hiomap *ctx;
> +	uint8_t *buf;
> +	size_t len;
> +
> +	scenario_enter(scenario_hiomap_protocol_write_two_blocks);
> +	assert(!ipmi_hiomap_init(&bl));
> +	ctx = container_of(bl, struct ipmi_hiomap, bl);
> +	len =  (1 << ctx->block_size_shift) + 1;
> +	buf = calloc(1, len);
> +	assert(buf);
> +	assert(!bl->write(bl, 0, buf, len));
> +	free(buf);
> +	ipmi_hiomap_exit(bl);
> +	scenario_exit();
> +}
> +

Looks good. Nice idea with the scenario reuse.

Patch 1/3 also modifies the read-path behaviour because
hiomap_window_move() is common to both read and write windows.
I think we should have a test for the read path too - i.e. that the
requested size at the protocol level least encapsulates the requested
size at the blocklevel API level.

Having said that, note that the failure fixed in this series is irrelevant
in the read path - a window always has to contain at least one block
(even if the host requests 0 blocks), and larger requests that are
truncated simply cause another read window request *if* the BMC
chooses to truncate in a similar manner. I think analysing the read
path in this manner is what lead me to overlook the write path :(

Cheers,

Andrew

>  static const struct scenario_event
>  scenario_hiomap_protocol_write_one_block_twice[] = {
>  	{ .type = scenario_event_p, .p = &hiomap_ack_call, },
> @@ -3079,7 +3115,9 @@ struct test_case test_cases[] = {
>  	TEST_CASE(test_hiomap_protocol_event_before_read),
>  	TEST_CASE(test_hiomap_protocol_event_during_read),
>  	TEST_CASE(test_hiomap_protocol_write_one_block),
> +	TEST_CASE(test_hiomap_protocol_write_one_byte),
>  	TEST_CASE(test_hiomap_protocol_write_two_blocks),
> +	TEST_CASE(test_hiomap_protocol_write_1block_1byte),
>  	TEST_CASE(test_hiomap_protocol_write_one_block_twice),
>  	TEST_CASE(test_hiomap_protocol_event_before_write),
>  	TEST_CASE(test_hiomap_protocol_event_during_write),
> -- 
> 2.14.3
> 
>

On 04/08/2019 07:53 AM, Andrew Jeffery wrote:
> 
> 
> On Fri, 5 Apr 2019, at 00:03, Vasant Hegde wrote:
>> Add test case to write:
>>    - 1 byte
>>    - 1 block and 1 byte data
>>
>> Cc: Andrew Jeffery <andrew@aj.id.au>
>> Cc: skiboot-stable@lists.ozlabs.org
>> Signed-off-by: Vasant Hegde <hegdevasant@linux.vnet.ibm.com>
>> ---
>>   libflash/test/test-ipmi-hiomap.c | 38 ++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 38 insertions(+)
>>
>> diff --git a/libflash/test/test-ipmi-hiomap.c b/libflash/test/test-ipmi-hiomap.c
>> index c4cc76d8c..315d76248 100644
>> --- a/libflash/test/test-ipmi-hiomap.c
>> +++ b/libflash/test/test-ipmi-hiomap.c
>> @@ -1061,6 +1061,23 @@ static void test_hiomap_protocol_write_one_block(void)
>>   	scenario_exit();
>>   }
>>   
>> +static void test_hiomap_protocol_write_one_byte(void)
>> +{
>> +	struct blocklevel_device *bl;
>> +	uint8_t *buf;
>> +	size_t len;
>> +
>> +	scenario_enter(scenario_hiomap_protocol_write_one_block);
>> +	assert(!ipmi_hiomap_init(&bl));
>> +	len = 1;
>> +	buf = calloc(1, len);
>> +	assert(buf);
>> +	assert(!bl->write(bl, 0, buf, len));
>> +	free(buf);
>> +	ipmi_hiomap_exit(bl);
>> +	scenario_exit();
>> +}
>> +
>>   static const struct scenario_event
>>   scenario_hiomap_protocol_write_two_blocks[] = {
>>   	{ .type = scenario_event_p, .p = &hiomap_ack_call, },
>> @@ -1128,6 +1145,25 @@ static void test_hiomap_protocol_write_two_blocks(void)
>>   	scenario_exit();
>>   }
>>   
>> +static void test_hiomap_protocol_write_1block_1byte(void)
>> +{
>> +	struct blocklevel_device *bl;
>> +	struct ipmi_hiomap *ctx;
>> +	uint8_t *buf;
>> +	size_t len;
>> +
>> +	scenario_enter(scenario_hiomap_protocol_write_two_blocks);
>> +	assert(!ipmi_hiomap_init(&bl));
>> +	ctx = container_of(bl, struct ipmi_hiomap, bl);
>> +	len =  (1 << ctx->block_size_shift) + 1;
>> +	buf = calloc(1, len);
>> +	assert(buf);
>> +	assert(!bl->write(bl, 0, buf, len));
>> +	free(buf);
>> +	ipmi_hiomap_exit(bl);
>> +	scenario_exit();
>> +}
>> +
> 
> Looks good. Nice idea with the scenario reuse.
> 
> Patch 1/3 also modifies the read-path behaviour because
> hiomap_window_move() is common to both read and write windows.
> I think we should have a test for the read path too - i.e. that the
> requested size at the protocol level least encapsulates the requested
> size at the blocklevel API level.

Yeah. You are right. My bad. I should have added test cases for read path
as well. Will fix it in v2.

-Vasant