Re: [PATCH net-next 00/10] NCSI Support

[Gavin Shan <gwshan@linux.vnet.ibm.com>]

On Thu, Jul 07, 2016 at 07:17:36PM +1000, Benjamin Herrenschmidt wrote:
>On Thu, 2016-07-07 at 12:12 +0300, Or Gerlitz wrote:
>> On Tue, Jul 5, 2016 at 8:44 PM, Alexei Starovoitov
>> <alexei.starovoitov@gmail.com> wrote:
>> > On Mon, Jul 04, 2016 at 01:03:06AM +0300, Or Gerlitz wrote:
>> 
>> > Or, since cx4 has ncsi as well, could you do a thorough review of this
>> > to make sure that it fits mellanox nics as well?
>> 
>> Hi Alexei, all
>> 
>> Yuval from our team who deals with host management did review on the
>> series, SB his feedback.
>> 

Thanks for the review and the comments.

>> 1. The initialization uses a single unicast MAC address which hints it
>> assumes that the management traffic is IPv4 only. The infrastructure
>> does not seem to be ready for IPv6 based management traffic.
>
>You mean the transfer of the MAC address from the BMC to the NIC for
>filtering incoming traffic ?
>

I think it's about global multicast filter which is used for IPv6 to
probe next neighbhour or hop. I will add it according to the retrieved
channel's capability in next revision.

>> 2. The code enables AEN messages (all the 3 messages defined by the
>> standard are enabled). AEN support mandates readiness on the BMC side
>> (running the reviewed code) and mandates support on the NIC. Not every
>> NIC can create AEN messages therefore I would recommend that the code
>> will only enable AENs which are declared as supported. To query
>> support for AEN messages, the BMC shall interpret the response for
>> "Get Capabilities" command and based on the returned response it can
>> detect which AEN message is supported by the NIC and may be enabled.
>
>Ok.
>

Yes, It makes sense. I will enable AEN messages according to channel's
capability in next revision.

>> 3. When stating support for NC-SI it is important to indicate which
>> exact version of NC-SI is supported (or required). From my observation
>> this code is based on NC-SI 1.0.0 as it does not use any of the
>> features which were introduced in the later versions.
>
>Probably correct, 1.0.1 I think is the only one that was available when
>we started that work. I found 1.1.0 on the DMTF web site, are you aware
>of anything more recent ?
>
>Gavin, you can get it here:
>
> https://www.dmtf.org/sites/default/files/standards/documents/DSP0222_1.1.0.pdf
>

Yeah, the code was implemented based on 1.0.1 specification as Ben said.
I go through 1.1.0 specification quickly. On page 65, there are more
optional commands added as below. I will define them in ncsi-pkt.h and
I don't think I need use any of them for now. Note the revision field
in NCSI packet header is still 0x1, same to the one in the code.

   Command Number           Description
   0x1B                     Get Package Status
   0x51                     PLDM
   0x52                     Get Package UUID

>> 4. HW arbitration is disabled in this implementation. It should be
>> defined how to add support to system which supports HW arbitration.
>> 

It's good point. Yes, HW arbitration is disabled in select package command
in current implementation. It can be enabled only when all available packages
and channels claim the capability in the get capability response. Currently,
there are two fields ("active_package" and "active_channel") in NCSI interface
(struct ncsi_dev_priv). I can reuse them to distinguish those cases (enabled
and disable HW arbitration):

   Enabled: active_package = first_available_package, active_channel = NULL;
   Disable: active_package = package_in_work,         active_channel = channel_in_work;

It affects the behaviour how AENs are processed. I don't think we need enable AEN
messages any more if I'm correct enough here.

>> 5. The code has an initialization sequence which is clear. Yet there
>> shall be a re-initialization sequence needed if a device goes through
>> asynchronous entry to initial state, which may happen if the NIC goes
>> through reset for any reason.
>
>Ok.
>

Yes, It makes sense as AEN isn't mandatory function supported by NIC. I will
change the code accordingly in next revision.

>Gavin: We can discuss these tomorrow if you need.
>

Thanks, Ben. I will talk to you tomorrow when you have free time. Thank you
very much for kindly helps on this.

Thanks,
Gavin

>Cheers,
>Ben.
>