PATCH: arch/ppc/8xx_io_fec.c

PATCH: arch/ppc/8xx_io_fec.c

[Jean-Denis Boyer]

[-- Attachment #1: Type: text/plain, Size: 2087 bytes --]


I discovered an old bug in the Fast Ethernet driver of the 860T.
In function fec_restart, the FEC is reset with no concern
to pending MDIO requests.

If an MDIO request is being served while the FEC is reset,
it is lost, causing the system to hang if it is waiting for
the answer (MDIO read). It may also lose a register modification
request (MDIO write).

1 - Especially at boot up, in function fec_enet_open, we could hang on:
        while(!fep->sequence_done)
            schedule();
2 - Or, the worst, in function mdio_read called through the ioctl,
    which would hang the calling process.

I encountered only the first problem, hanging at boot up just after printing:
  eth0: Phy @ 0x1, type DP83846A (0x20005c23)
The problem was extremely rare, due to a link down/link up sequence
that occurs at every boot up, and probably an MDIO request queued just
before the time of the FEC reset.

Attached is a patch (over 2.4.21-rc3) that adds two functions:
  static void stop_mii_queue(struct net_device *dev)
  static void start_mii_queue(struct net_device *dev)
that are both called by fec_restart.
The tests I performed easily demonstrates that we no longer loose
an MDIO request when fec_restart is called.

I also cleanen up some code around the MDIO management.

1 - In function mii_link_interrupt, I removed the code that juggles with
    the bit ECNTRL[ETHER_EN]. It is completly useless since the MDIO will
    work no matter the state of this bit. It can also as bad effects
    to put this bit to 1 without initializing the rest properly.
2 - In function fec_stop, I simplified the code, since there is no need
    to reconfigure all the registers after a graceful transmitter stop.
    Especially, the code that "Clear outstanding MII command interrupts."
    should not be there.

I'm waiting for comments.

--------------------------------------------
 Jean-Denis Boyer, Software Designer
 M5T Inc.
 4283 rue Garlock
 Sherbrooke (Québec)
 J1L 2C8  CANADA
 (819)829-3972 x241
--------------------------------------------

[-- Attachment #2: patch_8xx_fec_20030526.diff.gz --]
[-- Type: application/x-gzip, Size: 1449 bytes --]

Re: PATCH: arch/ppc/8xx_io_fec.c

[Tom Rini]

On Tue, May 27, 2003 at 10:38:49AM -0400, Jean-Denis Boyer wrote:

> I discovered an old bug in the Fast Ethernet driver of the 860T.
> In function fec_restart, the FEC is reset with no concern
> to pending MDIO requests.

Dan, do you see any problems with this?  If not, I'm going to put this
into a tree to get it out and into 2.4.22-pre.

--
Tom Rini
http://gate.crashing.org/~trini/

** Sent via the linuxppc-embedded mail list. See http://lists.linuxppc.org/

Re: PATCH: arch/ppc/8xx_io_fec.c

[Dan Malek]

Tom Rini wrote:

> Dan, do you see any problems with this?

Yes.  There are silicon revisions that require the FEC to
be enabled for the MDIO to operate.  I don't remember if it was
listed in errata or in some of the early supplements.  This code
change may work in this particular case, but I know it's required
for some parts.  I have not looked at the detail of the patch
and the code it moves to see if this is done elsewhere.

The code formatting looks like crap, so that needs to be fixed.

It would be nice to hear from other people that have recently made
changes for specific board/PHY combinations to see if this patch
affects them.

Thanks.


	-- Dan


** Sent via the linuxppc-embedded mail list. See http://lists.linuxppc.org/

Re: PATCH: arch/ppc/8xx_io_fec.c

[Tom Rini]

On Tue, May 27, 2003 at 03:33:52PM -0400, Dan Malek wrote:

> Tom Rini wrote:
>
> >Dan, do you see any problems with this?
>
> Yes.  There are silicon revisions that require the FEC to
> be enabled for the MDIO to operate.  I don't remember if it was
> listed in errata or in some of the early supplements.  This code
> change may work in this particular case, but I know it's required
> for some parts.  I have not looked at the detail of the patch
> and the code it moves to see if this is done elsewhere.

How would you suggest working around both this problem, and the problems
that Jean-Denis reported?  Having either be broken for the sake of the
other can't be acceptiable, can it? :)

> The code formatting looks like crap, so that needs to be fixed.

That's trivial 'tho. :)

--
Tom Rini
http://gate.crashing.org/~trini/

** Sent via the linuxppc-embedded mail list. See http://lists.linuxppc.org/

Re: PATCH: arch/ppc/8xx_io_fec.c

[Dan Malek]

Tom Rini wrote:

> How would you suggest working around both this problem, and the problems
> that Jean-Denis reported?

I understand the problem that needed to be fixed, and that was done.
It seems quite a few people have been making changes do this driver,
logic has been copied around and some of it doesn't make sense.

In your usual, "send me lots of patches" mode, there should have been
one to fix the real problem, and another to do "clean up." :-)

>>The code formatting looks like crap, so that needs to be fixed.
>
>
> That's trivial 'tho. :)

If you have time to do it, that's fine.  I don't :-)

Thanks.


	-- Dan


** Sent via the linuxppc-embedded mail list. See http://lists.linuxppc.org/

RE: PATCH: arch/ppc/8xx_io_fec.c

[Jean-Denis Boyer]

> Yes.  There are silicon revisions that require the FEC to
> be enabled for the MDIO to operate.

I tested on revisions B3 and D4, and did not see any problem.
Anyway, fec_stop was already disabling the FEC,
so if it was a problem, it would have broken there.

> The code formatting looks like crap, so that needs to be fixed.

Do you mean for the mixed tabs/spaces? :-( Sorry for that one...
I will fix and resubmit as soon as everyone agree on the changes.

Regards,

Jean-Denis Boyer

** Sent via the linuxppc-embedded mail list. See http://lists.linuxppc.org/

Re: PATCH: arch/ppc/8xx_io_fec.c

[Dan Malek]

Jean-Denis Boyer wrote:

> I tested on revisions B3 and D4, and did not see any problem.
> Anyway, fec_stop was already disabling the FEC,
> so if it was a problem, it would have broken there.

OK.  Just remember I mentioned this and be ready to fix it
when someone complains. :-)

> Do you mean for the mixed tabs/spaces? :-( Sorry for that one...
> I will fix and resubmit as soon as everyone agree on the changes.

Yes, and there is a Linux coding standards document that would be
nice if people followed:

	if (condition) {

not,
	if(condition){, or if( condition ){, or other weird variations

Thanks.


	-- Dan


** Sent via the linuxppc-embedded mail list. See http://lists.linuxppc.org/

Re: PATCH: arch/ppc/8xx_io_fec.c

[Steven Rothweiler]

>
> > Do you mean for the mixed tabs/spaces? :-( Sorry for that one...
> > I will fix and resubmit as soon as everyone agree on the changes.
>
> Yes, and there is a Linux coding standards document that would be
> nice if people followed:
>
> 	if (condition) {
>
> not,
> 	if(condition){, or if( condition ){, or other weird variations
>
> Thanks.
>
>
> 	-- Dan
>

I agree; I hate those "weird variations".

Did someone already coin FTFCS (Follow The <bleeping> Coding Standards)?
I think it applies here.

** Sent via the linuxppc-embedded mail list. See http://lists.linuxppc.org/