| Message ID | 20230731041013.2950307-2-quic_bjorande@quicinc.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | soc: qcom: aoss: Introduce debugfs interface and cleanup things | expand |
On Sun, Jul 30, 2023 at 09:10:10PM -0700, Bjorn Andersson wrote: > The existing implementation of qmp_send() requires the caller to provide > a buffer which is of word-aligned. The underlying reason for this is > that message ram only supports word accesses, but pushing this > requirement onto the clients results in the same boiler plate code > sprinkled in every call site. > > By using a temporary buffer in qmp_send() we can hide the underlying > hardware limitations from the clients and allow them to pass their > NUL-terminates C string directly. > > Signed-off-by: Bjorn Andersson <quic_bjorande@quicinc.com> > --- > drivers/net/ipa/ipa_power.c | 2 +- > drivers/remoteproc/qcom_q6v5.c | 2 +- > drivers/soc/qcom/qcom_aoss.c | 25 ++++++++++++------------- > include/linux/soc/qcom/qcom_aoss.h | 4 ++-- > 4 files changed, 16 insertions(+), 17 deletions(-) > > diff --git a/drivers/net/ipa/ipa_power.c b/drivers/net/ipa/ipa_power.c > index 921eecf3eff6..26181eeed975 100644 > --- a/drivers/net/ipa/ipa_power.c > +++ b/drivers/net/ipa/ipa_power.c > @@ -332,7 +332,7 @@ void ipa_power_retention(struct ipa *ipa, bool enable) > > (void)snprintf(buf, sizeof(buf), fmt, enable ? '1' : '0'); > > - ret = qmp_send(power->qmp, buf, sizeof(buf)); > + ret = qmp_send(power->qmp, buf); > if (ret) > dev_err(power->dev, "error %d sending QMP %sable request\n", > ret, enable ? "en" : "dis"); > diff --git a/drivers/remoteproc/qcom_q6v5.c b/drivers/remoteproc/qcom_q6v5.c > index 192c7aa0e39e..8b41a73fa4d1 100644 > --- a/drivers/remoteproc/qcom_q6v5.c > +++ b/drivers/remoteproc/qcom_q6v5.c > @@ -35,7 +35,7 @@ static int q6v5_load_state_toggle(struct qcom_q6v5 *q6v5, bool enable) > > WARN_ON(ret >= Q6V5_LOAD_STATE_MSG_LEN); > > - ret = qmp_send(q6v5->qmp, buf, sizeof(buf)); > + ret = qmp_send(q6v5->qmp, buf); > if (ret) > dev_err(q6v5->dev, "failed to toggle load state\n"); > > diff --git a/drivers/soc/qcom/qcom_aoss.c b/drivers/soc/qcom/qcom_aoss.c > index e376c32cc16e..5e74332515cf 100644 > --- a/drivers/soc/qcom/qcom_aoss.c > +++ b/drivers/soc/qcom/qcom_aoss.c > @@ -206,36 +206,35 @@ static bool qmp_message_empty(struct qmp *qmp) > * qmp_send() - send a message to the AOSS > * @qmp: qmp context > * @data: message to be sent > - * @len: length of the message > * > * Transmit @data to AOSS and wait for the AOSS to acknowledge the message. > - * @len must be a multiple of 4 and not longer than the mailbox size. Access is > - * synchronized by this implementation. > + * data must not be longer than the mailbox size. Access is synchronized by > + * this implementation. > * > * Return: 0 on success, negative errno on failure > */ > -int qmp_send(struct qmp *qmp, const void *data, size_t len) > +int qmp_send(struct qmp *qmp, const void *data) > { > long time_left; > + char buf[QMP_MSG_LEN]; > int ret; Hi Bjorn, please consider preserving reverse xmas tree - longest line to shortest - for local variable declarations in this Networking code. char buf[QMP_MSG_LEN]; long time_left; int ret; ...
On 7/30/2023 9:10 PM, Bjorn Andersson wrote: > diff --git a/drivers/soc/qcom/qcom_aoss.c b/drivers/soc/qcom/qcom_aoss.c > /* The message RAM only implements 32-bit accesses */ > __iowrite32_copy(qmp->msgram + qmp->offset + sizeof(u32), > - data, len / sizeof(u32)); > - writel(len, qmp->msgram + qmp->offset); > + buf, sizeof(buf) / sizeof(u32)); > + writel(sizeof(buf), qmp->msgram + qmp->offset); > Looks like we are telling the firmware the packet size will always be QMP_MSG_LEN? This should be ok but might be a problem when debugging. The AOSS firmware only logs size of the message instead of the full string because of memory constraints. We would normally match the firmware and host logs based on size, but won't be able to differentiate this way with a fixed size.
On Mon, Jul 31, 2023 at 02:29:44PM -0700, Chris Lew wrote: > > > On 7/30/2023 9:10 PM, Bjorn Andersson wrote: > > diff --git a/drivers/soc/qcom/qcom_aoss.c b/drivers/soc/qcom/qcom_aoss.c > > /* The message RAM only implements 32-bit accesses */ > > __iowrite32_copy(qmp->msgram + qmp->offset + sizeof(u32), > > - data, len / sizeof(u32)); > > - writel(len, qmp->msgram + qmp->offset); > > + buf, sizeof(buf) / sizeof(u32)); > > + writel(sizeof(buf), qmp->msgram + qmp->offset); > > Looks like we are telling the firmware the packet size will always be > QMP_MSG_LEN? > > This should be ok but might be a problem when debugging. The AOSS firmware > only logs size of the message instead of the full string because of memory > constraints. > Until now ipa_power_retention() has been passing 36 here, everyone else 64, so it is ok. > We would normally match the firmware and host logs based on size, but won't > be able to differentiate this way with a fixed size. I don't mind us changing it to ALIGN(len, 4), but as that would change the current behavior I'd like to do so in a subsequent patch. Speaking of behavior, is 64 the max message size? We inherited the 64 from the initial downstream implementation, but qmp->size is quite a bit bigger. Regards, Bjorn
On 7/31/2023 4:10 PM, Bjorn Andersson wrote: > On Mon, Jul 31, 2023 at 02:29:44PM -0700, Chris Lew wrote: >> >> >> On 7/30/2023 9:10 PM, Bjorn Andersson wrote: >>> diff --git a/drivers/soc/qcom/qcom_aoss.c b/drivers/soc/qcom/qcom_aoss.c >>> /* The message RAM only implements 32-bit accesses */ >>> __iowrite32_copy(qmp->msgram + qmp->offset + sizeof(u32), >>> - data, len / sizeof(u32)); >>> - writel(len, qmp->msgram + qmp->offset); >>> + buf, sizeof(buf) / sizeof(u32)); >>> + writel(sizeof(buf), qmp->msgram + qmp->offset); >> >> Looks like we are telling the firmware the packet size will always be >> QMP_MSG_LEN? >> >> This should be ok but might be a problem when debugging. The AOSS firmware >> only logs size of the message instead of the full string because of memory >> constraints. >> > > Until now ipa_power_retention() has been passing 36 here, everyone else > 64, so it is ok. > >> We would normally match the firmware and host logs based on size, but won't >> be able to differentiate this way with a fixed size. > > I don't mind us changing it to ALIGN(len, 4), but as that would change > the current behavior I'd like to do so in a subsequent patch. > > Speaking of behavior, is 64 the max message size? We inherited the 64 > from the initial downstream implementation, but qmp->size is quite a bit > bigger. > The max message size the firmware can handle is 0x64, so 100 bytes, but I haven't seen any messages go above 64 bytes. > Regards, > Bjorn
diff --git a/drivers/net/ipa/ipa_power.c b/drivers/net/ipa/ipa_power.c index 921eecf3eff6..26181eeed975 100644 --- a/drivers/net/ipa/ipa_power.c +++ b/drivers/net/ipa/ipa_power.c @@ -332,7 +332,7 @@ void ipa_power_retention(struct ipa *ipa, bool enable) (void)snprintf(buf, sizeof(buf), fmt, enable ? '1' : '0'); - ret = qmp_send(power->qmp, buf, sizeof(buf)); + ret = qmp_send(power->qmp, buf); if (ret) dev_err(power->dev, "error %d sending QMP %sable request\n", ret, enable ? "en" : "dis"); diff --git a/drivers/remoteproc/qcom_q6v5.c b/drivers/remoteproc/qcom_q6v5.c index 192c7aa0e39e..8b41a73fa4d1 100644 --- a/drivers/remoteproc/qcom_q6v5.c +++ b/drivers/remoteproc/qcom_q6v5.c @@ -35,7 +35,7 @@ static int q6v5_load_state_toggle(struct qcom_q6v5 *q6v5, bool enable) WARN_ON(ret >= Q6V5_LOAD_STATE_MSG_LEN); - ret = qmp_send(q6v5->qmp, buf, sizeof(buf)); + ret = qmp_send(q6v5->qmp, buf); if (ret) dev_err(q6v5->dev, "failed to toggle load state\n"); diff --git a/drivers/soc/qcom/qcom_aoss.c b/drivers/soc/qcom/qcom_aoss.c index e376c32cc16e..5e74332515cf 100644 --- a/drivers/soc/qcom/qcom_aoss.c +++ b/drivers/soc/qcom/qcom_aoss.c @@ -206,36 +206,35 @@ static bool qmp_message_empty(struct qmp *qmp) * qmp_send() - send a message to the AOSS * @qmp: qmp context * @data: message to be sent - * @len: length of the message * * Transmit @data to AOSS and wait for the AOSS to acknowledge the message. - * @len must be a multiple of 4 and not longer than the mailbox size. Access is - * synchronized by this implementation. + * data must not be longer than the mailbox size. Access is synchronized by + * this implementation. * * Return: 0 on success, negative errno on failure */ -int qmp_send(struct qmp *qmp, const void *data, size_t len) +int qmp_send(struct qmp *qmp, const void *data) { long time_left; + char buf[QMP_MSG_LEN]; int ret; if (WARN_ON(IS_ERR_OR_NULL(qmp) || !data)) return -EINVAL; - if (WARN_ON(len + sizeof(u32) > qmp->size)) + if (WARN_ON(strlen(data) >= sizeof(buf))) return -EINVAL; - if (WARN_ON(len % sizeof(u32))) - return -EINVAL; + strscpy_pad(buf, data, sizeof(buf)); mutex_lock(&qmp->tx_lock); /* The message RAM only implements 32-bit accesses */ __iowrite32_copy(qmp->msgram + qmp->offset + sizeof(u32), - data, len / sizeof(u32)); - writel(len, qmp->msgram + qmp->offset); + buf, sizeof(buf) / sizeof(u32)); + writel(sizeof(buf), qmp->msgram + qmp->offset); - /* Read back len to confirm data written in message RAM */ + /* Read back length to confirm data written in message RAM */ readl(qmp->msgram + qmp->offset); qmp_kick(qmp); @@ -262,7 +261,7 @@ static int qmp_qdss_clk_prepare(struct clk_hw *hw) static const char buf[QMP_MSG_LEN] = "{class: clock, res: qdss, val: 1}"; struct qmp *qmp = container_of(hw, struct qmp, qdss_clk); - return qmp_send(qmp, buf, sizeof(buf)); + return qmp_send(qmp, buf); } static void qmp_qdss_clk_unprepare(struct clk_hw *hw) @@ -270,7 +269,7 @@ static void qmp_qdss_clk_unprepare(struct clk_hw *hw) static const char buf[QMP_MSG_LEN] = "{class: clock, res: qdss, val: 0}"; struct qmp *qmp = container_of(hw, struct qmp, qdss_clk); - qmp_send(qmp, buf, sizeof(buf)); + qmp_send(qmp, buf); } static const struct clk_ops qmp_qdss_clk_ops = { @@ -344,7 +343,7 @@ static int qmp_cdev_set_cur_state(struct thermal_cooling_device *cdev, qmp_cdev->name, cdev_state ? "on" : "off"); - ret = qmp_send(qmp_cdev->qmp, buf, sizeof(buf)); + ret = qmp_send(qmp_cdev->qmp, buf); if (!ret) qmp_cdev->state = cdev_state; diff --git a/include/linux/soc/qcom/qcom_aoss.h b/include/linux/soc/qcom/qcom_aoss.h index 3c2a82e606f8..7a71406b6050 100644 --- a/include/linux/soc/qcom/qcom_aoss.h +++ b/include/linux/soc/qcom/qcom_aoss.h @@ -13,13 +13,13 @@ struct qmp; #if IS_ENABLED(CONFIG_QCOM_AOSS_QMP) -int qmp_send(struct qmp *qmp, const void *data, size_t len); +int qmp_send(struct qmp *qmp, const void *data); struct qmp *qmp_get(struct device *dev); void qmp_put(struct qmp *qmp); #else -static inline int qmp_send(struct qmp *qmp, const void *data, size_t len) +static inline int qmp_send(struct qmp *qmp, const void *data) { return -ENODEV; }
The existing implementation of qmp_send() requires the caller to provide a buffer which is of word-aligned. The underlying reason for this is that message ram only supports word accesses, but pushing this requirement onto the clients results in the same boiler plate code sprinkled in every call site. By using a temporary buffer in qmp_send() we can hide the underlying hardware limitations from the clients and allow them to pass their NUL-terminates C string directly. Signed-off-by: Bjorn Andersson <quic_bjorande@quicinc.com> --- drivers/net/ipa/ipa_power.c | 2 +- drivers/remoteproc/qcom_q6v5.c | 2 +- drivers/soc/qcom/qcom_aoss.c | 25 ++++++++++++------------- include/linux/soc/qcom/qcom_aoss.h | 4 ++-- 4 files changed, 16 insertions(+), 17 deletions(-)