* 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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).