On Tue, Apr 5, 2022 at 12:48 PM Jane Chu <jane.chu@xxxxxxxxxx> wrote:
>
> Refactor the pmem_clear_poison() in order to share common code
> later.
>
I would just add a note here about why, i.e. to factor out the common
shared code between the typical write path and the recovery write
path.
> Signed-off-by: Jane Chu <jane.chu@xxxxxxxxxx>
> ---
> drivers/nvdimm/pmem.c | 78 ++++++++++++++++++++++++++++---------------
> 1 file changed, 52 insertions(+), 26 deletions(-)
>
> diff --git a/drivers/nvdimm/pmem.c b/drivers/nvdimm/pmem.c
> index 0400c5a7ba39..56596be70400 100644
> --- a/drivers/nvdimm/pmem.c
> +++ b/drivers/nvdimm/pmem.c
> @@ -45,10 +45,27 @@ static struct nd_region *to_region(struct pmem_device *pmem)
> return to_nd_region(to_dev(pmem)->parent);
> }
>
> -static void hwpoison_clear(struct pmem_device *pmem,
> - phys_addr_t phys, unsigned int len)
> +static phys_addr_t to_phys(struct pmem_device *pmem, phys_addr_t offset)
> {
> + return (pmem->phys_addr + offset);
Christoph already mentioned dropping the unnecessary parenthesis.
> +}
> +
> +static sector_t to_sect(struct pmem_device *pmem, phys_addr_t offset)
> +{
> + return (offset - pmem->data_offset) >> SECTOR_SHIFT;
> +}
> +
> +static phys_addr_t to_offset(struct pmem_device *pmem, sector_t sector)
> +{
> + return ((sector << SECTOR_SHIFT) + pmem->data_offset);
> +}
> +
> +static void pmem_clear_hwpoison(struct pmem_device *pmem, phys_addr_t offset,
> + unsigned int len)
Perhaps now is a good time to rename this to something else like
pmem_clear_mce_nospec()? Just to make it more distinct from
pmem_clear_poison(). While "hwpoison" is the page flag name
pmem_clear_poison() is the function that's actually clearing the
poison in hardware ("hw") and the new pmem_clear_mce_nospec() is
toggling the page back into service.
> +{
> + phys_addr_t phys = to_phys(pmem, offset);
> unsigned long pfn_start, pfn_end, pfn;
> + unsigned int blks = len >> SECTOR_SHIFT;
>
> /* only pmem in the linear map supports HWPoison */
> if (is_vmalloc_addr(pmem->virt_addr))
> @@ -67,35 +84,44 @@ static void hwpoison_clear(struct pmem_device *pmem,
> if (test_and_clear_pmem_poison(page))
> clear_mce_nospec(pfn);
> }
> +
> + dev_dbg(to_dev(pmem), "%#llx clear %u sector%s\n",
> + (unsigned long long) to_sect(pmem, offset), blks,
> + blks > 1 ? "s" : "");
In anticipation of better tracing support and the fact that this is no
longer called from pmem_clear_poison() let's drop it for now.
> }
>
> -static blk_status_t pmem_clear_poison(struct pmem_device *pmem,
> +static void pmem_clear_bb(struct pmem_device *pmem, sector_t sector, long blks)
> +{
> + if (blks == 0)
> + return;
> + badblocks_clear(&pmem->bb, sector, blks);
> + if (pmem->bb_state)
> + sysfs_notify_dirent(pmem->bb_state);
> +}
> +
> +static long __pmem_clear_poison(struct pmem_device *pmem,
> phys_addr_t offset, unsigned int len)
> {
> - struct device *dev = to_dev(pmem);
> - sector_t sector;
> - long cleared;
> - blk_status_t rc = BLK_STS_OK;
> -
> - sector = (offset - pmem->data_offset) / 512;
> -
> - cleared = nvdimm_clear_poison(dev, pmem->phys_addr + offset, len);
> - if (cleared < len)
> - rc = BLK_STS_IOERR;
> - if (cleared > 0 && cleared / 512) {
> - hwpoison_clear(pmem, pmem->phys_addr + offset, cleared);
> - cleared /= 512;
> - dev_dbg(dev, "%#llx clear %ld sector%s\n",
> - (unsigned long long) sector, cleared,
> - cleared > 1 ? "s" : "");
> - badblocks_clear(&pmem->bb, sector, cleared);
> - if (pmem->bb_state)
> - sysfs_notify_dirent(pmem->bb_state);
> + phys_addr_t phys = to_phys(pmem, offset);
> + long cleared = nvdimm_clear_poison(to_dev(pmem), phys, len);
> +
> + if (cleared > 0) {
> + pmem_clear_hwpoison(pmem, offset, cleared);
> + arch_invalidate_pmem(pmem->virt_addr + offset, len);
> }
> + return cleared;
> +}
>
> - arch_invalidate_pmem(pmem->virt_addr + offset, len);
> +static blk_status_t pmem_clear_poison(struct pmem_device *pmem,
> + phys_addr_t offset, unsigned int len)
> +{
> + long cleared = __pmem_clear_poison(pmem, offset, len);
>
> - return rc;
> + if (cleared < 0)
> + return BLK_STS_IOERR;
> +
> + pmem_clear_bb(pmem, to_sect(pmem, offset), cleared >> SECTOR_SHIFT);
> + return (cleared < len) ? BLK_STS_IOERR : BLK_STS_OK;
I prefer "if / else" syntax instead of a ternary conditional.
> }
>
> static void write_pmem(void *pmem_addr, struct page *page,
> @@ -143,7 +169,7 @@ static blk_status_t pmem_do_read(struct pmem_device *pmem,
> sector_t sector, unsigned int len)
> {
> blk_status_t rc;
> - phys_addr_t pmem_off = sector * 512 + pmem->data_offset;
> + phys_addr_t pmem_off = to_offset(pmem, sector);
> void *pmem_addr = pmem->virt_addr + pmem_off;
>
> if (unlikely(is_bad_pmem(&pmem->bb, sector, len)))
> @@ -158,7 +184,7 @@ static blk_status_t pmem_do_write(struct pmem_device *pmem,
> struct page *page, unsigned int page_off,
> sector_t sector, unsigned int len)
> {
> - phys_addr_t pmem_off = sector * 512 + pmem->data_offset;
> + phys_addr_t pmem_off = to_offset(pmem, sector);
> void *pmem_addr = pmem->virt_addr + pmem_off;
>
> if (unlikely(is_bad_pmem(&pmem->bb, sector, len))) {
With those small fixups you can add:
Reviewed-by: Dan Williams <dan.j.williams@xxxxxxxxx>
