| Message ID | 20241124070808.194860-4-avri.altman@wdc.com (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | Untie the host lock entanglement - part 2 | expand |
On 11/23/24 11:08 PM, Avri Altman wrote: > Introduce a new clock gating lock to serialize access to some of the > clock gating members instead of the host_lock. > > While at it, simplify the code with the guard() macro and co for > automatic cleanup of the new lock. There are some explicit > spin_lock_irqsave/spin_unlock_irqrestore snaking instances I left behind > because I couldn't make heads or tails of it. > > Additionally, move the trace_ufshcd_clk_gating() call from inside the > region protected by the lock as it doesn't needs protection. Reviewed-by: Bart Van Assche <bvanassche@acm.org>
+ Dmitry On Sun, Nov 24, 2024 at 09:08:07AM +0200, Avri Altman wrote: [...] > @@ -9162,7 +9159,6 @@ static int ufshcd_setup_clocks(struct ufs_hba *hba, bool on) > int ret = 0; > struct ufs_clk_info *clki; > struct list_head *head = &hba->clk_list_head; > - unsigned long flags; > ktime_t start = ktime_get(); > bool clk_state_changed = false; > > @@ -9213,11 +9209,10 @@ static int ufshcd_setup_clocks(struct ufs_hba *hba, bool on) > clk_disable_unprepare(clki->clk); > } > } else if (!ret && on) { > - spin_lock_irqsave(hba->host->host_lock, flags); > - hba->clk_gating.state = CLKS_ON; > + scoped_guard(spinlock_irqsave, &hba->clk_gating.lock) This triggers the following lockdep warning on Qualcomm boards as reported by Dmitry offline: [ 4.388838] INFO: trying to register non-static key. [ 4.395673] The code is fine but needs lockdep annotation, or maybe [ 4.402118] you didn't initialize this object before use? [ 4.407673] turning off the locking correctness validator. [ 4.413334] CPU: 5 UID: 0 PID: 58 Comm: kworker/u32:1 Not tainted 6.12-rc1 #185 [ 4.413343] Hardware name: Qualcomm Technologies, Inc. Robotics RB5 (DT) [ 4.413362] Call trace: [ 4.413364] show_stack+0x18/0x24 (C) [ 4.413374] dump_stack_lvl+0x90/0xd0 [ 4.413384] dump_stack+0x18/0x24 [ 4.413392] register_lock_class+0x498/0x4a8 [ 4.413400] __lock_acquire+0xb4/0x1b90 [ 4.413406] lock_acquire+0x114/0x310 [ 4.413413] _raw_spin_lock_irqsave+0x60/0x88 [ 4.413423] ufshcd_setup_clocks+0x2c0/0x490 [ 4.413433] ufshcd_init+0x198/0x10ec [ 4.413437] ufshcd_pltfrm_init+0x600/0x7c0 [ 4.413444] ufs_qcom_probe+0x20/0x58 [ 4.413449] platform_probe+0x68/0xd8 [ 4.413459] really_probe+0xbc/0x268 [ 4.413466] __driver_probe_device+0x78/0x12c [ 4.413473] driver_probe_device+0x40/0x11c [ 4.413481] __device_attach_driver+0xb8/0xf8 [ 4.413489] bus_for_each_drv+0x84/0xe4 [ 4.413495] __device_attach+0xfc/0x18c [ 4.413502] device_initial_probe+0x14/0x20 [ 4.413510] bus_probe_device+0xb0/0xb4 [ 4.413517] deferred_probe_work_func+0x8c/0xc8 [ 4.413524] process_scheduled_works+0x250/0x658 [ 4.413534] worker_thread+0x15c/0x2c8 [ 4.413542] kthread+0x134/0x200 [ 4.413550] ret_from_fork+0x10/0x20 As lockdep found, clk_gating.lock is uninitialized when ufshcd_setup_clocks() is called for the first time. I looked into fixing it for a moment, but the overall locking for 'clk_gating.state' looks fragile i.e., there are instances where the code is not locked at all. So I'm just reporting to you here hoping that you'd have some idea to fix it. While submitting the fix, please add the following reported by tag: Reported-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> - Mani
On Tue, Jan 21, 2025 at 06:12:23PM +0530, Manivannan Sadhasivam wrote: > + Dmitry > Oops. Missed CCing Dmitry. Added now. - Mani > On Sun, Nov 24, 2024 at 09:08:07AM +0200, Avri Altman wrote: > > [...] > > > @@ -9162,7 +9159,6 @@ static int ufshcd_setup_clocks(struct ufs_hba *hba, bool on) > > int ret = 0; > > struct ufs_clk_info *clki; > > struct list_head *head = &hba->clk_list_head; > > - unsigned long flags; > > ktime_t start = ktime_get(); > > bool clk_state_changed = false; > > > > @@ -9213,11 +9209,10 @@ static int ufshcd_setup_clocks(struct ufs_hba *hba, bool on) > > clk_disable_unprepare(clki->clk); > > } > > } else if (!ret && on) { > > - spin_lock_irqsave(hba->host->host_lock, flags); > > - hba->clk_gating.state = CLKS_ON; > > + scoped_guard(spinlock_irqsave, &hba->clk_gating.lock) > > This triggers the following lockdep warning on Qualcomm boards as reported by > Dmitry offline: > > [ 4.388838] INFO: trying to register non-static key. > [ 4.395673] The code is fine but needs lockdep annotation, or maybe > [ 4.402118] you didn't initialize this object before use? > [ 4.407673] turning off the locking correctness validator. > [ 4.413334] CPU: 5 UID: 0 PID: 58 Comm: kworker/u32:1 Not tainted 6.12-rc1 #185 > [ 4.413343] Hardware name: Qualcomm Technologies, Inc. Robotics RB5 (DT) > [ 4.413362] Call trace: > [ 4.413364] show_stack+0x18/0x24 (C) > [ 4.413374] dump_stack_lvl+0x90/0xd0 > [ 4.413384] dump_stack+0x18/0x24 > [ 4.413392] register_lock_class+0x498/0x4a8 > [ 4.413400] __lock_acquire+0xb4/0x1b90 > [ 4.413406] lock_acquire+0x114/0x310 > [ 4.413413] _raw_spin_lock_irqsave+0x60/0x88 > [ 4.413423] ufshcd_setup_clocks+0x2c0/0x490 > [ 4.413433] ufshcd_init+0x198/0x10ec > [ 4.413437] ufshcd_pltfrm_init+0x600/0x7c0 > [ 4.413444] ufs_qcom_probe+0x20/0x58 > [ 4.413449] platform_probe+0x68/0xd8 > [ 4.413459] really_probe+0xbc/0x268 > [ 4.413466] __driver_probe_device+0x78/0x12c > [ 4.413473] driver_probe_device+0x40/0x11c > [ 4.413481] __device_attach_driver+0xb8/0xf8 > [ 4.413489] bus_for_each_drv+0x84/0xe4 > [ 4.413495] __device_attach+0xfc/0x18c > [ 4.413502] device_initial_probe+0x14/0x20 > [ 4.413510] bus_probe_device+0xb0/0xb4 > [ 4.413517] deferred_probe_work_func+0x8c/0xc8 > [ 4.413524] process_scheduled_works+0x250/0x658 > [ 4.413534] worker_thread+0x15c/0x2c8 > [ 4.413542] kthread+0x134/0x200 > [ 4.413550] ret_from_fork+0x10/0x20 > > As lockdep found, clk_gating.lock is uninitialized when ufshcd_setup_clocks() is > called for the first time. I looked into fixing it for a moment, but the overall > locking for 'clk_gating.state' looks fragile i.e., there are instances where the > code is not locked at all. So I'm just reporting to you here hoping that you'd > have some idea to fix it. > > While submitting the fix, please add the following reported by tag: > > Reported-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > > - Mani > > -- > மணிவண்ணன் சதாசிவம்
> > > @@ -9162,7 +9159,6 @@ static int ufshcd_setup_clocks(struct ufs_hba > *hba, bool on) > > > int ret = 0; > > > struct ufs_clk_info *clki; > > > struct list_head *head = &hba->clk_list_head; > > > - unsigned long flags; > > > ktime_t start = ktime_get(); > > > bool clk_state_changed = false; > > > > > > @@ -9213,11 +9209,10 @@ static int ufshcd_setup_clocks(struct ufs_hba > *hba, bool on) > > > clk_disable_unprepare(clki->clk); > > > } > > > } else if (!ret && on) { > > > - spin_lock_irqsave(hba->host->host_lock, flags); > > > - hba->clk_gating.state = CLKS_ON; > > > + scoped_guard(spinlock_irqsave, &hba->clk_gating.lock) > > > > This triggers the following lockdep warning on Qualcomm boards as > > reported by Dmitry offline: > > > > [ 4.388838] INFO: trying to register non-static key. > > [ 4.395673] The code is fine but needs lockdep annotation, or maybe > > [ 4.402118] you didn't initialize this object before use? > > [ 4.407673] turning off the locking correctness validator. > > [ 4.413334] CPU: 5 UID: 0 PID: 58 Comm: kworker/u32:1 Not tainted 6.12- > rc1 #185 > > [ 4.413343] Hardware name: Qualcomm Technologies, Inc. Robotics RB5 > (DT) > > [ 4.413362] Call trace: > > [ 4.413364] show_stack+0x18/0x24 (C) > > [ 4.413374] dump_stack_lvl+0x90/0xd0 > > [ 4.413384] dump_stack+0x18/0x24 > > [ 4.413392] register_lock_class+0x498/0x4a8 > > [ 4.413400] __lock_acquire+0xb4/0x1b90 > > [ 4.413406] lock_acquire+0x114/0x310 > > [ 4.413413] _raw_spin_lock_irqsave+0x60/0x88 > > [ 4.413423] ufshcd_setup_clocks+0x2c0/0x490 > > [ 4.413433] ufshcd_init+0x198/0x10ec > > [ 4.413437] ufshcd_pltfrm_init+0x600/0x7c0 > > [ 4.413444] ufs_qcom_probe+0x20/0x58 > > [ 4.413449] platform_probe+0x68/0xd8 > > [ 4.413459] really_probe+0xbc/0x268 > > [ 4.413466] __driver_probe_device+0x78/0x12c > > [ 4.413473] driver_probe_device+0x40/0x11c > > [ 4.413481] __device_attach_driver+0xb8/0xf8 > > [ 4.413489] bus_for_each_drv+0x84/0xe4 > > [ 4.413495] __device_attach+0xfc/0x18c > > [ 4.413502] device_initial_probe+0x14/0x20 > > [ 4.413510] bus_probe_device+0xb0/0xb4 > > [ 4.413517] deferred_probe_work_func+0x8c/0xc8 > > [ 4.413524] process_scheduled_works+0x250/0x658 > > [ 4.413534] worker_thread+0x15c/0x2c8 > > [ 4.413542] kthread+0x134/0x200 > > [ 4.413550] ret_from_fork+0x10/0x20 > > > > As lockdep found, clk_gating.lock is uninitialized when > > ufshcd_setup_clocks() is called for the first time. I looked into > > fixing it for a moment, but the overall locking for 'clk_gating.state' > > looks fragile i.e., there are instances where the code is not locked > > at all. So I'm just reporting to you here hoping that you'd have some idea > to fix it. Thanks for reporting this. How about just: +++ b/drivers/ufs/core/ufshcd.c @@ -9166,7 +9166,7 @@ static int ufshcd_setup_clocks(struct ufs_hba *hba, bool on) if (!IS_ERR_OR_NULL(clki->clk) && clki->enabled) clk_disable_unprepare(clki->clk); } - } else if (!ret && on) { + } else if (!ret && on && hba->clk_gating.is_initialized) { scoped_guard(spinlock_irqsave, &hba->clk_gating.lock) hba->clk_gating.state = CLKS_ON; trace_ufshcd_clk_gating(dev_name(hba->dev), What do you think? Thanks, Avri > > > > While submitting the fix, please add the following reported by tag: > > > > Reported-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > > > > - Mani > > > > -- > > மணிவண்ணன் சதாசிவம் > > -- > மணிவண்ணன் சதாசிவம்
diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c index 838bbab97438..b4c8f528c18f 100644 --- a/drivers/ufs/core/ufshcd.c +++ b/drivers/ufs/core/ufshcd.c @@ -1816,19 +1816,16 @@ static void ufshcd_exit_clk_scaling(struct ufs_hba *hba) static void ufshcd_ungate_work(struct work_struct *work) { int ret; - unsigned long flags; struct ufs_hba *hba = container_of(work, struct ufs_hba, clk_gating.ungate_work); cancel_delayed_work_sync(&hba->clk_gating.gate_work); - spin_lock_irqsave(hba->host->host_lock, flags); - if (hba->clk_gating.state == CLKS_ON) { - spin_unlock_irqrestore(hba->host->host_lock, flags); - return; + scoped_guard(spinlock_irqsave, &hba->clk_gating.lock) { + if (hba->clk_gating.state == CLKS_ON) + return; } - spin_unlock_irqrestore(hba->host->host_lock, flags); ufshcd_hba_vreg_set_hpm(hba); ufshcd_setup_clocks(hba, true); @@ -1863,7 +1860,7 @@ void ufshcd_hold(struct ufs_hba *hba) if (!ufshcd_is_clkgating_allowed(hba) || !hba->clk_gating.is_initialized) return; - spin_lock_irqsave(hba->host->host_lock, flags); + spin_lock_irqsave(&hba->clk_gating.lock, flags); hba->clk_gating.active_reqs++; start: @@ -1879,11 +1876,11 @@ void ufshcd_hold(struct ufs_hba *hba) */ if (ufshcd_can_hibern8_during_gating(hba) && ufshcd_is_link_hibern8(hba)) { - spin_unlock_irqrestore(hba->host->host_lock, flags); + spin_unlock_irqrestore(&hba->clk_gating.lock, flags); flush_result = flush_work(&hba->clk_gating.ungate_work); if (hba->clk_gating.is_suspended && !flush_result) return; - spin_lock_irqsave(hba->host->host_lock, flags); + spin_lock_irqsave(&hba->clk_gating.lock, flags); goto start; } break; @@ -1912,17 +1909,17 @@ void ufshcd_hold(struct ufs_hba *hba) */ fallthrough; case REQ_CLKS_ON: - spin_unlock_irqrestore(hba->host->host_lock, flags); + spin_unlock_irqrestore(&hba->clk_gating.lock, flags); flush_work(&hba->clk_gating.ungate_work); /* Make sure state is CLKS_ON before returning */ - spin_lock_irqsave(hba->host->host_lock, flags); + spin_lock_irqsave(&hba->clk_gating.lock, flags); goto start; default: dev_err(hba->dev, "%s: clk gating is in invalid state %d\n", __func__, hba->clk_gating.state); break; } - spin_unlock_irqrestore(hba->host->host_lock, flags); + spin_unlock_irqrestore(&hba->clk_gating.lock, flags); } EXPORT_SYMBOL_GPL(ufshcd_hold); @@ -1930,30 +1927,32 @@ static void ufshcd_gate_work(struct work_struct *work) { struct ufs_hba *hba = container_of(work, struct ufs_hba, clk_gating.gate_work.work); - unsigned long flags; int ret; - spin_lock_irqsave(hba->host->host_lock, flags); - /* - * In case you are here to cancel this work the gating state - * would be marked as REQ_CLKS_ON. In this case save time by - * skipping the gating work and exit after changing the clock - * state to CLKS_ON. - */ - if (hba->clk_gating.is_suspended || - (hba->clk_gating.state != REQ_CLKS_OFF)) { - hba->clk_gating.state = CLKS_ON; - trace_ufshcd_clk_gating(dev_name(hba->dev), - hba->clk_gating.state); - goto rel_lock; - } + scoped_guard(spinlock_irqsave, &hba->clk_gating.lock) { + /* + * In case you are here to cancel this work the gating state + * would be marked as REQ_CLKS_ON. In this case save time by + * skipping the gating work and exit after changing the clock + * state to CLKS_ON. + */ + if (hba->clk_gating.is_suspended || + hba->clk_gating.state != REQ_CLKS_OFF) { + hba->clk_gating.state = CLKS_ON; + trace_ufshcd_clk_gating(dev_name(hba->dev), + hba->clk_gating.state); + return; + } - if (ufshcd_is_ufs_dev_busy(hba) || - hba->ufshcd_state != UFSHCD_STATE_OPERATIONAL || - hba->clk_gating.active_reqs) - goto rel_lock; + if (hba->clk_gating.active_reqs) + return; + } - spin_unlock_irqrestore(hba->host->host_lock, flags); + scoped_guard(spinlock_irqsave, hba->host->host_lock) { + if (ufshcd_is_ufs_dev_busy(hba) || + hba->ufshcd_state != UFSHCD_STATE_OPERATIONAL) + return; + } /* put the link into hibern8 mode before turning off clocks */ if (ufshcd_can_hibern8_during_gating(hba)) { @@ -1964,7 +1963,7 @@ static void ufshcd_gate_work(struct work_struct *work) __func__, ret); trace_ufshcd_clk_gating(dev_name(hba->dev), hba->clk_gating.state); - goto out; + return; } ufshcd_set_link_hibern8(hba); } @@ -1984,32 +1983,34 @@ static void ufshcd_gate_work(struct work_struct *work) * prevent from doing cancel work multiple times when there are * new requests arriving before the current cancel work is done. */ - spin_lock_irqsave(hba->host->host_lock, flags); + guard(spinlock_irqsave)(&hba->clk_gating.lock); if (hba->clk_gating.state == REQ_CLKS_OFF) { hba->clk_gating.state = CLKS_OFF; trace_ufshcd_clk_gating(dev_name(hba->dev), hba->clk_gating.state); } -rel_lock: - spin_unlock_irqrestore(hba->host->host_lock, flags); -out: - return; } -/* host lock must be held before calling this variant */ static void __ufshcd_release(struct ufs_hba *hba) { + lockdep_assert_held(&hba->clk_gating.lock); + if (!ufshcd_is_clkgating_allowed(hba)) return; hba->clk_gating.active_reqs--; if (hba->clk_gating.active_reqs || hba->clk_gating.is_suspended || - hba->ufshcd_state != UFSHCD_STATE_OPERATIONAL || - ufshcd_has_pending_tasks(hba) || !hba->clk_gating.is_initialized || + !hba->clk_gating.is_initialized || hba->clk_gating.state == CLKS_OFF) return; + scoped_guard(spinlock_irqsave, hba->host->host_lock) { + if (ufshcd_has_pending_tasks(hba) || + hba->ufshcd_state != UFSHCD_STATE_OPERATIONAL) + return; + } + hba->clk_gating.state = REQ_CLKS_OFF; trace_ufshcd_clk_gating(dev_name(hba->dev), hba->clk_gating.state); queue_delayed_work(hba->clk_gating.clk_gating_workq, @@ -2019,11 +2020,8 @@ static void __ufshcd_release(struct ufs_hba *hba) void ufshcd_release(struct ufs_hba *hba) { - unsigned long flags; - - spin_lock_irqsave(hba->host->host_lock, flags); + guard(spinlock_irqsave)(&hba->clk_gating.lock); __ufshcd_release(hba); - spin_unlock_irqrestore(hba->host->host_lock, flags); } EXPORT_SYMBOL_GPL(ufshcd_release); @@ -2038,11 +2036,9 @@ static ssize_t ufshcd_clkgate_delay_show(struct device *dev, void ufshcd_clkgate_delay_set(struct device *dev, unsigned long value) { struct ufs_hba *hba = dev_get_drvdata(dev); - unsigned long flags; - spin_lock_irqsave(hba->host->host_lock, flags); + guard(spinlock_irqsave)(&hba->clk_gating.lock); hba->clk_gating.delay_ms = value; - spin_unlock_irqrestore(hba->host->host_lock, flags); } EXPORT_SYMBOL_GPL(ufshcd_clkgate_delay_set); @@ -2070,7 +2066,6 @@ static ssize_t ufshcd_clkgate_enable_store(struct device *dev, struct device_attribute *attr, const char *buf, size_t count) { struct ufs_hba *hba = dev_get_drvdata(dev); - unsigned long flags; u32 value; if (kstrtou32(buf, 0, &value)) @@ -2078,9 +2073,10 @@ static ssize_t ufshcd_clkgate_enable_store(struct device *dev, value = !!value; - spin_lock_irqsave(hba->host->host_lock, flags); + guard(spinlock_irqsave)(&hba->clk_gating.lock); + if (value == hba->clk_gating.is_enabled) - goto out; + return count; if (value) __ufshcd_release(hba); @@ -2088,8 +2084,7 @@ static ssize_t ufshcd_clkgate_enable_store(struct device *dev, hba->clk_gating.active_reqs++; hba->clk_gating.is_enabled = value; -out: - spin_unlock_irqrestore(hba->host->host_lock, flags); + return count; } @@ -2131,6 +2126,8 @@ static void ufshcd_init_clk_gating(struct ufs_hba *hba) INIT_DELAYED_WORK(&hba->clk_gating.gate_work, ufshcd_gate_work); INIT_WORK(&hba->clk_gating.ungate_work, ufshcd_ungate_work); + spin_lock_init(&hba->clk_gating.lock); + hba->clk_gating.clk_gating_workq = alloc_ordered_workqueue( "ufs_clk_gating_%d", WQ_MEM_RECLAIM | WQ_HIGHPRI, hba->host->host_no); @@ -9162,7 +9159,6 @@ static int ufshcd_setup_clocks(struct ufs_hba *hba, bool on) int ret = 0; struct ufs_clk_info *clki; struct list_head *head = &hba->clk_list_head; - unsigned long flags; ktime_t start = ktime_get(); bool clk_state_changed = false; @@ -9213,11 +9209,10 @@ static int ufshcd_setup_clocks(struct ufs_hba *hba, bool on) clk_disable_unprepare(clki->clk); } } else if (!ret && on) { - spin_lock_irqsave(hba->host->host_lock, flags); - hba->clk_gating.state = CLKS_ON; + scoped_guard(spinlock_irqsave, &hba->clk_gating.lock) + hba->clk_gating.state = CLKS_ON; trace_ufshcd_clk_gating(dev_name(hba->dev), hba->clk_gating.state); - spin_unlock_irqrestore(hba->host->host_lock, flags); } if (clk_state_changed) diff --git a/include/ufs/ufshcd.h b/include/ufs/ufshcd.h index d7aca9e61684..b6311069d901 100644 --- a/include/ufs/ufshcd.h +++ b/include/ufs/ufshcd.h @@ -403,6 +403,9 @@ enum clk_gating_state { * delay_ms * @ungate_work: worker to turn on clocks that will be used in case of * interrupt context + * @clk_gating_workq: workqueue for clock gating work. + * @lock: serialize access to some struct ufs_clk_gating members. An outer lock + * relative to the host lock * @state: the current clocks state * @delay_ms: gating delay in ms * @is_suspended: clk gating is suspended when set to 1 which can be used @@ -413,11 +416,14 @@ enum clk_gating_state { * @is_initialized: Indicates whether clock gating is initialized or not * @active_reqs: number of requests that are pending and should be waited for * completion before gating clocks. - * @clk_gating_workq: workqueue for clock gating work. */ struct ufs_clk_gating { struct delayed_work gate_work; struct work_struct ungate_work; + struct workqueue_struct *clk_gating_workq; + + spinlock_t lock; + enum clk_gating_state state; unsigned long delay_ms; bool is_suspended; @@ -426,7 +432,6 @@ struct ufs_clk_gating { bool is_enabled; bool is_initialized; int active_reqs; - struct workqueue_struct *clk_gating_workq; }; /**
Introduce a new clock gating lock to serialize access to some of the clock gating members instead of the host_lock. While at it, simplify the code with the guard() macro and co for automatic cleanup of the new lock. There are some explicit spin_lock_irqsave/spin_unlock_irqrestore snaking instances I left behind because I couldn't make heads or tails of it. Additionally, move the trace_ufshcd_clk_gating() call from inside the region protected by the lock as it doesn't needs protection. Signed-off-by: Avri Altman <avri.altman@wdc.com> --- drivers/ufs/core/ufshcd.c | 109 ++++++++++++++++++-------------------- include/ufs/ufshcd.h | 9 +++- 2 files changed, 59 insertions(+), 59 deletions(-)