netdev - RE: [PATCH v2 net-next 9/9] lan743x: Add PTP support

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]

Message-ID: <90A7E81AE28BAE4CBDDB3B35F187D2644073B039@CHN-SV-EXMX02.mchp-main.com>
Date:   Wed, 18 Jul 2018 20:04:08 +0000
From:   <Bryan.Whitehead@...rochip.com>
To:     <richardcochran@...il.com>
CC:     <davem@...emloft.net>, <netdev@...r.kernel.org>,
        <UNGLinuxDriver@...rochip.com>
Subject: RE: [PATCH v2 net-next 9/9] lan743x: Add PTP support

Hi Richard,

Thank you for your detailed feedback. I'm working on it now, but I feel it will take a little extra time to complete. Therefor I'm planning to remove PTP support from this patch series, and resubmit it in a new patch later.

I also have a few questions below.

> -----Original Message-----
> From: Richard Cochran [mailto:richardcochran@...il.com]
> Sent: Thursday, July 12, 2018 11:32 PM
> To: Bryan Whitehead - C21958 <Bryan.Whitehead@...rochip.com>
> Cc: davem@...emloft.net; netdev@...r.kernel.org; UNGLinuxDriver
> <UNGLinuxDriver@...rochip.com>
> Subject: Re: [PATCH v2 net-next 9/9] lan743x: Add PTP support
> 
...
> > +	if (cleanup) {
> > +		lan743x_ptp_unrequest_tx_timestamp(tx->adapter);
> > +		dev_kfree_skb(buffer_info->skb);
> > +	} else {
> > +		lan743x_ptp_tx_timestamp_skb(tx->adapter,
> > +					     buffer_info->skb,
> > +					     (buffer_info->flags &
> > +
> TX_BUFFER_INFO_FLAG_IGNORE_SYNC)
> > +					     != 0);
> 
> This is poor coding style.  Please find a better way.

Can you clarify what is poor and what would be better?
For example, should I change "X != 0" to "X ? true : false".

> > +#ifdef CONFIG_PTP_1588_CLOCK
> > +static int lan743x_ptp_enable_pps(struct lan743x_adapter *adapter) {
> > +	struct lan743x_ptp *ptp = &adapter->ptp;
> > +	u32 current_seconds = 0;
> > +	u32 target_seconds = 0;
> > +	u32 general_config = 0;
> > +	int result = -ENODEV;
> > +	int pps_bit = 0;
> 
> So this function is really *not* implementing the PTP_CLK_REQ_PPS feature
> but rather the PTP_CLK_REQ_PEROUT with a period of once per second.
> 
> PTP_CLK_REQ_PPS means placing a PPS event into the kernel's "hardpps"
> subsystem by calling ptp_clock_event().
> 
> I'm sorry this isn't really documented.  I should fix that.
> 
> If you HW can output arbitrary signals, then you should implement
> PTP_CLK_REQ_PEROUT.  In any case, you shouldn't advertise the
> ptp_clock_info.pps capability.

So you mean PPS is not intended to generate a physical signal?
It is only intended to call ptp_clock_event?
I can configure the hardware to generate an interrupt each second and then call 
ptp_clock_event. Would that satisfy the pps requirements?

Regarding PTP_CLK_REQ_PEROUT. Is that intended for physical signals?

Thanks,
Bryan