musb RPM sleep-while-atomic in 4.9-rc1

musb RPM sleep-while-atomic in 4.9-rc1

[Johan Hovold]

Hi Tony,

I'm getting the splat below when booting 4.9-rc1 on a BBB and
tracked it down to 65b3f50ed6fa ("usb: musb: Add PM runtime support for
MUSB DSPS glue layer") which added a synchronous RPM get in a timer
callback:

[    6.466226] BUG: sleeping function called from invalid context at /home/johan/work/omicron/src/linux/drivers/base/power/runtime.c:955
[    6.478850] in_atomic(): 1, irqs_disabled(): 0, pid: 59, name: grep
[    6.485434] 1 lock held by grep/59:
[    6.489102]  #0: [    6.490966]  (
((&glue->timer))[    6.494206] ){+.-...}
, at: [    6.497194] [<c0191084>] call_timer_fn+0x0/0x41c
[    6.502048] Preemption disabled at:[    6.505540] [<c01015e0>] __do_softirq+0x80/0x594
[    6.510393] 
[    6.511974] CPU: 0 PID: 59 Comm: grep Not tainted 4.9.0-rc1 #46
[    6.518190] Hardware name: Generic AM33XX (Flattened Device Tree)
[    6.524613] [<c01116e4>] (unwind_backtrace) from [<c010dec0>] (show_stack+0x20/0x24)
[    6.532753] [<c010dec0>] (show_stack) from [<c0364f1c>] (dump_stack+0x24/0x28)
[    6.540351] [<c0364f1c>] (dump_stack) from [<c015ea58>] (___might_sleep+0x1d8/0x2c4)
[    6.548486] [<c015ea58>] (___might_sleep) from [<c015ebb4>] (__might_sleep+0x70/0xa8)
[    6.556720] [<c015ebb4>] (__might_sleep) from [<c0413d60>] (__pm_runtime_resume+0x9c/0xa0)
[    6.565404] [<c0413d60>] (__pm_runtime_resume) from [<c04d0bc4>] (otg_timer+0x3c/0x254)
[    6.573813] [<c04d0bc4>] (otg_timer) from [<c0191180>] (call_timer_fn+0xfc/0x41c)
[    6.581675] [<c0191180>] (call_timer_fn) from [<c01915c0>] (expire_timers+0x120/0x210)
[    6.589990] [<c01915c0>] (expire_timers) from [<c0191acc>] (run_timer_softirq+0xa4/0xdc)
[    6.598487] [<c0191acc>] (run_timer_softirq) from [<c010168c>] (__do_softirq+0x12c/0x594)
[    6.607079] [<c010168c>] (__do_softirq) from [<c01384cc>] (irq_exit+0xf4/0x130)
[    6.614766] [<c01384cc>] (irq_exit) from [<c017d910>] (__handle_domain_irq+0x84/0xec)
[    6.622991] [<c017d910>] (__handle_domain_irq) from [<c0101500>] (omap_intc_handle_irq+0x44/0xa0)
[    6.632306] [<c0101500>] (omap_intc_handle_irq) from [<c010ee78>] (__irq_usr+0x58/0x80)
[    6.640716] Exception stack(0xcf669fb0 to 0xcf669ff8)
[    6.646028] 9fa0:                                     b6fd5050 0012ebb0 b6e71058 b6e1c088
[    6.654614] 9fc0: b6e07000 00000001 b6e1cb10 b6fd5000 00000001 00000000 b6fd38e8 bec32a54
[    6.663198] 9fe0: b6e1c870 bec32990 b6fb0a20 b6fb9758 20030010 ffffffff

Setting the irq_safe flag seems to do the trick, but not sure that's
what you intended to do.

I saw you posted some regression fixes lately, but they did not look
related to this at first glance at least.

Thanks,
Johan
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: musb RPM sleep-while-atomic in 4.9-rc1

[Tony Lindgren]

* Johan Hovold <johan-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> [161020 08:38]:
> Hi Tony,
> 
> I'm getting the splat below when booting 4.9-rc1 on a BBB and
> tracked it down to 65b3f50ed6fa ("usb: musb: Add PM runtime support for
> MUSB DSPS glue layer") which added a synchronous RPM get in a timer
> callback:

OK, sorry to hear about that. Care to email me your .config and how
to reproduce and what do you have connected like a hub? Also do
you use built-in gadgets or configure them via configfs?

> [    6.466226] BUG: sleeping function called from invalid context at /home/johan/work/omicron/src/linux/drivers/base/power/runtime.c:955
> [    6.478850] in_atomic(): 1, irqs_disabled(): 0, pid: 59, name: grep
> [    6.485434] 1 lock held by grep/59:
> [    6.489102]  #0: [    6.490966]  (
> ((&glue->timer))[    6.494206] ){+.-...}
> , at: [    6.497194] [<c0191084>] call_timer_fn+0x0/0x41c
> [    6.502048] Preemption disabled at:[    6.505540] [<c01015e0>] __do_softirq+0x80/0x594
> [    6.510393] 
> [    6.511974] CPU: 0 PID: 59 Comm: grep Not tainted 4.9.0-rc1 #46
> [    6.518190] Hardware name: Generic AM33XX (Flattened Device Tree)
> [    6.524613] [<c01116e4>] (unwind_backtrace) from [<c010dec0>] (show_stack+0x20/0x24)
> [    6.532753] [<c010dec0>] (show_stack) from [<c0364f1c>] (dump_stack+0x24/0x28)
> [    6.540351] [<c0364f1c>] (dump_stack) from [<c015ea58>] (___might_sleep+0x1d8/0x2c4)
> [    6.548486] [<c015ea58>] (___might_sleep) from [<c015ebb4>] (__might_sleep+0x70/0xa8)
> [    6.556720] [<c015ebb4>] (__might_sleep) from [<c0413d60>] (__pm_runtime_resume+0x9c/0xa0)
> [    6.565404] [<c0413d60>] (__pm_runtime_resume) from [<c04d0bc4>] (otg_timer+0x3c/0x254)
> [    6.573813] [<c04d0bc4>] (otg_timer) from [<c0191180>] (call_timer_fn+0xfc/0x41c)
> [    6.581675] [<c0191180>] (call_timer_fn) from [<c01915c0>] (expire_timers+0x120/0x210)
> [    6.589990] [<c01915c0>] (expire_timers) from [<c0191acc>] (run_timer_softirq+0xa4/0xdc)
> [    6.598487] [<c0191acc>] (run_timer_softirq) from [<c010168c>] (__do_softirq+0x12c/0x594)
> [    6.607079] [<c010168c>] (__do_softirq) from [<c01384cc>] (irq_exit+0xf4/0x130)
> [    6.614766] [<c01384cc>] (irq_exit) from [<c017d910>] (__handle_domain_irq+0x84/0xec)
> [    6.622991] [<c017d910>] (__handle_domain_irq) from [<c0101500>] (omap_intc_handle_irq+0x44/0xa0)
> [    6.632306] [<c0101500>] (omap_intc_handle_irq) from [<c010ee78>] (__irq_usr+0x58/0x80)
> [    6.640716] Exception stack(0xcf669fb0 to 0xcf669ff8)
> [    6.646028] 9fa0:                                     b6fd5050 0012ebb0 b6e71058 b6e1c088
> [    6.654614] 9fc0: b6e07000 00000001 b6e1cb10 b6fd5000 00000001 00000000 b6fd38e8 bec32a54
> [    6.663198] 9fe0: b6e1c870 bec32990 b6fb0a20 b6fb9758 20030010 ffffffff
> 
> Setting the irq_safe flag seems to do the trick, but not sure that's
> what you intended to do.

That's what we want to avoid as it keep the parent device permanently
enabled. To avoid that we want to just queue things and deal with them
from pm_runtime_resume.

> I saw you posted some regression fixes lately, but they did not look
> related to this at first glance at least.

Yeah this seems different. Can you still try v4.9-rc + patches from
thread "[PATCH 0/2] Fixes for two more musb regressions"?

Regards,

Tony
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: musb RPM sleep-while-atomic in 4.9-rc1

[Johan Hovold]

[-- Attachment #1: Type: text/plain, Size: 4011 bytes --]

On Fri, Oct 21, 2016 at 12:08:49AM -0700, Tony Lindgren wrote:
> * Johan Hovold <johan-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> [161020 08:38]:
> > Hi Tony,
> > 
> > I'm getting the splat below when booting 4.9-rc1 on a BBB and
> > tracked it down to 65b3f50ed6fa ("usb: musb: Add PM runtime support for
> > MUSB DSPS glue layer") which added a synchronous RPM get in a timer
> > callback:
> 
> OK, sorry to hear about that. Care to email me your .config and how
> to reproduce and what do you have connected like a hub? Also do
> you use built-in gadgets or configure them via configfs?

It happens even with no devices connected to the host port, and with no
gadgets configured (or the peripheral port simply disabled). Whenever
the glue timer fires and calls pm_runtime_get_sync() I get the splat.
For some reason the might_sleep() in get_sync() does not trigger the
first time, which means that I see this four seconds after probe, and
then every other second when the timer fires.

Attaching my defconfig.

> > [    6.466226] BUG: sleeping function called from invalid context at /home/johan/work/omicron/src/linux/drivers/base/power/runtime.c:955
> > [    6.478850] in_atomic(): 1, irqs_disabled(): 0, pid: 59, name: grep
> > [    6.485434] 1 lock held by grep/59:
> > [    6.489102]  #0: [    6.490966]  (
> > ((&glue->timer))[    6.494206] ){+.-...}
> > , at: [    6.497194] [<c0191084>] call_timer_fn+0x0/0x41c
> > [    6.502048] Preemption disabled at:[    6.505540] [<c01015e0>] __do_softirq+0x80/0x594
> > [    6.510393] 
> > [    6.511974] CPU: 0 PID: 59 Comm: grep Not tainted 4.9.0-rc1 #46
> > [    6.518190] Hardware name: Generic AM33XX (Flattened Device Tree)
> > [    6.524613] [<c01116e4>] (unwind_backtrace) from [<c010dec0>] (show_stack+0x20/0x24)
> > [    6.532753] [<c010dec0>] (show_stack) from [<c0364f1c>] (dump_stack+0x24/0x28)
> > [    6.540351] [<c0364f1c>] (dump_stack) from [<c015ea58>] (___might_sleep+0x1d8/0x2c4)
> > [    6.548486] [<c015ea58>] (___might_sleep) from [<c015ebb4>] (__might_sleep+0x70/0xa8)
> > [    6.556720] [<c015ebb4>] (__might_sleep) from [<c0413d60>] (__pm_runtime_resume+0x9c/0xa0)
> > [    6.565404] [<c0413d60>] (__pm_runtime_resume) from [<c04d0bc4>] (otg_timer+0x3c/0x254)
> > [    6.573813] [<c04d0bc4>] (otg_timer) from [<c0191180>] (call_timer_fn+0xfc/0x41c)
> > [    6.581675] [<c0191180>] (call_timer_fn) from [<c01915c0>] (expire_timers+0x120/0x210)
> > [    6.589990] [<c01915c0>] (expire_timers) from [<c0191acc>] (run_timer_softirq+0xa4/0xdc)
> > [    6.598487] [<c0191acc>] (run_timer_softirq) from [<c010168c>] (__do_softirq+0x12c/0x594)
> > [    6.607079] [<c010168c>] (__do_softirq) from [<c01384cc>] (irq_exit+0xf4/0x130)
> > [    6.614766] [<c01384cc>] (irq_exit) from [<c017d910>] (__handle_domain_irq+0x84/0xec)
> > [    6.622991] [<c017d910>] (__handle_domain_irq) from [<c0101500>] (omap_intc_handle_irq+0x44/0xa0)
> > [    6.632306] [<c0101500>] (omap_intc_handle_irq) from [<c010ee78>] (__irq_usr+0x58/0x80)
> > [    6.640716] Exception stack(0xcf669fb0 to 0xcf669ff8)
> > [    6.646028] 9fa0:                                     b6fd5050 0012ebb0 b6e71058 b6e1c088
> > [    6.654614] 9fc0: b6e07000 00000001 b6e1cb10 b6fd5000 00000001 00000000 b6fd38e8 bec32a54
> > [    6.663198] 9fe0: b6e1c870 bec32990 b6fb0a20 b6fb9758 20030010 ffffffff
> > 
> > Setting the irq_safe flag seems to do the trick, but not sure that's
> > what you intended to do.
> 
> That's what we want to avoid as it keep the parent device permanently
> enabled. To avoid that we want to just queue things and deal with them
> from pm_runtime_resume.

I figured, so then that pm_runtime_get_sync() in the dsps timer callback
needs to go.

> > I saw you posted some regression fixes lately, but they did not look
> > related to this at first glance at least.
> 
> Yeah this seems different. Can you still try v4.9-rc + patches from
> thread "[PATCH 0/2] Fixes for two more musb regressions"?

As expected, those two fixes makes no difference.

Thanks,
Johan

[-- Attachment #2: defconfig --]
[-- Type: text/plain, Size: 8159 bytes --]

CONFIG_COMPILE_TEST=y
CONFIG_DEFAULT_HOSTNAME="omicron"
# CONFIG_SWAP is not set
CONFIG_SYSVIPC=y
CONFIG_POSIX_MQUEUE=y
# CONFIG_CROSS_MEMORY_ATTACH is not set
# CONFIG_FHANDLE is not set
CONFIG_USELIB=y
CONFIG_NO_HZ_IDLE=y
CONFIG_HIGH_RES_TIMERS=y
CONFIG_BSD_PROCESS_ACCT=y
CONFIG_IKCONFIG=y
CONFIG_IKCONFIG_PROC=y
CONFIG_LOG_BUF_SHIFT=16
CONFIG_EXPERT=y
# CONFIG_ADVISE_SYSCALLS is not set
CONFIG_SLAB=y
CONFIG_PROFILING=y
CONFIG_MODULES=y
CONFIG_MODULE_FORCE_LOAD=y
CONFIG_MODULE_UNLOAD=y
# CONFIG_LBDAF is not set
# CONFIG_BLK_DEV_BSG is not set
CONFIG_PARTITION_ADVANCED=y
# CONFIG_IOSCHED_DEADLINE is not set
CONFIG_OMAP_RESET_CLOCKS=y
CONFIG_OMAP_MUX_DEBUG=y
CONFIG_ARCH_OMAP3=y
CONFIG_SOC_AM33XX=y
# CONFIG_SOC_OMAP3430 is not set
# CONFIG_SOC_TI81XX is not set
# CONFIG_MACH_OMAP3517EVM is not set
# CONFIG_MACH_OMAP3_PANDORA is not set
CONFIG_ARM_THUMBEE=y
# CONFIG_CACHE_L2X0 is not set
CONFIG_ARM_ERRATA_720789=y
CONFIG_ARM_ERRATA_754322=y
CONFIG_ARM_ERRATA_775420=y
CONFIG_PCI=y
CONFIG_HAVE_ARM_ARCH_TIMER=y
CONFIG_PREEMPT=y
CONFIG_HZ_1000=y
# CONFIG_HIGHPTE is not set
# CONFIG_COMPACTION is not set
CONFIG_ZBOOT_ROM_TEXT=0x0
CONFIG_ZBOOT_ROM_BSS=0x0
CONFIG_ARM_APPENDED_DTB=y
CONFIG_CMDLINE="root=/dev/nfs ip=dhcp debug"
CONFIG_CMDLINE_EXTEND=y
# CONFIG_COREDUMP is not set
CONFIG_PM_DEBUG=y
CONFIG_PM_ADVANCED_DEBUG=y
CONFIG_NET=y
CONFIG_PACKET=y
CONFIG_UNIX=y
CONFIG_INET=y
CONFIG_IP_PNP=y
CONFIG_IP_PNP_DHCP=y
# CONFIG_INET_XFRM_MODE_TRANSPORT is not set
# CONFIG_INET_XFRM_MODE_TUNNEL is not set
# CONFIG_INET_XFRM_MODE_BEET is not set
# CONFIG_INET_DIAG is not set
# CONFIG_IPV6 is not set
CONFIG_IRDA=m
CONFIG_IRCOMM=m
# CONFIG_WIRELESS is not set
# CONFIG_UEVENT_HELPER is not set
CONFIG_DEVTMPFS=y
CONFIG_DEVTMPFS_MOUNT=y
# CONFIG_STANDALONE is not set
CONFIG_FW_LOADER=m
CONFIG_MTD=y
CONFIG_MTD_CMDLINE_PARTS=y
CONFIG_MTD_BLOCK=y
CONFIG_MTD_NAND=y
CONFIG_MTD_NAND_ECC_BCH=y
CONFIG_MTD_NAND_OMAP2=y
CONFIG_MTD_UBI=y
CONFIG_OF_ALL_DTBS=y
CONFIG_OF_OVERLAY=y
CONFIG_BLK_DEV_LOOP=y
CONFIG_SCSI=y
# CONFIG_SCSI_PROC_FS is not set
CONFIG_BLK_DEV_SD=y
# CONFIG_SCSI_LOWLEVEL is not set
CONFIG_NETDEVICES=y
CONFIG_NETCONSOLE=m
# CONFIG_NET_VENDOR_3COM is not set
# CONFIG_NET_VENDOR_ADAPTEC is not set
# CONFIG_NET_VENDOR_AGERE is not set
# CONFIG_NET_VENDOR_ALTEON is not set
# CONFIG_NET_VENDOR_AMAZON is not set
# CONFIG_NET_VENDOR_AMD is not set
# CONFIG_NET_VENDOR_ARC is not set
# CONFIG_NET_VENDOR_ATHEROS is not set
# CONFIG_NET_CADENCE is not set
# CONFIG_NET_VENDOR_BROADCOM is not set
# CONFIG_NET_VENDOR_BROCADE is not set
# CONFIG_NET_VENDOR_CAVIUM is not set
# CONFIG_NET_VENDOR_CHELSIO is not set
# CONFIG_NET_VENDOR_CIRRUS is not set
# CONFIG_NET_VENDOR_CISCO is not set
# CONFIG_NET_VENDOR_DEC is not set
# CONFIG_NET_VENDOR_DLINK is not set
# CONFIG_NET_VENDOR_EMULEX is not set
# CONFIG_NET_VENDOR_EZCHIP is not set
# CONFIG_NET_VENDOR_EXAR is not set
# CONFIG_NET_VENDOR_FARADAY is not set
# CONFIG_NET_VENDOR_HISILICON is not set
# CONFIG_NET_VENDOR_HP is not set
# CONFIG_NET_VENDOR_INTEL is not set
# CONFIG_NET_VENDOR_MARVELL is not set
# CONFIG_NET_VENDOR_MELLANOX is not set
# CONFIG_NET_VENDOR_MICREL is not set
# CONFIG_NET_VENDOR_MYRI is not set
# CONFIG_NET_VENDOR_NATSEMI is not set
# CONFIG_NET_VENDOR_NETRONOME is not set
# CONFIG_NET_VENDOR_NVIDIA is not set
# CONFIG_NET_VENDOR_OKI is not set
# CONFIG_NET_PACKET_ENGINE is not set
# CONFIG_NET_VENDOR_QLOGIC is not set
# CONFIG_NET_VENDOR_QUALCOMM is not set
# CONFIG_NET_VENDOR_REALTEK is not set
# CONFIG_NET_VENDOR_RENESAS is not set
# CONFIG_NET_VENDOR_RDC is not set
# CONFIG_NET_VENDOR_ROCKER is not set
# CONFIG_NET_VENDOR_SAMSUNG is not set
# CONFIG_NET_VENDOR_SEEQ is not set
# CONFIG_NET_VENDOR_SILAN is not set
# CONFIG_NET_VENDOR_SIS is not set
CONFIG_SMSC911X=y
# CONFIG_NET_VENDOR_STMICRO is not set
# CONFIG_NET_VENDOR_SUN is not set
# CONFIG_NET_VENDOR_SYNOPSYS is not set
# CONFIG_NET_VENDOR_TEHUTI is not set
CONFIG_TI_CPSW=y
# CONFIG_NET_VENDOR_VIA is not set
# CONFIG_NET_VENDOR_WIZNET is not set
CONFIG_SMSC_PHY=y
CONFIG_USB_USBNET=y
# CONFIG_USB_NET_AX8817X is not set
# CONFIG_USB_NET_AX88179_178A is not set
CONFIG_USB_NET_CDC_EEM=y
# CONFIG_USB_NET_NET1080 is not set
CONFIG_USB_NET_RNDIS_HOST=y
# CONFIG_USB_NET_CDC_SUBSET is not set
# CONFIG_USB_NET_ZAURUS is not set
# CONFIG_WLAN is not set
# CONFIG_INPUT_LEDS is not set
# CONFIG_INPUT_MOUSEDEV is not set
CONFIG_INPUT_EVDEV=y
CONFIG_KEYBOARD_LM8323=m
# CONFIG_INPUT_MOUSE is not set
# CONFIG_VT is not set
CONFIG_LEGACY_PTY_COUNT=4
CONFIG_SERIAL_OMAP=y
CONFIG_SERIAL_OMAP_CONSOLE=y
# CONFIG_HW_RANDOM is not set
CONFIG_I2C_CHARDEV=y
# CONFIG_I2C_BRCMSTB is not set
CONFIG_PINCTRL_SINGLE=y
# CONFIG_PINCTRL_UNIPHIER is not set
CONFIG_DEBUG_GPIO=y
CONFIG_GPIO_SYSFS=y
CONFIG_GPIO_TEGRA=y
CONFIG_GPIO_TWL4030=y
CONFIG_POWER_SUPPLY=y
# CONFIG_HWMON is not set
CONFIG_WATCHDOG=y
CONFIG_OMAP_WATCHDOG=y
CONFIG_TWL4030_WATCHDOG=y
CONFIG_MFD_DA9052_I2C=y
CONFIG_MFD_TPS65217=y
CONFIG_REGULATOR_DA9052=m
CONFIG_REGULATOR_PBIAS=y
CONFIG_REGULATOR_TPS65217=y
CONFIG_REGULATOR_TWL4030=y
# CONFIG_HID is not set
# CONFIG_USB_HID is not set
CONFIG_USB=y
CONFIG_USB_ANNOUNCE_NEW_DEVICES=y
CONFIG_USB_MON=y
CONFIG_USB_EHCI_HCD=m
CONFIG_USB_ACM=m
CONFIG_USB_STORAGE=y
CONFIG_USB_MUSB_HDRC=y
CONFIG_USB_MUSB_DSPS=y
CONFIG_USB_TI_CPPI41_DMA=y
CONFIG_USB_SERIAL=m
CONFIG_USB_SERIAL_GENERIC=y
CONFIG_USB_SERIAL_SIMPLE=m
CONFIG_USB_SERIAL_AIRCABLE=m
CONFIG_USB_SERIAL_ARK3116=m
CONFIG_USB_SERIAL_BELKIN=m
CONFIG_USB_SERIAL_CH341=m
CONFIG_USB_SERIAL_WHITEHEAT=m
CONFIG_USB_SERIAL_DIGI_ACCELEPORT=m
CONFIG_USB_SERIAL_CP210X=m
CONFIG_USB_SERIAL_CYPRESS_M8=m
CONFIG_USB_SERIAL_EMPEG=m
CONFIG_USB_SERIAL_FTDI_SIO=m
CONFIG_USB_SERIAL_VISOR=m
CONFIG_USB_SERIAL_IPAQ=m
CONFIG_USB_SERIAL_IR=m
CONFIG_USB_SERIAL_EDGEPORT=m
CONFIG_USB_SERIAL_EDGEPORT_TI=m
CONFIG_USB_SERIAL_F81232=m
CONFIG_USB_SERIAL_GARMIN=m
CONFIG_USB_SERIAL_IPW=m
CONFIG_USB_SERIAL_IUU=m
CONFIG_USB_SERIAL_KEYSPAN_PDA=m
CONFIG_USB_SERIAL_KEYSPAN=m
CONFIG_USB_SERIAL_KLSI=m
CONFIG_USB_SERIAL_KOBIL_SCT=m
CONFIG_USB_SERIAL_MCT_U232=m
CONFIG_USB_SERIAL_METRO=m
CONFIG_USB_SERIAL_MOS7720=m
CONFIG_USB_SERIAL_MOS7840=m
CONFIG_USB_SERIAL_MXUPORT=m
CONFIG_USB_SERIAL_NAVMAN=m
CONFIG_USB_SERIAL_PL2303=m
CONFIG_USB_SERIAL_OTI6858=m
CONFIG_USB_SERIAL_QCAUX=m
CONFIG_USB_SERIAL_QUALCOMM=m
CONFIG_USB_SERIAL_SPCP8X5=m
CONFIG_USB_SERIAL_SAFE=m
CONFIG_USB_SERIAL_SIERRAWIRELESS=m
CONFIG_USB_SERIAL_SYMBOL=m
CONFIG_USB_SERIAL_TI=m
CONFIG_USB_SERIAL_CYBERJACK=m
CONFIG_USB_SERIAL_XIRCOM=m
CONFIG_USB_SERIAL_OPTION=m
CONFIG_USB_SERIAL_OMNINET=m
CONFIG_USB_SERIAL_OPTICON=m
CONFIG_USB_SERIAL_XSENS_MT=m
CONFIG_USB_SERIAL_WISHBONE=m
CONFIG_USB_SERIAL_SSU100=m
CONFIG_USB_SERIAL_QT2=m
CONFIG_USB_SERIAL_DEBUG=m
CONFIG_NOP_USB_XCEIV=y
CONFIG_AM335X_PHY_USB=y
CONFIG_USB_GADGET=y
CONFIG_USB_DUMMY_HCD=m
CONFIG_USB_G_SERIAL=m
CONFIG_MMC=y
CONFIG_MMC_OMAP_HS=y
CONFIG_NEW_LEDS=y
CONFIG_LEDS_CLASS=y
CONFIG_LEDS_TRIGGERS=y
CONFIG_LEDS_TRIGGER_TIMER=y
CONFIG_LEDS_TRIGGER_ONESHOT=y
CONFIG_LEDS_TRIGGER_HEARTBEAT=y
CONFIG_RTC_CLASS=y
CONFIG_RTC_DRV_TWL4030=y
# CONFIG_RTC_DRV_SPEAR is not set
CONFIG_RTC_DRV_OMAP=y
CONFIG_DMADEVICES=y
CONFIG_DMA_OMAP=y
CONFIG_TI_EDMA=y
# CONFIG_SH_DMAE_BASE is not set
# CONFIG_COMMON_CLK_XGENE is not set
# CONFIG_CLK_BCM_KONA is not set
# CONFIG_ARM_ARCH_TIMER_EVTSTREAM is not set
# CONFIG_IOMMU_SUPPORT is not set
CONFIG_SOC_TI=y
# CONFIG_ARM_SCPI_POWER_DOMAIN is not set
# CONFIG_MANDATORY_FILE_LOCKING is not set
CONFIG_VFAT_FS=y
CONFIG_TMPFS=y
CONFIG_UBIFS_FS=y
CONFIG_NFS_FS=y
CONFIG_ROOT_NFS=y
CONFIG_NLS_CODEPAGE_437=y
CONFIG_NLS_ISO8859_1=y
CONFIG_NLS_UTF8=y
CONFIG_PRINTK_TIME=y
CONFIG_DYNAMIC_DEBUG=y
# CONFIG_SECTION_MISMATCH_WARN_ONLY is not set
CONFIG_MAGIC_SYSRQ=y
CONFIG_DEBUG_SLAB=y
CONFIG_DEBUG_SLAB_LEAK=y
CONFIG_DEBUG_KMEMLEAK=y
# CONFIG_SCHED_DEBUG is not set
CONFIG_SCHED_STACK_END_CHECK=y
CONFIG_DEBUG_RT_MUTEXES=y
CONFIG_PROVE_LOCKING=y
CONFIG_DEBUG_ATOMIC_SLEEP=y
CONFIG_DEBUG_PI_LIST=y
CONFIG_RCU_EQS_DEBUG=y
CONFIG_STACK_TRACER=y
CONFIG_FUNCTION_PROFILER=y
# CONFIG_TRACING_EVENTS_GPIO is not set
CONFIG_DEBUG_LL=y
CONFIG_EARLY_PRINTK=y
# CONFIG_CRYPTO_ECHAINIV is not set
# CONFIG_CRYPTO_HW is not set

Re: musb RPM sleep-while-atomic in 4.9-rc1

[Tony Lindgren]

* Johan Hovold <johan-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> [161021 02:26]:
> On Fri, Oct 21, 2016 at 12:08:49AM -0700, Tony Lindgren wrote:
> > * Johan Hovold <johan-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> [161020 08:38]:
> > > Hi Tony,
> > > 
> > > I'm getting the splat below when booting 4.9-rc1 on a BBB and
> > > tracked it down to 65b3f50ed6fa ("usb: musb: Add PM runtime support for
> > > MUSB DSPS glue layer") which added a synchronous RPM get in a timer
> > > callback:
> > 
> > OK, sorry to hear about that. Care to email me your .config and how
> > to reproduce and what do you have connected like a hub? Also do
> > you use built-in gadgets or configure them via configfs?
> 
> It happens even with no devices connected to the host port, and with no
> gadgets configured (or the peripheral port simply disabled). Whenever
> the glue timer fires and calls pm_runtime_get_sync() I get the splat.
> For some reason the might_sleep() in get_sync() does not trigger the
> first time, which means that I see this four seconds after probe, and
> then every other second when the timer fires.

OK. I'm totally baffled how come I did not hit this earlier with
my tests. I did have some extra patches for using the pmic vbus irq
for cable detection, but that's only for the peripheral instance and
the host instance is still using timer.

> Attaching my defconfig.

Not seeing anything special there, musb built in, using dma. And no
reason why this should not always happen when polling the cable status.

> > > Setting the irq_safe flag seems to do the trick, but not sure that's
> > > what you intended to do.
> > 
> > That's what we want to avoid as it keep the parent device permanently
> > enabled. To avoid that we want to just queue things and deal with them
> > from pm_runtime_resume.
> 
> I figured, so then that pm_runtime_get_sync() in the dsps timer callback
> needs to go.

Agreed, that is clearly wrong to call from softirq context. I need to
figure out what is right fix here but don't have access to my bbb
until next week.

> > > I saw you posted some regression fixes lately, but they did not look
> > > related to this at first glance at least.
> > 
> > Yeah this seems different. Can you still try v4.9-rc + patches from
> > thread "[PATCH 0/2] Fixes for two more musb regressions"?
> 
> As expected, those two fixes makes no difference.

Yeah thanks for checking.

Regards,

Tony
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: musb RPM sleep-while-atomic in 4.9-rc1

[Johan Hovold]

On Fri, Oct 21, 2016 at 02:49:05AM -0700, Tony Lindgren wrote:
> * Johan Hovold <johan-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> [161021 02:26]:
> > On Fri, Oct 21, 2016 at 12:08:49AM -0700, Tony Lindgren wrote:
> > > * Johan Hovold <johan-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> [161020 08:38]:
> > > > Hi Tony,
> > > > 
> > > > I'm getting the splat below when booting 4.9-rc1 on a BBB and
> > > > tracked it down to 65b3f50ed6fa ("usb: musb: Add PM runtime support for
> > > > MUSB DSPS glue layer") which added a synchronous RPM get in a timer
> > > > callback:
> > > 
> > > OK, sorry to hear about that. Care to email me your .config and how
> > > to reproduce and what do you have connected like a hub? Also do
> > > you use built-in gadgets or configure them via configfs?
> > 
> > It happens even with no devices connected to the host port, and with no
> > gadgets configured (or the peripheral port simply disabled). Whenever
> > the glue timer fires and calls pm_runtime_get_sync() I get the splat.
> > For some reason the might_sleep() in get_sync() does not trigger the
> > first time, which means that I see this four seconds after probe, and
> > then every other second when the timer fires.
> 
> OK. I'm totally baffled how come I did not hit this earlier with
> my tests. I did have some extra patches for using the pmic vbus irq
> for cable detection, but that's only for the peripheral instance and
> the host instance is still using timer.

Yeah, I was a bit surprised as well given that other people have also
apparently run this code. Guess CONFIG_DEBUG_ATOMIC_SLEEP must have been
disabled?

> > > > Setting the irq_safe flag seems to do the trick, but not sure that's
> > > > what you intended to do.
> > > 
> > > That's what we want to avoid as it keep the parent device permanently
> > > enabled. To avoid that we want to just queue things and deal with them
> > > from pm_runtime_resume.
> > 
> > I figured, so then that pm_runtime_get_sync() in the dsps timer callback
> > needs to go.
> 
> Agreed, that is clearly wrong to call from softirq context. I need to
> figure out what is right fix here but don't have access to my bbb
> until next week.

Ok, I'll make do with irq_safe meanwhile.

Thanks,
Johan
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: musb RPM sleep-while-atomic in 4.9-rc1

[Tony Lindgren]

* Johan Hovold <johan-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> [161021 04:08]:
> On Fri, Oct 21, 2016 at 02:49:05AM -0700, Tony Lindgren wrote:
> > * Johan Hovold <johan-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> [161021 02:26]:
> > > On Fri, Oct 21, 2016 at 12:08:49AM -0700, Tony Lindgren wrote:
> > > > * Johan Hovold <johan-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> [161020 08:38]:
> > > > > Hi Tony,
> > > > > 
> > > > > I'm getting the splat below when booting 4.9-rc1 on a BBB and
> > > > > tracked it down to 65b3f50ed6fa ("usb: musb: Add PM runtime support for
> > > > > MUSB DSPS glue layer") which added a synchronous RPM get in a timer
> > > > > callback:
> > > > 
> > > > OK, sorry to hear about that. Care to email me your .config and how
> > > > to reproduce and what do you have connected like a hub? Also do
> > > > you use built-in gadgets or configure them via configfs?
> > > 
> > > It happens even with no devices connected to the host port, and with no
> > > gadgets configured (or the peripheral port simply disabled). Whenever
> > > the glue timer fires and calls pm_runtime_get_sync() I get the splat.
> > > For some reason the might_sleep() in get_sync() does not trigger the
> > > first time, which means that I see this four seconds after probe, and
> > > then every other second when the timer fires.
> > 
> > OK. I'm totally baffled how come I did not hit this earlier with
> > my tests. I did have some extra patches for using the pmic vbus irq
> > for cable detection, but that's only for the peripheral instance and
> > the host instance is still using timer.
> 
> Yeah, I was a bit surprised as well given that other people have also
> apparently run this code. Guess CONFIG_DEBUG_ATOMIC_SLEEP must have been
> disabled?

Yeh that's it, I've been just using omap2plus_defconfig where it seems
to be disabled. Probably a good idea to enable it there.

> > > > > Setting the irq_safe flag seems to do the trick, but not sure that's
> > > > > what you intended to do.
> > > > 
> > > > That's what we want to avoid as it keep the parent device permanently
> > > > enabled. To avoid that we want to just queue things and deal with them
> > > > from pm_runtime_resume.
> > > 
> > > I figured, so then that pm_runtime_get_sync() in the dsps timer callback
> > > needs to go.
> > 
> > Agreed, that is clearly wrong to call from softirq context. I need to
> > figure out what is right fix here but don't have access to my bbb
> > until next week.
> 
> Ok, I'll make do with irq_safe meanwhile.

OK

Regards,

Tony
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: musb RPM sleep-while-atomic in 4.9-rc1

[Tony Lindgren]

Hi,

* Tony Lindgren <tony-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org> [161021 04:28]:
> * Johan Hovold <johan-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> [161021 04:08]:
> > > Agreed, that is clearly wrong to call from softirq context. I need to
> > > figure out what is right fix here but don't have access to my bbb
> > > until next week.
> > 
> > Ok, I'll make do with irq_safe meanwhile.
> 
> OK

Below is a fix against v4.9-rc2 that seems to work for me.

Regards,

Tony

8< ---------------------
>From tony Mon Sep 17 00:00:00 2001
From: Tony Lindgren <tony-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org>
Date: Mon, 24 Oct 2016 09:18:02 -0700
Subject: [PATCH] usb: musb: Fix sleeping function called from invalid
 context for hdrc glue

Commit 65b3f50ed6fa ("usb: musb: Add PM runtime support for MUSB DSPS
glue layer") wrongly added a call for pm_runtime_get_sync to otg_timer
that runs in softirq context. That causes a "BUG: sleeping function called
from invalid context" every time when polling the cable status:

[<c015ebb4>] (__might_sleep) from [<c0413d60>] (__pm_runtime_resume+0x9c/0xa0)
[<c0413d60>] (__pm_runtime_resume) from [<c04d0bc4>] (otg_timer+0x3c/0x254)
[<c04d0bc4>] (otg_timer) from [<c0191180>] (call_timer_fn+0xfc/0x41c)
[<c0191180>] (call_timer_fn) from [<c01915c0>] (expire_timers+0x120/0x210)
[<c01915c0>] (expire_timers) from [<c0191acc>] (run_timer_softirq+0xa4/0xdc)
[<c0191acc>] (run_timer_softirq) from [<c010168c>] (__do_softirq+0x12c/0x594)

I did not notice that as I did not have CONFIG_DEBUG_ATOMIC_SLEEP enabled.

Let's fix the issue by adding dsps_check_status() and dsps_runtime_resume()
functions. And let's have otg_timer() check for the PM runtime status, and
if not already enabled, have dsps_runtime_resume() call dsps_check_status()
directly.

Fixes: 65b3f50ed6fa ("usb: musb: Add PM runtime support for MUSB DSPS
glue layer")
Reported-by: Johan Hovold <johan-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
Signed-off-by: Tony Lindgren <tony-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org>
---
 drivers/usb/musb/musb_dsps.c | 46 ++++++++++++++++++++++++++++++++++++--------
 1 file changed, 38 insertions(+), 8 deletions(-)

diff --git a/drivers/usb/musb/musb_dsps.c b/drivers/usb/musb/musb_dsps.c
--- a/drivers/usb/musb/musb_dsps.c
+++ b/drivers/usb/musb/musb_dsps.c
@@ -188,9 +188,8 @@ static void dsps_musb_disable(struct musb *musb)
 	musb_writeb(musb->mregs, MUSB_DEVCTL, 0);
 }
 
-static void otg_timer(unsigned long _musb)
+static void dsps_check_status(struct musb *musb)
 {
-	struct musb *musb = (void *)_musb;
 	void __iomem *mregs = musb->mregs;
 	struct device *dev = musb->controller;
 	struct dsps_glue *glue = dev_get_drvdata(dev->parent);
@@ -198,11 +197,6 @@ static void otg_timer(unsigned long _musb)
 	u8 devctl;
 	unsigned long flags;
 	int skip_session = 0;
-	int err;
-
-	err = pm_runtime_get_sync(dev);
-	if (err < 0)
-		dev_err(dev, "Poll could not pm_runtime_get: %i\n", err);
 
 	/*
 	 * We poll because DSPS IP's won't expose several OTG-critical
@@ -246,7 +240,25 @@ static void otg_timer(unsigned long _musb)
 		break;
 	}
 	spin_unlock_irqrestore(&musb->lock, flags);
+}
+
+static void otg_timer(unsigned long _musb)
+{
+	struct musb *musb = (void *)_musb;
+	struct device *dev = musb->controller;
+	int err;
+
+	err = pm_runtime_get(dev);
+	if (err < 0)
+		dev_err(dev, "Poll could not pm_runtime_get: %i\n", err);
+
+	/* If not active, dsps_runtime_resume() will call us again */
+	if (!pm_runtime_active(dev))
+		goto done;
+
+	dsps_check_status(musb);
 
+done:
 	pm_runtime_mark_last_busy(dev);
 	pm_runtime_put_autosuspend(dev);
 }
@@ -847,6 +859,21 @@ static const struct of_device_id musb_dsps_of_match[] = {
 };
 MODULE_DEVICE_TABLE(of, musb_dsps_of_match);
 
+static int dsps_runtime_resume(struct device *dev)
+{
+	struct dsps_glue *glue = dev_get_drvdata(dev);
+	struct musb *musb;
+
+	if (!glue->musb)
+		return 0;
+
+	musb = platform_get_drvdata(glue->musb);
+	if (musb)
+		dsps_check_status(musb);
+
+	return 0;
+}
+
 #ifdef CONFIG_PM_SLEEP
 static int dsps_suspend(struct device *dev)
 {
@@ -900,7 +927,10 @@ static int dsps_resume(struct device *dev)
 }
 #endif
 
-static SIMPLE_DEV_PM_OPS(dsps_pm_ops, dsps_suspend, dsps_resume);
+static const struct dev_pm_ops dsps_pm_ops = {
+	SET_RUNTIME_PM_OPS(NULL, dsps_runtime_resume, NULL)
+	SET_SYSTEM_SLEEP_PM_OPS(dsps_suspend, dsps_resume)
+};
 
 static struct platform_driver dsps_usbss_driver = {
 	.probe		= dsps_probe,
-- 
2.9.3
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: musb RPM sleep-while-atomic in 4.9-rc1

[Johan Hovold]

On Mon, Oct 24, 2016 at 10:35:38AM -0700, Tony Lindgren wrote:

> From: Tony Lindgren <tony-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org>
> Date: Mon, 24 Oct 2016 09:18:02 -0700
> Subject: [PATCH] usb: musb: Fix sleeping function called from invalid
>  context for hdrc glue
> 
> Commit 65b3f50ed6fa ("usb: musb: Add PM runtime support for MUSB DSPS
> glue layer") wrongly added a call for pm_runtime_get_sync to otg_timer
> that runs in softirq context. That causes a "BUG: sleeping function called
> from invalid context" every time when polling the cable status:
> 
> [<c015ebb4>] (__might_sleep) from [<c0413d60>] (__pm_runtime_resume+0x9c/0xa0)
> [<c0413d60>] (__pm_runtime_resume) from [<c04d0bc4>] (otg_timer+0x3c/0x254)
> [<c04d0bc4>] (otg_timer) from [<c0191180>] (call_timer_fn+0xfc/0x41c)
> [<c0191180>] (call_timer_fn) from [<c01915c0>] (expire_timers+0x120/0x210)
> [<c01915c0>] (expire_timers) from [<c0191acc>] (run_timer_softirq+0xa4/0xdc)
> [<c0191acc>] (run_timer_softirq) from [<c010168c>] (__do_softirq+0x12c/0x594)
> 
> I did not notice that as I did not have CONFIG_DEBUG_ATOMIC_SLEEP enabled.
> 
> Let's fix the issue by adding dsps_check_status() and dsps_runtime_resume()
> functions. And let's have otg_timer() check for the PM runtime status, and
> if not already enabled, have dsps_runtime_resume() call dsps_check_status()
> directly.

While this makes the sleeping-while-atomic warning go away, it does not
seem correct. In case we were suspended, the glue layer will now call
dsps_check_status while the controller (child) is still suspended:

[    8.577538] musb-hdrc musb-hdrc.0.auto: otg_timer
[    8.582642] musb-dsps 47401400.usb: dsps_runtime_resume
[    8.600651] musb-hdrc musb-hdrc.0.auto: dsps_check_status
[    8.630617] musb-hdrc musb-hdrc.0.auto: musb_runtime_resume

Thanks,
Johan
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: musb RPM sleep-while-atomic in 4.9-rc1

[Tony Lindgren]

* Johan Hovold <johan-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> [161025 01:33]:
> On Mon, Oct 24, 2016 at 10:35:38AM -0700, Tony Lindgren wrote:
> 
> > From: Tony Lindgren <tony-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org>
> > Date: Mon, 24 Oct 2016 09:18:02 -0700
> > Subject: [PATCH] usb: musb: Fix sleeping function called from invalid
> >  context for hdrc glue
> > 
> > Commit 65b3f50ed6fa ("usb: musb: Add PM runtime support for MUSB DSPS
> > glue layer") wrongly added a call for pm_runtime_get_sync to otg_timer
> > that runs in softirq context. That causes a "BUG: sleeping function called
> > from invalid context" every time when polling the cable status:
> > 
> > [<c015ebb4>] (__might_sleep) from [<c0413d60>] (__pm_runtime_resume+0x9c/0xa0)
> > [<c0413d60>] (__pm_runtime_resume) from [<c04d0bc4>] (otg_timer+0x3c/0x254)
> > [<c04d0bc4>] (otg_timer) from [<c0191180>] (call_timer_fn+0xfc/0x41c)
> > [<c0191180>] (call_timer_fn) from [<c01915c0>] (expire_timers+0x120/0x210)
> > [<c01915c0>] (expire_timers) from [<c0191acc>] (run_timer_softirq+0xa4/0xdc)
> > [<c0191acc>] (run_timer_softirq) from [<c010168c>] (__do_softirq+0x12c/0x594)
> > 
> > I did not notice that as I did not have CONFIG_DEBUG_ATOMIC_SLEEP enabled.
> > 
> > Let's fix the issue by adding dsps_check_status() and dsps_runtime_resume()
> > functions. And let's have otg_timer() check for the PM runtime status, and
> > if not already enabled, have dsps_runtime_resume() call dsps_check_status()
> > directly.
> 
> While this makes the sleeping-while-atomic warning go away, it does not
> seem correct. In case we were suspended, the glue layer will now call
> dsps_check_status while the controller (child) is still suspended:
> 
> [    8.577538] musb-hdrc musb-hdrc.0.auto: otg_timer
> [    8.582642] musb-dsps 47401400.usb: dsps_runtime_resume
> [    8.600651] musb-hdrc musb-hdrc.0.auto: dsps_check_status
> [    8.630617] musb-hdrc musb-hdrc.0.auto: musb_runtime_resume

Hmm that's a good point yeah. If we start doing something generic in
musb-core.c musb_runtime_resume, things could go wrong in the future.

With this particular hardware musb registers are always accessible
though when the parent of the two musb instances musb_am335x is done.

We could just reprogam the otg timer if !pm_runtime_active(dev). But
the sucky things with that approach is that when idle we have two
timers, one to wake-up, then another one to check the status.

Regards,

Tony
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: musb RPM sleep-while-atomic in 4.9-rc1

[Johan Hovold]

On Tue, Oct 25, 2016 at 08:11:10AM -0700, Tony Lindgren wrote:
> * Johan Hovold <johan-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> [161025 01:33]:
> > On Mon, Oct 24, 2016 at 10:35:38AM -0700, Tony Lindgren wrote:
> > 
> > > From: Tony Lindgren <tony-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org>
> > > Date: Mon, 24 Oct 2016 09:18:02 -0700
> > > Subject: [PATCH] usb: musb: Fix sleeping function called from invalid
> > >  context for hdrc glue
> > > 
> > > Commit 65b3f50ed6fa ("usb: musb: Add PM runtime support for MUSB DSPS
> > > glue layer") wrongly added a call for pm_runtime_get_sync to otg_timer
> > > that runs in softirq context. That causes a "BUG: sleeping function called
> > > from invalid context" every time when polling the cable status:
> > > 
> > > [<c015ebb4>] (__might_sleep) from [<c0413d60>] (__pm_runtime_resume+0x9c/0xa0)
> > > [<c0413d60>] (__pm_runtime_resume) from [<c04d0bc4>] (otg_timer+0x3c/0x254)
> > > [<c04d0bc4>] (otg_timer) from [<c0191180>] (call_timer_fn+0xfc/0x41c)
> > > [<c0191180>] (call_timer_fn) from [<c01915c0>] (expire_timers+0x120/0x210)
> > > [<c01915c0>] (expire_timers) from [<c0191acc>] (run_timer_softirq+0xa4/0xdc)
> > > [<c0191acc>] (run_timer_softirq) from [<c010168c>] (__do_softirq+0x12c/0x594)
> > > 
> > > I did not notice that as I did not have CONFIG_DEBUG_ATOMIC_SLEEP enabled.
> > > 
> > > Let's fix the issue by adding dsps_check_status() and dsps_runtime_resume()
> > > functions. And let's have otg_timer() check for the PM runtime status, and
> > > if not already enabled, have dsps_runtime_resume() call dsps_check_status()
> > > directly.
> > 
> > While this makes the sleeping-while-atomic warning go away, it does not
> > seem correct. In case we were suspended, the glue layer will now call
> > dsps_check_status while the controller (child) is still suspended:
> > 
> > [    8.577538] musb-hdrc musb-hdrc.0.auto: otg_timer
> > [    8.582642] musb-dsps 47401400.usb: dsps_runtime_resume
> > [    8.600651] musb-hdrc musb-hdrc.0.auto: dsps_check_status
> > [    8.630617] musb-hdrc musb-hdrc.0.auto: musb_runtime_resume
> 
> Hmm that's a good point yeah. If we start doing something generic in
> musb-core.c musb_runtime_resume, things could go wrong in the future.
> 
> With this particular hardware musb registers are always accessible
> though when the parent of the two musb instances musb_am335x is done.
> 
> We could just reprogam the otg timer if !pm_runtime_active(dev). But
> the sucky things with that approach is that when idle we have two
> timers, one to wake-up, then another one to check the status.

Maybe add another callback from musb_runtime_resume that can be used for
such deferred work instead?

Johan
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: musb RPM sleep-while-atomic in 4.9-rc1

[Tony Lindgren]

* Johan Hovold <johan-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> [161026 07:21]:
> On Tue, Oct 25, 2016 at 08:11:10AM -0700, Tony Lindgren wrote:
> > * Johan Hovold <johan-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> [161025 01:33]:
> > > On Mon, Oct 24, 2016 at 10:35:38AM -0700, Tony Lindgren wrote:
> > > 
> > > > From: Tony Lindgren <tony-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org>
> > > > Date: Mon, 24 Oct 2016 09:18:02 -0700
> > > > Subject: [PATCH] usb: musb: Fix sleeping function called from invalid
> > > >  context for hdrc glue
> > > > 
> > > > Commit 65b3f50ed6fa ("usb: musb: Add PM runtime support for MUSB DSPS
> > > > glue layer") wrongly added a call for pm_runtime_get_sync to otg_timer
> > > > that runs in softirq context. That causes a "BUG: sleeping function called
> > > > from invalid context" every time when polling the cable status:
> > > > 
> > > > [<c015ebb4>] (__might_sleep) from [<c0413d60>] (__pm_runtime_resume+0x9c/0xa0)
> > > > [<c0413d60>] (__pm_runtime_resume) from [<c04d0bc4>] (otg_timer+0x3c/0x254)
> > > > [<c04d0bc4>] (otg_timer) from [<c0191180>] (call_timer_fn+0xfc/0x41c)
> > > > [<c0191180>] (call_timer_fn) from [<c01915c0>] (expire_timers+0x120/0x210)
> > > > [<c01915c0>] (expire_timers) from [<c0191acc>] (run_timer_softirq+0xa4/0xdc)
> > > > [<c0191acc>] (run_timer_softirq) from [<c010168c>] (__do_softirq+0x12c/0x594)
> > > > 
> > > > I did not notice that as I did not have CONFIG_DEBUG_ATOMIC_SLEEP enabled.
> > > > 
> > > > Let's fix the issue by adding dsps_check_status() and dsps_runtime_resume()
> > > > functions. And let's have otg_timer() check for the PM runtime status, and
> > > > if not already enabled, have dsps_runtime_resume() call dsps_check_status()
> > > > directly.
> > > 
> > > While this makes the sleeping-while-atomic warning go away, it does not
> > > seem correct. In case we were suspended, the glue layer will now call
> > > dsps_check_status while the controller (child) is still suspended:
> > > 
> > > [    8.577538] musb-hdrc musb-hdrc.0.auto: otg_timer
> > > [    8.582642] musb-dsps 47401400.usb: dsps_runtime_resume
> > > [    8.600651] musb-hdrc musb-hdrc.0.auto: dsps_check_status
> > > [    8.630617] musb-hdrc musb-hdrc.0.auto: musb_runtime_resume
> > 
> > Hmm that's a good point yeah. If we start doing something generic in
> > musb-core.c musb_runtime_resume, things could go wrong in the future.
> > 
> > With this particular hardware musb registers are always accessible
> > though when the parent of the two musb instances musb_am335x is done.
> > 
> > We could just reprogam the otg timer if !pm_runtime_active(dev). But
> > the sucky things with that approach is that when idle we have two
> > timers, one to wake-up, then another one to check the status.
> 
> Maybe add another callback from musb_runtime_resume that can be used for
> such deferred work instead?

OK that's a good idea :)

Tony
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: musb RPM sleep-while-atomic in 4.9-rc1

[Tony Lindgren]

* Tony Lindgren <tony-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org> [161026 07:32]:
> * Johan Hovold <johan-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> [161026 07:21]:
> > On Tue, Oct 25, 2016 at 08:11:10AM -0700, Tony Lindgren wrote:
> > > * Johan Hovold <johan-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> [161025 01:33]:
> > > > On Mon, Oct 24, 2016 at 10:35:38AM -0700, Tony Lindgren wrote:
> > > > 
> > > > > From: Tony Lindgren <tony-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org>
> > > > > Date: Mon, 24 Oct 2016 09:18:02 -0700
> > > > > Subject: [PATCH] usb: musb: Fix sleeping function called from invalid
> > > > >  context for hdrc glue
> > > > > 
> > > > > Commit 65b3f50ed6fa ("usb: musb: Add PM runtime support for MUSB DSPS
> > > > > glue layer") wrongly added a call for pm_runtime_get_sync to otg_timer
> > > > > that runs in softirq context. That causes a "BUG: sleeping function called
> > > > > from invalid context" every time when polling the cable status:
> > > > > 
> > > > > [<c015ebb4>] (__might_sleep) from [<c0413d60>] (__pm_runtime_resume+0x9c/0xa0)
> > > > > [<c0413d60>] (__pm_runtime_resume) from [<c04d0bc4>] (otg_timer+0x3c/0x254)
> > > > > [<c04d0bc4>] (otg_timer) from [<c0191180>] (call_timer_fn+0xfc/0x41c)
> > > > > [<c0191180>] (call_timer_fn) from [<c01915c0>] (expire_timers+0x120/0x210)
> > > > > [<c01915c0>] (expire_timers) from [<c0191acc>] (run_timer_softirq+0xa4/0xdc)
> > > > > [<c0191acc>] (run_timer_softirq) from [<c010168c>] (__do_softirq+0x12c/0x594)
> > > > > 
> > > > > I did not notice that as I did not have CONFIG_DEBUG_ATOMIC_SLEEP enabled.
> > > > > 
> > > > > Let's fix the issue by adding dsps_check_status() and dsps_runtime_resume()
> > > > > functions. And let's have otg_timer() check for the PM runtime status, and
> > > > > if not already enabled, have dsps_runtime_resume() call dsps_check_status()
> > > > > directly.
> > > > 
> > > > While this makes the sleeping-while-atomic warning go away, it does not
> > > > seem correct. In case we were suspended, the glue layer will now call
> > > > dsps_check_status while the controller (child) is still suspended:
> > > > 
> > > > [    8.577538] musb-hdrc musb-hdrc.0.auto: otg_timer
> > > > [    8.582642] musb-dsps 47401400.usb: dsps_runtime_resume
> > > > [    8.600651] musb-hdrc musb-hdrc.0.auto: dsps_check_status
> > > > [    8.630617] musb-hdrc musb-hdrc.0.auto: musb_runtime_resume
> > > 
> > > Hmm that's a good point yeah. If we start doing something generic in
> > > musb-core.c musb_runtime_resume, things could go wrong in the future.
> > > 
> > > With this particular hardware musb registers are always accessible
> > > though when the parent of the two musb instances musb_am335x is done.
> > > 
> > > We could just reprogam the otg timer if !pm_runtime_active(dev). But
> > > the sucky things with that approach is that when idle we have two
> > > timers, one to wake-up, then another one to check the status.
> > 
> > Maybe add another callback from musb_runtime_resume that can be used for
> > such deferred work instead?
> 
> OK that's a good idea :)

Here's this fix updated for the callback. I also noticed that we have
musb_gadget_queue() also doing pm_runtime_get_sync so that's now
fixed too.

Regards,

Tony

8< -------------------------------
>From tony Mon Sep 17 00:00:00 2001
From: Tony Lindgren <tony-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org>
Date: Tue, 25 Oct 2016 08:42:00 -0700
Subject: [PATCH] usb: musb: Fix sleeping function called from invalid
 context for hdrc glue

Commit 65b3f50ed6fa ("usb: musb: Add PM runtime support for MUSB DSPS
glue layer") wrongly added a call for pm_runtime_get_sync to otg_timer
that runs in softirq context. That causes a "BUG: sleeping function called
from invalid context" every time when polling the cable status:

[<c015ebb4>] (__might_sleep) from [<c0413d60>] (__pm_runtime_resume+0x9c/0xa0)
[<c0413d60>] (__pm_runtime_resume) from [<c04d0bc4>] (otg_timer+0x3c/0x254)
[<c04d0bc4>] (otg_timer) from [<c0191180>] (call_timer_fn+0xfc/0x41c)
[<c0191180>] (call_timer_fn) from [<c01915c0>] (expire_timers+0x120/0x210)
[<c01915c0>] (expire_timers) from [<c0191acc>] (run_timer_softirq+0xa4/0xdc)
[<c0191acc>] (run_timer_softirq) from [<c010168c>] (__do_softirq+0x12c/0x594)

I did not notice that as I did not have CONFIG_DEBUG_ATOMIC_SLEEP enabled.

Let's fix the issue by adding dsps_check_status() and then register a
callback with musb_runtime_resume() so it gets called only when musb core
and it's parent devices are awake. Note that we don't want to do this from
PM runtime resume in musb_dsps.c as musb core is not awake yet at that
point as noted by Johan Hovold <johan-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>.

Note that musb_gadget_queue() also suffers from a similar issue when
connecting the cable and cannot use pm_runtime_get_sync().

Fixes: 65b3f50ed6fa ("usb: musb: Add PM runtime support for MUSB DSPS
glue layer")
Reported-by: Johan Hovold <johan-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
Signed-off-by: Tony Lindgren <tony-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org>
---
 drivers/usb/musb/musb_core.c   | 44 +++++++++++++++++++++++++++++++++++++++++-
 drivers/usb/musb/musb_core.h   |  7 +++++++
 drivers/usb/musb/musb_dsps.c   | 29 +++++++++++++++++++++-------
 drivers/usb/musb/musb_gadget.c | 16 +++++++++++++--
 4 files changed, 86 insertions(+), 10 deletions(-)

diff --git a/drivers/usb/musb/musb_core.c b/drivers/usb/musb/musb_core.c
--- a/drivers/usb/musb/musb_core.c
+++ b/drivers/usb/musb/musb_core.c
@@ -122,7 +122,6 @@ MODULE_AUTHOR(DRIVER_AUTHOR);
 MODULE_LICENSE("GPL");
 MODULE_ALIAS("platform:" MUSB_DRIVER_NAME);
 
-
 /*-------------------------------------------------------------------------*/
 
 static inline struct musb *dev_to_musb(struct device *dev)
@@ -1896,6 +1895,46 @@ static void musb_pm_runtime_check_session(struct musb *musb)
 	musb->session = s;
 }
 
+struct musb_resume_work {
+	void (*callback)(struct musb *musb, void *data);
+	void *data;
+	struct list_head node;
+};
+
+void musb_queue_on_resume(struct musb *musb,
+			  void (*callback)(struct musb *musb, void *data),
+			  void *data)
+{
+	struct musb_resume_work *w;
+	unsigned long flags;
+
+	w = devm_kzalloc(musb->controller, sizeof(*w), GFP_ATOMIC);
+	if (!w)
+		return;
+
+	w->callback = callback;
+	w->data = data;
+	spin_lock_irqsave(&musb->list_lock, flags);
+	list_add_tail(&w->node, &musb->resume_work);
+	spin_unlock_irqrestore(&musb->list_lock, flags);
+}
+EXPORT_SYMBOL_GPL(musb_queue_on_resume);
+
+static void musb_run_pending(struct musb *musb)
+{
+	struct musb_resume_work *w, *_w;
+	unsigned long flags;
+
+	spin_lock_irqsave(&musb->list_lock, flags);
+	list_for_each_entry_safe(w, _w, &musb->resume_work, node) {
+		if (w->callback)
+			w->callback(musb, w->data);
+		list_del(&w->node);
+		devm_kfree(musb->controller, w);
+	}
+	spin_unlock_irqrestore(&musb->list_lock, flags);
+}
+
 /* Only used to provide driver mode change events */
 static void musb_irq_work(struct work_struct *data)
 {
@@ -1969,6 +2008,7 @@ static struct musb *allocate_instance(struct device *dev,
 	INIT_LIST_HEAD(&musb->control);
 	INIT_LIST_HEAD(&musb->in_bulk);
 	INIT_LIST_HEAD(&musb->out_bulk);
+	INIT_LIST_HEAD(&musb->resume_work);
 
 	musb->vbuserr_retry = VBUSERR_RETRY_COUNT;
 	musb->a_wait_bcon = OTG_TIME_A_WAIT_BCON;
@@ -2065,6 +2105,7 @@ musb_init_controller(struct device *dev, int nIrq, void __iomem *ctrl)
 	}
 
 	spin_lock_init(&musb->lock);
+	spin_lock_init(&musb->list_lock);
 	musb->board_set_power = plat->set_power;
 	musb->min_power = plat->min_power;
 	musb->ops = plat->platform_ops;
@@ -2374,6 +2415,7 @@ static int musb_remove(struct platform_device *pdev)
 	 *  - Peripheral mode: peripheral is deactivated (or never-activated)
 	 *  - OTG mode: both roles are deactivated (or never-activated)
 	 */
+	musb_run_pending(musb);
 	musb_exit_debugfs(musb);
 
 	cancel_work_sync(&musb->irq_work);
diff --git a/drivers/usb/musb/musb_core.h b/drivers/usb/musb/musb_core.h
--- a/drivers/usb/musb/musb_core.h
+++ b/drivers/usb/musb/musb_core.h
@@ -303,6 +303,7 @@ struct musb_context_registers {
 struct musb {
 	/* device lock */
 	spinlock_t		lock;
+	spinlock_t		list_lock;	/* resume work list lock */
 
 	struct musb_io		io;
 	const struct musb_platform_ops *ops;
@@ -337,6 +338,7 @@ struct musb {
 	struct list_head	control;	/* of musb_qh */
 	struct list_head	in_bulk;	/* of musb_qh */
 	struct list_head	out_bulk;	/* of musb_qh */
+	struct list_head	resume_work;	/* pending work on resume */
 
 	struct timer_list	otg_timer;
 	struct notifier_block	nb;
@@ -540,6 +542,11 @@ extern irqreturn_t musb_interrupt(struct musb *);
 
 extern void musb_hnp_stop(struct musb *musb);
 
+extern void
+musb_queue_on_resume(struct musb *musb,
+		     void (*callback)(struct musb *musb, void *data),
+		     void *data);
+
 static inline void musb_platform_set_vbus(struct musb *musb, int is_on)
 {
 	if (musb->ops->set_vbus)
diff --git a/drivers/usb/musb/musb_dsps.c b/drivers/usb/musb/musb_dsps.c
--- a/drivers/usb/musb/musb_dsps.c
+++ b/drivers/usb/musb/musb_dsps.c
@@ -188,9 +188,8 @@ static void dsps_musb_disable(struct musb *musb)
 	musb_writeb(musb->mregs, MUSB_DEVCTL, 0);
 }
 
-static void otg_timer(unsigned long _musb)
+static void dsps_check_status(struct musb *musb)
 {
-	struct musb *musb = (void *)_musb;
 	void __iomem *mregs = musb->mregs;
 	struct device *dev = musb->controller;
 	struct dsps_glue *glue = dev_get_drvdata(dev->parent);
@@ -198,11 +197,6 @@ static void otg_timer(unsigned long _musb)
 	u8 devctl;
 	unsigned long flags;
 	int skip_session = 0;
-	int err;
-
-	err = pm_runtime_get_sync(dev);
-	if (err < 0)
-		dev_err(dev, "Poll could not pm_runtime_get: %i\n", err);
 
 	/*
 	 * We poll because DSPS IP's won't expose several OTG-critical
@@ -246,6 +240,27 @@ static void otg_timer(unsigned long _musb)
 		break;
 	}
 	spin_unlock_irqrestore(&musb->lock, flags);
+}
+
+static void dsps_check_status_resume_work(struct musb *musb, void *unused)
+{
+	dsps_check_status(musb);
+}
+
+static void otg_timer(unsigned long _musb)
+{
+	struct musb *musb = (void *)_musb;
+	struct device *dev = musb->controller;
+	int err;
+
+	err = pm_runtime_get(dev);
+	if (err < 0)
+		dev_err(dev, "Poll could not pm_runtime_get: %i\n", err);
+
+	if (pm_runtime_active(dev))
+		dsps_check_status(musb);
+	else
+		musb_queue_on_resume(musb, dsps_check_status_resume_work, NULL);
 
 	pm_runtime_mark_last_busy(dev);
 	pm_runtime_put_autosuspend(dev);
diff --git a/drivers/usb/musb/musb_gadget.c b/drivers/usb/musb/musb_gadget.c
--- a/drivers/usb/musb/musb_gadget.c
+++ b/drivers/usb/musb/musb_gadget.c
@@ -1222,6 +1222,13 @@ void musb_ep_restart(struct musb *musb, struct musb_request *req)
 		rxstate(musb, req);
 }
 
+void musb_ep_restart_resume_work(struct musb *musb, void *data)
+{
+	struct musb_request *req = data;
+
+	musb_ep_restart(musb, req);
+}
+
 static int musb_gadget_queue(struct usb_ep *ep, struct usb_request *req,
 			gfp_t gfp_flags)
 {
@@ -1271,8 +1278,13 @@ static int musb_gadget_queue(struct usb_ep *ep, struct usb_request *req,
 	list_add_tail(&request->list, &musb_ep->req_list);
 
 	/* it this is the head of the queue, start i/o ... */
-	if (!musb_ep->busy && &request->list == musb_ep->req_list.next)
-		musb_ep_restart(musb, request);
+	if (!musb_ep->busy && &request->list == musb_ep->req_list.next) {
+		if (pm_runtime_active(musb->controller))
+			musb_ep_restart(musb, request);
+		else
+			musb_queue_on_resume(musb, musb_ep_restart_resume_work,
+					     request);
+	}
 
 unlock:
 	spin_unlock_irqrestore(&musb->lock, lockflags);
-- 
2.9.3
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: musb RPM sleep-while-atomic in 4.9-rc1

[Johan Hovold]

On Thu, Oct 27, 2016 at 08:14:46AM -0700, Tony Lindgren wrote:
> * Tony Lindgren <tony-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org> [161026 07:32]:

> 8< -------------------------------
> From tony Mon Sep 17 00:00:00 2001
> From: Tony Lindgren <tony-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org>
> Date: Tue, 25 Oct 2016 08:42:00 -0700
> Subject: [PATCH] usb: musb: Fix sleeping function called from invalid
>  context for hdrc glue
> 
> Commit 65b3f50ed6fa ("usb: musb: Add PM runtime support for MUSB DSPS
> glue layer") wrongly added a call for pm_runtime_get_sync to otg_timer
> that runs in softirq context. That causes a "BUG: sleeping function called
> from invalid context" every time when polling the cable status:
> 
> [<c015ebb4>] (__might_sleep) from [<c0413d60>] (__pm_runtime_resume+0x9c/0xa0)
> [<c0413d60>] (__pm_runtime_resume) from [<c04d0bc4>] (otg_timer+0x3c/0x254)
> [<c04d0bc4>] (otg_timer) from [<c0191180>] (call_timer_fn+0xfc/0x41c)
> [<c0191180>] (call_timer_fn) from [<c01915c0>] (expire_timers+0x120/0x210)
> [<c01915c0>] (expire_timers) from [<c0191acc>] (run_timer_softirq+0xa4/0xdc)
> [<c0191acc>] (run_timer_softirq) from [<c010168c>] (__do_softirq+0x12c/0x594)
> 
> I did not notice that as I did not have CONFIG_DEBUG_ATOMIC_SLEEP enabled.
> 
> Let's fix the issue by adding dsps_check_status() and then register a
> callback with musb_runtime_resume() so it gets called only when musb core
> and it's parent devices are awake. Note that we don't want to do this from
> PM runtime resume in musb_dsps.c as musb core is not awake yet at that
> point as noted by Johan Hovold <johan-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>.

It seems some chunks are missing since this patch only runs the
deferred work at remove and not at runtime resume, effectively breaking
detection.

> Note that musb_gadget_queue() also suffers from a similar issue when
> connecting the cable and cannot use pm_runtime_get_sync().

You seem to have left that pm_runtime_get_sync() in there though.

> Fixes: 65b3f50ed6fa ("usb: musb: Add PM runtime support for MUSB DSPS
> glue layer")
> Reported-by: Johan Hovold <johan-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> Signed-off-by: Tony Lindgren <tony-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org>
> ---
>  drivers/usb/musb/musb_core.c   | 44 +++++++++++++++++++++++++++++++++++++++++-
>  drivers/usb/musb/musb_core.h   |  7 +++++++
>  drivers/usb/musb/musb_dsps.c   | 29 +++++++++++++++++++++-------
>  drivers/usb/musb/musb_gadget.c | 16 +++++++++++++--
>  4 files changed, 86 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/usb/musb/musb_core.c b/drivers/usb/musb/musb_core.c
> --- a/drivers/usb/musb/musb_core.c
> +++ b/drivers/usb/musb/musb_core.c
> @@ -122,7 +122,6 @@ MODULE_AUTHOR(DRIVER_AUTHOR);
>  MODULE_LICENSE("GPL");
>  MODULE_ALIAS("platform:" MUSB_DRIVER_NAME);
>  
> -
>  /*-------------------------------------------------------------------------*/
>  
>  static inline struct musb *dev_to_musb(struct device *dev)
> @@ -1896,6 +1895,46 @@ static void musb_pm_runtime_check_session(struct musb *musb)
>  	musb->session = s;
>  }
>  
> +struct musb_resume_work {
> +	void (*callback)(struct musb *musb, void *data);
> +	void *data;
> +	struct list_head node;
> +};
> +
> +void musb_queue_on_resume(struct musb *musb,
> +			  void (*callback)(struct musb *musb, void *data),
> +			  void *data)
> +{
> +	struct musb_resume_work *w;
> +	unsigned long flags;
> +
> +	w = devm_kzalloc(musb->controller, sizeof(*w), GFP_ATOMIC);
> +	if (!w)
> +		return;
> +
> +	w->callback = callback;
> +	w->data = data;
> +	spin_lock_irqsave(&musb->list_lock, flags);
> +	list_add_tail(&w->node, &musb->resume_work);
> +	spin_unlock_irqrestore(&musb->list_lock, flags);
> +}
> +EXPORT_SYMBOL_GPL(musb_queue_on_resume);
> +
> +static void musb_run_pending(struct musb *musb)
> +{
> +	struct musb_resume_work *w, *_w;
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&musb->list_lock, flags);
> +	list_for_each_entry_safe(w, _w, &musb->resume_work, node) {
> +		if (w->callback)
> +			w->callback(musb, w->data);
> +		list_del(&w->node);
> +		devm_kfree(musb->controller, w);
> +	}
> +	spin_unlock_irqrestore(&musb->list_lock, flags);
> +}
> +
>  /* Only used to provide driver mode change events */
>  static void musb_irq_work(struct work_struct *data)
>  {
> @@ -1969,6 +2008,7 @@ static struct musb *allocate_instance(struct device *dev,
>  	INIT_LIST_HEAD(&musb->control);
>  	INIT_LIST_HEAD(&musb->in_bulk);
>  	INIT_LIST_HEAD(&musb->out_bulk);
> +	INIT_LIST_HEAD(&musb->resume_work);
>  
>  	musb->vbuserr_retry = VBUSERR_RETRY_COUNT;
>  	musb->a_wait_bcon = OTG_TIME_A_WAIT_BCON;
> @@ -2065,6 +2105,7 @@ musb_init_controller(struct device *dev, int nIrq, void __iomem *ctrl)
>  	}
>  
>  	spin_lock_init(&musb->lock);
> +	spin_lock_init(&musb->list_lock);
>  	musb->board_set_power = plat->set_power;
>  	musb->min_power = plat->min_power;
>  	musb->ops = plat->platform_ops;
> @@ -2374,6 +2415,7 @@ static int musb_remove(struct platform_device *pdev)
>  	 *  - Peripheral mode: peripheral is deactivated (or never-activated)
>  	 *  - OTG mode: both roles are deactivated (or never-activated)
>  	 */
> +	musb_run_pending(musb);
>  	musb_exit_debugfs(musb);
>  
>  	cancel_work_sync(&musb->irq_work);
> diff --git a/drivers/usb/musb/musb_core.h b/drivers/usb/musb/musb_core.h
> --- a/drivers/usb/musb/musb_core.h
> +++ b/drivers/usb/musb/musb_core.h
> @@ -303,6 +303,7 @@ struct musb_context_registers {
>  struct musb {
>  	/* device lock */
>  	spinlock_t		lock;
> +	spinlock_t		list_lock;	/* resume work list lock */
>  
>  	struct musb_io		io;
>  	const struct musb_platform_ops *ops;
> @@ -337,6 +338,7 @@ struct musb {
>  	struct list_head	control;	/* of musb_qh */
>  	struct list_head	in_bulk;	/* of musb_qh */
>  	struct list_head	out_bulk;	/* of musb_qh */
> +	struct list_head	resume_work;	/* pending work on resume */
>  
>  	struct timer_list	otg_timer;
>  	struct notifier_block	nb;
> @@ -540,6 +542,11 @@ extern irqreturn_t musb_interrupt(struct musb *);
>  
>  extern void musb_hnp_stop(struct musb *musb);
>  
> +extern void
> +musb_queue_on_resume(struct musb *musb,
> +		     void (*callback)(struct musb *musb, void *data),
> +		     void *data);
> +
>  static inline void musb_platform_set_vbus(struct musb *musb, int is_on)
>  {
>  	if (musb->ops->set_vbus)
> diff --git a/drivers/usb/musb/musb_dsps.c b/drivers/usb/musb/musb_dsps.c
> --- a/drivers/usb/musb/musb_dsps.c
> +++ b/drivers/usb/musb/musb_dsps.c
> @@ -188,9 +188,8 @@ static void dsps_musb_disable(struct musb *musb)
>  	musb_writeb(musb->mregs, MUSB_DEVCTL, 0);
>  }
>  
> -static void otg_timer(unsigned long _musb)
> +static void dsps_check_status(struct musb *musb)
>  {
> -	struct musb *musb = (void *)_musb;
>  	void __iomem *mregs = musb->mregs;
>  	struct device *dev = musb->controller;
>  	struct dsps_glue *glue = dev_get_drvdata(dev->parent);
> @@ -198,11 +197,6 @@ static void otg_timer(unsigned long _musb)
>  	u8 devctl;
>  	unsigned long flags;
>  	int skip_session = 0;
> -	int err;
> -
> -	err = pm_runtime_get_sync(dev);
> -	if (err < 0)
> -		dev_err(dev, "Poll could not pm_runtime_get: %i\n", err);
>  
>  	/*
>  	 * We poll because DSPS IP's won't expose several OTG-critical
> @@ -246,6 +240,27 @@ static void otg_timer(unsigned long _musb)
>  		break;
>  	}
>  	spin_unlock_irqrestore(&musb->lock, flags);
> +}
> +
> +static void dsps_check_status_resume_work(struct musb *musb, void *unused)
> +{
> +	dsps_check_status(musb);
> +}
> +
> +static void otg_timer(unsigned long _musb)
> +{
> +	struct musb *musb = (void *)_musb;
> +	struct device *dev = musb->controller;
> +	int err;
> +
> +	err = pm_runtime_get(dev);
> +	if (err < 0)
> +		dev_err(dev, "Poll could not pm_runtime_get: %i\n", err);
> +
> +	if (pm_runtime_active(dev))
> +		dsps_check_status(musb);
> +	else
> +		musb_queue_on_resume(musb, dsps_check_status_resume_work, NULL);
>  
>  	pm_runtime_mark_last_busy(dev);
>  	pm_runtime_put_autosuspend(dev);
> diff --git a/drivers/usb/musb/musb_gadget.c b/drivers/usb/musb/musb_gadget.c
> --- a/drivers/usb/musb/musb_gadget.c
> +++ b/drivers/usb/musb/musb_gadget.c
> @@ -1222,6 +1222,13 @@ void musb_ep_restart(struct musb *musb, struct musb_request *req)
>  		rxstate(musb, req);
>  }
>  
> +void musb_ep_restart_resume_work(struct musb *musb, void *data)
> +{
> +	struct musb_request *req = data;
> +
> +	musb_ep_restart(musb, req);

This one is supposed to be called with musb->lock held (according to the
function header anyway).

> +}
> +
>  static int musb_gadget_queue(struct usb_ep *ep, struct usb_request *req,
>  			gfp_t gfp_flags)
>  {
> @@ -1271,8 +1278,13 @@ static int musb_gadget_queue(struct usb_ep *ep, struct usb_request *req,
>  	list_add_tail(&request->list, &musb_ep->req_list);
>  
>  	/* it this is the head of the queue, start i/o ... */
> -	if (!musb_ep->busy && &request->list == musb_ep->req_list.next)
> -		musb_ep_restart(musb, request);
> +	if (!musb_ep->busy && &request->list == musb_ep->req_list.next) {
> +		if (pm_runtime_active(musb->controller))
> +			musb_ep_restart(musb, request);
> +		else
> +			musb_queue_on_resume(musb, musb_ep_restart_resume_work,
> +					     request);
> +	}
>  
>  unlock:
>  	spin_unlock_irqrestore(&musb->lock, lockflags);

And then this looks like it could trigger an ABBA deadlock as musb->lock
is held while queue_on_resume() takes musb->list_lock, and
musb_run_pending() would take the same locks in the reverse order.

Johan
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: musb RPM sleep-while-atomic in 4.9-rc1

[Tony Lindgren]

* Johan Hovold <johan-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> [161027 09:45]:
> On Thu, Oct 27, 2016 at 08:14:46AM -0700, Tony Lindgren wrote:
> > * Tony Lindgren <tony-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org> [161026 07:32]:
> 
> > 8< -------------------------------
> > From tony Mon Sep 17 00:00:00 2001
> > From: Tony Lindgren <tony-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org>
> > Date: Tue, 25 Oct 2016 08:42:00 -0700
> > Subject: [PATCH] usb: musb: Fix sleeping function called from invalid
> >  context for hdrc glue
> > 
> > Commit 65b3f50ed6fa ("usb: musb: Add PM runtime support for MUSB DSPS
> > glue layer") wrongly added a call for pm_runtime_get_sync to otg_timer
> > that runs in softirq context. That causes a "BUG: sleeping function called
> > from invalid context" every time when polling the cable status:
> > 
> > [<c015ebb4>] (__might_sleep) from [<c0413d60>] (__pm_runtime_resume+0x9c/0xa0)
> > [<c0413d60>] (__pm_runtime_resume) from [<c04d0bc4>] (otg_timer+0x3c/0x254)
> > [<c04d0bc4>] (otg_timer) from [<c0191180>] (call_timer_fn+0xfc/0x41c)
> > [<c0191180>] (call_timer_fn) from [<c01915c0>] (expire_timers+0x120/0x210)
> > [<c01915c0>] (expire_timers) from [<c0191acc>] (run_timer_softirq+0xa4/0xdc)
> > [<c0191acc>] (run_timer_softirq) from [<c010168c>] (__do_softirq+0x12c/0x594)
> > 
> > I did not notice that as I did not have CONFIG_DEBUG_ATOMIC_SLEEP enabled.
> > 
> > Let's fix the issue by adding dsps_check_status() and then register a
> > callback with musb_runtime_resume() so it gets called only when musb core
> > and it's parent devices are awake. Note that we don't want to do this from
> > PM runtime resume in musb_dsps.c as musb core is not awake yet at that
> > point as noted by Johan Hovold <johan-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>.
> 
> It seems some chunks are missing since this patch only runs the
> deferred work at remove and not at runtime resume, effectively breaking
> detection.

Oops sorry yeah looks like I had that in a separate debug hacks patch..

> > Note that musb_gadget_queue() also suffers from a similar issue when
> > connecting the cable and cannot use pm_runtime_get_sync().
> 
> You seem to have left that pm_runtime_get_sync() in there though.

And that one I must have hosed when cleaning up, thanks for noticing
these. Updated patch below.

Regards,

Tony

8< ---------------------
>From tony Mon Sep 17 00:00:00 2001
From: Tony Lindgren <tony-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org>
Date: Tue, 25 Oct 2016 08:42:00 -0700
Subject: [PATCH] usb: musb: Fix sleeping function called from invalid
 context for hdrc glue

Commit 65b3f50ed6fa ("usb: musb: Add PM runtime support for MUSB DSPS
glue layer") wrongly added a call for pm_runtime_get_sync to otg_timer
that runs in softirq context. That causes a "BUG: sleeping function called
from invalid context" every time when polling the cable status:

[<c015ebb4>] (__might_sleep) from [<c0413d60>] (__pm_runtime_resume+0x9c/0xa0)
[<c0413d60>] (__pm_runtime_resume) from [<c04d0bc4>] (otg_timer+0x3c/0x254)
[<c04d0bc4>] (otg_timer) from [<c0191180>] (call_timer_fn+0xfc/0x41c)
[<c0191180>] (call_timer_fn) from [<c01915c0>] (expire_timers+0x120/0x210)
[<c01915c0>] (expire_timers) from [<c0191acc>] (run_timer_softirq+0xa4/0xdc)
[<c0191acc>] (run_timer_softirq) from [<c010168c>] (__do_softirq+0x12c/0x594)

I did not notice that as I did not have CONFIG_DEBUG_ATOMIC_SLEEP enabled.

Let's fix the issue by adding dsps_check_status() and then register a
callback with musb_runtime_resume() so it gets called only when musb core
and it's parent devices are awake. Note that we don't want to do this from
PM runtime resume in musb_dsps.c as musb core is not awake yet at that
point as noted by Johan Hovold <johan-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>.

Note that musb_gadget_queue() also suffers from a similar issue when
connecting the cable and cannot use pm_runtime_get_sync().

Fixes: 65b3f50ed6fa ("usb: musb: Add PM runtime support for MUSB DSPS
glue layer")
Reported-by: Johan Hovold <johan-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
Signed-off-by: Tony Lindgren <tony-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org>
---
 drivers/usb/musb/musb_core.c   | 48 ++++++++++++++++++++++++++++++++++++++++--
 drivers/usb/musb/musb_core.h   |  7 ++++++
 drivers/usb/musb/musb_dsps.c   | 29 +++++++++++++++++++------
 drivers/usb/musb/musb_gadget.c | 18 +++++++++++++---
 4 files changed, 90 insertions(+), 12 deletions(-)

diff --git a/drivers/usb/musb/musb_core.c b/drivers/usb/musb/musb_core.c
--- a/drivers/usb/musb/musb_core.c
+++ b/drivers/usb/musb/musb_core.c
@@ -122,7 +122,6 @@ MODULE_AUTHOR(DRIVER_AUTHOR);
 MODULE_LICENSE("GPL");
 MODULE_ALIAS("platform:" MUSB_DRIVER_NAME);
 
-
 /*-------------------------------------------------------------------------*/
 
 static inline struct musb *dev_to_musb(struct device *dev)
@@ -1896,6 +1895,46 @@ static void musb_pm_runtime_check_session(struct musb *musb)
 	musb->session = s;
 }
 
+struct musb_resume_work {
+	void (*callback)(struct musb *musb, void *data);
+	void *data;
+	struct list_head node;
+};
+
+void musb_queue_on_resume(struct musb *musb,
+			  void (*callback)(struct musb *musb, void *data),
+			  void *data)
+{
+	struct musb_resume_work *w;
+	unsigned long flags;
+
+	w = devm_kzalloc(musb->controller, sizeof(*w), GFP_ATOMIC);
+	if (!w)
+		return;
+
+	w->callback = callback;
+	w->data = data;
+	spin_lock_irqsave(&musb->list_lock, flags);
+	list_add_tail(&w->node, &musb->resume_work);
+	spin_unlock_irqrestore(&musb->list_lock, flags);
+}
+EXPORT_SYMBOL_GPL(musb_queue_on_resume);
+
+static void musb_run_pending(struct musb *musb)
+{
+	struct musb_resume_work *w, *_w;
+	unsigned long flags;
+
+	spin_lock_irqsave(&musb->list_lock, flags);
+	list_for_each_entry_safe(w, _w, &musb->resume_work, node) {
+		if (w->callback)
+			w->callback(musb, w->data);
+		list_del(&w->node);
+		devm_kfree(musb->controller, w);
+	}
+	spin_unlock_irqrestore(&musb->list_lock, flags);
+}
+
 /* Only used to provide driver mode change events */
 static void musb_irq_work(struct work_struct *data)
 {
@@ -1969,6 +2008,7 @@ static struct musb *allocate_instance(struct device *dev,
 	INIT_LIST_HEAD(&musb->control);
 	INIT_LIST_HEAD(&musb->in_bulk);
 	INIT_LIST_HEAD(&musb->out_bulk);
+	INIT_LIST_HEAD(&musb->resume_work);
 
 	musb->vbuserr_retry = VBUSERR_RETRY_COUNT;
 	musb->a_wait_bcon = OTG_TIME_A_WAIT_BCON;
@@ -2065,6 +2105,7 @@ musb_init_controller(struct device *dev, int nIrq, void __iomem *ctrl)
 	}
 
 	spin_lock_init(&musb->lock);
+	spin_lock_init(&musb->list_lock);
 	musb->board_set_power = plat->set_power;
 	musb->min_power = plat->min_power;
 	musb->ops = plat->platform_ops;
@@ -2374,6 +2415,7 @@ static int musb_remove(struct platform_device *pdev)
 	 *  - Peripheral mode: peripheral is deactivated (or never-activated)
 	 *  - OTG mode: both roles are deactivated (or never-activated)
 	 */
+	musb_run_pending(musb);
 	musb_exit_debugfs(musb);
 
 	cancel_work_sync(&musb->irq_work);
@@ -2645,8 +2687,10 @@ static int musb_runtime_resume(struct device *dev)
 	 * Also context restore without save does not make
 	 * any sense
 	 */
-	if (!first)
+	if (!first) {
 		musb_restore_context(musb);
+		musb_run_pending(musb);
+	}
 	first = 0;
 
 	if (musb->need_finish_resume) {
diff --git a/drivers/usb/musb/musb_core.h b/drivers/usb/musb/musb_core.h
--- a/drivers/usb/musb/musb_core.h
+++ b/drivers/usb/musb/musb_core.h
@@ -303,6 +303,7 @@ struct musb_context_registers {
 struct musb {
 	/* device lock */
 	spinlock_t		lock;
+	spinlock_t		list_lock;	/* resume work list lock */
 
 	struct musb_io		io;
 	const struct musb_platform_ops *ops;
@@ -337,6 +338,7 @@ struct musb {
 	struct list_head	control;	/* of musb_qh */
 	struct list_head	in_bulk;	/* of musb_qh */
 	struct list_head	out_bulk;	/* of musb_qh */
+	struct list_head	resume_work;	/* pending work on resume */
 
 	struct timer_list	otg_timer;
 	struct notifier_block	nb;
@@ -540,6 +542,11 @@ extern irqreturn_t musb_interrupt(struct musb *);
 
 extern void musb_hnp_stop(struct musb *musb);
 
+extern void
+musb_queue_on_resume(struct musb *musb,
+		     void (*callback)(struct musb *musb, void *data),
+		     void *data);
+
 static inline void musb_platform_set_vbus(struct musb *musb, int is_on)
 {
 	if (musb->ops->set_vbus)
diff --git a/drivers/usb/musb/musb_dsps.c b/drivers/usb/musb/musb_dsps.c
--- a/drivers/usb/musb/musb_dsps.c
+++ b/drivers/usb/musb/musb_dsps.c
@@ -188,9 +188,8 @@ static void dsps_musb_disable(struct musb *musb)
 	musb_writeb(musb->mregs, MUSB_DEVCTL, 0);
 }
 
-static void otg_timer(unsigned long _musb)
+static void dsps_check_status(struct musb *musb)
 {
-	struct musb *musb = (void *)_musb;
 	void __iomem *mregs = musb->mregs;
 	struct device *dev = musb->controller;
 	struct dsps_glue *glue = dev_get_drvdata(dev->parent);
@@ -198,11 +197,6 @@ static void otg_timer(unsigned long _musb)
 	u8 devctl;
 	unsigned long flags;
 	int skip_session = 0;
-	int err;
-
-	err = pm_runtime_get_sync(dev);
-	if (err < 0)
-		dev_err(dev, "Poll could not pm_runtime_get: %i\n", err);
 
 	/*
 	 * We poll because DSPS IP's won't expose several OTG-critical
@@ -246,6 +240,27 @@ static void otg_timer(unsigned long _musb)
 		break;
 	}
 	spin_unlock_irqrestore(&musb->lock, flags);
+}
+
+static void dsps_check_status_resume_work(struct musb *musb, void *unused)
+{
+	dsps_check_status(musb);
+}
+
+static void otg_timer(unsigned long _musb)
+{
+	struct musb *musb = (void *)_musb;
+	struct device *dev = musb->controller;
+	int err;
+
+	err = pm_runtime_get(dev);
+	if (err < 0)
+		dev_err(dev, "Poll could not pm_runtime_get: %i\n", err);
+
+	if (pm_runtime_active(dev))
+		dsps_check_status(musb);
+	else
+		musb_queue_on_resume(musb, dsps_check_status_resume_work, NULL);
 
 	pm_runtime_mark_last_busy(dev);
 	pm_runtime_put_autosuspend(dev);
diff --git a/drivers/usb/musb/musb_gadget.c b/drivers/usb/musb/musb_gadget.c
--- a/drivers/usb/musb/musb_gadget.c
+++ b/drivers/usb/musb/musb_gadget.c
@@ -1222,6 +1222,13 @@ void musb_ep_restart(struct musb *musb, struct musb_request *req)
 		rxstate(musb, req);
 }
 
+void musb_ep_restart_resume_work(struct musb *musb, void *data)
+{
+	struct musb_request *req = data;
+
+	musb_ep_restart(musb, req);
+}
+
 static int musb_gadget_queue(struct usb_ep *ep, struct usb_request *req,
 			gfp_t gfp_flags)
 {
@@ -1255,7 +1262,7 @@ static int musb_gadget_queue(struct usb_ep *ep, struct usb_request *req,
 
 	map_dma_buffer(request, musb, musb_ep);
 
-	pm_runtime_get_sync(musb->controller);
+	pm_runtime_get(musb->controller);
 	spin_lock_irqsave(&musb->lock, lockflags);
 
 	/* don't queue if the ep is down */
@@ -1271,8 +1278,13 @@ static int musb_gadget_queue(struct usb_ep *ep, struct usb_request *req,
 	list_add_tail(&request->list, &musb_ep->req_list);
 
 	/* it this is the head of the queue, start i/o ... */
-	if (!musb_ep->busy && &request->list == musb_ep->req_list.next)
-		musb_ep_restart(musb, request);
+	if (!musb_ep->busy && &request->list == musb_ep->req_list.next) {
+		if (pm_runtime_active(musb->controller))
+			musb_ep_restart(musb, request);
+		else
+			musb_queue_on_resume(musb, musb_ep_restart_resume_work,
+					     request);
+	}
 
 unlock:
 	spin_unlock_irqrestore(&musb->lock, lockflags);
-- 
2.9.3
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: musb RPM sleep-while-atomic in 4.9-rc1

[Johan Hovold]

On Thu, Oct 27, 2016 at 10:40:17AM -0700, Tony Lindgren wrote:
> * Johan Hovold <johan-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> [161027 09:45]:
> > On Thu, Oct 27, 2016 at 08:14:46AM -0700, Tony Lindgren wrote:
> > > * Tony Lindgren <tony-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org> [161026 07:32]:
> > 
> > > 8< -------------------------------
> > > From tony Mon Sep 17 00:00:00 2001
> > > From: Tony Lindgren <tony-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org>
> > > Date: Tue, 25 Oct 2016 08:42:00 -0700
> > > Subject: [PATCH] usb: musb: Fix sleeping function called from invalid
> > >  context for hdrc glue
> > > 
> > > Commit 65b3f50ed6fa ("usb: musb: Add PM runtime support for MUSB DSPS
> > > glue layer") wrongly added a call for pm_runtime_get_sync to otg_timer
> > > that runs in softirq context. That causes a "BUG: sleeping function called
> > > from invalid context" every time when polling the cable status:
> > > 
> > > [<c015ebb4>] (__might_sleep) from [<c0413d60>] (__pm_runtime_resume+0x9c/0xa0)
> > > [<c0413d60>] (__pm_runtime_resume) from [<c04d0bc4>] (otg_timer+0x3c/0x254)
> > > [<c04d0bc4>] (otg_timer) from [<c0191180>] (call_timer_fn+0xfc/0x41c)
> > > [<c0191180>] (call_timer_fn) from [<c01915c0>] (expire_timers+0x120/0x210)
> > > [<c01915c0>] (expire_timers) from [<c0191acc>] (run_timer_softirq+0xa4/0xdc)
> > > [<c0191acc>] (run_timer_softirq) from [<c010168c>] (__do_softirq+0x12c/0x594)
> > > 
> > > I did not notice that as I did not have CONFIG_DEBUG_ATOMIC_SLEEP enabled.
> > > 
> > > Let's fix the issue by adding dsps_check_status() and then register a
> > > callback with musb_runtime_resume() so it gets called only when musb core
> > > and it's parent devices are awake. Note that we don't want to do this from
> > > PM runtime resume in musb_dsps.c as musb core is not awake yet at that
> > > point as noted by Johan Hovold <johan-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>.
> > 
> > It seems some chunks are missing since this patch only runs the
> > deferred work at remove and not at runtime resume, effectively breaking
> > detection.
> 
> Oops sorry yeah looks like I had that in a separate debug hacks patch..
> 
> > > Note that musb_gadget_queue() also suffers from a similar issue when
> > > connecting the cable and cannot use pm_runtime_get_sync().
> > 
> > You seem to have left that pm_runtime_get_sync() in there though.
> 
> And that one I must have hosed when cleaning up, thanks for noticing
> these. Updated patch below.

I had a couple of inline comments to the previous version about locking
in the gadget code as well (hidden after too much context). Looks like
there's a lock missing for the deferred work, and something that seems
like a possible ABBA deadlock.

> diff --git a/drivers/usb/musb/musb_gadget.c b/drivers/usb/musb/musb_gadget.c
> --- a/drivers/usb/musb/musb_gadget.c
> +++ b/drivers/usb/musb/musb_gadget.c
> @@ -1222,6 +1222,13 @@ void musb_ep_restart(struct musb *musb, struct musb_request *req)
>  		rxstate(musb, req);
>  }
>  
> +void musb_ep_restart_resume_work(struct musb *musb, void *data)
> +{
> +	struct musb_request *req = data;
> +
> +	musb_ep_restart(musb, req);

This one is supposed to be called with musb->lock held (according to the
function header anyway).

> +}
> +
>  static int musb_gadget_queue(struct usb_ep *ep, struct usb_request *req,
>  			gfp_t gfp_flags)
>  {
> @@ -1255,7 +1262,7 @@ static int musb_gadget_queue(struct usb_ep *ep, struct usb_request *req,
>  
>  	map_dma_buffer(request, musb, musb_ep);
>  
> -	pm_runtime_get_sync(musb->controller);
> +	pm_runtime_get(musb->controller);
>  	spin_lock_irqsave(&musb->lock, lockflags);
>  
>  	/* don't queue if the ep is down */
> @@ -1271,8 +1278,13 @@ static int musb_gadget_queue(struct usb_ep *ep, struct usb_request *req,
>  	list_add_tail(&request->list, &musb_ep->req_list);
>  
>  	/* it this is the head of the queue, start i/o ... */
> -	if (!musb_ep->busy && &request->list == musb_ep->req_list.next)
> -		musb_ep_restart(musb, request);
> +	if (!musb_ep->busy && &request->list == musb_ep->req_list.next) {
> +		if (pm_runtime_active(musb->controller))
> +			musb_ep_restart(musb, request);
> +		else
> +			musb_queue_on_resume(musb, musb_ep_restart_resume_work,
> +					     request);
> +	}

But then this looks like it could trigger an ABBA deadlock as musb->lock
is held while queue_on_resume() takes musb->list_lock, and
musb_run_pending() would take the same locks in the reverse order.

>  
>  unlock:
>  	spin_unlock_irqrestore(&musb->lock, lockflags);

Johan
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: musb RPM sleep-while-atomic in 4.9-rc1

[Tony Lindgren]

* Johan Hovold <johan-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> [161027 11:46]:
> On Thu, Oct 27, 2016 at 10:40:17AM -0700, Tony Lindgren wrote:
> > And that one I must have hosed when cleaning up, thanks for noticing
> > these. Updated patch below.
> 
> I had a couple of inline comments to the previous version about locking
> in the gadget code as well (hidden after too much context). Looks like
> there's a lock missing for the deferred work, and something that seems
> like a possible ABBA deadlock.

Oh sorry, I totally missed those.

> > diff --git a/drivers/usb/musb/musb_gadget.c b/drivers/usb/musb/musb_gadget.c
> > --- a/drivers/usb/musb/musb_gadget.c
> > +++ b/drivers/usb/musb/musb_gadget.c
> > @@ -1222,6 +1222,13 @@ void musb_ep_restart(struct musb *musb, struct musb_request *req)
> >  		rxstate(musb, req);
> >  }
> >  
> > +void musb_ep_restart_resume_work(struct musb *musb, void *data)
> > +{
> > +	struct musb_request *req = data;
> > +
> > +	musb_ep_restart(musb, req);
> 
> This one is supposed to be called with musb->lock held (according to the
> function header anyway).

Good point, yeah that calls the monster functions txstate and rxstate.

> >  static int musb_gadget_queue(struct usb_ep *ep, struct usb_request *req,
> >  			gfp_t gfp_flags)
> >  {
> > @@ -1255,7 +1262,7 @@ static int musb_gadget_queue(struct usb_ep *ep, struct usb_request *req,
> >  
> >  	map_dma_buffer(request, musb, musb_ep);
> >  
> > -	pm_runtime_get_sync(musb->controller);
> > +	pm_runtime_get(musb->controller);
> >  	spin_lock_irqsave(&musb->lock, lockflags);
> >  
> >  	/* don't queue if the ep is down */
> > @@ -1271,8 +1278,13 @@ static int musb_gadget_queue(struct usb_ep *ep, struct usb_request *req,
> >  	list_add_tail(&request->list, &musb_ep->req_list);
> >  
> >  	/* it this is the head of the queue, start i/o ... */
> > -	if (!musb_ep->busy && &request->list == musb_ep->req_list.next)
> > -		musb_ep_restart(musb, request);
> > +	if (!musb_ep->busy && &request->list == musb_ep->req_list.next) {
> > +		if (pm_runtime_active(musb->controller))
> > +			musb_ep_restart(musb, request);
> > +		else
> > +			musb_queue_on_resume(musb, musb_ep_restart_resume_work,
> > +					     request);
> > +	}
> 
> But then this looks like it could trigger an ABBA deadlock as musb->lock
> is held while queue_on_resume() takes musb->list_lock, and
> musb_run_pending() would take the same locks in the reverse order.

It seems we can avoid that by locking only list_add_tail() and list_del():

list_for_each_entry_safe(w, _w, &musb->resume_work, node) {
	spin_lock_irqsave(&musb->list_lock, flags);
	list_del(&w->node);
	spin_unlock_irqrestore(&musb->list_lock, flags);
	if (w->callback)
		w->callback(musb, w->data);
	devm_kfree(musb->controller, w);
}

Or do you have some better ideas?

Regards,

Tony
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: musb RPM sleep-while-atomic in 4.9-rc1

[Johan Hovold]

On Thu, Oct 27, 2016 at 12:15:52PM -0700, Tony Lindgren wrote:
> * Johan Hovold <johan-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> [161027 11:46]:
> > On Thu, Oct 27, 2016 at 10:40:17AM -0700, Tony Lindgren wrote:

> > > diff --git a/drivers/usb/musb/musb_gadget.c b/drivers/usb/musb/musb_gadget.c
> > > --- a/drivers/usb/musb/musb_gadget.c
> > > +++ b/drivers/usb/musb/musb_gadget.c
> > > @@ -1222,6 +1222,13 @@ void musb_ep_restart(struct musb *musb, struct musb_request *req)
> > >  		rxstate(musb, req);
> > >  }
> > >  
> > > +void musb_ep_restart_resume_work(struct musb *musb, void *data)
> > > +{
> > > +	struct musb_request *req = data;
> > > +
> > > +	musb_ep_restart(musb, req);
> > 
> > This one is supposed to be called with musb->lock held (according to the
> > function header anyway).
> 
> Good point, yeah that calls the monster functions txstate and rxstate.
> 
> > >  static int musb_gadget_queue(struct usb_ep *ep, struct usb_request *req,
> > >  			gfp_t gfp_flags)
> > >  {
> > > @@ -1255,7 +1262,7 @@ static int musb_gadget_queue(struct usb_ep *ep, struct usb_request *req,
> > >  
> > >  	map_dma_buffer(request, musb, musb_ep);
> > >  
> > > -	pm_runtime_get_sync(musb->controller);
> > > +	pm_runtime_get(musb->controller);
> > >  	spin_lock_irqsave(&musb->lock, lockflags);
> > >  
> > >  	/* don't queue if the ep is down */
> > > @@ -1271,8 +1278,13 @@ static int musb_gadget_queue(struct usb_ep *ep, struct usb_request *req,
> > >  	list_add_tail(&request->list, &musb_ep->req_list);
> > >  
> > >  	/* it this is the head of the queue, start i/o ... */
> > > -	if (!musb_ep->busy && &request->list == musb_ep->req_list.next)
> > > -		musb_ep_restart(musb, request);
> > > +	if (!musb_ep->busy && &request->list == musb_ep->req_list.next) {
> > > +		if (pm_runtime_active(musb->controller))
> > > +			musb_ep_restart(musb, request);
> > > +		else
> > > +			musb_queue_on_resume(musb, musb_ep_restart_resume_work,
> > > +					     request);
> > > +	}
> > 
> > But then this looks like it could trigger an ABBA deadlock as musb->lock
> > is held while queue_on_resume() takes musb->list_lock, and
> > musb_run_pending() would take the same locks in the reverse order.
> 
> It seems we can avoid that by locking only list_add_tail() and list_del():
> 
> list_for_each_entry_safe(w, _w, &musb->resume_work, node) {
> 	spin_lock_irqsave(&musb->list_lock, flags);
> 	list_del(&w->node);
> 	spin_unlock_irqrestore(&musb->list_lock, flags);
> 	if (w->callback)
> 		w->callback(musb, w->data);
> 	devm_kfree(musb->controller, w);
> }

I think you still need to hold the lock while traversing the list (even
if you temporarily release it during the callback).

But this is related to another issue; what if a new callback is
registered while musb_runtime_resume is running? It seems we could end
up queueing work which might never be executed (e.g. if queued just
after musb_run_pending() is done or while processing the last
callback).

> Or do you have some better ideas?

Not right now sorry, and I'll be away from keyboard the rest of the day,
I'm afraid.

Johan
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: musb RPM sleep-while-atomic in 4.9-rc1

[Tony Lindgren]

* Johan Hovold <johan-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> [161028 02:45]:
> On Thu, Oct 27, 2016 at 12:15:52PM -0700, Tony Lindgren wrote:
> > * Johan Hovold <johan-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> [161027 11:46]:
> > > But then this looks like it could trigger an ABBA deadlock as musb->lock
> > > is held while queue_on_resume() takes musb->list_lock, and
> > > musb_run_pending() would take the same locks in the reverse order.
> > 
> > It seems we can avoid that by locking only list_add_tail() and list_del():
> > 
> > list_for_each_entry_safe(w, _w, &musb->resume_work, node) {
> > 	spin_lock_irqsave(&musb->list_lock, flags);
> > 	list_del(&w->node);
> > 	spin_unlock_irqrestore(&musb->list_lock, flags);
> > 	if (w->callback)
> > 		w->callback(musb, w->data);
> > 	devm_kfree(musb->controller, w);
> > }
> 
> I think you still need to hold the lock while traversing the list (even
> if you temporarily release it during the callback).

Hmm yeah we need iterate through the list again to avoid missing new
elements being added. I've updated the patch to use a the common
while (!list_empty(&musb->resume_work)) loop. Does that solve the
concern you had or did you also had some other concern there?

Regards,

Tony

8< ---------------------------
>From tony Mon Sep 17 00:00:00 2001
From: Tony Lindgren <tony-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org>
Date: Tue, 25 Oct 2016 08:42:00 -0700
Subject: [PATCH] usb: musb: Fix sleeping function called from invalid
 context for hdrc glue

Commit 65b3f50ed6fa ("usb: musb: Add PM runtime support for MUSB DSPS
glue layer") wrongly added a call for pm_runtime_get_sync to otg_timer
that runs in softirq context. That causes a "BUG: sleeping function called
from invalid context" every time when polling the cable status:

[<c015ebb4>] (__might_sleep) from [<c0413d60>] (__pm_runtime_resume+0x9c/0xa0)
[<c0413d60>] (__pm_runtime_resume) from [<c04d0bc4>] (otg_timer+0x3c/0x254)
[<c04d0bc4>] (otg_timer) from [<c0191180>] (call_timer_fn+0xfc/0x41c)
[<c0191180>] (call_timer_fn) from [<c01915c0>] (expire_timers+0x120/0x210)
[<c01915c0>] (expire_timers) from [<c0191acc>] (run_timer_softirq+0xa4/0xdc)
[<c0191acc>] (run_timer_softirq) from [<c010168c>] (__do_softirq+0x12c/0x594)

I did not notice that as I did not have CONFIG_DEBUG_ATOMIC_SLEEP enabled.

Let's fix the issue by adding dsps_check_status() and then register a
callback with musb_runtime_resume() so it gets called only when musb core
and it's parent devices are awake. Note that we don't want to do this from
PM runtime resume in musb_dsps.c as musb core is not awake yet at that
point as noted by Johan Hovold <johan-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>.

Note that musb_gadget_queue() also suffers from a similar issue when
connecting the cable and cannot use pm_runtime_get_sync().

Fixes: 65b3f50ed6fa ("usb: musb: Add PM runtime support for MUSB DSPS
glue layer")
Reported-by: Johan Hovold <johan-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
Signed-off-by: Tony Lindgren <tony-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org>
---
 drivers/usb/musb/musb_core.c   | 52 +++++++++++++++++++++++++++++++++++++++++-
 drivers/usb/musb/musb_core.h   |  7 ++++++
 drivers/usb/musb/musb_dsps.c   | 29 +++++++++++++++++------
 drivers/usb/musb/musb_gadget.c | 21 ++++++++++++++---
 4 files changed, 98 insertions(+), 11 deletions(-)

diff --git a/drivers/usb/musb/musb_core.c b/drivers/usb/musb/musb_core.c
--- a/drivers/usb/musb/musb_core.c
+++ b/drivers/usb/musb/musb_core.c
@@ -1896,6 +1896,51 @@ static void musb_pm_runtime_check_session(struct musb *musb)
 	musb->session = s;
 }
 
+struct musb_resume_work {
+	void (*callback)(struct musb *musb, void *data);
+	void *data;
+	struct list_head node;
+};
+
+void musb_queue_on_resume(struct musb *musb,
+			  void (*callback)(struct musb *musb, void *data),
+			  void *data)
+{
+	struct musb_resume_work *w;
+	unsigned long flags;
+
+	w = devm_kzalloc(musb->controller, sizeof(*w), GFP_ATOMIC);
+	if (!w)
+		return;
+
+	w->callback = callback;
+	w->data = data;
+	spin_lock_irqsave(&musb->list_lock, flags);
+	list_add_tail(&w->node, &musb->resume_work);
+	spin_unlock_irqrestore(&musb->list_lock, flags);
+}
+EXPORT_SYMBOL_GPL(musb_queue_on_resume);
+
+static void musb_run_pending(struct musb *musb)
+{
+	struct musb_resume_work *w;
+	unsigned long flags;
+
+	spin_lock_irqsave(&musb->list_lock, flags);
+	while (!list_empty(&musb->resume_work)) {
+		w = list_first_entry(&musb->resume_work,
+				     struct musb_resume_work,
+				     node);
+		list_del(&w->node);
+		spin_unlock_irqrestore(&musb->list_lock, flags);
+		if (w->callback)
+			w->callback(musb, w->data);
+		devm_kfree(musb->controller, w);
+		spin_lock_irqsave(&musb->list_lock, flags);
+	}
+	spin_unlock_irqrestore(&musb->list_lock, flags);
+}
+
 /* Only used to provide driver mode change events */
 static void musb_irq_work(struct work_struct *data)
 {
@@ -1969,6 +2014,7 @@ static struct musb *allocate_instance(struct device *dev,
 	INIT_LIST_HEAD(&musb->control);
 	INIT_LIST_HEAD(&musb->in_bulk);
 	INIT_LIST_HEAD(&musb->out_bulk);
+	INIT_LIST_HEAD(&musb->resume_work);
 
 	musb->vbuserr_retry = VBUSERR_RETRY_COUNT;
 	musb->a_wait_bcon = OTG_TIME_A_WAIT_BCON;
@@ -2065,6 +2111,7 @@ musb_init_controller(struct device *dev, int nIrq, void __iomem *ctrl)
 	}
 
 	spin_lock_init(&musb->lock);
+	spin_lock_init(&musb->list_lock);
 	musb->board_set_power = plat->set_power;
 	musb->min_power = plat->min_power;
 	musb->ops = plat->platform_ops;
@@ -2374,6 +2421,7 @@ static int musb_remove(struct platform_device *pdev)
 	 *  - Peripheral mode: peripheral is deactivated (or never-activated)
 	 *  - OTG mode: both roles are deactivated (or never-activated)
 	 */
+	musb_run_pending(musb);
 	musb_exit_debugfs(musb);
 
 	cancel_work_sync(&musb->irq_work);
@@ -2645,8 +2693,10 @@ static int musb_runtime_resume(struct device *dev)
 	 * Also context restore without save does not make
 	 * any sense
 	 */
-	if (!first)
+	if (!first) {
 		musb_restore_context(musb);
+		musb_run_pending(musb);
+	}
 	first = 0;
 
 	if (musb->need_finish_resume) {
diff --git a/drivers/usb/musb/musb_core.h b/drivers/usb/musb/musb_core.h
--- a/drivers/usb/musb/musb_core.h
+++ b/drivers/usb/musb/musb_core.h
@@ -303,6 +303,7 @@ struct musb_context_registers {
 struct musb {
 	/* device lock */
 	spinlock_t		lock;
+	spinlock_t		list_lock;	/* resume work list lock */
 
 	struct musb_io		io;
 	const struct musb_platform_ops *ops;
@@ -337,6 +338,7 @@ struct musb {
 	struct list_head	control;	/* of musb_qh */
 	struct list_head	in_bulk;	/* of musb_qh */
 	struct list_head	out_bulk;	/* of musb_qh */
+	struct list_head	resume_work;	/* pending work on resume */
 
 	struct timer_list	otg_timer;
 	struct notifier_block	nb;
@@ -540,6 +542,11 @@ extern irqreturn_t musb_interrupt(struct musb *);
 
 extern void musb_hnp_stop(struct musb *musb);
 
+extern void
+musb_queue_on_resume(struct musb *musb,
+		     void (*callback)(struct musb *musb, void *data),
+		     void *data);
+
 static inline void musb_platform_set_vbus(struct musb *musb, int is_on)
 {
 	if (musb->ops->set_vbus)
diff --git a/drivers/usb/musb/musb_dsps.c b/drivers/usb/musb/musb_dsps.c
--- a/drivers/usb/musb/musb_dsps.c
+++ b/drivers/usb/musb/musb_dsps.c
@@ -188,9 +188,8 @@ static void dsps_musb_disable(struct musb *musb)
 	musb_writeb(musb->mregs, MUSB_DEVCTL, 0);
 }
 
-static void otg_timer(unsigned long _musb)
+static void dsps_check_status(struct musb *musb)
 {
-	struct musb *musb = (void *)_musb;
 	void __iomem *mregs = musb->mregs;
 	struct device *dev = musb->controller;
 	struct dsps_glue *glue = dev_get_drvdata(dev->parent);
@@ -198,11 +197,6 @@ static void otg_timer(unsigned long _musb)
 	u8 devctl;
 	unsigned long flags;
 	int skip_session = 0;
-	int err;
-
-	err = pm_runtime_get_sync(dev);
-	if (err < 0)
-		dev_err(dev, "Poll could not pm_runtime_get: %i\n", err);
 
 	/*
 	 * We poll because DSPS IP's won't expose several OTG-critical
@@ -246,6 +240,27 @@ static void otg_timer(unsigned long _musb)
 		break;
 	}
 	spin_unlock_irqrestore(&musb->lock, flags);
+}
+
+static void dsps_check_status_resume_work(struct musb *musb, void *unused)
+{
+	dsps_check_status(musb);
+}
+
+static void otg_timer(unsigned long _musb)
+{
+	struct musb *musb = (void *)_musb;
+	struct device *dev = musb->controller;
+	int err;
+
+	err = pm_runtime_get(dev);
+	if (err < 0)
+		dev_err(dev, "Poll could not pm_runtime_get: %i\n", err);
+
+	if (pm_runtime_active(dev))
+		dsps_check_status(musb);
+	else
+		musb_queue_on_resume(musb, dsps_check_status_resume_work, NULL);
 
 	pm_runtime_mark_last_busy(dev);
 	pm_runtime_put_autosuspend(dev);
diff --git a/drivers/usb/musb/musb_gadget.c b/drivers/usb/musb/musb_gadget.c
--- a/drivers/usb/musb/musb_gadget.c
+++ b/drivers/usb/musb/musb_gadget.c
@@ -1222,6 +1222,16 @@ void musb_ep_restart(struct musb *musb, struct musb_request *req)
 		rxstate(musb, req);
 }
 
+void musb_ep_restart_resume_work(struct musb *musb, void *data)
+{
+	struct musb_request *req = data;
+	unsigned long flags;
+
+	spin_lock_irqsave(&musb->lock, flags);
+	musb_ep_restart(musb, req);
+	spin_unlock_irqrestore(&musb->lock, flags);
+}
+
 static int musb_gadget_queue(struct usb_ep *ep, struct usb_request *req,
 			gfp_t gfp_flags)
 {
@@ -1255,7 +1265,7 @@ static int musb_gadget_queue(struct usb_ep *ep, struct usb_request *req,
 
 	map_dma_buffer(request, musb, musb_ep);
 
-	pm_runtime_get_sync(musb->controller);
+	pm_runtime_get(musb->controller);
 	spin_lock_irqsave(&musb->lock, lockflags);
 
 	/* don't queue if the ep is down */
@@ -1271,8 +1281,13 @@ static int musb_gadget_queue(struct usb_ep *ep, struct usb_request *req,
 	list_add_tail(&request->list, &musb_ep->req_list);
 
 	/* it this is the head of the queue, start i/o ... */
-	if (!musb_ep->busy && &request->list == musb_ep->req_list.next)
-		musb_ep_restart(musb, request);
+	if (!musb_ep->busy && &request->list == musb_ep->req_list.next) {
+		if (pm_runtime_active(musb->controller))
+			musb_ep_restart(musb, request);
+		else
+			musb_queue_on_resume(musb, musb_ep_restart_resume_work,
+					     request);
+	}
 
 unlock:
 	spin_unlock_irqrestore(&musb->lock, lockflags);
-- 
2.9.3
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: musb RPM sleep-while-atomic in 4.9-rc1

[Johan Hovold]

On Fri, Oct 28, 2016 at 11:13:19AM -0700, Tony Lindgren wrote:
> * Johan Hovold <johan-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> [161028 02:45]:
> > On Thu, Oct 27, 2016 at 12:15:52PM -0700, Tony Lindgren wrote:
> > > * Johan Hovold <johan-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> [161027 11:46]:
> > > > But then this looks like it could trigger an ABBA deadlock as musb->lock
> > > > is held while queue_on_resume() takes musb->list_lock, and
> > > > musb_run_pending() would take the same locks in the reverse order.
> > > 
> > > It seems we can avoid that by locking only list_add_tail() and list_del():
> > > 
> > > list_for_each_entry_safe(w, _w, &musb->resume_work, node) {
> > > 	spin_lock_irqsave(&musb->list_lock, flags);
> > > 	list_del(&w->node);
> > > 	spin_unlock_irqrestore(&musb->list_lock, flags);
> > > 	if (w->callback)
> > > 		w->callback(musb, w->data);
> > > 	devm_kfree(musb->controller, w);
> > > }
> > 
> > I think you still need to hold the lock while traversing the list (even
> > if you temporarily release it during the callback).
> 
> Hmm yeah we need iterate through the list again to avoid missing new
> elements being added. I've updated the patch to use a the common
> while (!list_empty(&musb->resume_work)) loop. Does that solve the
> concern you had or did you also had some other concern there?

Yeah, while that minimises the race window it is still possible that the
timer callback checks pm_runtime_active() after the queue has been
processed but before the rpm status is updated. 

How about using a work struct and synchronous get for the deferred case?

Johan
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: musb RPM sleep-while-atomic in 4.9-rc1

[Tony Lindgren]

* Johan Hovold <johan-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> [161031 04:50]:
> On Fri, Oct 28, 2016 at 11:13:19AM -0700, Tony Lindgren wrote:
> > * Johan Hovold <johan-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> [161028 02:45]:
> > > On Thu, Oct 27, 2016 at 12:15:52PM -0700, Tony Lindgren wrote:
> > > > * Johan Hovold <johan-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> [161027 11:46]:
> > > > > But then this looks like it could trigger an ABBA deadlock as musb->lock
> > > > > is held while queue_on_resume() takes musb->list_lock, and
> > > > > musb_run_pending() would take the same locks in the reverse order.
> > > > 
> > > > It seems we can avoid that by locking only list_add_tail() and list_del():
> > > > 
> > > > list_for_each_entry_safe(w, _w, &musb->resume_work, node) {
> > > > 	spin_lock_irqsave(&musb->list_lock, flags);
> > > > 	list_del(&w->node);
> > > > 	spin_unlock_irqrestore(&musb->list_lock, flags);
> > > > 	if (w->callback)
> > > > 		w->callback(musb, w->data);
> > > > 	devm_kfree(musb->controller, w);
> > > > }
> > > 
> > > I think you still need to hold the lock while traversing the list (even
> > > if you temporarily release it during the callback).
> > 
> > Hmm yeah we need iterate through the list again to avoid missing new
> > elements being added. I've updated the patch to use a the common
> > while (!list_empty(&musb->resume_work)) loop. Does that solve the
> > concern you had or did you also had some other concern there?
> 
> Yeah, while that minimises the race window it is still possible that the
> timer callback checks pm_runtime_active() after the queue has been
> processed but before the rpm status is updated. 

OK. Sorry for the delay responding, had my motherboard fail
over the weekend..

> How about using a work struct and synchronous get for the deferred case?

Here's the patch updated to use the existing finish_resume_work.
Is that along the lines you were thinking?

Adding also Ladis to Cc too.

Regards,

Tony

8< ----------------------------
>From tony Mon Sep 17 00:00:00 2001
From: Tony Lindgren <tony-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org>
Date: Wed, 2 Nov 2016 19:59:05 -0700
Subject: [PATCH] usb: musb: Fix sleeping function called from invalid
 context for hdrc glue

Commit 65b3f50ed6fa ("usb: musb: Add PM runtime support for MUSB DSPS
glue layer") wrongly added a call for pm_runtime_get_sync to otg_timer
that runs in softirq context. That causes a "BUG: sleeping function called
from invalid context" every time when polling the cable status:

[<c015ebb4>] (__might_sleep) from [<c0413d60>] (__pm_runtime_resume+0x9c/0xa0)
[<c0413d60>] (__pm_runtime_resume) from [<c04d0bc4>] (otg_timer+0x3c/0x254)
[<c04d0bc4>] (otg_timer) from [<c0191180>] (call_timer_fn+0xfc/0x41c)
[<c0191180>] (call_timer_fn) from [<c01915c0>] (expire_timers+0x120/0x210)
[<c01915c0>] (expire_timers) from [<c0191acc>] (run_timer_softirq+0xa4/0xdc)
[<c0191acc>] (run_timer_softirq) from [<c010168c>] (__do_softirq+0x12c/0x594)

I did not notice that as I did not have CONFIG_DEBUG_ATOMIC_SLEEP enabled.
And looks like also musb_gadget_queue() suffers from the same problem.

Let's fix the issue by using a list of delayed work then call it on
resume. We can use the existing finish_resume_work for that. Note that
we want to do this only when both musb core and it's parent devices are
awake as noted by Johan Hovold <johan-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>. This allows us also
to get rid of musb_gadget_work and need_finish_resume flag.

Note that we now also need to get rid of static int first as that
won't work right on devices with two musb instances like am335x.

Note that we don't want to mess with deassert_reset_work as that's
more time sensitive and USB spec related instead of PM runtime related.

Fixes: 65b3f50ed6fa ("usb: musb: Add PM runtime support for MUSB DSPS
glue layer")
Reported-by: Johan Hovold <johan-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
Signed-off-by: Tony Lindgren <tony-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org>
---
 drivers/usb/musb/musb_core.c    | 89 +++++++++++++++++++++++++++++++++--------
 drivers/usb/musb/musb_core.h    |  9 ++++-
 drivers/usb/musb/musb_dsps.c    | 24 +++++++----
 drivers/usb/musb/musb_gadget.c  | 31 +++++++++-----
 drivers/usb/musb/musb_host.h    |  6 ++-
 drivers/usb/musb/musb_virthub.c |  5 +--
 6 files changed, 123 insertions(+), 41 deletions(-)

diff --git a/drivers/usb/musb/musb_core.c b/drivers/usb/musb/musb_core.c
--- a/drivers/usb/musb/musb_core.c
+++ b/drivers/usb/musb/musb_core.c
@@ -578,8 +578,9 @@ static irqreturn_t musb_stage0_irq(struct musb *musb, u8 int_usb,
 						| MUSB_PORT_STAT_RESUME;
 				musb->rh_timer = jiffies
 					+ msecs_to_jiffies(USB_RESUME_TIMEOUT);
-				musb->need_finish_resume = 1;
-
+				musb_queue_resume_work(musb,
+						       musb_host_finish_resume,
+						       NULL);
 				musb->xceiv->otg->state = OTG_STATE_A_HOST;
 				musb->is_active = 1;
 				musb_host_resume_root_hub(musb);
@@ -1969,6 +1970,7 @@ static struct musb *allocate_instance(struct device *dev,
 	INIT_LIST_HEAD(&musb->control);
 	INIT_LIST_HEAD(&musb->in_bulk);
 	INIT_LIST_HEAD(&musb->out_bulk);
+	INIT_LIST_HEAD(&musb->pending_list);
 
 	musb->vbuserr_retry = VBUSERR_RETRY_COUNT;
 	musb->a_wait_bcon = OTG_TIME_A_WAIT_BCON;
@@ -2018,6 +2020,64 @@ static void musb_free(struct musb *musb)
 	musb_host_free(musb);
 }
 
+struct musb_pending_work {
+	void (*callback)(struct musb *musb, void *data);
+	void *data;
+	struct list_head node;
+};
+
+static void musb_pending_work(struct work_struct *work)
+{
+	struct musb *musb;
+	struct musb_pending_work *w;
+	unsigned long flags;
+
+	musb = container_of(work, struct musb, finish_resume_work.work);
+	pm_runtime_get_sync(musb->controller);
+	spin_lock_irqsave(&musb->list_lock, flags);
+	while (!list_empty(&musb->pending_list)) {
+		w = list_first_entry(&musb->pending_list,
+				     struct musb_pending_work,
+				     node);
+		list_del(&w->node);
+		spin_unlock_irqrestore(&musb->list_lock, flags);
+		if (w->callback)
+			w->callback(musb, w->data);
+		devm_kfree(musb->controller, w);
+		spin_lock_irqsave(&musb->list_lock, flags);
+	}
+	spin_unlock_irqrestore(&musb->list_lock, flags);
+	pm_runtime_mark_last_busy(musb->controller);
+	pm_runtime_put_autosuspend(musb->controller);
+}
+
+void musb_queue_resume_work(struct musb *musb,
+			    void (*callback)(struct musb *musb, void *data),
+			    void *data)
+{
+	struct musb_pending_work *w;
+	unsigned long flags;
+
+	if (!callback)
+		return;
+
+	w = devm_kzalloc(musb->controller, sizeof(*w), GFP_ATOMIC);
+	if (!w)
+		return;
+
+	w->callback = callback;
+	w->data = data;
+	spin_lock_irqsave(&musb->list_lock, flags);
+	list_add_tail(&w->node, &musb->pending_list);
+	spin_unlock_irqrestore(&musb->list_lock, flags);
+}
+EXPORT_SYMBOL_GPL(musb_queue_resume_work);
+
+void musb_cancel_resume_work(struct musb *musb)
+{
+	cancel_delayed_work_sync(&musb->finish_resume_work);
+}
+
 static void musb_deassert_reset(struct work_struct *work)
 {
 	struct musb *musb;
@@ -2065,6 +2125,7 @@ musb_init_controller(struct device *dev, int nIrq, void __iomem *ctrl)
 	}
 
 	spin_lock_init(&musb->lock);
+	spin_lock_init(&musb->list_lock);
 	musb->board_set_power = plat->set_power;
 	musb->min_power = plat->min_power;
 	musb->ops = plat->platform_ops;
@@ -2215,7 +2276,7 @@ musb_init_controller(struct device *dev, int nIrq, void __iomem *ctrl)
 	/* Init IRQ workqueue before request_irq */
 	INIT_WORK(&musb->irq_work, musb_irq_work);
 	INIT_DELAYED_WORK(&musb->deassert_reset_work, musb_deassert_reset);
-	INIT_DELAYED_WORK(&musb->finish_resume_work, musb_host_finish_resume);
+	INIT_DELAYED_WORK(&musb->finish_resume_work, musb_pending_work);
 
 	/* setup musb parts of the core (especially endpoints) */
 	status = musb_core_init(plat->config->multipoint
@@ -2310,7 +2371,7 @@ musb_init_controller(struct device *dev, int nIrq, void __iomem *ctrl)
 
 fail3:
 	cancel_work_sync(&musb->irq_work);
-	cancel_delayed_work_sync(&musb->finish_resume_work);
+	musb_cancel_resume_work(musb);
 	cancel_delayed_work_sync(&musb->deassert_reset_work);
 	if (musb->dma_controller)
 		musb_dma_controller_destroy(musb->dma_controller);
@@ -2377,7 +2438,7 @@ static int musb_remove(struct platform_device *pdev)
 	musb_exit_debugfs(musb);
 
 	cancel_work_sync(&musb->irq_work);
-	cancel_delayed_work_sync(&musb->finish_resume_work);
+	musb_cancel_resume_work(musb);
 	cancel_delayed_work_sync(&musb->deassert_reset_work);
 	pm_runtime_get_sync(musb->controller);
 	musb_host_cleanup(musb);
@@ -2603,11 +2664,9 @@ static int musb_resume(struct device *dev)
 	mask = MUSB_DEVCTL_BDEVICE | MUSB_DEVCTL_FSDEV | MUSB_DEVCTL_LSDEV;
 	if ((devctl & mask) != (musb->context.devctl & mask))
 		musb->port1_status = 0;
-	if (musb->need_finish_resume) {
-		musb->need_finish_resume = 0;
-		schedule_delayed_work(&musb->finish_resume_work,
-				      msecs_to_jiffies(USB_RESUME_TIMEOUT));
-	}
+
+	schedule_delayed_work(&musb->finish_resume_work,
+			      msecs_to_jiffies(USB_RESUME_TIMEOUT));
 
 	/*
 	 * The USB HUB code expects the device to be in RPM_ACTIVE once it came
@@ -2633,8 +2692,8 @@ static int musb_runtime_suspend(struct device *dev)
 
 static int musb_runtime_resume(struct device *dev)
 {
-	struct musb	*musb = dev_to_musb(dev);
-	static int	first = 1;
+	struct musb *musb = dev_to_musb(dev);
+	struct delayed_work *d = &musb->finish_resume_work;
 
 	/*
 	 * When pm_runtime_get_sync called for the first time in driver
@@ -2645,12 +2704,8 @@ static int musb_runtime_resume(struct device *dev)
 	 * Also context restore without save does not make
 	 * any sense
 	 */
-	if (!first)
+	if (d->work.func) {
 		musb_restore_context(musb);
-	first = 0;
-
-	if (musb->need_finish_resume) {
-		musb->need_finish_resume = 0;
 		schedule_delayed_work(&musb->finish_resume_work,
 				msecs_to_jiffies(USB_RESUME_TIMEOUT));
 	}
diff --git a/drivers/usb/musb/musb_core.h b/drivers/usb/musb/musb_core.h
--- a/drivers/usb/musb/musb_core.h
+++ b/drivers/usb/musb/musb_core.h
@@ -303,6 +303,7 @@ struct musb_context_registers {
 struct musb {
 	/* device lock */
 	spinlock_t		lock;
+	spinlock_t		list_lock;	/* resume work list lock */
 
 	struct musb_io		io;
 	const struct musb_platform_ops *ops;
@@ -312,7 +313,6 @@ struct musb {
 	struct work_struct	irq_work;
 	struct delayed_work	deassert_reset_work;
 	struct delayed_work	finish_resume_work;
-	struct delayed_work	gadget_work;
 	u16			hwvers;
 
 	u16			intrrxe;
@@ -337,6 +337,7 @@ struct musb {
 	struct list_head	control;	/* of musb_qh */
 	struct list_head	in_bulk;	/* of musb_qh */
 	struct list_head	out_bulk;	/* of musb_qh */
+	struct list_head	pending_list;	/* pending work list */
 
 	struct timer_list	otg_timer;
 	struct notifier_block	nb;
@@ -404,7 +405,6 @@ struct musb {
 
 	/* is_suspended means USB B_PERIPHERAL suspend */
 	unsigned		is_suspended:1;
-	unsigned		need_finish_resume :1;
 
 	/* may_wakeup means remote wakeup is enabled */
 	unsigned		may_wakeup:1;
@@ -540,6 +540,11 @@ extern irqreturn_t musb_interrupt(struct musb *);
 
 extern void musb_hnp_stop(struct musb *musb);
 
+void musb_queue_resume_work(struct musb *musb,
+			    void (*callback)(struct musb *musb, void *data),
+			    void *data);
+void musb_cancel_resume_work(struct musb *musb);
+
 static inline void musb_platform_set_vbus(struct musb *musb, int is_on)
 {
 	if (musb->ops->set_vbus)
diff --git a/drivers/usb/musb/musb_dsps.c b/drivers/usb/musb/musb_dsps.c
--- a/drivers/usb/musb/musb_dsps.c
+++ b/drivers/usb/musb/musb_dsps.c
@@ -188,9 +188,8 @@ static void dsps_musb_disable(struct musb *musb)
 	musb_writeb(musb->mregs, MUSB_DEVCTL, 0);
 }
 
-static void otg_timer(unsigned long _musb)
+static void dsps_check_status(struct musb *musb, void *unused)
 {
-	struct musb *musb = (void *)_musb;
 	void __iomem *mregs = musb->mregs;
 	struct device *dev = musb->controller;
 	struct dsps_glue *glue = dev_get_drvdata(dev->parent);
@@ -198,11 +197,6 @@ static void otg_timer(unsigned long _musb)
 	u8 devctl;
 	unsigned long flags;
 	int skip_session = 0;
-	int err;
-
-	err = pm_runtime_get_sync(dev);
-	if (err < 0)
-		dev_err(dev, "Poll could not pm_runtime_get: %i\n", err);
 
 	/*
 	 * We poll because DSPS IP's won't expose several OTG-critical
@@ -246,6 +240,22 @@ static void otg_timer(unsigned long _musb)
 		break;
 	}
 	spin_unlock_irqrestore(&musb->lock, flags);
+}
+
+static void otg_timer(unsigned long _musb)
+{
+	struct musb *musb = (void *)_musb;
+	struct device *dev = musb->controller;
+	int err;
+
+	err = pm_runtime_get(dev);
+	if (err < 0)
+		dev_err(dev, "Poll could not pm_runtime_get: %i\n", err);
+
+	if (pm_runtime_active(dev))
+		dsps_check_status(musb, NULL);
+	else
+		musb_queue_resume_work(musb, dsps_check_status, NULL);
 
 	pm_runtime_mark_last_busy(dev);
 	pm_runtime_put_autosuspend(dev);
diff --git a/drivers/usb/musb/musb_gadget.c b/drivers/usb/musb/musb_gadget.c
--- a/drivers/usb/musb/musb_gadget.c
+++ b/drivers/usb/musb/musb_gadget.c
@@ -1222,6 +1222,16 @@ void musb_ep_restart(struct musb *musb, struct musb_request *req)
 		rxstate(musb, req);
 }
 
+void musb_ep_restart_resume_work(struct musb *musb, void *data)
+{
+	struct musb_request *req = data;
+	unsigned long flags;
+
+	spin_lock_irqsave(&musb->lock, flags);
+	musb_ep_restart(musb, req);
+	spin_unlock_irqrestore(&musb->lock, flags);
+}
+
 static int musb_gadget_queue(struct usb_ep *ep, struct usb_request *req,
 			gfp_t gfp_flags)
 {
@@ -1255,7 +1265,7 @@ static int musb_gadget_queue(struct usb_ep *ep, struct usb_request *req,
 
 	map_dma_buffer(request, musb, musb_ep);
 
-	pm_runtime_get_sync(musb->controller);
+	pm_runtime_get(musb->controller);
 	spin_lock_irqsave(&musb->lock, lockflags);
 
 	/* don't queue if the ep is down */
@@ -1271,8 +1281,14 @@ static int musb_gadget_queue(struct usb_ep *ep, struct usb_request *req,
 	list_add_tail(&request->list, &musb_ep->req_list);
 
 	/* it this is the head of the queue, start i/o ... */
-	if (!musb_ep->busy && &request->list == musb_ep->req_list.next)
-		musb_ep_restart(musb, request);
+	if (!musb_ep->busy && &request->list == musb_ep->req_list.next) {
+		if (pm_runtime_active(musb->controller))
+			musb_ep_restart(musb, request);
+		else
+			musb_queue_resume_work(musb,
+					       musb_ep_restart_resume_work,
+					       request);
+	}
 
 unlock:
 	spin_unlock_irqrestore(&musb->lock, lockflags);
@@ -1650,12 +1666,10 @@ static int musb_gadget_vbus_draw(struct usb_gadget *gadget, unsigned mA)
 	return usb_phy_set_power(musb->xceiv, mA);
 }
 
-static void musb_gadget_work(struct work_struct *work)
+static void musb_gadget_work(struct musb *musb, void *unused)
 {
-	struct musb *musb;
 	unsigned long flags;
 
-	musb = container_of(work, struct musb, gadget_work.work);
 	pm_runtime_get_sync(musb->controller);
 	spin_lock_irqsave(&musb->lock, flags);
 	musb_pullup(musb, musb->softconnect);
@@ -1677,7 +1691,7 @@ static int musb_gadget_pullup(struct usb_gadget *gadget, int is_on)
 	spin_lock_irqsave(&musb->lock, flags);
 	if (is_on != musb->softconnect) {
 		musb->softconnect = is_on;
-		schedule_delayed_work(&musb->gadget_work, 0);
+		musb_queue_resume_work(musb, musb_gadget_work, NULL);
 	}
 	spin_unlock_irqrestore(&musb->lock, flags);
 
@@ -1849,7 +1863,6 @@ int musb_gadget_setup(struct musb *musb)
 #elif IS_ENABLED(CONFIG_USB_MUSB_GADGET)
 	musb->g.is_otg = 0;
 #endif
-	INIT_DELAYED_WORK(&musb->gadget_work, musb_gadget_work);
 	musb_g_init_endpoints(musb);
 
 	musb->is_active = 0;
@@ -1871,7 +1884,7 @@ void musb_gadget_cleanup(struct musb *musb)
 	if (musb->port_mode == MUSB_PORT_MODE_HOST)
 		return;
 
-	cancel_delayed_work_sync(&musb->gadget_work);
+	musb_cancel_resume_work(musb);
 	usb_del_gadget_udc(&musb->g);
 }
 
diff --git a/drivers/usb/musb/musb_host.h b/drivers/usb/musb/musb_host.h
--- a/drivers/usb/musb/musb_host.h
+++ b/drivers/usb/musb/musb_host.h
@@ -94,7 +94,7 @@ extern void musb_host_resume_root_hub(struct musb *musb);
 extern void musb_host_poke_root_hub(struct musb *musb);
 extern void musb_port_suspend(struct musb *musb, bool do_suspend);
 extern void musb_port_reset(struct musb *musb, bool do_reset);
-extern void musb_host_finish_resume(struct work_struct *work);
+void musb_host_finish_resume(struct musb *musb, void *data);
 #else
 static inline struct musb *hcd_to_musb(struct usb_hcd *hcd)
 {
@@ -126,7 +126,9 @@ static inline void musb_host_poll_rh_status(struct musb *musb)	{}
 static inline void musb_host_poke_root_hub(struct musb *musb)	{}
 static inline void musb_port_suspend(struct musb *musb, bool do_suspend) {}
 static inline void musb_port_reset(struct musb *musb, bool do_reset) {}
-static inline void musb_host_finish_resume(struct work_struct *work) {}
+static inline void musb_host_finish_resume(struct musb *musb, void *data)
+{
+}
 #endif
 
 struct usb_hcd;
diff --git a/drivers/usb/musb/musb_virthub.c b/drivers/usb/musb/musb_virthub.c
--- a/drivers/usb/musb/musb_virthub.c
+++ b/drivers/usb/musb/musb_virthub.c
@@ -43,14 +43,11 @@
 
 #include "musb_core.h"
 
-void musb_host_finish_resume(struct work_struct *work)
+void musb_host_finish_resume(struct musb *musb, void *unused)
 {
-	struct musb *musb;
 	unsigned long flags;
 	u8 power;
 
-	musb = container_of(work, struct musb, finish_resume_work.work);

Re: musb RPM sleep-while-atomic in 4.9-rc1

[Ladislav Michl]

On Thu, Nov 03, 2016 at 02:26:35PM -0700, Tony Lindgren wrote:
[snip]
> Here's the patch updated to use the existing finish_resume_work.
> Is that along the lines you were thinking?
> 
> Adding also Ladis to Cc too.

Just gave it a try. Sure it wasn't supposed to fix "musb with hub"
issue, but at least it doesn not make things worse :-)

	ladis
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: musb RPM sleep-while-atomic in 4.9-rc1

[Johan Hovold]

On Thu, Nov 03, 2016 at 02:26:35PM -0700, Tony Lindgren wrote:
> * Johan Hovold <johan-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> [161031 04:50]:
> > On Fri, Oct 28, 2016 at 11:13:19AM -0700, Tony Lindgren wrote:
> > > * Johan Hovold <johan-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> [161028 02:45]:
> > > > On Thu, Oct 27, 2016 at 12:15:52PM -0700, Tony Lindgren wrote:
> > > > > * Johan Hovold <johan-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> [161027 11:46]:
> > > > > > But then this looks like it could trigger an ABBA deadlock as musb->lock
> > > > > > is held while queue_on_resume() takes musb->list_lock, and
> > > > > > musb_run_pending() would take the same locks in the reverse order.
> > > > > 
> > > > > It seems we can avoid that by locking only list_add_tail() and list_del():
> > > > > 
> > > > > list_for_each_entry_safe(w, _w, &musb->resume_work, node) {
> > > > > 	spin_lock_irqsave(&musb->list_lock, flags);
> > > > > 	list_del(&w->node);
> > > > > 	spin_unlock_irqrestore(&musb->list_lock, flags);
> > > > > 	if (w->callback)
> > > > > 		w->callback(musb, w->data);
> > > > > 	devm_kfree(musb->controller, w);
> > > > > }
> > > > 
> > > > I think you still need to hold the lock while traversing the list (even
> > > > if you temporarily release it during the callback).
> > > 
> > > Hmm yeah we need iterate through the list again to avoid missing new
> > > elements being added. I've updated the patch to use a the common
> > > while (!list_empty(&musb->resume_work)) loop. Does that solve the
> > > concern you had or did you also had some other concern there?
> > 
> > Yeah, while that minimises the race window it is still possible that the
> > timer callback checks pm_runtime_active() after the queue has been
> > processed but before the rpm status is updated. 
> 
> OK. Sorry for the delay responding, had my motherboard fail
> over the weekend..

Ouch. Hope the recovery process wasn't too painful.

> > How about using a work struct and synchronous get for the deferred case?
> 
> Here's the patch updated to use the existing finish_resume_work.
> Is that along the lines you were thinking?

Along those lines, yes, but I'm not sure about reusing
finish_resume_work currently used to deassert resume signalling only.

If work is already queued it seems you could end up deasserting resume
prematurely for example. It currently also add unnecessary latency to
other deferred work.

You also forgot to queue musb_host_finish_resume() from
musb_port_suspend() which looks like it would break port resume.

> 8< ----------------------------
> From tony Mon Sep 17 00:00:00 2001
> From: Tony Lindgren <tony-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org>
> Date: Wed, 2 Nov 2016 19:59:05 -0700
> Subject: [PATCH] usb: musb: Fix sleeping function called from invalid
>  context for hdrc glue
> 
> Commit 65b3f50ed6fa ("usb: musb: Add PM runtime support for MUSB DSPS
> glue layer") wrongly added a call for pm_runtime_get_sync to otg_timer
> that runs in softirq context. That causes a "BUG: sleeping function called
> from invalid context" every time when polling the cable status:
> 
> [<c015ebb4>] (__might_sleep) from [<c0413d60>] (__pm_runtime_resume+0x9c/0xa0)
> [<c0413d60>] (__pm_runtime_resume) from [<c04d0bc4>] (otg_timer+0x3c/0x254)
> [<c04d0bc4>] (otg_timer) from [<c0191180>] (call_timer_fn+0xfc/0x41c)
> [<c0191180>] (call_timer_fn) from [<c01915c0>] (expire_timers+0x120/0x210)
> [<c01915c0>] (expire_timers) from [<c0191acc>] (run_timer_softirq+0xa4/0xdc)
> [<c0191acc>] (run_timer_softirq) from [<c010168c>] (__do_softirq+0x12c/0x594)
> 
> I did not notice that as I did not have CONFIG_DEBUG_ATOMIC_SLEEP enabled.
> And looks like also musb_gadget_queue() suffers from the same problem.
> 
> Let's fix the issue by using a list of delayed work then call it on
> resume. We can use the existing finish_resume_work for that. Note that
> we want to do this only when both musb core and it's parent devices are
> awake as noted by Johan Hovold <johan-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>. This allows us also
> to get rid of musb_gadget_work and need_finish_resume flag.
> 
> Note that we now also need to get rid of static int first as that
> won't work right on devices with two musb instances like am335x.
> 
> Note that we don't want to mess with deassert_reset_work as that's
> more time sensitive and USB spec related instead of PM runtime related.
> 
> Fixes: 65b3f50ed6fa ("usb: musb: Add PM runtime support for MUSB DSPS
> glue layer")
> Reported-by: Johan Hovold <johan-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> Signed-off-by: Tony Lindgren <tony-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org>
> ---
>  drivers/usb/musb/musb_core.c    | 89 +++++++++++++++++++++++++++++++++--------
>  drivers/usb/musb/musb_core.h    |  9 ++++-
>  drivers/usb/musb/musb_dsps.c    | 24 +++++++----
>  drivers/usb/musb/musb_gadget.c  | 31 +++++++++-----
>  drivers/usb/musb/musb_host.h    |  6 ++-
>  drivers/usb/musb/musb_virthub.c |  5 +--
>  6 files changed, 123 insertions(+), 41 deletions(-)
> 
> diff --git a/drivers/usb/musb/musb_core.c b/drivers/usb/musb/musb_core.c
> --- a/drivers/usb/musb/musb_core.c
> +++ b/drivers/usb/musb/musb_core.c
> @@ -578,8 +578,9 @@ static irqreturn_t musb_stage0_irq(struct musb *musb, u8 int_usb,
>  						| MUSB_PORT_STAT_RESUME;
>  				musb->rh_timer = jiffies
>  					+ msecs_to_jiffies(USB_RESUME_TIMEOUT);
> -				musb->need_finish_resume = 1;
> -
> +				musb_queue_resume_work(musb,
> +						       musb_host_finish_resume,
> +						       NULL);
>  				musb->xceiv->otg->state = OTG_STATE_A_HOST;
>  				musb->is_active = 1;
>  				musb_host_resume_root_hub(musb);
> @@ -1969,6 +1970,7 @@ static struct musb *allocate_instance(struct device *dev,
>  	INIT_LIST_HEAD(&musb->control);
>  	INIT_LIST_HEAD(&musb->in_bulk);
>  	INIT_LIST_HEAD(&musb->out_bulk);
> +	INIT_LIST_HEAD(&musb->pending_list);
>  
>  	musb->vbuserr_retry = VBUSERR_RETRY_COUNT;
>  	musb->a_wait_bcon = OTG_TIME_A_WAIT_BCON;
> @@ -2018,6 +2020,64 @@ static void musb_free(struct musb *musb)
>  	musb_host_free(musb);
>  }
>  
> +struct musb_pending_work {
> +	void (*callback)(struct musb *musb, void *data);
> +	void *data;
> +	struct list_head node;
> +};
> +
> +static void musb_pending_work(struct work_struct *work)
> +{
> +	struct musb *musb;
> +	struct musb_pending_work *w;
> +	unsigned long flags;
> +
> +	musb = container_of(work, struct musb, finish_resume_work.work);
> +	pm_runtime_get_sync(musb->controller);

Should still check for errors here.

> +	spin_lock_irqsave(&musb->list_lock, flags);
> +	while (!list_empty(&musb->pending_list)) {
> +		w = list_first_entry(&musb->pending_list,
> +				     struct musb_pending_work,
> +				     node);
> +		list_del(&w->node);
> +		spin_unlock_irqrestore(&musb->list_lock, flags);
> +		if (w->callback)
> +			w->callback(musb, w->data);
> +		devm_kfree(musb->controller, w);
> +		spin_lock_irqsave(&musb->list_lock, flags);
> +	}
> +	spin_unlock_irqrestore(&musb->list_lock, flags);
> +	pm_runtime_mark_last_busy(musb->controller);
> +	pm_runtime_put_autosuspend(musb->controller);
> +}
> +
> +void musb_queue_resume_work(struct musb *musb,
> +			    void (*callback)(struct musb *musb, void *data),
> +			    void *data)
> +{
> +	struct musb_pending_work *w;
> +	unsigned long flags;
> +
> +	if (!callback)
> +		return;

WARN_ON (e.g. in case someone switches the arguments)?

> +
> +	w = devm_kzalloc(musb->controller, sizeof(*w), GFP_ATOMIC);
> +	if (!w)
> +		return;
> +
> +	w->callback = callback;
> +	w->data = data;
> +	spin_lock_irqsave(&musb->list_lock, flags);
> +	list_add_tail(&w->node, &musb->pending_list);
> +	spin_unlock_irqrestore(&musb->list_lock, flags);
> +}
> +EXPORT_SYMBOL_GPL(musb_queue_resume_work);
> +
> +void musb_cancel_resume_work(struct musb *musb)
> +{
> +	cancel_delayed_work_sync(&musb->finish_resume_work);
> +}
> +
>  static void musb_deassert_reset(struct work_struct *work)
>  {
>  	struct musb *musb;
> @@ -2065,6 +2125,7 @@ musb_init_controller(struct device *dev, int nIrq, void __iomem *ctrl)
>  	}
>  
>  	spin_lock_init(&musb->lock);
> +	spin_lock_init(&musb->list_lock);
>  	musb->board_set_power = plat->set_power;
>  	musb->min_power = plat->min_power;
>  	musb->ops = plat->platform_ops;
> @@ -2215,7 +2276,7 @@ musb_init_controller(struct device *dev, int nIrq, void __iomem *ctrl)
>  	/* Init IRQ workqueue before request_irq */
>  	INIT_WORK(&musb->irq_work, musb_irq_work);
>  	INIT_DELAYED_WORK(&musb->deassert_reset_work, musb_deassert_reset);
> -	INIT_DELAYED_WORK(&musb->finish_resume_work, musb_host_finish_resume);
> +	INIT_DELAYED_WORK(&musb->finish_resume_work, musb_pending_work);
>  
>  	/* setup musb parts of the core (especially endpoints) */
>  	status = musb_core_init(plat->config->multipoint
> @@ -2310,7 +2371,7 @@ musb_init_controller(struct device *dev, int nIrq, void __iomem *ctrl)
>  
>  fail3:
>  	cancel_work_sync(&musb->irq_work);
> -	cancel_delayed_work_sync(&musb->finish_resume_work);
> +	musb_cancel_resume_work(musb);
>  	cancel_delayed_work_sync(&musb->deassert_reset_work);
>  	if (musb->dma_controller)
>  		musb_dma_controller_destroy(musb->dma_controller);
> @@ -2377,7 +2438,7 @@ static int musb_remove(struct platform_device *pdev)
>  	musb_exit_debugfs(musb);
>  
>  	cancel_work_sync(&musb->irq_work);
> -	cancel_delayed_work_sync(&musb->finish_resume_work);
> +	musb_cancel_resume_work(musb);
>  	cancel_delayed_work_sync(&musb->deassert_reset_work);
>  	pm_runtime_get_sync(musb->controller);
>  	musb_host_cleanup(musb);
> @@ -2603,11 +2664,9 @@ static int musb_resume(struct device *dev)
>  	mask = MUSB_DEVCTL_BDEVICE | MUSB_DEVCTL_FSDEV | MUSB_DEVCTL_LSDEV;
>  	if ((devctl & mask) != (musb->context.devctl & mask))
>  		musb->port1_status = 0;
> -	if (musb->need_finish_resume) {
> -		musb->need_finish_resume = 0;
> -		schedule_delayed_work(&musb->finish_resume_work,
> -				      msecs_to_jiffies(USB_RESUME_TIMEOUT));
> -	}
> +
> +	schedule_delayed_work(&musb->finish_resume_work,
> +			      msecs_to_jiffies(USB_RESUME_TIMEOUT));
>  
>  	/*
>  	 * The USB HUB code expects the device to be in RPM_ACTIVE once it came
> @@ -2633,8 +2692,8 @@ static int musb_runtime_suspend(struct device *dev)
>  
>  static int musb_runtime_resume(struct device *dev)
>  {
> -	struct musb	*musb = dev_to_musb(dev);
> -	static int	first = 1;
> +	struct musb *musb = dev_to_musb(dev);
> +	struct delayed_work *d = &musb->finish_resume_work;
>  
>  	/*
>  	 * When pm_runtime_get_sync called for the first time in driver
> @@ -2645,12 +2704,8 @@ static int musb_runtime_resume(struct device *dev)
>  	 * Also context restore without save does not make
>  	 * any sense
>  	 */

Perhaps using a dedicated initialised flag in struct musb would be more
self-explanatory (and less error prone) than overloading
finish_resume_work->work.func. Could be done in a preparatory patch.

Otherwise the above comment should probably be updated and mention this
use of d->work.func.

> -	if (!first)
> +	if (d->work.func) {
>  		musb_restore_context(musb);
> -	first = 0;
> -
> -	if (musb->need_finish_resume) {
> -		musb->need_finish_resume = 0;
>  		schedule_delayed_work(&musb->finish_resume_work,

Use d as first argument here?

>  				msecs_to_jiffies(USB_RESUME_TIMEOUT));

Perhaps sleep in musb_host_finish_resume() instead of always adding this
delay which is only needed for deasserting resume signalling and not for
generic (runtime) resume work (if you decide to use a common work
queue).

But why schedule from resume at all? It seems you could just schedule
the deferred-work processing directly after having registered a
callback.

> -static void musb_gadget_work(struct work_struct *work)
> +static void musb_gadget_work(struct musb *musb, void *unused)
>  {
> -	struct musb *musb;
>  	unsigned long flags;
>  
> -	musb = container_of(work, struct musb, gadget_work.work);
>  	pm_runtime_get_sync(musb->controller);

The get would already have been taken care of by musb_pending_work().

>  	spin_lock_irqsave(&musb->lock, flags);
>  	musb_pullup(musb, musb->softconnect);
> @@ -1677,7 +1691,7 @@ static int musb_gadget_pullup(struct usb_gadget *gadget, int is_on)
>  	spin_lock_irqsave(&musb->lock, flags);
>  	if (is_on != musb->softconnect) {
>  		musb->softconnect = is_on;
> -		schedule_delayed_work(&musb->gadget_work, 0);
> +		musb_queue_resume_work(musb, musb_gadget_work, NULL);

You also need a pm_runtime_get() here to actually trigger the resume.

But if already resumed, the gadget work would not run until next
suspend-resume cycle. So if you want to reuse the resume work for this,
then you need to check the active status here and do the work directly
if already resumed as for musb_gadget_queue(). Or just always schedule
deferred work as mentioned above.

>  	}
>  	spin_unlock_irqrestore(&musb->lock, flags);
>  
> @@ -1849,7 +1863,6 @@ int musb_gadget_setup(struct musb *musb)
>  #elif IS_ENABLED(CONFIG_USB_MUSB_GADGET)
>  	musb->g.is_otg = 0;
>  #endif
> -	INIT_DELAYED_WORK(&musb->gadget_work, musb_gadget_work);
>  	musb_g_init_endpoints(musb);
>  
>  	musb->is_active = 0;
> @@ -1871,7 +1884,7 @@ void musb_gadget_cleanup(struct musb *musb)
>  	if (musb->port_mode == MUSB_PORT_MODE_HOST)
>  		return;
>  
> -	cancel_delayed_work_sync(&musb->gadget_work);
> +	musb_cancel_resume_work(musb);

Doing this here as well, looks a bit weird but I guess it is needed for
the probe error path where musb_gadget_cleanup() is called before
cancelling the pending work (remove() would not need it, though).

>  	usb_del_gadget_udc(&musb->g);
>  }

Thanks,
Johan
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: musb RPM sleep-while-atomic in 4.9-rc1

[Tony Lindgren]

* Johan Hovold <johan-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> [161104 07:16]:
> On Thu, Nov 03, 2016 at 02:26:35PM -0700, Tony Lindgren wrote:
> > OK. Sorry for the delay responding, had my motherboard fail
> > over the weekend..
> 
> Ouch. Hope the recovery process wasn't too painful.

Only the usual scratches on my knuckles :)

> > > How about using a work struct and synchronous get for the deferred case?
> > 
> > Here's the patch updated to use the existing finish_resume_work.
> > Is that along the lines you were thinking?
> 
> Along those lines, yes, but I'm not sure about reusing
> finish_resume_work currently used to deassert resume signalling only.

OK I'll drop the changes for the other ones. Those can be
then combined later as needed once we're done with fixes.

> If work is already queued it seems you could end up deasserting resume
> prematurely for example. It currently also add unnecessary latency to
> other deferred work.

Yeah good point so best to keep this one strictly limited to
resume work.

> You also forgot to queue musb_host_finish_resume() from
> musb_port_suspend() which looks like it would break port resume.

Oh OK will take a look. Might be best to leave that one out
that change for now.

> > +	struct musb_pending_work *w;
> > +	unsigned long flags;
> > +
> > +	if (!callback)
> > +		return;
> 
> WARN_ON (e.g. in case someone switches the arguments)?

Sure. Will fix also your other comments and make it into a
more minimal patch for fixes.

Regards,

Tony
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: musb RPM sleep-while-atomic in 4.9-rc1

[Tony Lindgren]

* Johan Hovold <johan-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> [161104 07:16]:
> Perhaps sleep in musb_host_finish_resume() instead of always adding this
> delay which is only needed for deasserting resume signalling and not for
> generic (runtime) resume work (if you decide to use a common work
> queue).

Looking at your comments again, this is a good idea for
future use.

> But why schedule from resume at all? It seems you could just schedule
> the deferred-work processing directly after having registered a
> callback.

But with this one we need to schedule the deferred work only
from resume as waking up things can take a while potentially.

Anyways, I'll post all the pending fixes as a new thread shortly.

Regards,

Tony
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html