Hi Marcel,
On Wed, Apr 11, 2012 at 01:04:22AM +0200, Marcel Schaal wrote:
> Hi Sarah,
>
> please find attached a patch that disables MSI for Freso Logic 1400 USB3
> controllers. I own a Asus G73Jw notebook which uses one of these
> controllers. It doesn't work using stock Fedora 15/16 (Kernel 2.4.42 to
> 3.3.1), but starts working if "pci=nomsi" is applied to Linux's grub
> command line.
> In commit [1] you say that "Some Fresco Logic hosts ...
> advertise MSI, but fail to actually generate MSI interrupts". I think this
> 1400 controller might be one of these, but I don't have any other computer
> that feature one of these devices. So I can't test it any further. I
> attached output [2] from lspci for this host as well.
Thanks for the patch. I suspected that there were more Fresco Logic
hosts that had this issue, so I'm glad you figured out how to fix it.
> This is my first Linux patch, so feel free to comment or modify on it.
Ok, in general, you should Cc the linux-usb mailing list (or the mailing
list for the project you're patching) instead of just mailing the
maintainer. That way if I happen to drop a patch, someone else will
notice. It also helps get more eyes on your patches to catch more bugs.
Also, when you send a patch, you need to send it inline in the body of
an email, not as an attachment. That makes it easier for people to
review, and easier for the maintainer to apply. It looks like your
patch contained all the same information as your email, which is good
because we want the important info to go into the description of the
patch, not get lost in the mailing list archives.
If you want to make comments that you don't want included in your patch
description, you can add them after the --- line, before the diff. git
will ignore any comments placed there. It would look something like
this:
>From e51e2a3b8e2c1693166cfef5f59cec261a666d65 Mon Sep 17 00:00:00 2001
From: Marcel Schaal <mail@xxxxxxxxxxxxxxx>
Date: Wed, 11 Apr 2012 00:21:55 +0200
Subject: [PATCH 1/1] xhci: Disable MSI for Fresco Logic 1400 hosts
This is a follow up fix for commit f5182b4155b9d686c5540a6822486400e34ddd98,
which disables MSI on several Fresco Logic hosts. The 1400 host on my Asus
G73Jw advertises MSI, but it doesn't work when using any USB device on this
host. Booting Linux with nomsi parameter makes the host work fine.
Signed-off-by: Marcel Schaal <mail@xxxxxxxxxxxxxxx>
---
This comment will be ignored.
drivers/usb/host/xhci-pci.c | 6 ++++--
As for your actual patch:
> From e51e2a3b8e2c1693166cfef5f59cec261a666d65 Mon Sep 17 00:00:00 2001
> From: Marcel Schaal <mail@xxxxxxxxxxxxxxx>
> Date: Wed, 11 Apr 2012 00:21:55 +0200
> Subject: [PATCH 1/1] xhci: Disable MSI for Fresco Logic 1400 hosts
You don't need to pass the -n flag to git format patch if there's only
one patch. So the 1/1 is unnecessary.
In general, what I do after I use git format-patch is to run `mutt -H
file.patch` Then that creates an email with all the right headers.
> This is a follow up fix for commit f5182b4155b9d686c5540a6822486400e34ddd98,
> which disables MSI on several Fresco Logic hosts. The 1400 host on my Asus
> G73Jw advertises MSI, but it doesn't work when using any USB device on this
> host. Booting Linux with nomsi parameter makes the host work fine.
>
> Signed-off-by: Marcel Schaal <mail@xxxxxxxxxxxxxxx>
> ---
> drivers/usb/host/xhci-pci.c | 6 ++++--
> 1 files changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/usb/host/xhci-pci.c b/drivers/usb/host/xhci-pci.c
> index ef98b38..d9e6e75 100644
> --- a/drivers/usb/host/xhci-pci.c
> +++ b/drivers/usb/host/xhci-pci.c
> @@ -29,6 +29,7 @@
> /* Device for a quirk */
> #define PCI_VENDOR_ID_FRESCO_LOGIC 0x1b73
> #define PCI_DEVICE_ID_FRESCO_LOGIC_PDK 0x1000
> +#define PCI_DEVICE_ID_FRESCO_LOGIC_PDK2 0x1400
Please call this new PCI device ID PCI_DEVICE_ID_FRESCO_LOGIC_ASUS_G73.
"PDK" meant product development kit from the USB-IF. Your host
controller is a real product, not a kit part, so we'll pick a different
name.
> #define PCI_VENDOR_ID_ETRON 0x1b6f
> #define PCI_DEVICE_ID_ASROCK_P67 0x7023
> @@ -57,8 +58,9 @@ static void xhci_pci_quirks(struct device *dev, struct xhci_hcd *xhci)
> struct pci_dev *pdev = to_pci_dev(dev);
>
> /* Look for vendor-specific quirks */
> - if (pdev->vendor == PCI_VENDOR_ID_FRESCO_LOGIC &&
> - pdev->device == PCI_DEVICE_ID_FRESCO_LOGIC_PDK) {
> + if (pdev->vendor == PCI_VENDOR_ID_FRESCO_LOGIC && (
Please move that '(' down to the start of the next line. Dangling
parenthesis look odd.
Did you run scripts/checkpatch.pl on this patch? You can do that with a
command like this:
git show HEAD | perl scripts/checkpatch.pl -
That script will warn you about any coding style issues. You might want
to read Documentation/CodingStyle and Documentation/SubmittingPatches as
well.
> + pdev->device == PCI_DEVICE_ID_FRESCO_LOGIC_PDK ||
> + pdev->device == PCI_DEVICE_ID_FRESCO_LOGIC_PDK2)) {
> if (pdev->revision == 0x0) {
> xhci->quirks |= XHCI_RESET_EP_QUIRK;
> xhci_dbg(xhci, "QUIRK: Fresco Logic xHC needs configure"
Otherwise the patch looks fine.
Sarah Sharp
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
