Message ID | 1455574327-2591-1-git-send-email-Larry.Finger@lwfinger.net (mailing list archive) |
---|---|
State | Accepted |
Delegated to: | Kalle Valo |
Headers | show |
On Tue, Feb 16, 2016 at 3:42 AM, Larry Finger <Larry.Finger@lwfinger.net> wrote: > Routine rtl_addr_delay() uses delay statements in code that can > sleep. To improve system responsiveness, the various delay statements > are changed. > > In addition, routines rtl_rfreg_delay() and rtl_bb_delay() are > rewritten to use the code in rtl_addr_delay() for most of their > input values. > > Suggested-by: Byeoungwook Kim <quddnr145@gmail.com> > Signed-off-by: Larry Finger <Larry.Finger@lwfinger.net> > --- > > Kalle, > > This patch will interfere with a set of 3 patches submitted by Byeoungwook Kim > <quddnr145@gmail.com> on Feb. 3, 2016 that are currently in the deferred list > at patchwork. > > Larry > > drivers/net/wireless/realtek/rtlwifi/core.c | 44 ++++++++--------------------- > 1 file changed, 12 insertions(+), 32 deletions(-) > > diff --git a/drivers/net/wireless/realtek/rtlwifi/core.c b/drivers/net/wireless/realtek/rtlwifi/core.c > index 02eba0e..16ad0d6 100644 > --- a/drivers/net/wireless/realtek/rtlwifi/core.c > +++ b/drivers/net/wireless/realtek/rtlwifi/core.c > @@ -54,59 +54,39 @@ EXPORT_SYMBOL(channel5g_80m); > void rtl_addr_delay(u32 addr) > { > if (addr == 0xfe) > - mdelay(50); > + msleep(50); > else if (addr == 0xfd) > - mdelay(5); > + msleep(5); > else if (addr == 0xfc) > - mdelay(1); > + msleep(1); > else if (addr == 0xfb) > - udelay(50); > + usleep_range(50, 100); > else if (addr == 0xfa) > - udelay(5); > + usleep_range(5, 10); > else if (addr == 0xf9) > - udelay(1); > + usleep_range(1, 2); why udelay is replaced by usleep_range? > } > EXPORT_SYMBOL(rtl_addr_delay); > > void rtl_rfreg_delay(struct ieee80211_hw *hw, enum radio_path rfpath, u32 addr, > u32 mask, u32 data) > { > - if (addr == 0xfe) { > - mdelay(50); > - } else if (addr == 0xfd) { > - mdelay(5); > - } else if (addr == 0xfc) { > - mdelay(1); > - } else if (addr == 0xfb) { > - udelay(50); > - } else if (addr == 0xfa) { > - udelay(5); > - } else if (addr == 0xf9) { > - udelay(1); > + if (addr >= 0xf9 && addr <= 0xfe) { > + rtl_addr_delay(addr); > } else { > rtl_set_rfreg(hw, rfpath, addr, mask, data); > - udelay(1); > + usleep_range(1, 2); > } > } > EXPORT_SYMBOL(rtl_rfreg_delay); > > void rtl_bb_delay(struct ieee80211_hw *hw, u32 addr, u32 data) > { > - if (addr == 0xfe) { > - mdelay(50); > - } else if (addr == 0xfd) { > - mdelay(5); > - } else if (addr == 0xfc) { > - mdelay(1); > - } else if (addr == 0xfb) { > - udelay(50); > - } else if (addr == 0xfa) { > - udelay(5); > - } else if (addr == 0xf9) { > - udelay(1); > + if (addr >= 0xf9 && addr <= 0xfe) { > + rtl_addr_delay(addr); > } else { > rtl_set_bbreg(hw, addr, MASKDWORD, data); > - udelay(1); > + usleep_range(1, 2); > } > } > EXPORT_SYMBOL(rtl_bb_delay); > -- > 2.1.4 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-wireless" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -Souptick -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 02/16/2016 12:17 AM, Souptick Joarder wrote: > On Tue, Feb 16, 2016 at 3:42 AM, Larry Finger <Larry.Finger@lwfinger.net> wrote: --snip-- >> else if (addr == 0xf9) >> - udelay(1); >> + usleep_range(1, 2); > > why udelay is replaced by usleep_range? I'm not sure of your level of sophistication, but here goes. All delay statements cause a processor to stay in control and make the system wait for that amount of time. A sleep statement allows a context switch, and the processor is able to run some other job. For that reason, sleeps are always preferred over delays as long as the code is not running in atomic context. There used to be a usleep() function, but as the system cannot promise to return from sleep after a specific delay, it was replaced with usleep_range(). It is true that the difference between delaying and sleeping for 1 usec would not be too destructive to the system, but I decided to convert every branch of that if structure to sleep statements. Larry -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
2016-02-16 7:12 GMT+09:00 Larry Finger <Larry.Finger@lwfinger.net>: > Routine rtl_addr_delay() uses delay statements in code that can > sleep. To improve system responsiveness, the various delay statements > are changed. > > In addition, routines rtl_rfreg_delay() and rtl_bb_delay() are > rewritten to use the code in rtl_addr_delay() for most of their > input values. > > Suggested-by: Byeoungwook Kim <quddnr145@gmail.com> > Signed-off-by: Larry Finger <Larry.Finger@lwfinger.net> > --- Why you are using 'Suggested-by:' of me? I think to that your commit was included a part of my commit(https://patchwork.kernel.org/patch/8197071/). > > Kalle, > > This patch will interfere with a set of 3 patches submitted by Byeoungwook Kim > <quddnr145@gmail.com> on Feb. 3, 2016 that are currently in the deferred list > at patchwork. > > Larry > > drivers/net/wireless/realtek/rtlwifi/core.c | 44 ++++++++--------------------- > 1 file changed, 12 insertions(+), 32 deletions(-) > > diff --git a/drivers/net/wireless/realtek/rtlwifi/core.c b/drivers/net/wireless/realtek/rtlwifi/core.c > index 02eba0e..16ad0d6 100644 > --- a/drivers/net/wireless/realtek/rtlwifi/core.c > +++ b/drivers/net/wireless/realtek/rtlwifi/core.c > @@ -54,59 +54,39 @@ EXPORT_SYMBOL(channel5g_80m); > void rtl_addr_delay(u32 addr) > { > if (addr == 0xfe) > - mdelay(50); > + msleep(50); > else if (addr == 0xfd) > - mdelay(5); > + msleep(5); > else if (addr == 0xfc) > - mdelay(1); > + msleep(1); > else if (addr == 0xfb) > - udelay(50); > + usleep_range(50, 100); > else if (addr == 0xfa) > - udelay(5); > + usleep_range(5, 10); > else if (addr == 0xf9) > - udelay(1); > + usleep_range(1, 2); > } > EXPORT_SYMBOL(rtl_addr_delay); > > void rtl_rfreg_delay(struct ieee80211_hw *hw, enum radio_path rfpath, u32 addr, > u32 mask, u32 data) > { > - if (addr == 0xfe) { > - mdelay(50); > - } else if (addr == 0xfd) { > - mdelay(5); > - } else if (addr == 0xfc) { > - mdelay(1); > - } else if (addr == 0xfb) { > - udelay(50); > - } else if (addr == 0xfa) { > - udelay(5); > - } else if (addr == 0xf9) { > - udelay(1); > + if (addr >= 0xf9 && addr <= 0xfe) { > + rtl_addr_delay(addr); > } else { > rtl_set_rfreg(hw, rfpath, addr, mask, data); > - udelay(1); > + usleep_range(1, 2); > } > } > EXPORT_SYMBOL(rtl_rfreg_delay); > > void rtl_bb_delay(struct ieee80211_hw *hw, u32 addr, u32 data) > { > - if (addr == 0xfe) { > - mdelay(50); > - } else if (addr == 0xfd) { > - mdelay(5); > - } else if (addr == 0xfc) { > - mdelay(1); > - } else if (addr == 0xfb) { > - udelay(50); > - } else if (addr == 0xfa) { > - udelay(5); > - } else if (addr == 0xf9) { > - udelay(1); > + if (addr >= 0xf9 && addr <= 0xfe) { > + rtl_addr_delay(addr); > } else { > rtl_set_bbreg(hw, addr, MASKDWORD, data); > - udelay(1); > + usleep_range(1, 2); > } > } > EXPORT_SYMBOL(rtl_bb_delay); > -- > 2.1.4 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-wireless" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html Regards, Byeoungwook -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 02/19/2016 11:48 PM, ByeoungWook Kim wrote: > 2016-02-16 7:12 GMT+09:00 Larry Finger <Larry.Finger@lwfinger.net>: >> Routine rtl_addr_delay() uses delay statements in code that can >> sleep. To improve system responsiveness, the various delay statements >> are changed. >> >> In addition, routines rtl_rfreg_delay() and rtl_bb_delay() are >> rewritten to use the code in rtl_addr_delay() for most of their >> input values. >> >> Suggested-by: Byeoungwook Kim <quddnr145@gmail.com> >> Signed-off-by: Larry Finger <Larry.Finger@lwfinger.net> >> --- > > Why you are using 'Suggested-by:' of me? > I think to that your commit was included a part of my > commit(https://patchwork.kernel.org/patch/8197071/). Your commit was placed in the deferred list of patches. It did lead me to look at that section and realize that using delays there was a bug, and that it should be fixed as quickly as possible. That argues against waiting for your patches to be implemented. I did use some of your changes. How do you think I should have referenced your material? Larry -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
2016-02-20 23:06 GMT+09:00 Larry Finger <Larry.Finger@lwfinger.net>: > On 02/19/2016 11:48 PM, ByeoungWook Kim wrote: >> >> 2016-02-16 7:12 GMT+09:00 Larry Finger <Larry.Finger@lwfinger.net>: >>> >>> Routine rtl_addr_delay() uses delay statements in code that can >>> sleep. To improve system responsiveness, the various delay statements >>> are changed. >>> >>> In addition, routines rtl_rfreg_delay() and rtl_bb_delay() are >>> rewritten to use the code in rtl_addr_delay() for most of their >>> input values. >>> >>> Suggested-by: Byeoungwook Kim <quddnr145@gmail.com> >>> Signed-off-by: Larry Finger <Larry.Finger@lwfinger.net> >>> --- >> >> >> Why you are using 'Suggested-by:' of me? >> I think to that your commit was included a part of my >> commit(https://patchwork.kernel.org/patch/8197071/). > > > Your commit was placed in the deferred list of patches. It did lead me to > look at that section and realize that using delays there was a bug, and that > it should be fixed as quickly as possible. That argues against waiting for > your patches to be implemented. I did use some of your changes. How do you > think I should have referenced your material? > > Larry > > I'm understood your mail. Only, I wondered why 'Suggested-by' was used. Thank for your answer. Regards, Byeoungwook -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> Routine rtl_addr_delay() uses delay statements in code that can > sleep. To improve system responsiveness, the various delay statements > are changed. > > In addition, routines rtl_rfreg_delay() and rtl_bb_delay() are > rewritten to use the code in rtl_addr_delay() for most of their > input values. > > Suggested-by: Byeoungwook Kim <quddnr145@gmail.com> > Signed-off-by: Larry Finger <Larry.Finger@lwfinger.net> Thanks, applied to wireless-drivers-next.git. Kalle Valo -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 2016-02-15 23:12, Larry Finger wrote: > Routine rtl_addr_delay() uses delay statements in code that can > sleep. To improve system responsiveness, the various delay statements > are changed. > > In addition, routines rtl_rfreg_delay() and rtl_bb_delay() are > rewritten to use the code in rtl_addr_delay() for most of their > input values. > > Suggested-by: Byeoungwook Kim <quddnr145@gmail.com> > Signed-off-by: Larry Finger <Larry.Finger@lwfinger.net> > --- > > Kalle, > > This patch will interfere with a set of 3 patches submitted by Byeoungwook Kim > <quddnr145@gmail.com> on Feb. 3, 2016 that are currently in the deferred list > at patchwork. > > Larry > > drivers/net/wireless/realtek/rtlwifi/core.c | 44 ++++++++--------------------- > 1 file changed, 12 insertions(+), 32 deletions(-) > > diff --git a/drivers/net/wireless/realtek/rtlwifi/core.c b/drivers/net/wireless/realtek/rtlwifi/core.c > index 02eba0e..16ad0d6 100644 > --- a/drivers/net/wireless/realtek/rtlwifi/core.c > +++ b/drivers/net/wireless/realtek/rtlwifi/core.c > @@ -54,59 +54,39 @@ EXPORT_SYMBOL(channel5g_80m); > void rtl_addr_delay(u32 addr) > { > if (addr == 0xfe) > - mdelay(50); > + msleep(50); > else if (addr == 0xfd) > - mdelay(5); > + msleep(5); > else if (addr == 0xfc) > - mdelay(1); > + msleep(1); > else if (addr == 0xfb) > - udelay(50); > + usleep_range(50, 100); > else if (addr == 0xfa) > - udelay(5); > + usleep_range(5, 10); > else if (addr == 0xf9) > - udelay(1); > + usleep_range(1, 2); > } > EXPORT_SYMBOL(rtl_addr_delay); > > void rtl_rfreg_delay(struct ieee80211_hw *hw, enum radio_path rfpath, u32 addr, > u32 mask, u32 data) > { > - if (addr == 0xfe) { > - mdelay(50); > - } else if (addr == 0xfd) { > - mdelay(5); > - } else if (addr == 0xfc) { > - mdelay(1); > - } else if (addr == 0xfb) { > - udelay(50); > - } else if (addr == 0xfa) { > - udelay(5); > - } else if (addr == 0xf9) { > - udelay(1); > + if (addr >= 0xf9 && addr <= 0xfe) { > + rtl_addr_delay(addr); > } else { > rtl_set_rfreg(hw, rfpath, addr, mask, data); > - udelay(1); > + usleep_range(1, 2); > } > } > EXPORT_SYMBOL(rtl_rfreg_delay); > > void rtl_bb_delay(struct ieee80211_hw *hw, u32 addr, u32 data) > { > - if (addr == 0xfe) { > - mdelay(50); > - } else if (addr == 0xfd) { > - mdelay(5); > - } else if (addr == 0xfc) { > - mdelay(1); > - } else if (addr == 0xfb) { > - udelay(50); > - } else if (addr == 0xfa) { > - udelay(5); > - } else if (addr == 0xf9) { > - udelay(1); > + if (addr >= 0xf9 && addr <= 0xfe) { > + rtl_addr_delay(addr); > } else { > rtl_set_bbreg(hw, addr, MASKDWORD, data); > - udelay(1); > + usleep_range(1, 2); > } > } > EXPORT_SYMBOL(rtl_bb_delay); > This breaks spectacularly when turning on a little bit of correctness checking: BUG: scheduling while atomic: wpa_supplicant/1116/0x00000002 [...] Call Trace: [<ffffffff81030009>] dump_trace+0x59/0x340 [<ffffffff810303ec>] show_stack_log_lvl+0xfc/0x190 [<ffffffff81031191>] show_stack+0x21/0x40 [<ffffffff813827ae>] dump_stack+0x5c/0x7e [<ffffffff81181337>] __schedule_bug+0x4b/0x59 [<ffffffff81680f12>] thread_return+0x562/0x6b0 [<ffffffff8168109c>] schedule+0x3c/0x90 [<ffffffff81683dfb>] schedule_hrtimeout_range_clock+0xab/0x130 [<ffffffff8168386b>] usleep_range+0x3b/0x40 [<ffffffffa0951ee4>] rtl92c_phy_config_rf_with_headerfile+0x104/0x2f0 [rtl8192ce] [<ffffffffa0953d30>] rtl92ce_phy_rf6052_config+0x1b0/0x310 [rtl8192ce] [<ffffffffa095055b>] rtl92ce_hw_init+0xa8b/0x17e0 [rtl8192ce] [<ffffffffa060f22b>] rtl_ps_enable_nic+0x3b/0xc0 [rtlwifi] [<ffffffffa0952d39>] rtl92c_phy_set_rf_power_state+0x509/0x760 [rtl8192ce] [<ffffffffa060f3d4>] rtl_ps_set_rf_state+0xd4/0x200 [rtlwifi] [<ffffffffa060f538>] _rtl_ps_inactive_ps+0x38/0xb0 [rtlwifi] [<ffffffffa060f628>] rtl_ips_nic_on+0x78/0xb0 [rtlwifi] [<ffffffffa060c678>] rtl_op_config+0x278/0x440 [rtlwifi] [<ffffffffa072c9b6>] ieee80211_hw_config+0x56/0x3a0 [mac80211] [<ffffffffa076123a>] ieee80211_add_chanctx+0x17a/0x1c0 [mac80211] [<ffffffffa0761e2d>] ieee80211_new_chanctx+0x2d/0x80 [mac80211] [<ffffffffa0763f6b>] ieee80211_vif_use_channel+0x1fb/0x250 [mac80211] [<ffffffffa0776718>] ieee80211_prep_connection+0x188/0x8e0 [mac80211] [<ffffffffa077cb0a>] ieee80211_mgd_auth+0x22a/0x330 [mac80211] [<ffffffffa0453e57>] cfg80211_mlme_auth+0x117/0x230 [cfg80211] [<ffffffffa043a917>] nl80211_authenticate+0x2a7/0x300 [cfg80211] [<ffffffff815d4c56>] genl_family_rcv_msg+0x1b6/0x380 [<ffffffff815d4e99>] genl_rcv_msg+0x79/0xb0 [<ffffffff815d3ea2>] netlink_rcv_skb+0x92/0xa0 [<ffffffff815d44b4>] genl_rcv+0x24/0x40 [<ffffffff815d3896>] netlink_unicast+0x146/0x1f0 [<ffffffff815d3c63>] netlink_sendmsg+0x323/0x3a0 [<ffffffff81588c80>] sock_sendmsg+0x30/0x40 [<ffffffff81589529>] ___sys_sendmsg+0x279/0x290 [<ffffffff81589d81>] __sys_sendmsg+0x41/0x70 [<ffffffff81003aa9>] do_syscall_64+0x59/0xb0 [<ffffffff81684da5>] entry_SYSCALL64_slow_path+0x25/0x25 So please revert this commit. I agree with the general goal, but this approach does not work. I had to in order to make my wifi work again. Then, when looking deeper, those local_irq_enable constructs in the hw_init handlers are making me nervous. Are you sure that no task context that did something like spin_lock_irq* enters this code? Please redesign this. I'm not really into the various code paths to make safe suggestions, sorry. Jan
Jan Kiszka <jan.kiszka@web.de> writes: > On 2016-02-15 23:12, Larry Finger wrote: >> Routine rtl_addr_delay() uses delay statements in code that can >> sleep. To improve system responsiveness, the various delay statements >> are changed. >> >> In addition, routines rtl_rfreg_delay() and rtl_bb_delay() are >> rewritten to use the code in rtl_addr_delay() for most of their >> input values. >> >> Suggested-by: Byeoungwook Kim <quddnr145@gmail.com> >> Signed-off-by: Larry Finger <Larry.Finger@lwfinger.net> [...] > This breaks spectacularly when turning on a little bit of correctness > checking: > > BUG: scheduling while atomic: wpa_supplicant/1116/0x00000002 This should fix it: https://git.kernel.org/cgit/linux/kernel/git/kvalo/wireless-drivers.git/commit/?id=de26859dcf363d520cc44e59f6dcaf20ebe0aadf
On 2016-06-04 18:52, Kalle Valo wrote: > Jan Kiszka <jan.kiszka@web.de> writes: > >> On 2016-02-15 23:12, Larry Finger wrote: >>> Routine rtl_addr_delay() uses delay statements in code that can >>> sleep. To improve system responsiveness, the various delay statements >>> are changed. >>> >>> In addition, routines rtl_rfreg_delay() and rtl_bb_delay() are >>> rewritten to use the code in rtl_addr_delay() for most of their >>> input values. >>> >>> Suggested-by: Byeoungwook Kim <quddnr145@gmail.com> >>> Signed-off-by: Larry Finger <Larry.Finger@lwfinger.net> > > [...] > >> This breaks spectacularly when turning on a little bit of correctness >> checking: >> >> BUG: scheduling while atomic: wpa_supplicant/1116/0x00000002 > > This should fix it: > > https://git.kernel.org/cgit/linux/kernel/git/kvalo/wireless-drivers.git/commit/?id=de26859dcf363d520cc44e59f6dcaf20ebe0aadf > Probably, will test later. But you should really work on making all these task-context-only. Threaded IRQs? Jan
diff --git a/drivers/net/wireless/realtek/rtlwifi/core.c b/drivers/net/wireless/realtek/rtlwifi/core.c index 02eba0e..16ad0d6 100644 --- a/drivers/net/wireless/realtek/rtlwifi/core.c +++ b/drivers/net/wireless/realtek/rtlwifi/core.c @@ -54,59 +54,39 @@ EXPORT_SYMBOL(channel5g_80m); void rtl_addr_delay(u32 addr) { if (addr == 0xfe) - mdelay(50); + msleep(50); else if (addr == 0xfd) - mdelay(5); + msleep(5); else if (addr == 0xfc) - mdelay(1); + msleep(1); else if (addr == 0xfb) - udelay(50); + usleep_range(50, 100); else if (addr == 0xfa) - udelay(5); + usleep_range(5, 10); else if (addr == 0xf9) - udelay(1); + usleep_range(1, 2); } EXPORT_SYMBOL(rtl_addr_delay); void rtl_rfreg_delay(struct ieee80211_hw *hw, enum radio_path rfpath, u32 addr, u32 mask, u32 data) { - if (addr == 0xfe) { - mdelay(50); - } else if (addr == 0xfd) { - mdelay(5); - } else if (addr == 0xfc) { - mdelay(1); - } else if (addr == 0xfb) { - udelay(50); - } else if (addr == 0xfa) { - udelay(5); - } else if (addr == 0xf9) { - udelay(1); + if (addr >= 0xf9 && addr <= 0xfe) { + rtl_addr_delay(addr); } else { rtl_set_rfreg(hw, rfpath, addr, mask, data); - udelay(1); + usleep_range(1, 2); } } EXPORT_SYMBOL(rtl_rfreg_delay); void rtl_bb_delay(struct ieee80211_hw *hw, u32 addr, u32 data) { - if (addr == 0xfe) { - mdelay(50); - } else if (addr == 0xfd) { - mdelay(5); - } else if (addr == 0xfc) { - mdelay(1); - } else if (addr == 0xfb) { - udelay(50); - } else if (addr == 0xfa) { - udelay(5); - } else if (addr == 0xf9) { - udelay(1); + if (addr >= 0xf9 && addr <= 0xfe) { + rtl_addr_delay(addr); } else { rtl_set_bbreg(hw, addr, MASKDWORD, data); - udelay(1); + usleep_range(1, 2); } } EXPORT_SYMBOL(rtl_bb_delay);
Routine rtl_addr_delay() uses delay statements in code that can sleep. To improve system responsiveness, the various delay statements are changed. In addition, routines rtl_rfreg_delay() and rtl_bb_delay() are rewritten to use the code in rtl_addr_delay() for most of their input values. Suggested-by: Byeoungwook Kim <quddnr145@gmail.com> Signed-off-by: Larry Finger <Larry.Finger@lwfinger.net> --- Kalle, This patch will interfere with a set of 3 patches submitted by Byeoungwook Kim <quddnr145@gmail.com> on Feb. 3, 2016 that are currently in the deferred list at patchwork. Larry drivers/net/wireless/realtek/rtlwifi/core.c | 44 ++++++++--------------------- 1 file changed, 12 insertions(+), 32 deletions(-)