Re: [PATCH v3 01/25] bus: mhi: Fix pm_state conversion to string
From: Alex Elder
Date: Wed Feb 16 2022 - 08:42:04 EST
On 2/16/22 5:33 AM, Manivannan Sadhasivam wrote:
On Tue, Feb 15, 2022 at 02:01:54PM -0600, Alex Elder wrote:
On 2/12/22 12:20 PM, Manivannan Sadhasivam wrote:
From: Paul Davey <paul.davey@xxxxxxxxxxxxxxxxxxx>
On big endian architectures the mhi debugfs files which report pm state
give "Invalid State" for all states. This is caused by using
find_last_bit which takes an unsigned long* while the state is passed in
as an enum mhi_pm_state which will be of int size.
I think this would have fixed it too, but your fix is better.
int index = find_last_bit(&(unsigned long)state, 32);
Fix by using __fls to pass the value of state instead of find_last_bit.
Fixes: a6e2e3522f29 ("bus: mhi: core: Add support for PM state transitions")
Signed-off-by: Paul Davey <paul.davey@xxxxxxxxxxxxxxxxxxx>
Reviewed-by: Manivannan Sadhasivam <mani@xxxxxxxxxx>
Reviewed-by: Hemant Kumar <hemantk@xxxxxxxxxxxxxx>
Cc: stable@xxxxxxxxxxxxxxx
Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@xxxxxxxxxx>
---
drivers/bus/mhi/core/init.c | 8 +++++---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/drivers/bus/mhi/core/init.c b/drivers/bus/mhi/core/init.c
index 046f407dc5d6..af484b03558a 100644
--- a/drivers/bus/mhi/core/init.c
+++ b/drivers/bus/mhi/core/init.c
@@ -79,10 +79,12 @@ static const char * const mhi_pm_state_str[] = {
const char *to_mhi_pm_state_str(enum mhi_pm_state state)
The mhi_pm_state enumerated type is an enumerated sequence, not
a bit mask. So knowing what the last (most significant) set bit
is not meaningful. Or normally it shouldn't be.
If mhi_pm_state really were a bit mask, then its values should
be defined that way, i.e.,
MHI_PM_STATE_DISABLE = 1 << 0,
MHI_PM_STATE_DISABLE = 1 << 1,
. . .
What's really going on is that the state value passed here
*is* a bitmask, whose bit positions are those mhi_pm_state
values. So the state argument should have type u32.
I agree with you. It should be u32.
This is a *separate* bug/issue. It could be fixed separately
(before this patch), but I'd be OK with just explaining why
this change would occur as part of this modified patch.
It makes sense to do it in the same patch itself as the change is
minimal and moreover this patch will also get backported to stable.
Sounds good to me. -Alex
{
- unsigned long pm_state = state;
- int index = find_last_bit(&pm_state, 32);
+ int index;
- if (index >= ARRAY_SIZE(mhi_pm_state_str))
+ if (state)
+ index = __fls(state);
+
+ if (!state || index >= ARRAY_SIZE(mhi_pm_state_str))
return "Invalid State";
Do this test and return first, and skip the additional
check for "if (state)".
We need to calculate index for the second check, so I guess the current
code is fine.
Thanks,
Mani
-Alex
return mhi_pm_state_str[index];