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

[Bjorn Andersson]

On Tue, Jun 11, 2024 at 10:53:01AM -0700, Chris Lew wrote:
> 
> 
> 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".
> 

You're right, I forgot about that.

This actually means that if we replace "smp2p" with NULL, we should get
the descriptive names we're looking for automagically (as the node name
is used to build dev_name()).

> 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.
> 

But this would be a change on the consumer side, right? To replace the
"q6v5" that we have hard coded for all the PAS remoteproc instances.

I'd be happy to see such change.

Regards,
Bjorn

> 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;
> > >   }
> > > -- 
> > >