netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: David Woodhouse <dwmw2@infradead.org>
To: Jeff Garzik <jgarzik@pobox.com>
Cc: Ben Hutchings <bhutchings@solarflare.com>,
	Jason Wang <jasowang@redhat.com>,
	"David S. Miller" <davem@davemloft.net>,
	netdev@vger.kernel.org
Subject: Re: 8139cp: set ring address before enabling receiver
Date: Wed, 21 Nov 2012 21:00:37 +0000	[thread overview]
Message-ID: <1353531637.26346.179.camel@shinybook.infradead.org> (raw)
In-Reply-To: <50AD3C51.9070105@pobox.com>

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

On Wed, 2012-11-21 at 15:40 -0500, Jeff Garzik wrote:
> A larger point is that the commit was created to avoid imagined
> disaster on simulated hardware...

In their defence, I suspect that qemu/kvm is probably now the most
common type of RTL8139 on Linux deployments :)

And since KVM is now capable of supporting an IOMMU, which most *real*
boxes with RTL8139 won't have, it was probably a *real* problem rather
than just an imagined one.

>   ...and wound up creating behavior that is (a) contra to the data
> sheet and (b) breaks real hardware.

And again in their defence... the data sheet does appear to be
suggesting something completely stupid. The patch I just submitted
doesn't do what the data sheet says *either*, although I did at least
test it on real hardware. How many versions of the 8139C+ are there?
Should I be looking for more testing on different revisions?

I had a quick play with the Cfg9346 register. I note that when you set
it to 0x80 it *does* disable both network and bus mastering... and we
set it to the 'Write Enable' value 0xC0 while we're configuring
everything. I wondered if that might perhaps be the thing that made the
original behaviour, and the recommendation in the data sheet, sane.

But it doesn't disable operation when it's in the "Unlock" mode. I tried
setting the driver's value of Cfg9346_Lock to 0xC0 (i.e. leave it
write-enabled at all times), hoping that it would then fail to do any
DMA and prove that the original code was actually safe after all. But
the driver is working fine.

-- 
dwmw2


[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 6171 bytes --]

  reply	other threads:[~2012-11-21 21:00 UTC|newest]

Thread overview: 22+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
     [not found] <20120602235020.2C0A57C006C@ra.kernel.org>
2012-11-21 16:57 ` 8139cp: set ring address before enabling receiver David Woodhouse
2012-11-21 18:12   ` Jeff Garzik
2012-11-21 19:51     ` David Woodhouse
2012-11-21 20:18       ` Ben Hutchings
2012-11-21 20:40         ` Jeff Garzik
2012-11-21 21:00           ` David Woodhouse [this message]
2012-11-21 21:10           ` Ben Hutchings
2012-11-21 20:27     ` [PATCH] 8139cp: set ring address after enabling C+ mode David Woodhouse
2012-11-21 20:40       ` Francois Romieu
2012-11-21 22:32         ` David Woodhouse
2012-11-21 22:40           ` David Miller
2012-11-21 22:52             ` David Woodhouse
2012-11-21 23:12               ` David Miller
2012-11-22  3:47       ` Jeff Garzik
2012-11-22  4:39         ` David Miller
2012-11-22  4:53           ` Jeff Garzik
2012-11-22  5:30             ` Jason Wang
2012-11-22 21:39             ` Francois Romieu
2012-11-22 23:12               ` David Woodhouse
2012-11-23 12:37               ` Gilboa Davara
2012-11-23  3:53             ` Jason Wang
2012-11-25 20:54       ` David Miller

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=1353531637.26346.179.camel@shinybook.infradead.org \
    --to=dwmw2@infradead.org \
    --cc=bhutchings@solarflare.com \
    --cc=davem@davemloft.net \
    --cc=jasowang@redhat.com \
    --cc=jgarzik@pobox.com \
    --cc=netdev@vger.kernel.org \
    /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
Be sure your reply has a Subject: header at the top and a blank line before the message body.
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).