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