Re: [PATCH] drm/i915/gt: Continue creating engine sysfs files even after a failure

[Andi Shyti <andi.shyti@linux.intel.com>]

Hi Rodrigo,

On Fri, Aug 23, 2024 at 09:41:31AM -0400, Rodrigo Vivi wrote:
> On Wed, Aug 21, 2024 at 09:32:48AM +0200, Andi Shyti wrote:
> > On Tue, Aug 20, 2024 at 05:22:40PM -0400, Rodrigo Vivi wrote:
> > > On Mon, Aug 19, 2024 at 01:31:40PM +0200, Andi Shyti wrote:
...
> > > > It might make sense to create an "inv-<engine_name>" if something
> > > > goes wrong, so that the user is aware that the engine exists, but
> > > > the sysfs file is not present.
> > > 
> > > well, if the sysfs dir/files creation is failing, then it will
> > > probably be unreliable anyway right?
> > 
> > Are you suggesting that "inv-<engine_name>" is OK?
> 
> it is okay I guess.
> But my point is more on, how are we going to create this if
> the creation mechanism is what is likely failing here.

We can fail for different reasons... but yeah you are right, it
doesn't make much sense, as also the creation of "inv-<...>"
interfaces might be unreliable.

> > > Also it looks something is off with the goto paths...
> > > 
> > > That if (0) is also ugly... probably better to use a
> > > kobject_put with continue on every failing point as well...
> > 
> > ehehe... I came to like it, to be honest. Besides I like single
> > exit paths instead of distributed returns. In this particular
> > case we would replcate the same "kobject_put() ... dev_warn()" in
> > several places, so that I'm not sure it's better.
> > 
> > If you like more we could do:
> > 
> > 	for (...) {
> > 		...
> > 		...
> > 		/* everything goes fine */
> > 		continue
> > 
> > err_engine:
> > 		kobject_put(...);
> > 		dev_warn(...);
> > 	}
> > 
> > And we avoid using the "if (0)" that you don't like.
> 
> nah, no strong feeling from my side. It is there, let's
> avoid unnecessary refactors.
> 
> Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> 
> on this patch as is. And sorry for the delay.

Thanks a lot for your review :-)

Andi