On 4/15/2020 3:04 AM, Daniel Wagner wrote: ...
+static void +__sli_queue_destroy(struct sli4 *sli4, struct sli4_queue *q) +{ + if (!q->dma.size) + return; + + dma_free_coherent(&sli4->pcidev->dev, q->dma.size, + q->dma.virt, q->dma.phys); + memset(&q->dma, 0, sizeof(struct efc_dma));Is this necessary to clear q->dma? Just asking if it's possible to avoid the additional work.
unfortunately, yes - at least q->dma.size must be cleared. It's used to detect validity (must be non-0).
...
+ q->dma.size = size * n_entries; + q->dma.virt = dma_alloc_coherent(&sli4->pcidev->dev, + q->dma.size, &q->dma.phys, + GFP_DMA); + if (!q->dma.virt) { + memset(&q->dma, 0, sizeof(struct efc_dma));So if __sli_queue_destroy() keeps clearing q->dma, than this one can go away, since if __sli_queue_init() fails __sli_queue_destroy() will be called.
Well, this is the same thing - with q->dma.size being set to 0 so the dma_free_coherent() is avoided in the destroy call.
...
+sli_mq_write(struct sli4 *sli4, struct sli4_queue *q, + u8 *entry) +{ + u8 *qe = q->dma.virt; + u32 qindex; + u32 val = 0; + unsigned long flags; + + spin_lock_irqsave(&q->lock, flags); + qindex = q->index; + qe += q->index * q->size; + + memcpy(qe, entry, q->size); + q->n_posted = 1;Shouldn't this be q->n_posted++ ? Or is it garanteed n_posted is 0?
yes - we post 1 at a time. ...
+sli_rq_write(struct sli4 *sli4, struct sli4_queue *q, + u8 *entry) +{ + u8 *qe = q->dma.virt; + u32 qindex, n_posted; + u32 val = 0; + + qindex = q->index; + qe += q->index * q->size; + + memcpy(qe, entry, q->size); + q->n_posted = 1;Again why not q->n_posted++ ?
Same thing.
I am confused why no lock is used here and in the fuction above. A few words on the locking desing would be highly appreciated.
Q lock is held while calling this function. There were comments in the caller. Will add one here.
Agree with the rest of the comments and will address. -- james
