linux-kernel - Re: [PATCH 05/11] mux: Add Intel Cherrytrail USB mux driver

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]

Message-ID: <70818328-5a80-abbc-2077-0ecac226951e@redhat.com>
Date:   Mon, 4 Sep 2017 16:07:56 +0200
From:   Hans de Goede <hdegoede@...hat.com>
To:     Andy Shevchenko <andy.shevchenko@...il.com>
Cc:     MyungJoo Ham <myungjoo.ham@...sung.com>,
        Chanwoo Choi <cw00.choi@...sung.com>,
        Guenter Roeck <linux@...ck-us.net>,
        Heikki Krogerus <heikki.krogerus@...ux.intel.com>,
        Darren Hart <dvhart@...radead.org>,
        Andy Shevchenko <andy@...radead.org>,
        Peter Rosin <peda@...ntia.se>,
        Mathias Nyman <mathias.nyman@...el.com>,
        Platform Driver <platform-driver-x86@...r.kernel.org>,
        devel@...verdev.osuosl.org,
        Kuppuswamy Sathyanarayanan 
        <sathyanarayanan.kuppuswamy@...ux.intel.com>,
        Sathyanarayanan Kuppuswamy Natarajan <sathyaosid@...il.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        USB <linux-usb@...r.kernel.org>
Subject: Re: [PATCH 05/11] mux: Add Intel Cherrytrail USB mux driver

Hi,

Thank you for the reviews!

On 02-09-17 12:19, Andy Shevchenko wrote:
> On Sat, Sep 2, 2017 at 12:48 AM, Hans de Goede <hdegoede@...hat.com> wrote:
>> Intel Cherrytrail SoCs have an internal USB mux for muxing the otg-port
>> USB data lines between the xHCI host controller and the dwc3 gadget
>> controller. On some Cherrytrail systems this mux is controlled through
>> AML code reacting on a GPIO IRQ connected to the USB OTG id pin (through
>> an _AIE ACPI method) so things just work.
>>
>> But on other Cherrytrail systems we need to control the mux ourselves
>> this driver exports the mux through the mux subsys, so that other drivers
>> can control it if necessary.
>>
>> This driver also updates the vbus-valid reporting to the dwc3 gadget
>> controller, as this uses the same registers as the mux. This is needed
>> for gadget/device mode to work properly (even on systems which control
>> the mux from their AML code).
>>
>> Note this depends on the xhci driver registering a platform device
>> named "intel_cht_usb_mux", which has an IOMEM resource 0 which points
>> to the Intel Vendor Defined XHCI extended capabilities region.
> 
>> +static void intel_cht_usb_mux_set_sw_mode(struct intel_cht_usb_mux *mux)
>> +{
>> +       u32 data;
>> +
>> +       data = readl(mux->base + DUAL_ROLE_CFG0);
> 
>> +       if (!(data & SW_IDPIN_EN)) {
> 
> Do we need it? I think this kind of microoptimixzations not worth it
> for most cases.

Heh, Peter Rosin (the mux subsys maintainer) was actually asking if
we could even get rid of doing the read all the time. Lets discuss
this further in my reply to Peter's reply.

> 
>> +               data |= SW_IDPIN_EN;
>> +               writel(data, mux->base + DUAL_ROLE_CFG0);
>> +       }
>> +}
> 
>> +       /* In most case it takes about 600ms to finish mode switching */
>> +       timeout = jiffies + msecs_to_jiffies(DUAL_ROLE_CFG1_POLL_TIMEOUT);
>> +
>> +       /* Polling on CFG1 register to confirm mode switch.*/
> 
>> +       while (1) {
> 
> do {} while (time_before()); ?

That means having to have a second conditional after the
loop to detect we timed-out and do the dev_warn, I would prefer to keep
this as is.

Regards,

Hans


> 
>> +               data = readl(mux->base + DUAL_ROLE_CFG1);
>> +               if (!!(data & HOST_MODE) == host_mode)
>> +                       break;
>> +
>> +               /* Interval for polling is set to about 5 - 10 ms */
>> +               usleep_range(5000, 10000);
>> +
>> +               if (time_after(jiffies, timeout)) {
>> +                       dev_warn(&mux_ctrl->chip->dev,
>> +                                "Timeout waiting for mux to switch\n");
>> +                       break;
>> +               }
>> +       }
> 
> 
>> +static void intel_cht_usb_mux_vbus_work(struct work_struct *work)
>> +{
>> +       struct intel_cht_usb_mux *mux =
>> +               container_of(work, struct intel_cht_usb_mux, vbus_work);
>> +       bool vbus_present = false;
>> +       int i;
> 
> unsigned int i; ?
> 
>> +
>> +       for (i = 0; i < ARRAY_SIZE(vbus_cable_ids); i++) {
>> +               if (extcon_get_state(mux->vbus_extcon, vbus_cable_ids[i]) > 0) {
>> +                       vbus_present = true;
>> +                       break;
>> +               }
>> +       }
>> +
>> +       intel_cht_usb_mux_set_vbus_valid(mux, vbus_present);
>> +}
> 
>> +static int intel_cht_usb_mux_probe(struct platform_device *pdev)
>> +{
>> +       struct device *dev = &pdev->dev;
>> +       struct intel_cht_usb_mux *mux;
>> +       struct mux_chip *mux_chip;
>> +       struct resource *res;
>> +       resource_size_t size;
> 
>> +       int i, ret;
> 
> Ditto.
> 
>> +       for (i = 0 ; i < ARRAY_SIZE(vbus_providers); i++) {
>> +               if (!acpi_dev_present(vbus_providers[i].hid, NULL,
>> +                                     vbus_providers[i].hrv))
>> +                       continue;
>