Re: [PATCH v3 16/31] elx: efct: Driver initialization routines

James Smart
2020-04-23T03:09:41Z

On 4/16/2020 12:11 AM, Hannes Reinecke wrote:
...
>> +    if (efct) {
>> +        memset(efct, 0, sizeof(*efct));
>> +        for (i = 0; i < ARRAY_SIZE(efct_devices); i++) {
>> +            if (!efct_devices[i]) {
>> +                efct->instance_index = i;
>> +                efct_devices[i] = efct;
>> +                break;
>> +            }
>> +        }
>> +
>> +        if (i == ARRAY_SIZE(efct_devices)) {
>> +            pr_err("Exceeded max supported devices.\n");
>> +            kfree(efct);
>> +            efct = NULL;
>> +        } else {
>> +            efct->attached = false;
>> +        }
>
> Errm. This is wrong.
> When we exit the for() loop above, i _will_ equal the array size.
> Surely you mean
>
> if (i < ARRAY_SIZE())
>
> right?
>

No. We want to free the structure if we went through the array and
didn't find an empty slot. It only breaks from the loop if it found an
empty slot and used it.

>> +        } else if (event->topology == SLI_LINK_TOPO_LOOP) {
>> +            u8    *buf = NULL;
>> +
>> +            efc_log_info(hw->os, "Link Up, LOOP, speed is %d\n",
>> +                      event->speed);
>> +            dma = &hw->loop_map;
>> +            dma->size = SLI4_MIN_LOOP_MAP_BYTES;
>> +            dma->virt = dma_alloc_coherent(&efct->pcidev->dev,
>> +                               dma->size, &dma->phys,
>> +                               GFP_DMA);
>> +            if (!dma->virt)
>> +                efc_log_err(hw->os, "efct_dma_alloc_fail\n");
>> +
>> +            buf = kmalloc(SLI4_BMBX_SIZE, GFP_ATOMIC);
>> +            if (!buf)
>> +                break;
>> +
>> +            if (!sli_cmd_read_topology(&hw->sli, buf,
>> +                          SLI4_BMBX_SIZE,
>> +                               &hw->loop_map)) {
>> +                rc = efct_hw_command(hw, buf, EFCT_CMD_NOWAIT,
>> +                             __efct_read_topology_cb,
>> +                             NULL);
>> +            }
>> +
>
> Not sure if this is a good idea; we'll have to allocate extra memory
> whenever the loop topology changes.
> Which typically happens when there had been a failure somewhere, and
> chances are that it'll affect our root fs, making memory allocation
> something we really need to look at.
> Can't we pre-allocate a buffer here somewhere in the global
> initialisation so that we don't have to allocate it every time?

Agree. will look into it.

...
>
> What happened to multiqueue support?
> The original lpfc driver did it, so we should regress for the new driver...

For a target driver ?

When something like this is in the tgt infrastructure we'll do something
with it.

-- james