From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S941678AbcIHL6E (ORCPT ); Thu, 8 Sep 2016 07:58:04 -0400 Received: from mx1.redhat.com ([209.132.183.28]:56590 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932308AbcIHL6B (ORCPT ); Thu, 8 Sep 2016 07:58:01 -0400 Subject: Re: [PATCH] phy: sun4i-usb: Use spinlock to guard phyctl register access To: Chen-Yu Tsai References: <20160908031418.12627-1-wens@csie.org> <883333c3-5685-905f-3e96-2c2498a22b8a@redhat.com> Cc: Kishon Vijay Abraham I , Maxime Ripard , linux-arm-kernel , linux-kernel , stable@vger.kernel.org From: Hans de Goede Message-ID: Date: Thu, 8 Sep 2016 13:57:56 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.2.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.27]); Thu, 08 Sep 2016 11:58:01 +0000 (UTC) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi, On 08-09-16 12:24, Chen-Yu Tsai wrote: > On Thu, Sep 8, 2016 at 6:16 PM, Hans de Goede wrote: >> Hi, >> >> >> On 08-09-16 05:14, Chen-Yu Tsai wrote: >>> >>> The musb driver calls into this phy driver to disable/enable squelch >>> detection. This function was introduced in 24fe86a617c5 ("phy: sun4i-usb: >>> Add a sunxi specific function for setting squelch-detect"). This >>> function in turn calls sun4i_usb_phy_write, which uses a mutex to >>> guard the common access register. Unfortunately musb does this >>> in atomic context, which results in the following warning with lock >>> debugging enabled: >>> >>> BUG: sleeping function called from invalid context at >>> kernel/locking/mutex.c:97 >>> in_atomic(): 1, irqs_disabled(): 128, pid: 96, name: kworker/0:2 >>> CPU: 0 PID: 96 Comm: kworker/0:2 Not tainted 4.8.0-rc4-00181-gd502f8ad1c3e >>> #13 >>> Hardware name: Allwinner sun8i Family >>> Workqueue: events musb_deassert_reset >>> [] (unwind_backtrace) from [] (show_stack+0xb/0xc) >>> [] (show_stack) from [] (dump_stack+0x67/0x74) >>> [] (dump_stack) from [] (mutex_lock+0x15/0x2c) >>> [] (mutex_lock) from [] >>> (sun4i_usb_phy_write+0x39/0xec) >>> [] (sun4i_usb_phy_write) from [] >>> (musb_port_reset+0xfb/0x184) >>> [] (musb_port_reset) from [] >>> (musb_deassert_reset+0x1f/0x2c) >>> [] (musb_deassert_reset) from [] >>> (process_one_work+0x129/0x2b8) >>> [] (process_one_work) from [] >>> (worker_thread+0xf3/0x424) >>> [] (worker_thread) from [] (kthread+0xa1/0xb8) >>> [] (kthread) from [] (ret_from_fork+0x11/0x20) >>> >>> Since the register access is mmio, we can use a spinlock to guard this >>> specific access, rather than the mutex that guards the entire phy. >>> >>> Fixes: ba4bdc9e1dc0 ("PHY: sunxi: Add driver for sunxi usb phy") >>> Cc: Hans de Goede >>> Cc: stable@vger.kernel.org >>> Signed-off-by: Chen-Yu Tsai >> >> >> Good catch, but you're actually replacing the locking calls in the only >> user of sun4i_usb_phy_data.mutex, so after your patch it is no longer >> used anywhere. Can you do a v2 just outright replacing it with a >> spinlock please ? > > Ah. I misread the other use of mutex_lock, which is actually using > the mutex in struct phy. Sure I'll just replace it. > > After sending this patch I wondered if I should have used > spin_lock_irqsave though. Good point, yes please use spin_lock_irqsave, I believe this code path may get called outside of irq handling so using spin_lock_irqsave is the right thing todo. Regards, Hans > > ChenYu > >> >> Thanks, >> >> Hans >> >> >> >> >>> --- >>> drivers/phy/phy-sun4i-usb.c | 8 ++++++-- >>> 1 file changed, 6 insertions(+), 2 deletions(-) >>> >>> diff --git a/drivers/phy/phy-sun4i-usb.c b/drivers/phy/phy-sun4i-usb.c >>> index fcf4d95ecc6d..ae4ac5457c64 100644 >>> --- a/drivers/phy/phy-sun4i-usb.c >>> +++ b/drivers/phy/phy-sun4i-usb.c >>> @@ -40,6 +40,7 @@ >>> #include >>> #include >>> #include >>> +#include >>> #include >>> #include >>> >>> @@ -115,6 +116,7 @@ struct sun4i_usb_phy_data { >>> const struct sun4i_usb_phy_cfg *cfg; >>> enum usb_dr_mode dr_mode; >>> struct mutex mutex; >>> + spinlock_t reg_lock; /* guard access to phyctl reg */ >>> struct sun4i_usb_phy { >>> struct phy *phy; >>> void __iomem *pmu; >>> @@ -183,7 +185,7 @@ static void sun4i_usb_phy_write(struct sun4i_usb_phy >>> *phy, u32 addr, u32 data, >>> void __iomem *phyctl = phy_data->base + >>> phy_data->cfg->phyctl_offset; >>> int i; >>> >>> - mutex_lock(&phy_data->mutex); >>> + spin_lock(&phy_data->reg_lock); >>> >>> if (phy_data->cfg->type == sun8i_a33_phy || >>> phy_data->cfg->type == sun50i_a64_phy) { >>> @@ -221,7 +223,8 @@ static void sun4i_usb_phy_write(struct sun4i_usb_phy >>> *phy, u32 addr, u32 data, >>> >>> data >>= 1; >>> } >>> - mutex_unlock(&phy_data->mutex); >>> + >>> + spin_unlock(&phy_data->reg_lock); >>> } >>> >>> static void sun4i_usb_phy_passby(struct sun4i_usb_phy *phy, int enable) >>> @@ -583,6 +586,7 @@ static int sun4i_usb_phy_probe(struct platform_device >>> *pdev) >>> return -ENOMEM; >>> >>> mutex_init(&data->mutex); >>> + spin_lock_init(&data->reg_lock); >>> INIT_DELAYED_WORK(&data->detect, sun4i_usb_phy0_id_vbus_det_scan); >>> dev_set_drvdata(dev, data); >>> data->cfg = of_device_get_match_data(dev); >>> >>