On Tue, Aug 8, 2017 at 5:07 PM, Vishal Verma <vishal.l.verma@xxxxxxxxx> wrote:
> In preparation for BTT error clearing, refactor the initial offset
> calculations. Until now, all callers of arena_{read,write}_bytes assumed
> a relative offset to the arena, and it was later adjusted for the
> initial offset. Until now, every time we calculated a relative offset,
> we passed it to these functions to do reads or writes, and so where the
> offset calculations happened didn't matter.
>
> For error clearing, there will be places where we calculate offsets to
> check for the presence of media errors, and the above scheme becomes
> error prone.
>
> Make the initial_offset calculations explicit for all callers of
> arena_{read,write}_bytes, and provide a helper to do them. The error
> checking/clearing code can then use these same helpers.
>
> Reported-by: Toshi Kani <toshi.kani@xxxxxxx>
> Signed-off-by: Vishal Verma <vishal.l.verma@xxxxxxxxx>
> ---
> drivers/nvdimm/btt.c | 69 ++++++++++++++++++++++++++++++++--------------------
> 1 file changed, 42 insertions(+), 27 deletions(-)
>
> diff --git a/drivers/nvdimm/btt.c b/drivers/nvdimm/btt.c
> index e9dd651..9acf06b 100644
> --- a/drivers/nvdimm/btt.c
> +++ b/drivers/nvdimm/btt.c
> @@ -31,14 +31,17 @@ enum log_ent_request {
> LOG_OLD_ENT
> };
>
> +static inline u64 calc_nsoff(struct nd_btt *nd_btt, u64 relative_offset)
> +{
> + return relative_offset + nd_btt->initial_offset;
> +}
> +
> static int arena_read_bytes(struct arena_info *arena, resource_size_t offset,
> void *buf, size_t n, unsigned long flags)
> {
> struct nd_btt *nd_btt = arena->nd_btt;
> struct nd_namespace_common *ndns = nd_btt->ndns;
>
> - /* arena offsets may be shifted from the base of the device */
> - offset += arena->nd_btt->initial_offset;
> return nvdimm_read_bytes(ndns, offset, buf, n, flags);
> }
>
> @@ -48,14 +51,13 @@ static int arena_write_bytes(struct arena_info *arena, resource_size_t offset,
> struct nd_btt *nd_btt = arena->nd_btt;
> struct nd_namespace_common *ndns = nd_btt->ndns;
>
> - /* arena offsets may be shifted from the base of the device */
> - offset += arena->nd_btt->initial_offset;
> return nvdimm_write_bytes(ndns, offset, buf, n, flags);
> }
So let's get rid of arena_{read,write}_bytes(). The only point of the
wrapper was to include an arena specific offset, but now we're pushing
all offset calculations to the caller so arena_{read,write}_bytes() is
equivalent to nvdimm_{read,write}_bytes(). However, since many of
these call sites now perform a common offset calculations maybe we can
provide a new helper for that to differentiate it from the error
clearing case? After patch 7 is applied it's no longer clear which
offset calculation is used where and for what reason, there's just too
many offset variables calculations and overload of the word 'offset'
for bugs to hide.
Maybe it's just late on a Friday and my eyes are going cross, but it
seems you have 2 classes of use cases between calc_nsoff and
lba_to_nsoff, perhaps separate read/write wrappers for those 2 cases?
Also, I find something unreadable about calc_nsoff, and reading the
prototype "u64 calc_nsoff(struct nd_btt *nd_btt, u64 relative_offset)"
doesn't help clarify. What is 'relative_offset' relative to? Is it the
base namespace / device offset?
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
