| Message ID | 20230412215855.593541-1-hdegoede@redhat.com |
|---|---|
| Headers | show |
| Series | Fix oops about sleeping in led_trigger_blink() | expand |
Hi Hans, Generally the series looks good to me, but it would be good to get some Tested-by(s). Reviewed-by: Jacek Anaszewski <jacek.anaszewski@gmail.com> On 4/12/23 23:58, Hans de Goede wrote: > Hi All, > > Here is a patch series to fix an oops about sleeping in led_trigger_blink() > + one other small bugfix. > > Patches 1-3 should arguably have a: > > Fixes: 0b9536c95709 ("leds: Add ability to blink via simple trigger") > > tag, but Fixes tags tend to lead to patches getting automatically added > to the stable series and I would prefer to see this series get some > significant testing time in mainline first, so I have chosen to omit > the tag. > > Regards, > > Hans > > > Hans de Goede (4): > leds: Change led_trigger_blink[_oneshot]() delay parameters to > pass-by-value > leds: Fix set_brightness_delayed() race > leds: Fix oops about sleeping in led_trigger_blink() > leds: Clear LED_INIT_DEFAULT_TRIGGER when clearing current trigger > > drivers/leds/led-core.c | 81 ++++++++++++++++++++---- > drivers/leds/led-triggers.c | 17 ++--- > drivers/leds/trigger/ledtrig-disk.c | 9 +-- > drivers/leds/trigger/ledtrig-mtd.c | 8 +-- > drivers/net/arcnet/arcnet.c | 8 +-- > drivers/power/supply/power_supply_leds.c | 5 +- > drivers/usb/common/led.c | 4 +- > include/linux/leds.h | 43 ++++++++++--- > net/mac80211/led.c | 2 +- > net/mac80211/led.h | 8 +-- > net/netfilter/xt_LED.c | 3 +- > 11 files changed, 125 insertions(+), 63 deletions(-) >
On Wed, 12 Apr 2023, Hans de Goede wrote: > Hi All, > > Here is a patch series to fix an oops about sleeping in led_trigger_blink() > + one other small bugfix. > > Patches 1-3 should arguably have a: > > Fixes: 0b9536c95709 ("leds: Add ability to blink via simple trigger") > > tag, but Fixes tags tend to lead to patches getting automatically added > to the stable series and I would prefer to see this series get some > significant testing time in mainline first, so I have chosen to omit > the tag. With subjects with the word "fix" in it, they will be hoovered up by the Stable auto-picker anyway. > Hans de Goede (4): > leds: Change led_trigger_blink[_oneshot]() delay parameters to > pass-by-value > leds: Fix set_brightness_delayed() race > leds: Fix oops about sleeping in led_trigger_blink() > leds: Clear LED_INIT_DEFAULT_TRIGGER when clearing current trigger > > drivers/leds/led-core.c | 81 ++++++++++++++++++++---- > drivers/leds/led-triggers.c | 17 ++--- > drivers/leds/trigger/ledtrig-disk.c | 9 +-- > drivers/leds/trigger/ledtrig-mtd.c | 8 +-- > drivers/net/arcnet/arcnet.c | 8 +-- > drivers/power/supply/power_supply_leds.c | 5 +- > drivers/usb/common/led.c | 4 +- > include/linux/leds.h | 43 ++++++++++--- > net/mac80211/led.c | 2 +- > net/mac80211/led.h | 8 +-- > net/netfilter/xt_LED.c | 3 +- > 11 files changed, 125 insertions(+), 63 deletions(-) > > -- > 2.39.1 >
Hi Lee, On 4/20/23 13:36, Lee Jones wrote: > On Wed, 12 Apr 2023, Hans de Goede wrote: > >> Hi All, >> >> Here is a patch series to fix an oops about sleeping in led_trigger_blink() >> + one other small bugfix. >> >> Patches 1-3 should arguably have a: >> >> Fixes: 0b9536c95709 ("leds: Add ability to blink via simple trigger") >> >> tag, but Fixes tags tend to lead to patches getting automatically added >> to the stable series and I would prefer to see this series get some >> significant testing time in mainline first, so I have chosen to omit >> the tag. > > With subjects with the word "fix" in it, they will be hoovered up by the > Stable auto-picker anyway. Ok, in that case patch 3 should have: Fixes: 0b9536c95709 ("leds: Add ability to blink via simple trigger") Patches 1-2 are more preparation patches for this. Patch 2 does fix another race, but I'm not sure we ever hit that. Can you add the fixes tag while merging these, or do you want a v2 of this series ? Regards, Hans > >> Hans de Goede (4): >> leds: Change led_trigger_blink[_oneshot]() delay parameters to >> pass-by-value >> leds: Fix set_brightness_delayed() race >> leds: Fix oops about sleeping in led_trigger_blink() >> leds: Clear LED_INIT_DEFAULT_TRIGGER when clearing current trigger >> >> drivers/leds/led-core.c | 81 ++++++++++++++++++++---- >> drivers/leds/led-triggers.c | 17 ++--- >> drivers/leds/trigger/ledtrig-disk.c | 9 +-- >> drivers/leds/trigger/ledtrig-mtd.c | 8 +-- >> drivers/net/arcnet/arcnet.c | 8 +-- >> drivers/power/supply/power_supply_leds.c | 5 +- >> drivers/usb/common/led.c | 4 +- >> include/linux/leds.h | 43 ++++++++++--- >> net/mac80211/led.c | 2 +- >> net/mac80211/led.h | 8 +-- >> net/netfilter/xt_LED.c | 3 +- >> 11 files changed, 125 insertions(+), 63 deletions(-) >> >> -- >> 2.39.1 >> >
On Thu, 20 Apr 2023, Hans de Goede wrote: > Hi Lee, > > On 4/20/23 13:36, Lee Jones wrote: > > On Wed, 12 Apr 2023, Hans de Goede wrote: > > > >> Hi All, > >> > >> Here is a patch series to fix an oops about sleeping in led_trigger_blink() > >> + one other small bugfix. > >> > >> Patches 1-3 should arguably have a: > >> > >> Fixes: 0b9536c95709 ("leds: Add ability to blink via simple trigger") > >> > >> tag, but Fixes tags tend to lead to patches getting automatically added > >> to the stable series and I would prefer to see this series get some > >> significant testing time in mainline first, so I have chosen to omit > >> the tag. > > > > With subjects with the word "fix" in it, they will be hoovered up by the > > Stable auto-picker anyway. > > Ok, in that case patch 3 should have: > > Fixes: 0b9536c95709 ("leds: Add ability to blink via simple trigger") > > Patches 1-2 are more preparation patches for this. Patch 2 does > fix another race, but I'm not sure we ever hit that. > > Can you add the fixes tag while merging these, or do you > want a v2 of this series ? I'm holding out for either a Pavel review or some Tested-by's suggested by Jacek.
Hi Lee, On 4/20/23 15:56, Lee Jones wrote: > On Thu, 20 Apr 2023, Hans de Goede wrote: > >> Hi Lee, >> >> On 4/20/23 13:36, Lee Jones wrote: >>> On Wed, 12 Apr 2023, Hans de Goede wrote: >>> >>>> Hi All, >>>> >>>> Here is a patch series to fix an oops about sleeping in led_trigger_blink() >>>> + one other small bugfix. >>>> >>>> Patches 1-3 should arguably have a: >>>> >>>> Fixes: 0b9536c95709 ("leds: Add ability to blink via simple trigger") >>>> >>>> tag, but Fixes tags tend to lead to patches getting automatically added >>>> to the stable series and I would prefer to see this series get some >>>> significant testing time in mainline first, so I have chosen to omit >>>> the tag. >>> >>> With subjects with the word "fix" in it, they will be hoovered up by the >>> Stable auto-picker anyway. >> >> Ok, in that case patch 3 should have: >> >> Fixes: 0b9536c95709 ("leds: Add ability to blink via simple trigger") >> >> Patches 1-2 are more preparation patches for this. Patch 2 does >> fix another race, but I'm not sure we ever hit that. >> >> Can you add the fixes tag while merging these, or do you >> want a v2 of this series ? > > I'm holding out for either a Pavel review or some Tested-by's suggested > by Jacek. Hmm, ok. I have asked Yauhen to give this a test since they have hit the oops/backtrace fixed by path 3/4 while testing the new leds-cht-wcove driver too. But Yauhen has the same hw as me (I have already tested this on 3 different laptop models). Note that Jacek did already give his Reviewed-by: Reviewed-by: Jacek Anaszewski <jacek.anaszewski@gmail.com> I think the bug this fixes was never an issue before because only very few triggers use regular blinking (rather then one-shot blinking which always uses the sw-blink implementation). To hit this you need to use one of the few triggers which actually use regular-blinking in combination with a driver which supports hw-blinking and where its blink_set callbavck may sleep. It looks to me like no-one has hit this combination before. Which is why there are no bug reports for the issue and which also is why finding testers is going to be tricky. I think that the best thing to do here is add this series to -next early in the upcoming cycle, so that it gets the maximum testing time possible in -next. Regards, Hans
On Tue, 25 Apr 2023, Hans de Goede wrote: > Hi Lee, > > On 4/20/23 15:56, Lee Jones wrote: > > On Thu, 20 Apr 2023, Hans de Goede wrote: > > > >> Hi Lee, > >> > >> On 4/20/23 13:36, Lee Jones wrote: > >>> On Wed, 12 Apr 2023, Hans de Goede wrote: > >>> > >>>> Hi All, > >>>> > >>>> Here is a patch series to fix an oops about sleeping in led_trigger_blink() > >>>> + one other small bugfix. > >>>> > >>>> Patches 1-3 should arguably have a: > >>>> > >>>> Fixes: 0b9536c95709 ("leds: Add ability to blink via simple trigger") > >>>> > >>>> tag, but Fixes tags tend to lead to patches getting automatically added > >>>> to the stable series and I would prefer to see this series get some > >>>> significant testing time in mainline first, so I have chosen to omit > >>>> the tag. > >>> > >>> With subjects with the word "fix" in it, they will be hoovered up by the > >>> Stable auto-picker anyway. > >> > >> Ok, in that case patch 3 should have: > >> > >> Fixes: 0b9536c95709 ("leds: Add ability to blink via simple trigger") > >> > >> Patches 1-2 are more preparation patches for this. Patch 2 does > >> fix another race, but I'm not sure we ever hit that. > >> > >> Can you add the fixes tag while merging these, or do you > >> want a v2 of this series ? > > > > I'm holding out for either a Pavel review or some Tested-by's suggested > > by Jacek. > > Hmm, ok. I have asked Yauhen to give this a test since they have hit > the oops/backtrace fixed by path 3/4 while testing the new leds-cht-wcove > driver too. > > But Yauhen has the same hw as me (I have already tested this on > 3 different laptop models). > > Note that Jacek did already give his Reviewed-by: > > Reviewed-by: Jacek Anaszewski <jacek.anaszewski@gmail.com> > > I think the bug this fixes was never an issue before because only > very few triggers use regular blinking (rather then one-shot > blinking which always uses the sw-blink implementation). > > To hit this you need to use one of the few triggers which > actually use regular-blinking in combination with a > driver which supports hw-blinking and where its blink_set > callbavck may sleep. It looks to me like no-one has hit > this combination before. Which is why there are no bug reports > for the issue and which also is why finding testers is going > to be tricky. > > I think that the best thing to do here is add this series to -next > early in the upcoming cycle, so that it gets the maximum testing > time possible in -next. Agree. Let's revisit this once the merge-window closes.
On Wed, Apr 26, 2023 at 03:34:39PM +0100, Lee Jones wrote: > On Tue, 25 Apr 2023, Hans de Goede wrote: > > > Hi Lee, > > > > On 4/20/23 15:56, Lee Jones wrote: > > > On Thu, 20 Apr 2023, Hans de Goede wrote: > > > > > >> Hi Lee, > > >> > > >> On 4/20/23 13:36, Lee Jones wrote: > > >>> On Wed, 12 Apr 2023, Hans de Goede wrote: > > >>> > > >>>> Hi All, > > >>>> > > >>>> Here is a patch series to fix an oops about sleeping in led_trigger_blink() > > >>>> + one other small bugfix. > > >>>> > > >>>> Patches 1-3 should arguably have a: > > >>>> > > >>>> Fixes: 0b9536c95709 ("leds: Add ability to blink via simple trigger") > > >>>> > > >>>> tag, but Fixes tags tend to lead to patches getting automatically added > > >>>> to the stable series and I would prefer to see this series get some > > >>>> significant testing time in mainline first, so I have chosen to omit > > >>>> the tag. > > >>> > > >>> With subjects with the word "fix" in it, they will be hoovered up by the > > >>> Stable auto-picker anyway. > > >> > > >> Ok, in that case patch 3 should have: > > >> > > >> Fixes: 0b9536c95709 ("leds: Add ability to blink via simple trigger") > > >> > > >> Patches 1-2 are more preparation patches for this. Patch 2 does > > >> fix another race, but I'm not sure we ever hit that. > > >> > > >> Can you add the fixes tag while merging these, or do you > > >> want a v2 of this series ? > > > > > > I'm holding out for either a Pavel review or some Tested-by's suggested > > > by Jacek. > > > > Hmm, ok. I have asked Yauhen to give this a test since they have hit > > the oops/backtrace fixed by path 3/4 while testing the new leds-cht-wcove > > driver too. > > > > But Yauhen has the same hw as me (I have already tested this on > > 3 different laptop models). > > > > Note that Jacek did already give his Reviewed-by: > > > > Reviewed-by: Jacek Anaszewski <jacek.anaszewski@gmail.com> > > > > I think the bug this fixes was never an issue before because only > > very few triggers use regular blinking (rather then one-shot > > blinking which always uses the sw-blink implementation). > > > > To hit this you need to use one of the few triggers which > > actually use regular-blinking in combination with a > > driver which supports hw-blinking and where its blink_set > > callbavck may sleep. It looks to me like no-one has hit > > this combination before. Which is why there are no bug reports > > for the issue and which also is why finding testers is going > > to be tricky. > > > > I think that the best thing to do here is add this series to -next > > early in the upcoming cycle, so that it gets the maximum testing > > time possible in -next. > > Agree. Let's revisit this once the merge-window closes. Tested-by: Yauhen Kharuzhy <jekhor@gmail.com> For entire series.
Hi Lee, On 4/26/23 16:34, Lee Jones wrote: > On Tue, 25 Apr 2023, Hans de Goede wrote: > >> Hi Lee, >> >> On 4/20/23 15:56, Lee Jones wrote: >>> On Thu, 20 Apr 2023, Hans de Goede wrote: >>> >>>> Hi Lee, >>>> >>>> On 4/20/23 13:36, Lee Jones wrote: >>>>> On Wed, 12 Apr 2023, Hans de Goede wrote: >>>>> >>>>>> Hi All, >>>>>> >>>>>> Here is a patch series to fix an oops about sleeping in led_trigger_blink() >>>>>> + one other small bugfix. >>>>>> >>>>>> Patches 1-3 should arguably have a: >>>>>> >>>>>> Fixes: 0b9536c95709 ("leds: Add ability to blink via simple trigger") >>>>>> >>>>>> tag, but Fixes tags tend to lead to patches getting automatically added >>>>>> to the stable series and I would prefer to see this series get some >>>>>> significant testing time in mainline first, so I have chosen to omit >>>>>> the tag. >>>>> >>>>> With subjects with the word "fix" in it, they will be hoovered up by the >>>>> Stable auto-picker anyway. >>>> >>>> Ok, in that case patch 3 should have: >>>> >>>> Fixes: 0b9536c95709 ("leds: Add ability to blink via simple trigger") >>>> >>>> Patches 1-2 are more preparation patches for this. Patch 2 does >>>> fix another race, but I'm not sure we ever hit that. >>>> >>>> Can you add the fixes tag while merging these, or do you >>>> want a v2 of this series ? >>> >>> I'm holding out for either a Pavel review or some Tested-by's suggested >>> by Jacek. >> >> Hmm, ok. I have asked Yauhen to give this a test since they have hit >> the oops/backtrace fixed by path 3/4 while testing the new leds-cht-wcove >> driver too. >> >> But Yauhen has the same hw as me (I have already tested this on >> 3 different laptop models). >> >> Note that Jacek did already give his Reviewed-by: >> >> Reviewed-by: Jacek Anaszewski <jacek.anaszewski@gmail.com> >> >> I think the bug this fixes was never an issue before because only >> very few triggers use regular blinking (rather then one-shot >> blinking which always uses the sw-blink implementation). >> >> To hit this you need to use one of the few triggers which >> actually use regular-blinking in combination with a >> driver which supports hw-blinking and where its blink_set >> callbavck may sleep. It looks to me like no-one has hit >> this combination before. Which is why there are no bug reports >> for the issue and which also is why finding testers is going >> to be tricky. >> >> I think that the best thing to do here is add this series to -next >> early in the upcoming cycle, so that it gets the maximum testing >> time possible in -next. > > Agree. Let's revisit this once the merge-window closes. The merge-window has closed now, and Yauhen has given their tested-by: Tested-by: Yauhen Kharuzhy <jekhor@gmail.com> Lee, can you merge this now please so that this gets the maximum possible time for testing in linux-next ? Regards, Hans
Hi All, Here is a patch series to fix an oops about sleeping in led_trigger_blink() + one other small bugfix. Patches 1-3 should arguably have a: Fixes: 0b9536c95709 ("leds: Add ability to blink via simple trigger") tag, but Fixes tags tend to lead to patches getting automatically added to the stable series and I would prefer to see this series get some significant testing time in mainline first, so I have chosen to omit the tag. Regards, Hans Hans de Goede (4): leds: Change led_trigger_blink[_oneshot]() delay parameters to pass-by-value leds: Fix set_brightness_delayed() race leds: Fix oops about sleeping in led_trigger_blink() leds: Clear LED_INIT_DEFAULT_TRIGGER when clearing current trigger drivers/leds/led-core.c | 81 ++++++++++++++++++++---- drivers/leds/led-triggers.c | 17 ++--- drivers/leds/trigger/ledtrig-disk.c | 9 +-- drivers/leds/trigger/ledtrig-mtd.c | 8 +-- drivers/net/arcnet/arcnet.c | 8 +-- drivers/power/supply/power_supply_leds.c | 5 +- drivers/usb/common/led.c | 4 +- include/linux/leds.h | 43 ++++++++++--- net/mac80211/led.c | 2 +- net/mac80211/led.h | 8 +-- net/netfilter/xt_LED.c | 3 +- 11 files changed, 125 insertions(+), 63 deletions(-)