Re: [PATCH v2 03/16] soc: qcom: apr: Add GPR support

[Srinivas Kandagatla]

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