| Message ID | 20241007060642.1978049-2-quic_sibis@quicinc.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | firmware: arm_scmi: Misc Fixes | expand |
On Mon, Oct 07, 2024 at 11:36:39AM +0530, Sibi Sankar wrote: > Currently the perf and powercap protocol relies on the protocol domain > attributes, which just ensures that one fastchannel per domain, before > instantiating fastchannels for all possible message-ids. Fix this by > ensuring that each message-id supports fastchannel before initialization. > Looks good to me. With the minor nit below addressed, Reviewed-by: Sudeep Holla <sudeep.holla@arm.com> (assuming you will take this all via pmdomain or qcom soc tree) > Reported-by: Johan Hovold <johan+linaro@kernel.org> > Closes: https://lore.kernel.org/lkml/ZoQjAWse2YxwyRJv@hovoldconsulting.com/ > Fixes: 6f9ea4dabd2d ("firmware: arm_scmi: Generalize the fast channel support") > Signed-off-by: Sibi Sankar <quic_sibis@quicinc.com> > --- > drivers/firmware/arm_scmi/driver.c | 9 +++++++++ > drivers/firmware/arm_scmi/protocols.h | 2 ++ > 2 files changed, 11 insertions(+) > > diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c > index 88c5c4ff4bb6..80a9a615672a 100644 > --- a/drivers/firmware/arm_scmi/driver.c > +++ b/drivers/firmware/arm_scmi/driver.c > @@ -56,6 +56,9 @@ static atomic_t transfer_last_id; > > static struct dentry *scmi_top_dentry; > > +static int scmi_protocol_msg_check(const struct scmi_protocol_handle *ph, > + u32 message_id, u32 *attributes); > I prefer to just move the function above if possible to avoid this extra declaration just keep keep it consistent with other such internal/static function calls within this file. No hard opinion, just preference to avoid me thinking(or scratching my head) why only this is done different few months down the line. -- Regards, Sudeep
On Mon, Oct 07, 2024 at 11:36:39AM +0530, Sibi Sankar wrote: > Currently the perf and powercap protocol relies on the protocol domain > attributes, which just ensures that one fastchannel per domain, before > instantiating fastchannels for all possible message-ids. Fix this by > ensuring that each message-id supports fastchannel before initialization. Perhaps you could include the error message I reported here so that anyone searching for that error will find this fix more easily. > Reported-by: Johan Hovold <johan+linaro@kernel.org> > Closes: https://lore.kernel.org/lkml/ZoQjAWse2YxwyRJv@hovoldconsulting.com/ > Fixes: 6f9ea4dabd2d ("firmware: arm_scmi: Generalize the fast channel support") > Signed-off-by: Sibi Sankar <quic_sibis@quicinc.com> Do we want this to be backported as well so that you should add a CC-stable tag? Either way, this does seem to address the FC errors I reported: Tested-by: Johan Hovold <johan+linaro@kernel.org> Johan
diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c index 88c5c4ff4bb6..80a9a615672a 100644 --- a/drivers/firmware/arm_scmi/driver.c +++ b/drivers/firmware/arm_scmi/driver.c @@ -56,6 +56,9 @@ static atomic_t transfer_last_id; static struct dentry *scmi_top_dentry; +static int scmi_protocol_msg_check(const struct scmi_protocol_handle *ph, + u32 message_id, u32 *attributes); + /** * struct scmi_xfers_info - Structure to manage transfer information * @@ -1841,6 +1844,7 @@ scmi_common_fastchannel_init(const struct scmi_protocol_handle *ph, int ret; u32 flags; u64 phys_addr; + u32 attributes; u8 size; void __iomem *addr; struct scmi_xfer *t; @@ -1849,6 +1853,11 @@ scmi_common_fastchannel_init(const struct scmi_protocol_handle *ph, struct scmi_msg_resp_desc_fc *resp; const struct scmi_protocol_instance *pi = ph_to_pi(ph); + /* Check if the MSG_ID supports fastchannel */ + ret = scmi_protocol_msg_check(ph, message_id, &attributes); + if (!ret && !MSG_SUPPORTS_FASTCHANNEL(attributes)) + return; + if (!p_addr) { ret = -EINVAL; goto err_out; diff --git a/drivers/firmware/arm_scmi/protocols.h b/drivers/firmware/arm_scmi/protocols.h index aaee57cdcd55..d62c4469d1fd 100644 --- a/drivers/firmware/arm_scmi/protocols.h +++ b/drivers/firmware/arm_scmi/protocols.h @@ -31,6 +31,8 @@ #define SCMI_PROTOCOL_VENDOR_BASE 0x80 +#define MSG_SUPPORTS_FASTCHANNEL(x) ((x) & BIT(0)) + enum scmi_common_cmd { PROTOCOL_VERSION = 0x0, PROTOCOL_ATTRIBUTES = 0x1,
Currently the perf and powercap protocol relies on the protocol domain attributes, which just ensures that one fastchannel per domain, before instantiating fastchannels for all possible message-ids. Fix this by ensuring that each message-id supports fastchannel before initialization. Reported-by: Johan Hovold <johan+linaro@kernel.org> Closes: https://lore.kernel.org/lkml/ZoQjAWse2YxwyRJv@hovoldconsulting.com/ Fixes: 6f9ea4dabd2d ("firmware: arm_scmi: Generalize the fast channel support") Signed-off-by: Sibi Sankar <quic_sibis@quicinc.com> --- drivers/firmware/arm_scmi/driver.c | 9 +++++++++ drivers/firmware/arm_scmi/protocols.h | 2 ++ 2 files changed, 11 insertions(+)