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

[James Smart <jsmart2021@gmail.com>]

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.