Re: [PATCH 4/6] rpmsg: glink: Move irq and mbox handling to transports

[Bjorn Andersson]

On Tue, Jan 24, 2023 at 10:55:47PM -0800, Chris Lew wrote:
> On 1/9/2023 2:39 PM, Bjorn Andersson wrote:
> > diff --git a/drivers/rpmsg/qcom_glink_native.c b/drivers/rpmsg/qcom_glink_native.c
[..]
> > @@ -305,8 +296,7 @@ static void qcom_glink_tx_write(struct qcom_glink *glink,
> >   static void qcom_glink_tx_kick(struct qcom_glink *glink)
> >   {
> > -	mbox_send_message(glink->mbox_chan, NULL);
> > -	mbox_client_txdone(glink->mbox_chan, 0);
> > +	glink->tx_pipe->kick(glink->tx_pipe);
> 
> I think that we need to check that tx_pipe is not null or validate that
> tx_pipe is not null in the native register probe.
> 

The function pointers are const, so it's only during development of a
transport that this could become NULL, and it's impossible to register
successfully without hitting that oops.

So I don't know if it's worth adding a runtime check for this. It's just
a handful of checks, but they would run trillions of times for no
purpose...

> >   }
> >   static void qcom_glink_send_read_notify(struct qcom_glink *glink)
[..]
> > @@ -35,6 +36,6 @@ struct qcom_glink *qcom_glink_native_probe(struct device *dev,
> >   					   struct qcom_glink_pipe *tx,
> >   					   bool intentless);
> >   void qcom_glink_native_remove(struct qcom_glink *glink);
> > +void qcom_glink_native_intr(struct qcom_glink *glink);
> > 
> 
> We could rename this away from qcom_glink_native_intr to something like
> qcom_glink_native_rx. Seeing this in the header, the purpose sounds a bit
> obscure.
> 

Perhaps qcom_glink_native_notify_rx()?

> > -void qcom_glink_native_unregister(struct qcom_glink *glink);
> >   #endif
> > diff --git a/drivers/rpmsg/qcom_glink_rpm.c b/drivers/rpmsg/qcom_glink_rpm.c
[..]
> > @@ -277,6 +301,24 @@ struct qcom_glink_smem *qcom_glink_smem_register(struct device *parent,
> >   		goto err_put_dev;
> >   	}
> > +	smem->irq = of_irq_get(smem->dev.of_node, 0);
> > +	ret = devm_request_irq(&smem->dev, smem->irq, qcom_glink_smem_intr,
> > +			       IRQF_NO_SUSPEND | IRQF_NO_AUTOEN,
> > +			       "glink-smem", smem);
> 
> Are we adding dropping IRQF_NO_SUSPEND and adding enable irq wake for smem
> in follow up change?
> 

Yes, while I haven't reviewed all the details of that discussion again,
I was planning to follow up with something on that after this has been
merged.

That way we can discuss/review that separately.

> > +	if (ret) {
> > +		dev_err(&smem->dev, "failed to request IRQ\n");
> > +		goto err_put_dev;
> > +	}
> > +
[..]
> > @@ -314,8 +361,13 @@ void qcom_glink_smem_unregister(struct qcom_glink_smem *smem)
> >   {
> >   	struct qcom_glink *glink = smem->glink;
> > +	disable_irq(smem->irq);
> > +
> >   	qcom_glink_native_remove(glink);
> > -	qcom_glink_native_unregister(glink);
> > +
> > +	device_unregister(&smem->dev);
> > +
> > +	mbox_free_channel(smem->mbox_chan);
> 
> This might need to be moved above device_unregister. I think the release
> function frees the smem structure.
> 

Yes, that looks correct.

Thank you for the review,
Bjorn

> >   }
> >   EXPORT_SYMBOL_GPL(qcom_glink_smem_unregister);