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.
