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; +} +
