From mboxrd@z Thu Jan 1 00:00:00 1970 From: Kevin Hilman Subject: Re: [PATCH 5/5 v4] mfd: omap: usb: Runtime PM support Date: Mon, 29 Aug 2011 11:36:34 -0700 Message-ID: <874o10t68d.fsf@ti.com> References: <1312899309-24067-1-git-send-email-keshava_mgowda@ti.com> <1312899309-24067-2-git-send-email-keshava_mgowda@ti.com> <1312899309-24067-3-git-send-email-keshava_mgowda@ti.com> <1312899309-24067-4-git-send-email-keshava_mgowda@ti.com> <1312899309-24067-5-git-send-email-keshava_mgowda@ti.com> <1312899309-24067-6-git-send-email-keshava_mgowda@ti.com> <20110810163142.GA10156@google.com> <20110812213037.GA24803@google.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: QUOTED-PRINTABLE Return-path: In-Reply-To: <20110812213037.GA24803-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org> (Todd Poynor's message of "Fri, 12 Aug 2011 14:30:37 -0700") Sender: linux-usb-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org To: Todd Poynor Cc: "Munegowda, Keshava" , linux-usb-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, linux-omap-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, balbi-l0cyMroinI0@public.gmane.org, gadiyar-l0cyMroinI0@public.gmane.org, sameo-VuQAYsv1563Yd54FQh9/CA@public.gmane.org, parthab-PpE0FKYn9XJWk0Htik3J/w@public.gmane.org, tony-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org, b-cousson-l0cyMroinI0@public.gmane.org, paul-DWxLp4Yu+b8AvxtiuMwx3w@public.gmane.org, johnstul-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org, vishwanath.bs-l0cyMroinI0@public.gmane.org, nm-l0cyMroinI0@public.gmane.org, vikram.pandita-l0cyMroinI0@public.gmane.org List-Id: linux-omap@vger.kernel.org Todd Poynor writes: > On Fri, Aug 12, 2011 at 12:20:21PM +0530, Munegowda, Keshava wrote: >> On Wed, Aug 10, 2011 at 10:01 PM, Todd Poynor wrote: >> >> @@ -913,12 +598,15 @@ static int usbhs_enable(struct device *dev) >> >> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 (pdata->ehci_data->reset_gpio_po= rt[1], 1); >> >> =C2=A0 =C2=A0 =C2=A0 } >> >> >> >> -end_count: >> >> - =C2=A0 =C2=A0 omap->count++; >> >> + =C2=A0 =C2=A0 pm_runtime_put_sync(dev); >> >> =C2=A0 =C2=A0 =C2=A0 spin_unlock_irqrestore(&omap->lock, flags); >> > >> > Is pm_runtime_irq_safe() needed (else I think runtime PM callbacks= may >> > re-enable IRQs... or there's the new *_suspend runtime PM calls th= at >> > may avoid this)? >>=20 >> pm_runtime_irq_safe() is not required; usbhs does not have a paren= t >> and it is the parent driver of >> ehci and ohci drivers. > > But the above expects IRQs to be disabled during the > pm_runtime_put_sync, and synchronous calls can turn IRQs back on in > rpm_idle: > > if (callback) { > spin_unlock_irq(&dev->power.lock); > > callback(dev); > > I see other folks who know this better than me are discussing USB run > time PM and might_sleep contexts, so I'll note this concern and let > others chime in if they think there's a real problem here. =46YI... The commit below fixes this mainline (merged as of v3.1-rc4). Kevin commit 02b26774afebb2d62695ba3230319d70d8c6cc2d Author: Kevin Hilman Date: Fri Aug 5 21:45:20 2011 +0200 PM / Runtime: Allow _put_sync() from interrupts-disabled context =20 Currently the use of pm_runtime_put_sync() is not safe from interrupts-disabled context because rpm_idle() will release the spinlock and enable interrupts for the idle callbacks. This enable= s interrupts during a time where interrupts were expected to be disabled, and can have strange side effects on drivers that expecte= d interrupts to be disabled. =20 This is not a bug since the documentation clearly states that only _put_sync_suspend() is safe in IRQ-safe mode. =20 However, pm_runtime_put_sync() could be made safe when in IRQ-safe mode by releasing the spinlock but not re-enabling interrupts, whic= h is what this patch aims to do. =20 Problem was found when using some buggy drivers that set pm_runtime_irq_safe() and used _put_sync() in interrupts-disabled context. =20 Reported-by: Colin Cross Tested-by: Nishanth Menon Signed-off-by: Kevin Hilman Signed-off-by: Rafael J. Wysocki -- 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 From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754801Ab1H2Sgm (ORCPT ); Mon, 29 Aug 2011 14:36:42 -0400 Received: from na3sys009aog120.obsmtp.com ([74.125.149.140]:49553 "EHLO na3sys009aog120.obsmtp.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754379Ab1H2Sgj convert rfc822-to-8bit (ORCPT ); Mon, 29 Aug 2011 14:36:39 -0400 From: Kevin Hilman To: Todd Poynor Cc: "Munegowda\, Keshava" , linux-usb@vger.kernel.org, linux-omap@vger.kernel.org, linux-kernel@vger.kernel.org, balbi@ti.com, gadiyar@ti.com, sameo@linux.intel.com, parthab@india.ti.com, tony@atomide.com, b-cousson@ti.com, paul@pwsan.com, johnstul@us.ibm.com, vishwanath.bs@ti.com, nm@ti.com, vikram.pandita@ti.com Subject: Re: [PATCH 5/5 v4] mfd: omap: usb: Runtime PM support Organization: Texas Instruments, Inc. References: <1312899309-24067-1-git-send-email-keshava_mgowda@ti.com> <1312899309-24067-2-git-send-email-keshava_mgowda@ti.com> <1312899309-24067-3-git-send-email-keshava_mgowda@ti.com> <1312899309-24067-4-git-send-email-keshava_mgowda@ti.com> <1312899309-24067-5-git-send-email-keshava_mgowda@ti.com> <1312899309-24067-6-git-send-email-keshava_mgowda@ti.com> <20110810163142.GA10156@google.com> <20110812213037.GA24803@google.com> Date: Mon, 29 Aug 2011 11:36:34 -0700 In-Reply-To: <20110812213037.GA24803@google.com> (Todd Poynor's message of "Fri, 12 Aug 2011 14:30:37 -0700") Message-ID: <874o10t68d.fsf@ti.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/23.1.50 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8BIT Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Todd Poynor writes: > On Fri, Aug 12, 2011 at 12:20:21PM +0530, Munegowda, Keshava wrote: >> On Wed, Aug 10, 2011 at 10:01 PM, Todd Poynor wrote: >> >> @@ -913,12 +598,15 @@ static int usbhs_enable(struct device *dev) >> >>                               (pdata->ehci_data->reset_gpio_port[1], 1); >> >>       } >> >> >> >> -end_count: >> >> -     omap->count++; >> >> +     pm_runtime_put_sync(dev); >> >>       spin_unlock_irqrestore(&omap->lock, flags); >> > >> > Is pm_runtime_irq_safe() needed (else I think runtime PM callbacks may >> > re-enable IRQs... or there's the new *_suspend runtime PM calls that >> > may avoid this)? >> >> pm_runtime_irq_safe() is not required; usbhs does not have a parent >> and it is the parent driver of >> ehci and ohci drivers. > > But the above expects IRQs to be disabled during the > pm_runtime_put_sync, and synchronous calls can turn IRQs back on in > rpm_idle: > > if (callback) { > spin_unlock_irq(&dev->power.lock); > > callback(dev); > > I see other folks who know this better than me are discussing USB run > time PM and might_sleep contexts, so I'll note this concern and let > others chime in if they think there's a real problem here. FYI... The commit below fixes this mainline (merged as of v3.1-rc4). Kevin commit 02b26774afebb2d62695ba3230319d70d8c6cc2d Author: Kevin Hilman Date: Fri Aug 5 21:45:20 2011 +0200 PM / Runtime: Allow _put_sync() from interrupts-disabled context Currently the use of pm_runtime_put_sync() is not safe from interrupts-disabled context because rpm_idle() will release the spinlock and enable interrupts for the idle callbacks. This enables interrupts during a time where interrupts were expected to be disabled, and can have strange side effects on drivers that expected interrupts to be disabled. This is not a bug since the documentation clearly states that only _put_sync_suspend() is safe in IRQ-safe mode. However, pm_runtime_put_sync() could be made safe when in IRQ-safe mode by releasing the spinlock but not re-enabling interrupts, which is what this patch aims to do. Problem was found when using some buggy drivers that set pm_runtime_irq_safe() and used _put_sync() in interrupts-disabled context. Reported-by: Colin Cross Tested-by: Nishanth Menon Signed-off-by: Kevin Hilman Signed-off-by: Rafael J. Wysocki