[PATCH 1/2] drm: debugfs: make drm_debugfs_remove_files() a void function

[PATCH 1/2] drm: debugfs: make drm_debugfs_remove_files() a void function

[Greg Kroah-Hartman]

The function can not fail, and no one checks the current return value,
so just mark it as a void function so no one gets confused.

Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Maxime Ripard <maxime.ripard@bootlin.com>
Cc: Sean Paul <sean@poorly.run>
Cc: David Airlie <airlied@linux.ie>
Cc: Daniel Vetter <daniel@ffwll.ch>
Cc: dri-devel@lists.freedesktop.org
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 drivers/gpu/drm/drm_debugfs.c | 5 ++---
 include/drm/drm_debugfs.h     | 9 ++++-----
 2 files changed, 6 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/drm_debugfs.c b/drivers/gpu/drm/drm_debugfs.c
index 6f2802e9bfb5..515569002c86 100644
--- a/drivers/gpu/drm/drm_debugfs.c
+++ b/drivers/gpu/drm/drm_debugfs.c
@@ -270,8 +270,8 @@ int drm_debugfs_init(struct drm_minor *minor, int minor_id,
 }
 
 
-int drm_debugfs_remove_files(const struct drm_info_list *files, int count,
-			     struct drm_minor *minor)
+void drm_debugfs_remove_files(const struct drm_info_list *files, int count,
+			      struct drm_minor *minor)
 {
 	struct list_head *pos, *q;
 	struct drm_info_node *tmp;
@@ -289,7 +289,6 @@ int drm_debugfs_remove_files(const struct drm_info_list *files, int count,
 		}
 	}
 	mutex_unlock(&minor->debugfs_lock);
-	return 0;
 }
 EXPORT_SYMBOL(drm_debugfs_remove_files);
 
diff --git a/include/drm/drm_debugfs.h b/include/drm/drm_debugfs.h
index ac0f75df1ac9..422d0d201c0a 100644
--- a/include/drm/drm_debugfs.h
+++ b/include/drm/drm_debugfs.h
@@ -81,8 +81,8 @@ struct drm_info_node {
 int drm_debugfs_create_files(const struct drm_info_list *files,
 			     int count, struct dentry *root,
 			     struct drm_minor *minor);
-int drm_debugfs_remove_files(const struct drm_info_list *files,
-			     int count, struct drm_minor *minor);
+void drm_debugfs_remove_files(const struct drm_info_list *files,
+			      int count, struct drm_minor *minor);
 #else
 static inline int drm_debugfs_create_files(const struct drm_info_list *files,
 					   int count, struct dentry *root,
@@ -91,10 +91,9 @@ static inline int drm_debugfs_create_files(const struct drm_info_list *files,
 	return 0;
 }
 
-static inline int drm_debugfs_remove_files(const struct drm_info_list *files,
-					   int count, struct drm_minor *minor)
+static inline void drm_debugfs_remove_files(const struct drm_info_list *files,
+					    int count, struct drm_minor *minor)
 {
-	return 0;
 }
 #endif
 
-- 
2.22.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 2/2] drm: debugfs: make drm_debugfs_create_files() never fail

[Greg Kroah-Hartman]

As stated before, there is no need to care if a debugfs function
succeeds or not, and no code logic in the kernel should ever change
based on a debugfs function return value, so make
drm_debugfs_create_files() never fail.  If it encounters an
odd/rare/impossible error (i.e. out of memory, or a duplicate debugfs
filename to be created), just keep on moving as if nothing improper had
happened.

Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Maxime Ripard <maxime.ripard@bootlin.com>
Cc: Sean Paul <sean@poorly.run>
Cc: David Airlie <airlied@linux.ie>
Cc: Daniel Vetter <daniel@ffwll.ch>
Cc: dri-devel@lists.freedesktop.org
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 drivers/gpu/drm/drm_debugfs.c | 26 ++++++--------------------
 1 file changed, 6 insertions(+), 20 deletions(-)

diff --git a/drivers/gpu/drm/drm_debugfs.c b/drivers/gpu/drm/drm_debugfs.c
index 515569002c86..009e1c0ac7b4 100644
--- a/drivers/gpu/drm/drm_debugfs.c
+++ b/drivers/gpu/drm/drm_debugfs.c
@@ -173,9 +173,8 @@ int drm_debugfs_create_files(const struct drm_info_list *files, int count,
 			     struct dentry *root, struct drm_minor *minor)
 {
 	struct drm_device *dev = minor->dev;
-	struct dentry *ent;
 	struct drm_info_node *tmp;
-	int i, ret;
+	int i;
 
 	for (i = 0; i < count; i++) {
 		u32 features = files[i].driver_features;
@@ -185,22 +184,13 @@ int drm_debugfs_create_files(const struct drm_info_list *files, int count,
 			continue;
 
 		tmp = kmalloc(sizeof(struct drm_info_node), GFP_KERNEL);
-		if (tmp == NULL) {
-			ret = -1;
-			goto fail;
-		}
-		ent = debugfs_create_file(files[i].name, S_IFREG | S_IRUGO,
-					  root, tmp, &drm_debugfs_fops);
-		if (!ent) {
-			DRM_ERROR("Cannot create /sys/kernel/debug/dri/%pd/%s\n",
-				  root, files[i].name);
-			kfree(tmp);
-			ret = -1;
-			goto fail;
-		}
+		if (tmp == NULL)
+			continue;
 
 		tmp->minor = minor;
-		tmp->dent = ent;
+		tmp->dent = debugfs_create_file(files[i].name,
+						S_IFREG | S_IRUGO, root, tmp,
+						&drm_debugfs_fops);
 		tmp->info_ent = &files[i];
 
 		mutex_lock(&minor->debugfs_lock);
@@ -208,10 +198,6 @@ int drm_debugfs_create_files(const struct drm_info_list *files, int count,
 		mutex_unlock(&minor->debugfs_lock);
 	}
 	return 0;
-
-fail:
-	drm_debugfs_remove_files(files, count, minor);
-	return ret;
 }
 EXPORT_SYMBOL(drm_debugfs_create_files);
 
-- 
2.22.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/2] drm: debugfs: make drm_debugfs_remove_files() a void function

[Daniel Vetter]

On Fri, Jun 14, 2019 at 11:51:09AM +0200, Greg Kroah-Hartman wrote:
> The function can not fail, and no one checks the current return value,
> so just mark it as a void function so no one gets confused.
> 
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Maxime Ripard <maxime.ripard@bootlin.com>
> Cc: Sean Paul <sean@poorly.run>
> Cc: David Airlie <airlied@linux.ie>
> Cc: Daniel Vetter <daniel@ffwll.ch>
> Cc: dri-devel@lists.freedesktop.org
> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> ---
>  drivers/gpu/drm/drm_debugfs.c | 5 ++---
>  include/drm/drm_debugfs.h     | 9 ++++-----
>  2 files changed, 6 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_debugfs.c b/drivers/gpu/drm/drm_debugfs.c
> index 6f2802e9bfb5..515569002c86 100644
> --- a/drivers/gpu/drm/drm_debugfs.c
> +++ b/drivers/gpu/drm/drm_debugfs.c
> @@ -270,8 +270,8 @@ int drm_debugfs_init(struct drm_minor *minor, int minor_id,
>  }
>  
>  
> -int drm_debugfs_remove_files(const struct drm_info_list *files, int count,
> -			     struct drm_minor *minor)
> +void drm_debugfs_remove_files(const struct drm_info_list *files, int count,
> +			      struct drm_minor *minor)

We're trying to entirely nuke this function here, see the kerneldoc for
drm_debugfs_create_files(). Only user left is tegra, and we call the
"remove all debugfs files" and the ->early_unregister hooks all from the
same place. So this can all be garbage collected. It's mildly annoying
because we'd need to move the kfree from ->early_unregister into ->destroy
callbacks, because connectors are unregister before we throw away all the
debugfs files in drm_dev_unregister(). But imo that's cleaner anway.

Up for that?

Cheers, Daniel
>  {
>  	struct list_head *pos, *q;
>  	struct drm_info_node *tmp;
> @@ -289,7 +289,6 @@ int drm_debugfs_remove_files(const struct drm_info_list *files, int count,
>  		}
>  	}
>  	mutex_unlock(&minor->debugfs_lock);
> -	return 0;
>  }
>  EXPORT_SYMBOL(drm_debugfs_remove_files);
>  
> diff --git a/include/drm/drm_debugfs.h b/include/drm/drm_debugfs.h
> index ac0f75df1ac9..422d0d201c0a 100644
> --- a/include/drm/drm_debugfs.h
> +++ b/include/drm/drm_debugfs.h
> @@ -81,8 +81,8 @@ struct drm_info_node {
>  int drm_debugfs_create_files(const struct drm_info_list *files,
>  			     int count, struct dentry *root,
>  			     struct drm_minor *minor);
> -int drm_debugfs_remove_files(const struct drm_info_list *files,
> -			     int count, struct drm_minor *minor);
> +void drm_debugfs_remove_files(const struct drm_info_list *files,
> +			      int count, struct drm_minor *minor);
>  #else
>  static inline int drm_debugfs_create_files(const struct drm_info_list *files,
>  					   int count, struct dentry *root,
> @@ -91,10 +91,9 @@ static inline int drm_debugfs_create_files(const struct drm_info_list *files,
>  	return 0;
>  }
>  
> -static inline int drm_debugfs_remove_files(const struct drm_info_list *files,
> -					   int count, struct drm_minor *minor)
> +static inline void drm_debugfs_remove_files(const struct drm_info_list *files,
> +					    int count, struct drm_minor *minor)
>  {
> -	return 0;
>  }
>  #endif
>  
> -- 
> 2.22.0
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/2] drm: debugfs: make drm_debugfs_create_files() never fail

[Daniel Vetter]

On Fri, Jun 14, 2019 at 11:51:10AM +0200, Greg Kroah-Hartman wrote:
> As stated before, there is no need to care if a debugfs function
> succeeds or not, and no code logic in the kernel should ever change
> based on a debugfs function return value, so make
> drm_debugfs_create_files() never fail.  If it encounters an
> odd/rare/impossible error (i.e. out of memory, or a duplicate debugfs
> filename to be created), just keep on moving as if nothing improper had
> happened.
> 
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Maxime Ripard <maxime.ripard@bootlin.com>
> Cc: Sean Paul <sean@poorly.run>
> Cc: David Airlie <airlied@linux.ie>
> Cc: Daniel Vetter <daniel@ffwll.ch>
> Cc: dri-devel@lists.freedesktop.org
> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

Applied this one to drm-misc-next, thanks.
-Daniel

> ---
>  drivers/gpu/drm/drm_debugfs.c | 26 ++++++--------------------
>  1 file changed, 6 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_debugfs.c b/drivers/gpu/drm/drm_debugfs.c
> index 515569002c86..009e1c0ac7b4 100644
> --- a/drivers/gpu/drm/drm_debugfs.c
> +++ b/drivers/gpu/drm/drm_debugfs.c
> @@ -173,9 +173,8 @@ int drm_debugfs_create_files(const struct drm_info_list *files, int count,
>  			     struct dentry *root, struct drm_minor *minor)
>  {
>  	struct drm_device *dev = minor->dev;
> -	struct dentry *ent;
>  	struct drm_info_node *tmp;
> -	int i, ret;
> +	int i;
>  
>  	for (i = 0; i < count; i++) {
>  		u32 features = files[i].driver_features;
> @@ -185,22 +184,13 @@ int drm_debugfs_create_files(const struct drm_info_list *files, int count,
>  			continue;
>  
>  		tmp = kmalloc(sizeof(struct drm_info_node), GFP_KERNEL);
> -		if (tmp == NULL) {
> -			ret = -1;
> -			goto fail;
> -		}
> -		ent = debugfs_create_file(files[i].name, S_IFREG | S_IRUGO,
> -					  root, tmp, &drm_debugfs_fops);
> -		if (!ent) {
> -			DRM_ERROR("Cannot create /sys/kernel/debug/dri/%pd/%s\n",
> -				  root, files[i].name);
> -			kfree(tmp);
> -			ret = -1;
> -			goto fail;
> -		}
> +		if (tmp == NULL)
> +			continue;
>  
>  		tmp->minor = minor;
> -		tmp->dent = ent;
> +		tmp->dent = debugfs_create_file(files[i].name,
> +						S_IFREG | S_IRUGO, root, tmp,
> +						&drm_debugfs_fops);
>  		tmp->info_ent = &files[i];
>  
>  		mutex_lock(&minor->debugfs_lock);
> @@ -208,10 +198,6 @@ int drm_debugfs_create_files(const struct drm_info_list *files, int count,
>  		mutex_unlock(&minor->debugfs_lock);
>  	}
>  	return 0;
> -
> -fail:
> -	drm_debugfs_remove_files(files, count, minor);
> -	return ret;
>  }
>  EXPORT_SYMBOL(drm_debugfs_create_files);
>  
> -- 
> 2.22.0
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/2] drm: debugfs: make drm_debugfs_remove_files() a void function

[Greg Kroah-Hartman]

On Fri, Jun 14, 2019 at 04:59:08PM +0200, Daniel Vetter wrote:
> On Fri, Jun 14, 2019 at 11:51:09AM +0200, Greg Kroah-Hartman wrote:
> > The function can not fail, and no one checks the current return value,
> > so just mark it as a void function so no one gets confused.
> > 
> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > Cc: Maxime Ripard <maxime.ripard@bootlin.com>
> > Cc: Sean Paul <sean@poorly.run>
> > Cc: David Airlie <airlied@linux.ie>
> > Cc: Daniel Vetter <daniel@ffwll.ch>
> > Cc: dri-devel@lists.freedesktop.org
> > Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > ---
> >  drivers/gpu/drm/drm_debugfs.c | 5 ++---
> >  include/drm/drm_debugfs.h     | 9 ++++-----
> >  2 files changed, 6 insertions(+), 8 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_debugfs.c b/drivers/gpu/drm/drm_debugfs.c
> > index 6f2802e9bfb5..515569002c86 100644
> > --- a/drivers/gpu/drm/drm_debugfs.c
> > +++ b/drivers/gpu/drm/drm_debugfs.c
> > @@ -270,8 +270,8 @@ int drm_debugfs_init(struct drm_minor *minor, int minor_id,
> >  }
> >  
> >  
> > -int drm_debugfs_remove_files(const struct drm_info_list *files, int count,
> > -			     struct drm_minor *minor)
> > +void drm_debugfs_remove_files(const struct drm_info_list *files, int count,
> > +			      struct drm_minor *minor)
> 
> We're trying to entirely nuke this function here, see the kerneldoc for
> drm_debugfs_create_files(). Only user left is tegra, and we call the
> "remove all debugfs files" and the ->early_unregister hooks all from the
> same place. So this can all be garbage collected. It's mildly annoying
> because we'd need to move the kfree from ->early_unregister into ->destroy
> callbacks, because connectors are unregister before we throw away all the
> debugfs files in drm_dev_unregister(). But imo that's cleaner anway.

I would love to see this function gone, it can also make things a lot
simpler from the point of view of creating the debugfs files as well, as
no dentries will need to be saved.

> Up for that?

Sure, I can do that.  I have a much larger patch messing with
drm_debugfs_create_files() that I want you all to be in a good mood for
when I submit it (it touches all drivers at once), so I might as well
clean this up first :)

Give me a week, I'm supposed to be writing my slides for a conference
now, instead I'm procrastinating by writing debugfs cleanup patches, I
need to stop...

thanks,

greg k-h
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/2] drm: debugfs: make drm_debugfs_remove_files() a void function

[Daniel Vetter]

On Fri, Jun 14, 2019 at 5:20 PM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> On Fri, Jun 14, 2019 at 04:59:08PM +0200, Daniel Vetter wrote:
> > On Fri, Jun 14, 2019 at 11:51:09AM +0200, Greg Kroah-Hartman wrote:
> > > The function can not fail, and no one checks the current return value,
> > > so just mark it as a void function so no one gets confused.
> > >
> > > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > > Cc: Maxime Ripard <maxime.ripard@bootlin.com>
> > > Cc: Sean Paul <sean@poorly.run>
> > > Cc: David Airlie <airlied@linux.ie>
> > > Cc: Daniel Vetter <daniel@ffwll.ch>
> > > Cc: dri-devel@lists.freedesktop.org
> > > Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > > ---
> > >  drivers/gpu/drm/drm_debugfs.c | 5 ++---
> > >  include/drm/drm_debugfs.h     | 9 ++++-----
> > >  2 files changed, 6 insertions(+), 8 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/drm_debugfs.c b/drivers/gpu/drm/drm_debugfs.c
> > > index 6f2802e9bfb5..515569002c86 100644
> > > --- a/drivers/gpu/drm/drm_debugfs.c
> > > +++ b/drivers/gpu/drm/drm_debugfs.c
> > > @@ -270,8 +270,8 @@ int drm_debugfs_init(struct drm_minor *minor, int minor_id,
> > >  }
> > >
> > >
> > > -int drm_debugfs_remove_files(const struct drm_info_list *files, int count,
> > > -                        struct drm_minor *minor)
> > > +void drm_debugfs_remove_files(const struct drm_info_list *files, int count,
> > > +                         struct drm_minor *minor)
> >
> > We're trying to entirely nuke this function here, see the kerneldoc for
> > drm_debugfs_create_files(). Only user left is tegra, and we call the
> > "remove all debugfs files" and the ->early_unregister hooks all from the
> > same place. So this can all be garbage collected. It's mildly annoying
> > because we'd need to move the kfree from ->early_unregister into ->destroy
> > callbacks, because connectors are unregister before we throw away all the
> > debugfs files in drm_dev_unregister(). But imo that's cleaner anway.
>
> I would love to see this function gone, it can also make things a lot
> simpler from the point of view of creating the debugfs files as well, as
> no dentries will need to be saved.
>
> > Up for that?
>
> Sure, I can do that.  I have a much larger patch messing with
> drm_debugfs_create_files() that I want you all to be in a good mood for
> when I submit it (it touches all drivers at once), so I might as well
> clean this up first :)

Oh don't worry, we've had a pile of cleanup todo tasks in this area
since a long time. You doing them all is going to make me a happy
camper :-)

Only thing to be aware of is that we have a bit a habit of dragging
good contributors of refactoring/cleanup/fundamental work like this
into the drm fold for good. You might get stuck ...

> Give me a week, I'm supposed to be writing my slides for a conference
> now, instead I'm procrastinating by writing debugfs cleanup patches, I
> need to stop...

:-)

Cheers, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/2] drm: debugfs: make drm_debugfs_remove_files() a void function

[Greg Kroah-Hartman]

On Fri, Jun 14, 2019 at 05:37:58PM +0200, Daniel Vetter wrote:
> On Fri, Jun 14, 2019 at 5:20 PM Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> >
> > On Fri, Jun 14, 2019 at 04:59:08PM +0200, Daniel Vetter wrote:
> > > On Fri, Jun 14, 2019 at 11:51:09AM +0200, Greg Kroah-Hartman wrote:
> > > > The function can not fail, and no one checks the current return value,
> > > > so just mark it as a void function so no one gets confused.
> > > >
> > > > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > > > Cc: Maxime Ripard <maxime.ripard@bootlin.com>
> > > > Cc: Sean Paul <sean@poorly.run>
> > > > Cc: David Airlie <airlied@linux.ie>
> > > > Cc: Daniel Vetter <daniel@ffwll.ch>
> > > > Cc: dri-devel@lists.freedesktop.org
> > > > Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > > > ---
> > > >  drivers/gpu/drm/drm_debugfs.c | 5 ++---
> > > >  include/drm/drm_debugfs.h     | 9 ++++-----
> > > >  2 files changed, 6 insertions(+), 8 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/drm_debugfs.c b/drivers/gpu/drm/drm_debugfs.c
> > > > index 6f2802e9bfb5..515569002c86 100644
> > > > --- a/drivers/gpu/drm/drm_debugfs.c
> > > > +++ b/drivers/gpu/drm/drm_debugfs.c
> > > > @@ -270,8 +270,8 @@ int drm_debugfs_init(struct drm_minor *minor, int minor_id,
> > > >  }
> > > >
> > > >
> > > > -int drm_debugfs_remove_files(const struct drm_info_list *files, int count,
> > > > -                        struct drm_minor *minor)
> > > > +void drm_debugfs_remove_files(const struct drm_info_list *files, int count,
> > > > +                         struct drm_minor *minor)
> > >
> > > We're trying to entirely nuke this function here, see the kerneldoc for
> > > drm_debugfs_create_files(). Only user left is tegra, and we call the
> > > "remove all debugfs files" and the ->early_unregister hooks all from the
> > > same place. So this can all be garbage collected. It's mildly annoying
> > > because we'd need to move the kfree from ->early_unregister into ->destroy
> > > callbacks, because connectors are unregister before we throw away all the
> > > debugfs files in drm_dev_unregister(). But imo that's cleaner anway.
> >
> > I would love to see this function gone, it can also make things a lot
> > simpler from the point of view of creating the debugfs files as well, as
> > no dentries will need to be saved.
> >
> > > Up for that?
> >
> > Sure, I can do that.  I have a much larger patch messing with
> > drm_debugfs_create_files() that I want you all to be in a good mood for
> > when I submit it (it touches all drivers at once), so I might as well
> > clean this up first :)
> 
> Oh don't worry, we've had a pile of cleanup todo tasks in this area
> since a long time. You doing them all is going to make me a happy
> camper :-)
> 
> Only thing to be aware of is that we have a bit a habit of dragging
> good contributors of refactoring/cleanup/fundamental work like this
> into the drm fold for good. You might get stuck ...

Hah...

Anyway, what tree/branch should I do this work on?  I see drm-next, is
that the one to use, but it doesn't seem to have the other patches you
all said you accepted from me for this debugfs cleanup already.

thanks,

greg k-h
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/2] drm: debugfs: make drm_debugfs_remove_files() a void function

[Daniel Vetter]

On Tue, Jun 18, 2019 at 02:01:35PM +0200, Greg Kroah-Hartman wrote:
> On Fri, Jun 14, 2019 at 05:37:58PM +0200, Daniel Vetter wrote:
> > On Fri, Jun 14, 2019 at 5:20 PM Greg Kroah-Hartman
> > <gregkh@linuxfoundation.org> wrote:
> > >
> > > On Fri, Jun 14, 2019 at 04:59:08PM +0200, Daniel Vetter wrote:
> > > > On Fri, Jun 14, 2019 at 11:51:09AM +0200, Greg Kroah-Hartman wrote:
> > > > > The function can not fail, and no one checks the current return value,
> > > > > so just mark it as a void function so no one gets confused.
> > > > >
> > > > > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > > > > Cc: Maxime Ripard <maxime.ripard@bootlin.com>
> > > > > Cc: Sean Paul <sean@poorly.run>
> > > > > Cc: David Airlie <airlied@linux.ie>
> > > > > Cc: Daniel Vetter <daniel@ffwll.ch>
> > > > > Cc: dri-devel@lists.freedesktop.org
> > > > > Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > > > > ---
> > > > >  drivers/gpu/drm/drm_debugfs.c | 5 ++---
> > > > >  include/drm/drm_debugfs.h     | 9 ++++-----
> > > > >  2 files changed, 6 insertions(+), 8 deletions(-)
> > > > >
> > > > > diff --git a/drivers/gpu/drm/drm_debugfs.c b/drivers/gpu/drm/drm_debugfs.c
> > > > > index 6f2802e9bfb5..515569002c86 100644
> > > > > --- a/drivers/gpu/drm/drm_debugfs.c
> > > > > +++ b/drivers/gpu/drm/drm_debugfs.c
> > > > > @@ -270,8 +270,8 @@ int drm_debugfs_init(struct drm_minor *minor, int minor_id,
> > > > >  }
> > > > >
> > > > >
> > > > > -int drm_debugfs_remove_files(const struct drm_info_list *files, int count,
> > > > > -                        struct drm_minor *minor)
> > > > > +void drm_debugfs_remove_files(const struct drm_info_list *files, int count,
> > > > > +                         struct drm_minor *minor)
> > > >
> > > > We're trying to entirely nuke this function here, see the kerneldoc for
> > > > drm_debugfs_create_files(). Only user left is tegra, and we call the
> > > > "remove all debugfs files" and the ->early_unregister hooks all from the
> > > > same place. So this can all be garbage collected. It's mildly annoying
> > > > because we'd need to move the kfree from ->early_unregister into ->destroy
> > > > callbacks, because connectors are unregister before we throw away all the
> > > > debugfs files in drm_dev_unregister(). But imo that's cleaner anway.
> > >
> > > I would love to see this function gone, it can also make things a lot
> > > simpler from the point of view of creating the debugfs files as well, as
> > > no dentries will need to be saved.
> > >
> > > > Up for that?
> > >
> > > Sure, I can do that.  I have a much larger patch messing with
> > > drm_debugfs_create_files() that I want you all to be in a good mood for
> > > when I submit it (it touches all drivers at once), so I might as well
> > > clean this up first :)
> > 
> > Oh don't worry, we've had a pile of cleanup todo tasks in this area
> > since a long time. You doing them all is going to make me a happy
> > camper :-)
> > 
> > Only thing to be aware of is that we have a bit a habit of dragging
> > good contributors of refactoring/cleanup/fundamental work like this
> > into the drm fold for good. You might get stuck ...
> 
> Hah...
> 
> Anyway, what tree/branch should I do this work on?  I see drm-next, is
> that the one to use, but it doesn't seem to have the other patches you
> all said you accepted from me for this debugfs cleanup already.

linux-next is usually a good starting point, drm stuff is a bit too much
spread around multiple trees. The only caveat is that some trees (drm-misc
and drm-intel) keep merging during the feature freeze (after -rc6) and
merge window, to collect patches for the merge-window+1. In those cases it
might be better to rebase on top of drm-tip:

https://cgit.freedesktop.org/drm-tip

It's kinda like linux-next, but for drm. Only downside is that not all drm
trees participate in that integration tree.

Simpelst is probably to just base on linux-next and dump everything that
hasn't landed after -rc2 onto dri-devel again.

Cheers, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/2] drm: debugfs: make drm_debugfs_remove_files() a void function

[Greg Kroah-Hartman]

On Tue, Jun 18, 2019 at 02:17:11PM +0200, Daniel Vetter wrote:
> On Tue, Jun 18, 2019 at 02:01:35PM +0200, Greg Kroah-Hartman wrote:
> > On Fri, Jun 14, 2019 at 05:37:58PM +0200, Daniel Vetter wrote:
> > > On Fri, Jun 14, 2019 at 5:20 PM Greg Kroah-Hartman
> > > <gregkh@linuxfoundation.org> wrote:
> > > >
> > > > On Fri, Jun 14, 2019 at 04:59:08PM +0200, Daniel Vetter wrote:
> > > > > On Fri, Jun 14, 2019 at 11:51:09AM +0200, Greg Kroah-Hartman wrote:
> > > > > > The function can not fail, and no one checks the current return value,
> > > > > > so just mark it as a void function so no one gets confused.
> > > > > >
> > > > > > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > > > > > Cc: Maxime Ripard <maxime.ripard@bootlin.com>
> > > > > > Cc: Sean Paul <sean@poorly.run>
> > > > > > Cc: David Airlie <airlied@linux.ie>
> > > > > > Cc: Daniel Vetter <daniel@ffwll.ch>
> > > > > > Cc: dri-devel@lists.freedesktop.org
> > > > > > Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > > > > > ---
> > > > > >  drivers/gpu/drm/drm_debugfs.c | 5 ++---
> > > > > >  include/drm/drm_debugfs.h     | 9 ++++-----
> > > > > >  2 files changed, 6 insertions(+), 8 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/gpu/drm/drm_debugfs.c b/drivers/gpu/drm/drm_debugfs.c
> > > > > > index 6f2802e9bfb5..515569002c86 100644
> > > > > > --- a/drivers/gpu/drm/drm_debugfs.c
> > > > > > +++ b/drivers/gpu/drm/drm_debugfs.c
> > > > > > @@ -270,8 +270,8 @@ int drm_debugfs_init(struct drm_minor *minor, int minor_id,
> > > > > >  }
> > > > > >
> > > > > >
> > > > > > -int drm_debugfs_remove_files(const struct drm_info_list *files, int count,
> > > > > > -                        struct drm_minor *minor)
> > > > > > +void drm_debugfs_remove_files(const struct drm_info_list *files, int count,
> > > > > > +                         struct drm_minor *minor)
> > > > >
> > > > > We're trying to entirely nuke this function here, see the kerneldoc for
> > > > > drm_debugfs_create_files(). Only user left is tegra, and we call the
> > > > > "remove all debugfs files" and the ->early_unregister hooks all from the
> > > > > same place. So this can all be garbage collected. It's mildly annoying
> > > > > because we'd need to move the kfree from ->early_unregister into ->destroy
> > > > > callbacks, because connectors are unregister before we throw away all the
> > > > > debugfs files in drm_dev_unregister(). But imo that's cleaner anway.
> > > >
> > > > I would love to see this function gone, it can also make things a lot
> > > > simpler from the point of view of creating the debugfs files as well, as
> > > > no dentries will need to be saved.
> > > >
> > > > > Up for that?
> > > >
> > > > Sure, I can do that.  I have a much larger patch messing with
> > > > drm_debugfs_create_files() that I want you all to be in a good mood for
> > > > when I submit it (it touches all drivers at once), so I might as well
> > > > clean this up first :)
> > > 
> > > Oh don't worry, we've had a pile of cleanup todo tasks in this area
> > > since a long time. You doing them all is going to make me a happy
> > > camper :-)
> > > 
> > > Only thing to be aware of is that we have a bit a habit of dragging
> > > good contributors of refactoring/cleanup/fundamental work like this
> > > into the drm fold for good. You might get stuck ...
> > 
> > Hah...
> > 
> > Anyway, what tree/branch should I do this work on?  I see drm-next, is
> > that the one to use, but it doesn't seem to have the other patches you
> > all said you accepted from me for this debugfs cleanup already.
> 
> linux-next is usually a good starting point, drm stuff is a bit too much
> spread around multiple trees. The only caveat is that some trees (drm-misc
> and drm-intel) keep merging during the feature freeze (after -rc6) and
> merge window, to collect patches for the merge-window+1. In those cases it
> might be better to rebase on top of drm-tip:
> 
> https://cgit.freedesktop.org/drm-tip
> 
> It's kinda like linux-next, but for drm. Only downside is that not all drm
> trees participate in that integration tree.
> 
> Simpelst is probably to just base on linux-next and dump everything that
> hasn't landed after -rc2 onto dri-devel again.

Thanks, I'll work off of that...

And what a tangled web we weave of debugfs files and pointers, it's
worse than sysfs here.  I'll send another email with why this work is so
hard to do...

thanks,

greg k-h
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel