The Linux kernel's log buffer provides many levels of verbosity,
associated with different semantic meanings. Care should be taken to
only log useful information to the info level, and log errors to the
error level.
The MediaTek UFS driver does not do this. It freely logs verbose debug
information to the info level, errors to the info level, and sometimes
errors to the warning level.
Adjust all the wrapped kprintf invocations to rectify this situation.
Use user-friendly %pe format codes for printing errors where possible.
Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
---
drivers/ufs/host/ufs-mediatek.c | 99 ++++++++++++++++++-----------------------
1 file changed, 43 insertions(+), 56 deletions(-)
diff --git a/drivers/ufs/host/ufs-mediatek.c b/drivers/ufs/host/ufs-mediatek.c
index ecf16e82a326..2b1f26b55782 100644
--- a/drivers/ufs/host/ufs-mediatek.c
+++ b/drivers/ufs/host/ufs-mediatek.c
@@ -192,8 +192,8 @@ static void ufs_mtk_crypto_enable(struct ufs_hba *hba)
ufs_mtk_crypto_ctrl(res, 1);
if (res.a0) {
- dev_info(hba->dev, "%s: crypto enable failed, err: %lu\n",
- __func__, res.a0);
+ dev_err(hba->dev, "%s: crypto enable failed with error %lu, disabling\n",
+ __func__, res.a0);
hba->caps &= ~UFSHCD_CAP_CRYPTO;
}
}
@@ -542,40 +542,38 @@ static void ufs_mtk_boost_crypt(struct ufs_hba *hba, bool boost)
ret = clk_prepare_enable(cfg->clk_crypt_mux);
if (ret) {
- dev_info(hba->dev, "clk_prepare_enable(): %d\n",
- ret);
+ dev_err(hba->dev, "%s: Failed to enable clk_crypt_mux: %pe\n",
+ __func__, ERR_PTR(ret));
return;
}
if (boost) {
ret = regulator_set_voltage(reg, volt, INT_MAX);
if (ret) {
- dev_info(hba->dev,
- "failed to set vcore to %d\n", volt);
+ dev_err(hba->dev, "%s: Failed to set vcore to %d: %pe\n",
+ __func__, volt, ERR_PTR(ret));
goto out;
}
- ret = clk_set_parent(cfg->clk_crypt_mux,
- cfg->clk_crypt_perf);
+ ret = clk_set_parent(cfg->clk_crypt_mux, cfg->clk_crypt_perf);
if (ret) {
- dev_info(hba->dev,
- "failed to set clk_crypt_perf\n");
+ dev_err(hba->dev, "%s: Failed to reparent clk_crypt_perf: %pe\n",
+ __func__, ERR_PTR(ret));
regulator_set_voltage(reg, 0, INT_MAX);
goto out;
}
} else {
- ret = clk_set_parent(cfg->clk_crypt_mux,
- cfg->clk_crypt_lp);
+ ret = clk_set_parent(cfg->clk_crypt_mux, cfg->clk_crypt_lp);
if (ret) {
- dev_info(hba->dev,
- "failed to set clk_crypt_lp\n");
+ dev_err(hba->dev, "%s: Failed to reparent clk_crypt_lp: %pe\n",
+ __func__, ERR_PTR(ret));
goto out;
}
ret = regulator_set_voltage(reg, 0, INT_MAX);
if (ret) {
- dev_info(hba->dev,
- "failed to set vcore to MIN\n");
+ dev_err(hba->dev, "%s: Failed to set vcore to minimum: %pe\n",
+ __func__, ERR_PTR(ret));
}
}
out:
@@ -763,10 +761,8 @@ static int ufs_mtk_setup_clocks(struct ufs_hba *hba, bool on,
if (clk_pwr_off) {
ufs_mtk_pwr_ctrl(hba, false);
} else {
- dev_warn(hba->dev, "Clock is not turned off, hba->ahit = 0x%x, AHIT = 0x%x\n",
- hba->ahit,
- ufshcd_readl(hba,
- REG_AUTO_HIBERNATE_IDLE_TIMER));
+ dev_warn(hba->dev, "Clock isn't off, hba->ahit = 0x%x, AHIT = 0x%x\n",
+ hba->ahit, ufshcd_readl(hba, REG_AUTO_HIBERNATE_IDLE_TIMER));
}
ufs_mtk_mcq_disable_irq(hba);
} else if (on && status == POST_CHANGE) {
@@ -810,11 +806,11 @@ static void ufs_mtk_mcq_set_irq_affinity(struct ufs_hba *hba, unsigned int cpu)
_cpu = (cpu == 0) ? 3 : cpu;
ret = irq_set_affinity(irq, cpumask_of(_cpu));
if (ret) {
- dev_err(hba->dev, "set irq %d affinity to CPU %d failed\n",
+ dev_err(hba->dev, "setting irq %d affinity to CPU %d failed\n",
irq, _cpu);
return;
}
- dev_info(hba->dev, "set irq %d affinity to CPU: %d\n", irq, _cpu);
+ dev_dbg(hba->dev, "set irq %d affinity to CPU %d\n", irq, _cpu);
}
static bool ufs_mtk_is_legacy_chipset(struct ufs_hba *hba, u32 hw_ip_ver)
@@ -830,7 +826,8 @@ static bool ufs_mtk_is_legacy_chipset(struct ufs_hba *hba, u32 hw_ip_ver)
default:
break;
}
- dev_info(hba->dev, "legacy IP version - 0x%x, is legacy : %d", hw_ip_ver, is_legacy);
+ dev_dbg(hba->dev, "IP version 0x%x, legacy = %s", hw_ip_ver,
+ str_true_false(is_legacy));
return is_legacy;
}
@@ -935,15 +932,12 @@ static void ufs_mtk_init_clocks(struct ufs_hba *hba)
}
}
- list_for_each_entry(clki, head, list) {
- dev_info(hba->dev, "clk \"%s\" present", clki->name);
- }
+ list_for_each_entry(clki, head, list)
+ dev_dbg(hba->dev, "clk \"%s\" present", clki->name);
if (!ufs_mtk_is_clk_scale_ready(hba)) {
hba->caps &= ~UFSHCD_CAP_CLK_SCALING;
- dev_info(hba->dev,
- "%s: Clk-scaling not ready. Feature disabled.",
- __func__);
+ dev_info(hba->dev, "%s: Clock scaling unavailable", __func__);
return;
}
@@ -953,8 +947,8 @@ static void ufs_mtk_init_clocks(struct ufs_hba *hba)
*/
reg = devm_regulator_get_optional(dev, "dvfsrc-vcore");
if (IS_ERR(reg)) {
- dev_info(dev, "failed to get dvfsrc-vcore: %ld",
- PTR_ERR(reg));
+ if (PTR_ERR(reg) != -ENODEV)
+ dev_err(dev, "Failed to get dvfsrc-vcore: %pe\n", reg);
return;
}
@@ -968,12 +962,9 @@ static void ufs_mtk_init_clocks(struct ufs_hba *hba)
host->mclk.vcore_volt = volt;
/* If default boot is max gear, request vcore */
- if (reg && volt && host->clk_scale_up) {
- if (regulator_set_voltage(reg, volt, INT_MAX)) {
- dev_info(hba->dev,
- "Failed to set vcore to %d\n", volt);
- }
- }
+ if (reg && volt && host->clk_scale_up)
+ if (regulator_set_voltage(reg, volt, INT_MAX))
+ dev_err(hba->dev, "Failed to set vcore to %d\n", volt);
}
static void ufs_mtk_setup_clk_gating(struct ufs_hba *hba)
@@ -1060,7 +1051,7 @@ static void ufs_mtk_init_mcq_irq(struct ufs_hba *hba)
}
host->mcq_intr_info[i].hba = hba;
host->mcq_intr_info[i].irq = irq;
- dev_info(hba->dev, "get platform mcq irq: %d, %d\n", i, irq);
+ dev_dbg(hba->dev, "get platform mcq irq: %d, %d\n", i, irq);
}
return;
@@ -1307,10 +1298,8 @@ static int ufs_mtk_pre_pwr_change(struct ufs_hba *hba,
host_params.desired_working_mode = UFS_PWM_MODE;
ret = ufshcd_negotiate_pwr_params(&host_params, dev_max_params, dev_req_params);
- if (ret) {
- pr_info("%s: failed to determine capabilities\n",
- __func__);
- }
+ if (ret)
+ dev_warn(hba->dev, "%s: failed to determine capabilities\n", __func__);
if (ufs_mtk_pmc_via_fastauto(hba, dev_req_params)) {
ufs_mtk_adjust_sync_length(hba);
@@ -1356,10 +1345,9 @@ static int ufs_mtk_pre_pwr_change(struct ufs_hba *hba,
ret = ufshcd_uic_change_pwr_mode(hba,
FASTAUTO_MODE << 4 | FASTAUTO_MODE);
- if (ret) {
- dev_err(hba->dev, "%s: HSG1B FASTAUTO failed ret=%d\n",
- __func__, ret);
- }
+ if (ret)
+ dev_err(hba->dev, "%s: HSG1B FASTAUTO failed: %pe\n",
+ __func__, ERR_PTR(ret));
}
/* if already configured to the requested pwr_mode, skip adapt */
@@ -1409,7 +1397,7 @@ static int ufs_mtk_auto_hibern8_disable(struct ufs_hba *hba)
out:
if (ret) {
- dev_warn(hba->dev, "exit h8 state fail, ret=%d\n", ret);
+ dev_err(hba->dev, "Failed to exit h8 state: %pe\n", ERR_PTR(ret));
ufshcd_force_error_recovery(hba);
@@ -1571,7 +1559,7 @@ static int ufs_mtk_device_reset(struct ufs_hba *hba)
/* Some devices may need time to respond to rst_n */
usleep_range(10000, 15000);
- dev_info(hba->dev, "device reset done\n");
+ dev_dbg(hba->dev, "device reset done\n");
return 0;
}
@@ -1607,12 +1595,12 @@ static int ufs_mtk_link_set_hpm(struct ufs_hba *hba)
/* Check link state to make sure exit h8 success */
err = ufs_mtk_wait_idle_state(hba, 5);
if (err) {
- dev_warn(hba->dev, "wait idle fail, err=%d\n", err);
+ dev_err(hba->dev, "Failed to wait for idle: %pe\n", ERR_PTR(err));
return err;
}
err = ufs_mtk_wait_link_state(hba, VS_LINK_UP, 100);
if (err) {
- dev_warn(hba->dev, "exit h8 state fail, err=%d\n", err);
+ dev_err(hba->dev, "Failed to wait for link to be up: %pe\n", ERR_PTR(err));
return err;
}
ufshcd_set_link_active(hba);
@@ -1905,20 +1893,19 @@ static void ufs_mtk_event_notify(struct ufs_hba *hba,
/* Print details of UIC Errors */
if (evt <= UFS_EVT_DME_ERR) {
- dev_info(hba->dev,
- "Host UIC Error Code (%s): %08x\n",
- ufs_uic_err_str[evt], val);
+ dev_err(hba->dev, "Host UIC Error Code (%s): %08x\n",
+ ufs_uic_err_str[evt], val);
reg = val;
}
if (evt == UFS_EVT_PA_ERR) {
for_each_set_bit(bit, ®, ARRAY_SIZE(ufs_uic_pa_err_str))
- dev_info(hba->dev, "%s\n", ufs_uic_pa_err_str[bit]);
+ dev_err(hba->dev, "%s\n", ufs_uic_pa_err_str[bit]);
}
if (evt == UFS_EVT_DL_ERR) {
for_each_set_bit(bit, ®, ARRAY_SIZE(ufs_uic_dl_err_str))
- dev_info(hba->dev, "%s\n", ufs_uic_dl_err_str[bit]);
+ dev_err(hba->dev, "%s\n", ufs_uic_dl_err_str[bit]);
}
}
@@ -2123,7 +2110,7 @@ static int ufs_mtk_mcq_config_resource(struct ufs_hba *hba)
/* fail mcq initialization if interrupt is not filled properly */
if (!host->mcq_nr_intr) {
- dev_info(hba->dev, "IRQs not ready. MCQ disabled.");
+ dev_err(hba->dev, "IRQs not ready. MCQ disabled.");
return -EINVAL;
}
--
2.53.0On Mon, 2026-02-16 at 14:37 +0100, Nicolas Frattaroli wrote:
> drivers/ufs/host/ufs-mediatek.c | 99 ++++++++++++++++++-------------
> ----------
> 1 file changed, 43 insertions(+), 56 deletions(-)
>
> diff --git a/drivers/ufs/host/ufs-mediatek.c b/drivers/ufs/host/ufs-
> mediatek.c
> index ecf16e82a326..2b1f26b55782 100644
> --- a/drivers/ufs/host/ufs-mediatek.c
> +++ b/drivers/ufs/host/ufs-mediatek.c
> @@ -192,8 +192,8 @@ static void ufs_mtk_crypto_enable(struct ufs_hba
> *hba)
>
> ufs_mtk_crypto_ctrl(res, 1);
> if (res.a0) {
> - dev_info(hba->dev, "%s: crypto enable failed, err:
> %lu\n",
> - __func__, res.a0);
> + dev_err(hba->dev, "%s: crypto enable failed with
> error %lu, disabling\n",
> + __func__, res.a0);
> hba->caps &= ~UFSHCD_CAP_CRYPTO;
> }
> }
> @@ -542,40 +542,38 @@ static void ufs_mtk_boost_crypt(struct ufs_hba
> *hba, bool boost)
>
> ret = clk_prepare_enable(cfg->clk_crypt_mux);
> if (ret) {
> - dev_info(hba->dev, "clk_prepare_enable(): %d\n",
> - ret);
> + dev_err(hba->dev, "%s: Failed to enable
> clk_crypt_mux: %pe\n",
> + __func__, ERR_PTR(ret));
> return;
> }
>
> if (boost) {
> ret = regulator_set_voltage(reg, volt, INT_MAX);
> if (ret) {
> - dev_info(hba->dev,
> - "failed to set vcore to %d\n",
> volt);
> + dev_err(hba->dev, "%s: Failed to set vcore
> to %d: %pe\n",
> + __func__, volt, ERR_PTR(ret));
> goto out;
> }
>
> - ret = clk_set_parent(cfg->clk_crypt_mux,
> - cfg->clk_crypt_perf);
> + ret = clk_set_parent(cfg->clk_crypt_mux, cfg-
> >clk_crypt_perf);
> if (ret) {
> - dev_info(hba->dev,
> - "failed to set clk_crypt_perf\n");
> + dev_err(hba->dev, "%s: Failed to reparent
> clk_crypt_perf: %pe\n",
> + __func__, ERR_PTR(ret));
> regulator_set_voltage(reg, 0, INT_MAX);
> goto out;
> }
> } else {
> - ret = clk_set_parent(cfg->clk_crypt_mux,
> - cfg->clk_crypt_lp);
> + ret = clk_set_parent(cfg->clk_crypt_mux, cfg-
> >clk_crypt_lp);
> if (ret) {
> - dev_info(hba->dev,
> - "failed to set clk_crypt_lp\n");
> + dev_err(hba->dev, "%s: Failed to reparent
> clk_crypt_lp: %pe\n",
> + __func__, ERR_PTR(ret));
> goto out;
> }
>
> ret = regulator_set_voltage(reg, 0, INT_MAX);
> if (ret) {
> - dev_info(hba->dev,
> - "failed to set vcore to MIN\n");
> + dev_err(hba->dev, "%s: Failed to set vcore
> to minimum: %pe\n",
> + __func__, ERR_PTR(ret));
> }
> }
> out:
> @@ -763,10 +761,8 @@ static int ufs_mtk_setup_clocks(struct ufs_hba
> *hba, bool on,
> if (clk_pwr_off) {
> ufs_mtk_pwr_ctrl(hba, false);
> } else {
> - dev_warn(hba->dev, "Clock is not turned off,
> hba->ahit = 0x%x, AHIT = 0x%x\n",
> - hba->ahit,
> - ufshcd_readl(hba,
> -
> REG_AUTO_HIBERNATE_IDLE_TIMER));
> + dev_warn(hba->dev, "Clock isn't off, hba-
> >ahit = 0x%x, AHIT = 0x%x\n",
> + hba->ahit, ufshcd_readl(hba,
> REG_AUTO_HIBERNATE_IDLE_TIMER));
> }
> ufs_mtk_mcq_disable_irq(hba);
> } else if (on && status == POST_CHANGE) {
> @@ -810,11 +806,11 @@ static void ufs_mtk_mcq_set_irq_affinity(struct
> ufs_hba *hba, unsigned int cpu)
> _cpu = (cpu == 0) ? 3 : cpu;
> ret = irq_set_affinity(irq, cpumask_of(_cpu));
> if (ret) {
> - dev_err(hba->dev, "set irq %d affinity to CPU %d
> failed\n",
> + dev_err(hba->dev, "setting irq %d affinity to CPU %d
> failed\n",
> irq, _cpu);
> return;
> }
> - dev_info(hba->dev, "set irq %d affinity to CPU: %d\n", irq,
> _cpu);
> + dev_dbg(hba->dev, "set irq %d affinity to CPU %d\n", irq,
> _cpu);
>
Is it more appropriate to use dev_info for state changes or for setting
changes?
> }
>
> static bool ufs_mtk_is_legacy_chipset(struct ufs_hba *hba, u32
> hw_ip_ver)
> @@ -830,7 +826,8 @@ static bool ufs_mtk_is_legacy_chipset(struct
> ufs_hba *hba, u32 hw_ip_ver)
> default:
> break;
> }
> - dev_info(hba->dev, "legacy IP version - 0x%x, is legacy :
> %d", hw_ip_ver, is_legacy);
> + dev_dbg(hba->dev, "IP version 0x%x, legacy = %s", hw_ip_ver,
> + str_true_false(is_legacy));
>
> return is_legacy;
> }
> @@ -935,15 +932,12 @@ static void ufs_mtk_init_clocks(struct ufs_hba
> *hba)
> }
> }
>
> - list_for_each_entry(clki, head, list) {
> - dev_info(hba->dev, "clk \"%s\" present", clki-
> >name);
> - }
> + list_for_each_entry(clki, head, list)
> + dev_dbg(hba->dev, "clk \"%s\" present", clki->name);
>
> if (!ufs_mtk_is_clk_scale_ready(hba)) {
> hba->caps &= ~UFSHCD_CAP_CLK_SCALING;
> - dev_info(hba->dev,
> - "%s: Clk-scaling not ready. Feature
> disabled.",
> - __func__);
> + dev_info(hba->dev, "%s: Clock scaling unavailable",
> __func__);
> return;
> }
>
> @@ -953,8 +947,8 @@ static void ufs_mtk_init_clocks(struct ufs_hba
> *hba)
> */
> reg = devm_regulator_get_optional(dev, "dvfsrc-vcore");
> if (IS_ERR(reg)) {
> - dev_info(dev, "failed to get dvfsrc-vcore: %ld",
> - PTR_ERR(reg));
> + if (PTR_ERR(reg) != -ENODEV)
> + dev_err(dev, "Failed to get dvfsrc-vcore:
> %pe\n", reg);
> return;
> }
>
> @@ -968,12 +962,9 @@ static void ufs_mtk_init_clocks(struct ufs_hba
> *hba)
> host->mclk.vcore_volt = volt;
>
> /* If default boot is max gear, request vcore */
> - if (reg && volt && host->clk_scale_up) {
> - if (regulator_set_voltage(reg, volt, INT_MAX)) {
> - dev_info(hba->dev,
> - "Failed to set vcore to %d\n",
> volt);
> - }
> - }
> + if (reg && volt && host->clk_scale_up)
> + if (regulator_set_voltage(reg, volt, INT_MAX))
> + dev_err(hba->dev, "Failed to set vcore to
> %d\n", volt);
> }
>
> static void ufs_mtk_setup_clk_gating(struct ufs_hba *hba)
> @@ -1060,7 +1051,7 @@ static void ufs_mtk_init_mcq_irq(struct ufs_hba
> *hba)
> }
> host->mcq_intr_info[i].hba = hba;
> host->mcq_intr_info[i].irq = irq;
> - dev_info(hba->dev, "get platform mcq irq: %d, %d\n",
> i, irq);
> + dev_dbg(hba->dev, "get platform mcq irq: %d, %d\n",
> i, irq);
> }
>
> return;
> @@ -1307,10 +1298,8 @@ static int ufs_mtk_pre_pwr_change(struct
> ufs_hba *hba,
> host_params.desired_working_mode = UFS_PWM_MODE;
>
> ret = ufshcd_negotiate_pwr_params(&host_params,
> dev_max_params, dev_req_params);
> - if (ret) {
> - pr_info("%s: failed to determine capabilities\n",
> - __func__);
> - }
> + if (ret)
> + dev_warn(hba->dev, "%s: failed to determine
> capabilities\n", __func__);
>
> if (ufs_mtk_pmc_via_fastauto(hba, dev_req_params)) {
> ufs_mtk_adjust_sync_length(hba);
> @@ -1356,10 +1345,9 @@ static int ufs_mtk_pre_pwr_change(struct
> ufs_hba *hba,
> ret = ufshcd_uic_change_pwr_mode(hba,
> FASTAUTO_MODE << 4 |
> FASTAUTO_MODE);
>
> - if (ret) {
> - dev_err(hba->dev, "%s: HSG1B FASTAUTO failed
> ret=%d\n",
> - __func__, ret);
> - }
> + if (ret)
> + dev_err(hba->dev, "%s: HSG1B FASTAUTO
> failed: %pe\n",
> + __func__, ERR_PTR(ret));
> }
>
> /* if already configured to the requested pwr_mode, skip
> adapt */
> @@ -1409,7 +1397,7 @@ static int ufs_mtk_auto_hibern8_disable(struct
> ufs_hba *hba)
>
> out:
> if (ret) {
> - dev_warn(hba->dev, "exit h8 state fail, ret=%d\n",
> ret);
> + dev_err(hba->dev, "Failed to exit h8 state: %pe\n",
> ERR_PTR(ret));
>
> ufshcd_force_error_recovery(hba);
>
> @@ -1571,7 +1559,7 @@ static int ufs_mtk_device_reset(struct ufs_hba
> *hba)
> /* Some devices may need time to respond to rst_n */
> usleep_range(10000, 15000);
>
> - dev_info(hba->dev, "device reset done\n");
> + dev_dbg(hba->dev, "device reset done\n");
>
Is it more appropriate to use dev_info for state changes or for setting
changes?
Thanks
Peter
On Tuesday, 24 February 2026 13:47:28 Central European Standard Time Peter Wang (王信友) wrote:
> On Mon, 2026-02-16 at 14:37 +0100, Nicolas Frattaroli wrote:
> > drivers/ufs/host/ufs-mediatek.c | 99 ++++++++++++++++++-------------
> > ----------
> > 1 file changed, 43 insertions(+), 56 deletions(-)
> >
> > diff --git a/drivers/ufs/host/ufs-mediatek.c b/drivers/ufs/host/ufs-
> > mediatek.c
> > index ecf16e82a326..2b1f26b55782 100644
> > --- a/drivers/ufs/host/ufs-mediatek.c
> > +++ b/drivers/ufs/host/ufs-mediatek.c
> > [... snip ...]
> > @@ -810,11 +806,11 @@ static void ufs_mtk_mcq_set_irq_affinity(struct
> > ufs_hba *hba, unsigned int cpu)
> > _cpu = (cpu == 0) ? 3 : cpu;
> > ret = irq_set_affinity(irq, cpumask_of(_cpu));
> > if (ret) {
> > - dev_err(hba->dev, "set irq %d affinity to CPU %d
> > failed\n",
> > + dev_err(hba->dev, "setting irq %d affinity to CPU %d
> > failed\n",
> > irq, _cpu);
> > return;
> > }
> > - dev_info(hba->dev, "set irq %d affinity to CPU: %d\n", irq,
> > _cpu);
> > + dev_dbg(hba->dev, "set irq %d affinity to CPU %d\n", irq,
> > _cpu);
> >
>
> Is it more appropriate to use dev_info for state changes or for setting
> changes?
Is this information a user would want to see in their bootup log in
every case? My understanding right now is no.
> > [... snip ...]
> > @@ -1571,7 +1559,7 @@ static int ufs_mtk_device_reset(struct ufs_hba
> > *hba)
> > /* Some devices may need time to respond to rst_n */
> > usleep_range(10000, 15000);
> >
> > - dev_info(hba->dev, "device reset done\n");
> > + dev_dbg(hba->dev, "device reset done\n");
> >
>
> Is it more appropriate to use dev_info for state changes or for setting
> changes?
Depends on your view of what's useful information for the user.
I can change both of these back to _info if I have to send out a next
revision, just to get this through though.
>
> Thanks
> Peter
>
Il 25/02/26 14:05, Nicolas Frattaroli ha scritto:
> On Tuesday, 24 February 2026 13:47:28 Central European Standard Time Peter Wang (王信友) wrote:
>> On Mon, 2026-02-16 at 14:37 +0100, Nicolas Frattaroli wrote:
>>> drivers/ufs/host/ufs-mediatek.c | 99 ++++++++++++++++++-------------
>>> ----------
>>> 1 file changed, 43 insertions(+), 56 deletions(-)
>>>
>>> diff --git a/drivers/ufs/host/ufs-mediatek.c b/drivers/ufs/host/ufs-
>>> mediatek.c
>>> index ecf16e82a326..2b1f26b55782 100644
>>> --- a/drivers/ufs/host/ufs-mediatek.c
>>> +++ b/drivers/ufs/host/ufs-mediatek.c
>>> [... snip ...]
>>> @@ -810,11 +806,11 @@ static void ufs_mtk_mcq_set_irq_affinity(struct
>>> ufs_hba *hba, unsigned int cpu)
>>> _cpu = (cpu == 0) ? 3 : cpu;
>>> ret = irq_set_affinity(irq, cpumask_of(_cpu));
>>> if (ret) {
>>> - dev_err(hba->dev, "set irq %d affinity to CPU %d
>>> failed\n",
>>> + dev_err(hba->dev, "setting irq %d affinity to CPU %d
>>> failed\n",
>>> irq, _cpu);
>>> return;
>>> }
>>> - dev_info(hba->dev, "set irq %d affinity to CPU: %d\n", irq,
>>> _cpu);
>>> + dev_dbg(hba->dev, "set irq %d affinity to CPU %d\n", irq,
>>> _cpu);
>>>
>>
>> Is it more appropriate to use dev_info for state changes or for setting
>> changes?
>
> Is this information a user would want to see in their bootup log in
> every case? My understanding right now is no.
>
>>> [... snip ...]
>>> @@ -1571,7 +1559,7 @@ static int ufs_mtk_device_reset(struct ufs_hba
>>> *hba)
>>> /* Some devices may need time to respond to rst_n */
>>> usleep_range(10000, 15000);
>>>
>>> - dev_info(hba->dev, "device reset done\n");
>>> + dev_dbg(hba->dev, "device reset done\n");
>>>
>>
>> Is it more appropriate to use dev_info for state changes or for setting
>> changes?
>
> Depends on your view of what's useful information for the user.
>
> I can change both of these back to _info if I have to send out a next
> revision, just to get this through though.
>
Definitely don't change that back to dev_info() as this is debugging information
that spams the kernel log for no reason.
This has to be dev_dbg().
Regards,
Angelo
>>
>> Thanks
>> Peter
>>
>
>
>
>
On Wed, 2026-02-25 at 14:18 +0100, AngeloGioacchino Del Regno wrote: > > Depends on your view of what's useful information for the user. > > > > I can change both of these back to _info if I have to send out a > > next > > revision, just to get this through though. > > > > Definitely don't change that back to dev_info() as this is debugging > information > that spams the kernel log for no reason. > > This has to be dev_dbg(). > > Regards, > Angelo > > > Hi AngeloGioacchino, Nicolas, At least, "device reset done" is important information that users would care about, and it should not spam the kernel log. You wouldn't expect device resets to occur repeatedly, would you? Thanks Peter
Il 26/02/26 08:00, Peter Wang (王信友) ha scritto: > On Wed, 2026-02-25 at 14:18 +0100, AngeloGioacchino Del Regno wrote: >>> Depends on your view of what's useful information for the user. >>> >>> I can change both of these back to _info if I have to send out a >>> next >>> revision, just to get this through though. >>> >> >> Definitely don't change that back to dev_info() as this is debugging >> information >> that spams the kernel log for no reason. >> >> This has to be dev_dbg(). >> >> Regards, >> Angelo >> >>> > > Hi AngeloGioacchino, Nicolas, > > At least, "device reset done" is important information that > users would care about, and it should not spam the kernel log. > You wouldn't expect device resets to occur repeatedly, would you? > Sorry Peter, but I'd argue that the users don't care about how much and when their UFS device resets. Users just want to use a device, without caring about any implementation detail. The spirit is: "radio silence as long as everything works good". Power users might want to check the kernel log in a problematic scenario to seek for a message that says that "something went horribly wrong", but other than developers, nobody cares about when UFS resets. From a developer standpoint, I do agree with you in that we do *not* want to see device resets occurring repeatedly, but we're talking about a user here. See it like this... imagine if all of the device drivers in the Linux kernel would say "device reset done": how many devices are present in one SoC (of course, ignoring subdevices on a board)? Of all those many devices, if all of them would print a message saying that their reset is done (and operation is ok), the kernel log would get quite a bit clogged, you'd need to have a bigger RAM carveout just for .. well, the kernel log itself, and then you'd have to grep the log, hoping to find the one single line that helps you finding an issue that you're having. This is the reason why keeping any message that is not exactly a *single* indication of an error (so, an actual issue) as a dev_dbg() is a sensible thing to do (and of course, with dynamic debug in the kernel, you can always activate that on-the-fly without recompiling to verify functionality should you have any immediate doubt). So while I agree about your reasons, I very strongly disagree about having this message as a dev_info(), nor anything else that is not dev_dbg() really. Regards, Angelo > Thanks > Peter >
On Thu, 2026-02-26 at 11:45 +0100, AngeloGioacchino Del Regno wrote: > Sorry Peter, but I'd argue that the users don't care about how much > and when > their UFS device resets. Users just want to use a device, without > caring > about any implementation detail. > The spirit is: "radio silence as long as everything works good". > > Power users might want to check the kernel log in a problematic > scenario to > seek for a message that says that "something went horribly wrong", > but other > than developers, nobody cares about when UFS resets. > > From a developer standpoint, I do agree with you in that we do *not* > want to > see device resets occurring repeatedly, but we're talking about a > user here. > > See it like this... imagine if all of the device drivers in the Linux > kernel > would say "device reset done": how many devices are present in one > SoC (of > course, ignoring subdevices on a board)? > > Of all those many devices, if all of them would print a message > saying that > their reset is done (and operation is ok), the kernel log would get > quite a > bit clogged, you'd need to have a bigger RAM carveout just for .. > well, the > kernel log itself, and then you'd have to grep the log, hoping to > find the > one single line that helps you finding an issue that you're having. > > This is the reason why keeping any message that is not exactly a > *single* > indication of an error (so, an actual issue) as a dev_dbg() is a > sensible > thing to do (and of course, with dynamic debug in the kernel, you can > always > activate that on-the-fly without recompiling to verify functionality > should > you have any immediate doubt). > > So while I agree about your reasons, I very strongly disagree about > having > this message as a dev_info(), nor anything else that is not dev_dbg() > really. > > Regards, > Angelo Hi AngeloGioacchino, I am not sure if you know that when UFS encounters an error, such as a UIC error or timeout, some errors can be so severe that they cannot be recovered without a reset. In these cases, we need to perform error handling or recovery by resetting the device. I agree that "radio silence is preferable as long as everything works well." However, users may sometimes wonder why their device (phone, tablet, laptop, etc.) shows good IO performance in tests, but the actual user experience is poor (laggy). This log can provide users with an explanation for IO lag during usage. I also agree that many devices are present in a single SoC, but I don't think there is much reset information throughout the system. Each device should ensure that the likelihood of a reset (error) is minimized. Thanks. Peter
© 2016 - 2026 Red Hat, Inc.