netdev - Re: [PATCH 0/2 net-next] phy: allow flagging a device as internal

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>] [day] [month] [year] [list]

Message-ID: <1369158482.2615.42.camel@bwh-desktop.uk.solarflarecom.com>
Date:	Tue, 21 May 2013 18:48:02 +0100
From:	Ben Hutchings <bhutchings@...arflare.com>
To:	Florian Fainelli <f.fainelli@...il.com>
CC:	David Miller <davem@...emloft.net>, <netdev@...r.kernel.org>,
	<afleming@...escale.com>, <jogo@...nwrt.org>
Subject: Re: [PATCH 0/2 net-next] phy: allow flagging a device as internal

On Tue, 2013-05-21 at 18:25 +0100, Florian Fainelli wrote:
> 2013/5/21 Ben Hutchings <bhutchings@...arflare.com>
>         On Tue, 2013-05-21 at 11:37 +0100, Florian Fainelli wrote:
>         > David, Andy,
>         >
>         > This small patch set updates libphy to allow PHY drivers to
>         flag a
>         > specific PHY device as internal, which is then used to
>         accurately
>         > report the transceiver type (internal vs external) in
>         ethtool.
>         >
>         > As far as I can tell we only have one in-tree driver for the
>         moment
>         > which is known to be for internal PHYs.
>         
>         
>         I don't think you should make any change like this until there
>         is proper
>         documentation of what the 'transceiver' field actually means.
>         
>         It appears that XCVR_EXTERNAL was originally used for a
>         transceiver
>         module external to the system, usually connected to an AUI
>         port.  The
>         modern equivalent of that might be SFP+ and similar module
>         slots, but
>         they're a bit different because the Physical Coding Sublayer
>         is usually
>         part of the controller.
>         
>         Many newer drivers are using XCVR_EXTERNAL to indicate that
>         the PHY is a
>         separate package from the controller, which is what you seem
>         to be doing
>         here.  But nowhere is that specified!
> 
> 
> Well, it depends how you define "specified", I can see quite a lot of
> drivers actually defining it to match the definition you gave above,
> so the de facto situation is like you describe it.

I would like to see someone (you?) provide a longer comment in ethtool.h
that says what XCVR_INTERNAL and XCVR_EXTERNAL are supposed to mean (the
other values are obviously driver-specific so don't matter as much).

>         The transceiver field doesn't even really matter much unless
>         the user
>         has a choice of which to use. 
> 
> 
> This is about reporting accurate information, the field is not
> user-changeable using libphy's ethtool knobs.

The information cannot be accurate without also being clear.

>         Does anyone make boards like that any
>         more?  And it's probably entirely redundant with the port and
>         phy_address fields in that case, anyway.
> 
> 
> At least I need to know from my driver if the PHY is an internal one
> (package perspective) or not because there is a different
> configuration path depending on this. I could just drop the hunk which
> sets cmd->transceiver then.

I think that makes more sense.

Ben.

-- 
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

--
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