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

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] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 14 Sep 2016 23:10:48 +0300
From:   Grygorii Strashko <grygorii.strashko@...com>
To:     Richard Cochran <richardcochran@...il.com>
CC:     "David S. Miller" <davem@...emloft.net>, <netdev@...r.kernel.org>,
        Mugunthan V N <mugunthanvnm@...com>,
        Sekhar Nori <nsekhar@...com>, <linux-kernel@...r.kernel.org>,
        <linux-omap@...r.kernel.org>, WingMan Kwok <w-kwok2@...com>
Subject: Re: [PATCH 3/9] net: ethernet: ti: cpts: rework
 initialization/deinitialization

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