* drivers: usb: musb: question about missing break in switch
@ 2017-02-09 8:37 Gustavo A. R. Silva
2017-02-09 13:36 ` Bin Liu
0 siblings, 1 reply; 3+ messages in thread
From: Gustavo A. R. Silva @ 2017-02-09 8:37 UTC (permalink / raw)
To: b-liu, Greg KH; +Cc: linux-usb, linux-kernel, Peter Senna Tschudin
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?
In case it is not, I will write a patch to fix this, but first it
would be great to hear any comment about it.
Thank you
--
Gustavo A. R. Silva
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: drivers: usb: musb: question about missing break in switch
2017-02-09 8:37 drivers: usb: musb: question about missing break in switch Gustavo A. R. Silva
@ 2017-02-09 13:36 ` Bin Liu
2017-02-10 1:25 ` Gustavo A. R. Silva
0 siblings, 1 reply; 3+ messages in thread
From: Bin Liu @ 2017-02-09 13:36 UTC (permalink / raw)
To: Gustavo A. R. Silva
Cc: Greg KH, linux-usb, linux-kernel, Peter Senna Tschudin
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.
Regards,
-Bin.
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: drivers: usb: musb: question about missing break in switch
2017-02-09 13:36 ` Bin Liu
@ 2017-02-10 1:25 ` Gustavo A. R. Silva
0 siblings, 0 replies; 3+ messages in thread
From: Gustavo A. R. Silva @ 2017-02-10 1:25 UTC (permalink / raw)
To: Bin Liu; +Cc: Greg KH, linux-usb, linux-kernel, Peter Senna Tschudin
Hello Bin,
Quoting Bin Liu <b-liu@ti.com>:
> 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
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2017-02-10 2:18 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-02-09 8:37 drivers: usb: musb: question about missing break in switch Gustavo A. R. Silva
2017-02-09 13:36 ` Bin Liu
2017-02-10 1:25 ` Gustavo A. R. Silva
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.