DomainKey-Signature: a=rsa-sha1; c=nofws;
        d=gmail.com; s=gamma;
        h=mime-version:in-reply-to:references:date:message-id:subject:from:to
         :cc:content-type:content-transfer-encoding;
        b=jeazQJeX+6/oAvW/oXuRR/XiNV0BuynZvjOI/TxG5UqPsyhmfryPcP5u2E/leGpznb
         FMSrP60Cnfai26yDkFNV+90A5E7hEnyIQqozp7PpPDAOMmkVHEw5eaXheZtgzB4hMKvu
         SJFkcTNwi0piIn7tOBl92KjLwfV26bzFHUeOE=
MIME-Version: 1.0
In-Reply-To: <20100218052223.GA13254@kroah.com>
References: <4B7CAF95.6020306@gmail.com> <20100218042626.GB11649@kroah.com>
	 <51f3faa71002172113o65a85ed6y5fa39d0b3a96f7ce@mail.gmail.com>
	 <20100218052223.GA13254@kroah.com>
Date: Thu, 18 Feb 2010 18:33:42 -0600
Message-ID: <51f3faa71002181633w1649a648s37ae73da342d0c3f@mail.gmail.com>
Subject: Re: [PATCH 2.6.34] ehci-hcd: add option to enable 64-bit DMA support
From: Robert Hancock <hancockrwd@gmail.com>
To: Greg KH <greg@kroah.com>
Cc: linux-kernel <linux-kernel@vger.kernel.org>,
       Linux-usb <linux-usb@vger.kernel.org>, dbrownell@users.sourceforge.net
Content-Type: text/plain; charset=ISO-8859-1
Content-Transfer-Encoding: 8BIT
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 5433
Lines: 109

On Wed, Feb 17, 2010 at 11:22 PM, Greg KH <greg@kroah.com> wrote:
> On Wed, Feb 17, 2010 at 11:13:05PM -0600, Robert Hancock wrote:
>> On Wed, Feb 17, 2010 at 10:26 PM, Greg KH <greg@kroah.com> wrote:
>> > On Wed, Feb 17, 2010 at 09:10:13PM -0600, Robert Hancock wrote:
>> >> Add a module parameter to allow the user to enable 64-bit DMA support in EHCI,
>> >> which has been forcibly disabled since 2003 - see:
>> >>
>> >> http://www.mail-archive.com/linux-usb-devel@lists.sourceforge.net/msg17230.html
>> >>
>> >> At that time the comment was "it'd only matter on a few big Intel boxes anyway",
>> >> however the situation is much different today when many new machines have 4GB
>> >> or more of RAM and IOMMU/SWIOTLB are thus needlessly required for USB transfers.
>> >> For now, the support remains disabled by default and is controlled by an
>> >> allow_64bit module parameter.
>> >>
>> >> Note that some USB device drivers may require updates to pass the DMA
>> >> capabilities up to their higher layers to avoid unnecessary IOMMU or bounce-
>> >> buffer use (i.e. networking layer NETIF_F_HIGHDMA). Some of these checks were
>> >> disabled by the patch listed above, and more may be required again today.
>> >> However, those previous checks were done incorrectly using dma_supported,
>> >> which checks to see whether a device's DMA mask can be validly set to a given
>> >> mask, not whether its previously set mask will accomodate the mask passed in.
>> >>
>> >> Signed-off-by: Robert Hancock <hancockrwd@gmail.com>
>> >
>> > What is the "advantage" that setting this option would allow people to
>> > do that the code currently does not? ?Is such an advantage measurable at
>> > the slow rates that the EHCI driver controls?
>>
>> I expect it would likely be quite system-dependent. However,
>> particularly with devices like high-speed USB storage on systems with
>> lots of RAM, especially 8GB or more (where more of the memory can't be
>> addressed with 32-bit than can), I suspect it would likely be
>> measurable in terms of CPU usage, throughput or both, especially if
>> there's no hardware IOMMU and software bounce-buffering is required.
>
> So you did not measure it?
>
> Hm, I guess this change must not be necessary :)

I'll try and run some tests and see what I can quantify. However, I
only have 4GB of RAM on my machine (with a 1GB memory hole) and so a
random memory allocation only has a 25% chance of ending up in the
area where it would make a difference, so it may take a bit of doing.

>
>> > Is there any way to dynamically figure out if we can enable this or not?
>> > Adding module paramaters sucks, as they are hard to configure for most
>> > users, and they tend to be ignored.
>>
>> Well, the option only has an effect if the controller indicates the
>> 64-bit addressing feature flag in the first place. The only issue
>> would be with potential controllers that report the feature flag but
>> it doesn't work. Based on the libata experience with AHCI which has a
>> similar 64-bit feature flag, there are some controllers that didn't do
>> it properly, but not many (only ATI SB600 it appears, and only on
>> certain boards, as it seems it was a BIOS configuration issue).
>>
>> Having the ability to turn it on manually for testing would likely be
>> the first step towards turning it on for all controllers that indicate
>> support, not the end goal in itself.
>
> But we disabled it on purpose, because of problems, do we want those
> problems again?

AFAICS, it was disabled because of problems with kernel code, not with
hardware (and it appears the issue was with the code that detected the
expanded DMA mask in the USB device driver code, not the HCD driver).
CCing David Brownell who may know more.

>
>> > And are you really ok with enabling this on a system-wide level, and not
>> > on a per-controller level? ?Does that work properly on all systems?
>>
>> As above, the only issue is if the controller reports the capability
>> but it doesn't function properly. If a user runs into the case where
>> one of their controllers doesn't work with it enabled, that likely
>> means that we need to add blacklisting for that device ID, etc. (or
>> blacklist 64-bit DMA for their platform in general).
>
> blacklists are hard to keep up to date, you are always handling bug
> reports after-the-fact. ?That's not something you should add lightly.

I'm not a fan of blacklists in general (we do have too many of them),
but I don't expect a lot of entries in here. Having a device that
explicitly indicates it does something it can't do seems unlikely
(especially when the capability flag is most likely implemented in
silicon and not by the BIOS writer..)

>
>> > And if the system does not support it, and a user enables it, who is
>> > going to support their broken system? ?:)
>>
>> That would be me/us :-) We need those reports though, to know how to
>> proceed, and better to be from those who opted into testing for now..
>
> But we need a _reason_ to enable it.
>
> What user needs this?
>
> And if there is no speed benifit, why take the risk?

I expect there will be a speed benefit on some configurations. The
only question is how much..
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/