linux-kernel - Re: [PATCH 08/12] xhci: Add Intel extended cap / otg phy mux handling

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Sun, 25 Feb 2018 14:01:06 +0100
From:   Hans de Goede <hdegoede@...hat.com>
To:     Andy Shevchenko <andy.shevchenko@...il.com>
Cc:     Darren Hart <dvhart@...radead.org>,
        Andy Shevchenko <andy@...radead.org>,
        MyungJoo Ham <myungjoo.ham@...sung.com>,
        Chanwoo Choi <cw00.choi@...sung.com>,
        Mathias Nyman <mathias.nyman@...el.com>,
        Heikki Krogerus <heikki.krogerus@...ux.intel.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Platform Driver <platform-driver-x86@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        USB <linux-usb@...r.kernel.org>
Subject: Re: [PATCH 08/12] xhci: Add Intel extended cap / otg phy mux handling

Hi,

On 16-02-18 14:53, Andy Shevchenko wrote:
> On Fri, Feb 16, 2018 at 12:47 PM, Hans de Goede <hdegoede@...hat.com> wrote:
>> The xHCI controller on various Intel SoCs has an extended cap mmio-range
>> which contains registers to control the muxing to the xHCI (host mode)
>> or the dwc3 (device mode) and vbus-detection for the otg usb-phy.
>>
>> Having a role-sw driver included in the xhci code (under drivers/usb/host)
>> is not desirable. So this commit adds a simple handler for this extended
>> capability, which creates a platform device with the caps mmio region as
>> resource, this allows us to write a separate platform role-sw driver for
>> the role-switch.
>>
>> Note this commit adds a call to the new xhci_ext_cap_init() function
>> to xhci_pci_probe(), it is added here because xhci_ext_cap_init() must
>> be called only once. If in the future we also want to handle ext-caps
>> on non pci xHCI HCDs from xhci_ext_cap_init() a call to it should also
>> be added to other bus probe paths.
> 
> SPDX?

Ack, fixed for v2.

>> +/*
>> + * XHCI extended capability handling
>> + *
>> + * Copyright (c) 2017 Hans de Goede <hdegoede@...hat.com>
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 as
>> + * published by the Free Software Foundation.
>> + */
> 
>> +       pdev = platform_device_alloc("intel_xhci_usb_sw", PLATFORM_DEVID_NONE);
> 
> Perhaps,
> 
> #define USB_SW_DRV_NAME "..."

Ack, fixed for v2.

>> +       if (!pdev) {
>> +               xhci_err(xhci, "couldn't allocate intel_xhci_usb_sw pdev\n");
> 
> ...and re-use it everywhere here.
> 
> pdev -> platform device.
> 
>> +               return -ENOMEM;
>> +       }
>> +
>> +       res.start = hcd->rsrc_start + cap_offset;
>> +       res.end   = res.start + 0x3ff;
> 
> Is this magic always the same? Where its value comes from?
> At least define with comment.

Ack, I've added a USB_SW_RESOURCE_SIZE #define for this for v2.

>> +int xhci_ext_cap_init(struct xhci_hcd *xhci)
>> +{
>> +       void __iomem *base = &xhci->cap_regs->hc_capbase;
>> +       u32 cap_offset, val;
>> +       int ret;
>> +
>> +       cap_offset = xhci_find_next_ext_cap(base, 0, 0);
>> +
>> +       while (cap_offset) {
>> +               val = readl(base + cap_offset);
>> +
>> +               switch (XHCI_EXT_CAPS_ID(val)) {
>> +               case XHCI_EXT_CAPS_VENDOR_INTEL:
>> +                       if (xhci->quirks & XHCI_INTEL_USB_ROLE_SW) {
> 
>> +                               ret = xhci_create_intel_xhci_sw_pdev(
>> +                                                           xhci, cap_offset);
> 
> Can we leave xhci on previous line?

Fixed for v2.

>> +                               if (ret)
>> +                                       return ret;
>> +                       }
>> +                       break;
>> +               }
>> +               cap_offset = xhci_find_next_ext_cap(base, cap_offset, 0);
>> +       }
>> +
>> +       return 0;
>> +}
> 
> 

Regards,

Hans