| Message ID | 20230605131135.396854-1-tvrtko.ursulin@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915/selftests: Add some missing error propagation | expand |
On Mon, Jun 05, 2023 at 02:11:35PM +0100, Tvrtko Ursulin wrote: > From: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > > Add some missing error propagation in live_parallel_switch. > > To avoid needlessly burdening the various backport processes, note I am > not marking it as a fix against any patches and not copying stable since > it is debug/selftests only code. > This patch is unlikely to make a difference in real life, but I don't think avoiding Fixes tags and backports is the right thing. I would add a Fixes tag and not add a stable tag. The real burden with Fixes tags is if it breaks someone's system. But if it's breaking selftests then hopefully those are the people best able to deal with it. Fixes tags are different from stable tags. If the code is very recent then the fixes tag can automatically allow us to filter out that patch from going back to stable. So for new patches Fixes is the opposite of CC'ing stable. If the bug is old, then adding a Fixes tag does increase the chance of a backport though, that's true. My guess is that if the stable maintainers thought that selftests/ were causing too much issue with backports they would add a grep line to their scripts to solve that problem. Instead we were having the opposite discussion the other week where the bpf people didn't want to backport selftest stuff and Greg wanted to. https://lore.kernel.org/all/2023052647-tacking-wince-85c5@gregkh/ regards, dan carpenter
Hi Tvrtko, > Add some missing error propagation in live_parallel_switch. > > To avoid needlessly burdening the various backport processes, note I am > not marking it as a fix against any patches and not copying stable since > it is debug/selftests only code. which I did :/ But I guess you are right and it's not necessary. > Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > Reported-by: Dan Carpenter <dan.carpenter@linaro.org> > Cc: Andi Shyti <andi.shyti@linux.intel.com> > --- > .../gpu/drm/i915/gem/selftests/i915_gem_context.c | 14 ++++++++++---- > 1 file changed, 10 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/i915/gem/selftests/i915_gem_context.c b/drivers/gpu/drm/i915/gem/selftests/i915_gem_context.c > index ad6a3b2fb387..7021b6e9b219 100644 > --- a/drivers/gpu/drm/i915/gem/selftests/i915_gem_context.c > +++ b/drivers/gpu/drm/i915/gem/selftests/i915_gem_context.c > @@ -348,8 +348,10 @@ static int live_parallel_switch(void *arg) > continue; > > ce = intel_context_create(data[m].ce[0]->engine); > - if (IS_ERR(ce)) > + if (IS_ERR(ce)) { > + err = PTR_ERR(ce); > goto out; > + } > > err = intel_context_pin(ce); > if (err) { > @@ -369,8 +371,10 @@ static int live_parallel_switch(void *arg) > > worker = kthread_create_worker(0, "igt/parallel:%s", > data[n].ce[0]->engine->name); > - if (IS_ERR(worker)) > + if (IS_ERR(worker)) { > + err = PTR_ERR(worker); > goto out; > + } > > data[n].worker = worker; > } > @@ -399,8 +403,10 @@ static int live_parallel_switch(void *arg) > } > } > > - if (igt_live_test_end(&t)) > - err = -EIO; > + if (igt_live_test_end(&t)) { > + err = err ?: -EIO; Nice catch! Reviewed-by: Andi Shyti <andi.shyti@linux.intel.com> Andi > + break; > + } > } > > out: > -- > 2.39.2
On 05/06/2023 14:43, Dan Carpenter wrote: > On Mon, Jun 05, 2023 at 02:11:35PM +0100, Tvrtko Ursulin wrote: >> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com> >> >> Add some missing error propagation in live_parallel_switch. >> >> To avoid needlessly burdening the various backport processes, note I am >> not marking it as a fix against any patches and not copying stable since >> it is debug/selftests only code. >> > > This patch is unlikely to make a difference in real life, but I don't > think avoiding Fixes tags and backports is the right thing. > > I would add a Fixes tag and not add a stable tag. > > The real burden with Fixes tags is if it breaks someone's system. But > if it's breaking selftests then hopefully those are the people best > able to deal with it. > > Fixes tags are different from stable tags. If the code is very recent > then the fixes tag can automatically allow us to filter out that patch > from going back to stable. So for new patches Fixes is the opposite of > CC'ing stable. > > If the bug is old, then adding a Fixes tag does increase the chance of a > backport though, that's true. > > My guess is that if the stable maintainers thought that selftests/ were > causing too much issue with backports they would add a grep line to > their scripts to solve that problem. Instead we were having the > opposite discussion the other week where the bpf people didn't want to > backport selftest stuff and Greg wanted to. I just don't see the benefit since to my knowledge no one outside our CI systems runs selftests. And this implies mostly the current development kernel is tested. So backporting is irrelevant. Even with just the Fixes: tags the internal tooling will be picking the patches up during the -rc phase and even that can cause conflicts and some mental load to maintainers. Granted, *if* patch truly is a fix for a selftest failure which can actually happen then it is useful to pick it up for the -rc window. Although that feels extremely rare, otherwise it would have been spotted much before. In any case, I struggle to make myself interested into Fixes: tag for "impossible" selftests failures. But I can also put them in, 99% of time is not a big deal: Fixes: 50d16d44cce4 ("drm/i915/selftests: Exercise context switching in parallel") Fixes: 6407cf533217 ("drm/i915/selftests: Stop using kthread_stop()") Stable is even worse since to handle them the pointless workload is even bigger. But if stable wants everything sure, we can send everything. :) Cc: <stable@vger.kernel.org> # v5.5+ As long as it is accepted that it is unlikely no one will bother to create conflict free backports for all kernels where those don't apply. Regards, Tvrtko > https://lore.kernel.org/all/2023052647-tacking-wince-85c5@gregkh/ > > regards, > dan carpenter >
On 05/06/2023 14:47, Andi Shyti wrote: > Hi Tvrtko, > >> Add some missing error propagation in live_parallel_switch. >> >> To avoid needlessly burdening the various backport processes, note I am >> not marking it as a fix against any patches and not copying stable since >> it is debug/selftests only code. > > which I did :/ > > But I guess you are right and it's not necessary. > >> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> >> Reported-by: Dan Carpenter <dan.carpenter@linaro.org> >> Cc: Andi Shyti <andi.shyti@linux.intel.com> >> --- >> .../gpu/drm/i915/gem/selftests/i915_gem_context.c | 14 ++++++++++---- >> 1 file changed, 10 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/gem/selftests/i915_gem_context.c b/drivers/gpu/drm/i915/gem/selftests/i915_gem_context.c >> index ad6a3b2fb387..7021b6e9b219 100644 >> --- a/drivers/gpu/drm/i915/gem/selftests/i915_gem_context.c >> +++ b/drivers/gpu/drm/i915/gem/selftests/i915_gem_context.c >> @@ -348,8 +348,10 @@ static int live_parallel_switch(void *arg) >> continue; >> >> ce = intel_context_create(data[m].ce[0]->engine); >> - if (IS_ERR(ce)) >> + if (IS_ERR(ce)) { >> + err = PTR_ERR(ce); >> goto out; >> + } >> >> err = intel_context_pin(ce); >> if (err) { >> @@ -369,8 +371,10 @@ static int live_parallel_switch(void *arg) >> >> worker = kthread_create_worker(0, "igt/parallel:%s", >> data[n].ce[0]->engine->name); >> - if (IS_ERR(worker)) >> + if (IS_ERR(worker)) { >> + err = PTR_ERR(worker); >> goto out; >> + } >> >> data[n].worker = worker; >> } >> @@ -399,8 +403,10 @@ static int live_parallel_switch(void *arg) >> } >> } >> >> - if (igt_live_test_end(&t)) >> - err = -EIO; >> + if (igt_live_test_end(&t)) { >> + err = err ?: -EIO; > > Nice catch! > > Reviewed-by: Andi Shyti <andi.shyti@linux.intel.com> Thanks, pushed! (with Fixes tags added) Regards, Tvrtko
diff --git a/drivers/gpu/drm/i915/gem/selftests/i915_gem_context.c b/drivers/gpu/drm/i915/gem/selftests/i915_gem_context.c index ad6a3b2fb387..7021b6e9b219 100644 --- a/drivers/gpu/drm/i915/gem/selftests/i915_gem_context.c +++ b/drivers/gpu/drm/i915/gem/selftests/i915_gem_context.c @@ -348,8 +348,10 @@ static int live_parallel_switch(void *arg) continue; ce = intel_context_create(data[m].ce[0]->engine); - if (IS_ERR(ce)) + if (IS_ERR(ce)) { + err = PTR_ERR(ce); goto out; + } err = intel_context_pin(ce); if (err) { @@ -369,8 +371,10 @@ static int live_parallel_switch(void *arg) worker = kthread_create_worker(0, "igt/parallel:%s", data[n].ce[0]->engine->name); - if (IS_ERR(worker)) + if (IS_ERR(worker)) { + err = PTR_ERR(worker); goto out; + } data[n].worker = worker; } @@ -399,8 +403,10 @@ static int live_parallel_switch(void *arg) } } - if (igt_live_test_end(&t)) - err = -EIO; + if (igt_live_test_end(&t)) { + err = err ?: -EIO; + break; + } } out: