Re: [PATCH 3/9] net: ethernet: ti: cpts: rework initialization/deinitialization

[Grygorii Strashko]

On 09/14/2016 04:52 PM, Richard Cochran wrote:
> On Wed, Sep 14, 2016 at 04:02:25PM +0300, Grygorii Strashko wrote:
>> @@ -323,7 +307,7 @@ void cpts_rx_timestamp(struct cpts *cpts, struct sk_buff *skb)
>>  	u64 ns;
>>  	struct skb_shared_hwtstamps *ssh;
>>  
>> -	if (!cpts->rx_enable)
>> +	if (!cpts || !cpts->rx_enable)
>>  		return;
> 
> This function is in the hot path, and you have added a pointless new
> test.  Don't do that.
> 
>>  	ns = cpts_find_ts(cpts, skb, CPTS_EV_RX);
>>  	if (!ns)
>> @@ -338,7 +322,7 @@ void cpts_tx_timestamp(struct cpts *cpts, struct sk_buff *skb)
>>  	u64 ns;
>>  	struct skb_shared_hwtstamps ssh;
>>  
>> -	if (!(skb_shinfo(skb)->tx_flags & SKBTX_IN_PROGRESS))
>> +	if (!cpts || !(skb_shinfo(skb)->tx_flags & SKBTX_IN_PROGRESS))
>>  		return;
> 
> Same here.
> 
>>  	ns = cpts_find_ts(cpts, skb, CPTS_EV_TX);
>>  	if (!ns)
>> @@ -348,53 +332,102 @@ void cpts_tx_timestamp(struct cpts *cpts, struct sk_buff *skb)
>>  	skb_tstamp_tx(skb, &ssh);
>>  }
>>  
>> -int cpts_register(struct device *dev, struct cpts *cpts,
>> -		  u32 mult, u32 shift)
>> +int cpts_register(struct cpts *cpts)
>>  {
>>  	int err, i;
>> -	unsigned long flags;
>>  
>> -	cpts->info = cpts_info;
>> -	cpts->clock = ptp_clock_register(&cpts->info, dev);
>> -	if (IS_ERR(cpts->clock)) {
>> -		err = PTR_ERR(cpts->clock);
>> -		cpts->clock = NULL;
>> -		return err;
>> -	}
>> -	spin_lock_init(&cpts->lock);
>> -
>> -	cpts->cc.read = cpts_systim_read;
>> -	cpts->cc.mask = CLOCKSOURCE_MASK(32);
>> -	cpts->cc_mult = mult;
>> -	cpts->cc.mult = mult;
>> -	cpts->cc.shift = shift;
>> +	if (!cpts)
>> +		return -EINVAL;
> 
> Not hot path, but still silly.  The caller should never pass NULL.

Ok. I can't say I'd like all this checks, but there are internal requirement 
to allow CPTS to be disabled though DT on KS2 (even if built in).
I'd try to clarify and return back here.

But It'll be good to know your position - acceptable/can be discussed/completely unacceptable?


-- 
regards,
-grygorii