[Skiboot] [PATCH 1/3] libflash/ipmi-hiomap: Fix blocks count issue

[Vasant Hegde]

On 04/08/2019 08:38 AM, Andrew Jeffery wrote:
> 
> 
> On Fri, 5 Apr 2019, at 00:03, Vasant Hegde wrote:
>> We convert data size to block count and pass block count to BMC.
>> If data size is not block aligned then we endup sending block count
>> less than actual data. BMC will write partial data to flash memory.
>>
>> Sample log :
>> [  594.388458416,7] HIOMAP: Marked flash dirty at 0x42010 for 8
>> [  594.398756487,7] HIOMAP: Flushed writes
>> [  594.409596439,7] HIOMAP: Marked flash dirty at 0x42018 for 3970
>> [  594.419897507,7] HIOMAP: Flushed writes
>>
>> In this case HIOMAP sent data with block count=0 and hence BMC didn't
>> flush data to flash.
>>
>> Lets fix this issue by adjusting block count before sending it to BMC.
>>
>> Cc: Andrew Jeffery <andrew at aj.id.au>
>> Cc: skiboot-stable at lists.ozlabs.org
>> Signed-off-by: Vasant Hegde <hegdevasant at linux.vnet.ibm.com>
>> ---
>>   libflash/ipmi-hiomap.c | 16 +++++++++++++---
>>   1 file changed, 13 insertions(+), 3 deletions(-)
>>
>> diff --git a/libflash/ipmi-hiomap.c b/libflash/ipmi-hiomap.c
>> index 56492fa87..da5fa29c9 100644
>> --- a/libflash/ipmi-hiomap.c
>> +++ b/libflash/ipmi-hiomap.c
>> @@ -49,6 +49,16 @@ static inline uint16_t bytes_to_blocks(struct
>> ipmi_hiomap *ctx, uint32_t bytes)
>>   	return bytes >> ctx->block_size_shift;
>>   }
>>   
> 
> I have a bunch of nitty renames below, but I think it improves readability:
> 
>> +static inline uint16_t bytes_to_blocks_align_up(struct ipmi_hiomap
>> *ctx,
>> +						uint32_t pos, uint32_t bytes)
> 
> s/bytes/len
> 
>> +{
>> +	uint32_t size_shift = 1 << ctx->block_size_shift;
> 
> s/size_shift/block_size/
> 
>> +	uint32_t pos_offset = pos & (size_shift - 1);
> 
> Similarly s/pos_offset/delta/
> 
>> +	uint32_t len = ALIGN_UP((bytes + pos_offset), size_shift);
> 
> s/len/aligned/
> 
> Then:
> 
> uint32_t blocks = len >> ctx->block_size_shift;
> 
> We know that we need to fit blocks into a uint16_t as well, but the only
> constraint on ctx->block_size_shift is that it's at least 12. If it is less than
> 16 we probably want to validate that the aligned length we've generated
> fits in a uint16_t, so:
> 
> uint32_t mask = ((1 << 16) - 1);
> 
> assert(!(blocks & ~mask));
> 
> Then:
> 
>   return blocks & mask;
> 
>> +
>> +	return len >> ctx->block_size_shift;
>> +}
> 
> Putting it all together:
> 
> static inline uint16_t bytes_to_blocks_align_up(struct ipmi_hiomap *ctx,
> 						uint32_t pos, uint32_t len)
> {
>      uint32_t block_size = 1 << ctx->block_size_shift;
>      uint32_t delta = pos & (block_size - 1);
>      uint32_t aligned = ALIGN_UP((len + delta), block_size);
>      uint32_t blocks = aligned >> ctx->block_size_shift;
>      uint32_t mask = ((1 << 16) - 1);
> 
>      assert(!(blocks & ~mask));
> 
>      return blocks & mask;
> }
> 
> Thoughts? Note that I haven't tested this suggestion :)

Looks good. It should work fine. Let me test it.

-Vasant