| Message ID | 20190614095110.3716-1-gregkh@linuxfoundation.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] drm: debugfs: make drm_debugfs_remove_files() a void function | expand |
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 >
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
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
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
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
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
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
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(-)