Re: [PATCH v3 04/31] elx: libefc_sli: queue create/destroy/parse routines

James Smart
2020-04-22T05:05:32Z

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