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

James Smart
2019-10-25T22:24:51Z

Thanks. We mostly agree with the comment written and will work on the
changes.

Exceptions or answers to questions are inline below.

-- james

On 10/25/2019 8:35 AM, Daniel Wagner wrote:
>> +static void *
>> +sli_config_cmd_init(struct sli4_s *sli4, void *buf,
>> + size_t size, u32 length,
>> + struct efc_dma_s *dma)
>> +{
>> + struct sli4_cmd_sli_config_s *sli_config = NULL;
>> + u32 flags = 0;
>> +
>> + if (length > sizeof(sli_config->payload.embed) && !dma) {
>> + efc_log_info(sli4, "length(%d) > payload(%ld)\n",
>> + length, sizeof(sli_config->payload.embed));
>> + return NULL;
>> + }
>
> ...this logs something but what does it tell? I suppose it has
> something to do if a data are embedded or not.

yep - if its too big to be embedded and there's isn't a dma address to
use for non-embedded format, it's an error. We will make that log
message reflect what I just said.

>> + cqv2->hdr.opcode = CMN_CREATE_CQ;
>> + cqv2->hdr.subsystem = SLI4_SUBSYSTEM_COMMON;
>> + cqv2->hdr.dw3_version = cpu_to_le32(CMD_V2);
>
> Is this now a the command version? Shouldn't it be V0 as the
> documentation writes?

nope comment was wrong. We'll remove the comment. We won't bother with
routine names reflecting cmd version # unless the driver has to use more
than 1 version.

>> +static int
>> +sli_cmd_common_destroy_cq(struct sli4_s *sli4, void *buf,
>> + size_t size, u16 cq_id)
>> +{
>> + struct sli4_rqst_cmn_destroy_cq_s *cq = NULL;
>> +
>> + /* Payload length must accommodate both request and response */
>
> Is this common? Is this true for all commands? If so maybe have this
> kind of information at the beginning of the file explaining some of
> the inner workings of the code would certainly help.

For the SLI_CONFIG mailbox command, which is a wrapper that issues a
bunch of other mailbox commands specified by subsystem and
subsystem-specific opcode - yes, it's true.

We'll clean this up. Likely remove the indicated comment and say
something up in sli_config_cmd_init().

> sli_cmd_common_destroy_eq(), sli_cmd_common_destroy_cq() and
> sli_cmd_common_destroy_mq() look almost identically. Could those
> function be unified?

We'll look at better commonizing through small service routines and/or
macros. We'll see if unification falls out.

> So many function look almost identical. Is there no better way to
> create the commands? Or is something like a generic command creation
> function worse to maintain? There is so much copy paste... I stop now
> pointing out the same issues again.

Same as last comment. A few helper macros should distill it to the items
that are specific to the individual commands.