| 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
| ||
|
Date: Fri, 8 Mar 2013 13:21:20 +0530
From: Rayagond K <rayagond@...avyalabs.com>
To: Giuseppe CAVALLARO <peppe.cavallaro@...com>
Cc: Richard Cochran <richardcochran@...il.com>, netdev@...r.kernel.org,
bh74.an@...sung.com
Subject: Re: [net-next.git 2/9] stmmac: add IEEE 1588-2002 PTP support
On Fri, Mar 8, 2013 at 12:32 PM, Giuseppe CAVALLARO
<peppe.cavallaro@...com> wrote:
>
> On 3/8/2013 7:34 AM, Richard Cochran wrote:
>>
>> I have a few comments, below.
>
>
> thanks for them.
>
>
>>> HW Timestamp support can be enabled while configuring the Kernel and
>>> the Koption is: STMMAC_USE_HWSTAMP
>>> At runtime, the support is verified by looking at the HW capability
>>> register.
>>
>>
>> As davem said, since you have the ability to detect this at run time,
>> then there is no need for a kconfig option.
>
>
> I'll try to give you more details about this replying to D. Miller
>
> [snip]
>
>
>>> + /* get tx/rx timestamp low value */
>>> + u32 (*get_timestamp_low) (struct dma_desc *p);
>>> + /* get tx/rx timestamp high value */
>>> + u32 (*get_timestamp_high) (struct dma_desc *p);
>>
>>
>> These two separate functions should be combined into one.
>
>
> ok
>
>
>>> + /* get rx timestamp status */
>>> + int (*get_rx_timestamp_status) (struct dma_desc *p);
>
>
>>> +
>>> +static int enh_desc_get_rx_timestamp_status(struct dma_desc *p)
>>> +{
>>> + /* FIXME if Enhance descriptor with 8 DWORDS is enabled */
>>
>>
>> Why not fix these FIXMEs for the next respin?
>
>
> This is fixed in the patch #5 where we use the extended descriptors
> for PTP2.
>
>
>>> + if ((p->des2 == 0xffffffff) && (p->des3 == 0xffffffff))
>>> + /* timestamp is currupted, hence don't store it */
>>
>>
>> corrupted
>
>
> thanks
>
>
>
>>> + /* Convert the ptp_clock to nano second
>>> + * formula = (1/ptp_clock) * 1000000000
>>> + * where, ptp_clock = 50MHz for FINE correction method &
>>> + * ptp_clock = STMMAC_SYSCLOCK for COARSE correction method
>>> + */
>>
>>
>> What are these coarse/fine method? Can't we always use the fine one?
>
>
> This is explained in the "4.1.2 System Time Register Module" of Synopsys
> Databook. Summarizing, the MAC can have an optional module and use this
> coarse correction method. In the fine correction method, a slave clock’s
> frequency drift with respect to the master clock is corrected over a period
> of time instead of in one clock, as in coarse correction.
>
> Pls, Rayagond feels free to provide more details...
Yes Synopsys core support two method for correcting the system time ie
COARSE method and FINE method.
In the fine correction method, a slave clock’s frequency drift with
respect to the master clock (as defined in IEEE 1588) is corrected
over a period of time instead of in one clock, as in coarse
correction. This helps maintain linear time and does not introduce
drastic changes (or a large jitter) in the reference time between PTP
Sync message intervals.
And updating the SSNR (Sub Second Increment Register) depends on which
method is selected during the initialization.
>
>>> + if (value & PTP_TCR_TSCFUPDT)
>>> + data = (1000000000ULL / 50000000);
>>> + else
>>> + data = (1000000000ULL / STMMAC_SYSCLOCK);
>>> +
>>> + writel(data, ioaddr + PTP_SSIR);
>>> +}
>>> +
>>> +static int stmmac_init_systime(void __iomem *ioaddr, u32 sec, u32 nsec)
>>> +{
>>> + int limit;
>>> + u32 value;
>>> +
>>> + /* wait for previous(if any) system time initialize to complete
>>> */
>>> + limit = 100;
>>> + while (limit--) {
>>> + if (!(readl(ioaddr + PTP_TCR) & PTP_TCR_TSINIT))
>>> + break;
>>> + mdelay(10);
>>> + }
>>> +
>>> + if (limit < 0)
>>> + return -EBUSY;
>>> +
>>
>>
>> Ugh, this is terrible...
>>
>>> + writel(sec, ioaddr + PTP_STSUR);
>>> + writel(nsec, ioaddr + PTP_STNSUR);
>>> + /* issue command to initialize the system time value */
>>> + value = readl(ioaddr + PTP_TCR);
>>> + value |= PTP_TCR_TSINIT;
>>> + writel(value, ioaddr + PTP_TCR);
>>> +
>>> + /* wait for present system time initialize to complete */
>>> + limit = 100;
>>> + while (limit--) {
>>> + if (!(readl(ioaddr + PTP_TCR) & PTP_TCR_TSINIT))
>>> + break;
>>> + mdelay(10);
>>> + }
>>> + if (limit < 0)
>>> + return -EBUSY;
>>
>>
>> ... for a number of reasons.
>>
>> 1. You are polling for 100*10ms = one second, and the caller has
>> disabled interrupts! This is way too long. Is the hardware really
>> that slow? If so, then you need to use delayed work or find some
>> other way not to block.
Sorry for this, during testing I just used maximum delay as there was
bug in the HW initially. But after fixing the HW issue I noticed that
HW is not that much slow. We will reduce the delay in all functions.
>>
>> 2. You are waiting both before and after for the status bit. Pick one
>> or the other. You don't need both.
>>
>> This same pattern is repeated a few times here, and it needs fixing in
>> each case.
Sure will change it. I added just to make sure double check here.
Practically first wait is not required, will remove this.
>
>
> This is for the Author. I've no HW where test. Rayagond tested all on his
> side. Pls Rayagond fixes it and gives me the new code.
>
>
>>> + struct stmmac_priv *priv = netdev_priv(dev);
>>> + struct hwtstamp_config config;
>>> + struct timespec now;
>>> + u64 temp = 0;
>>
>>
>> You add this new code here, but you change it all around again a few
>> patches later. Please just submit the final, combined version.
>
>
> we kept these separately because the patch #5 (for example) depends on
> another one that adds the extended descriptor support. Also If I add
> all the code in a single patch this will be very big. I had some
> problems to review all separately. So I suspect that if we merge all
> in a single patch this will not help (especially myself). At any rate,
> tell me if you prefer to have a single patch. I can do that.
>
> peppe
>>
>>
>> Thanks,
>> Richard
>>
>
Thanks
Rayagond.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Powered by blists - more mailing lists