Hi,
a bunch of small SCMI updates based on v6.9-rc1.
Mainly adding traces for weird SCMI messages like late timed-out replies,
out of order unexpected messages and malformed messages due to spurious
mbox IRQs.
Thanks,
Cristian
V2:
- rebased on v6.9-rc1
- added a common trace helper for weird messages
- added traces for spurious MBOX IRQs
Cristian Marussi (5):
include: trace: Widen the tag buffer in trace_scmi_dump_msg
firmware: arm_scmi: Add helper to trace bad messages
firmware: arm_scmi: Add message dump traces for bad and unexpected
replies
firmware: arm_scmi: Simplify scmi_devm_notifier_unregister
firmware: arm_scmi: Use dev_err_probe to bail out
drivers/firmware/arm_scmi/common.h | 11 ++++
drivers/firmware/arm_scmi/driver.c | 83 ++++++++++++++++++++++++++---
drivers/firmware/arm_scmi/mailbox.c | 4 +-
drivers/firmware/arm_scmi/notify.c | 30 ++---------
include/linux/scmi_protocol.h | 2 -
include/trace/events/scmi.h | 6 ++-
6 files changed, 97 insertions(+), 39 deletions(-)
--
2.44.0
A bigger buffer allow for more diverse tag names.
Signed-off-by: Cristian Marussi <[email protected]>
---
include/trace/events/scmi.h | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/include/trace/events/scmi.h b/include/trace/events/scmi.h
index 422c1ad9484d..127300481123 100644
--- a/include/trace/events/scmi.h
+++ b/include/trace/events/scmi.h
@@ -7,6 +7,8 @@
#include <linux/tracepoint.h>
+#define TRACE_SCMI_MAX_TAG_LEN 6
+
TRACE_EVENT(scmi_fc_call,
TP_PROTO(u8 protocol_id, u8 msg_id, u32 res_id, u32 val1, u32 val2),
TP_ARGS(protocol_id, msg_id, res_id, val1, val2),
@@ -150,7 +152,7 @@ TRACE_EVENT(scmi_msg_dump,
__field(u8, channel_id)
__field(u8, protocol_id)
__field(u8, msg_id)
- __array(char, tag, 5)
+ __array(char, tag, TRACE_SCMI_MAX_TAG_LEN)
__field(u16, seq)
__field(int, status)
__field(size_t, len)
@@ -162,7 +164,7 @@ TRACE_EVENT(scmi_msg_dump,
__entry->channel_id = channel_id;
__entry->protocol_id = protocol_id;
__entry->msg_id = msg_id;
- strscpy(__entry->tag, tag, 5);
+ strscpy(__entry->tag, tag, TRACE_SCMI_MAX_TAG_LEN);
__entry->seq = seq;
__entry->status = status;
__entry->len = len;
--
2.44.0
Unregistering SCMI notifications using the managed devres interface can be
done providing as a reference simply the previously successfully registered
notification block since it could have been registered only on one kernel
notification_chain: drop any reference to SCMI protocol, events and
sources.
Devres internal helpers can search for the provided notification block
reference and, once found, the associated devres object will already
provide the above SCMI references for the event.
Signed-off-by: Cristian Marussi <[email protected]>
---
drivers/firmware/arm_scmi/notify.c | 30 ++++--------------------------
include/linux/scmi_protocol.h | 2 --
2 files changed, 4 insertions(+), 28 deletions(-)
diff --git a/drivers/firmware/arm_scmi/notify.c b/drivers/firmware/arm_scmi/notify.c
index 27c52531194d..e160ecb22948 100644
--- a/drivers/firmware/arm_scmi/notify.c
+++ b/drivers/firmware/arm_scmi/notify.c
@@ -1513,17 +1513,12 @@ static int scmi_devm_notifier_register(struct scmi_device *sdev,
static int scmi_devm_notifier_match(struct device *dev, void *res, void *data)
{
struct scmi_notifier_devres *dres = res;
- struct scmi_notifier_devres *xres = data;
+ struct notifier_block *nb = data;
- if (WARN_ON(!dres || !xres))
+ if (WARN_ON(!dres || !nb))
return 0;
- return dres->proto_id == xres->proto_id &&
- dres->evt_id == xres->evt_id &&
- dres->nb == xres->nb &&
- ((!dres->src_id && !xres->src_id) ||
- (dres->src_id && xres->src_id &&
- dres->__src_id == xres->__src_id));
+ return dres->nb == nb;
}
/**
@@ -1531,10 +1526,6 @@ static int scmi_devm_notifier_match(struct device *dev, void *res, void *data)
* notifier_block for an event
* @sdev: A reference to an scmi_device whose embedded struct device is to
* be used for devres accounting.
- * @proto_id: Protocol ID
- * @evt_id: Event ID
- * @src_id: Source ID, when NULL register for events coming form ALL possible
- * sources
* @nb: A standard notifier block to register for the specified event
*
* Generic devres managed helper to explicitly un-register a notifier_block
@@ -1544,25 +1535,12 @@ static int scmi_devm_notifier_match(struct device *dev, void *res, void *data)
* Return: 0 on Success
*/
static int scmi_devm_notifier_unregister(struct scmi_device *sdev,
- u8 proto_id, u8 evt_id,
- const u32 *src_id,
struct notifier_block *nb)
{
int ret;
- struct scmi_notifier_devres dres;
-
- dres.handle = sdev->handle;
- dres.proto_id = proto_id;
- dres.evt_id = evt_id;
- if (src_id) {
- dres.__src_id = *src_id;
- dres.src_id = &dres.__src_id;
- } else {
- dres.src_id = NULL;
- }
ret = devres_release(&sdev->dev, scmi_devm_release_notifier,
- scmi_devm_notifier_match, &dres);
+ scmi_devm_notifier_match, nb);
WARN_ON(ret);
diff --git a/include/linux/scmi_protocol.h b/include/linux/scmi_protocol.h
index b807141acc14..a3addb07e00a 100644
--- a/include/linux/scmi_protocol.h
+++ b/include/linux/scmi_protocol.h
@@ -783,8 +783,6 @@ struct scmi_notify_ops {
const u32 *src_id,
struct notifier_block *nb);
int (*devm_event_notifier_unregister)(struct scmi_device *sdev,
- u8 proto_id, u8 evt_id,
- const u32 *src_id,
struct notifier_block *nb);
int (*event_notifier_register)(const struct scmi_handle *handle,
u8 proto_id, u8 evt_id,
--
2.44.0
Trace also late-timed-out, out-of-order and unexpected/spurious messages.
Signed-off-by: Cristian Marussi <[email protected]>
---
drivers/firmware/arm_scmi/driver.c | 10 ++++++++++
drivers/firmware/arm_scmi/mailbox.c | 4 +++-
2 files changed, 13 insertions(+), 1 deletion(-)
diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c
index 7fc1c5b1a2a4..207ed1a52d69 100644
--- a/drivers/firmware/arm_scmi/driver.c
+++ b/drivers/firmware/arm_scmi/driver.c
@@ -861,6 +861,9 @@ scmi_xfer_command_acquire(struct scmi_chan_info *cinfo, u32 msg_hdr)
"Message for %d type %d is not expected!\n",
xfer_id, msg_type);
spin_unlock_irqrestore(&minfo->xfer_lock, flags);
+
+ scmi_bad_message_trace(cinfo, msg_hdr, MSG_UNEXPECTED);
+
return xfer;
}
refcount_inc(&xfer->users);
@@ -885,6 +888,9 @@ scmi_xfer_command_acquire(struct scmi_chan_info *cinfo, u32 msg_hdr)
dev_err(cinfo->dev,
"Invalid message type:%d for %d - HDR:0x%X state:%d\n",
msg_type, xfer_id, msg_hdr, xfer->state);
+
+ scmi_bad_message_trace(cinfo, msg_hdr, MSG_INVALID);
+
/* On error the refcount incremented above has to be dropped */
__scmi_xfer_put(minfo, xfer);
xfer = ERR_PTR(-EINVAL);
@@ -921,6 +927,9 @@ static void scmi_handle_notification(struct scmi_chan_info *cinfo,
if (IS_ERR(xfer)) {
dev_err(dev, "failed to get free message slot (%ld)\n",
PTR_ERR(xfer));
+
+ scmi_bad_message_trace(cinfo, msg_hdr, MSG_NOMEM);
+
scmi_clear_channel(info, cinfo);
return;
}
@@ -1040,6 +1049,7 @@ void scmi_rx_callback(struct scmi_chan_info *cinfo, u32 msg_hdr, void *priv)
break;
default:
WARN_ONCE(1, "received unknown msg_type:%d\n", msg_type);
+ scmi_bad_message_trace(cinfo, msg_hdr, MSG_UNKNOWN);
break;
}
}
diff --git a/drivers/firmware/arm_scmi/mailbox.c b/drivers/firmware/arm_scmi/mailbox.c
index b8d470417e8f..fb0824af7180 100644
--- a/drivers/firmware/arm_scmi/mailbox.c
+++ b/drivers/firmware/arm_scmi/mailbox.c
@@ -56,7 +56,9 @@ static void rx_callback(struct mbox_client *cl, void *m)
*/
if (cl->knows_txdone && !shmem_channel_free(smbox->shmem)) {
dev_warn(smbox->cinfo->dev, "Ignoring spurious A2P IRQ !\n");
- return;
+ return scmi_bad_message_trace(smbox->cinfo,
+ shmem_read_header(smbox->shmem),
+ MSG_MBOX_SPURIOUS);
}
scmi_rx_callback(smbox->cinfo, shmem_read_header(smbox->shmem), NULL);
--
2.44.0
On Mon, Mar 25, 2024 at 08:46:18PM +0000, Cristian Marussi wrote:
> Trace also late-timed-out, out-of-order and unexpected/spurious messages.
>
> Signed-off-by: Cristian Marussi <[email protected]>
> ---
> drivers/firmware/arm_scmi/driver.c | 10 ++++++++++
> drivers/firmware/arm_scmi/mailbox.c | 4 +++-
> 2 files changed, 13 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c
> index 7fc1c5b1a2a4..207ed1a52d69 100644
> --- a/drivers/firmware/arm_scmi/driver.c
> +++ b/drivers/firmware/arm_scmi/driver.c
> @@ -861,6 +861,9 @@ scmi_xfer_command_acquire(struct scmi_chan_info *cinfo, u32 msg_hdr)
> "Message for %d type %d is not expected!\n",
> xfer_id, msg_type);
> spin_unlock_irqrestore(&minfo->xfer_lock, flags);
> +
> + scmi_bad_message_trace(cinfo, msg_hdr, MSG_UNEXPECTED);
> +
> return xfer;
> }
> refcount_inc(&xfer->users);
> @@ -885,6 +888,9 @@ scmi_xfer_command_acquire(struct scmi_chan_info *cinfo, u32 msg_hdr)
> dev_err(cinfo->dev,
> "Invalid message type:%d for %d - HDR:0x%X state:%d\n",
> msg_type, xfer_id, msg_hdr, xfer->state);
> +
> + scmi_bad_message_trace(cinfo, msg_hdr, MSG_INVALID);
> +
> /* On error the refcount incremented above has to be dropped */
> __scmi_xfer_put(minfo, xfer);
> xfer = ERR_PTR(-EINVAL);
> @@ -921,6 +927,9 @@ static void scmi_handle_notification(struct scmi_chan_info *cinfo,
> if (IS_ERR(xfer)) {
> dev_err(dev, "failed to get free message slot (%ld)\n",
> PTR_ERR(xfer));
> +
> + scmi_bad_message_trace(cinfo, msg_hdr, MSG_NOMEM);
> +
> scmi_clear_channel(info, cinfo);
> return;
> }
> @@ -1040,6 +1049,7 @@ void scmi_rx_callback(struct scmi_chan_info *cinfo, u32 msg_hdr, void *priv)
> break;
> default:
> WARN_ONCE(1, "received unknown msg_type:%d\n", msg_type);
> + scmi_bad_message_trace(cinfo, msg_hdr, MSG_UNKNOWN);
> break;
> }
> }
> diff --git a/drivers/firmware/arm_scmi/mailbox.c b/drivers/firmware/arm_scmi/mailbox.c
> index b8d470417e8f..fb0824af7180 100644
> --- a/drivers/firmware/arm_scmi/mailbox.c
> +++ b/drivers/firmware/arm_scmi/mailbox.c
> @@ -56,7 +56,9 @@ static void rx_callback(struct mbox_client *cl, void *m)
> */
> if (cl->knows_txdone && !shmem_channel_free(smbox->shmem)) {
> dev_warn(smbox->cinfo->dev, "Ignoring spurious A2P IRQ !\n");
> - return;
> + return scmi_bad_message_trace(smbox->cinfo,
> + shmem_read_header(smbox->shmem),
> + MSG_MBOX_SPURIOUS);
From previous patch, IIUC scmi_bad_message_trace is a void func and doesn't
return anything. Did you not get any build error/warning for this ?
--
Regards,
Sudeep
On Tue, Mar 26, 2024 at 11:24:38AM +0000, Sudeep Holla wrote:
> On Mon, Mar 25, 2024 at 08:46:18PM +0000, Cristian Marussi wrote:
> > Trace also late-timed-out, out-of-order and unexpected/spurious messages.
> >
> > Signed-off-by: Cristian Marussi <[email protected]>
> > ---
> > drivers/firmware/arm_scmi/driver.c | 10 ++++++++++
> > drivers/firmware/arm_scmi/mailbox.c | 4 +++-
> > 2 files changed, 13 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c
> > index 7fc1c5b1a2a4..207ed1a52d69 100644
> > --- a/drivers/firmware/arm_scmi/driver.c
> > +++ b/drivers/firmware/arm_scmi/driver.c
> > @@ -861,6 +861,9 @@ scmi_xfer_command_acquire(struct scmi_chan_info *cinfo, u32 msg_hdr)
> > "Message for %d type %d is not expected!\n",
> > xfer_id, msg_type);
> > spin_unlock_irqrestore(&minfo->xfer_lock, flags);
> > +
> > + scmi_bad_message_trace(cinfo, msg_hdr, MSG_UNEXPECTED);
> > +
> > return xfer;
> > }
> > refcount_inc(&xfer->users);
> > @@ -885,6 +888,9 @@ scmi_xfer_command_acquire(struct scmi_chan_info *cinfo, u32 msg_hdr)
> > dev_err(cinfo->dev,
> > "Invalid message type:%d for %d - HDR:0x%X state:%d\n",
> > msg_type, xfer_id, msg_hdr, xfer->state);
> > +
> > + scmi_bad_message_trace(cinfo, msg_hdr, MSG_INVALID);
> > +
> > /* On error the refcount incremented above has to be dropped */
> > __scmi_xfer_put(minfo, xfer);
> > xfer = ERR_PTR(-EINVAL);
> > @@ -921,6 +927,9 @@ static void scmi_handle_notification(struct scmi_chan_info *cinfo,
> > if (IS_ERR(xfer)) {
> > dev_err(dev, "failed to get free message slot (%ld)\n",
> > PTR_ERR(xfer));
> > +
> > + scmi_bad_message_trace(cinfo, msg_hdr, MSG_NOMEM);
> > +
> > scmi_clear_channel(info, cinfo);
> > return;
> > }
> > @@ -1040,6 +1049,7 @@ void scmi_rx_callback(struct scmi_chan_info *cinfo, u32 msg_hdr, void *priv)
> > break;
> > default:
> > WARN_ONCE(1, "received unknown msg_type:%d\n", msg_type);
> > + scmi_bad_message_trace(cinfo, msg_hdr, MSG_UNKNOWN);
> > break;
> > }
> > }
> > diff --git a/drivers/firmware/arm_scmi/mailbox.c b/drivers/firmware/arm_scmi/mailbox.c
> > index b8d470417e8f..fb0824af7180 100644
> > --- a/drivers/firmware/arm_scmi/mailbox.c
> > +++ b/drivers/firmware/arm_scmi/mailbox.c
> > @@ -56,7 +56,9 @@ static void rx_callback(struct mbox_client *cl, void *m)
> > */
> > if (cl->knows_txdone && !shmem_channel_free(smbox->shmem)) {
> > dev_warn(smbox->cinfo->dev, "Ignoring spurious A2P IRQ !\n");
> > - return;
> > + return scmi_bad_message_trace(smbox->cinfo,
> > + shmem_read_header(smbox->shmem),
> > + MSG_MBOX_SPURIOUS);
>
> From previous patch, IIUC scmi_bad_message_trace is a void func and doesn't
> return anything. Did you not get any build error/warning for this ?
>
No...I am building with W=1....but note that the caller itself here
rx_callback() is supposed to return a void...
..in V3 I may just split this into 2 lines and leave the return; alone on its
own line to avoid any confusion...
Thanks,
Cristian
On Tue, Mar 26, 2024 at 11:57:23AM +0000, Cristian Marussi wrote:
> On Tue, Mar 26, 2024 at 11:24:38AM +0000, Sudeep Holla wrote:
> > On Mon, Mar 25, 2024 at 08:46:18PM +0000, Cristian Marussi wrote:
> > > Trace also late-timed-out, out-of-order and unexpected/spurious messages.
> > >
> > > Signed-off-by: Cristian Marussi <[email protected]>
> > > ---
> > > drivers/firmware/arm_scmi/driver.c | 10 ++++++++++
> > > drivers/firmware/arm_scmi/mailbox.c | 4 +++-
> > > 2 files changed, 13 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c
> > > index 7fc1c5b1a2a4..207ed1a52d69 100644
> > > --- a/drivers/firmware/arm_scmi/driver.c
> > > +++ b/drivers/firmware/arm_scmi/driver.c
> > > @@ -861,6 +861,9 @@ scmi_xfer_command_acquire(struct scmi_chan_info *cinfo, u32 msg_hdr)
> > > "Message for %d type %d is not expected!\n",
> > > xfer_id, msg_type);
> > > spin_unlock_irqrestore(&minfo->xfer_lock, flags);
> > > +
> > > + scmi_bad_message_trace(cinfo, msg_hdr, MSG_UNEXPECTED);
> > > +
> > > return xfer;
> > > }
> > > refcount_inc(&xfer->users);
> > > @@ -885,6 +888,9 @@ scmi_xfer_command_acquire(struct scmi_chan_info *cinfo, u32 msg_hdr)
> > > dev_err(cinfo->dev,
> > > "Invalid message type:%d for %d - HDR:0x%X state:%d\n",
> > > msg_type, xfer_id, msg_hdr, xfer->state);
> > > +
> > > + scmi_bad_message_trace(cinfo, msg_hdr, MSG_INVALID);
> > > +
> > > /* On error the refcount incremented above has to be dropped */
> > > __scmi_xfer_put(minfo, xfer);
> > > xfer = ERR_PTR(-EINVAL);
> > > @@ -921,6 +927,9 @@ static void scmi_handle_notification(struct scmi_chan_info *cinfo,
> > > if (IS_ERR(xfer)) {
> > > dev_err(dev, "failed to get free message slot (%ld)\n",
> > > PTR_ERR(xfer));
> > > +
> > > + scmi_bad_message_trace(cinfo, msg_hdr, MSG_NOMEM);
> > > +
> > > scmi_clear_channel(info, cinfo);
> > > return;
> > > }
> > > @@ -1040,6 +1049,7 @@ void scmi_rx_callback(struct scmi_chan_info *cinfo, u32 msg_hdr, void *priv)
> > > break;
> > > default:
> > > WARN_ONCE(1, "received unknown msg_type:%d\n", msg_type);
> > > + scmi_bad_message_trace(cinfo, msg_hdr, MSG_UNKNOWN);
> > > break;
> > > }
> > > }
> > > diff --git a/drivers/firmware/arm_scmi/mailbox.c b/drivers/firmware/arm_scmi/mailbox.c
> > > index b8d470417e8f..fb0824af7180 100644
> > > --- a/drivers/firmware/arm_scmi/mailbox.c
> > > +++ b/drivers/firmware/arm_scmi/mailbox.c
> > > @@ -56,7 +56,9 @@ static void rx_callback(struct mbox_client *cl, void *m)
> > > */
> > > if (cl->knows_txdone && !shmem_channel_free(smbox->shmem)) {
> > > dev_warn(smbox->cinfo->dev, "Ignoring spurious A2P IRQ !\n");
> > > - return;
> > > + return scmi_bad_message_trace(smbox->cinfo,
> > > + shmem_read_header(smbox->shmem),
> > > + MSG_MBOX_SPURIOUS);
> >
> > From previous patch, IIUC scmi_bad_message_trace is a void func and doesn't
> > return anything. Did you not get any build error/warning for this ?
> >
>
> No...I am building with W=1....but note that the caller itself here
> rx_callback() is supposed to return a void...
>
> ...in V3 I may just split this into 2 lines and leave the return; alone on its
> own line to avoid any confusion...
Not need to respin unless I find something that needs reposting, can fix this
myself.
--
Regards,
Sudeep
On Mon, 25 Mar 2024 20:46:15 +0000, Cristian Marussi wrote:
> a bunch of small SCMI updates based on v6.9-rc1.
> Mainly adding traces for weird SCMI messages like late timed-out replies,
> out of order unexpected messages and malformed messages due to spurious
> mbox IRQs.
>
> Thanks,
> Cristian
>
> [...]
Applied to sudeep.holla/linux (for-next/scmi/updates), thanks!
[1/5] include: trace: Widen the tag buffer in trace_scmi_dump_msg
https://git.kernel.org/sudeep.holla/c/da251ce21061
[2/5] firmware: arm_scmi: Add helper to trace bad messages
https://git.kernel.org/sudeep.holla/c/5dc0e0b1f0ea
[3/5] firmware: arm_scmi: Add message dump traces for bad and unexpected replies
https://git.kernel.org/sudeep.holla/c/5076ab66db16
[4/5] firmware: arm_scmi: Simplify scmi_devm_notifier_unregister
https://git.kernel.org/sudeep.holla/c/264a2c520628
[5/5] firmware: arm_scmi: Use dev_err_probe to bail out
https://git.kernel.org/sudeep.holla/c/3a7d93d1f71b
--
Regards,
Sudeep