Re: drivers: usb: musb: question about missing break in switch

[Gustavo A. R. Silva]

Hello Bin,

Quoting Bin Liu <b-liu@xxxxxx>:

> On Thu, Feb 09, 2017 at 02:37:34AM -0600, Gustavo A. R. Silva wrote:
> > Hello everybody,
> >
> > I ran into the following piece of code at
> > drivers/usb/musb/musb_core.c:1854 (linux-next)
> >
> > 1854/*
> > 1855 * Check the musb devctl session bit to determine if we want to
> > 1856 * allow PM runtime for the device. In general, we want to keep things
> > 1857 * active when the session bit is set except after host disconnect.
> > 1858 *
> > 1859 * Only called from musb_irq_work. If this ever needs to get called
> > 1860 * elsewhere, proper locking must be implemented for musb->session.
> > 1861 */
> > 1862static void musb_pm_runtime_check_session(struct musb *musb)
> > 1863{
> > 1864        u8 devctl, s;
> > 1865        int error;
> > 1866
> > 1867        devctl = musb_readb(musb->mregs, MUSB_DEVCTL);
> > 1868
> > 1869        /* Handle session status quirks first */
> > 1870        s = MUSB_DEVCTL_FSDEV | MUSB_DEVCTL_LSDEV |
> > 1871                MUSB_DEVCTL_HR;
> > 1872        switch (devctl & ~s) {
> > 1873        case MUSB_QUIRK_B_INVALID_VBUS_91:
> > 1874                if (musb->quirk_retries--) {
> > 1875                        musb_dbg(musb,
> > 1876                                 "Poll devctl on invalid vbus,
> > assume no session");
> > 1877                        schedule_delayed_work(&musb->irq_work,
> > 1878                                              msecs_to_jiffies(1000));
> > 1879
> > 1880                        return;
> > 1881                }
> > 1882        case MUSB_QUIRK_A_DISCONNECT_19:
> > 1883                if (musb->quirk_retries--) {
> > 1884                        musb_dbg(musb,
> > 1885                                 "Poll devctl on possible host
> > mode disconnect");
> > 1886                        schedule_delayed_work(&musb->irq_work,
> > 1887                                              msecs_to_jiffies(1000));
> > 1888
> > 1889                        return;
> > 1890                }
> > 1891                if (!musb->session)
> > 1892                        break;
> > 1893                musb_dbg(musb, "Allow PM on possible host mode
> > disconnect");
> > 1894                pm_runtime_mark_last_busy(musb->controller);
> > 1895                pm_runtime_put_autosuspend(musb->controller);
> > 1896                musb->session = false;
> > 1897                return;
> > 1898        default:
> > 1899                break;
> > 1900        }
> > 1901
> > 1902        /* No need to do anything if session has not changed */
> > 1903        s = devctl & MUSB_DEVCTL_SESSION;
> > 1904        if (s == musb->session)
> > 1905                return;
> > 1906
> > 1907        /* Block PM or allow PM? */
> > 1908        if (s) {
> > 1909                musb_dbg(musb, "Block PM on active session:
> > %02x", devctl);
> > 1910                error = pm_runtime_get_sync(musb->controller);
> > 1911                if (error < 0)
> > 1912                        dev_err(musb->controller, "Could not
> > enable: %i\n",
> > 1913                                error);
> > 1914                musb->quirk_retries = 3;
> > 1915        } else {
> > 1916                musb_dbg(musb, "Allow PM with no session:  
> > %02x", devctl);
> > 1917                pm_runtime_mark_last_busy(musb->controller);
> > 1918                pm_runtime_put_autosuspend(musb->controller);
> > 1919        }
> > 1920
> > 1921        musb->session = s;
> > 1922}
> >
> > The thing is that the case for MUSB_QUIRK_B_INVALID_VBUS_91 is not
> > terminated by a break statement, and it falls through to the next
> > case MUSB_QUIRK_A_DISCONNECT_19, in case "if
> > (musb->quirk_retries--)" turns to be false.
> >
> > My question here is if this code is intentional?
>
> Yes, it is. For both MUSB_QUIRK_B_INVALID_VBUS_91 and
> MUSB_QUIRK_A_DISCONNECT_19 cases, we first do retries, then if
> musb->session is set, we allow PM.

Thanks for clarifying :)
--
Gustavo A. R. Silva