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 2:59 AM, Daniel Wagner wrote:
I noticed sometimes there are also BIT() used. Wouldn't it make sense
to the whole driver to use one or the other version of bit
definitions?
We don't want to have BIT() used. Any references will be removed.
+
+/**
+ * @brief Generic Common Create EQ/CQ/MQ/WQ/RQ Queue completion
+ */
+struct sli4_rsp_cmn_create_queue_s {
+ struct sli4_rsp_hdr_s hdr;
+ __le16 q_id;
+ u8 rsvd18;
+ u8 ulp;
+ __le32 db_offset;
+ __le16 db_rs;
+ __le16 db_fmt;
+};
Just wondering about all these definitions here: These structs
describes the wire format, no? Shouldn't this marked with __packed? I
keep forgetting the rules.
not wire format, but rather the endianness of the adapter interface.
yes, it's probably good practice to use __packed. The existing
definitions should have been ok as the layouts should never have created
a condition where pad would have been added. but... better safe than sorry.
Picking up my question from patch #1, what's the idea about the enums
and defines? Why are the last two ones not an enum?
Well, its a code volume issue. We migrated old code which was mostly
defines. When close attention was made to properly code for endianness
in register definitions and things at the lower interfaces, we used
enums. Some things changed while others didn't. Upon conclusion, we
saw a large amount of both and it is a lot of work for no technical gain
and limited readability gain to make them all one way or the other.
I asked around as to whether we must be all one type or not and there's
not a mandate to be one or the other or even specifically when to do
what. So we've stuck with what we have.
+/**
+ * @brief WQ_CREATE
+ *
+ * Create a Work Queue for FC.
+ */
+#define SLI4_WQ_CREATE_V0_MAX_PAGES 4
+struct sli4_rqst_wq_create_s {
+ struct sli4_rqst_hdr_s hdr;
+ u8 num_pages;
+ u8 dua_byte;
+ __le16 cq_id;
+ struct sli4_dmaaddr_s page_phys_addr[SLI4_WQ_CREATE_V0_MAX_PAGES];
+ u8 bqu_byte;
+ u8 ulp;
+ __le16 rsvd;
+};
+
+struct sli4_rsp_wq_create_s {
+ struct sli4_rsp_cmn_create_queue_s q_rsp;
+};
+
+/**
+ * @brief WQ_CREATE_V1
+ *
+ * Create a version 1 Work Queue for FC use.
+ */
Why does the workqueue code encode a version? Isn't this pure driver
code?
The same command can have multiple forms. yes, there's no need to be
calling out the version if they are the same between versions. We'll fix
this.
