| Message ID | 20190708022514.7161-1-hu1.chen@intel.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | usb: roles: Add PM callbacks | expand |
Hello! On 08.07.2019 5: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> > > 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 [...] > @@ -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); Why not just pass &pdev->dev here? > + > + return 0; > +} > + [...] MBR, Sergei
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); >
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);