Add support for the GLINK rpmsg transport to register a rpmsg chrdev.
This will create the rpmsg_ctrl nodes for userspace clients to open
rpmsg epts. The rpmsg chrdev allocation is done by allocating a local
channel which also allocates an ept. We need to add some guards against
edge cases for this chrdev because it will never fully open.
Changes since v4:
- Resending by removing approved patches
Changes since v3:
- Change to device_add_group for rpmsg name attr
- Add patch to unregister the rpmsg device
- Add patch to support compat ioctl for rpmsg char driver
Changes since v2:
- Revert change to make glink attribute table const
Changes since v1:
- Add explanation to dt-bindings commit message
- Add patch complete_all the open_req/ack variables
- Add patch to prevent null pointer dereference in chrdev channel release
- Change chrdev allocation to use glink channel allocation
- Change glink attr struct to const
Arun Kumar Neelakantam (1):
rpmsg: glink: unregister rpmsg device during endpoint destroy
Chris Lew (4):
rpmsg: glink: Use complete_all for open states
rpmsg: Guard against null endpoint ops in destroy
rpmsg: glink: Add support for rpmsg glink chrdev
rpmsg: glink: Expose rpmsg name attr for glink
drivers/rpmsg/qcom_glink_native.c | 79 +++++++++++++++++++++++++++++++++++++--
drivers/rpmsg/rpmsg_core.c | 2 +-
2 files changed, 77 insertions(+), 4 deletions(-)
--
2.7.4
From: Chris Lew <[email protected]>
In RPMSG GLINK the chrdev device will allocate an ept as part of the
rpdev creation. This device will not register endpoint ops even though
it has an allocated ept. Protect against the case where the device is
being destroyed.
Signed-off-by: Chris Lew <[email protected]>
Signed-off-by: Arun Kumar Neelakantam <[email protected]>
---
drivers/rpmsg/rpmsg_core.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/rpmsg/rpmsg_core.c b/drivers/rpmsg/rpmsg_core.c
index e330ec4..d6c3275 100644
--- a/drivers/rpmsg/rpmsg_core.c
+++ b/drivers/rpmsg/rpmsg_core.c
@@ -81,7 +81,7 @@ EXPORT_SYMBOL(rpmsg_create_ept);
*/
void rpmsg_destroy_ept(struct rpmsg_endpoint *ept)
{
- if (ept)
+ if (ept && ept->ops)
ept->ops->destroy_ept(ept);
}
EXPORT_SYMBOL(rpmsg_destroy_ept);
--
2.7.4
From: Chris Lew <[email protected]>
The open_req and open_ack completion variables are the state variables
to represet a remote channel as open. Use complete_all so there are no
races with waiters and using completion_done.
Signed-off-by: Chris Lew <[email protected]>
Signed-off-by: Arun Kumar Neelakantam <[email protected]>
---
drivers/rpmsg/qcom_glink_native.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/rpmsg/qcom_glink_native.c b/drivers/rpmsg/qcom_glink_native.c
index 1995f5b..604f11f 100644
--- a/drivers/rpmsg/qcom_glink_native.c
+++ b/drivers/rpmsg/qcom_glink_native.c
@@ -970,7 +970,7 @@ static int qcom_glink_rx_open_ack(struct qcom_glink *glink, unsigned int lcid)
return -EINVAL;
}
- complete(&channel->open_ack);
+ complete_all(&channel->open_ack);
return 0;
}
@@ -1413,7 +1413,7 @@ static int qcom_glink_rx_open(struct qcom_glink *glink, unsigned int rcid,
channel->rcid = ret;
spin_unlock_irqrestore(&glink->idr_lock, flags);
- complete(&channel->open_req);
+ complete_all(&channel->open_req);
if (create_device) {
rpdev = kzalloc(sizeof(*rpdev), GFP_KERNEL);
--
2.7.4
Rpmsg device unregister is not happening if channel close is triggered
from local side and causing re-registration of device failures.
Unregister rpmsg device for local close in endpoint destroy path.
Signed-off-by: Arun Kumar Neelakantam <[email protected]>
---
drivers/rpmsg/qcom_glink_native.c | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/drivers/rpmsg/qcom_glink_native.c b/drivers/rpmsg/qcom_glink_native.c
index 0e8a28c0..fc8ef66 100644
--- a/drivers/rpmsg/qcom_glink_native.c
+++ b/drivers/rpmsg/qcom_glink_native.c
@@ -1207,6 +1207,7 @@ static void qcom_glink_destroy_ept(struct rpmsg_endpoint *ept)
{
struct glink_channel *channel = to_glink_channel(ept);
struct qcom_glink *glink = channel->glink;
+ struct rpmsg_channel_info chinfo;
unsigned long flags;
spin_lock_irqsave(&channel->recv_lock, flags);
@@ -1214,6 +1215,13 @@ static void qcom_glink_destroy_ept(struct rpmsg_endpoint *ept)
spin_unlock_irqrestore(&channel->recv_lock, flags);
/* Decouple the potential rpdev from the channel */
+ if (channel->rpdev) {
+ strncpy(chinfo.name, channel->name, sizeof(chinfo.name));
+ chinfo.src = RPMSG_ADDR_ANY;
+ chinfo.dst = RPMSG_ADDR_ANY;
+
+ rpmsg_unregister_device(glink->dev, &chinfo);
+ }
channel->rpdev = NULL;
qcom_glink_send_close_req(glink, channel);
@@ -1477,6 +1485,7 @@ static void qcom_glink_rx_close(struct qcom_glink *glink, unsigned int rcid)
rpmsg_unregister_device(glink->dev, &chinfo);
}
+ channel->rpdev = NULL;
qcom_glink_send_close_ack(glink, channel->rcid);
--
2.7.4
From: Chris Lew <[email protected]>
Expose the name field as an attr so clients listening to uevents for
rpmsg can identify the edge the events correspond to.
Signed-off-by: Chris Lew <[email protected]>
Signed-off-by: Arun Kumar Neelakantam <[email protected]>
---
drivers/rpmsg/qcom_glink_native.c | 26 ++++++++++++++++++++++++++
1 file changed, 26 insertions(+)
diff --git a/drivers/rpmsg/qcom_glink_native.c b/drivers/rpmsg/qcom_glink_native.c
index 3a7f87c..0e8a28c0 100644
--- a/drivers/rpmsg/qcom_glink_native.c
+++ b/drivers/rpmsg/qcom_glink_native.c
@@ -1574,6 +1574,26 @@ static void qcom_glink_cancel_rx_work(struct qcom_glink *glink)
kfree(dcmd);
}
+static ssize_t rpmsg_name_show(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ int ret = 0;
+ const char *name;
+
+ ret = of_property_read_string(dev->of_node, "label", &name);
+ if (ret < 0)
+ name = dev->of_node->name;
+
+ return snprintf(buf, RPMSG_NAME_SIZE, "%s\n", name);
+}
+static DEVICE_ATTR_RO(rpmsg_name);
+
+static struct attribute *qcom_glink_attrs[] = {
+ &dev_attr_rpmsg_name.attr,
+ NULL
+};
+ATTRIBUTE_GROUPS(qcom_glink);
+
static void qcom_glink_device_release(struct device *dev)
{
struct rpmsg_device *rpdev = to_rpmsg_device(dev);
@@ -1638,6 +1658,12 @@ struct qcom_glink *qcom_glink_native_probe(struct device *dev,
idr_init(&glink->lcids);
idr_init(&glink->rcids);
+ glink->dev->groups = qcom_glink_groups;
+
+ ret = device_add_groups(dev, qcom_glink_groups);
+ if (ret)
+ dev_err(dev, "failed to add groups\n");
+
ret = of_property_read_string(dev->of_node, "label", &glink->name);
if (ret < 0)
glink->name = dev->of_node->name;
--
2.7.4
From: Chris Lew <[email protected]>
RPMSG provides a char device interface to userspace. Probe the rpmsg
chrdev channel to enable the rpmsg_ctrl device creation on glink
transports.
Signed-off-by: Chris Lew <[email protected]>
Signed-off-by: Arun Kumar Neelakantam <[email protected]>
---
drivers/rpmsg/qcom_glink_native.c | 40 ++++++++++++++++++++++++++++++++++++++-
1 file changed, 39 insertions(+), 1 deletion(-)
diff --git a/drivers/rpmsg/qcom_glink_native.c b/drivers/rpmsg/qcom_glink_native.c
index 604f11f..3a7f87c 100644
--- a/drivers/rpmsg/qcom_glink_native.c
+++ b/drivers/rpmsg/qcom_glink_native.c
@@ -1178,7 +1178,7 @@ static int qcom_glink_announce_create(struct rpmsg_device *rpdev)
__be32 *val = defaults;
int size;
- if (glink->intentless)
+ if (glink->intentless || !completion_done(&channel->open_ack))
return 0;
prop = of_find_property(np, "qcom,intents", NULL);
@@ -1574,6 +1574,40 @@ static void qcom_glink_cancel_rx_work(struct qcom_glink *glink)
kfree(dcmd);
}
+static void qcom_glink_device_release(struct device *dev)
+{
+ struct rpmsg_device *rpdev = to_rpmsg_device(dev);
+ struct glink_channel *channel = to_glink_channel(rpdev->ept);
+
+ /* Release qcom_glink_alloc_channel() reference */
+ kref_put(&channel->refcount, qcom_glink_channel_release);
+ kfree(rpdev);
+}
+
+static int qcom_glink_create_chrdev(struct qcom_glink *glink)
+{
+ struct rpmsg_device *rpdev;
+ struct glink_channel *channel;
+
+ rpdev = kzalloc(sizeof(*rpdev), GFP_KERNEL);
+ if (!rpdev)
+ return -ENOMEM;
+
+ channel = qcom_glink_alloc_channel(glink, "rpmsg_chrdev");
+ if (IS_ERR(channel)) {
+ kfree(rpdev);
+ return PTR_ERR(channel);
+ }
+ channel->rpdev = rpdev;
+
+ rpdev->ept = &channel->ept;
+ rpdev->ops = &glink_device_ops;
+ rpdev->dev.parent = glink->dev;
+ rpdev->dev.release = qcom_glink_device_release;
+
+ return rpmsg_chrdev_register_device(rpdev);
+}
+
struct qcom_glink *qcom_glink_native_probe(struct device *dev,
unsigned long features,
struct qcom_glink_pipe *rx,
@@ -1633,6 +1667,10 @@ struct qcom_glink *qcom_glink_native_probe(struct device *dev,
if (ret)
return ERR_PTR(ret);
+ ret = qcom_glink_create_chrdev(glink);
+ if (ret)
+ dev_err(glink->dev, "failed to register chrdev\n");
+
return glink;
}
EXPORT_SYMBOL_GPL(qcom_glink_native_probe);
--
2.7.4
Hi Arun,
On Wed, May 13, 2020 at 10:40:02AM +0530, Arun Kumar Neelakantam wrote:
> From: Chris Lew <[email protected]>
>
> The open_req and open_ack completion variables are the state variables
> to represet a remote channel as open. Use complete_all so there are no
s/represet/represent
> races with waiters and using completion_done.
>
> Signed-off-by: Chris Lew <[email protected]>
> Signed-off-by: Arun Kumar Neelakantam <[email protected]>
> ---
> drivers/rpmsg/qcom_glink_native.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/rpmsg/qcom_glink_native.c b/drivers/rpmsg/qcom_glink_native.c
> index 1995f5b..604f11f 100644
> --- a/drivers/rpmsg/qcom_glink_native.c
> +++ b/drivers/rpmsg/qcom_glink_native.c
> @@ -970,7 +970,7 @@ static int qcom_glink_rx_open_ack(struct qcom_glink *glink, unsigned int lcid)
> return -EINVAL;
> }
>
> - complete(&channel->open_ack);
> + complete_all(&channel->open_ack);
If you do this and as per the note in the comment section above
completion_done(), there shouldn't be a need to call completion_done() in
qcom_glink_announce_create().
Thanks,
Mathieu
>
> return 0;
> }
> @@ -1413,7 +1413,7 @@ static int qcom_glink_rx_open(struct qcom_glink *glink, unsigned int rcid,
> channel->rcid = ret;
> spin_unlock_irqrestore(&glink->idr_lock, flags);
>
> - complete(&channel->open_req);
> + complete_all(&channel->open_req);
>
> if (create_device) {
> rpdev = kzalloc(sizeof(*rpdev), GFP_KERNEL);
> --
> 2.7.4
On Wed, May 13, 2020 at 10:40:04AM +0530, Arun Kumar Neelakantam wrote:
> From: Chris Lew <[email protected]>
>
> RPMSG provides a char device interface to userspace. Probe the rpmsg
> chrdev channel to enable the rpmsg_ctrl device creation on glink
> transports.
>
> Signed-off-by: Chris Lew <[email protected]>
> Signed-off-by: Arun Kumar Neelakantam <[email protected]>
> ---
> drivers/rpmsg/qcom_glink_native.c | 40 ++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 39 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/rpmsg/qcom_glink_native.c b/drivers/rpmsg/qcom_glink_native.c
> index 604f11f..3a7f87c 100644
> --- a/drivers/rpmsg/qcom_glink_native.c
> +++ b/drivers/rpmsg/qcom_glink_native.c
> @@ -1178,7 +1178,7 @@ static int qcom_glink_announce_create(struct rpmsg_device *rpdev)
> __be32 *val = defaults;
> int size;
>
> - if (glink->intentless)
> + if (glink->intentless || !completion_done(&channel->open_ack))
Please move this to patch 01.
> return 0;
>
> prop = of_find_property(np, "qcom,intents", NULL);
> @@ -1574,6 +1574,40 @@ static void qcom_glink_cancel_rx_work(struct qcom_glink *glink)
> kfree(dcmd);
> }
>
> +static void qcom_glink_device_release(struct device *dev)
> +{
> + struct rpmsg_device *rpdev = to_rpmsg_device(dev);
> + struct glink_channel *channel = to_glink_channel(rpdev->ept);
> +
> + /* Release qcom_glink_alloc_channel() reference */
> + kref_put(&channel->refcount, qcom_glink_channel_release);
> + kfree(rpdev);
> +}
> +
> +static int qcom_glink_create_chrdev(struct qcom_glink *glink)
> +{
> + struct rpmsg_device *rpdev;
> + struct glink_channel *channel;
> +
> + rpdev = kzalloc(sizeof(*rpdev), GFP_KERNEL);
> + if (!rpdev)
> + return -ENOMEM;
> +
> + channel = qcom_glink_alloc_channel(glink, "rpmsg_chrdev");
> + if (IS_ERR(channel)) {
> + kfree(rpdev);
> + return PTR_ERR(channel);
> + }
> + channel->rpdev = rpdev;
> +
> + rpdev->ept = &channel->ept;
> + rpdev->ops = &glink_device_ops;
> + rpdev->dev.parent = glink->dev;
> + rpdev->dev.release = qcom_glink_device_release;
> +
> + return rpmsg_chrdev_register_device(rpdev);
> +}
> +
> struct qcom_glink *qcom_glink_native_probe(struct device *dev,
> unsigned long features,
> struct qcom_glink_pipe *rx,
> @@ -1633,6 +1667,10 @@ struct qcom_glink *qcom_glink_native_probe(struct device *dev,
> if (ret)
> return ERR_PTR(ret);
>
> + ret = qcom_glink_create_chrdev(glink);
> + if (ret)
> + dev_err(glink->dev, "failed to register chrdev\n");
> +
> return glink;
> }
> EXPORT_SYMBOL_GPL(qcom_glink_native_probe);
> --
> 2.7.4
On Wed, May 13, 2020 at 10:40:06AM +0530, Arun Kumar Neelakantam wrote:
> Rpmsg device unregister is not happening if channel close is triggered
> from local side and causing re-registration of device failures.
>
> Unregister rpmsg device for local close in endpoint destroy path.
>
> Signed-off-by: Arun Kumar Neelakantam <[email protected]>
> ---
> drivers/rpmsg/qcom_glink_native.c | 9 +++++++++
> 1 file changed, 9 insertions(+)
>
> diff --git a/drivers/rpmsg/qcom_glink_native.c b/drivers/rpmsg/qcom_glink_native.c
> index 0e8a28c0..fc8ef66 100644
> --- a/drivers/rpmsg/qcom_glink_native.c
> +++ b/drivers/rpmsg/qcom_glink_native.c
> @@ -1207,6 +1207,7 @@ static void qcom_glink_destroy_ept(struct rpmsg_endpoint *ept)
> {
> struct glink_channel *channel = to_glink_channel(ept);
> struct qcom_glink *glink = channel->glink;
> + struct rpmsg_channel_info chinfo;
> unsigned long flags;
>
> spin_lock_irqsave(&channel->recv_lock, flags);
> @@ -1214,6 +1215,13 @@ static void qcom_glink_destroy_ept(struct rpmsg_endpoint *ept)
> spin_unlock_irqrestore(&channel->recv_lock, flags);
>
> /* Decouple the potential rpdev from the channel */
> + if (channel->rpdev) {
If we proceed this way no other channel can have an rpdev. I would hope that
unregistration of rpdev would be more symetrical to what is done in patch 03.
Thanks,
Mathieu
> + strncpy(chinfo.name, channel->name, sizeof(chinfo.name));
> + chinfo.src = RPMSG_ADDR_ANY;
> + chinfo.dst = RPMSG_ADDR_ANY;
> +
> + rpmsg_unregister_device(glink->dev, &chinfo);
> + }
> channel->rpdev = NULL;
>
> qcom_glink_send_close_req(glink, channel);
> @@ -1477,6 +1485,7 @@ static void qcom_glink_rx_close(struct qcom_glink *glink, unsigned int rcid)
>
> rpmsg_unregister_device(glink->dev, &chinfo);
> }
> + channel->rpdev = NULL;
>
> qcom_glink_send_close_ack(glink, channel->rcid);
>
> --
> 2.7.4
On 5/14/2020 2:29 AM, Mathieu Poirier wrote:
> Hi Arun,
>
> On Wed, May 13, 2020 at 10:40:02AM +0530, Arun Kumar Neelakantam wrote:
>> From: Chris Lew <[email protected]>
>>
>> The open_req and open_ack completion variables are the state variables
>> to represet a remote channel as open. Use complete_all so there are no
> s/represet/represent
done added in patch set 6
>
>> races with waiters and using completion_done.
>>
>> Signed-off-by: Chris Lew <[email protected]>
>> Signed-off-by: Arun Kumar Neelakantam <[email protected]>
>> ---
>> drivers/rpmsg/qcom_glink_native.c | 4 ++--
>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/rpmsg/qcom_glink_native.c b/drivers/rpmsg/qcom_glink_native.c
>> index 1995f5b..604f11f 100644
>> --- a/drivers/rpmsg/qcom_glink_native.c
>> +++ b/drivers/rpmsg/qcom_glink_native.c
>> @@ -970,7 +970,7 @@ static int qcom_glink_rx_open_ack(struct qcom_glink *glink, unsigned int lcid)
>> return -EINVAL;
>> }
>>
>> - complete(&channel->open_ack);
>> + complete_all(&channel->open_ack);
> If you do this and as per the note in the comment section above
> completion_done(), there shouldn't be a need to call completion_done() in
> qcom_glink_announce_create().
>
> Thanks,
> Mathieu
the completion_done() check still required to avoid sending intent
request on channel which only opened by remote.
>
>
>>
>> return 0;
>> }
>> @@ -1413,7 +1413,7 @@ static int qcom_glink_rx_open(struct qcom_glink *glink, unsigned int rcid,
>> channel->rcid = ret;
>> spin_unlock_irqrestore(&glink->idr_lock, flags);
>>
>> - complete(&channel->open_req);
>> + complete_all(&channel->open_req);
>>
>> if (create_device) {
>> rpdev = kzalloc(sizeof(*rpdev), GFP_KERNEL);
>> --
>> 2.7.4
On 5/14/2020 3:43 AM, Mathieu Poirier wrote:
> On Wed, May 13, 2020 at 10:40:06AM +0530, Arun Kumar Neelakantam wrote:
>> Rpmsg device unregister is not happening if channel close is triggered
>> from local side and causing re-registration of device failures.
>>
>> Unregister rpmsg device for local close in endpoint destroy path.
>>
>> Signed-off-by: Arun Kumar Neelakantam <[email protected]>
>> ---
>> drivers/rpmsg/qcom_glink_native.c | 9 +++++++++
>> 1 file changed, 9 insertions(+)
>>
>> diff --git a/drivers/rpmsg/qcom_glink_native.c b/drivers/rpmsg/qcom_glink_native.c
>> index 0e8a28c0..fc8ef66 100644
>> --- a/drivers/rpmsg/qcom_glink_native.c
>> +++ b/drivers/rpmsg/qcom_glink_native.c
>> @@ -1207,6 +1207,7 @@ static void qcom_glink_destroy_ept(struct rpmsg_endpoint *ept)
>> {
>> struct glink_channel *channel = to_glink_channel(ept);
>> struct qcom_glink *glink = channel->glink;
>> + struct rpmsg_channel_info chinfo;
>> unsigned long flags;
>>
>> spin_lock_irqsave(&channel->recv_lock, flags);
>> @@ -1214,6 +1215,13 @@ static void qcom_glink_destroy_ept(struct rpmsg_endpoint *ept)
>> spin_unlock_irqrestore(&channel->recv_lock, flags);
>>
>> /* Decouple the potential rpdev from the channel */
>> + if (channel->rpdev) {
> If we proceed this way no other channel can have an rpdev. I would hope that
> unregistration of rpdev would be more symetrical to what is done in patch 03.
>
> Thanks,
> Mathieu
Unregister here also required along with in qcom_glink_rx_close()
otherwise if the remote open the channel again it map to stale rpmsg
device.
>
>> + strncpy(chinfo.name, channel->name, sizeof(chinfo.name));
>> + chinfo.src = RPMSG_ADDR_ANY;
>> + chinfo.dst = RPMSG_ADDR_ANY;
>> +
>> + rpmsg_unregister_device(glink->dev, &chinfo);
>> + }
>> channel->rpdev = NULL;
>>
>> qcom_glink_send_close_req(glink, channel);
>> @@ -1477,6 +1485,7 @@ static void qcom_glink_rx_close(struct qcom_glink *glink, unsigned int rcid)
>>
>> rpmsg_unregister_device(glink->dev, &chinfo);
>> }
>> + channel->rpdev = NULL;
>>
>> qcom_glink_send_close_ack(glink, channel->rcid);
>>
>> --
>> 2.7.4