Re: [PATCH] usb: roles: Add PM callbacks

[Hans de Goede <hdegoede@redhat.com>]

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);
>