On 4/11/2022 9:26 PM, Dan Williams wrote:
> 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.
Okay.
>
>> 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.
I get your point. How about calling the function explicitly
pmem_mkpage_present()? or pmem_page_present_on()? or
pmem_set_page_present()? because setting a poisoned present requires
both clearing the HWpoison bit & clear mce_nospec.
>
>> +{
>> + 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.
OKay.
>
>> }
>>
>> -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.
>
Got it.
>> }
>>
>> 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>
Thanks!
-jane
