Re: [PATCH 02/32] elx: libefc_sli: SLI Descriptors and Queue entries

[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 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.