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:   Fri, 18 Nov 2016 11:14:16 +0100
From:   Nicolas Ferre <nicolas.ferre@...el.com>
To:     Rafal Ozieblo <rafalo@...ence.com>,
        Harini Katakam <harinikatakamlinux@...il.com>,
        Andrei Pistirica <Andrei.Pistirica@...rochip.com>
CC:     "harini.katakam@...inx.com" <harini.katakam@...inx.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [RFC PATCH 2/2] net: macb: Add 64 bit addressing support for GEM

Le 18/11/2016 à 10:59, Rafal Ozieblo a écrit :
>>                                                     
>> -----Original Message-----                           
>> From: Harini Katakam [mailto:harinikatakamlinux@...il.com] 
>> Sent: 18 listopada 2016 10:44                              
>> To: Rafal Ozieblo                                          
>> Cc: Nicolas Ferre; Andrei Pistirica; harini.katakam@...inx.com; netdev@...r.kernel.org; linux-kernel@...r.kernel.org                                                                                             
>> Subject: Re: [RFC PATCH 2/2] net: macb: Add 64 bit addressing support for GEM                           
>>                                                                                                        
>> Hi Rafal,                                                                                               
>>                                                                                                        
>> On Fri, Nov 18, 2016 at 3:00 PM, Rafal Ozieblo <rafalo@...ence.com> wrote:                              
>>>> -----Original Message-----                                                                            
>>>> From: Nicolas Ferre [mailto:nicolas.ferre@...el.com]                                                  
>>>> Sent: 18 listopada 2016 10:10                                                                         
>>>> To: Rafal Ozieblo; Harini Katakam; Andrei Pistirica                                                   
>>>> Cc: harini.katakam@...inx.com; netdev@...r.kernel.org;                                                
>>>> linux-kernel@...r.kernel.org                                                                          
>>>> Subject: Re: [RFC PATCH 2/2] net: macb: Add 64 bit addressing support                                 
>>>> for GEM                                                                                               
>>>>                                                                                                      
>>>> Le 18/11/2016 à 09:59, Rafal Ozieblo a écrit :                                                        
>>>>> Hello,                                                                                              
>>>>>                                                                                                     
>>>>>> From: Harini Katakam [mailto:harinikatakamlinux@...il.com]                                         
>>>>>> Sent: 18 listopada 2016 05:30                                                                      
>>>>>> To: Rafal Ozieblo                                                                                  
>>>>>> Cc: Nicolas Ferre; harini.katakam@...inx.com;                                                      
>>>>>> netdev@...r.kernel.org; linux-kernel@...r.kernel.org                                               
>>>>>> Subject: Re: [RFC PATCH 2/2] net: macb: Add 64 bit addressing                                      
>>>>>> support for GEM                                                                                    
>>>>>>                                                                                                    
>>>>>> Hi Rafal,                                                                                          
>>>>>>                                                                                                    
>>>>>> On Thu, Nov 17, 2016 at 7:05 PM, Rafal Ozieblo <rafalo@...ence.com> wrote:                         
>>>>>>> -----Original Message-----                                                                        
>>>>>>> From: Nicolas Ferre [mailto:nicolas.ferre@...el.com]                                              
>>>>>>> Sent: 17 listopada 2016 14:29                                                                     
>>>>>>> To: Harini Katakam; Rafal Ozieblo                                                                 
>>>>>>> Cc: harini.katakam@...inx.com; netdev@...r.kernel.org;                                            
>>>>>>> linux-kernel@...r.kernel.org                                                                      
>>>>>>> Subject: Re: [RFC PATCH 2/2] net: macb: Add 64 bit addressing                                     
>>>>>>> support for GEM                                                                                   
>>>>>>>                                                                                                   
>>>>>>>> Le 17/11/2016 à 13:21, Harini Katakam a écrit :                                                  
>>>>>>>>> Hi Rafal,                                                                                       
>>>>>>>>>                                                                                                 
>>>>>>>>> On Thu, Nov 17, 2016 at 5:20 PM, Rafal Ozieblo <rafalo@...ence.com> wrote:                      
>>>>>>>>>> Hello,                                                                                         
>>>>>>>>>> I think, there could a bug in your patch.                                                      
>>>>>>>>>>                                                                                                
>>>>>>>>>>> +                                                                                             
>>>>>>>>>>> +#ifdef CONFIG_ARCH_DMA_ADDR_T_64BIT                                                          
>>>>>>>>>>> +             dmacfg |= GEM_BIT(ADDR64); #endif                                               
>>>>>>>>>>                                                                                                
>>>>>>>>>> You enable 64 bit addressing (64b dma bus width) always when appropriate architecture config option is enabled.                                                                                         
>>>>>>>>>> But there are some legacy controllers which do not support that feature. According Cadence hardware team:                                                                                               
>>>>>>>>>> "64 bit addressing was added in July 2013. Earlier version do not have it.                     
>>>>>>>>>> This feature was enhanced in release August 2014 to have separate upper address values for transmit and receive."                                                                                       
>>>>>>>>>>                                                                                                
>>>>>>>>>>> /* Bitfields in NSR */                                                                        
>>>>>>>>>>> @@ -474,6 +479,10 @@                                                                          
>>>>>>>>>>>  struct macb_dma_desc {                                                                       
>>>>>>>>>>  >      u32     addr;                                                                          
>>>>>>>>>>>       u32     ctrl;                                                                           
>>>>>>>>>>> +#ifdef CONFIG_ARCH_DMA_ADDR_T_64BIT                                                          
>>>>>>>>>>> +     u32     addrh;                                                                          
>>>>>>>>>>> +     u32     resvd;                                                                          
>>>>>>>>>>> +#endif                                                                                       
>>>>>>>>>>>  };                                                                                           
>>>>>>>>>>                                                                                                
>>>>>>>>>> It will not work for legacy hardware. Old descriptor is 2 words wide, the new one is 4 words wide.                                                                                                      
>>>>>>>>>> If you enable CONFIG_ARCH_DMA_ADDR_T_64BIT but hardware doesn't                                
>>>>>>>>>> support it at all, you will miss every second descriptor.                                      
>>>>>>>>>>                                                                                                
>>>>>>>>>                                                                                                 
>>>>>>>>> True, this feature is not available in all of Cadence IP versions.                              
>>>>>>>>> In fact, the IP version Zynq does not support this. But the one in ZynqMP does.                 
>>>>>>>>> So, we enable kernel config for 64 bit DMA addressing for this                                  
>>>>>>>>> SoC and hence the driver picks it up. My assumption was that if                                 
>>>>>>>>> the legacy IP does not support                                                                  
>>>>>>>>> 64 bit addressing, then this DMA option wouldn't be enabled.                                    
>>>>>>>>>                                                                                                 
>>>>>>>>> There is a design config register in Cadence IP which is being                                  
>>>>>>>>> read to check for 64 bit address support - DMA mask is set based on that.                       
>>>>>>>>> But the addition of two descriptor words cannot be based on this runtime check.                 
>>>>>>>>> For this reason, all the static changes were placed under this check.                           
>>>>>>>>                                                                                                  
>>>>>>>> We have quite a bunch of options in this driver to determinate what is the real capacity of the underlying hardware.                                                                                      
>>>>>>>> If HW configuration registers are not appropriate, and it seems they are not, I would advice to simply use the DT compatibility string.                                                                   
>>>>>>>>                                                                                                  
>>>>>>>> Best regards,                                                                                    
>>>>>>>> --                                                                                               
>>>>>>>> Nicolas Ferre                                                                                    
>>>>>>>                                                                                                   
>>>>>>> HW configuration registers are appropriate. The issue is that this code doesn’t use the capability bit to switch between different dma descriptors (2 words vs. 4 words).                                  
>>>>>>> DMA descriptor size is chosen based on kernel configuration, not based on hardware capabilities.  
>>>>>>                                                                                                    
>>>>>> HW configuration register does give appropriate information.                                       
>>>>>> But addition of two address words in the macb descriptor structure is a static change.             
>>>>>>                                                                                                    
>>>>>> +static inline void macb_set_addr(struct macb_dma_desc *desc,                                      
>>>>>> +dma_addr_t                                                                                        
>>>>>> +addr) {                                                                                           
>>>>>> +       desc->addr = (u32)addr;                                                                    
>>>>>> +#ifdef CONFIG_ARCH_DMA_ADDR_T_64BIT                                                               
>>>>>> +       desc->addrh = (u32)(addr >> 32); #endif                                                    
>>>>>> +                                                                                                  
>>>>>>                                                                                                    
>>>>>> Even if the #ifdef condition here is changed to HW config check, addr and addrh are different.     
>>>>>> And "addrh" entry has to be present for 64 bit desc case to be handled separately.                 
>>>>>> Can you please tell me how you propose change in DMA descriptor                                    
>>>>>> structure from                                                                                     
>>>>>> 4 to 2 or 2 to 4 words *after* reading the DCFG register?                                          
>>>>>                                                                                                     
>>>>> It is very complex problem. I wrote to you because I faced the same issue. I'm working on PTP implementation for macb. When PTP is enabled there are additional two words in buffer descriptor.              
>>>>
>>>> Moreover, we can use PTP without the 64bits descriptor support (early GEM revisions).
>>>>
>>>> BTW, note that there is an initiative ongoing with Andrei to support
>>>> PTP/1588 on these devices with patches already send: please synchronize with him.
>>>> https://urldefense.proofpoint.com/v2/url?u=https-3A__marc.info_-3Fl-3D
>>>> linux-2Dnetdev-26m-3D147282092309112-26w-3D3&d=DgIDaQ&c=aUq983L2pue2Fq
>>>> KFoP6PGHMJQyoJ7kl3s3GZ-_haXqY&r=MWSZN_jP4BHjuU9UFsrndALGeJqIXzD0WtGCtC
>>>> g4L5E&m=jUmZzqwn1uRrvcqF3BHCHFC6ZsKoZkU3vT8gJ-7fnsE&s=kr_km0MKQBzpkaOt
>>>> 0fkM-FIajN1-pOylzzTjsXi-vak&e=
>>>> or
>>>> https://urldefense.proofpoint.com/v2/url?u=https-3A__patchwork.kernel.
>>>> org_patch_9310989_&d=DgIDaQ&c=aUq983L2pue2FqKFoP6PGHMJQyoJ7kl3s3GZ-_ha
>>>> XqY&r=MWSZN_jP4BHjuU9UFsrndALGeJqIXzD0WtGCtCg4L5E&m=jUmZzqwn1uRrvcqF3B
>>>> HCHFC6ZsKoZkU3vT8gJ-7fnsE&s=ZbXVD5Lb5zGn7wUKIPYHxagIEKp_vJvrnkRa4qJfmT
>>>> Y&e=
>>>> and
>>>> https://urldefense.proofpoint.com/v2/url?u=https-3A__patchwork.kernel.
>>>> org_patch_9310991_&d=DgIDaQ&c=aUq983L2pue2FqKFoP6PGHMJQyoJ7kl3s3GZ-_ha
>>>> XqY&r=MWSZN_jP4BHjuU9UFsrndALGeJqIXzD0WtGCtCg4L5E&m=jUmZzqwn1uRrvcqF3B
>>>> HCHFC6ZsKoZkU3vT8gJ-7fnsE&s=Z0kRqUqh5Is4TmuaY7A4hnpizfdeq3HrgPhyDAMLuA
>>>> 8&e=
>>>>
>>>> Regards,
>>>> --
>>>> Nicolas Ferre
>>>
>>> Above patch doesn’t use hardware timestamps in descriptors at all. It doesn't use appropriate accuracy.
>>> We have our PTP patch ready, which use timestamps from descriptor. But we have not sent it yet because of compatibility issue.
>>> Of course, we can do it like Harini did:
>>>
>>> struct macb_dma_desc {
>>>         u32     addr;
>>>         u32     ctrl;
>>> #ifdef CONFIG_ARCH_DMA_ADDR_T_64BIT
>>>         u32     addrh;
>>>         u32     resvd;
>>> #endif
>>> #if IS_ENABLED(CONFIG_PTP_1588_CLOCK)
>>>         u32     dma_desc_ts_1;
>>>         u32     dma_desc_ts_2;
>>> #endif
>>> };
>>>
>>> But in that approach we lose backward compatibility.
>>>
>>> What are your suggestion? Can we send patch like it is or wait for some common solution with backward compatibility?
>>>
>>
>> Yes, Andre's version of Cadence does not ability to report have RX timestamp.
>> The version I worked with did. This is the old series I sent:
>> https://urldefense.proofpoint.com/v2/url?u=https-3A__lkml.org_lkml_2015_9_11_92&d=DgIFaQ&c=aUq983L2pue2FqKFoP6PGHMJQyoJ7kl3s3GZ-_haXqY&r=MWSZN_jP4BHjuU9UFsrndALGeJqIXzD0WtGCtCg4L5E&m=1A1qxdlwY3kMlJ1JUJJL1EqLzWz4AKtfOxZf_vu8bWo&s=NGpEbbC4bYXUZjJ6n0Ud8F8p3fPz5EhTJ1O9-NKwCkQ&e=
>> But right now, i'm working on building on top of Andre's changes.
>>
>> Regards,
>> Harini
>>
> I can’t see a place where you enable extended descriptor for PTP. Did you add support for extended PTP descriptor?
> "DMA Configuration Register" 0x010:
> 29	tx_bd_extended_mode_en	"Enable TX extended BD mode. See TX BD control register definition for description of feature."
> 28	rx_bd_extended_mode_en	"Enable RX extended BD mode. See RX BD control register definition for description of feature."
> 
> Can I send you our patch for comparison ? 

You mean "send you and everyone else on the mailing-list" right?
Even if the first initiative has a bit or priority, like Harini's one
ported further by Andrei, we are still eager to share code and ideas to
make this support go forward.

I do think that the steps will be to include generic support for PTP
(without 64bits descriptors) and then plug the 64bits descriptor support
which is an newer extension of it (and definitively more robust and
leading to more accurate behavior, I know). Basically like Harini
suggested: "building on top of Andrei's changes".

Best regards,
-- 
Nicolas Ferre

Powered by blists - more mailing lists