lkml.org 
[lkml]   [2018]   [Apr]   [28]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH v6 02/24] soc: qcom: Add APR bus driver
From
Date
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
>

\
 
 \ /
  Last update: 2018-04-28 13:13    [W:0.153 / U:0.056 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site