netdev - Re: [PATCH V6 net-next 06/11] net: Introduce a new MII time stamping interface.

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: <20191217092155.GL6994@lunn.ch>
Date:   Tue, 17 Dec 2019 10:21:55 +0100
From:   Andrew Lunn <andrew@...n.ch>
To:     Richard Cochran <richardcochran@...il.com>
Cc:     netdev@...r.kernel.org, David Miller <davem@...emloft.net>,
        devicetree@...r.kernel.org,
        Florian Fainelli <f.fainelli@...il.com>,
        Heiner Kallweit <hkallweit1@...il.com>,
        Jacob Keller <jacob.e.keller@...el.com>,
        Mark Rutland <mark.rutland@....com>,
        Miroslav Lichvar <mlichvar@...hat.com>,
        Murali Karicheri <m-karicheri2@...com>,
        Rob Herring <robh+dt@...nel.org>,
        Willem de Bruijn <willemb@...gle.com>,
        Wingman Kwok <w-kwok2@...com>
Subject: Re: [PATCH V6 net-next 06/11] net: Introduce a new MII time stamping
 interface.

> +static int dp83640_hwtstamp(struct mii_timestamper *mii_ts,
> +			    struct ifreq *ifr);
> +static int dp83640_ts_info(struct mii_timestamper *mii_ts,
> +			   struct ethtool_ts_info *info);
> +static bool dp83640_rxtstamp(struct mii_timestamper *mii_ts,
> +			     struct sk_buff *skb, int type);
> +static void dp83640_txtstamp(struct mii_timestamper *mii_ts,
> +			     struct sk_buff *skb, int type);
>  static void rx_timestamp_work(struct work_struct *work);

Hi Richard

Forward declarations are considered bad. Please add a new patch to the
series which moves code around first. You can probably move
dp83640_probe() further down.

> -static bool dp83640_rxtstamp(struct phy_device *phydev,
> +static bool dp83640_rxtstamp(struct mii_timestamper *mii_ts,
>  			     struct sk_buff *skb, int type)
>  {
> -	struct dp83640_private *dp83640 = phydev->priv;
> +	struct dp83640_private *dp83640 =
> +		container_of(mii_ts, struct dp83640_private, mii_ts);
>  	struct dp83640_skb_info *skb_info = (struct dp83640_skb_info *)skb->cb;
>  	struct list_head *this, *next;
>  	struct rxts *rxts;

David will probably complain about reverse christmas tree. Having to
fold dp83640_private is unfortunate here. Maybe consider adding a
macro for the container_of() so you can avoid the fold?

> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> index 0887ed2bb050..ee45838f90c9 100644
> --- a/drivers/net/phy/phy_device.c
> +++ b/drivers/net/phy/phy_device.c
> @@ -919,6 +919,8 @@ static void phy_link_change(struct phy_device *phydev, bool up, bool do_carrier)
>  			netif_carrier_off(netdev);
>  	}
>  	phydev->adjust_link(netdev);
> +	if (phydev->mii_ts && phydev->mii_ts->link_state)
> +		phydev->mii_ts->link_state(phydev->mii_ts, phydev);
>  }

FYI:

When using phylink, not phylib, this call will not happen. You need to
add a similar bit of code in phylink_mac_config().

For the moment what you have is sufficient. I doubt anybody is using
the dp83640 with phylink, and the new hardware you are targeting seems
to be RGMII based, not SERDES, which is the main use case for PHYLINK.

   Andrew