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

[Rodrigo Vivi <rodrigo.vivi@intel.com>]

On Wed, Aug 21, 2024 at 09:32:48AM +0200, Andi Shyti wrote:
> Hi Rodrigo,
> 
> 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:
> > > The i915 driver generates sysfs entries for each engine of the
> > > GPU in /sys/class/drm/cardX/engines/.
> > > 
> > > The process is straightforward: we loop over the UABI engines and
> > > for each one, we:
> > > 
> > >  - Create the object.
> > >  - Create basic files.
> > >  - If the engine supports timeslicing, create timeslice duration files.
> > >  - If the engine supports preemption, create preemption-related files.
> > >  - Create default value files.
> > > 
> > > Currently, if any of these steps fail, the process stops, and no
> > > further sysfs files are created.
> > > 
> > > However, it's not necessary to stop the process on failure.
> > > Instead, we can continue creating the remaining sysfs files for
> > > the other engines. Even if some files fail to be created, the
> > > list of engines can still be retrieved by querying i915.
> > > 
> > > Signed-off-by: Andi Shyti <andi.shyti@linux.intel.com>
> > > ---
> > > Hi,
> > > 
> > > 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.

> 
> > > One further improvement would be to provide more information
> > > about thei failure reason the dev_warn() message.
> > 
> > So, perhaps this patch should already go there and remove
> > the dev_err and add individual dev_warn for each failing path?
> 
> That's a suggestion, but it doesn't mean that it necessarily
> improves things as it might add some innecessary information.
> Just thinking.

okay then.

> 
> > 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,
> Andi