Re: [RFC PATCH 2/3] drm: define new accel major and register it

[Oded Gabbay <ogabbay@kernel.org>]

On Sun, Oct 23, 2022 at 3:40 PM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> On Sun, Oct 23, 2022 at 12:46:21AM +0300, Oded Gabbay wrote:
> > The accelerator devices will be exposed to the user space with a new,
> > dedicated major number - 261.
> >
> > The drm core registers the new major number as a char device and create
> > corresponding sysfs and debugfs root entries, same as for the drm major.
> >
> > In case CONFIG_ACCEL is not selected, this code is not compiled in.
> >
> > Signed-off-by: Oded Gabbay <ogabbay@kernel.org>
> > ---
> >  Documentation/admin-guide/devices.txt |  5 +++
> >  drivers/gpu/drm/drm_drv.c             | 45 +++++++++++++++++++++++
> >  drivers/gpu/drm/drm_internal.h        |  3 ++
> >  drivers/gpu/drm/drm_sysfs.c           | 52 +++++++++++++++++++++++++++
> >  include/drm/drm_ioctl.h               |  1 +
> >  5 files changed, 106 insertions(+)
> >
> > diff --git a/Documentation/admin-guide/devices.txt b/Documentation/admin-guide/devices.txt
> > index 9764d6edb189..06c525e01ea5 100644
> > --- a/Documentation/admin-guide/devices.txt
> > +++ b/Documentation/admin-guide/devices.txt
> > @@ -3080,6 +3080,11 @@
> >                 ...
> >                 255 = /dev/osd255     256th OSD Device
> >
> > + 261 char    Compute Acceleration Devices
> > +               0 = /dev/accel/accel0 First acceleration device
> > +               1 = /dev/accel/accel1 Second acceleration device
> > +                 ...
> > +
> >   384-511 char        RESERVED FOR DYNAMIC ASSIGNMENT
> >               Character devices that request a dynamic allocation of major
> >               number will take numbers starting from 511 and downward,
> > diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> > index 8214a0b1ab7f..b58ffb1433d6 100644
> > --- a/drivers/gpu/drm/drm_drv.c
> > +++ b/drivers/gpu/drm/drm_drv.c
> > @@ -67,6 +67,10 @@ static bool drm_core_init_complete;
> >
> >  static struct dentry *drm_debugfs_root;
> >
> > +#ifdef CONFIG_ACCEL
> > +static struct dentry *accel_debugfs_root;
> > +#endif
> > +
> >  DEFINE_STATIC_SRCU(drm_unplug_srcu);
> >
> >  /*
> > @@ -1031,9 +1035,19 @@ static const struct file_operations drm_stub_fops = {
> >       .llseek = noop_llseek,
> >  };
> >
> > +static void accel_core_exit(void)
> > +{
> > +#ifdef CONFIG_ACCEL
> > +     unregister_chrdev(ACCEL_MAJOR, "accel");
> > +     debugfs_remove(accel_debugfs_root);
> > +     accel_sysfs_destroy();
> > +#endif
> > +}
>
> Why is all of this in drm_drv.c?
>
> Why not put it in drm/accel/accel.c or something like that?  Then put
> the proper stuff into a .h file and then you have no #ifdef in the .c
> files.
I thought about that, adding an accel.c in drivers/accel/ and putting
this code there.
Eventually I thought that for two functions it's not worth it, but I
guess that in addition to the reason you gave, one can argue that
there will probably be more code in that file anyway, so why not open
it now.
I'll change this if no one else thinks otherwise.
Oded

>
> Keeping #ifdef out of C files is key, please do not do things like you
> have here.  Especially as it ends up with this kind of mess:
>
> > +static int __init accel_core_init(void)
> > +{
> > +#ifdef CONFIG_ACCEL
> > +     int ret;
> > +
> > +     ret = accel_sysfs_init();
> > +     if (ret < 0) {
> > +             DRM_ERROR("Cannot create ACCEL class: %d\n", ret);
> > +             goto error;
> > +     }
> > +
> > +     accel_debugfs_root = debugfs_create_dir("accel", NULL);
> > +
> > +     ret = register_chrdev(ACCEL_MAJOR, "accel", &drm_stub_fops);
> > +     if (ret < 0)
> > +             goto error;
> > +
> > +error:
> > +     /* Any cleanup will be done in drm_core_exit() that will call
> > +      * to accel_core_exit()
> > +      */
> > +     return ret;
> > +#else
> > +     return 0;
> > +#endif
> > +}
>
>
> That's just a mess.
>
> thanks,
>
> greg k-h