linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Jeff Garzik <jgarzik@mandrakesoft.com>
To: Bogdan Costescu <bogdan.costescu@iwr.uni-heidelberg.de>
Cc: Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
	netdev@oss.sgi.com, "David S. Miller" <davem@redhat.com>,
	Linus Torvalds <torvalds@transmeta.com>
Subject: Re: PATCH: ethtool MII helpers
Date: Tue, 12 Jun 2001 13:40:38 -0400	[thread overview]
Message-ID: <3B265416.58941C3C@mandrakesoft.com> (raw)
In-Reply-To: <Pine.LNX.4.33.0106121835340.22227-100000@kenzo.iwr.uni-heidelberg.de>

Bogdan Costescu wrote:
> On Sun, 10 Jun 2001, Jeff Garzik wrote:
> - I don't know what the long-term plan is about ethtool vs. MII ioctl's.
> If you do plan to replace completely the MII ioctl's, there should be a
> way to access _all_ MII registers provided by the PHY, even if you do this
> in a restricted way (i.e. for CAP_NET_ADMIN only). There is also useful
> info in other registers than the 4 you have in your implementation.

What are you doing that you need to access all registers from userspace?

On to your larger question, ethtool versus MII ioctls.   That is an
issue that weighs heavily on me.  Right now we have quite a bit of
deployed code using MII ioctls, and there is a gigabit MII standard; so,
Becker's argument is that each driver should provide a set of MII
ioctls, emulating behavior when hardware isn't exactly per spec.  (yes,
right now they are SIOCDEVPRIVATE, but that can be easily changed to
SIOCDEVMIIxxx)

David's argument is for ethtool, which originally comes out of the sparc
port (see include/asm-sparc/ethtool.h in older trees), and has been
around for a while, but doesn't enjoy the massive deployment that the
MII ioctls enjoy.  We have control over the ethtool API, and we can
correct its deficiencies, whereas any MII spec deficiencies must be
worked out inside the driver.  Further, there is the question of "how
much MII to implement" -- currently the MII-ioctl-based net drivers all
implement -basic- MII, but I guarantee that you will find
per-driver(per-chip) differences in the MII implementation... which is a
flaw in the MII ioctl implementation in the driver, regardless of how
the chip is designed.  There are completeness flaws in more than one MII
ioctl implementation.  Several drivers will return zeroes for the MII id
registers, for example.  The ethtool API doesn't have that problem.

For 2.4, my conclusion is:  for drivers that already implement MII
ioctls as SIOCDEVPRIVATE, continue to support those ioctls.  In
addition, support ethtool.  For drivers without support for either, just
add ethtool support.  The patch being discussed will cut down on
duplicate code for both cases.

Further, for the userland ethtool program, support for MII ioctls will
be added soon, so that there will be no need for additional mii-tool or
mii-diag tools.

For 2.5?  I don't know.  I am not a visionary.  I defer that to Linus
and David and Donald and Jamal and Alexey and...  I am mainly a
maintainer and merge monkey, only implementing new APIs when the needs
are blindingly obvious.


> - You are proposing some caching for the MII registers. I suppose that you
> would like to have this code also working with whatever caching will be
> done for MII access that was recently discussed. Wouldn't this produce
> double caching under some circumstances ?

You misunderstood the code.  The "caching" here is whatever is -already-
being done by the driver.  Many Becker-style drivers cache the
advertising value.  If such a driver uses the ethtool MII code, that is
one less MII read that needs to occur.

If the driver author wishes to cache more stuff, they have to do so in
the obvious way.  struct ethtool_mii_info is only used for helper
functions which are only used inside netdrv_ioctl().


> +       int speed;              /* 10, 100, 1000 or -1 (ask hw)         */
> 
> Please note that the comment specifies 1000, while the code in several
> places assumes only 2 possibilities: 10 and 100.

planning for the future :)  Yes, the code only supports 10/100, as I
mentioned in my introductory message.


> +       if (mii->autoneg < 0)
> +               autoneg = mii->autoneg = (bmcr & BMCR_ANENABLE) ? 1 : 0;
> +       else    autoneg = mii->autoneg;
> 
> You don't read anything from the hardware at this point. Why do you want
> caching ?

I don't understand your question.  Of course we have read BMCR from the
hardware at that point, read the code...


> Not related: I know that this comes from David Miller's older work, but
> wouldn't be possible to have a more uniform naming scheme ? You have
> BMCR_ANENABLE, but you have BMSR_ANEGCAPABLE...

capable != enable.. I prefer them different, so I am therefore
unmotivated to change anything ;-)


> +       if (mii->full_duplex < 0)
> +               full_duplex = mii->full_duplex =
> +                       mii_nway_result(negotiated) & LPA_DUPLEX;
> +       else    full_duplex = mii->full_duplex;
> 
> If autoneg. is disabled, I don't think that you always get useful info in
> 'negotiated'. Applies to the next chunk, too.
> 
> +       if (mii->speed < 0) {
> +               if (negotiated & LPA_100)
> +                       speed = mii->speed = 100;
> +               else
> +                       speed = mii->speed = 10;
> +       } else
> +               speed = mii->speed;

interesting point, thanks.


> +       ecmd->transceiver = XCVR_INTERNAL;
> 
> I didn't understand what XCVR_INTERNAL should mean as opposed to
> XCVR_EXTERNAL or whatever.

It is really up to interpretation of the individual driver author (or in
this case mii.c author), because the net core doesn't know nor care
about XCVR_xxx.


> +       if (advert != mii->advertising) {
> +               bmcr |= BMCR_ANRESTART;
> +               mii->mdio_write(dev, mii->phy_id, MII_ADVERTISE, advert);
> +               mii->advertising = advert;
> +       }
> +
> +       /* some phys need autoneg dis/enabled separately from other settings */
> +       if ((bmcr & BMCR_ANENABLE) && (!(mii->bmcr & BMCR_ANENABLE))) {
> +               mii->mdio_write(dev, mii->phy_id, MII_BMCR,
> +                               mii->bmcr | BMCR_ANENABLE | BMCR_ANRESTART);
> +               bmcr &= ~BMCR_ANRESTART;
> +       } else if ((!(bmcr & BMCR_ANENABLE)) && (mii->bmcr & BMCR_ANENABLE)) {
> +               mii->mdio_write(dev, mii->phy_id, MII_BMCR,
> +                               mii->bmcr & ~BMCR_ANENABLE);
> +       }
> 
> This is nice, but I would like to able to restart autonegotiation even
> without changing any of the advertised capabilities. If I missed this
> possibility, please point me to it...

no, that is a capability which needs to be added to ethtool. 
ETHTOOL_RENEG or ETHTOOL_ANRESTART or something.  Basically kick the
link state machine, whether such a state machine is in the driver or in
the MII phy.  That's the one big thing that mii-tool can do that ethtool
cannot, AFAICS.

	Jeff


-- 
Jeff Garzik      | Andre the Giant has a posse.
Building 1024    |
MandrakeSoft     |

  reply	other threads:[~2001-06-12 17:41 UTC|newest]

Thread overview: 14+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2001-06-10 17:34 Jeff Garzik
2001-06-11  5:59 ` Pekka Savola
2001-06-11  6:10   ` Keith Owens
2001-06-12 17:09 ` Bogdan Costescu
2001-06-12 17:40   ` Jeff Garzik [this message]
2001-06-12 18:40     ` PATCH: ethtool MII helpers (vers 2) Jeff Garzik
2001-06-13 15:29     ` PATCH: ethtool MII helpers Bogdan Costescu
2001-06-13 16:56     ` Donald Becker
2001-06-13 20:24       ` Jeff Garzik
2001-06-15 17:22         ` Bogdan Costescu
2001-06-22  5:10 ` Chris Wedgwood
2001-06-22  5:24   ` Jeff Garzik
2001-06-22  5:34     ` Chris Wedgwood
2001-06-22  5:58       ` Jeff Garzik

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=3B265416.58941C3C@mandrakesoft.com \
    --to=jgarzik@mandrakesoft.com \
    --cc=bogdan.costescu@iwr.uni-heidelberg.de \
    --cc=davem@redhat.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=netdev@oss.sgi.com \
    --cc=torvalds@transmeta.com \
    --subject='Re: PATCH: ethtool MII helpers' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).