[PATCH v2 0/5] SCMI misc small-updates

[PATCH v2 0/5] SCMI misc small-updates

[Cristian Marussi]

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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 1/5] include: trace: Widen the tag buffer in trace_scmi_dump_msg

[Cristian Marussi]

A bigger buffer allow for more diverse tag names.

Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
---
 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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 2/5] firmware: arm_scmi: Add helper to trace bad messages

[Cristian Marussi]

Upon reception of malformed and unexpected timed-out SCMI messages, it is
not possible to trace those bad messages in their entirety, because usually
we cannot even retrieve the payload, or it is just not reliable.

Add a helper to trace at least the content of the header of the received
message while associating a meaningful tag and error code.

Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
---
 drivers/firmware/arm_scmi/common.h | 11 +++++++++
 drivers/firmware/arm_scmi/driver.c | 39 ++++++++++++++++++++++++++++++
 2 files changed, 50 insertions(+)

diff --git a/drivers/firmware/arm_scmi/common.h b/drivers/firmware/arm_scmi/common.h
index 6affbfdd1dec..b5ac25dbc1ca 100644
--- a/drivers/firmware/arm_scmi/common.h
+++ b/drivers/firmware/arm_scmi/common.h
@@ -301,6 +301,17 @@ extern const struct scmi_desc scmi_optee_desc;
 
 void scmi_rx_callback(struct scmi_chan_info *cinfo, u32 msg_hdr, void *priv);
 
+enum scmi_bad_msg {
+	MSG_UNEXPECTED = -1,
+	MSG_INVALID = -2,
+	MSG_UNKNOWN = -3,
+	MSG_NOMEM = -4,
+	MSG_MBOX_SPURIOUS = -5,
+};
+
+void scmi_bad_message_trace(struct scmi_chan_info *cinfo, u32 msg_hdr,
+			    enum scmi_bad_msg err);
+
 /* shmem related declarations */
 struct scmi_shared_mem;
 
diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c
index 2709598f3008..7fc1c5b1a2a4 100644
--- a/drivers/firmware/arm_scmi/driver.c
+++ b/drivers/firmware/arm_scmi/driver.c
@@ -696,6 +696,45 @@ scmi_xfer_lookup_unlocked(struct scmi_xfers_info *minfo, u16 xfer_id)
 	return xfer ?: ERR_PTR(-EINVAL);
 }
 
+/**
+ * scmi_bad_message_trace  - A helper to trace weird messages
+ *
+ * @cinfo: A reference to the channel descriptor on which the message was
+ *	   received
+ * @msg_hdr: Message header to track
+ * @err: A specific error code used as a status value in traces.
+ *
+ * This helper can be used to trace any kind of weird, incomplete, unexpected,
+ * timed-out message that arrives and as such, can be traced only referring to
+ * the header content, since the payload is missing/unreliable.
+ */
+void scmi_bad_message_trace(struct scmi_chan_info *cinfo, u32 msg_hdr,
+			    enum scmi_bad_msg err)
+{
+	char *tag;
+	struct scmi_info *info = handle_to_scmi_info(cinfo->handle);
+
+	switch (MSG_XTRACT_TYPE(msg_hdr)) {
+	case MSG_TYPE_COMMAND:
+		tag = "!RESP";
+		break;
+	case MSG_TYPE_DELAYED_RESP:
+		tag = "!DLYD";
+		break;
+	case MSG_TYPE_NOTIFICATION:
+		tag = "!NOTI";
+		break;
+	default:
+		tag = "!UNKN";
+		break;
+	}
+
+	trace_scmi_msg_dump(info->id, cinfo->id,
+			    MSG_XTRACT_PROT_ID(msg_hdr),
+			    MSG_XTRACT_ID(msg_hdr), tag,
+			    MSG_XTRACT_TOKEN(msg_hdr), err, NULL, 0);
+}
+
 /**
  * scmi_msg_response_validate  - Validate message type against state of related
  * xfer
-- 
2.44.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 3/5] firmware: arm_scmi: Add message dump traces for bad and unexpected replies

[Cristian Marussi]

Trace also late-timed-out, out-of-order and unexpected/spurious messages.

Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
---
 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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 4/5] firmware: arm_scmi: Simplify scmi_devm_notifier_unregister

[Cristian Marussi]

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 <cristian.marussi@arm.com>
---
 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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 5/5] firmware: arm_scmi: Use dev_err_probe to bail out

[Cristian Marussi]

Use dev_err_probe on the failure path of SCMI core probing.

Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
---
 drivers/firmware/arm_scmi/driver.c | 34 +++++++++++++++++++++++-------
 1 file changed, 26 insertions(+), 8 deletions(-)

diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c
index 207ed1a52d69..d0091459a276 100644
--- a/drivers/firmware/arm_scmi/driver.c
+++ b/drivers/firmware/arm_scmi/driver.c
@@ -2540,6 +2540,10 @@ scmi_txrx_setup(struct scmi_info *info, struct device_node *of_node,
 			ret = 0;
 	}
 
+	if (ret)
+		dev_err(info->dev,
+			"failed to setup channel for protocol:0x%X\n", prot_id);
+
 	return ret;
 }
 
@@ -2809,6 +2813,7 @@ static int scmi_debugfs_raw_mode_setup(struct scmi_info *info)
 static int scmi_probe(struct platform_device *pdev)
 {
 	int ret;
+	char *err_str = "probe failure\n";
 	struct scmi_handle *handle;
 	const struct scmi_desc *desc;
 	struct scmi_info *info;
@@ -2859,27 +2864,37 @@ static int scmi_probe(struct platform_device *pdev)
 
 	if (desc->ops->link_supplier) {
 		ret = desc->ops->link_supplier(dev);
-		if (ret)
+		if (ret) {
+			err_str = "transport not ready\n";
 			goto clear_ida;
+		}
 	}
 
 	/* Setup all channels described in the DT at first */
 	ret = scmi_channels_setup(info);
-	if (ret)
+	if (ret) {
+		err_str = "failed to setup channels\n";
 		goto clear_ida;
+	}
 
 	ret = bus_register_notifier(&scmi_bus_type, &info->bus_nb);
-	if (ret)
+	if (ret) {
+		err_str = "failed to register bus notifier\n";
 		goto clear_txrx_setup;
+	}
 
 	ret = blocking_notifier_chain_register(&scmi_requested_devices_nh,
 					       &info->dev_req_nb);
-	if (ret)
+	if (ret) {
+		err_str = "failed to register device notifier\n";
 		goto clear_bus_notifier;
+	}
 
 	ret = scmi_xfer_info_init(info);
-	if (ret)
+	if (ret) {
+		err_str = "failed to init xfers pool\n";
 		goto clear_dev_req_notifier;
+	}
 
 	if (scmi_top_dentry) {
 		info->dbg = scmi_debugfs_common_setup(info);
@@ -2916,9 +2931,11 @@ static int scmi_probe(struct platform_device *pdev)
 	 */
 	ret = scmi_protocol_acquire(handle, SCMI_PROTOCOL_BASE);
 	if (ret) {
-		dev_err(dev, "unable to communicate with SCMI\n");
-		if (coex)
+		err_str = "unable to communicate with SCMI\n";
+		if (coex) {
+			dev_err(dev, err_str);
 			return 0;
+		}
 		goto notification_exit;
 	}
 
@@ -2972,7 +2989,8 @@ static int scmi_probe(struct platform_device *pdev)
 	scmi_cleanup_txrx_channels(info);
 clear_ida:
 	ida_free(&scmi_id, info->id);
-	return ret;
+
+	return dev_err_probe(dev, ret, err_str);
 }
 
 static void scmi_remove(struct platform_device *pdev)
-- 
2.44.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 3/5] firmware: arm_scmi: Add message dump traces for bad and unexpected replies

[Sudeep Holla]

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 <cristian.marussi@arm.com>
> ---
>  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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 3/5] firmware: arm_scmi: Add message dump traces for bad and unexpected replies

[Cristian Marussi]

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 <cristian.marussi@arm.com>
> > ---
> >  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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 3/5] firmware: arm_scmi: Add message dump traces for bad and unexpected replies

[Sudeep Holla]

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 <cristian.marussi@arm.com>
> > > ---
> > >  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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 0/5] SCMI misc small-updates

[Sudeep Holla]

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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel