Re: [PATCH v7 5/6] pmem: refactor pmem_clear_poison()

[Dan Williams]

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>