lkml.org 
[lkml]   [2023]   [Jan]   [25]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH 2/6] rpmsg: glink: smem: Wrap driver context


    On 1/9/2023 2:39 PM, Bjorn Andersson wrote:
    > The Glink SMEM driver allocates a struct device and hangs two
    > devres-allocated pipe objects thereon. To facilitate the move of
    > interrupt and mailbox handling to the driver, introduce a wrapper object
    > capturing the device, glink reference and remote processor id.
    >
    > The type of the remoteproc reference is updated, as these are
    > specifically targetting the SMEM implementation.

    s/targetting/targeting

    >
    > Signed-off-by: Bjorn Andersson <quic_bjorande@quicinc.com>
    > ---
    > drivers/remoteproc/qcom_common.h | 3 +-
    > drivers/rpmsg/qcom_glink_smem.c | 76 ++++++++++++++++++++------------
    > include/linux/rpmsg/qcom_glink.h | 12 ++---
    > 3 files changed, 55 insertions(+), 36 deletions(-)
    >
    > diff --git a/drivers/remoteproc/qcom_common.h b/drivers/remoteproc/qcom_common.h
    > index c35adf730be0..2747c7d9ba44 100644
    > --- a/drivers/remoteproc/qcom_common.h
    > +++ b/drivers/remoteproc/qcom_common.h
    > @@ -6,6 +6,7 @@
    > #include "remoteproc_internal.h"
    > #include <linux/soc/qcom/qmi.h>
    >
    > +struct qcom_glink_smem;
    > struct qcom_sysmon;
    >
    > struct qcom_rproc_glink {
    > @@ -15,7 +16,7 @@ struct qcom_rproc_glink {
    >
    > struct device *dev;
    > struct device_node *node;
    > - struct qcom_glink *edge;
    > + struct qcom_glink_smem *edge;
    > };
    >
    > struct qcom_rproc_subdev {
    > diff --git a/drivers/rpmsg/qcom_glink_smem.c b/drivers/rpmsg/qcom_glink_smem.c
    > index 579bc4443f6d..703e63fa5a86 100644
    > --- a/drivers/rpmsg/qcom_glink_smem.c
    > +++ b/drivers/rpmsg/qcom_glink_smem.c
    > @@ -33,6 +33,14 @@
    > #define SMEM_GLINK_NATIVE_XPRT_FIFO_0 479
    > #define SMEM_GLINK_NATIVE_XPRT_FIFO_1 480
    >
    > +struct qcom_glink_smem {
    > + struct device dev;
    > +
    > + struct qcom_glink *glink;
    > +
    > + u32 remote_pid;
    > +};
    > +
    > struct glink_smem_pipe {
    > struct qcom_glink_pipe native;
    >
    > @@ -41,7 +49,7 @@ struct glink_smem_pipe {
    >
    > void *fifo;
    >
    > - int remote_pid;
    > + struct qcom_glink_smem *smem;
    > };
    >
    > #define to_smem_pipe(p) container_of(p, struct glink_smem_pipe, native)
    > @@ -49,13 +57,14 @@ struct glink_smem_pipe {
    > static size_t glink_smem_rx_avail(struct qcom_glink_pipe *np)
    > {
    > struct glink_smem_pipe *pipe = to_smem_pipe(np);
    > + struct qcom_glink_smem *smem = pipe->smem;
    > size_t len;
    > void *fifo;
    > u32 head;
    > u32 tail;
    >
    > if (!pipe->fifo) {
    > - fifo = qcom_smem_get(pipe->remote_pid,
    > + fifo = qcom_smem_get(smem->remote_pid,
    > SMEM_GLINK_NATIVE_XPRT_FIFO_1, &len);
    > if (IS_ERR(fifo)) {
    > pr_err("failed to acquire RX fifo handle: %ld\n",
    > @@ -179,45 +188,49 @@ static void glink_smem_tx_write(struct qcom_glink_pipe *glink_pipe,
    >
    > static void qcom_glink_smem_release(struct device *dev)
    > {
    > - kfree(dev);
    > + struct qcom_glink_smem *smem = container_of(dev, struct qcom_glink_smem, dev);
    > +
    > + kfree(smem);
    > }
    >
    > -struct qcom_glink *qcom_glink_smem_register(struct device *parent,
    > - struct device_node *node)
    > +struct qcom_glink_smem *qcom_glink_smem_register(struct device *parent,
    > + struct device_node *node)
    > {
    > struct glink_smem_pipe *rx_pipe;
    > struct glink_smem_pipe *tx_pipe;
    > struct qcom_glink *glink;
    > - struct device *dev;
    > + struct qcom_glink_smem *smem;

    I think we're following reverse christmas tree in this file

    > u32 remote_pid;
    > __le32 *descs;
    > size_t size;
    > int ret;
    >
    > - dev = kzalloc(sizeof(*dev), GFP_KERNEL);
    > - if (!dev)
    > + smem = kzalloc(sizeof(*smem), GFP_KERNEL);
    > + if (!smem)
    > return ERR_PTR(-ENOMEM);
    >

    Would it be proper to keep a pointer to dev and avoid all the changes to
    smem->dev use?

    dev = &smem->dev;

    > - dev->parent = parent;
    > - dev->of_node = node;
    > - dev->release = qcom_glink_smem_release;
    > - dev_set_name(dev, "%s:%pOFn", dev_name(parent->parent), node);
    > - ret = device_register(dev);
    > + smem->dev.parent = parent;
    > + smem->dev.of_node = node;
    > + smem->dev.release = qcom_glink_smem_release;
    > + dev_set_name(&smem->dev, "%s:%pOFn", dev_name(parent->parent), node);
    > + ret = device_register(&smem->dev);
    > if (ret) {
    > pr_err("failed to register glink edge\n");
    > - put_device(dev);
    > + put_device(&smem->dev);
    > return ERR_PTR(ret);
    > }
    >
    > - ret = of_property_read_u32(dev->of_node, "qcom,remote-pid",
    > + ret = of_property_read_u32(smem->dev.of_node, "qcom,remote-pid",
    > &remote_pid);
    > if (ret) {
    > - dev_err(dev, "failed to parse qcom,remote-pid\n");
    > + dev_err(&smem->dev, "failed to parse qcom,remote-pid\n");
    > goto err_put_dev;
    > }
    >
    > - rx_pipe = devm_kzalloc(dev, sizeof(*rx_pipe), GFP_KERNEL);
    > - tx_pipe = devm_kzalloc(dev, sizeof(*tx_pipe), GFP_KERNEL);
    > + smem->remote_pid = remote_pid;
    > +
    > + rx_pipe = devm_kzalloc(&smem->dev, sizeof(*rx_pipe), GFP_KERNEL);
    > + tx_pipe = devm_kzalloc(&smem->dev, sizeof(*tx_pipe), GFP_KERNEL);
    > if (!rx_pipe || !tx_pipe) {
    > ret = -ENOMEM;
    > goto err_put_dev;
    > @@ -226,20 +239,20 @@ struct qcom_glink *qcom_glink_smem_register(struct device *parent,
    > ret = qcom_smem_alloc(remote_pid,
    > SMEM_GLINK_NATIVE_XPRT_DESCRIPTOR, 32);
    > if (ret && ret != -EEXIST) {
    > - dev_err(dev, "failed to allocate glink descriptors\n");
    > + dev_err(&smem->dev, "failed to allocate glink descriptors\n");
    > goto err_put_dev;
    > }
    >
    > descs = qcom_smem_get(remote_pid,
    > SMEM_GLINK_NATIVE_XPRT_DESCRIPTOR, &size);
    > if (IS_ERR(descs)) {
    > - dev_err(dev, "failed to acquire xprt descriptor\n");
    > + dev_err(&smem->dev, "failed to acquire xprt descriptor\n");
    > ret = PTR_ERR(descs);
    > goto err_put_dev;
    > }
    >
    > if (size != 32) {
    > - dev_err(dev, "glink descriptor of invalid size\n");
    > + dev_err(&smem->dev, "glink descriptor of invalid size\n");
    > ret = -EINVAL;
    > goto err_put_dev;
    > }
    > @@ -252,31 +265,31 @@ struct qcom_glink *qcom_glink_smem_register(struct device *parent,
    > ret = qcom_smem_alloc(remote_pid, SMEM_GLINK_NATIVE_XPRT_FIFO_0,
    > SZ_16K);
    > if (ret && ret != -EEXIST) {
    > - dev_err(dev, "failed to allocate TX fifo\n");
    > + dev_err(&smem->dev, "failed to allocate TX fifo\n");
    > goto err_put_dev;
    > }
    >
    > tx_pipe->fifo = qcom_smem_get(remote_pid, SMEM_GLINK_NATIVE_XPRT_FIFO_0,
    > &tx_pipe->native.length);
    > if (IS_ERR(tx_pipe->fifo)) {
    > - dev_err(dev, "failed to acquire TX fifo\n");
    > + dev_err(&smem->dev, "failed to acquire TX fifo\n");
    > ret = PTR_ERR(tx_pipe->fifo);
    > goto err_put_dev;
    > }
    >
    > + rx_pipe->smem = smem;
    > rx_pipe->native.avail = glink_smem_rx_avail;
    > rx_pipe->native.peak = glink_smem_rx_peak;
    > rx_pipe->native.advance = glink_smem_rx_advance;
    > - rx_pipe->remote_pid = remote_pid;
    >
    > + tx_pipe->smem = smem;
    > tx_pipe->native.avail = glink_smem_tx_avail;
    > tx_pipe->native.write = glink_smem_tx_write;
    > - tx_pipe->remote_pid = remote_pid;
    >
    > *rx_pipe->tail = 0;
    > *tx_pipe->head = 0;
    >
    > - glink = qcom_glink_native_probe(dev,
    > + glink = qcom_glink_native_probe(&smem->dev,
    > GLINK_FEATURE_INTENT_REUSE,
    > &rx_pipe->native, &tx_pipe->native,
    > false);
    > @@ -285,17 +298,22 @@ struct qcom_glink *qcom_glink_smem_register(struct device *parent,
    > goto err_put_dev;
    > }
    >
    > - return glink;
    > + smem->glink = glink;
    > +
    > + return smem;
    > +
    >
    > err_put_dev:
    > - device_unregister(dev);
    > + device_unregister(&smem->dev);
    >
    > return ERR_PTR(ret);
    > }
    > EXPORT_SYMBOL_GPL(qcom_glink_smem_register);
    >
    > -void qcom_glink_smem_unregister(struct qcom_glink *glink)
    > +void qcom_glink_smem_unregister(struct qcom_glink_smem *smem)
    > {
    > + struct qcom_glink *glink = smem->glink;
    > +
    > qcom_glink_native_remove(glink);
    > qcom_glink_native_unregister(glink);
    > }
    > diff --git a/include/linux/rpmsg/qcom_glink.h b/include/linux/rpmsg/qcom_glink.h
    > index 22fc3a69b683..bfbd48f435fa 100644
    > --- a/include/linux/rpmsg/qcom_glink.h
    > +++ b/include/linux/rpmsg/qcom_glink.h
    > @@ -5,7 +5,7 @@
    >
    > #include <linux/device.h>
    >
    > -struct qcom_glink;
    > +struct qcom_glink_smem;
    >
    > #if IS_ENABLED(CONFIG_RPMSG_QCOM_GLINK)
    > void qcom_glink_ssr_notify(const char *ssr_name);
    > @@ -15,20 +15,20 @@ static inline void qcom_glink_ssr_notify(const char *ssr_name) {}
    >
    > #if IS_ENABLED(CONFIG_RPMSG_QCOM_GLINK_SMEM)
    >
    > -struct qcom_glink *qcom_glink_smem_register(struct device *parent,
    > - struct device_node *node);
    > -void qcom_glink_smem_unregister(struct qcom_glink *glink);
    > +struct qcom_glink_smem *qcom_glink_smem_register(struct device *parent,
    > + struct device_node *node);
    > +void qcom_glink_smem_unregister(struct qcom_glink_smem *glink);
    >
    > #else
    >
    > -static inline struct qcom_glink *
    > +static inline struct qcom_glink_smem *
    > qcom_glink_smem_register(struct device *parent,
    > struct device_node *node)
    > {
    > return NULL;
    > }
    >
    > -static inline void qcom_glink_smem_unregister(struct qcom_glink *glink) {}
    > +static inline void qcom_glink_smem_unregister(struct qcom_glink_smem *glink) {}
    > #endif
    >
    > #endif

    \
     
     \ /
      Last update: 2023-03-26 23:55    [W:3.909 / U:0.088 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site