Re: [PATCH 11/22] firmware: arm_scmi: Add SCMIv3.1 extended names protocols support

From: Florian Fainelli
Date: Wed Jun 15 2022 - 13:20:01 EST


On 6/15/22 09:29, Cristian Marussi wrote:
On Wed, Jun 15, 2022 at 09:10:03AM -0700, Florian Fainelli wrote:
On 6/15/22 02:40, Cristian Marussi wrote:
On Wed, Jun 15, 2022 at 09:18:03AM +0100, Cristian Marussi wrote:
On Wed, Jun 15, 2022 at 05:45:11AM +0200, Florian Fainelli wrote:


On 3/30/2022 5:05 PM, Cristian Marussi wrote:
Using the common protocol helper implementation add support for all new
SCMIv3.1 extended names commands related to all protocols with the
exception of SENSOR_AXIS_GET_NAME.

Signed-off-by: Cristian Marussi <cristian.marussi@xxxxxxx>

This causes the following splat on a platform where regulators fail to
initialize:


Hi Florian,

thanks for the report.

It seems a memory error while allocating so it was not meant to be
solved by the fixes, anyway, I've never seen this splat in my testing
and at first sight I cannot see anything wrong in the devm_k* calls
inside scmi_voltage_protocol_init...is there any particular config in
your setup ?

Moreover, the WARNING line 5402 seems to match v5.19-rc1 and it has
slightly changed with -rc-1, so I'll try rebasing on that at first and
see if I can reproduce the issue locally.


I just re-tested the series rebased on v519-rc1 plus fixes and I cannot
reproduce in my setup with a few (~9) good and bad voltage domains.

How many voltage domains are advertised by the platform in your setup ?

There are 11 voltage regulators on this platform, and of course, now that I
am trying to reproduce the splat I reported I just cannot anymore... I will
let you know if there is anything that needs to be done. Thanks for being
responsive as usual!

... you're welcome...

I'm trying to figure out where an abnormal mem request could happen...

I think the problem is/was with the number of voltage domains being reported which was way too big and passed directly, without any clamping to devm_kcalloc() resulting the splat indicating that the allocation was beyond the MAX_ORDER. The specification allows for up to 2^16 domains which would still be way too much to allocate using kmalloc() so we could/should consider vmalloc() here eventually?

In all likelihood though we probably won't find a system with 65k voltage domains.


can you try adding this (for brutal debugging) when you try ?
(...just to rule out funny fw replies.... :D)

Sure, nothing weird coming out and it succeeded in enumerating all of the regulators, I smell a transient issue with our firmware implementation, maybe...

[ 0.560544] arm-scmi brcm_scmi@0: num_returned:1 num_remaining:0
[ 0.560617] arm-scmi brcm_scmi@0: num_returned:1 num_remaining:0
[ 0.560673] arm-scmi brcm_scmi@0: num_returned:1 num_remaining:0
[ 0.560730] arm-scmi brcm_scmi@0: num_returned:1 num_remaining:0
[ 0.560881] arm-scmi brcm_scmi@0: num_returned:1 num_remaining:0
[ 0.560940] arm-scmi brcm_scmi@0: num_returned:1 num_remaining:0
[ 0.560996] arm-scmi brcm_scmi@0: num_returned:1 num_remaining:0
[ 0.561054] arm-scmi brcm_scmi@0: num_returned:1 num_remaining:0
[ 0.561110] arm-scmi brcm_scmi@0: num_returned:1 num_remaining:0
[ 0.561168] arm-scmi brcm_scmi@0: num_returned:1 num_remaining:0
[ 0.561225] arm-scmi brcm_scmi@0: num_returned:1 num_remaining:0
[ 0.561652] scmi-regulator scmi_dev.2: Regulator stb_vreg_2 registered for domain [2]
[ 0.561858] scmi-regulator scmi_dev.2: Regulator stb_vreg_3 registered for domain [3]
[ 0.562030] scmi-regulator scmi_dev.2: Regulator stb_vreg_4 registered for domain [4]
[ 0.562190] scmi-regulator scmi_dev.2: Regulator stb_vreg_5 registered for domain [5]
[ 0.564427] scmi-regulator scmi_dev.2: Regulator stb_vreg_6 registered for domain [6]
[ 0.564638] scmi-regulator scmi_dev.2: Regulator stb_vreg_7 registered for domain [7]
[ 0.564817] scmi-regulator scmi_dev.2: Regulator stb_vreg_8 registered for domain [8]
[ 0.565030] scmi-regulator scmi_dev.2: Regulator stb_vreg_9 registered for domain [9]
[ 0.565191] scmi-regulator scmi_dev.2: Regulator stb_vreg_10 registered for domain [10]

--
Florian