Messages in this thread | | | Subject | Re: [PATCH v2 03/16] soc: qcom: apr: Add GPR support | From | Srinivas Kandagatla <> | Date | Thu, 15 Jul 2021 11:31:45 +0100 |
| |
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; >> +} >> + >
| |