Messages in this thread |  | | Subject | Re: [PATCH v6 02/24] soc: qcom: Add APR bus driver | From | Srinivas Kandagatla <> | Date | Sat, 28 Apr 2018 12:12:51 +0100 |
| |
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 >
|  |