From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753481AbcIIJn3 (ORCPT ); Fri, 9 Sep 2016 05:43:29 -0400 Received: from mx1.redhat.com ([209.132.183.28]:60812 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751091AbcIIJn0 (ORCPT ); Fri, 9 Sep 2016 05:43:26 -0400 Subject: Re: [PATCH v2] phy: sun4i-usb: Use spinlock to guard phyctl register access To: Chen-Yu Tsai , Kishon Vijay Abraham I , Maxime Ripard References: <20160909035818.16273-1-wens@csie.org> Cc: linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, stable@vger.kernel.org From: Hans de Goede Message-ID: <1db79dae-50d7-4b01-caec-d7415b9a0986@redhat.com> Date: Fri, 9 Sep 2016 11:43:22 +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: <20160909035818.16273-1-wens@csie.org> Content-Type: text/plain; charset=windows-1252; 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.29]); Fri, 09 Sep 2016 09:43:26 +0000 (UTC) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi, On 09-09-16 05:58, 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 Looks good to me: Reviewed-by: Hans de Goede Regards, Hans > --- > > Changes since v1: > > - Remove the old mutex, since it is no longer used > - Use irqsave/irqrestore variant of spinlock ops > > --- > drivers/phy/phy-sun4i-usb.c | 11 +++++++---- > 1 file changed, 7 insertions(+), 4 deletions(-) > > diff --git a/drivers/phy/phy-sun4i-usb.c b/drivers/phy/phy-sun4i-usb.c > index fcf4d95ecc6d..34b7f0c2e019 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 > > @@ -114,7 +115,7 @@ struct sun4i_usb_phy_data { > void __iomem *base; > 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; > @@ -181,9 +182,10 @@ static void sun4i_usb_phy_write(struct sun4i_usb_phy *phy, u32 addr, u32 data, > struct sun4i_usb_phy_data *phy_data = to_sun4i_usb_phy_data(phy); > u32 temp, usbc_bit = BIT(phy->index * 2); > void __iomem *phyctl = phy_data->base + phy_data->cfg->phyctl_offset; > + unsigned long flags; > int i; > > - mutex_lock(&phy_data->mutex); > + spin_lock_irqsave(&phy_data->reg_lock, flags); > > 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_irqrestore(&phy_data->reg_lock, flags); > } > > static void sun4i_usb_phy_passby(struct sun4i_usb_phy *phy, int enable) > @@ -582,7 +585,7 @@ static int sun4i_usb_phy_probe(struct platform_device *pdev) > if (!data) > 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); >