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

[Srinivas Kandagatla <srinivas.kandagatla@linaro.org>]

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