Message ID | 20210701022405.10817-1-digetx@gmail.com |
---|---|
Headers | show |
Series | Add OTG mode support to Tegra USB PHY, SMB347 and Nexus 7 | expand |
01.07.2021 05:23, Dmitry Osipenko пишет: > static int tegra_usb_phy_init(struct usb_phy *u_phy) > @@ -967,12 +1057,26 @@ static int tegra_usb_phy_init(struct usb_phy *u_phy) > goto disable_vbus; > } > > + err = tegra_usb_phy_configure_pmc(phy); > + if (err) > + goto close_phy; > + > err = tegra_usb_phy_power_on(phy); > if (err) > goto close_phy; > > + if (phy->irq > 0) { > + err = request_irq(phy->irq, tegra_usb_phy_isr, IRQF_SHARED, > + dev_name(phy->u_phy.dev), phy); > + if (err) > + goto pwr_off_phy; > + } There were reports that this patch was casing an unhandled USB interrupt event on some devices. I thought this problem was fixed already, but looking again at the offending kernel log again, it still should be a problem. The interrupt fires from the usb_add_hcd() of the CI driver before CI driver have requested interrupt in ci_hdrc_probe(). So either CI driver should request interrupt earlier or Tegra PHY driver should keep shared interrupt disabled after requesting it, the latter variant should be more robust. I'll improve it in v2.
On Thu, Jul 01, 2021 at 04:55:03PM +0300, Dmitry Osipenko wrote: > 01.07.2021 05:23, Dmitry Osipenko пишет: > > static int tegra_usb_phy_init(struct usb_phy *u_phy) > > @@ -967,12 +1057,26 @@ static int tegra_usb_phy_init(struct usb_phy *u_phy) > > goto disable_vbus; > > } > > > > + err = tegra_usb_phy_configure_pmc(phy); > > + if (err) > > + goto close_phy; > > + > > err = tegra_usb_phy_power_on(phy); > > if (err) > > goto close_phy; > > > > + if (phy->irq > 0) { > > + err = request_irq(phy->irq, tegra_usb_phy_isr, IRQF_SHARED, > > + dev_name(phy->u_phy.dev), phy); > > + if (err) > > + goto pwr_off_phy; > > + } > > There were reports that this patch was casing an unhandled USB interrupt > event on some devices. I thought this problem was fixed already, but > looking again at the offending kernel log again, it still should be a > problem. > > The interrupt fires from the usb_add_hcd() of the CI driver before CI > driver have requested interrupt in ci_hdrc_probe(). So either CI driver > should request interrupt earlier or Tegra PHY driver should keep shared > interrupt disabled after requesting it, the latter variant should be > more robust. I'll improve it in v2. I'd suggest the first solution, as the latter is a workaround for what is a normal shared interrupt behaviour. Maybe a controller reset is needed in CI driver before going on with PHY init? Best Regards Michał Mirosław
On Thu, Jul 01, 2021 at 05:23:58AM +0300, Dmitry Osipenko wrote: > The HNP work can be re-scheduled while it's still in-fly. This results in > re-initialization of the busy work, resetting the hrtimer's list node of > the work and crashing kernel with null dereference within kernel/timer > once work's timer is expired. It's very easy to trigger this problem by > re-plugging USB cable quickly. Initialize HNP work only once to fix this > trouble. [...] > - INIT_DELAYED_WORK(&fsm->hnp_polling_work, otg_hnp_polling_work); > + if (!fsm->hnp_work_inited) { > + INIT_DELAYED_WORK(&fsm->hnp_polling_work, otg_hnp_polling_work); > + fsm->hnp_work_inited = true; > + } > + Maybe you could just add an initialization function to be called by users of otg_fsm? It seems that only chipidea driver uses this struct currently. Best Regards Michał Mirosław
09.07.2021 01:32, Michał Mirosław пишет: > On Thu, Jul 01, 2021 at 04:55:03PM +0300, Dmitry Osipenko wrote: >> 01.07.2021 05:23, Dmitry Osipenko пишет: >>> static int tegra_usb_phy_init(struct usb_phy *u_phy) >>> @@ -967,12 +1057,26 @@ static int tegra_usb_phy_init(struct usb_phy *u_phy) >>> goto disable_vbus; >>> } >>> >>> + err = tegra_usb_phy_configure_pmc(phy); >>> + if (err) >>> + goto close_phy; >>> + >>> err = tegra_usb_phy_power_on(phy); >>> if (err) >>> goto close_phy; >>> >>> + if (phy->irq > 0) { >>> + err = request_irq(phy->irq, tegra_usb_phy_isr, IRQF_SHARED, >>> + dev_name(phy->u_phy.dev), phy); >>> + if (err) >>> + goto pwr_off_phy; >>> + } >> >> There were reports that this patch was casing an unhandled USB interrupt >> event on some devices. I thought this problem was fixed already, but >> looking again at the offending kernel log again, it still should be a >> problem. >> >> The interrupt fires from the usb_add_hcd() of the CI driver before CI >> driver have requested interrupt in ci_hdrc_probe(). So either CI driver >> should request interrupt earlier or Tegra PHY driver should keep shared >> interrupt disabled after requesting it, the latter variant should be >> more robust. I'll improve it in v2. > > I'd suggest the first solution, as the latter is a workaround for what > is a normal shared interrupt behaviour. Maybe a controller reset is > needed in CI driver before going on with PHY init? I already implemented the second solution. The controller reset should be okay. We could improve it all later on if will ever be needed, so far it's unnecessary. I can't really work on improving the CI interrupt because it requires to have a special testing setup to reproduce the problem and I don't have that setup.
09.07.2021 01:42, Michał Mirosław пишет: > On Thu, Jul 01, 2021 at 05:23:58AM +0300, Dmitry Osipenko wrote: >> The HNP work can be re-scheduled while it's still in-fly. This results in >> re-initialization of the busy work, resetting the hrtimer's list node of >> the work and crashing kernel with null dereference within kernel/timer >> once work's timer is expired. It's very easy to trigger this problem by >> re-plugging USB cable quickly. Initialize HNP work only once to fix this >> trouble. > [...] >> - INIT_DELAYED_WORK(&fsm->hnp_polling_work, otg_hnp_polling_work); >> + if (!fsm->hnp_work_inited) { >> + INIT_DELAYED_WORK(&fsm->hnp_polling_work, otg_hnp_polling_work); >> + fsm->hnp_work_inited = true; >> + } >> + > > Maybe you could just add an initialization function to be called by > users of otg_fsm? It seems that only chipidea driver uses this > struct currently. If there are any out-of-tree users of the OTG FSM, then they will all get the fix too using the universal solution.