Re: [PATCH V2 1/2] soc: qcom: smp2p: Add remote name into smp2p irq devname

[Chris Lew]

On 6/11/2024 9:06 AM, Bjorn Andersson wrote:
> On Tue, Jun 11, 2024 at 06:03:50PM +0530, Sudeepgoud Patil wrote:
> > Add smp2p irq devname which fetches remote name from respective
> > smp2p dtsi node, which makes the wakeup source distinguishable
> > in irq wakeup prints.
> >
> > Signed-off-by: Sudeepgoud Patil <quic_sudeepgo@xxxxxxxxxxx>
> > ---
> >   drivers/soc/qcom/smp2p.c | 14 +++++++++++++-
> >   1 file changed, 13 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/soc/qcom/smp2p.c b/drivers/soc/qcom/smp2p.c
> > index a21241cbeec7..a77fee048b38 100644
> > --- a/drivers/soc/qcom/smp2p.c
> > +++ b/drivers/soc/qcom/smp2p.c
> > @@ -122,6 +122,7 @@ struct smp2p_entry {
> >    * @ssr_ack_enabled: SMP2P_FEATURE_SSR_ACK feature is supported and was enabled
> >    * @ssr_ack: current cached state of the local ack bit
> >    * @negotiation_done: whether negotiating finished
> > + * @irq_devname: poniter to the smp2p irq devname
> >    * @local_pid:	processor id of the inbound edge
> >    * @remote_pid:	processor id of the outbound edge
> >    * @ipc_regmap:	regmap for the outbound ipc
> > @@ -146,6 +147,7 @@ struct qcom_smp2p {
> >   	bool ssr_ack;
> >   	bool negotiation_done;
> >   
> > +	char *irq_devname;
> >   	unsigned local_pid;
> >   	unsigned remote_pid;
> >   
> > @@ -614,10 +616,16 @@ static int qcom_smp2p_probe(struct platform_device *pdev)
> >   	/* Kick the outgoing edge after allocating entries */
> >   	qcom_smp2p_kick(smp2p);
> >   
> > +	smp2p->irq_devname = kasprintf(GFP_KERNEL, "%s", pdev->dev.of_node->name);
>
> That's a lot of extra instructions for copying a string, which doesn't
> need to be copied because of_node->name is const char and the argument
> to devm_request_threaded_irq() is const char.
>
> So, kstrdup_const() is what you're looking for.
>
> You can then go devm_kstrdup_const() and avoid the kfree() (then
> kfree_const()) below.
>
>
> That said, looking at /proc/interrupts, I think it would make sense to
> make this devm_kasprintf(..., "smp2p-%s", name);

Is it ok to rely on the "of_node->name"? I think device tree tends to 
always have the node name as "smp2p-%s" already, so ("smp2p-%s", name) 
would result in "smp2p-smp2p-adsp".

Also Sudeepgoud, I think this will update the irqname in 
/proc/interrupts for the ipcc irqchip entry. It would also be helpful if 
we could differentiate the instances of smp2p irqchips as well. That way 
we can see what processors the 'ready' and 'fatal' interrupts apply to 
in /proc/interrupts.

Can you refer to my internal patch that adds .irq_print_chip() and 
incorporate those changes here?

> Regards,
> Bjorn
>
> > +	if (!smp2p->irq_devname) {
> > +		ret = -ENOMEM;
> > +		goto unwind_interfaces;
> > +	}
> > +
> >   	ret = devm_request_threaded_irq(&pdev->dev, irq,
> >   					NULL, qcom_smp2p_intr,
> >   					IRQF_ONESHOT,
> > -					"smp2p", (void *)smp2p);
> > +					smp2p->irq_devname, (void *)smp2p);
> >   	if (ret) {
> >   		dev_err(&pdev->dev, "failed to request interrupt\n");
> >   		goto unwind_interfaces;
> > @@ -650,6 +658,8 @@ static int qcom_smp2p_probe(struct platform_device *pdev)
> >   	list_for_each_entry(entry, &smp2p->outbound, node)
> >   		qcom_smem_state_unregister(entry->state);
> >   
> > +	kfree(smp2p->irq_devname);
> > +
> >   	smp2p->out->valid_entries = 0;
> >   
> >   release_mbox:
> > @@ -677,6 +687,8 @@ static void qcom_smp2p_remove(struct platform_device *pdev)
> >   
> >   	mbox_free_channel(smp2p->mbox_chan);
> >   
> > +	kfree(smp2p->irq_devname);
> > +
> >   	smp2p->out->valid_entries = 0;
> >   }
> >   
> > --