* [PATCH] ufs: core: Initialize a variable mode for PA_PWRMODE [not found] <CGME20251002070057epcas1p49ac487359f24f6813ba8f9f44bcf0924@epcas1p4.samsung.com> @ 2025-10-02 7:00 ` Wonkon Kim 2025-10-02 8:00 ` Peter Wang (王信友) ` (2 more replies) 0 siblings, 3 replies; 8+ messages in thread From: Wonkon Kim @ 2025-10-02 7:00 UTC (permalink / raw) To: James.Bottomley, martin.petersen, peter.wang, linux-scsi, linux-kernel Cc: wkon.kim If ufshcd_dme_get() fails uic cmd error, a variable mode has a garbage value. It may return unintended result for pwr mode restore. Initialize it as 0 and will return true when ufshcd_dme_get() fails, because PA power mode 0 is not defined. Signed-off-by: Wonkon Kim <wkon.kim@samsung.com> --- drivers/ufs/core/ufshcd.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c index 9a43102b2b21..a4438a3cb73a 100644 --- a/drivers/ufs/core/ufshcd.c +++ b/drivers/ufs/core/ufshcd.c @@ -6581,7 +6581,7 @@ static inline void ufshcd_recover_pm_error(struct ufs_hba *hba) static bool ufshcd_is_pwr_mode_restore_needed(struct ufs_hba *hba) { struct ufs_pa_layer_attr *pwr_info = &hba->pwr_info; - u32 mode; + u32 mode = 0; ufshcd_dme_get(hba, UIC_ARG_MIB(PA_PWRMODE), &mode); -- 2.34.1 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] ufs: core: Initialize a variable mode for PA_PWRMODE 2025-10-02 7:00 ` [PATCH] ufs: core: Initialize a variable mode for PA_PWRMODE Wonkon Kim @ 2025-10-02 8:00 ` Peter Wang (王信友) 2025-10-02 16:18 ` Bart Van Assche 2025-10-02 16:23 ` Bart Van Assche 2 siblings, 0 replies; 8+ messages in thread From: Peter Wang (王信友) @ 2025-10-02 8:00 UTC (permalink / raw) To: linux-scsi@vger.kernel.org, wkon.kim@samsung.com, James.Bottomley@HansenPartnership.com, linux-kernel@vger.kernel.org, martin.petersen@oracle.com On Thu, 2025-10-02 at 16:00 +0900, Wonkon Kim wrote: > > If ufshcd_dme_get() fails uic cmd error, > a variable mode has a garbage value. > It may return unintended result for pwr mode restore. > > Initialize it as 0 and will return true when ufshcd_dme_get() fails, > because PA power mode 0 is not defined. > > Signed-off-by: Wonkon Kim <wkon.kim@samsung.com> > --- > drivers/ufs/core/ufshcd.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c > index 9a43102b2b21..a4438a3cb73a 100644 > --- a/drivers/ufs/core/ufshcd.c > +++ b/drivers/ufs/core/ufshcd.c > @@ -6581,7 +6581,7 @@ static inline void > ufshcd_recover_pm_error(struct ufs_hba *hba) > static bool ufshcd_is_pwr_mode_restore_needed(struct ufs_hba *hba) > { > struct ufs_pa_layer_attr *pwr_info = &hba->pwr_info; > - u32 mode; > + u32 mode = 0; > > ufshcd_dme_get(hba, UIC_ARG_MIB(PA_PWRMODE), &mode); > > -- > 2.34.1 > Reviewed-by: Peter Wang <peter.wang@mediatek.com> ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] ufs: core: Initialize a variable mode for PA_PWRMODE 2025-10-02 7:00 ` [PATCH] ufs: core: Initialize a variable mode for PA_PWRMODE Wonkon Kim 2025-10-02 8:00 ` Peter Wang (王信友) @ 2025-10-02 16:18 ` Bart Van Assche 2025-10-02 16:23 ` Bart Van Assche 2 siblings, 0 replies; 8+ messages in thread From: Bart Van Assche @ 2025-10-02 16:18 UTC (permalink / raw) To: Wonkon Kim, James.Bottomley, martin.petersen, peter.wang, linux-scsi, linux-kernel On 10/2/25 12:00 AM, Wonkon Kim wrote: > static bool ufshcd_is_pwr_mode_restore_needed(struct ufs_hba *hba) > { > struct ufs_pa_layer_attr *pwr_info = &hba->pwr_info; > - u32 mode; > + u32 mode = 0; > > ufshcd_dme_get(hba, UIC_ARG_MIB(PA_PWRMODE), &mode); Wouldn't it be better to check the ufshcd_dme_get() return value rather than to zero-initialize 'mode'? I think that would make ufshcd_is_pwr_mode_restore_needed() easier to read compared to applying the above patch. Thanks, Bart. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] ufs: core: Initialize a variable mode for PA_PWRMODE 2025-10-02 7:00 ` [PATCH] ufs: core: Initialize a variable mode for PA_PWRMODE Wonkon Kim 2025-10-02 8:00 ` Peter Wang (王信友) 2025-10-02 16:18 ` Bart Van Assche @ 2025-10-02 16:23 ` Bart Van Assche 2025-10-13 2:23 ` Wonkon Kim 2025-10-13 8:20 ` Wonkon Kim 2 siblings, 2 replies; 8+ messages in thread From: Bart Van Assche @ 2025-10-02 16:23 UTC (permalink / raw) To: Wonkon Kim, James.Bottomley, martin.petersen, peter.wang, linux-scsi, linux-kernel On 10/2/25 12:00 AM, Wonkon Kim wrote: > static bool ufshcd_is_pwr_mode_restore_needed(struct ufs_hba *hba) > { > struct ufs_pa_layer_attr *pwr_info = &hba->pwr_info; > - u32 mode; > + u32 mode = 0; > > ufshcd_dme_get(hba, UIC_ARG_MIB(PA_PWRMODE), &mode); Since there is more code that passes a pointer to an uninitialized variable to ufshcd_dme_get(), the untested patch below may be a better solution: diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c index 127b691402f9..5226fbca29ec 100644 --- a/drivers/ufs/core/ufshcd.c +++ b/drivers/ufs/core/ufshcd.c @@ -4277,8 +4277,8 @@ int ufshcd_dme_get_attr(struct ufs_hba *hba, u32 attr_sel, get, UIC_GET_ATTR_ID(attr_sel), UFS_UIC_COMMAND_RETRIES - retries); - if (mib_val && !ret) - *mib_val = uic_cmd.argument3; + if (mib_val) + *mib_val = ret == 0 ? uic_cmd.argument3 : 0; if (peer && (hba->quirks & UFSHCD_QUIRK_DME_PEER_ACCESS_AUTO_MODE) && pwr_mode_change) ^ permalink raw reply related [flat|nested] 8+ messages in thread
* RE: [PATCH] ufs: core: Initialize a variable mode for PA_PWRMODE 2025-10-02 16:23 ` Bart Van Assche @ 2025-10-13 2:23 ` Wonkon Kim 2025-10-13 8:20 ` Wonkon Kim 1 sibling, 0 replies; 8+ messages in thread From: Wonkon Kim @ 2025-10-13 2:23 UTC (permalink / raw) To: 'Bart Van Assche', James.Bottomley, martin.petersen, peter.wang, linux-scsi, linux-kernel > On 10/2/25 12:00 AM, Wonkon Kim wrote: > > static bool ufshcd_is_pwr_mode_restore_needed(struct ufs_hba *hba) > > { > > struct ufs_pa_layer_attr *pwr_info = &hba->pwr_info; > > - u32 mode; > > + u32 mode = 0; > > > > ufshcd_dme_get(hba, UIC_ARG_MIB(PA_PWRMODE), &mode); > > Since there is more code that passes a pointer to an uninitialized > variable to ufshcd_dme_get(), the untested patch below may be a better > solution: > > diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c index > 127b691402f9..5226fbca29ec 100644 > --- a/drivers/ufs/core/ufshcd.c > +++ b/drivers/ufs/core/ufshcd.c > @@ -4277,8 +4277,8 @@ int ufshcd_dme_get_attr(struct ufs_hba *hba, u32 > attr_sel, > get, UIC_GET_ATTR_ID(attr_sel), > UFS_UIC_COMMAND_RETRIES - retries); > > - if (mib_val && !ret) > - *mib_val = uic_cmd.argument3; > + if (mib_val) > + *mib_val = ret == 0 ? uic_cmd.argument3 : 0; > > if (peer && (hba->quirks & UFSHCD_QUIRK_DME_PEER_ACCESS_AUTO_MODE) > && pwr_mode_change) > > Sorry for late reply. Ok, I agree with you and it would be more general. There is a code to initialize a variable. Do you think it also needs to fix? diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c index a4438a3cb73a..d593ff7ea63d 100644 --- a/drivers/ufs/core/ufshcd.c +++ b/drivers/ufs/core/ufshcd.c @@ -4990,7 +4990,7 @@ EXPORT_SYMBOL_GPL(ufshcd_hba_enable); static int ufshcd_disable_tx_lcc(struct ufs_hba *hba, bool peer) { - int tx_lanes = 0, i, err = 0; + int tx_lanes, i, err = 0; if (!peer) ufshcd_dme_get(hba, UIC_ARG_MIB(PA_CONNECTEDTXDATALANES), &tx_lanes); else ufshcd_dme_peer_get(hba, UIC_ARG_MIB(PA_CONNECTEDTXDATALANES), &tx_lanes); Thanks, Wonkon Kim. ^ permalink raw reply related [flat|nested] 8+ messages in thread
* RE: [PATCH] ufs: core: Initialize a variable mode for PA_PWRMODE 2025-10-02 16:23 ` Bart Van Assche 2025-10-13 2:23 ` Wonkon Kim @ 2025-10-13 8:20 ` Wonkon Kim 2025-10-13 16:19 ` Bart Van Assche 1 sibling, 1 reply; 8+ messages in thread From: Wonkon Kim @ 2025-10-13 8:20 UTC (permalink / raw) To: 'Bart Van Assche', James.Bottomley, martin.petersen, peter.wang, linux-scsi, linux-kernel > On 10/2/25 12:00 AM, Wonkon Kim wrote: > > static bool ufshcd_is_pwr_mode_restore_needed(struct ufs_hba *hba) > > { > > struct ufs_pa_layer_attr *pwr_info = &hba->pwr_info; > > - u32 mode; > > + u32 mode = 0; > > > > ufshcd_dme_get(hba, UIC_ARG_MIB(PA_PWRMODE), &mode); > > Since there is more code that passes a pointer to an uninitialized > variable to ufshcd_dme_get(), the untested patch below may be a better > solution: > > diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c index > 127b691402f9..5226fbca29ec 100644 > --- a/drivers/ufs/core/ufshcd.c > +++ b/drivers/ufs/core/ufshcd.c > @@ -4277,8 +4277,8 @@ int ufshcd_dme_get_attr(struct ufs_hba *hba, u32 > attr_sel, > get, UIC_GET_ATTR_ID(attr_sel), > UFS_UIC_COMMAND_RETRIES - retries); > > - if (mib_val && !ret) > - *mib_val = uic_cmd.argument3; > + if (mib_val) > + *mib_val = ret == 0 ? uic_cmd.argument3 : 0; > > if (peer && (hba->quirks & UFSHCD_QUIRK_DME_PEER_ACCESS_AUTO_MODE) > && pwr_mode_change) > > There are some attributes to use 0 as valid value. e.g. PA_MAXRXHSGEAR is set to 0 for NO_HS=0 If it has 0 for valid value, most of value 0 are regarded as FALSE, unsupported or minimum. And these cases seems to check ret for command success/fail in code. However, is it ok to set 0 for ufshcd_send_uic_cmd() fail? If it can't, it needs to initialize mode. Value 0 for PA_PWRMODE is invalid. Thanks, Wonkon Kim. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] ufs: core: Initialize a variable mode for PA_PWRMODE 2025-10-13 8:20 ` Wonkon Kim @ 2025-10-13 16:19 ` Bart Van Assche 2025-10-14 4:35 ` Wonkon Kim 0 siblings, 1 reply; 8+ messages in thread From: Bart Van Assche @ 2025-10-13 16:19 UTC (permalink / raw) To: Wonkon Kim, James.Bottomley, martin.petersen, peter.wang, linux-scsi, linux-kernel On 10/13/25 1:20 AM, Wonkon Kim wrote: >> On 10/2/25 12:00 AM, Wonkon Kim wrote: >>> static bool ufshcd_is_pwr_mode_restore_needed(struct ufs_hba *hba) >>> { >>> struct ufs_pa_layer_attr *pwr_info = &hba->pwr_info; >>> - u32 mode; >>> + u32 mode = 0; >>> >>> ufshcd_dme_get(hba, UIC_ARG_MIB(PA_PWRMODE), &mode); >> >> Since there is more code that passes a pointer to an uninitialized >> variable to ufshcd_dme_get(), the untested patch below may be a better >> solution: >> >> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c index >> 127b691402f9..5226fbca29ec 100644 >> --- a/drivers/ufs/core/ufshcd.c >> +++ b/drivers/ufs/core/ufshcd.c >> @@ -4277,8 +4277,8 @@ int ufshcd_dme_get_attr(struct ufs_hba *hba, u32 >> attr_sel, >> get, UIC_GET_ATTR_ID(attr_sel), >> UFS_UIC_COMMAND_RETRIES - retries); >> >> - if (mib_val && !ret) >> - *mib_val = uic_cmd.argument3; >> + if (mib_val) >> + *mib_val = ret == 0 ? uic_cmd.argument3 : 0; >> >> if (peer && (hba->quirks & UFSHCD_QUIRK_DME_PEER_ACCESS_AUTO_MODE) >> && pwr_mode_change) >> >> > > There are some attributes to use 0 as valid value. > e.g. PA_MAXRXHSGEAR is set to 0 for NO_HS=0 > If it has 0 for valid value, most of value 0 are regarded as FALSE, unsupported or minimum. > And these cases seems to check ret for command success/fail in code. > However, is it ok to set 0 for ufshcd_send_uic_cmd() fail? > > If it can't, it needs to initialize mode. > Value 0 for PA_PWRMODE is invalid. Hi Wonkon, Modifying ufshcd_dme_get_attr() doesn't exclude checking the return value of ufshcd_dme_get_attr(). I propose to modify ufshcd_dme_get_attr() such that it always initializes *mib_val and also to check the ufshcd_dme_get_attr() return value wherever appropriate. Thanks, Bart. ^ permalink raw reply [flat|nested] 8+ messages in thread
* RE: [PATCH] ufs: core: Initialize a variable mode for PA_PWRMODE 2025-10-13 16:19 ` Bart Van Assche @ 2025-10-14 4:35 ` Wonkon Kim 0 siblings, 0 replies; 8+ messages in thread From: Wonkon Kim @ 2025-10-14 4:35 UTC (permalink / raw) To: 'Bart Van Assche', James.Bottomley, martin.petersen, peter.wang, linux-scsi, linux-kernel > On 10/13/25 1:20 AM, Wonkon Kim wrote: > >> On 10/2/25 12:00 AM, Wonkon Kim wrote: > >>> static bool ufshcd_is_pwr_mode_restore_needed(struct ufs_hba *hba) > >>> { > >>> struct ufs_pa_layer_attr *pwr_info = &hba->pwr_info; > >>> - u32 mode; > >>> + u32 mode = 0; > >>> > >>> ufshcd_dme_get(hba, UIC_ARG_MIB(PA_PWRMODE), &mode); > >> > >> Since there is more code that passes a pointer to an uninitialized > >> variable to ufshcd_dme_get(), the untested patch below may be a > >> better > >> solution: > >> > >> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c > >> index 127b691402f9..5226fbca29ec 100644 > >> --- a/drivers/ufs/core/ufshcd.c > >> +++ b/drivers/ufs/core/ufshcd.c > >> @@ -4277,8 +4277,8 @@ int ufshcd_dme_get_attr(struct ufs_hba *hba, > >> u32 attr_sel, > >> get, UIC_GET_ATTR_ID(attr_sel), > >> UFS_UIC_COMMAND_RETRIES - retries); > >> > >> - if (mib_val && !ret) > >> - *mib_val = uic_cmd.argument3; > >> + if (mib_val) > >> + *mib_val = ret == 0 ? uic_cmd.argument3 : 0; > >> > >> if (peer && (hba->quirks & UFSHCD_QUIRK_DME_PEER_ACCESS_AUTO_MODE) > >> && pwr_mode_change) > >> > >> > > > > There are some attributes to use 0 as valid value. > > e.g. PA_MAXRXHSGEAR is set to 0 for NO_HS=0 If it has 0 for valid > > value, most of value 0 are regarded as FALSE, unsupported or minimum. > > And these cases seems to check ret for command success/fail in code. > > However, is it ok to set 0 for ufshcd_send_uic_cmd() fail? > > > > If it can't, it needs to initialize mode. > > Value 0 for PA_PWRMODE is invalid. > > Hi Wonkon, > > Modifying ufshcd_dme_get_attr() doesn't exclude checking the return value > of ufshcd_dme_get_attr(). I propose to modify > ufshcd_dme_get_attr() such that it always initializes *mib_val and also to > check the ufshcd_dme_get_attr() return value wherever appropriate. > > Thanks, > > Bart. Hi Bart, I got it. I'll update it. Thanks, Wonkon Kim. ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2025-10-14 4:35 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <CGME20251002070057epcas1p49ac487359f24f6813ba8f9f44bcf0924@epcas1p4.samsung.com>
2025-10-02 7:00 ` [PATCH] ufs: core: Initialize a variable mode for PA_PWRMODE Wonkon Kim
2025-10-02 8:00 ` Peter Wang (王信友)
2025-10-02 16:18 ` Bart Van Assche
2025-10-02 16:23 ` Bart Van Assche
2025-10-13 2:23 ` Wonkon Kim
2025-10-13 8:20 ` Wonkon Kim
2025-10-13 16:19 ` Bart Van Assche
2025-10-14 4:35 ` Wonkon Kim
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox