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