Many thanks Pierre for the review,
On 14/07/2021 17:20, Pierre-Louis Bossart wrote:
+void gpr_free_port(gpr_port_t *port) +{
- struct packet_router *gpr = port->pr;
- unsigned long flags;
- spin_lock_irqsave(&gpr->svcs_lock, flags);
- idr_remove(&gpr->svcs_idr, port->id);
- spin_unlock_irqrestore(&gpr->svcs_lock, flags);
maybe add a comment somewhere on why you need the irqsave/irqrestore version of spin_lock/unlock?
All the responses to the messages arrive in interrupt context which could try to find the matching svc idr, so we needed this irq version of spinlock here. I did move this handling to a work queue in the past, so we should be able to relax this lock to non-irq version.
It's not very clear by looking at this patch only that those locks are handled in interrupt context.
- kfree(port);
+} +EXPORT_SYMBOL_GPL(gpr_free_port);
+gpr_port_t *gpr_alloc_port(struct apr_device *gdev, struct device *dev,
gpr_port_cb cb, void *priv)+{
- struct packet_router *pr = dev_get_drvdata(gdev->dev.parent);
- gpr_port_t *port;
- struct pkt_router_svc *svc;
- int id;
- port = kzalloc(sizeof(*port), GFP_KERNEL);
- if (!port)
return ERR_PTR(-ENOMEM);- svc = port;
- svc->callback = cb;
- svc->pr = pr;
- svc->priv = priv;
- svc->dev = dev;
- spin_lock_init(&svc->lock);
- spin_lock(&pr->svcs_lock);
- id = idr_alloc_cyclic(&pr->svcs_idr, svc, GPR_DYNAMIC_PORT_START,
GPR_DYNAMIC_PORT_END, GFP_ATOMIC);- if (id < 0) {
dev_err(dev, "Unable to allocate dynamic GPR src port\n");kfree(port);spin_unlock(&pr->svcs_lock);nit-pick: more this before the dev_err?
I agree, will do that in next spin.
return ERR_PTR(-ENOMEM);- }
- svc->id = id;
- spin_unlock(&pr->svcs_lock);
- dev_info(dev, "Adding GPR src port (%x)\n", svc->id);
- return port;
+}
+static int gpr_do_rx_callback(struct packet_router *gpr, struct apr_rx_buf *abuf) +{
- uint16_t hdr_size, ver;
- struct pkt_router_svc *svc = NULL;
unnecessary init? it's overritten...
Yep.
--srini
- struct gpr_resp_pkt resp;
- struct gpr_hdr *hdr;
- unsigned long flags;
- void *buf = abuf->buf;
- int len = abuf->len;
- hdr = buf;
- ver = hdr->version;
- if (ver > GPR_PKT_VER + 1)
return -EINVAL;- hdr_size = hdr->hdr_size;
- if (hdr_size < GPR_PKT_HEADER_WORD_SIZE) {
dev_err(gpr->dev, "GPR: Wrong hdr size:%d\n", hdr_size);return -EINVAL;- }
- if (hdr->pkt_size < GPR_PKT_HEADER_BYTE_SIZE || hdr->pkt_size != len) {
dev_err(gpr->dev, "GPR: Wrong packet size\n");return -EINVAL;- }
- resp.hdr = *hdr;
- resp.payload_size = hdr->pkt_size - (hdr_size * 4);
- /*
* NOTE: hdr_size is not same as GPR_HDR_SIZE as remote can include* optional headers in to gpr_hdr which should be ignored*/- if (resp.payload_size > 0)
resp.payload = buf + (hdr_size * 4);- spin_lock_irqsave(&gpr->svcs_lock, flags);
- svc = idr_find(&gpr->svcs_idr, hdr->dest_port);
... here
- spin_unlock_irqrestore(&gpr->svcs_lock, flags);
- if (!svc) {
dev_err(gpr->dev, "GPR: Port(%x) is not registered\n",hdr->dest_port);return -EINVAL;- }
- if (svc->callback)
svc->callback(&resp, svc->priv, 0);- return 0;
+}