Re: [Intel-wired-lan] [PATCH 2/9] ice: introduce PTP state machine

[Przemek Kitszel <przemyslaw.kitszel@intel.com>]

On 8/7/23 19:32, Jacob Keller wrote:
> 
> 
> On 8/7/2023 4:12 AM, Przemek Kitszel wrote:
>> On 8/7/23 12:36, Karol Kolacinski wrote:
>>> Add PTP state machine so that the driver can correctly identify PTP
>>> state around resets.
>>> When the driver got information about ungraceful reset, PTP was not
>>> prepared for reset and it returned error. When this situation occurs,
>>> prepare PTP before rebuilding its structures.
>>>
>>> Signed-off-by: Karol Kolacinski <karol.kolacinski@intel.com>
>>> ---
>>>    drivers/net/ethernet/intel/ice/ice.h         |   1 -
>>>    drivers/net/ethernet/intel/ice/ice_ethtool.c |   2 +-
>>>    drivers/net/ethernet/intel/ice/ice_ptp.c     | 131 +++++++++++++------
>>>    drivers/net/ethernet/intel/ice/ice_ptp.h     |  10 ++
>>>    4 files changed, 99 insertions(+), 45 deletions(-)
>>>
>>> diff --git a/drivers/net/ethernet/intel/ice/ice.h b/drivers/net/ethernet/intel/ice/ice.h
>>> index 34be1cb1e28f..86f6f94da535 100644
>>> --- a/drivers/net/ethernet/intel/ice/ice.h
>>> +++ b/drivers/net/ethernet/intel/ice/ice.h
>>> @@ -490,7 +490,6 @@ enum ice_pf_flags {
>>>    	ICE_FLAG_DCB_ENA,
>>>    	ICE_FLAG_FD_ENA,
>>>    	ICE_FLAG_PTP_SUPPORTED,		/* PTP is supported by NVM */
>>> -	ICE_FLAG_PTP,			/* PTP is enabled by software */
>>>    	ICE_FLAG_ADV_FEATURES,
>>>    	ICE_FLAG_TC_MQPRIO,		/* support for Multi queue TC */
>>>    	ICE_FLAG_CLS_FLOWER,
>>> diff --git a/drivers/net/ethernet/intel/ice/ice_ethtool.c b/drivers/net/ethernet/intel/ice/ice_ethtool.c
>>> index d3cb08e66dcb..7d57ecf48da0 100644
>>> --- a/drivers/net/ethernet/intel/ice/ice_ethtool.c
>>> +++ b/drivers/net/ethernet/intel/ice/ice_ethtool.c
>>> @@ -3275,7 +3275,7 @@ ice_get_ts_info(struct net_device *dev, struct ethtool_ts_info *info)
>>>    	struct ice_pf *pf = ice_netdev_to_pf(dev);
>>>    
>>>    	/* only report timestamping if PTP is enabled */
>>> -	if (!test_bit(ICE_FLAG_PTP, pf->flags))
>>> +	if (!test_bit(ICE_FLAG_PTP_SUPPORTED, pf->flags))
>>>    		return ethtool_op_get_ts_info(dev, info);
>>>    
>>>    	info->so_timestamping = SOF_TIMESTAMPING_TX_SOFTWARE |
>>> diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.c b/drivers/net/ethernet/intel/ice/ice_ptp.c
>>> index 0669ca905c46..a6ea90b9461e 100644
>>> --- a/drivers/net/ethernet/intel/ice/ice_ptp.c
>>> +++ b/drivers/net/ethernet/intel/ice/ice_ptp.c
>>> @@ -255,6 +255,31 @@ ice_verify_pin_e810t(struct ptp_clock_info *info, unsigned int pin,
>>>    	return ice_ptp_set_sma_e810t(info, pin, func);
>>>    }
>>>    
>>> +/**
>>> + * ice_ptp_state_str - Convert PTP state to readable string
>>> + * @state: PTP state to convert
>>> + *
>>> + * Returns: the human readable string representation of the provided PTP
>>> + * state, used for printing error messages.
>>> + */
>>> +static const char *ice_ptp_state_str(enum ice_ptp_state state)
>>> +{
>>> +	switch (state) {
>>> +	case ICE_PTP_UNINIT:
>>> +		return "UNINITIALIZED";
>>> +	case ICE_PTP_INITIALIZING:
>>> +		return "INITIALIZING";
>>> +	case ICE_PTP_READY:
>>> +		return "READY";
>>> +	case ICE_PTP_RESETTING:
>>> +		return "RESETTING";
>>> +	case ICE_PTP_ERROR:
>>> +		return "ERROR";
>>> +	}
>>> +
>>> +	return "UNKNOWN";
>>> +}
>>> +
>>>    /**
>>>     * ice_ptp_configure_tx_tstamp - Enable or disable Tx timestamp interrupt
>>>     * @pf: The PF pointer to search in
>>> @@ -1285,7 +1310,7 @@ void ice_ptp_link_change(struct ice_pf *pf, u8 port, bool linkup)
>>>    	struct ice_ptp_port *ptp_port;
>>>    	struct ice_hw *hw = &pf->hw;
>>>    
>>> -	if (!test_bit(ICE_FLAG_PTP, pf->flags))
>>> +	if (pf->ptp.state != ICE_PTP_READY)
>>
>> test_bit() is atomic API, but "just reading/using variable" is rather not.
>> Please extend commit message to say something about why transition  here
>> (here=whole commit) is safe.
>>
> 
> Just use of "test_bit()" doesn't really provide much more than
> potentially some memory barriers. On its own, it doesn't actually
> provide any synchronization mechanism. I guess this could be "READ_ONCE"
> or use some barrier to make sure this doesn't re-order the read, but
> otherwise its about as atomic as before. Either we see the value as
> being ready or we don't. That's essentially no different than the bit flag.
> 
> Unless we were using something like "test_and_set" or "test_and_clear"
> the use of atomic flags here isn't providing any actual protection or
> synchronization beyond avoiding screwing up the flags variable itself.
> 
> I considered swapping to an atomic value like using atomic_set or
> something but it really felt like overkill when writing it in the
> out-of-tree driver. (Yes, the lack of proper synchronization primitives
> in ice is rather annoying...)
> 
> In my understanding this should be no worse than before since the state
> field is always either directly assigned or directly read. We're not
> replacing something like "test_and_set_bit" so we aren't any worse than
> before.
> 
> This could be clarified better in the commit message.  Note that
> originally we kept the flag and introduced the state field, then later
> removed the flag. Some of this detail was lost when squashing everything
> together.

Thank you :)

As a whole this series makes things better, so it's not an issue.

Some part of the question originates in me not knowing the driver well, 
what only proves the need for clarification/s everywhere ;)

_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan