[v2,1/5] libflash/ipmi-hiomap: Fix blocks count issue
diff mbox series

Message ID 20190408060541.11246-1-hegdevasant@linux.vnet.ibm.com
State Accepted
Delegated to: Vasant Hegde
Headers show
Series
  • [v2,1/5] libflash/ipmi-hiomap: Fix blocks count issue
Related show

Commit Message

Vasant Hegde April 8, 2019, 6:05 a.m. UTC
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@aj.id.au>
Cc: skiboot-stable@lists.ozlabs.org
Signed-off-by: Vasant Hegde <hegdevasant@linux.vnet.ibm.com>
---
 libflash/ipmi-hiomap.c | 21 ++++++++++++++++++---
 1 file changed, 18 insertions(+), 3 deletions(-)

Comments

Andrew Jeffery April 9, 2019, 12:53 a.m. UTC | #1
On Mon, 8 Apr 2019, at 15:35, 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@aj.id.au>
> Cc: skiboot-stable@lists.ozlabs.org
> Signed-off-by: Vasant Hegde <hegdevasant@linux.vnet.ibm.com>

Reviewed-by: Andrew Jeffery <andrew@aj.id.au>

Thanks for cleaning up my mess :)

> ---
>  libflash/ipmi-hiomap.c | 21 ++++++++++++++++++---
>  1 file changed, 18 insertions(+), 3 deletions(-)
> 
> diff --git a/libflash/ipmi-hiomap.c b/libflash/ipmi-hiomap.c
> index 56492fa87..9174eabd2 100644
> --- a/libflash/ipmi-hiomap.c
> +++ b/libflash/ipmi-hiomap.c
> @@ -49,6 +49,21 @@ static inline uint16_t bytes_to_blocks(struct 
> ipmi_hiomap *ctx, uint32_t bytes)
>  	return bytes >> ctx->block_size_shift;
>  }
>  
> +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;
> +	/* Our protocol can handle block count < sizeof(u16) */
> +	uint32_t mask = ((1 << 16) - 1);
> +
> +	assert(!(blocks & ~mask));
> +
> +	return blocks & mask;
> +}
> +
>  /* Call under ctx->lock */
>  static int hiomap_protocol_ready(struct ipmi_hiomap *ctx)
>  {
> @@ -321,7 +336,7 @@ static int hiomap_window_move(struct ipmi_hiomap 
> *ctx, uint8_t command,
>  
>  	range = (struct hiomap_v2_range *)&req[2];
>  	range->offset = cpu_to_le16(bytes_to_blocks(ctx, pos));
> -	range->size = cpu_to_le16(bytes_to_blocks(ctx, len));
> +	range->size = cpu_to_le16(bytes_to_blocks_align_up(ctx, pos, len));
>  
>  	msg = ipmi_mkmsg(IPMI_DEFAULT_INTERFACE,
>  		         bmc_platform->sw->ipmi_oem_hiomap_cmd,
> @@ -384,7 +399,7 @@ static int hiomap_mark_dirty(struct ipmi_hiomap 
> *ctx, uint64_t offset,
>  	pos = offset - ctx->current.cur_pos;
>  	range = (struct hiomap_v2_range *)&req[2];
>  	range->offset = cpu_to_le16(bytes_to_blocks(ctx, pos));
> -	range->size = cpu_to_le16(bytes_to_blocks(ctx, size));
> +	range->size = cpu_to_le16(bytes_to_blocks_align_up(ctx, pos, size));
>  
>  	msg = ipmi_mkmsg(IPMI_DEFAULT_INTERFACE,
>  		         bmc_platform->sw->ipmi_oem_hiomap_cmd,
> @@ -494,7 +509,7 @@ static int hiomap_erase(struct ipmi_hiomap *ctx, 
> uint64_t offset,
>  	pos = offset - ctx->current.cur_pos;
>  	range = (struct hiomap_v2_range *)&req[2];
>  	range->offset = cpu_to_le16(bytes_to_blocks(ctx, pos));
> -	range->size = cpu_to_le16(bytes_to_blocks(ctx, size));
> +	range->size = cpu_to_le16(bytes_to_blocks_align_up(ctx, pos, size));
>  
>  	msg = ipmi_mkmsg(IPMI_DEFAULT_INTERFACE,
>  		         bmc_platform->sw->ipmi_oem_hiomap_cmd,
> -- 
> 2.14.3
> 
>
Stewart Smith April 11, 2019, 4:53 a.m. UTC | #2
"Andrew Jeffery" <andrew@aj.id.au> writes:
> On Mon, 8 Apr 2019, at 15:35, 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@aj.id.au>
>> Cc: skiboot-stable@lists.ozlabs.org
>> Signed-off-by: Vasant Hegde <hegdevasant@linux.vnet.ibm.com>
>
> Reviewed-by: Andrew Jeffery <andrew@aj.id.au>
>
> Thanks for cleaning up my mess :)

Pretty amazing we missed having tests for this kind of thing for so
long.

Series merged to master as of 857f046d3ab00ec12dcb06ddabfed6bdfe00a819

Patch
diff mbox series

diff --git a/libflash/ipmi-hiomap.c b/libflash/ipmi-hiomap.c
index 56492fa87..9174eabd2 100644
--- a/libflash/ipmi-hiomap.c
+++ b/libflash/ipmi-hiomap.c
@@ -49,6 +49,21 @@  static inline uint16_t bytes_to_blocks(struct ipmi_hiomap *ctx, uint32_t bytes)
 	return bytes >> ctx->block_size_shift;
 }
 
+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;
+	/* Our protocol can handle block count < sizeof(u16) */
+	uint32_t mask = ((1 << 16) - 1);
+
+	assert(!(blocks & ~mask));
+
+	return blocks & mask;
+}
+
 /* Call under ctx->lock */
 static int hiomap_protocol_ready(struct ipmi_hiomap *ctx)
 {
@@ -321,7 +336,7 @@  static int hiomap_window_move(struct ipmi_hiomap *ctx, uint8_t command,
 
 	range = (struct hiomap_v2_range *)&req[2];
 	range->offset = cpu_to_le16(bytes_to_blocks(ctx, pos));
-	range->size = cpu_to_le16(bytes_to_blocks(ctx, len));
+	range->size = cpu_to_le16(bytes_to_blocks_align_up(ctx, pos, len));
 
 	msg = ipmi_mkmsg(IPMI_DEFAULT_INTERFACE,
 		         bmc_platform->sw->ipmi_oem_hiomap_cmd,
@@ -384,7 +399,7 @@  static int hiomap_mark_dirty(struct ipmi_hiomap *ctx, uint64_t offset,
 	pos = offset - ctx->current.cur_pos;
 	range = (struct hiomap_v2_range *)&req[2];
 	range->offset = cpu_to_le16(bytes_to_blocks(ctx, pos));
-	range->size = cpu_to_le16(bytes_to_blocks(ctx, size));
+	range->size = cpu_to_le16(bytes_to_blocks_align_up(ctx, pos, size));
 
 	msg = ipmi_mkmsg(IPMI_DEFAULT_INTERFACE,
 		         bmc_platform->sw->ipmi_oem_hiomap_cmd,
@@ -494,7 +509,7 @@  static int hiomap_erase(struct ipmi_hiomap *ctx, uint64_t offset,
 	pos = offset - ctx->current.cur_pos;
 	range = (struct hiomap_v2_range *)&req[2];
 	range->offset = cpu_to_le16(bytes_to_blocks(ctx, pos));
-	range->size = cpu_to_le16(bytes_to_blocks(ctx, size));
+	range->size = cpu_to_le16(bytes_to_blocks_align_up(ctx, pos, size));
 
 	msg = ipmi_mkmsg(IPMI_DEFAULT_INTERFACE,
 		         bmc_platform->sw->ipmi_oem_hiomap_cmd,