| Message ID | 20241028125657.1271512-1-pierre.gondois@arm.com (mailing list archive) |
|---|---|
| State | Queued |
| Delegated to: | Rafael Wysocki |
| Headers | show |
| Series | [v2] ACPI: CPPC: Make rmw_lock a raw_spin_lock | expand |
On Mon, Oct 28, 2024 at 1:57 PM Pierre Gondois <pierre.gondois@arm.com> wrote: > > The following BUG was triggered. sugov_update_shared() locks a > raw_spinlock while cpc_write() locks a spinlock. To have a correct > wait-type order, update rmw_lock to a raw_spinlock. > > Also save irq state. > > ============================= > [ BUG: Invalid wait context ] > 6.12.0-rc2-XXX #406 Not tainted > ----------------------------- > kworker/1:1/62 is trying to lock: > ffffff8801593030 (&cpc_ptr->rmw_lock){+.+.}-{3:3}, at: cpc_write+0xcc/0x370 > other info that might help us debug this: > context-{5:5} > 2 locks held by kworker/1:1/62: > #0: ffffff897ef5ec98 (&rq->__lock){-.-.}-{2:2}, at: raw_spin_rq_lock_nested+0x2c/0x50 > #1: ffffff880154e238 (&sg_policy->update_lock){....}-{2:2}, at: sugov_update_shared+0x3c/0x280 > stack backtrace: > CPU: 1 UID: 0 PID: 62 Comm: kworker/1:1 Not tainted 6.12.0-rc2-g9654bd3e8806 #406 > Workqueue: 0x0 (events) > Call trace: > dump_backtrace+0xa4/0x130 > show_stack+0x20/0x38 > dump_stack_lvl+0x90/0xd0 > dump_stack+0x18/0x28 > __lock_acquire+0x480/0x1ad8 > lock_acquire+0x114/0x310 > _raw_spin_lock+0x50/0x70 > cpc_write+0xcc/0x370 > cppc_set_perf+0xa0/0x3a8 > cppc_cpufreq_fast_switch+0x40/0xc0 > cpufreq_driver_fast_switch+0x4c/0x218 > sugov_update_shared+0x234/0x280 > update_load_avg+0x6ec/0x7b8 > dequeue_entities+0x108/0x830 > dequeue_task_fair+0x58/0x408 > __schedule+0x4f0/0x1070 > schedule+0x54/0x130 > worker_thread+0xc0/0x2e8 > kthread+0x130/0x148 > ret_from_fork+0x10/0x20 > > Fixes: 60949b7b8054 ("ACPI: CPPC: Fix MASK_VAL() usage") > Signed-off-by: Pierre Gondois <pierre.gondois@arm.com> > --- > > Notes: > v2: > - Do the actual changes. > > drivers/acpi/cppc_acpi.c | 9 +++++---- > include/acpi/cppc_acpi.h | 2 +- > 2 files changed, 6 insertions(+), 5 deletions(-) > > diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c > index c3fc2c05d868..1a40f0514eaa 100644 > --- a/drivers/acpi/cppc_acpi.c > +++ b/drivers/acpi/cppc_acpi.c > @@ -867,7 +867,7 @@ int acpi_cppc_processor_probe(struct acpi_processor *pr) > > /* Store CPU Logical ID */ > cpc_ptr->cpu_id = pr->id; > - spin_lock_init(&cpc_ptr->rmw_lock); > + raw_spin_lock_init(&cpc_ptr->rmw_lock); > > /* Parse PSD data for this CPU */ > ret = acpi_get_psd(cpc_ptr, handle); > @@ -1087,6 +1087,7 @@ static int cpc_write(int cpu, struct cpc_register_resource *reg_res, u64 val) > int pcc_ss_id = per_cpu(cpu_pcc_subspace_idx, cpu); > struct cpc_reg *reg = ®_res->cpc_entry.reg; > struct cpc_desc *cpc_desc; > + unsigned long flags; > > size = GET_BIT_WIDTH(reg); > > @@ -1126,7 +1127,7 @@ static int cpc_write(int cpu, struct cpc_register_resource *reg_res, u64 val) > return -ENODEV; > } > > - spin_lock(&cpc_desc->rmw_lock); > + raw_spin_lock_irqsave(&cpc_desc->rmw_lock, flags); > switch (size) { > case 8: > prev_val = readb_relaxed(vaddr); > @@ -1141,7 +1142,7 @@ static int cpc_write(int cpu, struct cpc_register_resource *reg_res, u64 val) > prev_val = readq_relaxed(vaddr); > break; > default: > - spin_unlock(&cpc_desc->rmw_lock); > + raw_spin_unlock_irqrestore(&cpc_desc->rmw_lock, flags); > return -EFAULT; > } > val = MASK_VAL_WRITE(reg, prev_val, val); > @@ -1174,7 +1175,7 @@ static int cpc_write(int cpu, struct cpc_register_resource *reg_res, u64 val) > } > > if (reg->space_id == ACPI_ADR_SPACE_SYSTEM_MEMORY) > - spin_unlock(&cpc_desc->rmw_lock); > + raw_spin_unlock_irqrestore(&cpc_desc->rmw_lock, flags); > > return ret_val; > } > diff --git a/include/acpi/cppc_acpi.h b/include/acpi/cppc_acpi.h > index 76e44e102780..62d368bcd9ec 100644 > --- a/include/acpi/cppc_acpi.h > +++ b/include/acpi/cppc_acpi.h > @@ -65,7 +65,7 @@ struct cpc_desc { > int write_cmd_status; > int write_cmd_id; > /* Lock used for RMW operations in cpc_write() */ > - spinlock_t rmw_lock; > + raw_spinlock_t rmw_lock; > struct cpc_register_resource cpc_regs[MAX_CPC_REG_ENT]; > struct acpi_psd_package domain_info; > struct kobject kobj; > -- Applied (with edited changelog) as 6.12-rc material, thanks!
diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c index c3fc2c05d868..1a40f0514eaa 100644 --- a/drivers/acpi/cppc_acpi.c +++ b/drivers/acpi/cppc_acpi.c @@ -867,7 +867,7 @@ int acpi_cppc_processor_probe(struct acpi_processor *pr) /* Store CPU Logical ID */ cpc_ptr->cpu_id = pr->id; - spin_lock_init(&cpc_ptr->rmw_lock); + raw_spin_lock_init(&cpc_ptr->rmw_lock); /* Parse PSD data for this CPU */ ret = acpi_get_psd(cpc_ptr, handle); @@ -1087,6 +1087,7 @@ static int cpc_write(int cpu, struct cpc_register_resource *reg_res, u64 val) int pcc_ss_id = per_cpu(cpu_pcc_subspace_idx, cpu); struct cpc_reg *reg = ®_res->cpc_entry.reg; struct cpc_desc *cpc_desc; + unsigned long flags; size = GET_BIT_WIDTH(reg); @@ -1126,7 +1127,7 @@ static int cpc_write(int cpu, struct cpc_register_resource *reg_res, u64 val) return -ENODEV; } - spin_lock(&cpc_desc->rmw_lock); + raw_spin_lock_irqsave(&cpc_desc->rmw_lock, flags); switch (size) { case 8: prev_val = readb_relaxed(vaddr); @@ -1141,7 +1142,7 @@ static int cpc_write(int cpu, struct cpc_register_resource *reg_res, u64 val) prev_val = readq_relaxed(vaddr); break; default: - spin_unlock(&cpc_desc->rmw_lock); + raw_spin_unlock_irqrestore(&cpc_desc->rmw_lock, flags); return -EFAULT; } val = MASK_VAL_WRITE(reg, prev_val, val); @@ -1174,7 +1175,7 @@ static int cpc_write(int cpu, struct cpc_register_resource *reg_res, u64 val) } if (reg->space_id == ACPI_ADR_SPACE_SYSTEM_MEMORY) - spin_unlock(&cpc_desc->rmw_lock); + raw_spin_unlock_irqrestore(&cpc_desc->rmw_lock, flags); return ret_val; } diff --git a/include/acpi/cppc_acpi.h b/include/acpi/cppc_acpi.h index 76e44e102780..62d368bcd9ec 100644 --- a/include/acpi/cppc_acpi.h +++ b/include/acpi/cppc_acpi.h @@ -65,7 +65,7 @@ struct cpc_desc { int write_cmd_status; int write_cmd_id; /* Lock used for RMW operations in cpc_write() */ - spinlock_t rmw_lock; + raw_spinlock_t rmw_lock; struct cpc_register_resource cpc_regs[MAX_CPC_REG_ENT]; struct acpi_psd_package domain_info; struct kobject kobj;
The following BUG was triggered. sugov_update_shared() locks a raw_spinlock while cpc_write() locks a spinlock. To have a correct wait-type order, update rmw_lock to a raw_spinlock. Also save irq state. ============================= [ BUG: Invalid wait context ] 6.12.0-rc2-XXX #406 Not tainted ----------------------------- kworker/1:1/62 is trying to lock: ffffff8801593030 (&cpc_ptr->rmw_lock){+.+.}-{3:3}, at: cpc_write+0xcc/0x370 other info that might help us debug this: context-{5:5} 2 locks held by kworker/1:1/62: #0: ffffff897ef5ec98 (&rq->__lock){-.-.}-{2:2}, at: raw_spin_rq_lock_nested+0x2c/0x50 #1: ffffff880154e238 (&sg_policy->update_lock){....}-{2:2}, at: sugov_update_shared+0x3c/0x280 stack backtrace: CPU: 1 UID: 0 PID: 62 Comm: kworker/1:1 Not tainted 6.12.0-rc2-g9654bd3e8806 #406 Workqueue: 0x0 (events) Call trace: dump_backtrace+0xa4/0x130 show_stack+0x20/0x38 dump_stack_lvl+0x90/0xd0 dump_stack+0x18/0x28 __lock_acquire+0x480/0x1ad8 lock_acquire+0x114/0x310 _raw_spin_lock+0x50/0x70 cpc_write+0xcc/0x370 cppc_set_perf+0xa0/0x3a8 cppc_cpufreq_fast_switch+0x40/0xc0 cpufreq_driver_fast_switch+0x4c/0x218 sugov_update_shared+0x234/0x280 update_load_avg+0x6ec/0x7b8 dequeue_entities+0x108/0x830 dequeue_task_fair+0x58/0x408 __schedule+0x4f0/0x1070 schedule+0x54/0x130 worker_thread+0xc0/0x2e8 kthread+0x130/0x148 ret_from_fork+0x10/0x20 Fixes: 60949b7b8054 ("ACPI: CPPC: Fix MASK_VAL() usage") Signed-off-by: Pierre Gondois <pierre.gondois@arm.com> --- Notes: v2: - Do the actual changes. drivers/acpi/cppc_acpi.c | 9 +++++---- include/acpi/cppc_acpi.h | 2 +- 2 files changed, 6 insertions(+), 5 deletions(-)