> > +++ 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@xxxxxxx +49 911 74053 688
> SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg
> HRB 36809 (AG Nürnberg), GF: Felix Imendörffer
Attachment:
smime.p7s
Description: S/MIME Cryptographic Signature
