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

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

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?

> > 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.

> 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.

Thanks,
Andi