Thanks Bjorn for the review comments.
On 28/04/18 05:51, Bjorn Andersson wrote:
On Thu 26 Apr 02:45 PDT 2018, Srinivas Kandagatla wrote:
diff --git a/drivers/soc/qcom/apr.c b/drivers/soc/qcom/apr.c
[..]
+int apr_send_pkt(struct apr_device *adev, void *buf)
Sorry, but I think we have discussed this before?
Yes, I did mention that I would give it a try and see, This change was
pretty intrusive when I last looked at this.
I agree with you on asymmetry! I will change this and add struc apr_pkt
which would apr_hdr followed by payload. This should also work for
callback as well.
"buf" isn't some random buffer to be sent, it is a apr_hdr followed by
some data. As such I think you should make this type struct apr_hdr *,
or if you think that doesn't imply there's payload make a type apr_pkt
that has a payload[] at the end.
It will make it obvious for both future readers and the compiler what
kind of data we're passing here.
This comment also applies to functions calling functions that calls
apr_send_pkt() as they too lug around a void *.
+{
+ struct apr *apr = dev_get_drvdata(adev->dev.parent);
+ struct apr_hdr *hdr;
+ unsigned long flags;
+ int ret;
+
+ spin_lock_irqsave(&adev->lock, flags);
+
+ hdr = (struct apr_hdr *)buf;
+ hdr->src_domain = APR_DOMAIN_APPS;
+ hdr->src_svc = adev->svc_id;
+ hdr->dest_domain = adev->domain_id;
+ hdr->dest_svc = adev->svc_id;
+
+ ret = rpmsg_trysend(apr->ch, buf, hdr->pkt_size);
+ if (ret) {
+ dev_err(&adev->dev, "Unable to send APR pkt %d\n",
+ hdr->pkt_size);
Afaict all callers of this function will print an error message,
sometimes on more than one level in the stack. And if some code path
does retry sending you will get a printout for each attempt, even though
the caller is fine with it.
I would recommend unlocking the spinlock and then do:
I can do that !!
return ret ? : hdr->pkt_size;
+ } else {
+ ret = hdr->pkt_size;
+ }
+
+ spin_unlock_irqrestore(&adev->lock, flags);
+
+ return ret;
+}
+EXPORT_SYMBOL_GPL(apr_send_pkt);
+
+
+static int apr_callback(struct rpmsg_device *rpdev, void *buf,
+ int len, void *priv, u32 addr)
+{
+ struct apr *apr = dev_get_drvdata(&rpdev->dev);
+ struct apr_client_message data;
+ struct apr_device *p, *c_svc = NULL;
+ struct apr_driver *adrv = NULL;
+ struct apr_hdr *hdr;
+ unsigned long flags;
+ uint16_t hdr_size;
+ uint16_t msg_type;
+ uint16_t ver;
+ uint16_t svc;
+
+ if (len <= APR_HDR_SIZE) {
+ dev_err(apr->dev, "APR: Improper apr pkt received:%p %d\n",
+ buf, len);
+ return -EINVAL;
+ }
+
+ hdr = buf;
+ ver = APR_HDR_FIELD_VER(hdr->hdr_field);
+ if (ver > APR_PKT_VER + 1)
+ return -EINVAL;
+
+ hdr_size = APR_HDR_FIELD_SIZE_BYTES(hdr->hdr_field);
+ if (hdr_size < APR_HDR_SIZE) {
+ dev_err(apr->dev, "APR: Wrong hdr size:%d\n", hdr_size);
+ return -EINVAL;
+ }
+
+ if (hdr->pkt_size < APR_HDR_SIZE) {
I think it would be nice to make sure that hdr->pkt_size is < len as
well, to reject messages that larger than the incoming buffer.
The pkt_size should be in the ballpark of len, could this check be
changed to hdr->pkt_size != len?
Yep, It makes sense, I can add that check here.
+ dev_err(apr->dev, "APR: Wrong paket size\n");
+ return -EINVAL;
+ }
+
+ msg_type = APR_HDR_FIELD_MT(hdr->hdr_field);
+ if (msg_type >= APR_MSG_TYPE_MAX && msg_type != APR_BASIC_RSP_RESULT) {
+ dev_err(apr->dev, "APR: Wrong message type: %d\n", msg_type);
+ return -EINVAL;
+ }
+
+ if (hdr->src_domain >= APR_DOMAIN_MAX ||
+ hdr->dest_domain >= APR_DOMAIN_MAX ||
+ hdr->src_svc >= APR_SVC_MAX ||
+ hdr->dest_svc >= APR_SVC_MAX) {
+ dev_err(apr->dev, "APR: Wrong APR header\n");
+ return -EINVAL;
+ }
+
+ svc = hdr->dest_svc;
+ spin_lock_irqsave(&apr->svcs_lock, flags);
+ list_for_each_entry(p, &apr->svcs, node) {
Rather than doing a O(n) search for the svc with svc_id you could use a
idr or a radix_tree to keep the id -> svc mapping.
Am not 100% sure idr is correct thing here, as the svc_ids are static
and the list we are talking here in worst case would be max of 13
entires, in audio case it is just 4 services.
I think using radix_tree would be over do.
+ if (svc == p->svc_id) {
+ c_svc = p;
+ if (c_svc->dev.driver)
+ adrv = to_apr_driver(c_svc->dev.driver);
+ break;
+ }
+ }
+ spin_unlock_irqrestore(&apr->svcs_lock, flags);
+
+ if (!adrv) {
+ dev_err(apr->dev, "APR: service is not registered\n");
+ return -EINVAL;
+ }
+
+ data.payload_size = hdr->pkt_size - hdr_size;
+ data.opcode = hdr->opcode;
+ data.src_port = hdr->src_port;
+ data.dest_port = hdr->dest_port;
+ data.token = hdr->token;
+ data.msg_type = msg_type;
+
+ if (data.payload_size > 0)
+ data.payload = buf + hdr_size;
+
Making a verbatim copy of parts of the hdr and then passing that to the
APR devices creates an asymmetry in the send/callback API, without a
whole lot of benefits. I would prefer that you introduce the apr_pkt, as
described above, and once you have validated the size et al and found
the service you just pass it along.
Okay, this would be a big rework, I will do it in next version.
+ adrv->callback(c_svc, &data);
+
+ return 0;
+}
[..]
+static int apr_uevent(struct device *dev, struct kobj_uevent_env *env)
+{
+ struct apr_device *adev = to_apr_device(dev);
+ int ret;
+
+ ret = of_device_uevent_modalias(dev, env);
+ if (ret != -ENODEV)
+ return ret;
+
+ return add_uevent_var(env, "MODALIAS= apr:%s", adev->name);
No space between '=' and "apr".
Yep.
+}
+
+struct bus_type aprbus = {
+ .name = "aprbus",
Most busses doesn't have "bus" in the name.
Yep, just "apr" make sense.
+ .match = apr_device_match,
+ .probe = apr_device_probe,
+ .uevent = apr_uevent,
+ .remove = apr_device_remove,
+};
+EXPORT_SYMBOL_GPL(aprbus);
+
+static int apr_add_device(struct device *dev, struct device_node *np,
+ const struct apr_device_id *id)
+{
+ struct apr *apr = dev_get_drvdata(dev);
+ struct apr_device *adev = NULL;
+
+ adev = kzalloc(sizeof(*adev), GFP_KERNEL);
+ if (!adev)
+ return -ENOMEM;
+
+ spin_lock_init(&adev->lock);
+
+ adev->svc_id = id->svc_id;
+ adev->domain_id = id->domain_id;
+ adev->version = id->svc_version;
+ if (np)
+ strncpy(adev->name, np->name, APR_NAME_SIZE);
+ else
+ strncpy(adev->name, id->name, APR_NAME_SIZE);
+
+ dev_set_name(&adev->dev, "aprsvc:%s:%x:%x", adev->name,
+ id->domain_id, id->svc_id);
+
+ adev->dev.bus = &aprbus;
+ adev->dev.parent = dev;
+ adev->dev.of_node = np;
+ adev->dev.release = apr_dev_release;
+ adev->dev.driver = NULL;
+
+ spin_lock(&apr->svcs_lock);
+ list_add_tail(&adev->node, &apr->svcs);
+ spin_unlock(&apr->svcs_lock);
+
+ dev_info(dev, "Adding APR dev: %s\n", dev_name(&adev->dev));
+
+ return device_register(&adev->dev);
If this fails you must call put_device(&adev->dev);
Agree!!
+}
+
+static void of_register_apr_devices(struct device *dev)
+{
+ struct apr *apr = dev_get_drvdata(dev);
+ struct device_node *node;
+
+ for_each_child_of_node(dev->of_node, node) {
+ struct apr_device_id id = { {0} };
+
+ if (of_property_read_u32(node, "reg", &id.svc_id))
+ continue;
+
+ id.domain_id = apr->dest_domain_id;
+
+ if (apr_add_device(dev, node, &id))
+ dev_err(dev, "Failed to add arp %d svc\n", id.svc_id);
+ }
+}
[..]
+
+static int __init apr_init(void)
+{
+ int ret;
+
+ ret = bus_register(&aprbus);
+ if (!ret)
+ ret = register_rpmsg_driver(&apr_driver);
bus_unregister() if ret here.
Yep.
Thanks,
srini
+
+ return ret;
+}
+
Regards,
Bjorn
--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
