Re: [PATCH i-g-t v3 1/6] lib/igt_sysfs: Add support for device unbinding

[Francois Dugast <francois.dugast@intel.com>]

Hi,

On Wed, Mar 12, 2025 at 10:25:15PM -0500, Lucas De Marchi wrote:
> On Thu, Mar 06, 2025 at 03:40:36PM +0530, Satyanarayana K V P wrote:
> > It does not harm anything when we try to unbind a device which
> > is already unbinded. We do not want to throw assertion when
> > the device is already unbinded.
> > 
> > Signed-off-by: Satyanarayana K V P <satyanarayana.k.v.p@intel.com>
> > Cc: Michał Wajdeczko <michal.wajdeczko@intel.com>
> > Cc: Francois Dugast <francois.dugast@intel.com>
> > Reviewed-by: Francois Dugast <francois.dugast@intel.com>
> > ---
> > lib/igt_sysfs.c | 4 ++++
> > lib/igt_sysfs.h | 1 +
> > 2 files changed, 5 insertions(+)
> > 
> > diff --git a/lib/igt_sysfs.c b/lib/igt_sysfs.c
> > index 2e4c2ee63..3fffaf96b 100644
> > --- a/lib/igt_sysfs.c
> > +++ b/lib/igt_sysfs.c
> > @@ -1509,6 +1509,10 @@ int xe_sysfs_driver_do(int xe_device, char pci_slot[], enum xe_sysfs_driver_acti
> > 		igt_assert(igt_sysfs_set(sysfs, "unbind", pci_slot));
> > 		close(sysfs);
> > 		break;
> > +	case XE_SYSFS_DRIVER_TRY_UNBIND:
> 
> oh no, this function uses the wrong design,

Yes, my bad.

> let's not make it worse.
> xe_sysfs_driver_do() should really be dropped and we should use the
> proper functions.

Those would be igt_kmod_unbind() and a new function igt_kmod_bind()
which would be allowed to fail, that is with no systematic assert
on igt_sysfs_set() as it should fail in the case of fault injection.

> 
> It's also in the wrong lib layer. igt_sysfs is for supporting sysfs
> things like opening, writing, mapping from a device to the right
> directory, setting an value, etc. We shouldn't do to do arbitrary thing
> **using** sysfs.

Another reason to completely get rid of xe_sysfs_driver_do().

> 
> Finally, when we want to unbind we shouldn't really pass an fd. The
> caller has no idea if the library function will close it or not. That
> triggers several different paths in the kernel as userspace holds or not
> an fd open.
> 
> Please do not more to this function. We already have igt_kmod_unbind(),
> we could have igt_kmod_unbind_device(). As we could have dedicated
> functions in the right layer for all the other actions here.
> 
> Lucas De Marchi