lkml.org 
[lkml]   [2021]   [Jul]   [15]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH v2 03/16] soc: qcom: apr: Add GPR support
From
Date
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;
>> +}
>> +
>

\
 
 \ /
  Last update: 2021-07-15 12:32    [W:0.060 / U:0.740 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site