netdev - Re: [PATCH net-next 04/11] net/mlx5_core: HW data structs/types definitions preparation for mlx5 ehternet driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CAPcc5PjsucECHPOvB49rQ6joY7Wtgs3oX6DdPwQP=RPzvkKcBg@mail.gmail.com>
Date:	Sun, 12 Apr 2015 18:54:37 +0300
From:	Amir Vadai <amirv@...lanox.com>
To:	Joe Perches <joe@...ches.com>
Cc:	Amir Vadai <amirv@...lanox.com>,
	"David S. Miller" <davem@...emloft.net>,
	netdev <netdev@...r.kernel.org>,
	Yevgeny Petrilin <yevgenyp@...lanox.com>,
	Saeed Mahameed <saeedm@...lanox.com>,
	Or Gerlitz <ogerlitz@...lanox.com>,
	Achiad Shochat <achiad@...lanox.com>,
	Ido Shamay <idos@...lanox.com>, dotanb@...lanox.com
Subject: Re: [PATCH net-next 04/11] net/mlx5_core: HW data structs/types
 definitions preparation for mlx5 ehternet driver

On Wed, Apr 8, 2015 at 7:46 PM, Joe Perches <joe@...ches.com> wrote:
> On Wed, 2015-04-08 at 17:51 +0300, Amir Vadai wrote:
Hi,

This file is based on the PRM: Programmer Reference Manual (aka
firmware spec), which describes the interface between the device and
the driver.

> []
>> - Update mlx5_ifc.h to include latest HW structs aligned to PRM rev 0.25
>
> trivia
Yes, But since PRM keeps evolving we want to keep track the PRM
version that this file was based on.

>
>> diff --git a/include/linux/mlx5/mlx5_ifc.h b/include/linux/mlx5/mlx5_ifc.h
> []
>> +   Autogenerated file.
>> +   Date: 2015-03-09 14:03
>> +   Source Document Name: Mellanox <Doc Name>
>> +   Source Document Version: 0.24
>> +   Generated by adb_to_c.py (EAT.ME Version: 1.0.70)
>> +*/
>
> That's a nice little detail to add.
Will remove it in V1

>
>>  #ifndef MLX5_IFC_H
>>  #define MLX5_IFC_H
>>
>>  enum {
>> +     MLX5_EVENT_TYPE_CODING_COMPLETION_EVENTS                   = 0x0,
>> +     MLX5_EVENT_TYPE_CODING_PATH_MIGRATED_SUCCEEDED             = 0x1,
>> +     MLX5_EVENT_TYPE_CODING_COMMUNICATION_ESTABLISHED           = 0x2,
>> +     MLX5_EVENT_TYPE_CODING_SEND_QUEUE_DRAINED                  = 0x3,
>> +     MLX5_EVENT_TYPE_CODING_LAST_WQE_REACHED                    = 0x13,
>> +     MLX5_EVENT_TYPE_CODING_SRQ_LIMIT                           = 0x14,
>> +     MLX5_EVENT_TYPE_CODING_DCT_ALL_CONNECTIONS_CLOSED          = 0x1c,
>> +     MLX5_EVENT_TYPE_CODING_DCT_ACCESS_KEY_VIOLATION            = 0x1d,
>> +     MLX5_EVENT_TYPE_CODING_CQ_ERROR                            = 0x4,
>> +     MLX5_EVENT_TYPE_CODING_LOCAL_WQ_CATASTROPHIC_ERROR         = 0x5,
>> +     MLX5_EVENT_TYPE_CODING_PATH_MIGRATION_FAILED               = 0x7,
>> +     MLX5_EVENT_TYPE_CODING_PAGE_FAULT_EVENT                    = 0xc,
>> +     MLX5_EVENT_TYPE_CODING_INVALID_REQUEST_LOCAL_WQ_ERROR      = 0x10,
>> +     MLX5_EVENT_TYPE_CODING_LOCAL_ACCESS_VIOLATION_WQ_ERROR     = 0x11,
>> +     MLX5_EVENT_TYPE_CODING_LOCAL_SRQ_CATASTROPHIC_ERROR        = 0x12,
>> +     MLX5_EVENT_TYPE_CODING_INTERNAL_ERROR                      = 0x8,
>> +     MLX5_EVENT_TYPE_CODING_PORT_STATE_CHANGE                   = 0x9,
>> +     MLX5_EVENT_TYPE_CODING_GPIO_EVENT                          = 0x15,
>> +     MLX5_EVENT_TYPE_CODING_REMOTE_CONFIGURATION_PROTOCOL_EVENT = 0x19,
>> +     MLX5_EVENT_TYPE_CODING_DOORBELL_BLUEFLAME_CONGESTION_EVENT = 0x1a,
>> +     MLX5_EVENT_TYPE_CODING_STALL_VL_EVENT                      = 0x1b,
>> +     MLX5_EVENT_TYPE_CODING_DROPPED_PACKET_LOGGED_EVENT         = 0x1f,
>> +     MLX5_EVENT_TYPE_CODING_COMMAND_INTERFACE_COMPLETION        = 0xa,
>> +     MLX5_EVENT_TYPE_CODING_PAGE_REQUEST                        = 0xb
>> +};
>
> Curious ordering and numbering.
> There's no 0xd or 0xe, 0x16, 0x17, 0x18, 0x1e
> Are those reserved?
>
Yes. Unused values are reserved. And the ordering is the same like we
have in the PRM


>> +struct mlx5_ifc_flow_table_fields_supported_bits {
>> +     u8         outer_dmac[0x1];
>
> This structure has one use, but so far that seems
> unused.  Will this be used later?
Yes. Those structures are being used through the macros MLX5_[GS]ET*

>
> Decimal is generally easier to read than hex.
We decided to use hex values, since the PRM is written with hex values.

> The code is generally easier too if instead of an
> array definition of size 1 it's a simple type.
This entry actually describes one bit.

>
> If these are autogenerated, and length/position are important,
> it could be nice to do something like:
>
> struct <foo> {
>         type    array1[decimal_size];   /* Hex offset */
>         ...
> }
>
> []
>
>> +struct mlx5_ifc_phys_layer_cntrs_bits {
>> +     u8         time_since_last_clear_high[0x20];
>> +
>
> Why all the blank lines?
Our tables are accessed as dwords. We add blank line between every
dword in the PRM.

>
> If this is autogenerated from a document with comments,
> maybe the autogenerator could elide the blank lines too
> when in a structure.
The autogenerator completely ignores comments.

Thanks,
Amir
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html