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

[Hannes Reinecke <hare@suse.de>]

On 12/20/19 11:36 PM, James Smart wrote:
> This patch continues the libefc_sli SLI-4 library population.
> 
> This patch add SLI-4 Data structures and defines for:
> - Buffer Descriptors (BDEs)
> - Scatter/Gather List elements (SGEs)
> - Queues and their Entry Descriptions for:
>    Event Queues (EQs), Completion Queues (CQs),
>    Receive Queues (RQs), and the Mailbox Queue (MQ).
> 
> Signed-off-by: Ram Vegesna <ram.vegesna@broadcom.com>
> Signed-off-by: James Smart <jsmart2021@gmail.com>
> ---
>  drivers/scsi/elx/include/efc_common.h |   25 +
>  drivers/scsi/elx/libefc_sli/sli4.h    | 1768 +++++++++++++++++++++++++++++++++
>  2 files changed, 1793 insertions(+)
>  create mode 100644 drivers/scsi/elx/include/efc_common.h
> 
> diff --git a/drivers/scsi/elx/include/efc_common.h b/drivers/scsi/elx/include/efc_common.h
> new file mode 100644
> index 000000000000..3fc48876c531
> --- /dev/null
> +++ b/drivers/scsi/elx/include/efc_common.h
> @@ -0,0 +1,25 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Copyright (C) 2019 Broadcom. All Rights Reserved. The term
> + * “Broadcom” refers to Broadcom Inc. and/or its subsidiaries.
> + */
> +
> +#ifndef __EFC_COMMON_H__
> +#define __EFC_COMMON_H__
> +
> +#include <linux/pci.h>
> +
> +#define EFC_SUCCESS 0
> +#define EFC_FAIL 1
> +
> +struct efc_dma {
> +	void		*virt;
> +	void            *alloc;
> +	dma_addr_t	phys;
> +
> +	size_t		size;
> +	size_t          len;
> +	struct pci_dev	*pdev;
> +};
> +
> +#endif /* __EFC_COMMON_H__ */
> diff --git a/drivers/scsi/elx/libefc_sli/sli4.h b/drivers/scsi/elx/libefc_sli/sli4.h
> index 02c671cf57ef..f86a9e72ed43 100644
> --- a/drivers/scsi/elx/libefc_sli/sli4.h
> +++ b/drivers/scsi/elx/libefc_sli/sli4.h
> @@ -12,6 +12,8 @@
>  #ifndef _SLI4_H
>  #define _SLI4_H
>  
> +#include "../include/efc_common.h"
> +
>  /*************************************************************************
>   * Common SLI-4 register offsets and field definitions
>   */
> @@ -236,4 +238,1770 @@ struct sli4_reg {
>  	u32	off;
>  };
>  
> +struct sli4_dmaaddr {
> +	__le32 low;
> +	__le32 high;
> +};
> +
> +/* a 3-word BDE with address 1st 2 words, length last word */
> +struct sli4_bufptr {
> +	struct sli4_dmaaddr addr;
> +	__le32 length;
> +};
> +
> +/* a 3-word BDE with length as first word, address last 2 words */
> +struct sli4_bufptr_len1st {
> +	__le32 length0;
> +	struct sli4_dmaaddr addr;
> +};
> +
> +/* Buffer Descriptor Entry (BDE) */
> +enum {
> +	SLI4_BDE_MASK_BUFFER_LEN	= 0x00ffffff,
> +	SLI4_BDE_MASK_BDE_TYPE		= 0xff000000,
> +};
> +
> +struct sli4_bde {
> +	__le32		bde_type_buflen;
> +	union {
> +		struct sli4_dmaaddr data;
> +		struct {
> +			__le32	offset;
> +			__le32	rsvd2;
> +		} imm;
> +		struct sli4_dmaaddr blp;
> +	} u;
> +};
> +
> +/* Buffer Descriptors */
> +enum {
> +	BDE_TYPE_SHIFT		= 24,
> +	BDE_TYPE_BDE_64		= 0x00,	/* Generic 64-bit data */
> +	BDE_TYPE_BDE_IMM	= 0x01,	/* Immediate data */
> +	BDE_TYPE_BLP		= 0x40,	/* Buffer List Pointer */
> +};
> +
> +/* Scatter-Gather Entry (SGE) */
> +#define SLI4_SGE_MAX_RESERVED			3
> +
> +enum {
> +	/* DW2 */
> +	SLI4_SGE_DATA_OFFSET_MASK	= 0x07FFFFFF,
> +	/*DW2W1*/
> +	SLI4_SGE_TYPE_SHIFT		= 27,
> +	SLI4_SGE_TYPE_MASK		= 0xf << SLI4_SGE_TYPE_SHIFT,
> +	/*SGE Types*/
> +	SLI4_SGE_TYPE_DATA		= 0x00,
> +	SLI4_SGE_TYPE_DIF		= 0x04,	/* Data Integrity Field */
> +	SLI4_SGE_TYPE_LSP		= 0x05,	/* List Segment Pointer */
> +	SLI4_SGE_TYPE_PEDIF		= 0x06,	/* Post Encryption Engine DIF */
> +	SLI4_SGE_TYPE_PESEED		= 0x07,	/* Post Encryption DIF Seed */
> +	SLI4_SGE_TYPE_DISEED		= 0x08,	/* DIF Seed */
> +	SLI4_SGE_TYPE_ENC		= 0x09,	/* Encryption */
> +	SLI4_SGE_TYPE_ATM		= 0x0a,	/* DIF Application Tag Mask */
> +	SLI4_SGE_TYPE_SKIP		= 0x0c,	/* SKIP */
> +
> +	SLI4_SGE_LAST			= (1 << 31),
> +};
> +
> +struct sli4_sge {
> +	__le32		buffer_address_high;
> +	__le32		buffer_address_low;
> +	__le32		dw2_flags;
> +	__le32		buffer_length;
> +};
> +
I am really not a big fan of anonymous enums, especially not if they are
scoped for specific structures.
Can you please avoid the use of anonymous enums, and name them according
to the structure where they are indended to be used?
Ideally the structure should reference named enums directly, but I do
agree that this it not always possible or desired.
But we should at least name them accordingly to give the developer a
hint where these values are expected to occur.

Eg in the above case

enum sli4_sge_flags {

or similar would make the intended usage clearer.

> +/* T10 DIF Scatter-Gather Entry (SGE) */
> +struct sli4_dif_sge {
> +	__le32		buffer_address_high;
> +	__le32		buffer_address_low;
> +	__le32		dw2_flags;
> +	__le32		rsvd12;
> +};
> +
> +/* Data Integrity Seed (DISEED) SGE */
> +enum {
> +	/* DW2W1 */
> +	DISEED_SGE_HS			= (1 << 2),
> +	DISEED_SGE_WS			= (1 << 3),
> +	DISEED_SGE_IC			= (1 << 4),
> +	DISEED_SGE_ICS			= (1 << 5),
> +	DISEED_SGE_ATRT			= (1 << 6),
> +	DISEED_SGE_AT			= (1 << 7),
> +	DISEED_SGE_FAT			= (1 << 8),
> +	DISEED_SGE_NA			= (1 << 9),
> +	DISEED_SGE_HI			= (1 << 10),
> +
> +	/* DW3W1 */
> +	DISEED_SGE_BS_MASK		= 0x0007,
> +	DISEED_SGE_AI			= (1 << 3),
> +	DISEED_SGE_ME			= (1 << 4),
> +	DISEED_SGE_RE			= (1 << 5),
> +	DISEED_SGE_CE			= (1 << 6),
> +	DISEED_SGE_NR			= (1 << 7),
> +
> +	DISEED_SGE_OP_RX_SHIFT		= 8,
> +	DISEED_SGE_OP_RX_MASK		= (0xf << DISEED_SGE_OP_RX_SHIFT),
> +	DISEED_SGE_OP_TX_SHIFT		= 12,
> +	DISEED_SGE_OP_TX_MASK		= (0xf << DISEED_SGE_OP_TX_SHIFT),
> +
> +	/* Opcode values */
> +	DISEED_SGE_OP_IN_NODIF_OUT_CRC	= 0x00,
> +	DISEED_SGE_OP_IN_CRC_OUT_NODIF	= 0x01,
> +	DISEED_SGE_OP_IN_NODIF_OUT_CSUM	= 0x02,
> +	DISEED_SGE_OP_IN_CSUM_OUT_NODIF	= 0x03,
> +	DISEED_SGE_OP_IN_CRC_OUT_CRC	= 0x04,
> +	DISEED_SGE_OP_IN_CSUM_OUT_CSUM	= 0x05,
> +	DISEED_SGE_OP_IN_CRC_OUT_CSUM	= 0x06,
> +	DISEED_SGE_OP_IN_CSUM_OUT_CRC	= 0x07,
> +	DISEED_SGE_OP_IN_RAW_OUT_RAW	= 0x08,
> +
> +};
> +
Similar here: please use individual named enums, not one giant anonymous
enum containing different value for different use-cases.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      Teamlead Storage & Networking
hare@suse.de			                  +49 911 74053 688
SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), GF: Felix Imendörffer