| Message ID | 20230202182116.38334-1-athierry@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | scsi: ufs: probe hba and add lus synchronously | expand |
On 2/2/23 10:21, Adrien Thierry wrote: > This patch fixes both the warning and the deadlock, by probing hba and > lus synchronously during ufs initialization. I'm concerned that this change will slow down booting of Android devices. Please find another solution, e.g. building devfreq and the ondemand governor into the kernel instead of as loadable kernel modules. Thanks, Bart.
Hi Bart, > Please find another solution, e.g. building devfreq and the ondemand > governor into the kernel instead of as loadable kernel modules. Another solution could be to change the kernel Kconfigs to force DEVFREQ_GOV_SIMPLE_ONDEMAND (and possibly other devfreq-related options as well) to be builtin when SCSI_UFSHCD is enabled (builtin or module). Is that what you meant? Best, Adrien
On 2/3/23 12:40, Adrien Thierry wrote: > Another solution could be to change the kernel Kconfigs to force > DEVFREQ_GOV_SIMPLE_ONDEMAND (and possibly other devfreq-related options as > well) to be builtin when SCSI_UFSHCD is enabled (builtin or module). Is > that what you meant? Are all UFS users using the devfreq framework? Otherwise this sounds good to me. Thanks, Bart.
> Are all UFS users using the devfreq framework? Otherwise this sounds good to me.
The warning originates from the UFS core :
ufshcd_init() -> ufshcd_async_scan() -> ufshcd_add_lus() ->
ufshcd_devfreq_init()
ufshcd_devfreq_init() is called as long as
ufshcd_is_clkscaling_supported(hba) returns true, i.e.
UFSHCD_CAP_CLK_SCALING is set. This is not the case for all UFS users, but
it could potentially be if they start supporting clock scaling. Moreover,
in the current Kconfigs, DEVFREQ_GOV_SIMPLE_ONDEMAND is already selected
when SCSI_UFSHCD is enabled. We just need to force it to be builtin.
Best,
Adrien
Hi Bart, > Another solution could be to change the kernel Kconfigs to force > DEVFREQ_GOV_SIMPLE_ONDEMAND (and possibly other devfreq-related options as > well) to be builtin when SCSI_UFSHCD is enabled (builtin or module). Is > that what you meant? After diving more into this, I could not find a way in the Kconfig language to force a module to be builtin, so I'm not sure if the idea is implementable. I had another idea. Instead of running the whole ufshcd_async_scan() function synchronously (and potentially slow down boot time on Android devices as you mentioned), we could only move devfreq initialization out of the async routine, ie. this chunk of code: drivers/ufs/core/ufshcd.c:8140 if (ufshcd_is_clkscaling_supported(hba)) { memcpy(&hba->clk_scaling.saved_pwr_info.info, &hba->pwr_info, sizeof(struct ufs_pa_layer_attr)); hba->clk_scaling.saved_pwr_info.is_valid = true; hba->clk_scaling.is_allowed = true; ret = ufshcd_devfreq_init(hba); if (ret) goto out; hba->clk_scaling.is_enabled = true; ufshcd_init_clk_scaling_sysfs(hba); } I tested this idea on the Qualcomm sa8540p-ride, and UFS clock scaling seems to be working without issue. No error on boot and the /sys/kernel/tracing/events/ufs/ufshcd_clk_scaling traces are similar with and without the change when running fio to put UFS under load. Do you think this could be an acceptable compromise for boot time? It should not slow things down too much since the really time-consuming parts (ie. UFS initialization) would still be in the async routine. Best, Adrien
On 2/7/23 14:54, Adrien Thierry wrote: > Do you think this could be an acceptable compromise for boot time? It > should not slow things down too much since the really time-consuming parts > (ie. UFS initialization) would still be in the async routine. Hi Adrien, That sounds good to me. Thank you for having suggested an alternative solution. Bart.
Hi Bart, > Hi Adrien, > > That sounds good to me. Thank you for having suggested an alternative > solution. > > Bart. I posted a new patch [1]. Can you please give your feedback when you have the chance to review? Thank you, Adrien [1] https://lore.kernel.org/all/20230209211456.54250-1-athierry@redhat.com/
diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c index 3a1c4d31e010..2cf8e09b1a9d 100644 --- a/drivers/ufs/core/ufshcd.c +++ b/drivers/ufs/core/ufshcd.c @@ -245,7 +245,6 @@ static const struct ufs_dev_quirk ufs_fixups[] = { }; static irqreturn_t ufshcd_tmc_handler(struct ufs_hba *hba); -static void ufshcd_async_scan(void *data, async_cookie_t cookie); static int ufshcd_reset_and_restore(struct ufs_hba *hba); static int ufshcd_eh_host_reset_handler(struct scsi_cmnd *cmd); static int ufshcd_clear_tm_cmd(struct ufs_hba *hba, int tag); @@ -8263,36 +8262,6 @@ static int ufshcd_probe_hba(struct ufs_hba *hba, bool init_dev_params) return ret; } -/** - * ufshcd_async_scan - asynchronous execution for probing hba - * @data: data pointer to pass to this function - * @cookie: cookie data - */ -static void ufshcd_async_scan(void *data, async_cookie_t cookie) -{ - struct ufs_hba *hba = (struct ufs_hba *)data; - int ret; - - down(&hba->host_sem); - /* Initialize hba, detect and initialize UFS device */ - ret = ufshcd_probe_hba(hba, true); - up(&hba->host_sem); - if (ret) - goto out; - - /* Probe and add UFS logical units */ - ret = ufshcd_add_lus(hba); -out: - /* - * If we failed to initialize the device or the device is not - * present, turn off the power/clocks etc. - */ - if (ret) { - pm_runtime_put_sync(hba->dev); - ufshcd_hba_exit(hba); - } -} - static enum scsi_timeout_action ufshcd_eh_timed_out(struct scsi_cmnd *scmd) { struct ufs_hba *hba = shost_priv(scmd->device->host); @@ -9896,12 +9865,23 @@ int ufshcd_init(struct ufs_hba *hba, void __iomem *mmio_base, unsigned int irq) */ ufshcd_set_ufs_dev_active(hba); - async_schedule(ufshcd_async_scan, hba); + /* Initialize hba, detect and initialize UFS device */ + err = ufshcd_probe_hba(hba, true); + if (err) + goto out_power_off; + + /* Probe and add UFS logical units */ + err = ufshcd_add_lus(hba); + if (err) + goto out_power_off; + ufs_sysfs_add_nodes(hba->dev); device_enable_async_suspend(dev); return 0; +out_power_off: + pm_runtime_put_sync(hba->dev); free_tmf_queue: blk_mq_destroy_queue(hba->tmf_queue); blk_put_queue(hba->tmf_queue);
During ufs initialization, hba and lus probing are asynchronous. ufshcd_async_scan() calls ufshcd_add_lus(), which in turn initializes devfreq for ufs. The simple ondemand governor is then loaded. If it is build as a module, request_module() is called and throws a warning: WARNING: CPU: 7 PID: 167 at kernel/kmod.c:136 __request_module+0x1e0/0x460 Modules linked in: crct10dif_ce llcc_qcom phy_qcom_qmp_usb ufs_qcom phy_qcom_snps_femto_v2 ufshcd_pltfrm phy_qcom_qmp_combo ufshcd_core phy_qcom_qmp_ufs qcom_wdt socinfo fuse ipv6 CPU: 7 PID: 167 Comm: kworker/u16:3 Not tainted 6.2.0-rc6-00009-g58706f7fb045 #1 Hardware name: Qualcomm SA8540P Ride (DT) Workqueue: events_unbound async_run_entry_fn pstate: 00400005 (nzcv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--) pc : __request_module+0x1e0/0x460 lr : __request_module+0x1d8/0x460 sp : ffff800009323b90 x29: ffff800009323b90 x28: 0000000000000000 x27: 0000000000000000 x26: ffff800009323d50 x25: ffff7b9045f57810 x24: ffff7b9045f57830 x23: ffffdc5a83e426e8 x22: ffffdc5ae80a9818 x21: 0000000000000001 x20: ffffdc5ae7502f98 x19: ffff7b9045f57800 x18: ffffffffffffffff x17: 312f716572667665 x16: 642f7366752e3030 x15: 0000000000000000 x14: 000000000000021c x13: 0000000000005400 x12: ffff7b9042ed7614 x11: ffff7b9042ed7600 x10: 00000000636c0890 x9 : 0000000000000038 x8 : ffff7b9045f2c880 x7 : ffff7b9045f57c68 x6 : 0000000000000080 x5 : 0000000000000000 x4 : 8000000000000000 x3 : 0000000000000000 x2 : 0000000000000000 x1 : ffffdc5ae5d382f0 x0 : 0000000000000001 Call trace: __request_module+0x1e0/0x460 try_then_request_governor+0x7c/0x100 devfreq_add_device+0x4b0/0x5fc ufshcd_async_scan+0x1d4/0x310 [ufshcd_core] async_run_entry_fn+0x34/0xe0 process_one_work+0x1d0/0x320 worker_thread+0x14c/0x444 kthread+0x10c/0x110 ret_from_fork+0x10/0x20 This occurs because synchronous module loading from async is not allowed. According to __request_module(): /* * We don't allow synchronous module loading from async. Module * init may invoke async_synchronize_full() which will end up * waiting for this task which already is waiting for the module * loading to complete, leading to a deadlock. */ I experienced such a deadlock on the Qualcomm QDrive3/sa8540p-ride. With DEVFREQ_GOV_SIMPLE_ONDEMAND=m, the boot hangs after the warning. This patch fixes both the warning and the deadlock, by probing hba and lus synchronously during ufs initialization. Signed-off-by: Adrien Thierry <athierry@redhat.com> --- drivers/ufs/core/ufshcd.c | 44 +++++++++++---------------------------- 1 file changed, 12 insertions(+), 32 deletions(-)