netdev - Re: [PATCH net-next] 1588 support on bcm54210pe

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: <01f35484-e8b6-d0bb-dba7-d1e0407c00ca@gmail.com>
Date:   Sat, 23 Apr 2022 07:40:49 -0700
From:   Florian Fainelli <f.fainelli@...il.com>
To:     Richard Cochran <richardcochran@...il.com>,
        Andrew Lunn <andrew@...n.ch>
Cc:     Lasse Johnsen <lasse@...ebeat.app>, netdev@...r.kernel.org,
        Gordon Hollingworth <gordon@...pberrypi.com>,
        Ahmad Byagowi <clk@...com>,
        Heiner Kallweit <hkallweit1@...il.com>,
        Russell King <linux@...linux.org.uk>,
        bcm-kernel-feedback-list@...adcom.com,
        Florian Fainelli <f.fainelli@...il.com>
Subject: Re: [PATCH net-next] 1588 support on bcm54210pe



On 4/22/2022 12:48 PM, Richard Cochran wrote:
> On Fri, Apr 22, 2022 at 05:00:07PM +0200, Andrew Lunn wrote:
> 
>>> I am confident that this code is relevant exclusively to the
>>> BCM54210PE.
> 
> Not true.
> 
>> It will not even work with the BCM54210, BCM54210S and
>>> BCM54210SE PHYs.
> 
> The registers you used are also present in the BCM541xx devices.
> Pretty sure your code would work on those devices (after adjusting
> register offsets).
> 
>> Florian can probably tell us more, but often hardware like this is
>> shared by multiple devices. If it is, you might want to use a more
>> generic prefix.
> 
> My understanding is that there are two implementions, gen1 and gen2.
> Your bcm542xx and the bcm541xx are both gen1, and both support inband
> Rx time stamping.

That is correct. Lasse for your future submission please address the 
following:

- conform to the usual patch submission style and break up your changes 
between bcmgenet.c (although I doubt you need to change it), broadcom.c 
and bcm-phy-lib.[ch]

- do not create a PHY device entry specifically for BCM54210PE, use the 
existing BCM54210 entry and add checks using the revision field of 
phydev->phy_id where necessary. There are already many entries in this 
driver, adding more does not help maintaining it. Also, I went through 
several months of work fixing bugs and adding decent power management 
features to this driver that all PHYs should leverage, adding a new 
entry means we need to verify whether all code paths are hit or not

- move generic code, such as all of the PTP code into bcm-phy-lib.[ch] 
where it can easily be re-used across multiple PHY device driver entries 
(54810, 54210 etc.)

Thanks!

> 
> Because the registers are all the same (just the offsets are
> different), I'd like to see a common module that can be used by all
> gen1 devices.  The module could be named bcm-ptp-gen1.c for example.

I would prefer that we just stick to adding that code to 
bcm-phy-lib.[ch] which all Broadcom PHY drivers can use and we can 
decide whether we want to add a Kconfig option specifically for PTP.

Cheers
-- 
Florian