| Message ID | 20211116173255.19936-1-vincent.guittot@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | arm/scmi: fix base agent discover response | expand |
On Tue, Nov 16, 2021 at 06:32:55PM +0100, Vincent Guittot wrote: > According to scmi specification, the response of the discover agent request > is made of: > - int32 status > - uint32 agent_id > - uint8 name[16] > > but the current implementation doesn't take into account the agent_id field > and only allocates a rx buffer of SCMI_MAX_STR_SIZE length > Hi Vincent, > Allocate the correct length for rx buffer and copy the name from the > correct offset in the response. > > While no error were returned until v5.15, v5.16-rc1 fails with virtio_scmi > transport channel: > > [ 1.093253] arm-scmi firmware:scmi0: SCMI Notifications - Core Enabled. > [ 1.114776] arm-scmi firmware:scmi0: SCMI Protocol v2.0 'Linaro:PMWG' Firmware version 0x2090000 > [ 1.117544] scmi-virtio virtio0: tx:used len 28 is larger than in buflen 24 > Good catch...I was (still) not testing BASE_DISCOVER_AGENT in my virtio setup given it's optional.... Once implemented the command I could reproduce and test you fix. [ 1.239629] arm-scmi firmware:scmi: SCMI Notifications - Core Enabled. [ 1.266401] arm-scmi firmware:scmi: SCMI Protocol v2.0 'EMU-SCMI-VM:userland' Firmware version 0xdeadbeef [ 1.280360] arm-scmi firmware:scmi: Found 3 protocol(s) 3 agent(s) [ 1.286304] arm-scmi firmware:scmi: Agent 0: AGENT_00 [ 1.294115] arm-scmi firmware:scmi: Agent 1: AGENT_01 [ 1.301062] arm-scmi firmware:scmi: Agent 2: AGENT_02 LGTM. Maybe it's worth also a Fixes... The earlier where this was introduced seems: Fixes: b6f20ff8bd94 ("firmware: arm_scmi: add common infrastructure and support for base protocol") and then it was carried on (:D) after heavily refactoring in: Fixes: 8d3581c2526f ("firmware: arm_scmi: Port base protocol to new interface") FWIW, Reviewed-by: Cristian Marussi <cristian.marussi@arm.com> Tested-by: Cristian Marussi <cristian.marussi@arm.com> Thanks, Cristian
Hi Cristian, On Tue, 16 Nov 2021 at 19:28, Cristian Marussi <cristian.marussi@arm.com> wrote: > > On Tue, Nov 16, 2021 at 06:32:55PM +0100, Vincent Guittot wrote: > > According to scmi specification, the response of the discover agent request > > is made of: > > - int32 status > > - uint32 agent_id > > - uint8 name[16] > > > > but the current implementation doesn't take into account the agent_id field > > and only allocates a rx buffer of SCMI_MAX_STR_SIZE length > > > > Hi Vincent, > > > Allocate the correct length for rx buffer and copy the name from the > > correct offset in the response. > > > > While no error were returned until v5.15, v5.16-rc1 fails with virtio_scmi > > transport channel: > > > > [ 1.093253] arm-scmi firmware:scmi0: SCMI Notifications - Core Enabled. > > [ 1.114776] arm-scmi firmware:scmi0: SCMI Protocol v2.0 'Linaro:PMWG' Firmware version 0x2090000 > > [ 1.117544] scmi-virtio virtio0: tx:used len 28 is larger than in buflen 24 > > > > Good catch...I was (still) not testing BASE_DISCOVER_AGENT in my virtio setup > given it's optional.... > > Once implemented the command I could reproduce and test you fix. > > [ 1.239629] arm-scmi firmware:scmi: SCMI Notifications - Core Enabled. > [ 1.266401] arm-scmi firmware:scmi: SCMI Protocol v2.0 'EMU-SCMI-VM:userland' Firmware version 0xdeadbeef > [ 1.280360] arm-scmi firmware:scmi: Found 3 protocol(s) 3 agent(s) > [ 1.286304] arm-scmi firmware:scmi: Agent 0: AGENT_00 > [ 1.294115] arm-scmi firmware:scmi: Agent 1: AGENT_01 > [ 1.301062] arm-scmi firmware:scmi: Agent 2: AGENT_02 > > LGTM. > > Maybe it's worth also a Fixes... I was not sure which commit to go back. > > The earlier where this was introduced seems: > > Fixes: b6f20ff8bd94 ("firmware: arm_scmi: add common infrastructure and support for base protocol") I'm going to put this one to make sure that all LTS will get a fix > > and then it was carried on (:D) after heavily refactoring in: > > Fixes: 8d3581c2526f ("firmware: arm_scmi: Port base protocol to new interface") > > FWIW, > > Reviewed-by: Cristian Marussi <cristian.marussi@arm.com> > Tested-by: Cristian Marussi <cristian.marussi@arm.com> Thanks > > Thanks, > Cristian >
diff --git a/drivers/firmware/arm_scmi/base.c b/drivers/firmware/arm_scmi/base.c index de416f9e7921..f5219334fd3a 100644 --- a/drivers/firmware/arm_scmi/base.c +++ b/drivers/firmware/arm_scmi/base.c @@ -34,6 +34,12 @@ struct scmi_msg_resp_base_attributes { __le16 reserved; }; +struct scmi_msg_resp_base_discover_agent { + __le32 agent_id; + u8 name[SCMI_MAX_STR_SIZE]; +}; + + struct scmi_msg_base_error_notify { __le32 event_control; #define BASE_TP_NOTIFY_ALL BIT(0) @@ -225,18 +231,21 @@ static int scmi_base_discover_agent_get(const struct scmi_protocol_handle *ph, int id, char *name) { int ret; + struct scmi_msg_resp_base_discover_agent *agent_info; struct scmi_xfer *t; ret = ph->xops->xfer_get_init(ph, BASE_DISCOVER_AGENT, - sizeof(__le32), SCMI_MAX_STR_SIZE, &t); + sizeof(__le32), sizeof(*agent_info), &t); if (ret) return ret; put_unaligned_le32(id, t->tx.buf); ret = ph->xops->do_xfer(ph, t); - if (!ret) - strlcpy(name, t->rx.buf, SCMI_MAX_STR_SIZE); + if (!ret) { + agent_info = t->rx.buf; + strlcpy(name, agent_info->name, SCMI_MAX_STR_SIZE); + } ph->xops->xfer_put(ph, t);
According to scmi specification, the response of the discover agent request is made of: - int32 status - uint32 agent_id - uint8 name[16] but the current implementation doesn't take into account the agent_id field and only allocates a rx buffer of SCMI_MAX_STR_SIZE length Allocate the correct length for rx buffer and copy the name from the correct offset in the response. While no error were returned until v5.15, v5.16-rc1 fails with virtio_scmi transport channel: [ 1.093253] arm-scmi firmware:scmi0: SCMI Notifications - Core Enabled. [ 1.114776] arm-scmi firmware:scmi0: SCMI Protocol v2.0 'Linaro:PMWG' Firmware version 0x2090000 [ 1.117544] scmi-virtio virtio0: tx:used len 28 is larger than in buflen 24 Signed-off-by: Vincent Guittot <vincent.guittot@linaro.org> --- drivers/firmware/arm_scmi/base.c | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-)