| Message ID | 20190404133306.27317-1-hegdevasant@linux.vnet.ibm.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [1/3] libflash/ipmi-hiomap: Fix blocks count issue | expand |
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@aj.id.au> > Cc: skiboot-stable@lists.ozlabs.org > Signed-off-by: Vasant Hegde <hegdevasant@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 :) > + > /* Call under ctx->lock */ > static int hiomap_protocol_ready(struct ipmi_hiomap *ctx) > { > @@ -321,7 +331,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 +394,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 +504,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, I've gone through the protocol docs and this appears to be a complete fix. Thanks. Andrew > -- > 2.14.3 > >
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@aj.id.au> >> Cc: skiboot-stable@lists.ozlabs.org >> Signed-off-by: Vasant Hegde <hegdevasant@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
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; } +static inline uint16_t bytes_to_blocks_align_up(struct ipmi_hiomap *ctx, + uint32_t pos, uint32_t bytes) +{ + uint32_t size_shift = 1 << ctx->block_size_shift; + uint32_t pos_offset = pos & (size_shift - 1); + uint32_t len = ALIGN_UP((bytes + pos_offset), size_shift); + + return len >> ctx->block_size_shift; +} + /* Call under ctx->lock */ static int hiomap_protocol_ready(struct ipmi_hiomap *ctx) { @@ -321,7 +331,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 +394,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 +504,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,
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 | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-)