[PATCH] drm/arm/komeda: Add a condition check before removing sysfs attribute

[PATCH] drm/arm/komeda: Add a condition check before removing sysfs attribute

[oushixiong1025]

From: Shixiong Ou <oushixiong@kylinos.cn>

[WHY] If the call to sysfs_create_group() fails, there is
      no need to call function sysfs_remove_group().

[HOW] Add a condition check before removing sysfs attribute.

Signed-off-by: Shixiong Ou <oushixiong@kylinos.cn>
---
 drivers/gpu/drm/arm/display/komeda/komeda_dev.c | 7 ++++++-
 drivers/gpu/drm/arm/display/komeda/komeda_dev.h | 2 ++
 2 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_dev.c b/drivers/gpu/drm/arm/display/komeda/komeda_dev.c
index 5ba62e637a61..7d646f978640 100644
--- a/drivers/gpu/drm/arm/display/komeda/komeda_dev.c
+++ b/drivers/gpu/drm/arm/display/komeda/komeda_dev.c
@@ -259,6 +259,8 @@ struct komeda_dev *komeda_dev_create(struct device *dev)
 		goto err_cleanup;
 	}
 
+	mdev->sysfs_attr_enabled = true;
+
 	mdev->err_verbosity = KOMEDA_DEV_PRINT_ERR_EVENTS;
 
 	komeda_debugfs_init(mdev);
@@ -278,7 +280,10 @@ void komeda_dev_destroy(struct komeda_dev *mdev)
 	const struct komeda_dev_funcs *funcs = mdev->funcs;
 	int i;
 
-	sysfs_remove_group(&dev->kobj, &komeda_sysfs_attr_group);
+	if (mdev->sysfs_attr_enabled) {
+		sysfs_remove_group(&dev->kobj, &komeda_sysfs_attr_group);
+		mdev->sysfs_attr_enabled = false;
+	}
 
 	debugfs_remove_recursive(mdev->debugfs_root);
 
diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_dev.h b/drivers/gpu/drm/arm/display/komeda/komeda_dev.h
index 5b536f0cb548..af087540325c 100644
--- a/drivers/gpu/drm/arm/display/komeda/komeda_dev.h
+++ b/drivers/gpu/drm/arm/display/komeda/komeda_dev.h
@@ -216,6 +216,8 @@ struct komeda_dev {
 #define KOMEDA_DEV_PRINT_DUMP_STATE_ON_EVENT BIT(8)
 	/* Disable rate limiting of event prints (normally one per commit) */
 #define KOMEDA_DEV_PRINT_DISABLE_RATELIMIT BIT(12)
+
+	bool sysfs_attr_enabled;
 };
 
 static inline bool
-- 
2.17.1

Re: [PATCH] drm/arm/komeda: Add a condition check before removing sysfs attribute

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 513 bytes --]

On Thu, Feb 20, 2025 at 04:53:58PM +0800, oushixiong1025@163.com wrote:
> From: Shixiong Ou <oushixiong@kylinos.cn>
> 
> [WHY] If the call to sysfs_create_group() fails, there is
>       no need to call function sysfs_remove_group().
> 
> [HOW] Add a condition check before removing sysfs attribute.
> 
> Signed-off-by: Shixiong Ou <oushixiong@kylinos.cn>

What are you trying to fix exactly? AFAIK, there's no code path that
calls komeda_dev_destroy() after komeda_dev_create() has failed.

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 273 bytes --]

Re: [PATCH] drm/arm/komeda: Add a condition check before removing sysfs attribute

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 849 bytes --]

On Thu, Feb 20, 2025 at 05:20:48PM +0800, Shixiong Ou wrote:
> 
> 在 2025/2/20 17:12, Maxime Ripard 写道:
> > On Thu, Feb 20, 2025 at 04:53:58PM +0800,oushixiong1025@163.com  wrote:
> > > From: Shixiong Ou<oushixiong@kylinos.cn>
> > > 
> > > [WHY] If the call to sysfs_create_group() fails, there is
> > >        no need to call function sysfs_remove_group().
> > > 
> > > [HOW] Add a condition check before removing sysfs attribute.
> > > 
> > > Signed-off-by: Shixiong Ou<oushixiong@kylinos.cn>
> > What are you trying to fix exactly? AFAIK, there's no code path that
> > calls komeda_dev_destroy() after komeda_dev_create() has failed.
>
> |if calling |||sysfs_create_group() fails, it will go to lable
> 'err_cleanup:', and it will call |komeda_dev_destroy() laterly.

Ah, right. Then that's what you should fix.

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 273 bytes --]

Re: [PATCH] drm/arm/komeda: Add a condition check before removing sysfs attribute

[Liviu Dudau]

Hello,

Replying from my personal email as the corporate system seems to have blackholed your emails
while I was on holiday.

Can you tell me more why you think that if sysfs_create_group() fails we should not call
sysfs_remove_group()? After all, we don't know how far sysfs_create_group() has progressed before
it encountered an error, so we still need to do some cleanup.

Best regards,
Liviu

On Thu, Feb 20, 2025 at 04:53:58PM +0800, oushixiong1025@163.com wrote:
> From: Shixiong Ou <oushixiong@kylinos.cn>
> 
> [WHY] If the call to sysfs_create_group() fails, there is
>       no need to call function sysfs_remove_group().
> 
> [HOW] Add a condition check before removing sysfs attribute.
> 
> Signed-off-by: Shixiong Ou <oushixiong@kylinos.cn>
> ---
>  drivers/gpu/drm/arm/display/komeda/komeda_dev.c | 7 ++++++-
>  drivers/gpu/drm/arm/display/komeda/komeda_dev.h | 2 ++
>  2 files changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_dev.c b/drivers/gpu/drm/arm/display/komeda/komeda_dev.c
> index 5ba62e637a61..7d646f978640 100644
> --- a/drivers/gpu/drm/arm/display/komeda/komeda_dev.c
> +++ b/drivers/gpu/drm/arm/display/komeda/komeda_dev.c
> @@ -259,6 +259,8 @@ struct komeda_dev *komeda_dev_create(struct device *dev)
>  		goto err_cleanup;
>  	}
>  
> +	mdev->sysfs_attr_enabled = true;
> +
>  	mdev->err_verbosity = KOMEDA_DEV_PRINT_ERR_EVENTS;
>  
>  	komeda_debugfs_init(mdev);
> @@ -278,7 +280,10 @@ void komeda_dev_destroy(struct komeda_dev *mdev)
>  	const struct komeda_dev_funcs *funcs = mdev->funcs;
>  	int i;
>  
> -	sysfs_remove_group(&dev->kobj, &komeda_sysfs_attr_group);
> +	if (mdev->sysfs_attr_enabled) {
> +		sysfs_remove_group(&dev->kobj, &komeda_sysfs_attr_group);
> +		mdev->sysfs_attr_enabled = false;
> +	}
>  
>  	debugfs_remove_recursive(mdev->debugfs_root);
>  
> diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_dev.h b/drivers/gpu/drm/arm/display/komeda/komeda_dev.h
> index 5b536f0cb548..af087540325c 100644
> --- a/drivers/gpu/drm/arm/display/komeda/komeda_dev.h
> +++ b/drivers/gpu/drm/arm/display/komeda/komeda_dev.h
> @@ -216,6 +216,8 @@ struct komeda_dev {
>  #define KOMEDA_DEV_PRINT_DUMP_STATE_ON_EVENT BIT(8)
>  	/* Disable rate limiting of event prints (normally one per commit) */
>  #define KOMEDA_DEV_PRINT_DISABLE_RATELIMIT BIT(12)
> +
> +	bool sysfs_attr_enabled;
>  };
>  
>  static inline bool
> -- 
> 2.17.1
> 

-- 
Everyone who uses computers frequently has had, from time to time,
a mad desire to attack the precocious abacus with an axe.
       	   	      	     	  -- John D. Clark, Ignition!

Re: [PATCH] drm/arm/komeda: Add a condition check before removing sysfs attribute

[Liviu Dudau]

On Wed, Feb 26, 2025 at 10:52:56AM +0800, Shixiong Ou wrote:
> 
> Hello,
> 
> In my opinion, the corresponding error handling has already been implemented
> in
> sysfs_create_group(), so we do not need to call sysfs_remove_group() if
> sysfs_create_group() fails.

You are right, I stand corrected.

Acked-by: Liviu Dudau <liviu.dudau@arm.com>

Best regards,
Liviu

> 
> 
> Thanks and Regards,
> Shixiong Ou.
> 
> 
> 在 2025/2/25 18:28, Liviu Dudau 写道:
> > Hello,
> > 
> > Replying from my personal email as the corporate system seems to have blackholed your emails
> > while I was on holiday.
> > 
> > Can you tell me more why you think that if sysfs_create_group() fails we should not call
> > sysfs_remove_group()? After all, we don't know how far sysfs_create_group() has progressed before
> > it encountered an error, so we still need to do some cleanup.
> > 
> > Best regards,
> > Liviu
> > 
> > On Thu, Feb 20, 2025 at 04:53:58PM +0800,oushixiong1025@163.com  wrote:
> > > From: Shixiong Ou<oushixiong@kylinos.cn>
> > > 
> > > [WHY] If the call to sysfs_create_group() fails, there is
> > >        no need to call function sysfs_remove_group().
> > > 
> > > [HOW] Add a condition check before removing sysfs attribute.
> > > 
> > > Signed-off-by: Shixiong Ou<oushixiong@kylinos.cn>
> > > ---
> > >   drivers/gpu/drm/arm/display/komeda/komeda_dev.c | 7 ++++++-
> > >   drivers/gpu/drm/arm/display/komeda/komeda_dev.h | 2 ++
> > >   2 files changed, 8 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_dev.c b/drivers/gpu/drm/arm/display/komeda/komeda_dev.c
> > > index 5ba62e637a61..7d646f978640 100644
> > > --- a/drivers/gpu/drm/arm/display/komeda/komeda_dev.c
> > > +++ b/drivers/gpu/drm/arm/display/komeda/komeda_dev.c
> > > @@ -259,6 +259,8 @@ struct komeda_dev *komeda_dev_create(struct device *dev)
> > >   		goto err_cleanup;
> > >   	}
> > > +	mdev->sysfs_attr_enabled = true;
> > > +
> > >   	mdev->err_verbosity = KOMEDA_DEV_PRINT_ERR_EVENTS;
> > >   	komeda_debugfs_init(mdev);
> > > @@ -278,7 +280,10 @@ void komeda_dev_destroy(struct komeda_dev *mdev)
> > >   	const struct komeda_dev_funcs *funcs = mdev->funcs;
> > >   	int i;
> > > -	sysfs_remove_group(&dev->kobj, &komeda_sysfs_attr_group);
> > > +	if (mdev->sysfs_attr_enabled) {
> > > +		sysfs_remove_group(&dev->kobj, &komeda_sysfs_attr_group);
> > > +		mdev->sysfs_attr_enabled = false;
> > > +	}
> > >   	debugfs_remove_recursive(mdev->debugfs_root);
> > > diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_dev.h b/drivers/gpu/drm/arm/display/komeda/komeda_dev.h
> > > index 5b536f0cb548..af087540325c 100644
> > > --- a/drivers/gpu/drm/arm/display/komeda/komeda_dev.h
> > > +++ b/drivers/gpu/drm/arm/display/komeda/komeda_dev.h
> > > @@ -216,6 +216,8 @@ struct komeda_dev {
> > >   #define KOMEDA_DEV_PRINT_DUMP_STATE_ON_EVENT BIT(8)
> > >   	/* Disable rate limiting of event prints (normally one per commit) */
> > >   #define KOMEDA_DEV_PRINT_DISABLE_RATELIMIT BIT(12)
> > > +
> > > +	bool sysfs_attr_enabled;
> > >   };
> > >   static inline bool
> > > -- 
> > > 2.17.1
> > > 

-- 
Everyone who uses computers frequently has had, from time to time,
a mad desire to attack the precocious abacus with an axe.
       	   	      	     	  -- John D. Clark, Ignition!