Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Subject: Re: [PATCH] usb: roles: Add PM callbacks
To:     "Chen, Hu" <hu1.chen@intel.com>
Cc:     Balaji <m.balaji@intel.com>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org
References: <20190708022514.7161-1-hu1.chen@intel.com>
From:   Hans de Goede <hdegoede@redhat.com>
Message-ID: <8d304bb6-9849-ea77-3bd6-fe378de78ce7@redhat.com>
Date:   Mon, 8 Jul 2019 16:10:23 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101
 Thunderbird/60.7.0
MIME-Version: 1.0
In-Reply-To: <20190708022514.7161-1-hu1.chen@intel.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Hi,

On 08-07-19 04:25, Chen, Hu wrote:
> On some Broxton NUC, the usb role is lost after S3 (it becomes "none").
> Add PM callbacks to address this issue: save the role during suspend and
> restore usb to that role during resume.
> 
> Test:
> Run Android on UC6CAY, a NUC powered by Broxton. Access this NUC via
> "adb shell" from a host PC. After a suspend/resume cycle, the adb still
> works well.
> 
> Signed-off-by: Chen, Hu <hu1.chen@intel.com>
> Signed-off-by: Balaji <m.balaji@intel.com>

Hmm, but what if the user has say unplugged a host-adapter with a usb-stick
in there and plugged in a cable to a computer *while suspended* because the
battery was getting low?

I see 2 scenarios here:

1) We may get an interrupt for the role change, followed by an
intel_xhci_usb_set_role. In this case the interrupt handling may happen
before the intel_xhci_usb_resume call and we would end up restoring the
wrong value

2) The role may be changed underneath us by the firmware / AML code
without us ever becoming aware of this.

For the specific problem discussed in the commit message it might be better
to do something like:

static int intel_xhci_usb_resume(struct platform_device *pdev)
{
	struct intel_xhci_usb_data *data = platform_get_drvdata(pdev);
	struct device *dev = &pdev->dev;

	if (intel_xhci_usb_get_role(dev) == USB_ROLE_NONE &&
	    power_supply_is_system_supplied())
		intel_xhci_usb_set_role(dev, USB_ROLE_DEVICE);

	return 0;
}

But that is somewhat specific for solving the problem described in
the commit message, but it has the advantage of not being able to
cause any regressions (that I can think of).

Regards,

Hans


> 
> diff --git a/drivers/usb/roles/intel-xhci-usb-role-switch.c b/drivers/usb/roles/intel-xhci-usb-role-switch.c
> index 277de96181f9..caa1cfab41cc 100644
> --- a/drivers/usb/roles/intel-xhci-usb-role-switch.c
> +++ b/drivers/usb/roles/intel-xhci-usb-role-switch.c
> @@ -37,6 +37,7 @@
>   struct intel_xhci_usb_data {
>   	struct usb_role_switch *role_sw;
>   	void __iomem *base;
> +	enum usb_role role;
>   };
>   
>   static int intel_xhci_usb_set_role(struct device *dev, enum usb_role role)
> @@ -167,6 +168,30 @@ static int intel_xhci_usb_remove(struct platform_device *pdev)
>   	return 0;
>   }
>   
> +static int intel_xhci_usb_suspend(struct platform_device *pdev,
> +				  pm_message_t state)
> +{
> +	struct intel_xhci_usb_data *data = platform_get_drvdata(pdev);
> +	struct device *dev = &pdev->dev;
> +
> +	data->role = intel_xhci_usb_get_role(dev);
> +
> +	return 0;
> +}
> +
> +static int intel_xhci_usb_resume(struct platform_device *pdev)
> +{
> +	struct intel_xhci_usb_data *data = platform_get_drvdata(pdev);
> +	struct device *dev = &pdev->dev;
> +
> +	if (intel_xhci_usb_get_role(dev) != data->role) {
> +		if (intel_xhci_usb_set_role(dev, data->role) != 0)
> +			dev_warn(dev, "Failed to set role during resume\n");
> +	}
> +
> +	return 0;
> +}
> +
>   static const struct platform_device_id intel_xhci_usb_table[] = {
>   	{ .name = DRV_NAME },
>   	{}
> @@ -180,6 +205,8 @@ static struct platform_driver intel_xhci_usb_driver = {
>   	.id_table = intel_xhci_usb_table,
>   	.probe = intel_xhci_usb_probe,
>   	.remove = intel_xhci_usb_remove,
> +	.suspend = intel_xhci_usb_suspend,
> +	.resume = intel_xhci_usb_resume,
>   };
>   
>   module_platform_driver(intel_xhci_usb_driver);
>