Re: [dpdk-dev] [PATCH 03/49] net/ice/base: add API to configure MIB

[Maxime Coquelin <maxime.coquelin@redhat.com>]

On 6/5/19 2:00 AM, Stillwell Jr, Paul M wrote:
>> -----Original Message-----
>> From: Maxime Coquelin <maxime.coquelin@redhat.com>
>> Sent: Tuesday, June 4, 2019 10:15 AM
>> To: Rong, Leyi <leyi.rong@intel.com>; Zhang, Qi Z <qi.z.zhang@intel.com>
>> Cc: dev@dpdk.org; Cao, Chinh T <chinh.t.cao@intel.com>; Ertman, David M
>> <david.m.ertman@intel.com>; Stillwell Jr, Paul M
>> <paul.m.stillwell.jr@intel.com>
>> Subject: Re: [dpdk-dev] [PATCH 03/49] net/ice/base: add API to configure
>> MIB
>>
>>
>>
>> On 6/4/19 7:42 AM, Leyi Rong wrote:
>>> Add ice_cfg_lldp_mib_change and treat DCBx state NOT_STARTED as valid.
>>>
>>> Signed-off-by: Chinh T Cao <chinh.t.cao@intel.com>
>>> Signed-off-by: Dave Ertman <david.m.ertman@intel.com>
>>> Signed-off-by: Paul M Stillwell Jr <paul.m.stillwell.jr@intel.com>
>>> Signed-off-by: Leyi Rong <leyi.rong@intel.com>
>>> ---
>>>    drivers/net/ice/base/ice_dcb.c | 41
>> +++++++++++++++++++++++++++++-----
>>>    drivers/net/ice/base/ice_dcb.h |  3 ++-
>>>    2 files changed, 38 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/net/ice/base/ice_dcb.c
>>> b/drivers/net/ice/base/ice_dcb.c index a7810578d..100c4bb0f 100644
>>> --- a/drivers/net/ice/base/ice_dcb.c
>>> +++ b/drivers/net/ice/base/ice_dcb.c
>>> @@ -927,10 +927,11 @@ enum ice_status ice_get_dcb_cfg(struct
>> ice_port_info *pi)
>>>    /**
>>>     * ice_init_dcb
>>>     * @hw: pointer to the HW struct
>>> + * @enable_mib_change: enable MIB change event
>>>     *
>>>     * Update DCB configuration from the Firmware
>>>     */
>>> -enum ice_status ice_init_dcb(struct ice_hw *hw)
>>> +enum ice_status ice_init_dcb(struct ice_hw *hw, bool
>>> +enable_mib_change)
>>>    {
>>>    	struct ice_port_info *pi = hw->port_info;
>>>    	enum ice_status ret = ICE_SUCCESS;
>>> @@ -944,7 +945,8 @@ enum ice_status ice_init_dcb(struct ice_hw *hw)
>>>    	pi->dcbx_status = ice_get_dcbx_status(hw);
>>>
>>>    	if (pi->dcbx_status == ICE_DCBX_STATUS_DONE ||
>>> -	    pi->dcbx_status == ICE_DCBX_STATUS_IN_PROGRESS) {
>>> +	    pi->dcbx_status == ICE_DCBX_STATUS_IN_PROGRESS ||
>>> +	    pi->dcbx_status == ICE_DCBX_STATUS_NOT_STARTED) {
>>
>> Should this really be in this patch?
>> It does not seem related to the API addition.
>>
> 
> This seems ok since the commit message says that we changed the API and are treating dcbx_status in a different manor. Is the objection that we have 2 things in one commit?

Well, it depends if DCBx NOT_STARTED becomes valid thanks to
ice_cfg_lldp_mib_change addition. It is not obvious by looking at the
commit message and the patch itself.

It this is not the case, then 2 commits are prefered, as one could
backport only the DCBx NOT_STARTED part.

> 
>>>    		/* Get current DCBX configuration */
>>>    		ret = ice_get_dcb_cfg(pi);
>>>    		pi->is_sw_lldp = (hw->adminq.sq_last_status ==
>> ICE_AQ_RC_EPERM);
>>> @@ -952,13 +954,42 @@ enum ice_status ice_init_dcb(struct ice_hw *hw)
>>>    			return ret;
>>>    	} else if (pi->dcbx_status == ICE_DCBX_STATUS_DIS) {
>>>    		return ICE_ERR_NOT_READY;
>>> -	} else if (pi->dcbx_status == ICE_DCBX_STATUS_MULTIPLE_PEERS) {
>>
>> Ditto.
>>
>