| Message ID | 20250402104254.149998-1-dev.mbstr@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | firmware: arm_scmi: add timeout in do_xfer_with_response() | expand |
On Wed, Apr 02, 2025 at 01:42:54PM +0300, Matthew Bystrin wrote: > Add timeout argument to do_xfer_with_response() with subsequent changes > in corresponding drivers. To maintain backward compatibility use > previous hardcoded timeout value. > > According to SCMI specification [1] there is no defined timeout for > delayed messages in the interface. While hardcoded 2 seconds timeout > might be good enough for existing protocol drivers, moving it to the > function argument may be useful for vendor-specific protocols with > different timing needs. > Please post this patch along with the vendor specific protocols mentioned above and with the reasoning as why 2s is not sufficient. Also instead of churning up existing users/usage, we can explore to had one with this timeout as alternative if you present and convince the validity of your use-case and the associated timing requirement.
On Wed, Apr 02, 2025 at 11:59:47AM +0100, Sudeep Holla wrote: > On Wed, Apr 02, 2025 at 01:42:54PM +0300, Matthew Bystrin wrote: > > Add timeout argument to do_xfer_with_response() with subsequent changes > > in corresponding drivers. To maintain backward compatibility use > > previous hardcoded timeout value. > > Hi Matthew, Sudeep, this is something I had my eyes on since a while and never get back to it....so thanks for looking at this first of all... > > According to SCMI specification [1] there is no defined timeout for > > delayed messages in the interface. While hardcoded 2 seconds timeout > > might be good enough for existing protocol drivers, moving it to the > > function argument may be useful for vendor-specific protocols with > > different timing needs. > > > > Please post this patch along with the vendor specific protocols mentioned > above and with the reasoning as why 2s is not sufficient. Ack on this, it would be good to understand why a huge 2 secs is not enough...and also... > > Also instead of churning up existing users/usage, we can explore to had > one with this timeout as alternative if you present and convince the > validity of your use-case and the associated timing requirement. > ...with the proposed patch (and any kind of alternative API proposed by Sudeep) the delayed response timeout becomes a parameter of the method do_xfer_with_response() and so, as a consequence, this timoeut becomes effectively configurable per-transaction, while usually a timeout is commonly configurable per-channel, so valid as a whole for any protocol on that channel across the whole platform, AND optionally describable as different from the default standard value via DT props (like max-rx-timeout). Is this what we want ? (a per-transaction configurable timeout ?) If not, it could be an option to make instead this a per-channel optional new DT described property so that you can configure globally a different delayed timeout. If yes, how this new parameter is meant to be used/configured/chosen ? on a per-protocol/command basis, unrelated to the specific platform we run on ? Thanks, Cristian
On Wed, Apr 02, 2025 at 05:05:55PM +0100, Cristian Marussi wrote: > On Wed, Apr 02, 2025 at 11:59:47AM +0100, Sudeep Holla wrote: > > On Wed, Apr 02, 2025 at 01:42:54PM +0300, Matthew Bystrin wrote: > > > Add timeout argument to do_xfer_with_response() with subsequent changes > > > in corresponding drivers. To maintain backward compatibility use > > > previous hardcoded timeout value. > > > > > Hi Matthew, Sudeep, > > this is something I had my eyes on since a while and never get back to > it....so thanks for looking at this first of all... > > > > According to SCMI specification [1] there is no defined timeout for > > > delayed messages in the interface. While hardcoded 2 seconds timeout > > > might be good enough for existing protocol drivers, moving it to the > > > function argument may be useful for vendor-specific protocols with > > > different timing needs. > > > > > > > Please post this patch along with the vendor specific protocols mentioned > > above and with the reasoning as why 2s is not sufficient. > > Ack on this, it would be good to understand why a huge 2 secs is not > enough...and also... > > > > > Also instead of churning up existing users/usage, we can explore to had > > one with this timeout as alternative if you present and convince the > > validity of your use-case and the associated timing requirement. > > > > ...with the proposed patch (and any kind of alternative API proposed > by Sudeep) the delayed response timeout becomes a parameter of the method > do_xfer_with_response() and so, as a consequence, this timoeut becomes > effectively configurable per-transaction, while usually a timeout is > commonly configurable per-channel, so valid as a whole for any protocol > on that channel across the whole platform, AND optionally describable as > different from the default standard value via DT props (like max-rx-timeout). > > Is this what we want ? (a per-transaction configurable timeout ?) > > If not, it could be an option to make instead this a per-channel optional > new DT described property so that you can configure globally a different > delayed timeout. > > If yes, how this new parameter is meant to be used/configured/chosen ? > on a per-protocol/command basis, unrelated to the specific platform we run on ? > +1 on all the points above. I agree this must be per transport. If it is per message then you need to think why it needs to sync command. Why can't it be changed to async or use notification. I am waiting to see the usage in your vendor protocols to understand it in more detail.
diff --git a/drivers/firmware/arm_scmi/clock.c b/drivers/firmware/arm_scmi/clock.c index 2ed2279388f0..4b5cd73384c3 100644 --- a/drivers/firmware/arm_scmi/clock.c +++ b/drivers/firmware/arm_scmi/clock.c @@ -596,7 +596,7 @@ static int scmi_clock_rate_set(const struct scmi_protocol_handle *ph, cfg->value_high = cpu_to_le32(rate >> 32); if (flags & CLOCK_SET_ASYNC) { - ret = ph->xops->do_xfer_with_response(ph, t); + ret = ph->xops->do_xfer_with_response(ph, t, SCMI_DEFAULT_TIMEOUT); if (!ret) { struct scmi_msg_resp_set_rate_complete *resp; diff --git a/drivers/firmware/arm_scmi/common.h b/drivers/firmware/arm_scmi/common.h index 10ea7962323e..34527366c909 100644 --- a/drivers/firmware/arm_scmi/common.h +++ b/drivers/firmware/arm_scmi/common.h @@ -29,7 +29,6 @@ #define SCMI_MAX_CHANNELS 256 -#define SCMI_MAX_RESPONSE_TIMEOUT (2 * MSEC_PER_SEC) #define SCMI_SHMEM_MAX_PAYLOAD_SIZE 104 diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c index 1c75a4c9c371..51c6634d5505 100644 --- a/drivers/firmware/arm_scmi/driver.c +++ b/drivers/firmware/arm_scmi/driver.c @@ -1490,6 +1490,7 @@ static void reset_rx_to_maxsz(const struct scmi_protocol_handle *ph, * * @ph: Pointer to SCMI protocol handle * @xfer: Transfer to initiate and wait for response + * @timeout_ms: Delayed response wait timeout, if unsure use SCMI_DEFAULT_TIMEOUT * * Using asynchronous commands in atomic/polling mode should be avoided since * it could cause long busy-waiting here, so ignore polling for the delayed @@ -1509,9 +1510,10 @@ static void reset_rx_to_maxsz(const struct scmi_protocol_handle *ph, * return corresponding error, else if all goes well, return 0. */ static int do_xfer_with_response(const struct scmi_protocol_handle *ph, - struct scmi_xfer *xfer) + struct scmi_xfer *xfer, unsigned long timeout_ms) { - int ret, timeout = msecs_to_jiffies(SCMI_MAX_RESPONSE_TIMEOUT); + int ret; + unsigned long timeout = msecs_to_jiffies(timeout_ms); DECLARE_COMPLETION_ONSTACK(async_response); xfer->async_done = &async_response; diff --git a/drivers/firmware/arm_scmi/powercap.c b/drivers/firmware/arm_scmi/powercap.c index 1fa79bba492e..82565dacd301 100644 --- a/drivers/firmware/arm_scmi/powercap.c +++ b/drivers/firmware/arm_scmi/powercap.c @@ -386,7 +386,7 @@ static int scmi_powercap_xfer_cap_set(const struct scmi_protocol_handle *ph, if (!pc->async_powercap_cap_set || ignore_dresp) { ret = ph->xops->do_xfer(ph, t); } else { - ret = ph->xops->do_xfer_with_response(ph, t); + ret = ph->xops->do_xfer_with_response(ph, t, SCMI_DEFAULT_TIMEOUT); if (!ret) { struct scmi_msg_resp_powercap_cap_set_complete *resp; diff --git a/drivers/firmware/arm_scmi/protocols.h b/drivers/firmware/arm_scmi/protocols.h index aaee57cdcd55..b1e3cf3601fe 100644 --- a/drivers/firmware/arm_scmi/protocols.h +++ b/drivers/firmware/arm_scmi/protocols.h @@ -307,7 +307,8 @@ struct scmi_xfer_ops { int (*do_xfer)(const struct scmi_protocol_handle *ph, struct scmi_xfer *xfer); int (*do_xfer_with_response)(const struct scmi_protocol_handle *ph, - struct scmi_xfer *xfer); + struct scmi_xfer *xfer, + unsigned long timeout_ms); void (*xfer_put)(const struct scmi_protocol_handle *ph, struct scmi_xfer *xfer); }; diff --git a/drivers/firmware/arm_scmi/reset.c b/drivers/firmware/arm_scmi/reset.c index 0aa82b96f41b..a458b1c16c51 100644 --- a/drivers/firmware/arm_scmi/reset.c +++ b/drivers/firmware/arm_scmi/reset.c @@ -198,7 +198,7 @@ static int scmi_domain_reset(const struct scmi_protocol_handle *ph, u32 domain, dom->reset_state = cpu_to_le32(state); if (flags & ASYNCHRONOUS_RESET) - ret = ph->xops->do_xfer_with_response(ph, t); + ret = ph->xops->do_xfer_with_response(ph, t, SCMI_DEFAULT_TIMEOUT); else ret = ph->xops->do_xfer(ph, t); diff --git a/drivers/firmware/arm_scmi/sensors.c b/drivers/firmware/arm_scmi/sensors.c index 791efd0f82d7..9a07399237f5 100644 --- a/drivers/firmware/arm_scmi/sensors.c +++ b/drivers/firmware/arm_scmi/sensors.c @@ -871,7 +871,7 @@ static int scmi_sensor_reading_get(const struct scmi_protocol_handle *ph, s = si->sensors + sensor_id; if (s->async) { sensor->flags = cpu_to_le32(SENSOR_READ_ASYNC); - ret = ph->xops->do_xfer_with_response(ph, t); + ret = ph->xops->do_xfer_with_response(ph, t, SCMI_DEFAULT_TIMEOUT); if (!ret) { struct scmi_resp_sensor_reading_complete *resp; @@ -943,7 +943,7 @@ scmi_sensor_reading_get_timestamped(const struct scmi_protocol_handle *ph, sensor->id = cpu_to_le32(sensor_id); if (s->async) { sensor->flags = cpu_to_le32(SENSOR_READ_ASYNC); - ret = ph->xops->do_xfer_with_response(ph, t); + ret = ph->xops->do_xfer_with_response(ph, t, SCMI_DEFAULT_TIMEOUT); if (!ret) { int i; struct scmi_resp_sensor_reading_complete_v3 *resp; diff --git a/drivers/firmware/arm_scmi/voltage.c b/drivers/firmware/arm_scmi/voltage.c index fda6a1573609..26b1f034256b 100644 --- a/drivers/firmware/arm_scmi/voltage.c +++ b/drivers/firmware/arm_scmi/voltage.c @@ -348,7 +348,7 @@ static int scmi_voltage_level_set(const struct scmi_protocol_handle *ph, ret = ph->xops->do_xfer(ph, t); } else { cmd->flags = cpu_to_le32(0x1); - ret = ph->xops->do_xfer_with_response(ph, t); + ret = ph->xops->do_xfer_with_response(ph, t, SCMI_DEFAULT_TIMEOUT); if (!ret) { struct scmi_resp_voltage_level_set_complete *resp; diff --git a/include/linux/scmi_protocol.h b/include/linux/scmi_protocol.h index 688466a0e816..2426daae8a87 100644 --- a/include/linux/scmi_protocol.h +++ b/include/linux/scmi_protocol.h @@ -16,6 +16,7 @@ #define SCMI_MAX_STR_SIZE 64 #define SCMI_SHORT_NAME_MAX_SIZE 16 #define SCMI_MAX_NUM_RATES 16 +#define SCMI_DEFAULT_TIMEOUT (2 * MSEC_PER_SEC) /** * struct scmi_revision_info - version information structure
Add timeout argument to do_xfer_with_response() with subsequent changes in corresponding drivers. To maintain backward compatibility use previous hardcoded timeout value. According to SCMI specification [1] there is no defined timeout for delayed messages in the interface. While hardcoded 2 seconds timeout might be good enough for existing protocol drivers, moving it to the function argument may be useful for vendor-specific protocols with different timing needs. Link: https://developer.arm.com/Architectures/System%20Control%20and%20Management%20Interface Signed-off-by: Matthew Bystrin <dev.mbstr@gmail.com> --- drivers/firmware/arm_scmi/clock.c | 2 +- drivers/firmware/arm_scmi/common.h | 1 - drivers/firmware/arm_scmi/driver.c | 6 ++++-- drivers/firmware/arm_scmi/powercap.c | 2 +- drivers/firmware/arm_scmi/protocols.h | 3 ++- drivers/firmware/arm_scmi/reset.c | 2 +- drivers/firmware/arm_scmi/sensors.c | 4 ++-- drivers/firmware/arm_scmi/voltage.c | 2 +- include/linux/scmi_protocol.h | 1 + 9 files changed, 13 insertions(+), 10 deletions(-)