linux-kernel - Re: [PATCH V2 1/2] soc: qcom: smp2p: Add irqchip state support

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <5xhvxnqkvuy6lthzpyk64pm6v3zovcftfb3a2jep3oulrtohpd@o6whhdwevi5h>
Date: Mon, 3 Nov 2025 19:41:09 -0600
From: Bjorn Andersson <andersson@...nel.org>
To: Deepak Kumar Singh <deepak.singh@....qualcomm.com>
Cc: chris.lew@....qualcomm.com, konradybcio@...nel.org, 
	jingyi.wang@....qualcomm.com, linux-kernel@...r.kernel.org, linux-arm-msm@...r.kernel.org, 
	linux-remoteproc@...r.kernel.org
Subject: Re: [PATCH V2 1/2] soc: qcom: smp2p: Add irqchip state support

On Mon, Nov 03, 2025 at 08:59:28PM +0530, Deepak Kumar Singh wrote:
> From: Chris Lew <chris.lew@....qualcomm.com>
> 
> A remoteproc booted during earlier boot stages such as UEFI or the
> bootloader, may need to be attached to without restarting the remoteproc
> hardware. To do this the remoteproc will need to check the ready and
> handover states in smp2p without an interrupt notification.
> 

The structure of the commit message is really good. But we don't _need_
any of this in order to attach to a remoteproc without restarting it.

We _need_ this stuff to determine if a remoteproc was started by the
bootloader, and to determine if it has crashed.

> Add support for the .irq_get_irqchip_state callback so remoteproc can
> read the current state of the fatal, ready and handover bits.
> 
> Signed-off-by: Chris Lew <chris.lew@....qualcomm.com>
> Signed-off-by: Deepak Kumar Singh <deepak.singh@....qualcomm.com>
> ---
>  drivers/soc/qcom/smp2p.c | 61 ++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 61 insertions(+)
> 
> diff --git a/drivers/soc/qcom/smp2p.c b/drivers/soc/qcom/smp2p.c
> index cb515c2340c1..39628df36745 100644
> --- a/drivers/soc/qcom/smp2p.c
> +++ b/drivers/soc/qcom/smp2p.c
> @@ -48,6 +48,9 @@
>  #define SMP2P_MAGIC 0x504d5324
>  #define SMP2P_ALL_FEATURES	SMP2P_FEATURE_SSR_ACK
>  
> +#define ONE 1
> +#define TWO 2

You forgot "#define PLEASE_DONT true"...

Sorry if I wasn't expressing myself clearly enough, we're using defines
of magic values to make the code easier to read, follow, and understand.

Giving trivial integer version numbers a "human readable" name doesn't
meet this criteria. Use 1 and 2 in the code.

> +
>  /**
>   * struct smp2p_smem_item - in memory communication structure
>   * @magic:		magic number
> @@ -222,6 +225,42 @@ static void qcom_smp2p_negotiate(struct qcom_smp2p *smp2p)
>  	}
>  }
>  
> +static void qcom_smp2p_start_in(struct qcom_smp2p *smp2p)
> +{
> +	unsigned int smem_id = smp2p->smem_items[SMP2P_INBOUND];
> +	unsigned int pid = smp2p->remote_pid;
> +	char buf[SMP2P_MAX_ENTRY_NAME];
> +	struct smp2p_smem_item *in;
> +	struct smp2p_entry *entry;
> +	size_t size;
> +	int i;
> +
> +	in = qcom_smem_get(pid, smem_id, &size);
> +	if (IS_ERR(in))
> +		return;
> +
> +	smp2p->in = in;
> +
> +	/* Check if version is initialized and set to v2.

Newline after the /* in multi-line comments please.

> +	 * Early enumeration of inbound entries is required only
> +	 * for early boot processors which have smp2p version 2.

"required only for early boot processors which have smp2p version 2",
does "version 2" imply "early boot processor"?

Does "required" imply that we could do this for other subsystems as
well, but we choose not do do so? This comment should be sufficient for
me not to feel the urge of removing the condition 3 months from now.


Also, isn't it the case that for all non-early-boot subsystems, they
haven't been running yet, so qcom_smem_get() above will fail?


Please start over, and rewrite this comment from the angle of describing
how this fits into the handling of early boot systems.

> +	 */
> +	if (in->version != TWO)
> +		return;
> +
> +	for (i = smp2p->valid_entries; i < in->valid_entries; i++) {
> +		list_for_each_entry(entry, &smp2p->inbound, node) {
> +			memcpy(buf, in->entries[i].name, sizeof(buf));
> +			if (!strcmp(buf, entry->name)) {
> +				entry->value = &in->entries[i].value;
> +				entry->last_value = readl(entry->value);
> +				break;
> +			}
> +		}
> +	}
> +	smp2p->valid_entries = i;

Why don't we just call qcom_smp2p_notify_in() instead of all this?

> +}
> +
>  static void qcom_smp2p_notify_in(struct qcom_smp2p *smp2p)
>  {
>  	struct smp2p_smem_item *in;
> @@ -368,12 +407,31 @@ static void smp2p_irq_print_chip(struct irq_data *irqd, struct seq_file *p)
>  	seq_printf(p, "%8s", dev_name(entry->smp2p->dev));
>  }
>  
> +static int smp2p_irq_get_irqchip_state(struct irq_data *irqd, enum irqchip_irq_state which,
> +				       bool *state)
> +{
> +	struct smp2p_entry *entry = irq_data_get_irq_chip_data(irqd);
> +	u32 val;
> +
> +	if (which != IRQCHIP_STATE_LINE_LEVEL)
> +		return -EINVAL;
> +
> +	if (!entry->value)
> +		return -ENODEV;
> +
> +	val = readl(entry->value);
> +	*state = !!(val & BIT(irqd_to_hwirq(irqd)));
> +
> +	return 0;
> +}
> +
>  static struct irq_chip smp2p_irq_chip = {
>  	.name           = "smp2p",
>  	.irq_mask       = smp2p_mask_irq,
>  	.irq_unmask     = smp2p_unmask_irq,
>  	.irq_set_type	= smp2p_set_irq_type,
>  	.irq_print_chip = smp2p_irq_print_chip,
> +	.irq_get_irqchip_state = smp2p_irq_get_irqchip_state,

This part looks good.

Regards,
Bjorn

>  };
>  
>  static int smp2p_irq_map(struct irq_domain *d,
> @@ -618,6 +676,9 @@ static int qcom_smp2p_probe(struct platform_device *pdev)
>  		}
>  	}
>  
> +	/* Check inbound entries in the case of early boot processor */
> +	qcom_smp2p_start_in(smp2p);
> +
>  	/* Kick the outgoing edge after allocating entries */
>  	qcom_smp2p_kick(smp2p);
>  
> -- 
> 2.34.1
>