Skip to content

Commit d3abaf4

Browse files
djbwdjbw
committed
acpi, nfit: Fix Address Range Scrub completion tracking
The Address Range Scrub implementation tried to skip running scrubs against ranges that were already scrubbed by the BIOS. Unfortunately that support also resulted in early scrub completions as evidenced by this debug output from nfit_test: nd_region region9: ARS: range 1 short complete nd_region region3: ARS: range 1 short complete nd_region region4: ARS: range 2 ARS start (0) nd_region region4: ARS: range 2 short complete ...i.e. completions without any indications that the scrub was started. This state of affairs was hard to see in the code due to the proliferation of state bits and mistakenly trying to track done state per-range when the completion is a global property of the bus. So, kill the four ARS state bits (ARS_REQ, ARS_REQ_REDO, ARS_DONE, and ARS_SHORT), and replace them with just 2 request flags ARS_REQ_SHORT and ARS_REQ_LONG. The implementation will still complete and reap the results of BIOS initiated ARS, but it will not attempt to use that information to affect the completion status of scrubbing the ranges from a Linux perspective. Instead, try to synchronously run a short ARS per range at init time and schedule a long scrub in the background. If ARS is busy with an ARS request, schedule both a short and a long scrub for when ARS returns to idle. This logic also satisfies the intent of what ARS_REQ_REDO was trying to achieve. The new rule is that the REQ flag stays set until the next successful ars_start() for that range. With the new policy that the REQ flags are not cleared until the next start, the implementation no longer loses requests as can be seen from the following log: nd_region region3: ARS: range 1 ARS start short (0) nd_region region9: ARS: range 1 ARS start short (0) nd_region region3: ARS: range 1 complete nd_region region4: ARS: range 2 ARS start short (0) nd_region region9: ARS: range 1 complete nd_region region9: ARS: range 1 ARS start long (0) nd_region region4: ARS: range 2 complete nd_region region3: ARS: range 1 ARS start long (0) nd_region region9: ARS: range 1 complete nd_region region3: ARS: range 1 complete nd_region region4: ARS: range 2 ARS start long (0) nd_region region4: ARS: range 2 complete ...note that the nfit_test emulated driver provides 2 buses, that is why some of the range indices are duplicated. Notice that each range now successfully completes a short and long scrub. Cc: <[email protected]> Fixes: 14c73f9 ("nfit, address-range-scrub: introduce nfit_spa->ars_state") Fixes: cc3d345 ("acpi/nfit: queue issuing of ars when an uc error...") Reported-by: Jacek Zloch <[email protected]> Reported-by: Krzysztof Rusocki <[email protected]> Reviewed-by: Dave Jiang <[email protected]> Signed-off-by: Dan Williams <[email protected]>
1 parent f366d32 commit d3abaf4

File tree

2 files changed

+101
-78
lines changed

2 files changed

+101
-78
lines changed

‎drivers/acpi/nfit/core.c

Lines changed: 96 additions & 73 deletions
Original file line numberDiff line numberDiff line change
@@ -2549,7 +2549,8 @@ static int ars_get_cap(struct acpi_nfit_desc *acpi_desc,
25492549
return cmd_rc;
25502550
}
25512551

2552-
static int ars_start(struct acpi_nfit_desc *acpi_desc, struct nfit_spa *nfit_spa)
2552+
static int ars_start(struct acpi_nfit_desc *acpi_desc,
2553+
struct nfit_spa *nfit_spa, enum nfit_ars_state req_type)
25532554
{
25542555
int rc;
25552556
int cmd_rc;
@@ -2560,7 +2561,7 @@ static int ars_start(struct acpi_nfit_desc *acpi_desc, struct nfit_spa *nfit_spa
25602561
memset(&ars_start, 0, sizeof(ars_start));
25612562
ars_start.address = spa->address;
25622563
ars_start.length = spa->length;
2563-
if (test_bit(ARS_SHORT, &nfit_spa->ars_state))
2564+
if (req_type == ARS_REQ_SHORT)
25642565
ars_start.flags = ND_ARS_RETURN_PREV_DATA;
25652566
if (nfit_spa_type(spa) == NFIT_SPA_PM)
25662567
ars_start.type = ND_ARS_PERSISTENT;
@@ -2617,6 +2618,15 @@ static void ars_complete(struct acpi_nfit_desc *acpi_desc,
26172618
struct nd_region *nd_region = nfit_spa->nd_region;
26182619
struct device *dev;
26192620

2621+
lockdep_assert_held(&acpi_desc->init_mutex);
2622+
/*
2623+
* Only advance the ARS state for ARS runs initiated by the
2624+
* kernel, ignore ARS results from BIOS initiated runs for scrub
2625+
* completion tracking.
2626+
*/
2627+
if (acpi_desc->scrub_spa != nfit_spa)
2628+
return;
2629+
26202630
if ((ars_status->address >= spa->address && ars_status->address
26212631
< spa->address + spa->length)
26222632
|| (ars_status->address < spa->address)) {
@@ -2636,28 +2646,13 @@ static void ars_complete(struct acpi_nfit_desc *acpi_desc,
26362646
} else
26372647
return;
26382648

2639-
if (test_bit(ARS_DONE, &nfit_spa->ars_state))
2640-
return;
2641-
2642-
if (!test_and_clear_bit(ARS_REQ, &nfit_spa->ars_state))
2643-
return;
2644-
2649+
acpi_desc->scrub_spa = NULL;
26452650
if (nd_region) {
26462651
dev = nd_region_dev(nd_region);
26472652
nvdimm_region_notify(nd_region, NVDIMM_REVALIDATE_POISON);
26482653
} else
26492654
dev = acpi_desc->dev;
2650-
2651-
dev_dbg(dev, "ARS: range %d %s complete\n", spa->range_index,
2652-
test_bit(ARS_SHORT, &nfit_spa->ars_state)
2653-
? "short" : "long");
2654-
clear_bit(ARS_SHORT, &nfit_spa->ars_state);
2655-
if (test_and_clear_bit(ARS_REQ_REDO, &nfit_spa->ars_state)) {
2656-
set_bit(ARS_SHORT, &nfit_spa->ars_state);
2657-
set_bit(ARS_REQ, &nfit_spa->ars_state);
2658-
dev_dbg(dev, "ARS: processing scrub request received while in progress\n");
2659-
} else
2660-
set_bit(ARS_DONE, &nfit_spa->ars_state);
2655+
dev_dbg(dev, "ARS: range %d complete\n", spa->range_index);
26612656
}
26622657

26632658
static int ars_status_process_records(struct acpi_nfit_desc *acpi_desc)
@@ -2938,46 +2933,55 @@ static int acpi_nfit_query_poison(struct acpi_nfit_desc *acpi_desc)
29382933
return 0;
29392934
}
29402935

2941-
static int ars_register(struct acpi_nfit_desc *acpi_desc, struct nfit_spa *nfit_spa,
2942-
int *query_rc)
2936+
static int ars_register(struct acpi_nfit_desc *acpi_desc,
2937+
struct nfit_spa *nfit_spa)
29432938
{
2944-
int rc = *query_rc;
2939+
int rc;
29452940

2946-
if (no_init_ars)
2941+
if (no_init_ars || test_bit(ARS_FAILED, &nfit_spa->ars_state))
29472942
return acpi_nfit_register_region(acpi_desc, nfit_spa);
29482943

2949-
set_bit(ARS_REQ, &nfit_spa->ars_state);
2950-
set_bit(ARS_SHORT, &nfit_spa->ars_state);
2944+
set_bit(ARS_REQ_SHORT, &nfit_spa->ars_state);
2945+
set_bit(ARS_REQ_LONG, &nfit_spa->ars_state);
29512946

2952-
switch (rc) {
2947+
switch (acpi_nfit_query_poison(acpi_desc)) {
29532948
case 0:
29542949
case -EAGAIN:
2955-
rc = ars_start(acpi_desc, nfit_spa);
2956-
if (rc == -EBUSY) {
2957-
*query_rc = rc;
2950+
rc = ars_start(acpi_desc, nfit_spa, ARS_REQ_SHORT);
2951+
/* shouldn't happen, try again later */
2952+
if (rc == -EBUSY)
29582953
break;
2959-
} else if (rc == 0) {
2960-
rc = acpi_nfit_query_poison(acpi_desc);
2961-
} else {
2954+
if (rc) {
29622955
set_bit(ARS_FAILED, &nfit_spa->ars_state);
29632956
break;
29642957
}
2965-
if (rc == -EAGAIN)
2966-
clear_bit(ARS_SHORT, &nfit_spa->ars_state);
2967-
else if (rc == 0)
2968-
ars_complete(acpi_desc, nfit_spa);
2958+
clear_bit(ARS_REQ_SHORT, &nfit_spa->ars_state);
2959+
rc = acpi_nfit_query_poison(acpi_desc);
2960+
if (rc)
2961+
break;
2962+
acpi_desc->scrub_spa = nfit_spa;
2963+
ars_complete(acpi_desc, nfit_spa);
2964+
/*
2965+
* If ars_complete() says we didn't complete the
2966+
* short scrub, we'll try again with a long
2967+
* request.
2968+
*/
2969+
acpi_desc->scrub_spa = NULL;
29692970
break;
29702971
case -EBUSY:
2972+
case -ENOMEM:
29712973
case -ENOSPC:
2974+
/*
2975+
* BIOS was using ARS, wait for it to complete (or
2976+
* resources to become available) and then perform our
2977+
* own scrubs.
2978+
*/
29722979
break;
29732980
default:
29742981
set_bit(ARS_FAILED, &nfit_spa->ars_state);
29752982
break;
29762983
}
29772984

2978-
if (test_and_clear_bit(ARS_DONE, &nfit_spa->ars_state))
2979-
set_bit(ARS_REQ, &nfit_spa->ars_state);
2980-
29812985
return acpi_nfit_register_region(acpi_desc, nfit_spa);
29822986
}
29832987

@@ -2999,6 +3003,8 @@ static unsigned int __acpi_nfit_scrub(struct acpi_nfit_desc *acpi_desc,
29993003
struct device *dev = acpi_desc->dev;
30003004
struct nfit_spa *nfit_spa;
30013005

3006+
lockdep_assert_held(&acpi_desc->init_mutex);
3007+
30023008
if (acpi_desc->cancel)
30033009
return 0;
30043010

@@ -3022,21 +3028,49 @@ static unsigned int __acpi_nfit_scrub(struct acpi_nfit_desc *acpi_desc,
30223028

30233029
ars_complete_all(acpi_desc);
30243030
list_for_each_entry(nfit_spa, &acpi_desc->spas, list) {
3031+
enum nfit_ars_state req_type;
3032+
int rc;
3033+
30253034
if (test_bit(ARS_FAILED, &nfit_spa->ars_state))
30263035
continue;
3027-
if (test_bit(ARS_REQ, &nfit_spa->ars_state)) {
3028-
int rc = ars_start(acpi_desc, nfit_spa);
3029-
3030-
clear_bit(ARS_DONE, &nfit_spa->ars_state);
3031-
dev = nd_region_dev(nfit_spa->nd_region);
3032-
dev_dbg(dev, "ARS: range %d ARS start (%d)\n",
3033-
nfit_spa->spa->range_index, rc);
3034-
if (rc == 0 || rc == -EBUSY)
3035-
return 1;
3036-
dev_err(dev, "ARS: range %d ARS failed (%d)\n",
3037-
nfit_spa->spa->range_index, rc);
3038-
set_bit(ARS_FAILED, &nfit_spa->ars_state);
3036+
3037+
/* prefer short ARS requests first */
3038+
if (test_bit(ARS_REQ_SHORT, &nfit_spa->ars_state))
3039+
req_type = ARS_REQ_SHORT;
3040+
else if (test_bit(ARS_REQ_LONG, &nfit_spa->ars_state))
3041+
req_type = ARS_REQ_LONG;
3042+
else
3043+
continue;
3044+
rc = ars_start(acpi_desc, nfit_spa, req_type);
3045+
3046+
dev = nd_region_dev(nfit_spa->nd_region);
3047+
dev_dbg(dev, "ARS: range %d ARS start %s (%d)\n",
3048+
nfit_spa->spa->range_index,
3049+
req_type == ARS_REQ_SHORT ? "short" : "long",
3050+
rc);
3051+
/*
3052+
* Hmm, we raced someone else starting ARS? Try again in
3053+
* a bit.
3054+
*/
3055+
if (rc == -EBUSY)
3056+
return 1;
3057+
if (rc == 0) {
3058+
dev_WARN_ONCE(dev, acpi_desc->scrub_spa,
3059+
"scrub start while range %d active\n",
3060+
acpi_desc->scrub_spa->spa->range_index);
3061+
clear_bit(req_type, &nfit_spa->ars_state);
3062+
acpi_desc->scrub_spa = nfit_spa;
3063+
/*
3064+
* Consider this spa last for future scrub
3065+
* requests
3066+
*/
3067+
list_move_tail(&nfit_spa->list, &acpi_desc->spas);
3068+
return 1;
30393069
}
3070+
3071+
dev_err(dev, "ARS: range %d ARS failed (%d)\n",
3072+
nfit_spa->spa->range_index, rc);
3073+
set_bit(ARS_FAILED, &nfit_spa->ars_state);
30403074
}
30413075
return 0;
30423076
}
@@ -3092,6 +3126,7 @@ static void acpi_nfit_init_ars(struct acpi_nfit_desc *acpi_desc,
30923126
struct nd_cmd_ars_cap ars_cap;
30933127
int rc;
30943128

3129+
set_bit(ARS_FAILED, &nfit_spa->ars_state);
30953130
memset(&ars_cap, 0, sizeof(ars_cap));
30963131
rc = ars_get_cap(acpi_desc, &ars_cap, nfit_spa);
30973132
if (rc < 0)
@@ -3108,16 +3143,14 @@ static void acpi_nfit_init_ars(struct acpi_nfit_desc *acpi_desc,
31083143
nfit_spa->clear_err_unit = ars_cap.clear_err_unit;
31093144
acpi_desc->max_ars = max(nfit_spa->max_ars, acpi_desc->max_ars);
31103145
clear_bit(ARS_FAILED, &nfit_spa->ars_state);
3111-
set_bit(ARS_REQ, &nfit_spa->ars_state);
31123146
}
31133147

31143148
static int acpi_nfit_register_regions(struct acpi_nfit_desc *acpi_desc)
31153149
{
31163150
struct nfit_spa *nfit_spa;
3117-
int rc, query_rc;
3151+
int rc;
31183152

31193153
list_for_each_entry(nfit_spa, &acpi_desc->spas, list) {
3120-
set_bit(ARS_FAILED, &nfit_spa->ars_state);
31213154
switch (nfit_spa_type(nfit_spa->spa)) {
31223155
case NFIT_SPA_VOLATILE:
31233156
case NFIT_SPA_PM:
@@ -3126,20 +3159,12 @@ static int acpi_nfit_register_regions(struct acpi_nfit_desc *acpi_desc)
31263159
}
31273160
}
31283161

3129-
/*
3130-
* Reap any results that might be pending before starting new
3131-
* short requests.
3132-
*/
3133-
query_rc = acpi_nfit_query_poison(acpi_desc);
3134-
if (query_rc == 0)
3135-
ars_complete_all(acpi_desc);
3136-
31373162
list_for_each_entry(nfit_spa, &acpi_desc->spas, list)
31383163
switch (nfit_spa_type(nfit_spa->spa)) {
31393164
case NFIT_SPA_VOLATILE:
31403165
case NFIT_SPA_PM:
31413166
/* register regions and kick off initial ARS run */
3142-
rc = ars_register(acpi_desc, nfit_spa, &query_rc);
3167+
rc = ars_register(acpi_desc, nfit_spa);
31433168
if (rc)
31443169
return rc;
31453170
break;
@@ -3334,7 +3359,8 @@ static int acpi_nfit_clear_to_send(struct nvdimm_bus_descriptor *nd_desc,
33343359
return 0;
33353360
}
33363361

3337-
int acpi_nfit_ars_rescan(struct acpi_nfit_desc *acpi_desc, unsigned long flags)
3362+
int acpi_nfit_ars_rescan(struct acpi_nfit_desc *acpi_desc,
3363+
enum nfit_ars_state req_type)
33383364
{
33393365
struct device *dev = acpi_desc->dev;
33403366
int scheduled = 0, busy = 0;
@@ -3354,14 +3380,10 @@ int acpi_nfit_ars_rescan(struct acpi_nfit_desc *acpi_desc, unsigned long flags)
33543380
if (test_bit(ARS_FAILED, &nfit_spa->ars_state))
33553381
continue;
33563382

3357-
if (test_and_set_bit(ARS_REQ, &nfit_spa->ars_state)) {
3383+
if (test_and_set_bit(req_type, &nfit_spa->ars_state))
33583384
busy++;
3359-
set_bit(ARS_REQ_REDO, &nfit_spa->ars_state);
3360-
} else {
3361-
if (test_bit(ARS_SHORT, &flags))
3362-
set_bit(ARS_SHORT, &nfit_spa->ars_state);
3385+
else
33633386
scheduled++;
3364-
}
33653387
}
33663388
if (scheduled) {
33673389
sched_ars(acpi_desc);
@@ -3547,10 +3569,11 @@ static void acpi_nfit_update_notify(struct device *dev, acpi_handle handle)
35473569
static void acpi_nfit_uc_error_notify(struct device *dev, acpi_handle handle)
35483570
{
35493571
struct acpi_nfit_desc *acpi_desc = dev_get_drvdata(dev);
3550-
unsigned long flags = (acpi_desc->scrub_mode == HW_ERROR_SCRUB_ON) ?
3551-
0 : 1 << ARS_SHORT;
35523572

3553-
acpi_nfit_ars_rescan(acpi_desc, flags);
3573+
if (acpi_desc->scrub_mode == HW_ERROR_SCRUB_ON)
3574+
acpi_nfit_ars_rescan(acpi_desc, ARS_REQ_LONG);
3575+
else
3576+
acpi_nfit_ars_rescan(acpi_desc, ARS_REQ_SHORT);
35543577
}
35553578

35563579
void __acpi_nfit_notify(struct device *dev, acpi_handle handle, u32 event)

‎drivers/acpi/nfit/nfit.h

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -118,10 +118,8 @@ enum nfit_dimm_notifiers {
118118
};
119119

120120
enum nfit_ars_state {
121-
ARS_REQ,
122-
ARS_REQ_REDO,
123-
ARS_DONE,
124-
ARS_SHORT,
121+
ARS_REQ_SHORT,
122+
ARS_REQ_LONG,
125123
ARS_FAILED,
126124
};
127125

@@ -205,6 +203,7 @@ struct acpi_nfit_desc {
205203
struct device *dev;
206204
u8 ars_start_flags;
207205
struct nd_cmd_ars_status *ars_status;
206+
struct nfit_spa *scrub_spa;
208207
struct delayed_work dwork;
209208
struct list_head list;
210209
struct kernfs_node *scrub_count_state;
@@ -259,7 +258,8 @@ struct nfit_blk {
259258

260259
extern struct list_head acpi_descs;
261260
extern struct mutex acpi_desc_lock;
262-
int acpi_nfit_ars_rescan(struct acpi_nfit_desc *acpi_desc, unsigned long flags);
261+
int acpi_nfit_ars_rescan(struct acpi_nfit_desc *acpi_desc,
262+
enum nfit_ars_state req_type);
263263

264264
#ifdef CONFIG_X86_MCE
265265
void nfit_mce_register(void);

0 commit comments

Comments
 (0)