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@xxxxxxxxxxxxxxx>
> > ---
> > 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
