RE: [PATCH 01/24] mpi3mr: add mpi30 Rev-R headers and Kconfig

[Kashyap Desai <kashyap.desai@broadcom.com>]

[-- Attachment #1: Type: text/plain, Size: 8238 bytes --]

> > +++ b/drivers/scsi/mpi3mr/mpi/mpi30_api.h
> > @@ -0,0 +1,21 @@
> > +/* SPDX-License-Identifier: GPL-2.0-or-later */
> > +/*
> > + *  Copyright 2019-2020 Broadcom Inc. All rights reserved.
> > + *
>
> 2020? Sure you don't want to update it to 2021?
This file is common for application, firmware, driver and UEFI driver and
we are consumer of this file. Further update on this driver will have
updated revision whenever maintainer of this header file post the new
update.

> > +typedef struct _MPI3_SCSI_IO_CDB_EEDP32 {
> > +    U8              CDB[20];                            /* 0x00 */
> > +    __be32          PrimaryReferenceTag;                /* 0x14 */
> > +    U16             PrimaryApplicationTag;              /* 0x18 */
> > +    U16             PrimaryApplicationTagMask;          /* 0x1A */
> > +    U32             TransferLength;                     /* 0x1C */
> > +} MPI3_SCSI_IO_CDB_EEDP32, MPI3_POINTER
> PTR_MPI3_SCSI_IO_CDB_EEDP32,
> > +  Mpi3ScsiIoCdbEedp32_t, MPI3_POINTER pMpi3ScsiIoCdbEedp32_t;
> > +
>
> As noted by Bart, this is a bit naff.
> I can live with having driver-specific typedefs, but having
> driver-specific typedefs _just_ for little endian is pushing it.
> (And incidentally also cancels your argument that you need the
> driver-specific types for cross-development.)
> So please, be consistent, and either use driver-specific typedefs
> throughout or revert to use basic linux types.

As explained earlier - This file is common for application, firmware,
driver and UEFI driver and we are consumer of this file.
Please consider header file as an exception.

>
> > +typedef union _MPI3_SCSO_IO_CDB_UNION {
> > +    U8                      CDB32[32];
> > +    MPI3_SCSI_IO_CDB_EEDP32 EEDP32;
> > +    MPI3_SGE_SIMPLE         SGE;
> > +} MPI3_SCSO_IO_CDB_UNION, MPI3_POINTER
> PTR_MPI3_SCSO_IO_CDB_UNION,
> > +  Mpi3ScsiIoCdb_t, MPI3_POINTER pMpi3ScsiIoCdb_t;
> > +
> > +typedef struct _MPI3_SCSI_IO_REQUEST {
> > +    U16                     HostTag;                        /* 0x00
*/
> > +    U8                      IOCUseOnly02;                   /* 0x02
*/
> > +    U8                      Function;                       /* 0x03
*/
> > +    U16                     IOCUseOnly04;                   /* 0x04
*/
> > +    U8                      IOCUseOnly06;                   /* 0x06
*/
> > +    U8                      MsgFlags;                       /* 0x07
*/
> > +    U16                     ChangeCount;                    /* 0x08
*/
> > +    U16                     DevHandle;                      /* 0x0A
*/
> > +    U32                     Flags;                          /* 0x0C
*/
> > +    U32                     SkipCount;                      /* 0x10
*/
> > +    U32                     DataLength;                     /* 0x14
*/
> > +    U8                      LUN[8];                         /* 0x18
*/
> > +    MPI3_SCSO_IO_CDB_UNION  CDB;                            /* 0x20
*/
> > +    MPI3_SGE_UNION          SGL[4];                         /* 0x40
*/
> > +} MPI3_SCSI_IO_REQUEST, MPI3_POINTER PTR_MPI3_SCSI_IO_REQUEST,
> > +  Mpi3SCSIIORequest_t, MPI3_POINTER pMpi3SCSIIORequest_t;
> > +
>
> Ho-hum.
> What happens if you have SGLs with more than 4 entries?

It is dynamic array. SGL[3] is primarily used in driver and you can notice
that it is for DIF/DIX metadata. Driver will fill simple SGLs in main
frame if there is a room.
Usually MPT Frame size is 128 byte, but another size can be defined by FW
as well. If there are more than 4 SGLs and driver cannot accommodate in
main frame, it will use chain frame which will have remaining SGLs.
Chain frame logic is same as generic mpt3sas interface (with slight
modification in fields based on hardware interface.)

> > diff --git a/drivers/scsi/mpi3mr/mpi/mpi30_type.h
> b/drivers/scsi/mpi3mr/mpi/mpi30_type.h
> > new file mode 100644
> > index 000000000000..5de35e7a660f
> > --- /dev/null
> > +++ b/drivers/scsi/mpi3mr/mpi/mpi30_type.h
> > @@ -0,0 +1,89 @@
> > +/* SPDX-License-Identifier: GPL-2.0-or-later */
> > +/*
> > + *  Copyright 2016-2020 Broadcom Inc. All rights reserved.
> > + *
> > + *           Name: mpi30_type.h
> > + *    Description: MPI basic type definitions
> > + *  Creation Date: 10/07/2016
> > + *        Version: 03.00.00
> > + */
> > +#ifndef MPI30_TYPE_H
> > +#define MPI30_TYPE_H     1
> > +
> >
> +/***************************************************************
> **************
> > + * Define MPI3_POINTER if it has not already been defined. By default
*
> > + * MPI3_POINTER is defined to be a near pointer. MPI3_POINTER can be
> defined *
> > + * as a far pointer by defining MPI3_POINTER as "far *" before this
header
> *
> > + * file is included.
*
> > +
> *****************************************************************
> ***********/
> > +#ifndef MPI3_POINTER
> > +#define MPI3_POINTER    *
> > +#endif  /* MPI3_POINTER */
> > +
> > +/* The basic types may have already been included by mpi_type.h or
> mpi2_type.h*/
> > +#if !defined(MPI_TYPE_H) && !defined(MPI2_TYPE_H)
> > +#if 1
> >
> +/***************************************************************
> **************
> > +*
> > +*               Basic Types
> > +*
> >
> +****************************************************************
> *************/
> > +
> > +typedef u8 U8;
> > +typedef __le16 U16;
> > +typedef __le32 U32;
> > +typedef __le64 U64 __aligned(4);
> > +
> >
> +/***************************************************************
> **************
> > +*
> > +*               Pointer Types
> > +*
> >
> +****************************************************************
> *************/
> > +
> > +typedef U8 * PU8;
> > +typedef U16 * PU16;
> > +typedef U32 * PU32;
> > +typedef U64 * PU64;
> > +#else
> >
> +/***************************************************************
> **************
> > + *              Basic Types
*
> > +
> *****************************************************************
> ***********/
> > +typedef int8_t      S8;
> > +typedef uint8_t     U8;
> > +typedef int16_t     S16;
> > +typedef uint16_t    U16;
> > +typedef int32_t     S32;
> > +typedef uint32_t    U32;
> > +typedef int64_t     S64;
> > +typedef uint64_t    U64;
> > +
> >
> +/***************************************************************
> **************
> > + *              Structure Types
*
> > +
> *****************************************************************
> ***********/
> > +typedef struct _S64struct {
> > +    U32         Low;
> > +    S32         High;
> > +} S64struct;
> > +
> > +typedef struct _U64struct {
> > +    U32         Low;
> > +    U32         High;
> > +} U64struct;
> > +
>
> I wonder why you need these structures on a 64-bit system ...

Please consider all such things in header file as an exception since
header files are common and created to all platforms and users must use
without modified version to avoid API compatibility issues.
Mpi3mr linux driver is not using this but some other users like UEFI
driver, windows drivers etc might be a consumer of it.

>
> >
> +/***************************************************************
> **************
> > + *              Pointer Types
*
> > +
> *****************************************************************
> ***********/
> > +typedef S8 * PS8;
> > +typedef U8 * PU8;
> > +typedef S16 * PS16;
> > +typedef U16 * PU16;
> > +typedef S32         *PS32;
> > +typedef U32         *PU32;
> > +typedef S64 * PS64;
> > +typedef U64 * PU64;
> > +typedef S64struct * PS64struct;
> > +typedef U64struct * PU64struct;
> > +#endif
> > +#endif  /* MPI_TYPE_H && MPI2_TYPE_H */
> > +
> > +#endif  /* MPI30_TYPE_H */
> >
> And I do agree with Bart, please kill the pointer types.
> It's not serving any purpose whatsoever.

Same as above.
>
> Cheers,
>
> Hannes
> --
> Dr. Hannes Reinecke		           Kernel Storage Architect
> 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

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4212 bytes --]