From: Paul Davey paul.davey@alliedtelesis.co.nz
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.
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@alliedtelesis.co.nz Reviewed-by: Manivannan Sadhasivam mani@kernel.org Reviewed-by: Hemant Kumar hemantk@codeaurora.org Cc: stable@vger.kernel.org Signed-off-by: Manivannan Sadhasivam manivannan.sadhasivam@linaro.org --- 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) { - 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";
return mhi_pm_state_str[index];
On 2/12/22 12:20 PM, Manivannan Sadhasivam wrote:
From: Paul Davey paul.davey@alliedtelesis.co.nz
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@alliedtelesis.co.nz Reviewed-by: Manivannan Sadhasivam mani@kernel.org Reviewed-by: Hemant Kumar hemantk@codeaurora.org Cc: stable@vger.kernel.org Signed-off-by: Manivannan Sadhasivam manivannan.sadhasivam@linaro.org
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.
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.
{
- 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)".
-Alex
return mhi_pm_state_str[index];
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@alliedtelesis.co.nz
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@alliedtelesis.co.nz Reviewed-by: Manivannan Sadhasivam mani@kernel.org Reviewed-by: Hemant Kumar hemantk@codeaurora.org Cc: stable@vger.kernel.org Signed-off-by: Manivannan Sadhasivam manivannan.sadhasivam@linaro.org
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.
{
- 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
-Alexreturn mhi_pm_state_str[index];
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@alliedtelesis.co.nz
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@alliedtelesis.co.nz Reviewed-by: Manivannan Sadhasivam mani@kernel.org Reviewed-by: Hemant Kumar hemantk@codeaurora.org Cc: stable@vger.kernel.org Signed-off-by: Manivannan Sadhasivam manivannan.sadhasivam@linaro.org
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
-Alexreturn mhi_pm_state_str[index];
linux-stable-mirror@lists.linaro.org
-
Alex Elder -
Manivannan Sadhasivam