Re: [Intel-gfx] [PATCH v4a 00/38] timers: Use timer_shutdown*() before freeing timers
On Sat, 5 Nov 2022 14:13:14 -0700 Linus Torvalds wrote: > (Comparing output is also fun because the ordering of the patches is > random, so consecutive runs with the same rule will give different > patches. I assume that it's just because it's done in parallel, but it > doesn't help the "try to see what changes when you change the script" > ;) What I do to compare is: patch -p1 < cocci1.patch git commit -a git show | patch -p1 -R patch -p1 < cocci2.patch git diff Then I see how things changed. This is how I was able to show you the tweaks I made. -- Steve
Re: [Intel-gfx] [PATCH v4a 00/38] timers: Use timer_shutdown*() before freeing timers
On Sat, 5 Nov 2022 14:13:14 -0700 Linus Torvalds wrote: > And trying "when != ptr->timer" actually does the right thing in that > it gets rid of the case where the timer is modified outside of the > del_timer() case, *but* it also causes odd other changes to the > output. > > Look at what it generates for that > >drivers/media/usb/pvrusb2/pvrusb2-hdw.c > > file, which finds a lot of triggers with the "when != ptr->timer", > but only does one without it. I added an expression, and it appears to work: At least for this case. @@ expression E; identifier ptr, timer, rfield, slab; @@ ( - del_timer(>timer); + timer_shutdown(>timer); | - del_timer_sync(>timer); + timer_shutdown_sync(>timer); ) ... when != ptr->timer.function = E; ( kfree_rcu(ptr, rfield); | kmem_cache_free(slab, ptr); | kfree(ptr); ) Now I need to add return and goto cases here. -- Steve
Re: [Intel-gfx] [PATCH v4a 00/38] timers: Use timer_shutdown*() before freeing timers
On Sat, Nov 5, 2022 at 2:03 PM Jason A. Donenfeld wrote: > > Something that might help here is changing the `...` into > `... when exists` or into `... when != ptr` or similar. I actually tried that. You don't want "when exists", you'd want "when forall", but that seems to be the default. And trying "when != ptr->timer" actually does the right thing in that it gets rid of the case where the timer is modified outside of the del_timer() case, *but* it also causes odd other changes to the output. Look at what it generates for that drivers/media/usb/pvrusb2/pvrusb2-hdw.c file, which finds a lot of triggers with the "when != ptr->timer", but only does one without it. So I gave up, just because I clearly don't understand the rules. (Comparing output is also fun because the ordering of the patches is random, so consecutive runs with the same rule will give different patches. I assume that it's just because it's done in parallel, but it doesn't help the "try to see what changes when you change the script" ;) Linus
Re: [Intel-gfx] [PATCH v4a 00/38] timers: Use timer_shutdown*() before freeing timers
On Sat, Nov 05, 2022 at 12:36:42PM -0400, Steven Rostedt wrote: > --8< > @@ > identifier ptr, timer, rfield, slab; > @@ > ( > - del_timer(>timer); > + timer_shutdown(>timer); > | > - del_timer_sync(>timer); > + timer_shutdown_sync(>timer); > ) > ... > ( > kfree_rcu(ptr, rfield); > | > kmem_cache_free(slab, ptr); > | > kfree(ptr); > ) > -->8 Something that might help here is changing the `...` into `... when exists` or into `... when != ptr` or similar. See this section of the manual: https://coccinelle.gitlabpages.inria.fr/website/docs/main_grammar004.html Jason
Re: [Intel-gfx] [PATCH v4a 00/38] timers: Use timer_shutdown*() before freeing timers
On Sat, Nov 05, 2022 at 02:00:24AM -0400, Steven Rostedt wrote: > > Back in April, I posted an RFC patch set to help mitigate a common issue > where a timer gets armed just before it is freed, and when the timer > goes off, it crashes in the timer code without any evidence of who the > culprit was. I got side tracked and never finished up on that patch set. > Since this type of crash is still our #1 crash we are seeing in the field, > it has become a priority again to finish it. > > The last version of that patch set is here: > > https://lore.kernel.org/all/20221104054053.431922...@goodmis.org/ > > I'm calling this version 4a as it only has obvious changes were the timer that > is being shutdown is in the same function where it will be freed or released, > as this series should be "safe" for adding. I'll be calling the other patches > 4b for the next merge window. > For the series, as far as my testbed goes: Build results: total: 152 pass: 152 fail: 0 Qemu test results: total: 500 pass: 500 fail: 0 No runtime crashes or warnings observed. Tested-by: Guenter Roeck Guenter
Re: [Intel-gfx] [PATCH v4a 00/38] timers: Use timer_shutdown*() before freeing timers
On Sat, Nov 5, 2022 at 11:04 AM Steven Rostedt wrote: > > Here's the changes I made after running the script Please. No. What part of "I don't want extra crud" was I unclear on? I'm not interested in converting everything. That's clearly a 6.,2 issue, possibly even longer considering how complicated the networking side has been. I'm not AT ALL interested in "oh, I then added my own small cleanups on top to random files because I happened to notice them". Repeat after me: "If the script didn't catch them, they weren't trivially obvious". And it does seem that right now the script itself is a bit too generous, which is why it didn't notice that sometimes there wasn't a kfree after all because of a goto around it. So clearly that "..." doesn't really work, I think it accepts "_any_ path leads to the second situation" rather than "_all_ paths lead to the second situation". But yeah, my coccinelle-foo is very weak too, and maybe there's no pattern for "no flow control". I would also like the coccinelle script to notice the "timer is used afterwards", so that it does *not* modify that case that does del_timer(>timer); dch->timer.function = NULL; since now the timer is modified in between the del_timer() and the kfree. Again, that timer modification is then made pointless by changing the del_timer() to a "timer_shutdown()", but at that point it is no longer a "so obvious non-semantic change that it should be scripted". At that point it's a manual thing. So I think the "..." in your script should be "no flow control, and no access to the timer", but do not know how to do that in coccinelle. Julia? And this thread has way too many participants, I suspect some email systems will just mark it as spam as a result. Which is partly *why* I would like to get rid of noisy changes that really don't matter - but I would like it to be truly mindlessly obvious that there are *zero* questions about it, and absolutely no manual intervention because the patch is so strict that it's just unquestionably correct. Linus
Re: [Intel-gfx] [PATCH v4a 00/38] timers: Use timer_shutdown*() before freeing timers
On Sat, 5 Nov 2022 14:03:56 -0400
Steven Rostedt wrote:
> --- a/drivers/isdn/hardware/mISDN/hfcmulti.c
> +++ b/drivers/isdn/hardware/mISDN/hfcmulti.c
> @@ -4544,7 +4544,7 @@ release_port(struct hfc_multi *hc, struct dchannel *dch)
> spin_lock_irqsave(>lock, flags);
>
> if (dch->timer.function) {
> - del_timer(>timer);
> + timer_shutdown(>timer);
> dch->timer.function = NULL;
> }
>
I still hate the above.
-- Steve
Re: [Intel-gfx] [PATCH v4a 00/38] timers: Use timer_shutdown*() before freeing timers
On Sat, 5 Nov 2022 12:36:42 -0400 Steven Rostedt wrote: > --8< > @@ > identifier ptr, timer, rfield, slab; > @@ > ( > - del_timer(>timer); > + timer_shutdown(>timer); > | > - del_timer_sync(>timer); > + timer_shutdown_sync(>timer); > ) > ... > ( > kfree_rcu(ptr, rfield); > | > kmem_cache_free(slab, ptr); > | > kfree(ptr); > ) > -->8 Below is the result of the above patch, but I did do the following modifications because of the one case where it it exited the function after the del_timer(). And the other case was that it doesn't handle multiple timers for the same object. Here's the changes I made after running the script: diff --git a/drivers/net/wireless/intel/iwlwifi/mvm/sta.c b/drivers/net/wireless/intel/iwlwifi/mvm/sta.c index 76ea44cebd90..cbd8053a9e35 100644 --- a/drivers/net/wireless/intel/iwlwifi/mvm/sta.c +++ b/drivers/net/wireless/intel/iwlwifi/mvm/sta.c @@ -2826,7 +2826,7 @@ int iwl_mvm_sta_rx_agg(struct iwl_mvm *mvm, struct ieee80211_sta *sta, /* synchronize all rx queues so we can safely delete */ iwl_mvm_free_reorder(mvm, baid_data); - timer_shutdown_sync(_data->session_timer); + del_timer_sync(_data->session_timer); RCU_INIT_POINTER(mvm->baid_map[baid], NULL); kfree_rcu(baid_data, rcu_head); IWL_DEBUG_HT(mvm, "BAID %d is free\n", baid); diff --git a/drivers/net/wireless/microchip/wilc1000/hif.c b/drivers/net/wireless/microchip/wilc1000/hif.c index 3414feb1763f..131f9fd97c37 100644 --- a/drivers/net/wireless/microchip/wilc1000/hif.c +++ b/drivers/net/wireless/microchip/wilc1000/hif.c @@ -1520,8 +1520,8 @@ int wilc_deinit(struct wilc_vif *vif) mutex_lock(>wilc->deinit_lock); - del_timer_sync(_drv->scan_timer); - del_timer_sync(_drv->connect_timer); + timer_shutdown_sync(_drv->scan_timer); + timer_shutdown_sync(_drv->connect_timer); del_timer_sync(>periodic_rssi); timer_shutdown_sync(_drv->remain_on_ch_timer); And below is the script plus the above changes: Is that acceptable after adding patches 2-5 and removing the del_singleshot_timer_sync() change? -- Steve From: "Steven Rostedt (Google)" Subject: [PATCH] treewide: Convert del_timer*() to timer_shutdown*() I used the coccinelle script to make the below changes: @@ identifier ptr, timer, rfield, slab; @@ ( - del_timer(>timer); + timer_shutdown(>timer); | - del_timer_sync(>timer); + timer_shutdown_sync(>timer); ) ... ( kfree_rcu(ptr, rfield); | kmem_cache_free(slab, ptr); | kfree(ptr); ) Signed-off-by: Steven Rostedt (Google) --- arch/sh/drivers/push-switch.c | 2 +- block/blk-iocost.c | 2 +- block/blk-iolatency.c | 2 +- block/kyber-iosched.c | 2 +- drivers/acpi/apei/ghes.c | 2 +- drivers/atm/idt77252.c | 4 ++-- drivers/block/drbd/drbd_main.c | 2 +- drivers/block/loop.c | 2 +- drivers/bluetooth/hci_bcsp.c | 2 +- drivers/bluetooth/hci_qca.c| 2 +- drivers/gpu/drm/i915/i915_sw_fence.c | 2 +- drivers/hid/hid-wiimote-core.c | 2 +- drivers/input/keyboard/locomokbd.c | 2 +- drivers/input/keyboard/omap-keypad.c | 2 +- drivers/input/mouse/alps.c | 2 +- drivers/isdn/hardware/mISDN/hfcmulti.c | 2 +- drivers/isdn/mISDN/l1oip_core.c| 2 +- drivers/isdn/mISDN/timerdev.c | 4 ++-- drivers/leds/trigger/ledtrig-activity.c| 2 +- drivers/leds/trigger/ledtrig-heartbeat.c | 2 +- drivers/leds/trigger/ledtrig-pattern.c | 2 +- drivers/leds/trigger/ledtrig-transient.c | 2 +- drivers/media/pci/ivtv/ivtv-driver.c | 2 +- drivers/media/usb/pvrusb2/pvrusb2-hdw.c| 4 ++-- drivers/media/usb/s2255/s2255drv.c | 4 ++-- drivers/net/ethernet/intel/i40e/i40e_main.c| 6 +++--- drivers/net/ethernet/marvell/sky2.c| 2 +- drivers/net/ethernet/sun/sunvnet.c | 2 +- drivers/net/usb/sierra_net.c | 2 +- drivers/net/wireless/intel/iwlwifi/iwl-dbg-tlv.c | 2 +- drivers/net/wireless/intersil/hostap/hostap_ap.c | 2 +- drivers/net/wireless/marvell/mwifiex/main.c| 2 +- drivers/net/wireless/microchip/wilc1000/hif.c | 6 +++--- drivers/nfc/pn533/pn533.c | 2 +- drivers/nfc/pn533/uart.c | 2 +- drivers/pcmcia/bcm63xx_pcmcia.c| 2 +- drivers/pcmcia/electra_cf.c| 2 +-
Re: [Intel-gfx] [PATCH v4a 00/38] timers: Use timer_shutdown*() before freeing timers
On Sat, 5 Nov 2022 08:59:36 -0700
Linus Torvalds wrote:
> Others in the series were *definitely* not scripted, doing clearly
> manual cleanups:
>
> -if (dch->timer.function) {
> -del_timer(>timer);
> -dch->timer.function = NULL;
> -}
> +timer_shutdown(>timer);
>
> so no, this does *not* make me feel "ok, this is all trivial".
I just ran the script and the above code turned to:
diff --git a/drivers/isdn/hardware/mISDN/hfcmulti.c
b/drivers/isdn/hardware/mISDN/hfcmulti.c
index 4f7eaa17fb27..2695bbde52db 100644
--- a/drivers/isdn/hardware/mISDN/hfcmulti.c
+++ b/drivers/isdn/hardware/mISDN/hfcmulti.c
@@ -4544,7 +4544,7 @@ release_port(struct hfc_multi *hc, struct dchannel *dch)
spin_lock_irqsave(>lock, flags);
if (dch->timer.function) {
- del_timer(>timer);
+ timer_shutdown(>timer);
dch->timer.function = NULL;
}
Which is silly. Because timer_shutdown() makes timer.function = NULL.
That's why I changed it. And it really shouldn't be touching
timer.function anyway.
-- Steve
Re: [Intel-gfx] [PATCH v6 14/23] drm/modes: Properly generate a drm_display_mode from a named mode
Den 26.10.2022 17.33, skrev max...@cerno.tech:
> The framework will get the drm_display_mode from the drm_cmdline_mode it
> got by parsing the video command line argument by calling
> drm_connector_pick_cmdline_mode().
>
> The heavy lifting will then be done by the drm_mode_create_from_cmdline_mode()
> function.
>
> In the case of the named modes though, there's no real code to make that
> translation and we rely on the drivers to guess which actual display mode
> we meant.
>
> Let's modify drm_mode_create_from_cmdline_mode() to properly generate the
> drm_display_mode we mean when passing a named mode.
>
> Signed-off-by: Maxime Ripard
>
> ---
> Changes in v6:
> - Fix get_modes to return 0 instead of an error code
> - Rename the tests to follow the DRM test naming convention
>
> Changes in v5:
> - Switched to KUNIT_ASSERT_NOT_NULL
> ---
> drivers/gpu/drm/drm_modes.c | 34 ++-
> drivers/gpu/drm/tests/drm_client_modeset_test.c | 77
> -
> 2 files changed, 109 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
> index dc037f7ceb37..85aa9898c229 100644
> --- a/drivers/gpu/drm/drm_modes.c
> +++ b/drivers/gpu/drm/drm_modes.c
> @@ -2497,6 +2497,36 @@ bool drm_mode_parse_command_line_for_connector(const
> char *mode_option,
> }
> EXPORT_SYMBOL(drm_mode_parse_command_line_for_connector);
>
> +static struct drm_display_mode *drm_named_mode(struct drm_device *dev,
> +struct drm_cmdline_mode *cmd)
> +{
> + struct drm_display_mode *mode;
> + unsigned int i;
> +
> + for (i = 0; i < ARRAY_SIZE(drm_named_modes); i++) {
> + const struct drm_named_mode *named_mode = _named_modes[i];
> +
> + if (strcmp(cmd->name, named_mode->name))
> + continue;
> +
> + if (!named_mode->tv_mode)
> + continue;
> +
> + mode = drm_analog_tv_mode(dev,
> + named_mode->tv_mode,
> + named_mode->pixel_clock_khz * 1000,
> + named_mode->xres,
> + named_mode->yres,
> + named_mode->flags &
> DRM_MODE_FLAG_INTERLACE);
> + if (!mode)
> + return NULL;
> +
> + return mode;
> + }
> +
> + return NULL;
> +}
> +
> /**
> * drm_mode_create_from_cmdline_mode - convert a command line modeline into
> a DRM display mode
> * @dev: DRM device to create the new mode for
> @@ -2514,7 +2544,9 @@ drm_mode_create_from_cmdline_mode(struct drm_device
> *dev,
> if (cmd->xres == 0 || cmd->yres == 0)
> return NULL;
>
> - if (cmd->cvt)
> + if (strlen(cmd->name))
> + mode = drm_named_mode(dev, cmd);
I'm trying to track how this generated mode fits into to it all and
AFAICS if the connector already supports a mode with the same xres/yres
as the named mode, the named mode will never be created because of the
check at the beginning of drm_helper_probe_add_cmdline_mode(). It will
just mark the existing mode with USERDEF and return.
If the connector doesn't already support a mode with such a resolution
it will be created, but should we do that? If the driver supported such
a mode it would certainly already have added it to the mode list,
wouldn't it? After all it's just 2 variants NTSC and PAL.
We have this in drm_client_modeset.c:drm_connector_pick_cmdline_mode():
list_for_each_entry(mode, >modes, head) {
/* Check (optional) mode name first */
if (!strcmp(mode->name, cmdline_mode->name))
return mode;
Here it looks like the named mode thing is a way to choose a mode, not
to add one.
I couldn't find any documentation on how named modes is supposed to
work, have you seen any?
Noralf.
> + else if (cmd->cvt)
> mode = drm_cvt_mode(dev,
> cmd->xres, cmd->yres,
> cmd->refresh_specified ? cmd->refresh : 60,
Re: [Intel-gfx] [PATCH v4a 00/38] timers: Use timer_shutdown*() before freeing timers
On Sat, 5 Nov 2022 12:36:42 -0400
Steven Rostedt wrote:
> On Sat, 5 Nov 2022 08:59:36 -0700
> Linus Torvalds wrote:
>
> > On Fri, Nov 4, 2022 at 11:01 PM Steven Rostedt wrote:
> >
> > >
> > > Patch 1 fixes an issue with sunrpc/xprt where it incorrectly uses
> > > del_singleshot_timer_sync() for something that is not a oneshot timer. As
> > > this
> > > will be converted to shutdown, this needs to be fixed first.
> >
> > So this is the kind of thing that I would *not* want to get eartly.
>
> So I'll have to break up patch 5 to not update the
> del_singleshot_timer_sync() to a timer_shutdown_sync(), because that
> breaks this code.
>
> Hmm, since that is a functional change, it probably should wait till
> the merge window. I'll move this patch and that part of patch 5 to the
> second part of the series for the merge window.
>
> >
> > I really would want to get just the infrastructure in to let people
> > start doing conversions.
> >
> > And then the "mindlessly obvious patches that are done by scripting
> > and can not possibly matter".
> >
> > The kinds that do not *need* review, because they are mechanical, and
> > that just cause pointless noise for the rest of the patches that *do*
> > want review.
> >
> > Not this kind of thing that is so subtle that you have to explain it.
> > That's not a "scripted patch for no semantic change".
> >
> > So leave the del_singleshot_timer_sync() cases alone, they are
> > irrelevant for the new infrastructure and for the "mindless scripted
> > conversion" patches.
> >
> > > Patches 2-4 changes existing timer_shutdown() functions used locally in
> > > ARM and
> > > some drivers to better namespace names.
> >
> > Ok, these are relevant.
> >
> > > Patch 5 implements the new timer_shutdown() and timer_shutdown_sync()
> > > functions
> > > that disable re-arming the timer after they are called.
> >
> > This is obviously what I'd want early so that people can start doign
> > this in their trees.
>
> But will need to remove the part that it changes del_singleshot_timer_sync().
>
>
> >
> > > Patches 6-28 change all the locations where there's a kfree(),
> > > kfree_rcu(),
> > > kmem_cache_free() and one call_rcu() call where the RCU function frees the
> > > timer (the workqueue patch) in the same function as the
> > > del_timer{,_sync}() is
> > > called on that timer, and there's no extra exit path between the
> > > del_timer and
> > > freeing of the timer.
> >
> > So honestly, I was literally hoping for a "this is the coccinelle
> > script" kind of patch.
>
> The above actual was, but I walked through them manually too, because I
> don't trust my conccinelle skills. All but the call_rcu() one was
> caught by conccinelle. That's why I pointed out the worqueue one. I'll
> remove that from this series.
>
> >
> > Now there seems to be a number of patches here that are actualyl
> > really hard to see that they are "obviously correct" and I can't tell
> > if they are actually scripted or not.
>
> Yes they are. The script that found these were:
>
Julia,
Perhaps you can help me here. I have the following script to find
places that call del_timer*() that need to be converted to
timer_shutdown*() if later on in the same function the timer is being
freed.
> --8<
> @@
> identifier ptr, timer, rfield, slab;
> @@
> (
> - del_timer(>timer);
> + timer_shutdown(>timer);
> |
> - del_timer_sync(>timer);
> + timer_shutdown_sync(>timer);
> )
> ...
> (
> kfree_rcu(ptr, rfield);
> |
> kmem_cache_free(slab, ptr);
> |
> kfree(ptr);
> )
> -->8
>
Above is the code I used. But it gets more than it should, see below.
> So any function that had a del_timer*(>timer) and then that obj
> was freed with kfree(), kfree_rcu() or kmem_cache_free() was updated.
>
> What I did manually was to make sure there was no exit of the routine
> between those two calls. I'm sure coccinelle could do that too, but I'm
> not good enough at it to add that feature.
>
> The reason the patches don't look obvious is because the distance
> between the del_timer() and the free may be quite far. I walked through
> these patches at least 3 times manually to make sure they are all OK.
>
>
> >
> > They don't *look* scripted, but I can't really tell. I looked at the
> > patches with ten lines of context, and I didn't see the immediately
> > following kfree() even in that expanded patch context, so it's fairly
> > far away.
>
> Yes, some are like a 100 lines away.
>
> >
> > Others in the series were *definitely* not scripted, doing clearly
> > manual cleanups:
> >
> > -if (dch->timer.function) {
> > -del_timer(>timer);
> > -dch->timer.function = NULL;
> > -}
> > +timer_shutdown(>timer);
> >
> > so no, this does *not* make me feel "ok, this is all trivial".
>
> Sorry, I'll remove that. It's
Re: [Intel-gfx] [PATCH v4a 00/38] timers: Use timer_shutdown*() before freeing timers
On Sat, 5 Nov 2022 08:59:36 -0700
Linus Torvalds wrote:
> On Fri, Nov 4, 2022 at 11:01 PM Steven Rostedt wrote:
> >
> > Patch 1 fixes an issue with sunrpc/xprt where it incorrectly uses
> > del_singleshot_timer_sync() for something that is not a oneshot timer. As
> > this
> > will be converted to shutdown, this needs to be fixed first.
>
> So this is the kind of thing that I would *not* want to get eartly.
So I'll have to break up patch 5 to not update the
del_singleshot_timer_sync() to a timer_shutdown_sync(), because that
breaks this code.
Hmm, since that is a functional change, it probably should wait till
the merge window. I'll move this patch and that part of patch 5 to the
second part of the series for the merge window.
>
> I really would want to get just the infrastructure in to let people
> start doing conversions.
>
> And then the "mindlessly obvious patches that are done by scripting
> and can not possibly matter".
>
> The kinds that do not *need* review, because they are mechanical, and
> that just cause pointless noise for the rest of the patches that *do*
> want review.
>
> Not this kind of thing that is so subtle that you have to explain it.
> That's not a "scripted patch for no semantic change".
>
> So leave the del_singleshot_timer_sync() cases alone, they are
> irrelevant for the new infrastructure and for the "mindless scripted
> conversion" patches.
>
> > Patches 2-4 changes existing timer_shutdown() functions used locally in ARM
> > and
> > some drivers to better namespace names.
>
> Ok, these are relevant.
>
> > Patch 5 implements the new timer_shutdown() and timer_shutdown_sync()
> > functions
> > that disable re-arming the timer after they are called.
>
> This is obviously what I'd want early so that people can start doign
> this in their trees.
But will need to remove the part that it changes del_singleshot_timer_sync().
>
> > Patches 6-28 change all the locations where there's a kfree(), kfree_rcu(),
> > kmem_cache_free() and one call_rcu() call where the RCU function frees the
> > timer (the workqueue patch) in the same function as the del_timer{,_sync}()
> > is
> > called on that timer, and there's no extra exit path between the del_timer
> > and
> > freeing of the timer.
>
> So honestly, I was literally hoping for a "this is the coccinelle
> script" kind of patch.
The above actual was, but I walked through them manually too, because I
don't trust my conccinelle skills. All but the call_rcu() one was
caught by conccinelle. That's why I pointed out the worqueue one. I'll
remove that from this series.
>
> Now there seems to be a number of patches here that are actualyl
> really hard to see that they are "obviously correct" and I can't tell
> if they are actually scripted or not.
Yes they are. The script that found these were:
--8<
@@
identifier ptr, timer, rfield, slab;
@@
(
- del_timer(>timer);
+ timer_shutdown(>timer);
|
- del_timer_sync(>timer);
+ timer_shutdown_sync(>timer);
)
...
(
kfree_rcu(ptr, rfield);
|
kmem_cache_free(slab, ptr);
|
kfree(ptr);
)
-->8
So any function that had a del_timer*(>timer) and then that obj
was freed with kfree(), kfree_rcu() or kmem_cache_free() was updated.
What I did manually was to make sure there was no exit of the routine
between those two calls. I'm sure coccinelle could do that too, but I'm
not good enough at it to add that feature.
The reason the patches don't look obvious is because the distance
between the del_timer() and the free may be quite far. I walked through
these patches at least 3 times manually to make sure they are all OK.
>
> They don't *look* scripted, but I can't really tell. I looked at the
> patches with ten lines of context, and I didn't see the immediately
> following kfree() even in that expanded patch context, so it's fairly
> far away.
Yes, some are like a 100 lines away.
>
> Others in the series were *definitely* not scripted, doing clearly
> manual cleanups:
>
> -if (dch->timer.function) {
> -del_timer(>timer);
> -dch->timer.function = NULL;
> -}
> +timer_shutdown(>timer);
>
> so no, this does *not* make me feel "ok, this is all trivial".
Sorry, I'll remove that. It's basically open-coding the
timer_shutdown() as the way it shuts down the timer is simply by
setting the timer.function to NULL.
>
> IOW, I'd really want *just* the infrastructure and *just* the provably
> trivial stuff. If it wasn't some scripted really obvious thing that
> cannot possibly change anything and that wasn't then edited manually
> for some reason, I really don't want it early.
>
> IOW, any early conversions I'd take are literally about removing pure
> mindless noise. Not about doing conversions.
>
> And I wouldn't mind it as a single conversion patch that has the
> coccinelle script as the explanation.
I'll need to
Re: [Intel-gfx] [PATCH v4a 00/38] timers: Use timer_shutdown*() before freeing timers
On Fri, Nov 4, 2022 at 11:01 PM Steven Rostedt wrote:
>
> Patch 1 fixes an issue with sunrpc/xprt where it incorrectly uses
> del_singleshot_timer_sync() for something that is not a oneshot timer. As this
> will be converted to shutdown, this needs to be fixed first.
So this is the kind of thing that I would *not* want to get eartly.
I really would want to get just the infrastructure in to let people
start doing conversions.
And then the "mindlessly obvious patches that are done by scripting
and can not possibly matter".
The kinds that do not *need* review, because they are mechanical, and
that just cause pointless noise for the rest of the patches that *do*
want review.
Not this kind of thing that is so subtle that you have to explain it.
That's not a "scripted patch for no semantic change".
So leave the del_singleshot_timer_sync() cases alone, they are
irrelevant for the new infrastructure and for the "mindless scripted
conversion" patches.
> Patches 2-4 changes existing timer_shutdown() functions used locally in ARM
> and
> some drivers to better namespace names.
Ok, these are relevant.
> Patch 5 implements the new timer_shutdown() and timer_shutdown_sync()
> functions
> that disable re-arming the timer after they are called.
This is obviously what I'd want early so that people can start doign
this in their trees.
> Patches 6-28 change all the locations where there's a kfree(), kfree_rcu(),
> kmem_cache_free() and one call_rcu() call where the RCU function frees the
> timer (the workqueue patch) in the same function as the del_timer{,_sync}() is
> called on that timer, and there's no extra exit path between the del_timer and
> freeing of the timer.
So honestly, I was literally hoping for a "this is the coccinelle
script" kind of patch.
Now there seems to be a number of patches here that are actualyl
really hard to see that they are "obviously correct" and I can't tell
if they are actually scripted or not.
They don't *look* scripted, but I can't really tell. I looked at the
patches with ten lines of context, and I didn't see the immediately
following kfree() even in that expanded patch context, so it's fairly
far away.
Others in the series were *definitely* not scripted, doing clearly
manual cleanups:
-if (dch->timer.function) {
-del_timer(>timer);
-dch->timer.function = NULL;
-}
+timer_shutdown(>timer);
so no, this does *not* make me feel "ok, this is all trivial".
IOW, I'd really want *just* the infrastructure and *just* the provably
trivial stuff. If it wasn't some scripted really obvious thing that
cannot possibly change anything and that wasn't then edited manually
for some reason, I really don't want it early.
IOW, any early conversions I'd take are literally about removing pure
mindless noise. Not about doing conversions.
And I wouldn't mind it as a single conversion patch that has the
coccinelle script as the explanation.
Really just THAT kind of "100% mindless conversion".
Linus
[Intel-gfx] BUG: i915: flickering/temporary artifacts after resume
Hi, I have a HP Pavilion 15" laptop that occasionally misbehaves after a resume from suspend mode. The problem is obvious when the screen updates e.g. after moving the mouse and the window focus changing, or when a terminal scrolls, I get a ficker of random short horizontal white lines over the top of the windows that then disappear. These appear to be predominantly focused towards the top of the screen, although they do also occur lower down but less obviously. Soemtimes these artifacts don't disappear until the next update - I attempted to capture them, but of course that provokes another screen update and they disappear. When this problem occurs, suspending and resuming again doesn't appear to fix the issue - only a reboot does. Environment: 00:02.0 VGA compatible controller: Intel Corporation HD Graphics 620 (rev 02) (prog-if 00 [VGA controller]) DeviceName: Intel Kabylake HD Graphics ULT GT2 Subsystem: Hewlett-Packard Company HD Graphics 620 Flags: bus master, fast devsel, latency 0, IRQ 130, IOMMU group 1 Memory at a000 (64-bit, non-prefetchable) [size=16M] Memory at 9000 (64-bit, prefetchable) [size=256M] I/O ports at 4000 [size=64] Expansion ROM at 000c [virtual] [disabled] [size=128K] Capabilities: [40] Vendor Specific Information: Len=0c Capabilities: [70] Express Root Complex Integrated Endpoint, MSI 00 Capabilities: [ac] MSI: Enable+ Count=1/1 Maskable- 64bit- Capabilities: [d0] Power Management version 2 Capabilities: [100] Process Address Space ID (PASID) Capabilities: [200] Address Translation Service (ATS) Capabilities: [300] Page Request Interface (PRI) Kernel driver in use: i915 Kernel modules: i915 Debian Bullseye (stable), Xorg 1.20.11, libdrm 2.4.104, intel xorg driver 2.99.917+git20200714. Kernel messages related to DRM: Linux version 5.10.0-19-amd64 (debian-ker...@lists.debian.org) (gcc-10 (Debian 1 0.2.1-6) 10.2.1 20210110, GNU ld (GNU Binutils for Debian) 2.35.2) #1 SMP Debian 5.10.149-1 (2022-10-17) ... i915 :00:02.0: [drm] VT-d active for gfx access checking generic (9000 408000) vs hw (a000 100) checking generic (9000 408000) vs hw (9000 1000) fb0: switching to inteldrmfb from EFI VGA Console: switching to colour dummy device 80x25 i915 :00:02.0: vgaarb: deactivate vga console i915 :00:02.0: vgaarb: changed VGA decodes: olddecodes=io+mem,decodes=io+mem:owns=io+mem i915 :00:02.0: firmware: direct-loading firmware i915/kbl_dmc_ver1_04.bin i915 :00:02.0: [drm] Finished loading DMC firmware i915/kbl_dmc_ver1_04.bin (v1.4) [drm] Initialized i915 1.6.0 20200917 for :00:02.0 on minor 0 ACPI: Video Device [GFX0] (multi-head: yes rom: no post: no) input: Video Bus as /devices/LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00/LNXVIDEO:00/input/input6 fbcon: i915drmfb (fb0) is primary device Console: switching to colour frame buffer device 170x48 i915 :00:02.0: [drm] fb0: i915drmfb frame buffer device mei_hdcp :00:16.0-b638ab7e-94e2-4ea2-a552-d1c54b627f04: bound :00:02.0 (ops i915_hdcp_component_ops [i915]) snd_hda_intel :00:1f.3: bound :00:02.0 (ops i915_audio_component_bind_ops [i915]) (NULL device *): firmware: direct-loading firmware i915/kbl_dmc_ver1_04.bin mei_hdcp :00:16.0-b638ab7e-94e2-4ea2-a552-d1c54b627f04: bound :00:02.0 ( ops i915_hdcp_component_ops [i915]) The last two lines repeat each time the system is suspended/resumed. No errors or warnings appear to be logged either from the kernel nor in the Xorg log file specific to i915 (there's the usual Xorg whinging about the system being too slow... so an i5-7200U 2.5GHz isn't fast enough for Xorg!) It feels like some setting within the Intel GPU hardware that controls memory access timing isn't being properly restored. Is this a known issue? Thanks. Russell. -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
Re: [Intel-gfx] [PATCH v4a 00/38] timers: Use timer_shutdown*() before freeing timers
On Sat, 5 Nov 2022 07:18:17 -0700 Guenter Roeck wrote: > Just in case you didn't notice: > > Looking through the resulting code, I think some of the remaining > calls to del_singleshot_timer_sync() can be converted as well. > > The calls in drivers/staging/wlan-ng/prism2usb.c:prism2sta_disconnect_usb() > are obvious (the containing data structure is freed in the same function). > For drivers/char/tpm/tpm-dev-common.c:tpm_common_release(), the containing > data structure is freed in the calling code. Well, actually it is. In patch 5/38: -#define del_singleshot_timer_sync(t) del_timer_sync(t) +#define del_singleshot_timer_sync(t) timer_shutdown_sync(t) This was the reason for patch 1. It was the only user of that function that reused the timer after calling that function. -- Steve
Re: [Intel-gfx] [PATCH v4a 00/38] timers: Use timer_shutdown*() before freeing timers
On Sat, Nov 05, 2022 at 02:00:24AM -0400, Steven Rostedt wrote: > > Back in April, I posted an RFC patch set to help mitigate a common issue > where a timer gets armed just before it is freed, and when the timer > goes off, it crashes in the timer code without any evidence of who the > culprit was. I got side tracked and never finished up on that patch set. > Since this type of crash is still our #1 crash we are seeing in the field, > it has become a priority again to finish it. > > The last version of that patch set is here: > > https://lore.kernel.org/all/20221104054053.431922...@goodmis.org/ > > I'm calling this version 4a as it only has obvious changes were the timer that > is being shutdown is in the same function where it will be freed or released, > as this series should be "safe" for adding. I'll be calling the other patches > 4b for the next merge window. > Just in case you didn't notice: Looking through the resulting code, I think some of the remaining calls to del_singleshot_timer_sync() can be converted as well. The calls in drivers/staging/wlan-ng/prism2usb.c:prism2sta_disconnect_usb() are obvious (the containing data structure is freed in the same function). For drivers/char/tpm/tpm-dev-common.c:tpm_common_release(), the containing data structure is freed in the calling code. Thanks, Guenter
Re: [Intel-gfx] [PATCH v6 09/23] drm/modes: Switch to named mode descriptors
Den 26.10.2022 17.33, skrev max...@cerno.tech: > The current named mode parsing relies only the mode name, and doesn't allow only the -> only on the > to specify any other parameter. > > Let's convert that string list to an array of a custom structure that will > hold the name and some additional parameters in the future. > > Signed-off-by: Maxime Ripard > --- Reviewed-by: Noralf Trønnes
Re: [Intel-gfx] [PATCH v6 08/23] drm/modes: Move named modes parsing to a separate function
Den 26.10.2022 17.33, skrev max...@cerno.tech: > The current construction of the named mode parsing doesn't allow to extend > it easily. Let's move it to a separate function so we can add more > parameters and modes. > > In order for the tests to still pass, some extra checks are needed, so > it's not a 1:1 move. > > Signed-off-by: Maxime Ripard > > --- Reviewed-by: Noralf Trønnes
[Intel-gfx] ✗ Fi.CI.IGT: failure for Fix live busy stats selftest failure
== Series Details == Series: Fix live busy stats selftest failure URL : https://patchwork.freedesktop.org/series/110557/ State : failure == Summary == CI Bug Log - changes from CI_DRM_12346_full -> Patchwork_110557v1_full Summary --- **FAILURE** Serious unknown changes coming with Patchwork_110557v1_full absolutely need to be verified manually. If you think the reported changes have nothing to do with the changes introduced in Patchwork_110557v1_full, please notify your bug team to allow them to document this new failure mode, which will reduce false positives in CI. Participating hosts (10 -> 9) -- Missing(1): shard-dg1 Possible new issues --- Here are the unknown changes that may have been introduced in Patchwork_110557v1_full: ### IGT changes ### Possible regressions * igt@api_intel_allocator@fork-simple-stress: - shard-tglb: [PASS][1] -> [INCOMPLETE][2] [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12346/shard-tglb7/igt@api_intel_alloca...@fork-simple-stress.html [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110557v1/shard-tglb5/igt@api_intel_alloca...@fork-simple-stress.html * igt@gem_eio@reset-stress: - shard-snb: [PASS][3] -> [INCOMPLETE][4] [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12346/shard-snb7/igt@gem_...@reset-stress.html [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110557v1/shard-snb6/igt@gem_...@reset-stress.html * igt@kms_plane_scaling@plane-upscale-with-modifiers-20x20@pipe-a-dp-1: - shard-apl: [PASS][5] -> [DMESG-WARN][6] [5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12346/shard-apl7/igt@kms_plane_scaling@plane-upscale-with-modifiers-20...@pipe-a-dp-1.html [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110557v1/shard-apl3/igt@kms_plane_scaling@plane-upscale-with-modifiers-20...@pipe-a-dp-1.html Known issues Here are the changes found in Patchwork_110557v1_full that come from known issues: ### CI changes ### Possible fixes * boot: - shard-glk: ([PASS][7], [PASS][8], [FAIL][9], [PASS][10], [PASS][11], [PASS][12], [PASS][13], [PASS][14], [PASS][15], [PASS][16], [PASS][17], [PASS][18], [PASS][19], [PASS][20], [PASS][21], [PASS][22], [PASS][23], [PASS][24], [PASS][25], [PASS][26], [PASS][27], [PASS][28], [PASS][29], [PASS][30], [PASS][31]) ([i915#4392]) -> ([PASS][32], [PASS][33], [PASS][34], [PASS][35], [PASS][36], [PASS][37], [PASS][38], [PASS][39], [PASS][40], [PASS][41], [PASS][42], [PASS][43], [PASS][44], [PASS][45], [PASS][46], [PASS][47], [PASS][48], [PASS][49], [PASS][50], [PASS][51], [PASS][52], [PASS][53], [PASS][54], [PASS][55], [PASS][56]) [7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12346/shard-glk9/boot.html [8]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12346/shard-glk9/boot.html [9]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12346/shard-glk9/boot.html [10]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12346/shard-glk9/boot.html [11]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12346/shard-glk1/boot.html [12]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12346/shard-glk1/boot.html [13]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12346/shard-glk1/boot.html [14]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12346/shard-glk2/boot.html [15]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12346/shard-glk2/boot.html [16]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12346/shard-glk2/boot.html [17]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12346/shard-glk3/boot.html [18]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12346/shard-glk3/boot.html [19]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12346/shard-glk3/boot.html [20]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12346/shard-glk5/boot.html [21]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12346/shard-glk5/boot.html [22]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12346/shard-glk5/boot.html [23]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12346/shard-glk6/boot.html [24]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12346/shard-glk6/boot.html [25]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12346/shard-glk7/boot.html [26]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12346/shard-glk7/boot.html [27]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12346/shard-glk7/boot.html [28]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12346/shard-glk8/boot.html [29]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12346/shard-glk8/boot.html [30]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12346/shard-glk8/boot.html [31]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12346/shard-glk8/boot.html [32]:
[Intel-gfx] ✗ Fi.CI.IGT: failure for series starting with [1/2] drm/i915/display: Do both crawl and squash when changing cdclk
== Series Details == Series: series starting with [1/2] drm/i915/display: Do both crawl and squash when changing cdclk URL : https://patchwork.freedesktop.org/series/110554/ State : failure == Summary == CI Bug Log - changes from CI_DRM_12346_full -> Patchwork_110554v1_full Summary --- **FAILURE** Serious unknown changes coming with Patchwork_110554v1_full absolutely need to be verified manually. If you think the reported changes have nothing to do with the changes introduced in Patchwork_110554v1_full, please notify your bug team to allow them to document this new failure mode, which will reduce false positives in CI. Participating hosts (10 -> 9) -- Missing(1): shard-dg1 Possible new issues --- Here are the unknown changes that may have been introduced in Patchwork_110554v1_full: ### IGT changes ### Possible regressions * igt@kms_plane_scaling@plane-upscale-with-modifiers-20x20@pipe-a-dp-1: - shard-apl: [PASS][1] -> [DMESG-WARN][2] [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12346/shard-apl7/igt@kms_plane_scaling@plane-upscale-with-modifiers-20...@pipe-a-dp-1.html [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110554v1/shard-apl2/igt@kms_plane_scaling@plane-upscale-with-modifiers-20...@pipe-a-dp-1.html Known issues Here are the changes found in Patchwork_110554v1_full that come from known issues: ### CI changes ### Issues hit * boot: - shard-skl: ([PASS][3], [PASS][4], [PASS][5], [PASS][6], [PASS][7], [PASS][8], [PASS][9], [PASS][10], [PASS][11], [PASS][12], [PASS][13], [PASS][14], [PASS][15], [PASS][16], [PASS][17], [PASS][18]) -> ([PASS][19], [PASS][20], [PASS][21], [FAIL][22], [PASS][23], [PASS][24], [PASS][25], [PASS][26], [PASS][27], [PASS][28], [PASS][29], [PASS][30], [PASS][31], [PASS][32]) ([i915#5032]) [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12346/shard-skl4/boot.html [4]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12346/shard-skl10/boot.html [5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12346/shard-skl9/boot.html [6]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12346/shard-skl9/boot.html [7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12346/shard-skl9/boot.html [8]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12346/shard-skl9/boot.html [9]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12346/shard-skl7/boot.html [10]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12346/shard-skl7/boot.html [11]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12346/shard-skl7/boot.html [12]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12346/shard-skl6/boot.html [13]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12346/shard-skl6/boot.html [14]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12346/shard-skl4/boot.html [15]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12346/shard-skl4/boot.html [16]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12346/shard-skl3/boot.html [17]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12346/shard-skl3/boot.html [18]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12346/shard-skl10/boot.html [19]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110554v1/shard-skl10/boot.html [20]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110554v1/shard-skl10/boot.html [21]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110554v1/shard-skl10/boot.html [22]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110554v1/shard-skl3/boot.html [23]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110554v1/shard-skl3/boot.html [24]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110554v1/shard-skl4/boot.html [25]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110554v1/shard-skl4/boot.html [26]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110554v1/shard-skl6/boot.html [27]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110554v1/shard-skl6/boot.html [28]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110554v1/shard-skl6/boot.html [29]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110554v1/shard-skl7/boot.html [30]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110554v1/shard-skl7/boot.html [31]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110554v1/shard-skl9/boot.html [32]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110554v1/shard-skl9/boot.html Possible fixes * boot: - shard-glk: ([PASS][33], [PASS][34], [FAIL][35], [PASS][36], [PASS][37], [PASS][38], [PASS][39], [PASS][40], [PASS][41], [PASS][42], [PASS][43], [PASS][44], [PASS][45], [PASS][46], [PASS][47], [PASS][48], [PASS][49], [PASS][50], [PASS][51], [PASS][52], [PASS][53], [PASS][54], [PASS][55], [PASS][56], [PASS][57])
Re: [Intel-gfx] [PATCH v4a 31/38] timers: drm: Use timer_shutdown_sync() for on stack timers
Den 05.11.2022 07.00, skrev Steven Rostedt: > From: "Steven Rostedt (Google)" > > Before a timer is released, timer_shutdown_sync() must be called. > > Link: https://lore.kernel.org/all/20221104054053.431922...@goodmis.org/ > > Cc: "Noralf Trønnes" > Cc: David Airlie > Cc: Daniel Vetter > Cc: Jani Nikula > Cc: Joonas Lahtinen > Cc: Rodrigo Vivi > Cc: Tvrtko Ursulin > Cc: dri-de...@lists.freedesktop.org > Cc: intel-gfx@lists.freedesktop.org > Signed-off-by: Steven Rostedt (Google) > --- Acked-by: Noralf Trønnes > drivers/gpu/drm/gud/gud_pipe.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/gud/gud_pipe.c b/drivers/gpu/drm/gud/gud_pipe.c > index 7c6dc2bcd14a..08429bdd57cf 100644 > --- a/drivers/gpu/drm/gud/gud_pipe.c > +++ b/drivers/gpu/drm/gud/gud_pipe.c > @@ -272,7 +272,7 @@ static int gud_usb_bulk(struct gud_device *gdrm, size_t > len) > > usb_sg_wait(); > > - if (!del_timer_sync()) > + if (!timer_shutdown_sync()) > ret = -ETIMEDOUT; > else if (ctx.sgr.status < 0) > ret = ctx.sgr.status;
[Intel-gfx] ✗ Fi.CI.IGT: failure for Add GT oriented dmesg output
== Series Details == Series: Add GT oriented dmesg output URL : https://patchwork.freedesktop.org/series/110550/ State : failure == Summary == CI Bug Log - changes from CI_DRM_12344_full -> Patchwork_110550v1_full Summary --- **FAILURE** Serious unknown changes coming with Patchwork_110550v1_full absolutely need to be verified manually. If you think the reported changes have nothing to do with the changes introduced in Patchwork_110550v1_full, please notify your bug team to allow them to document this new failure mode, which will reduce false positives in CI. Participating hosts (11 -> 9) -- Missing(2): shard-rkl shard-dg1 Possible new issues --- Here are the unknown changes that may have been introduced in Patchwork_110550v1_full: ### IGT changes ### Possible regressions * igt@gem_mmap_gtt@fault-concurrent-x: - shard-snb: [PASS][1] -> [CRASH][2] [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12344/shard-snb5/igt@gem_mmap_...@fault-concurrent-x.html [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110550v1/shard-snb6/igt@gem_mmap_...@fault-concurrent-x.html * igt@i915_suspend@fence-restore-tiled2untiled: - shard-skl: [PASS][3] -> [INCOMPLETE][4] [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12344/shard-skl7/igt@i915_susp...@fence-restore-tiled2untiled.html [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110550v1/shard-skl9/igt@i915_susp...@fence-restore-tiled2untiled.html * igt@kms_plane_scaling@plane-upscale-with-modifiers-20x20@pipe-a-dp-1: - shard-apl: [PASS][5] -> [DMESG-WARN][6] [5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12344/shard-apl8/igt@kms_plane_scaling@plane-upscale-with-modifiers-20...@pipe-a-dp-1.html [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110550v1/shard-apl8/igt@kms_plane_scaling@plane-upscale-with-modifiers-20...@pipe-a-dp-1.html Known issues Here are the changes found in Patchwork_110550v1_full that come from known issues: ### CI changes ### Issues hit * boot: - shard-apl: ([PASS][7], [PASS][8], [PASS][9], [PASS][10], [PASS][11], [PASS][12], [PASS][13], [PASS][14], [PASS][15], [PASS][16], [PASS][17], [PASS][18], [PASS][19], [PASS][20], [PASS][21], [PASS][22], [PASS][23], [PASS][24], [PASS][25], [PASS][26], [PASS][27], [PASS][28], [PASS][29], [PASS][30], [PASS][31]) -> ([PASS][32], [PASS][33], [PASS][34], [PASS][35], [PASS][36], [PASS][37], [PASS][38], [PASS][39], [PASS][40], [PASS][41], [PASS][42], [PASS][43], [PASS][44], [PASS][45], [PASS][46], [PASS][47], [PASS][48], [PASS][49], [PASS][50], [PASS][51], [FAIL][52], [PASS][53], [PASS][54], [PASS][55], [PASS][56]) ([i915#4386]) [7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12344/shard-apl1/boot.html [8]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12344/shard-apl1/boot.html [9]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12344/shard-apl1/boot.html [10]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12344/shard-apl1/boot.html [11]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12344/shard-apl2/boot.html [12]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12344/shard-apl2/boot.html [13]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12344/shard-apl2/boot.html [14]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12344/shard-apl2/boot.html [15]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12344/shard-apl3/boot.html [16]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12344/shard-apl3/boot.html [17]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12344/shard-apl3/boot.html [18]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12344/shard-apl6/boot.html [19]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12344/shard-apl6/boot.html [20]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12344/shard-apl6/boot.html [21]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12344/shard-apl6/boot.html [22]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12344/shard-apl6/boot.html [23]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12344/shard-apl7/boot.html [24]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12344/shard-apl7/boot.html [25]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12344/shard-apl7/boot.html [26]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12344/shard-apl7/boot.html [27]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12344/shard-apl8/boot.html [28]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12344/shard-apl8/boot.html [29]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12344/shard-apl8/boot.html [30]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12344/shard-apl8/boot.html [31]:
[Intel-gfx] ✗ Fi.CI.IGT: failure for vfio-ccw parent rework (rev3)
== Series Details == Series: vfio-ccw parent rework (rev3) URL : https://patchwork.freedesktop.org/series/109899/ State : failure == Summary == CI Bug Log - changes from CI_DRM_12344_full -> Patchwork_109899v3_full Summary --- **FAILURE** Serious unknown changes coming with Patchwork_109899v3_full absolutely need to be verified manually. If you think the reported changes have nothing to do with the changes introduced in Patchwork_109899v3_full, please notify your bug team to allow them to document this new failure mode, which will reduce false positives in CI. Participating hosts (11 -> 9) -- Missing(2): shard-rkl shard-dg1 Possible new issues --- Here are the unknown changes that may have been introduced in Patchwork_109899v3_full: ### IGT changes ### Possible regressions * igt@gem_mmap_gtt@flink-race: - shard-skl: NOTRUN -> [INCOMPLETE][1] [1]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_109899v3/shard-skl3/igt@gem_mmap_...@flink-race.html Known issues Here are the changes found in Patchwork_109899v3_full that come from known issues: ### CI changes ### Issues hit * boot: - shard-apl: ([PASS][2], [PASS][3], [PASS][4], [PASS][5], [PASS][6], [PASS][7], [PASS][8], [PASS][9], [PASS][10], [PASS][11], [PASS][12], [PASS][13], [PASS][14], [PASS][15], [PASS][16], [PASS][17], [PASS][18], [PASS][19], [PASS][20], [PASS][21], [PASS][22], [PASS][23], [PASS][24], [PASS][25], [PASS][26]) -> ([FAIL][27], [PASS][28], [PASS][29], [PASS][30], [PASS][31], [PASS][32], [PASS][33], [PASS][34], [PASS][35], [PASS][36], [PASS][37], [PASS][38], [PASS][39], [PASS][40], [PASS][41], [PASS][42], [PASS][43], [PASS][44], [PASS][45], [PASS][46], [PASS][47], [PASS][48], [PASS][49], [PASS][50], [PASS][51]) ([i915#4386]) [2]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12344/shard-apl1/boot.html [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12344/shard-apl1/boot.html [4]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12344/shard-apl1/boot.html [5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12344/shard-apl1/boot.html [6]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12344/shard-apl2/boot.html [7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12344/shard-apl2/boot.html [8]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12344/shard-apl2/boot.html [9]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12344/shard-apl2/boot.html [10]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12344/shard-apl3/boot.html [11]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12344/shard-apl3/boot.html [12]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12344/shard-apl3/boot.html [13]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12344/shard-apl6/boot.html [14]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12344/shard-apl6/boot.html [15]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12344/shard-apl6/boot.html [16]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12344/shard-apl6/boot.html [17]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12344/shard-apl6/boot.html [18]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12344/shard-apl7/boot.html [19]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12344/shard-apl7/boot.html [20]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12344/shard-apl7/boot.html [21]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12344/shard-apl7/boot.html [22]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12344/shard-apl8/boot.html [23]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12344/shard-apl8/boot.html [24]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12344/shard-apl8/boot.html [25]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12344/shard-apl8/boot.html [26]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12344/shard-apl8/boot.html [27]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_109899v3/shard-apl2/boot.html [28]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_109899v3/shard-apl2/boot.html [29]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_109899v3/shard-apl2/boot.html [30]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_109899v3/shard-apl2/boot.html [31]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_109899v3/shard-apl3/boot.html [32]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_109899v3/shard-apl1/boot.html [33]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_109899v3/shard-apl3/boot.html [34]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_109899v3/shard-apl3/boot.html [35]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_109899v3/shard-apl3/boot.html [36]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_109899v3/shard-apl3/boot.html [37]:
[Intel-gfx] [PATCH v4a 11/38] timers: drm: Use timer_shutdown_sync() before freeing timer
From: "Steven Rostedt (Google)" Before a timer is freed, timer_shutdown_sync() must be called. Link: https://lore.kernel.org/all/20221104054053.431922...@goodmis.org/ Cc: "Noralf Trønnes" Cc: David Airlie Cc: Daniel Vetter Cc: Jani Nikula Cc: Joonas Lahtinen Cc: Rodrigo Vivi Cc: Tvrtko Ursulin Cc: dri-de...@lists.freedesktop.org Cc: intel-gfx@lists.freedesktop.org Signed-off-by: Steven Rostedt (Google) --- drivers/gpu/drm/i915/i915_sw_fence.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/gpu/drm/i915/i915_sw_fence.c b/drivers/gpu/drm/i915/i915_sw_fence.c index 6fc0d1b89690..bfaa9a67dc35 100644 --- a/drivers/gpu/drm/i915/i915_sw_fence.c +++ b/drivers/gpu/drm/i915/i915_sw_fence.c @@ -465,7 +465,7 @@ static void irq_i915_sw_fence_work(struct irq_work *wrk) struct i915_sw_dma_fence_cb_timer *cb = container_of(wrk, typeof(*cb), work); - del_timer_sync(>timer); + timer_shutdown_sync(>timer); dma_fence_put(cb->dma); kfree_rcu(cb, rcu); -- 2.35.1
[Intel-gfx] [PATCH v4a 31/38] timers: drm: Use timer_shutdown_sync() for on stack timers
From: "Steven Rostedt (Google)" Before a timer is released, timer_shutdown_sync() must be called. Link: https://lore.kernel.org/all/20221104054053.431922...@goodmis.org/ Cc: "Noralf Trønnes" Cc: David Airlie Cc: Daniel Vetter Cc: Jani Nikula Cc: Joonas Lahtinen Cc: Rodrigo Vivi Cc: Tvrtko Ursulin Cc: dri-de...@lists.freedesktop.org Cc: intel-gfx@lists.freedesktop.org Signed-off-by: Steven Rostedt (Google) --- drivers/gpu/drm/gud/gud_pipe.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/gpu/drm/gud/gud_pipe.c b/drivers/gpu/drm/gud/gud_pipe.c index 7c6dc2bcd14a..08429bdd57cf 100644 --- a/drivers/gpu/drm/gud/gud_pipe.c +++ b/drivers/gpu/drm/gud/gud_pipe.c @@ -272,7 +272,7 @@ static int gud_usb_bulk(struct gud_device *gdrm, size_t len) usb_sg_wait(); - if (!del_timer_sync()) + if (!timer_shutdown_sync()) ret = -ETIMEDOUT; else if (ctx.sgr.status < 0) ret = ctx.sgr.status; -- 2.35.1
[Intel-gfx] [PATCH v4a 00/38] timers: Use timer_shutdown*() before freeing timers
Back in April, I posted an RFC patch set to help mitigate a common issue
where a timer gets armed just before it is freed, and when the timer
goes off, it crashes in the timer code without any evidence of who the
culprit was. I got side tracked and never finished up on that patch set.
Since this type of crash is still our #1 crash we are seeing in the field,
it has become a priority again to finish it.
The last version of that patch set is here:
https://lore.kernel.org/all/20221104054053.431922...@goodmis.org/
I'm calling this version 4a as it only has obvious changes were the timer that
is being shutdown is in the same function where it will be freed or released,
as this series should be "safe" for adding. I'll be calling the other patches
4b for the next merge window.
Patch 1 fixes an issue with sunrpc/xprt where it incorrectly uses
del_singleshot_timer_sync() for something that is not a oneshot timer. As this
will be converted to shutdown, this needs to be fixed first.
Patches 2-4 changes existing timer_shutdown() functions used locally in ARM and
some drivers to better namespace names.
Patch 5 implements the new timer_shutdown() and timer_shutdown_sync() functions
that disable re-arming the timer after they are called.
Patches 6-28 change all the locations where there's a kfree(), kfree_rcu(),
kmem_cache_free() and one call_rcu() call where the RCU function frees the
timer (the workqueue patch) in the same function as the del_timer{,_sync}() is
called on that timer, and there's no extra exit path between the del_timer and
freeing of the timer.
Patches 29-32 add timer_shutdown*() on on-stack timers that are about to be
released at the end of the function.
Patches 33-37 add timer_shutdown*() on module timers in the module exit code.
Patch 38 simply converts an open coded "shutdown" code into timer_shutdown(),
as a way timer_shutdown() disables the timer is by setting that timer function
to NULL.
Linus, I sorted the patches this way to let you see which you would think is
safe to go into this -rc. I honestly believe that they are all safe, but that's
just my own opinion.
This series is here:
git://git.kernel.org/pub/scm/linux/kernel/git/rostedt/linux-trace.git
timers-start
Head SHA1: f58b516a65bac76f1bfa00126856d6c6c3d24a40
Steven Rostedt (Google) (38):
SUNRPC/xprt: Use del_timer_sync() instead of del_singleshot_timer_sync()
ARM: spear: Do not use timer namespace for timer_shutdown() function
clocksource/drivers/arm_arch_timer: Do not use timer namespace for
timer_shutdown() function
clocksource/drivers/sp804: Do not use timer namespace for
timer_shutdown() function
timers: Add timer_shutdown_sync() and timer_shutdown() to be called
before freeing timers
timers: sh: Use timer_shutdown_sync() before freeing timer
timers: block: Use timer_shutdown_sync() before freeing timer
timers: ACPI: Use timer_shutdown_sync() before freeing timer
timers: atm: Use timer_shutdown_sync() before freeing timer
timers: Bluetooth: Use timer_shutdown_sync() before freeing timer
timers: drm: Use timer_shutdown_sync() before freeing timer
timers: HID: Use timer_shutdown_sync() before freeing timer
timers: Input: Use timer_shutdown_sync() before freeing timer
timers: mISDN: Use timer_shutdown_sync() before freeing timer
timers: leds: Use timer_shutdown_sync() before freeing timer
timers: media: Use timer_shutdown_sync() before freeing timer
timers: net: Use timer_shutdown_sync() before freeing timer
timers: usb: Use timer_shutdown_sync() before freeing timer
timers: nfc: pn533: Use timer_shutdown_sync() before freeing timer
timers: pcmcia: Use timer_shutdown_sync() before freeing timer
timers: scsi: Use timer_shutdown_sync() and timer_shutdown() before
freeing timer
timers: tty: Use timer_shutdown_sync() before freeing timer
timers: ext4: Use timer_shutdown_sync() before freeing timer
timers: fs/nilfs2: Use timer_shutdown_sync() before freeing timer
timers: ALSA: Use timer_shutdown_sync() before freeing timer
timers: jbd2: Use timer_shutdown() before freeing timer
timers: sched/psi: Use timer_shutdown_sync() before freeing timer
timers: workqueue: Use timer_shutdown_sync() before freeing timer
random: use timer_shutdown_sync() for on stack timers
timers: dma-buf: Use timer_shutdown_sync() for on stack timers
timers: drm: Use timer_shutdown_sync() for on stack timers
timers: media: Use timer_shutdown_sync() for on stack timers
timers: s390/cmm: Use timer_shutdown_sync() before a module is released
timers: atm: Use timer_shutdown_sync() before a module is released
timers: hangcheck: Use timer_shutdown_sync() before a module is released
timers: ipmi: Use timer_shutdown_sync() before a module is released
timers: Input: Use timer_shutdown_sync() before a module is released
timers: PM:
